| Message ID | ae68394f080f0ea5acaf3377cb1d18f29e42c646.1391274628.git.moinejf@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
On Tue, 4 Feb 2014 13:30:14 +0000 Mark Brown <broonie@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.
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.
On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote: > Mark Brown <broonie@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.
On Tue, 4 Feb 2014 17:54:10 +0000 Mark Brown <broonie@kernel.org> wrote: > On Tue, Feb 04, 2014 at 06:16:05PM +0100, Jean-Francois Moine wrote: > > Mark Brown <broonie@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?
On Tue, Feb 04, 2014 at 07:59:21PM +0100, Jean-Francois Moine wrote: > Mark Brown <broonie@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.
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 0000000..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, + struct snd_soc_dai *dai) +{ + struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec); + + priv->dai_id = 0; /* streaming stop */ + tda_start_stop(priv); +} + +static const struct snd_soc_dai_ops tda_ops = { + .startup = tda_startup, + .shutdown = tda_shutdown, +}; + +static const struct snd_soc_dai_driver tda998x_dai[] = { + { + .name = "i2s-hifi", + .id = AFMT_I2S, + .playback = { + .stream_name = "HDMI I2S Playback", + .channels_min = 1, + .channels_max = 8, + .rates = SNDRV_PCM_RATE_CONTINUOUS, + .rate_min = 5512, + .rate_max = 192000, + .formats = TDA998X_FORMATS, + }, + .ops = &tda_ops, + }, + { + .name = "spdif-hifi", + .id = AFMT_SPDIF, + .playback = { + .stream_name = "HDMI SPDIF Playback", + .channels_min = 1, + .channels_max = 2, + .rates = SNDRV_PCM_RATE_CONTINUOUS, + .rate_min = 22050, + .rate_max = 192000, + .formats = TDA998X_FORMATS, + }, + .ops = &tda_ops, + }, +}; + +static const struct snd_soc_dapm_widget tda_widgets[] = { + SND_SOC_DAPM_OUTPUT("hdmi-out"), +}; +static const struct snd_soc_dapm_route tda_routes[] = { + { "hdmi-out", NULL, "HDMI I2S Playback" }, + { "hdmi-out", NULL, "HDMI SPDIF Playback" }, +}; + +static int tda_probe(struct snd_soc_codec *codec) +{ + struct tda_priv *priv; + struct device_node *np = codec->dev->of_node; + int i, j, ret; + const char *p; + + priv = devm_kzalloc(codec->dev, sizeof(*priv), GFP_KERNEL); + if (!priv) + return -ENOMEM; + snd_soc_codec_set_drvdata(codec, priv); + priv->codec = codec; + + /* get the audio input ports (I2s and S/PDIF) */ + for (i = 0; i < 2; i++) { + u32 port; + + ret = of_property_read_u32_index(np, "audio-ports", i, &port); + if (ret) { + if (i == 0) + dev_err(codec->dev, + "bad or missing audio-ports\n"); + break; + } + ret = of_property_read_string_index(np, "audio-port-names", + i, &p); + if (ret) { + dev_err(codec->dev, + "missing audio-port-names[%d]\n", i); + break; + } + if (strcmp(p, "i2s") == 0) { + j = 0; + } else if (strcmp(p, "spdif") == 0) { + j = 1; + } else { + dev_err(codec->dev, + "bad audio-port-names '%s'\n", p); + break; + } + priv->ports[j] = port; + } + + /* get the tda998x device */ + return tda_get_encoder(priv); +} + +static const struct snd_soc_codec_driver soc_codec_tda998x = { + .probe = tda_probe, + .dapm_widgets = tda_widgets, + .num_dapm_widgets = ARRAY_SIZE(tda_widgets), + .dapm_routes = tda_routes, + .num_dapm_routes = ARRAY_SIZE(tda_routes), +}; + +static int tda998x_dev_probe(struct platform_device *pdev) +{ + return snd_soc_register_codec(&pdev->dev, + &soc_codec_tda998x, + tda998x_dai, ARRAY_SIZE(tda998x_dai)); +} + +static int tda998x_dev_remove(struct platform_device *pdev) +{ + snd_soc_unregister_codec(&pdev->dev); + return 0; +} + +static const struct of_device_id tda998x_codec_ids[] = { + { .compatible = "nxp,tda998x-codec", }, + { } +}; +MODULE_DEVICE_TABLE(of, tda998x_codec_ids); + +static struct platform_driver tda998x_driver = { + .probe = tda998x_dev_probe, + .remove = tda998x_dev_remove, + .driver = { + .name = "tda998x-codec", + .owner = THIS_MODULE, + .of_match_table = tda998x_codec_ids, + }, +}; + +module_platform_driver(tda998x_driver); + +MODULE_AUTHOR("Jean-Francois Moine"); +MODULE_DESCRIPTION("TDA998x codec driver"); +MODULE_LICENSE("GPL");
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 <moinejf@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