Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, Feb 04, 2014 at 07:59:21PM +0100, Jean-Francois Moine wrote: > Mark Brown wrote: > > You shouldn't be representing this as a separate node in the DT unless > > there really is a distinct and reusable IP, otherwise you're putting > > Linux implementation details in there. Describe the hardware, not the > > implemementation. > If there is no 'compatible' node for the tda998x CODEC in the DT, the > simple-card is not usable, simply because you want the CODEC DAIs to be > defined by 'phandle + index' instead of by DAI name. This is a bit circular, though - it's only happening because you decided to push everything onto a subnode in the DT. If you just work with the existing device this is no different to any other device. > > > I don't understand. The tda CODEC can only be used with the TDA998x I2C > > > driver. It might have been included in the tda998x source as well. > > You shouldn't have the default settings there at all, that's not the > > normal idiom for MFDs. I'd also not expect to have to build the CODEC > > driver just because I built the DRM component. > As the tda998x handles audio in HDMI, it would be a pity if you should > connect an other cable to your screen. My screen doesn't have any speakers anyway :P (I'm writing this on a computer with the monitor connected via HDMI). Besides, this is more about build coverage stuff than anything else. > So, as I understand from your remarks, the CODEC should be included in > the tda998x driver, and, then, as the simple-card cannot be used, there > should be a Cubox specific audio card driver for the (kirkwood audio + > tda998x HDMI + S/PDIF) set. Am I right? No, it shouldn't be. signature.asc Description: Digital signature
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, 4 Feb 2014 17:54:10 + Mark Brown wrote: > On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote: > > Mark Brown wrote: > > > > > + /* load the optional CODEC */ > > > > + of_platform_populate(np, NULL, NULL, >dev); > > > > Why is this using of_platform_populate()? That's a very odd way of > > > doing things. > > > The i2c does not populate the subnodes in the DT. I did not find why, > > but, what is sure is that if of_platform_populate() is not called, the > > tda CODEC module is not loaded. > > You shouldn't be representing this as a separate node in the DT unless > there really is a distinct and reusable IP, otherwise you're putting > Linux implementation details in there. Describe the hardware, not the > implemementation. If there is no 'compatible' node for the tda998x CODEC in the DT, the simple-card is not usable, simply because you want the CODEC DAIs to be defined by 'phandle + index' instead of by DAI name. > > You may find an other example in drivers/mfd/twl-core.c. > > The TWL drivers aren't always a shining example of how to do things - > they were one of the earliest MFDs so there's warts in there. > > > > > +config SND_SOC_TDA998X > > > > + tristate > > > > + depends on OF > > > > + default y if DRM_I2C_NXP_TDA998X=y > > > > + default m if DRM_I2C_NXP_TDA998X=m > > > > Make this visible if it can be selected from DT so it can be used with > > > generic cards. > > > I don't understand. The tda CODEC can only be used with the TDA998x I2C > > driver. It might have been included in the tda998x source as well. > > You shouldn't have the default settings there at all, that's not the > normal idiom for MFDs. I'd also not expect to have to build the CODEC > driver just because I built the DRM component. As the tda998x handles audio in HDMI, it would be a pity if you should connect an other cable to your screen. > > Now, the CODEC is declared inside the tda998x as a node child. But, in > > a bad DT, the tda CODEC could be declared anywhere, even inside a other > > DRM I2C slave encoder, in which case, bad things would happen... > > So, part of the problem here is that this is being explicitly declared > in the DT leading to more sources for error. Simple-card constraint. > > > What does this actually do? No information is being passed in to the > > > core function here, not even any information on if it's starting or > > > stopping. Looking at the rest of the code I can't help thinking it > > > might be clearer to inline this possibly with a lookup helper, the code > > > is very small and the lack of parameters makes it hard to follow. > > > I thought it was simple enough. The function tda_start_stop() is called > > from 2 places: > > It's not at all obvious - _audio_update() isn't a terribly descriptive > name, just looking at that function by itself I had no idea what it was > supposed to be doing. The first purpose of the function is to set the audio input port in the tda998x. Streaming stop could have been omitted, but I thought it could be interesting to stop HDMI audio when there is a super HiFi device connected to the S/PDIF connector. > > - on audio start in tda_startup with the audio type (DAI id) > > priv->dai_id = dai->id; > > > - on audio stop with a null audio type > > priv->dai_id = 0; /* streaming stop */ > > > On stream start, the DAI id is never null, as explained in the patch 1: > > > The audio format values in the encoder configuration interface are > > changed to non null values so that the value 0 is used in the audio > > function to indicate that audio streaming is stopped. > > > and on streaming stop the port is not meaningful. > > > I will add a null item in the enum (AFMT_NO_AUDIO). > > So we can't use both streams simultaneously then? That's a bit sad. That's how the NXP tda998x family works (and surely many other HDMI transmitters). So, as I understand from your remarks, the CODEC should be included in the tda998x driver, and, then, as the simple-card cannot be used, there should be a Cubox specific audio card driver for the (kirkwood audio + tda998x HDMI + S/PDIF) set. Am I right? -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote: > Mark Brown wrote: > > > + /* load the optional CODEC */ > > > + of_platform_populate(np, NULL, NULL, >dev); > > Why is this using of_platform_populate()? That's a very odd way of > > doing things. > The i2c does not populate the subnodes in the DT. I did not find why, > but, what is sure is that if of_platform_populate() is not called, the > tda CODEC module is not loaded. You shouldn't be representing this as a separate node in the DT unless there really is a distinct and reusable IP, otherwise you're putting Linux implementation details in there. Describe the hardware, not the implemementation. > You may find an other example in drivers/mfd/twl-core.c. The TWL drivers aren't always a shining example of how to do things - they were one of the earliest MFDs so there's warts in there. > > > +config SND_SOC_TDA998X > > > + tristate > > > + depends on OF > > > + default y if DRM_I2C_NXP_TDA998X=y > > > + default m if DRM_I2C_NXP_TDA998X=m > > Make this visible if it can be selected from DT so it can be used with > > generic cards. > I don't understand. The tda CODEC can only be used with the TDA998x I2C > driver. It might have been included in the tda998x source as well. You shouldn't have the default settings there at all, that's not the normal idiom for MFDs. I'd also not expect to have to build the CODEC driver just because I built the DRM component. > Now, the CODEC is declared inside the tda998x as a node child. But, in > a bad DT, the tda CODEC could be declared anywhere, even inside a other > DRM I2C slave encoder, in which case, bad things would happen... So, part of the problem here is that this is being explicitly declared in the DT leading to more sources for error. > > What does this actually do? No information is being passed in to the > > core function here, not even any information on if it's starting or > > stopping. Looking at the rest of the code I can't help thinking it > > might be clearer to inline this possibly with a lookup helper, the code > > is very small and the lack of parameters makes it hard to follow. > I thought it was simple enough. The function tda_start_stop() is called > from 2 places: It's not at all obvious - _audio_update() isn't a terribly descriptive name, just looking at that function by itself I had no idea what it was supposed to be doing. > - on audio start in tda_startup with the audio type (DAI id) > priv->dai_id = dai->id; > - on audio stop with a null audio type > priv->dai_id = 0; /* streaming stop */ > On stream start, the DAI id is never null, as explained in the patch 1: > The audio format values in the encoder configuration interface are > changed to non null values so that the value 0 is used in the audio > function to indicate that audio streaming is stopped. > and on streaming stop the port is not meaningful. > I will add a null item in the enum (AFMT_NO_AUDIO). So we can't use both streams simultaneously then? That's a bit sad. signature.asc Description: Digital signature
Re: [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, Feb 04, 2014 at 02:36:50PM +0100, Lars-Peter Clausen wrote:
> On 02/04/2014 02:30 PM, Mark Brown wrote:
> >>+static const struct snd_soc_dapm_route tda_routes[] = {
> >>+ { "hdmi-out", NULL, "HDMI I2S Playback" },
> >>+ { "hdmi-out", NULL, "HDMI SPDIF Playback" },
> >>+};
> >S/PDIF.
> Won't this cause issues with the debugfs widget entries? It's
> fixable by escaping it (replace it by a dash or something) in the
> debugfs widget filename, but I don't think we do this right now.
Oh, bother, so it will.
signature.asc
Description: Digital signature
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, 4 Feb 2014 13:30:14 +
Mark Brown wrote:
> On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote:
>
> > + /* load the optional CODEC */
> > + of_platform_populate(np, NULL, NULL, >dev);
> > +
>
> Why is this using of_platform_populate()? That's a very odd way of
> doing things.
The i2c does not populate the subnodes in the DT. I did not find why,
but, what is sure is that if of_platform_populate() is not called, the
tda CODEC module is not loaded.
You may find an other example in drivers/mfd/twl-core.c.
> > +config SND_SOC_TDA998X
> > + tristate
> > + depends on OF
> > + default y if DRM_I2C_NXP_TDA998X=y
> > + default m if DRM_I2C_NXP_TDA998X=m
> > +
>
> Make this visible if it can be selected from DT so it can be used with
> generic cards.
I don't understand. The tda CODEC can only be used with the TDA998x I2C
driver. It might have been included in the tda998x source as well.
> > +static int tda_get_encoder(struct tda_priv *priv)
> > +{
> > + struct snd_soc_codec *codec = priv->codec;
> > + struct device_node *np;
> > +
> > + /* get the parent tda998x device */
> > + np = of_get_parent(codec->dev->of_node);
> > + if (!np || !of_device_is_compatible(np, "nxp,tda998x")) {
> > + dev_err(codec->dev, "no or bad parent!\n");
> > + return -EINVAL;
> > + }
> > + priv->i2c_client = of_find_i2c_device_by_node(np);
> > + of_node_put(np);
> > + return 0;
> > +}
>
> Why does this need to be checked like this? We don't normally have this
> sort of code to check that the parent is correct.
In my previous submit, the tda CODEC was not declared inside the
tda998x I2c device, so, its location was searched from phandle.
Now, the CODEC is declared inside the tda998x as a node child. But, in
a bad DT, the tda CODEC could be declared anywhere, even inside a other
DRM I2C slave encoder, in which case, bad things would happen...
> > +static int tda_start_stop(struct tda_priv *priv)
> > +{
> > + int port;
> > +
> > + /* give the audio parameters to the HDMI encoder */
> > + if (priv->dai_id == AFMT_I2S)
> > + port = priv->ports[0];
> > + else
> > + port = priv->ports[1];
> > + tda998x_audio_update(priv->i2c_client, priv->dai_id, port);
> > + return 0;
> > +}
>
> What does this actually do? No information is being passed in to the
> core function here, not even any information on if it's starting or
> stopping. Looking at the rest of the code I can't help thinking it
> might be clearer to inline this possibly with a lookup helper, the code
> is very small and the lack of parameters makes it hard to follow.
I thought it was simple enough. The function tda_start_stop() is called
from 2 places:
- on audio start in tda_startup with the audio type (DAI id)
priv->dai_id = dai->id;
- on audio stop with a null audio type
priv->dai_id = 0; /* streaming stop */
On stream start, the DAI id is never null, as explained in the patch 1:
The audio format values in the encoder configuration interface are
changed to non null values so that the value 0 is used in the audio
function to indicate that audio streaming is stopped.
and on streaming stop the port is not meaningful.
I will add a null item in the enum (AFMT_NO_AUDIO).
> > +static const struct snd_soc_dapm_route tda_routes[] = {
> > + { "hdmi-out", NULL, "HDMI I2S Playback" },
> > + { "hdmi-out", NULL, "HDMI SPDIF Playback" },
> > +};
>
> S/PDIF.
Did you ever try that with debugfs?
BTW, this patch series may be delayed for some time: the tda998x driver
has to be reworked for DT support.
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On 02/04/2014 02:30 PM, Mark Brown wrote:
[...]
What does this actually do? No information is being passed in to the
core function here, not even any information on if it's starting or
stopping. Looking at the rest of the code I can't help thinking it
might be clearer to inline this possibly with a lookup helper, the code
is very small and the lack of parameters makes it hard to follow.
+static const struct snd_soc_dapm_route tda_routes[] = {
+ { "hdmi-out", NULL, "HDMI I2S Playback" },
+ { "hdmi-out", NULL, "HDMI SPDIF Playback" },
+};
S/PDIF.
Won't this cause issues with the debugfs widget entries? It's fixable by
escaping it (replace it by a dash or something) in the debugfs widget
filename, but I don't think we do this right now.
- Lars
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote:
> + /* load the optional CODEC */
> + of_platform_populate(np, NULL, NULL, >dev);
> +
Why is this using of_platform_populate()? That's a very odd way of
doing things.
> +config SND_SOC_TDA998X
> + tristate
> + depends on OF
> + default y if DRM_I2C_NXP_TDA998X=y
> + default m if DRM_I2C_NXP_TDA998X=m
> +
Make this visible if it can be selected from DT so it can be used with
generic cards.
> +static int tda_get_encoder(struct tda_priv *priv)
> +{
> + struct snd_soc_codec *codec = priv->codec;
> + struct device_node *np;
> +
> + /* get the parent tda998x device */
> + np = of_get_parent(codec->dev->of_node);
> + if (!np || !of_device_is_compatible(np, "nxp,tda998x")) {
> + dev_err(codec->dev, "no or bad parent!\n");
> + return -EINVAL;
> + }
> + priv->i2c_client = of_find_i2c_device_by_node(np);
> + of_node_put(np);
> + return 0;
> +}
Why does this need to be checked like this? We don't normally have this
sort of code to check that the parent is correct.
> +static int tda_start_stop(struct tda_priv *priv)
> +{
> + int port;
> +
> + /* give the audio parameters to the HDMI encoder */
> + if (priv->dai_id == AFMT_I2S)
> + port = priv->ports[0];
> + else
> + port = priv->ports[1];
> + tda998x_audio_update(priv->i2c_client, priv->dai_id, port);
> + return 0;
> +}
What does this actually do? No information is being passed in to the
core function here, not even any information on if it's starting or
stopping. Looking at the rest of the code I can't help thinking it
might be clearer to inline this possibly with a lookup helper, the code
is very small and the lack of parameters makes it hard to follow.
> +static const struct snd_soc_dapm_route tda_routes[] = {
> + { "hdmi-out", NULL, "HDMI I2S Playback" },
> + { "hdmi-out", NULL, "HDMI SPDIF Playback" },
> +};
S/PDIF.
signature.asc
Description: Digital signature
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote:
+ /* load the optional CODEC */
+ of_platform_populate(np, NULL, NULL, client-dev);
+
Why is this using of_platform_populate()? That's a very odd way of
doing things.
+config SND_SOC_TDA998X
+ tristate
+ depends on OF
+ default y if DRM_I2C_NXP_TDA998X=y
+ default m if DRM_I2C_NXP_TDA998X=m
+
Make this visible if it can be selected from DT so it can be used with
generic cards.
+static int tda_get_encoder(struct tda_priv *priv)
+{
+ struct snd_soc_codec *codec = priv-codec;
+ struct device_node *np;
+
+ /* get the parent tda998x device */
+ np = of_get_parent(codec-dev-of_node);
+ if (!np || !of_device_is_compatible(np, nxp,tda998x)) {
+ dev_err(codec-dev, no or bad parent!\n);
+ return -EINVAL;
+ }
+ priv-i2c_client = of_find_i2c_device_by_node(np);
+ of_node_put(np);
+ return 0;
+}
Why does this need to be checked like this? We don't normally have this
sort of code to check that the parent is correct.
+static int tda_start_stop(struct tda_priv *priv)
+{
+ int port;
+
+ /* give the audio parameters to the HDMI encoder */
+ if (priv-dai_id == AFMT_I2S)
+ port = priv-ports[0];
+ else
+ port = priv-ports[1];
+ tda998x_audio_update(priv-i2c_client, priv-dai_id, port);
+ return 0;
+}
What does this actually do? No information is being passed in to the
core function here, not even any information on if it's starting or
stopping. Looking at the rest of the code I can't help thinking it
might be clearer to inline this possibly with a lookup helper, the code
is very small and the lack of parameters makes it hard to follow.
+static const struct snd_soc_dapm_route tda_routes[] = {
+ { hdmi-out, NULL, HDMI I2S Playback },
+ { hdmi-out, NULL, HDMI SPDIF Playback },
+};
S/PDIF.
signature.asc
Description: Digital signature
Re: [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On 02/04/2014 02:30 PM, Mark Brown wrote:
[...]
What does this actually do? No information is being passed in to the
core function here, not even any information on if it's starting or
stopping. Looking at the rest of the code I can't help thinking it
might be clearer to inline this possibly with a lookup helper, the code
is very small and the lack of parameters makes it hard to follow.
+static const struct snd_soc_dapm_route tda_routes[] = {
+ { hdmi-out, NULL, HDMI I2S Playback },
+ { hdmi-out, NULL, HDMI SPDIF Playback },
+};
S/PDIF.
Won't this cause issues with the debugfs widget entries? It's fixable by
escaping it (replace it by a dash or something) in the debugfs widget
filename, but I don't think we do this right now.
- Lars
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, 4 Feb 2014 13:30:14 +
Mark Brown broo...@kernel.org wrote:
On Sun, Jan 26, 2014 at 07:45:36PM +0100, Jean-Francois Moine wrote:
+ /* load the optional CODEC */
+ of_platform_populate(np, NULL, NULL, client-dev);
+
Why is this using of_platform_populate()? That's a very odd way of
doing things.
The i2c does not populate the subnodes in the DT. I did not find why,
but, what is sure is that if of_platform_populate() is not called, the
tda CODEC module is not loaded.
You may find an other example in drivers/mfd/twl-core.c.
+config SND_SOC_TDA998X
+ tristate
+ depends on OF
+ default y if DRM_I2C_NXP_TDA998X=y
+ default m if DRM_I2C_NXP_TDA998X=m
+
Make this visible if it can be selected from DT so it can be used with
generic cards.
I don't understand. The tda CODEC can only be used with the TDA998x I2C
driver. It might have been included in the tda998x source as well.
+static int tda_get_encoder(struct tda_priv *priv)
+{
+ struct snd_soc_codec *codec = priv-codec;
+ struct device_node *np;
+
+ /* get the parent tda998x device */
+ np = of_get_parent(codec-dev-of_node);
+ if (!np || !of_device_is_compatible(np, nxp,tda998x)) {
+ dev_err(codec-dev, no or bad parent!\n);
+ return -EINVAL;
+ }
+ priv-i2c_client = of_find_i2c_device_by_node(np);
+ of_node_put(np);
+ return 0;
+}
Why does this need to be checked like this? We don't normally have this
sort of code to check that the parent is correct.
In my previous submit, the tda CODEC was not declared inside the
tda998x I2c device, so, its location was searched from phandle.
Now, the CODEC is declared inside the tda998x as a node child. But, in
a bad DT, the tda CODEC could be declared anywhere, even inside a other
DRM I2C slave encoder, in which case, bad things would happen...
+static int tda_start_stop(struct tda_priv *priv)
+{
+ int port;
+
+ /* give the audio parameters to the HDMI encoder */
+ if (priv-dai_id == AFMT_I2S)
+ port = priv-ports[0];
+ else
+ port = priv-ports[1];
+ tda998x_audio_update(priv-i2c_client, priv-dai_id, port);
+ return 0;
+}
What does this actually do? No information is being passed in to the
core function here, not even any information on if it's starting or
stopping. Looking at the rest of the code I can't help thinking it
might be clearer to inline this possibly with a lookup helper, the code
is very small and the lack of parameters makes it hard to follow.
I thought it was simple enough. The function tda_start_stop() is called
from 2 places:
- on audio start in tda_startup with the audio type (DAI id)
priv-dai_id = dai-id;
- on audio stop with a null audio type
priv-dai_id = 0; /* streaming stop */
On stream start, the DAI id is never null, as explained in the patch 1:
The audio format values in the encoder configuration interface are
changed to non null values so that the value 0 is used in the audio
function to indicate that audio streaming is stopped.
and on streaming stop the port is not meaningful.
I will add a null item in the enum (AFMT_NO_AUDIO).
+static const struct snd_soc_dapm_route tda_routes[] = {
+ { hdmi-out, NULL, HDMI I2S Playback },
+ { hdmi-out, NULL, HDMI SPDIF Playback },
+};
S/PDIF.
Did you ever try that with debugfs?
BTW, this patch series may be delayed for some time: the tda998x driver
has to be reworked for DT support.
--
Ken ar c'hentañ | ** Breizh ha Linux atav! **
Jef | http://moinejf.free.fr/
--
To unsubscribe from this list: send the line unsubscribe linux-kernel in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Re: [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, Feb 04, 2014 at 02:36:50PM +0100, Lars-Peter Clausen wrote:
On 02/04/2014 02:30 PM, Mark Brown wrote:
+static const struct snd_soc_dapm_route tda_routes[] = {
+ { hdmi-out, NULL, HDMI I2S Playback },
+ { hdmi-out, NULL, HDMI SPDIF Playback },
+};
S/PDIF.
Won't this cause issues with the debugfs widget entries? It's
fixable by escaping it (replace it by a dash or something) in the
debugfs widget filename, but I don't think we do this right now.
Oh, bother, so it will.
signature.asc
Description: Digital signature
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote: Mark Brown broo...@kernel.org wrote: + /* load the optional CODEC */ + of_platform_populate(np, NULL, NULL, client-dev); Why is this using of_platform_populate()? That's a very odd way of doing things. The i2c does not populate the subnodes in the DT. I did not find why, but, what is sure is that if of_platform_populate() is not called, the tda CODEC module is not loaded. You shouldn't be representing this as a separate node in the DT unless there really is a distinct and reusable IP, otherwise you're putting Linux implementation details in there. Describe the hardware, not the implemementation. You may find an other example in drivers/mfd/twl-core.c. The TWL drivers aren't always a shining example of how to do things - they were one of the earliest MFDs so there's warts in there. +config SND_SOC_TDA998X + tristate + depends on OF + default y if DRM_I2C_NXP_TDA998X=y + default m if DRM_I2C_NXP_TDA998X=m Make this visible if it can be selected from DT so it can be used with generic cards. I don't understand. The tda CODEC can only be used with the TDA998x I2C driver. It might have been included in the tda998x source as well. You shouldn't have the default settings there at all, that's not the normal idiom for MFDs. I'd also not expect to have to build the CODEC driver just because I built the DRM component. Now, the CODEC is declared inside the tda998x as a node child. But, in a bad DT, the tda CODEC could be declared anywhere, even inside a other DRM I2C slave encoder, in which case, bad things would happen... So, part of the problem here is that this is being explicitly declared in the DT leading to more sources for error. What does this actually do? No information is being passed in to the core function here, not even any information on if it's starting or stopping. Looking at the rest of the code I can't help thinking it might be clearer to inline this possibly with a lookup helper, the code is very small and the lack of parameters makes it hard to follow. I thought it was simple enough. The function tda_start_stop() is called from 2 places: It's not at all obvious - _audio_update() isn't a terribly descriptive name, just looking at that function by itself I had no idea what it was supposed to be doing. - on audio start in tda_startup with the audio type (DAI id) priv-dai_id = dai-id; - on audio stop with a null audio type priv-dai_id = 0; /* streaming stop */ On stream start, the DAI id is never null, as explained in the patch 1: The audio format values in the encoder configuration interface are changed to non null values so that the value 0 is used in the audio function to indicate that audio streaming is stopped. and on streaming stop the port is not meaningful. I will add a null item in the enum (AFMT_NO_AUDIO). So we can't use both streams simultaneously then? That's a bit sad. signature.asc Description: Digital signature
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, 4 Feb 2014 17:54:10 + Mark Brown broo...@kernel.org wrote: On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote: Mark Brown broo...@kernel.org wrote: + /* load the optional CODEC */ + of_platform_populate(np, NULL, NULL, client-dev); Why is this using of_platform_populate()? That's a very odd way of doing things. The i2c does not populate the subnodes in the DT. I did not find why, but, what is sure is that if of_platform_populate() is not called, the tda CODEC module is not loaded. You shouldn't be representing this as a separate node in the DT unless there really is a distinct and reusable IP, otherwise you're putting Linux implementation details in there. Describe the hardware, not the implemementation. If there is no 'compatible' node for the tda998x CODEC in the DT, the simple-card is not usable, simply because you want the CODEC DAIs to be defined by 'phandle + index' instead of by DAI name. You may find an other example in drivers/mfd/twl-core.c. The TWL drivers aren't always a shining example of how to do things - they were one of the earliest MFDs so there's warts in there. +config SND_SOC_TDA998X + tristate + depends on OF + default y if DRM_I2C_NXP_TDA998X=y + default m if DRM_I2C_NXP_TDA998X=m Make this visible if it can be selected from DT so it can be used with generic cards. I don't understand. The tda CODEC can only be used with the TDA998x I2C driver. It might have been included in the tda998x source as well. You shouldn't have the default settings there at all, that's not the normal idiom for MFDs. I'd also not expect to have to build the CODEC driver just because I built the DRM component. As the tda998x handles audio in HDMI, it would be a pity if you should connect an other cable to your screen. Now, the CODEC is declared inside the tda998x as a node child. But, in a bad DT, the tda CODEC could be declared anywhere, even inside a other DRM I2C slave encoder, in which case, bad things would happen... So, part of the problem here is that this is being explicitly declared in the DT leading to more sources for error. Simple-card constraint. What does this actually do? No information is being passed in to the core function here, not even any information on if it's starting or stopping. Looking at the rest of the code I can't help thinking it might be clearer to inline this possibly with a lookup helper, the code is very small and the lack of parameters makes it hard to follow. I thought it was simple enough. The function tda_start_stop() is called from 2 places: It's not at all obvious - _audio_update() isn't a terribly descriptive name, just looking at that function by itself I had no idea what it was supposed to be doing. The first purpose of the function is to set the audio input port in the tda998x. Streaming stop could have been omitted, but I thought it could be interesting to stop HDMI audio when there is a super HiFi device connected to the S/PDIF connector. - on audio start in tda_startup with the audio type (DAI id) priv-dai_id = dai-id; - on audio stop with a null audio type priv-dai_id = 0; /* streaming stop */ On stream start, the DAI id is never null, as explained in the patch 1: The audio format values in the encoder configuration interface are changed to non null values so that the value 0 is used in the audio function to indicate that audio streaming is stopped. and on streaming stop the port is not meaningful. I will add a null item in the enum (AFMT_NO_AUDIO). So we can't use both streams simultaneously then? That's a bit sad. That's how the NXP tda998x family works (and surely many other HDMI transmitters). So, as I understand from your remarks, the CODEC should be included in the tda998x driver, and, then, as the simple-card cannot be used, there should be a Cubox specific audio card driver for the (kirkwood audio + tda998x HDMI + S/PDIF) set. Am I right? -- Ken ar c'hentañ | ** Breizh ha Linux atav! ** Jef | http://moinejf.free.fr/ -- To unsubscribe from this list: send the line unsubscribe linux-kernel in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
On Tue, Feb 04, 2014 at 07:59:21PM +0100, Jean-Francois Moine wrote: Mark Brown broo...@kernel.org wrote: You shouldn't be representing this as a separate node in the DT unless there really is a distinct and reusable IP, otherwise you're putting Linux implementation details in there. Describe the hardware, not the implemementation. If there is no 'compatible' node for the tda998x CODEC in the DT, the simple-card is not usable, simply because you want the CODEC DAIs to be defined by 'phandle + index' instead of by DAI name. This is a bit circular, though - it's only happening because you decided to push everything onto a subnode in the DT. If you just work with the existing device this is no different to any other device. I don't understand. The tda CODEC can only be used with the TDA998x I2C driver. It might have been included in the tda998x source as well. You shouldn't have the default settings there at all, that's not the normal idiom for MFDs. I'd also not expect to have to build the CODEC driver just because I built the DRM component. As the tda998x handles audio in HDMI, it would be a pity if you should connect an other cable to your screen. My screen doesn't have any speakers anyway :P (I'm writing this on a computer with the monitor connected via HDMI). Besides, this is more about build coverage stuff than anything else. So, as I understand from your remarks, the CODEC should be included in the tda998x driver, and, then, as the simple-card cannot be used, there should be a Cubox specific audio card driver for the (kirkwood audio + tda998x HDMI + S/PDIF) set. Am I right? No, it shouldn't be. signature.asc Description: Digital signature
[PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
This patch adds a CODEC driver for the NXP TDA998x HDMI transmitter.
The CODEC handles both I2S and S/PDIF input and does dynamic input
switch in the TDA998x I2C driver on start/stop audio streaming.
This driver is DT only and it is loaded from its DT description as
a subnode in the TDA998x node.
Signed-off-by: Jean-Francois Moine
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 +
sound/soc/codecs/Kconfig | 6 ++
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tda998x.c| 216 ++
4 files changed, 228 insertions(+)
create mode 100644 sound/soc/codecs/tda998x.c
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2643be4..68f0b7b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,6 +20,7 @@
#include
#include
#include
+#include
#include
#include
@@ -1387,6 +1388,9 @@ tda998x_encoder_init(struct i2c_client *client,
priv->vip_cntrl_2 = video;
}
+ /* load the optional CODEC */
+ of_platform_populate(np, NULL, NULL, >dev);
+
return 0;
fail:
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index b33b45d..747e387 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -352,6 +352,12 @@ config SND_SOC_STAC9766
config SND_SOC_TAS5086
tristate
+config SND_SOC_TDA998X
+ tristate
+ depends on OF
+ default y if DRM_I2C_NXP_TDA998X=y
+ default m if DRM_I2C_NXP_TDA998X=m
+
config SND_SOC_TLV320AIC23
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bc12676..a53d09e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -62,6 +62,7 @@ snd-soc-sta32x-objs := sta32x.o
snd-soc-sta529-objs := sta529.o
snd-soc-stac9766-objs := stac9766.o
snd-soc-tas5086-objs := tas5086.o
+snd-soc-tda998x-objs := tda998x.o
snd-soc-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic26-objs := tlv320aic26.o
snd-soc-tlv320aic3x-objs := tlv320aic3x.o
@@ -192,6 +193,7 @@ obj-$(CONFIG_SND_SOC_STA32X) += snd-soc-sta32x.o
obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o
obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
+obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o
obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o
obj-$(CONFIG_SND_SOC_TLV320AIC26) += snd-soc-tlv320aic26.o
obj-$(CONFIG_SND_SOC_TLV320AIC3X) += snd-soc-tlv320aic3x.o
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
new file mode 100644
index 000..34d7086
--- /dev/null
+++ b/sound/soc/codecs/tda998x.c
@@ -0,0 +1,216 @@
+/*
+ * ALSA SoC TDA998X driver
+ *
+ * This driver is used by the NXP TDA998x HDMI transmitter.
+ *
+ * Copyright (C) 2014 Jean-Francois Moine
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#define TDA998X_FORMATS(SNDRV_PCM_FMTBIT_S16_LE | \
+ SNDRV_PCM_FMTBIT_S20_3LE | \
+ SNDRV_PCM_FMTBIT_S24_LE | \
+ SNDRV_PCM_FMTBIT_S32_LE)
+
+struct tda_priv {
+ struct i2c_client *i2c_client;
+ struct snd_soc_codec *codec;
+ u8 ports[2];
+ int dai_id;
+ u8 *eld;
+};
+
+static int tda_get_encoder(struct tda_priv *priv)
+{
+ struct snd_soc_codec *codec = priv->codec;
+ struct device_node *np;
+
+ /* get the parent tda998x device */
+ np = of_get_parent(codec->dev->of_node);
+ if (!np || !of_device_is_compatible(np, "nxp,tda998x")) {
+ dev_err(codec->dev, "no or bad parent!\n");
+ return -EINVAL;
+ }
+ priv->i2c_client = of_find_i2c_device_by_node(np);
+ of_node_put(np);
+ return 0;
+}
+
+static int tda_start_stop(struct tda_priv *priv)
+{
+ int port;
+
+ /* give the audio parameters to the HDMI encoder */
+ if (priv->dai_id == AFMT_I2S)
+ port = priv->ports[0];
+ else
+ port = priv->ports[1];
+ tda998x_audio_update(priv->i2c_client, priv->dai_id, port);
+ return 0;
+}
+
+static int tda_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+ /* memorize the used DAI */
+ priv->dai_id = dai->id;
+
+ /* start the TDA998x audio */
+ return tda_start_stop(priv);
+}
+
+static void tda_shutdown(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+ priv->dai_id = 0; /* streaming stop */
+
[PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x
This patch adds a CODEC driver for the NXP TDA998x HDMI transmitter.
The CODEC handles both I2S and S/PDIF input and does dynamic input
switch in the TDA998x I2C driver on start/stop audio streaming.
This driver is DT only and it is loaded from its DT description as
a subnode in the TDA998x node.
Signed-off-by: Jean-Francois Moine moin...@free.fr
---
drivers/gpu/drm/i2c/tda998x_drv.c | 4 +
sound/soc/codecs/Kconfig | 6 ++
sound/soc/codecs/Makefile | 2 +
sound/soc/codecs/tda998x.c| 216 ++
4 files changed, 228 insertions(+)
create mode 100644 sound/soc/codecs/tda998x.c
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c
b/drivers/gpu/drm/i2c/tda998x_drv.c
index 2643be4..68f0b7b 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,6 +20,7 @@
#include linux/hdmi.h
#include linux/module.h
#include linux/irq.h
+#include linux/of_platform.h
#include sound/asoundef.h
#include drm/drmP.h
@@ -1387,6 +1388,9 @@ tda998x_encoder_init(struct i2c_client *client,
priv-vip_cntrl_2 = video;
}
+ /* load the optional CODEC */
+ of_platform_populate(np, NULL, NULL, client-dev);
+
return 0;
fail:
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index b33b45d..747e387 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -352,6 +352,12 @@ config SND_SOC_STAC9766
config SND_SOC_TAS5086
tristate
+config SND_SOC_TDA998X
+ tristate
+ depends on OF
+ default y if DRM_I2C_NXP_TDA998X=y
+ default m if DRM_I2C_NXP_TDA998X=m
+
config SND_SOC_TLV320AIC23
tristate
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bc12676..a53d09e 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -62,6 +62,7 @@ snd-soc-sta32x-objs := sta32x.o
snd-soc-sta529-objs := sta529.o
snd-soc-stac9766-objs := stac9766.o
snd-soc-tas5086-objs := tas5086.o
+snd-soc-tda998x-objs := tda998x.o
snd-soc-tlv320aic23-objs := tlv320aic23.o
snd-soc-tlv320aic26-objs := tlv320aic26.o
snd-soc-tlv320aic3x-objs := tlv320aic3x.o
@@ -192,6 +193,7 @@ obj-$(CONFIG_SND_SOC_STA32X) += snd-soc-sta32x.o
obj-$(CONFIG_SND_SOC_STA529) += snd-soc-sta529.o
obj-$(CONFIG_SND_SOC_STAC9766) += snd-soc-stac9766.o
obj-$(CONFIG_SND_SOC_TAS5086) += snd-soc-tas5086.o
+obj-$(CONFIG_SND_SOC_TDA998X) += snd-soc-tda998x.o
obj-$(CONFIG_SND_SOC_TLV320AIC23) += snd-soc-tlv320aic23.o
obj-$(CONFIG_SND_SOC_TLV320AIC26) += snd-soc-tlv320aic26.o
obj-$(CONFIG_SND_SOC_TLV320AIC3X) += snd-soc-tlv320aic3x.o
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
new file mode 100644
index 000..34d7086
--- /dev/null
+++ b/sound/soc/codecs/tda998x.c
@@ -0,0 +1,216 @@
+/*
+ * ALSA SoC TDA998X driver
+ *
+ * This driver is used by the NXP TDA998x HDMI transmitter.
+ *
+ * Copyright (C) 2014 Jean-Francois Moine
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#include linux/module.h
+#include sound/soc.h
+#include sound/pcm.h
+#include linux/of.h
+#include linux/i2c.h
+#include drm/drm_encoder_slave.h
+#include drm/i2c/tda998x.h
+
+#define TDA998X_FORMATS(SNDRV_PCM_FMTBIT_S16_LE | \
+ SNDRV_PCM_FMTBIT_S20_3LE | \
+ SNDRV_PCM_FMTBIT_S24_LE | \
+ SNDRV_PCM_FMTBIT_S32_LE)
+
+struct tda_priv {
+ struct i2c_client *i2c_client;
+ struct snd_soc_codec *codec;
+ u8 ports[2];
+ int dai_id;
+ u8 *eld;
+};
+
+static int tda_get_encoder(struct tda_priv *priv)
+{
+ struct snd_soc_codec *codec = priv-codec;
+ struct device_node *np;
+
+ /* get the parent tda998x device */
+ np = of_get_parent(codec-dev-of_node);
+ if (!np || !of_device_is_compatible(np, nxp,tda998x)) {
+ dev_err(codec-dev, no or bad parent!\n);
+ return -EINVAL;
+ }
+ priv-i2c_client = of_find_i2c_device_by_node(np);
+ of_node_put(np);
+ return 0;
+}
+
+static int tda_start_stop(struct tda_priv *priv)
+{
+ int port;
+
+ /* give the audio parameters to the HDMI encoder */
+ if (priv-dai_id == AFMT_I2S)
+ port = priv-ports[0];
+ else
+ port = priv-ports[1];
+ tda998x_audio_update(priv-i2c_client, priv-dai_id, port);
+ return 0;
+}
+
+static int tda_startup(struct snd_pcm_substream *substream,
+ struct snd_soc_dai *dai)
+{
+ struct tda_priv *priv = snd_soc_codec_get_drvdata(dai-codec);
+
+ /* memorize the used DAI */
+ priv-dai_id = dai-id;
+
+ /* start the TDA998x audio */
+ return tda_start_stop(priv);
+}
+
+static void tda_shutdown(struct snd_pcm_substream *substream,
+
