[alsa-devel] [PATCH v3 0/5] add a TDA998x CODEC
The TDA998x HDMI transmitter accepts audio input from either I2S or S/PDIF. Theses inputs have different intrinsic constraints and these constraints may be modified by the audio parameters of the connected video device.
The choice of I2S or S/PDIF may be the done by the user or by automatic processing (DPCM?) at each audio starting time. This asks for a dynamic audio input switch in the HDMI driver.
This patch series implements the TDA998x specific CODEC.
A simple function call mechanism is used for exchanges between the CODEC and the HDMI driver.
Note: the changes in the TDA998x I2C driver are based on my previous patch series: http://lists.freedesktop.org/archives/dri-devel/2014-January/052837.html
- v3 - move the DT description of the CODEC as a subnode of the TDA998x I2C (Mark Brown remark - is this the right way?) - change CODEC loading mechanism - simplify tda998x i2c reference search - CODEC DT documentation in drm/i2c/tda998x.txt
- v2 - add ACLK setting and code optimization in patch 1 - from Mark Brown's remarks in patch 2: - don't compile the codec when CONFIG_ALL_CODECS - simplify the code about start/stop audio - fix coding style errors - add audio-port-names associated to audio-ports - add audio-port-names in patch 4 - add patch 5 'adjust the audio CTS_N pre-divider from audio format' for the Beaglebone-Black board (Jyri Sarha)
Jean-Francois Moine (5): drm/i2c: tda998x: add a function for dynamic audio input switch ASoC: tda998x: add a codec driver for the TDA998x ASoC: tda998x: add DT documentation of the tda998x CODEC ASoC: tda998x: adjust the audio hw parameters from EDID ASoC: tda998x: adjust the audio CTS_N pre-divider from audio format
.../devicetree/bindings/drm/i2c/tda998x.txt | 17 ++ drivers/gpu/drm/i2c/tda998x_drv.c | 86 +++++- include/drm/i2c/tda998x.h | 11 +- sound/soc/codecs/Kconfig | 6 + sound/soc/codecs/Makefile | 2 + sound/soc/codecs/tda998x.c | 303 +++++++++++++++++++++ 6 files changed, 419 insertions(+), 6 deletions(-) create mode 100644 sound/soc/codecs/tda998x.c
When both I2S and S/PDIF are wired from the audio device to the TDA998x, the user or some internal mechanism may choose to do audio streaming via either inputs.
This patch adds an exported function in the TDA998x driver which initializes the audio input parameters according to the audio subsystem.
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.
As the audio clock depends on the input type, it is set so. Then, the configuration value audio_clk_cfg is now ignored.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 48 ++++++++++++++++++++++++++++++++++++--- include/drm/i2c/tda998x.h | 7 ++++-- 2 files changed, 50 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 2f97290..2643be4 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -35,6 +35,7 @@ struct tda998x_priv { struct i2c_client *hdmi; uint16_t rev; uint8_t current_page; + u8 audio_active; int dpms; bool is_hdmi_sink; u8 vip_cntrl_0; @@ -640,12 +641,11 @@ static void tda998x_configure_audio(struct tda998x_priv *priv, struct drm_display_mode *mode, struct tda998x_encoder_params *p) { - uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv; + uint8_t buf[6], clksel_aip, clksel_fs, cts_n, adiv, aclk; uint32_t n;
/* Enable audio ports */ reg_write(priv, REG_ENA_AP, p->audio_cfg); - reg_write(priv, REG_ENA_ACLK, p->audio_clk_cfg);
/* Set audio input source */ switch (p->audio_format) { @@ -654,6 +654,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, clksel_aip = AIP_CLKSEL_AIP_SPDIF; clksel_fs = AIP_CLKSEL_FS_FS64SPDIF; cts_n = CTS_N_M(3) | CTS_N_K(3); + aclk = 0; /* no clock */ break;
case AFMT_I2S: @@ -661,6 +662,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; cts_n = CTS_N_M(3) | CTS_N_K(3); + aclk = 1; /* clock enable */ break;
default: @@ -672,6 +674,7 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_clear(priv, REG_AIP_CNTRL_0, AIP_CNTRL_0_LAYOUT | AIP_CNTRL_0_ACR_MAN); /* auto CTS */ reg_write(priv, REG_CTS_N, cts_n); + reg_write(priv, REG_ENA_ACLK, aclk);
/* * Audio input somehow depends on HDMI line rate which is @@ -728,6 +731,37 @@ tda998x_configure_audio(struct tda998x_priv *priv, tda998x_write_aif(priv, p); }
+/* tda998x codec interface */ +void tda998x_audio_update(struct i2c_client *client, + int format, + int port) +{ + struct tda998x_priv *priv = i2c_get_clientdata(client); + struct tda998x_encoder_params *p = &priv->params; + + /* if the audio output is active, it may be a second start or a stop */ + if (format == 0 || priv->audio_active) { + if (format == 0) { + priv->audio_active = 0; + reg_write(priv, REG_ENA_AP, 0); + } + return; + } + priv->audio_active = 1; + + p->audio_cfg = port; + + /* don't restart audio if same input format */ + if (format == p->audio_format) { + reg_write(priv, REG_ENA_AP, p->audio_cfg); + return; + } + p->audio_format = format; + + tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); +} +EXPORT_SYMBOL_GPL(tda998x_audio_update); + /* DRM encoder functions */
static void @@ -750,6 +784,9 @@ tda998x_encoder_set_config(struct drm_encoder *encoder, void *params) (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
priv->params = *p; + + if (p->audio_cfg) + priv->audio_active = 1; }
static void @@ -999,7 +1036,7 @@ tda998x_encoder_mode_set(struct drm_encoder *encoder,
tda998x_write_avi(priv, adjusted_mode);
- if (priv->params.audio_cfg) + if (priv->audio_active) tda998x_configure_audio(priv, adjusted_mode, &priv->params); } @@ -1239,10 +1276,15 @@ tda998x_encoder_init(struct i2c_client *client, if (!priv) return -ENOMEM;
+ i2c_set_clientdata(client, priv); + priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3); priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1); priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
+ priv->params.audio_frame[1] = 1; /* channels - 1 */ + priv->params.audio_sample_rate = 48000; /* 48kHz */ + priv->current_page = 0xff; priv->hdmi = client; priv->cec = i2c_new_dummy(client->adapter, 0x34); diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 3e419d9..7e4806d 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -20,11 +20,14 @@ struct tda998x_encoder_params { u8 audio_frame[6];
enum { - AFMT_SPDIF, - AFMT_I2S + AFMT_I2S = 1, + AFMT_SPDIF } audio_format;
unsigned audio_sample_rate; };
+void tda998x_audio_update(struct i2c_client *client, + int format, + int port); #endif
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
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");
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, 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, 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 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.
The supported audio parameters are described in the EDID which is received by the HDMI transmitter from the connected screen. Use these ones to adjust the audio stream parameters.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 15 ++++++++ include/drm/i2c/tda998x.h | 1 + sound/soc/codecs/tda998x.c | 79 ++++++++++++++++++++++++++++++++++++++- 3 files changed, 94 insertions(+), 1 deletion(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index 68f0b7b..b833fa5 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -47,6 +47,8 @@ struct tda998x_priv { wait_queue_head_t wq_edid; volatile int wq_edid_wait; struct drm_encoder *encoder; + + u8 *eld; };
#define to_tda998x_priv(x) ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv) @@ -733,6 +735,14 @@ tda998x_configure_audio(struct tda998x_priv *priv, }
/* tda998x codec interface */ +u8 *tda998x_audio_get_eld(struct i2c_client *client) +{ + struct tda998x_priv *priv = i2c_get_clientdata(client); + + return priv->eld; +} +EXPORT_SYMBOL_GPL(tda998x_audio_get_eld); + void tda998x_audio_update(struct i2c_client *client, int format, int port) @@ -1187,6 +1197,11 @@ tda998x_encoder_get_modes(struct drm_encoder *encoder, drm_mode_connector_update_edid_property(connector, edid); n = drm_add_edid_modes(connector, edid); priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid); + + /* keep the EDID as ELD for the audio subsystem */ + drm_edid_to_eld(connector, edid); + priv->eld = connector->eld; + kfree(edid); }
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 7e4806d..99387ae 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -27,6 +27,7 @@ struct tda998x_encoder_params { unsigned audio_sample_rate; };
+u8 *tda998x_audio_get_eld(struct i2c_client *client); void tda998x_audio_update(struct i2c_client *client, int format, int port); diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c index 34d7086..0493163 100644 --- a/sound/soc/codecs/tda998x.c +++ b/sound/soc/codecs/tda998x.c @@ -64,10 +64,79 @@ 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); + u8 *eld = NULL; + static unsigned rates_mask[] = { + SNDRV_PCM_RATE_32000, + SNDRV_PCM_RATE_44100, + SNDRV_PCM_RATE_48000, + SNDRV_PCM_RATE_88200, + SNDRV_PCM_RATE_96000, + SNDRV_PCM_RATE_176400, + SNDRV_PCM_RATE_192000, + };
/* memorize the used DAI */ priv->dai_id = dai->id;
+ /* get the ELD from the tda998x driver */ + if (!priv->i2c_client) + tda_get_encoder(priv); + if (priv->i2c_client) + eld = tda998x_audio_get_eld(priv->i2c_client); + + /* adjust the hw params from the ELD (EDID) */ + if (eld) { + struct snd_soc_dai_driver *dai_drv = dai->driver; + struct snd_soc_pcm_stream *stream = &dai_drv->playback; + u8 *sad; + int sad_count; + unsigned eld_ver, mnl, rates, rate_mask, i; + unsigned max_channels, fmt; + u64 formats; + + eld_ver = eld[0] >> 3; + if (eld_ver != 2 && eld_ver != 31) + return 0; + + mnl = eld[4] & 0x1f; + if (mnl > 16) + return 0; + + sad_count = eld[5] >> 4; + sad = eld + 20 + mnl; + + /* Start from the basic audio settings */ + max_channels = 2; + rates = 0; + fmt = 0; + while (sad_count--) { + switch (sad[0] & 0x78) { + case 0x08: /* PCM */ + max_channels = max(max_channels, (sad[0] & 7) + 1u); + rates |= sad[1]; + fmt |= sad[2] & 0x07; + break; + } + sad += 3; + } + + for (rate_mask = i = 0; i < ARRAY_SIZE(rates_mask); i++) + if (rates & 1 << i) + rate_mask |= rates_mask[i]; + formats = 0; + if (fmt & 1) + formats |= SNDRV_PCM_FMTBIT_S16_LE; + if (fmt & 2) + formats |= SNDRV_PCM_FMTBIT_S20_3LE; + if (fmt & 4) + formats |= SNDRV_PCM_FMTBIT_S24_LE; + + /* change the snd_soc_pcm_stream values of the driver */ + stream->rates = rate_mask; + stream->channels_max = max_channels; + stream->formats = formats; + } + /* start the TDA998x audio */ return tda_start_stop(priv); } @@ -182,9 +251,17 @@ static const struct snd_soc_codec_driver soc_codec_tda998x = {
static int tda998x_dev_probe(struct platform_device *pdev) { + struct snd_soc_dai_driver *dai_drv; + + /* copy the DAI driver to a writable area */ + dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL); + if (!dai_drv) + return -ENOMEM; + memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai)); + return snd_soc_register_codec(&pdev->dev, &soc_codec_tda998x, - tda998x_dai, ARRAY_SIZE(tda998x_dai)); + dai_drv, ARRAY_SIZE(tda998x_dai)); }
static int tda998x_dev_remove(struct platform_device *pdev)
On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
/* change the snd_soc_pcm_stream values of the driver */stream->rates = rate_mask;stream->channels_max = max_channels;stream->formats = formats;
- /* copy the DAI driver to a writable area */
- dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL);
- if (!dai_drv)
return -ENOMEM;- memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai));
The code should be doing this by setting constraints based on the current setup rather than by editing the data structure - the expecation is very much that the data won't change so this prevents surprises with future work on the core.
On Tue, 4 Feb 2014 18:06:25 +0000 Mark Brown broonie@kernel.org wrote:
On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
/* change the snd_soc_pcm_stream values of the driver */stream->rates = rate_mask;stream->channels_max = max_channels;stream->formats = formats;
- /* copy the DAI driver to a writable area */
- dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL);
- if (!dai_drv)
return -ENOMEM;- memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai));
The code should be doing this by setting constraints based on the current setup rather than by editing the data structure - the expecation is very much that the data won't change so this prevents surprises with future work on the core.
As it is done in the soc core, in soc_pcm_open(), the runtime hw_params are initialized after the call to the CODEC startup, and the next CODEC event is hw_params() when the user has already chosen all the parameters.
So, in the CODEC, I don't see how I could update the parameters dictated by the EDID otherwise in changing the DAI driver parameters.
On 02/05/2014 10:11 AM, Jean-Francois Moine wrote:
On Tue, 4 Feb 2014 18:06:25 +0000 Mark Brown broonie@kernel.org wrote:
On Mon, Jan 27, 2014 at 09:48:54AM +0100, Jean-Francois Moine wrote:
/* change the snd_soc_pcm_stream values of the driver */stream->rates = rate_mask;stream->channels_max = max_channels;stream->formats = formats;
- /* copy the DAI driver to a writable area */
- dai_drv = devm_kzalloc(&pdev->dev, sizeof(tda998x_dai), GFP_KERNEL);
- if (!dai_drv)
return -ENOMEM;- memcpy(dai_drv, tda998x_dai, sizeof(tda998x_dai));
The code should be doing this by setting constraints based on the current setup rather than by editing the data structure - the expecation is very much that the data won't change so this prevents surprises with future work on the core.
As it is done in the soc core, in soc_pcm_open(), the runtime hw_params are initialized after the call to the CODEC startup, and the next CODEC event is hw_params() when the user has already chosen all the parameters.
So, in the CODEC, I don't see how I could update the parameters dictated by the EDID otherwise in changing the DAI driver parameters.
The startup function is the right place. But instead of modifying the DAI use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to setup the additional constraints that come from the EDID.
Bonus points for making this a generic helper function that takes a runtime and a EDID and then applies the EDID constraints on the runtime.
- Lars
On Wed, Feb 05, 2014 at 10:19:22AM +0100, Lars-Peter Clausen wrote:
On 02/05/2014 10:11 AM, Jean-Francois Moine wrote:
So, in the CODEC, I don't see how I could update the parameters dictated by the EDID otherwise in changing the DAI driver parameters.
The startup function is the right place. But instead of modifying the DAI use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to setup the additional constraints that come from the EDID.
Right.
Bonus points for making this a generic helper function that takes a runtime and a EDID and then applies the EDID constraints on the runtime.
...as previously requested. I know there was some discusion of broader moves to factor this stuff out but it'd still be good to keep it separated out even prior to a final non-EDID based solution so it's easier to refactor.
On 02/05/2014 12:18 PM, Mark Brown wrote:
On Wed, Feb 05, 2014 at 10:19:22AM +0100, Lars-Peter Clausen wrote:
[..]
Bonus points for making this a generic helper function that takes a runtime and a EDID and then applies the EDID constraints on the runtime.
...as previously requested. I know there was some discusion of broader moves to factor this stuff out but it'd still be good to keep it separated out even prior to a final non-EDID based solution so it's easier to refactor.
I think it will always be EDID (or ELD) based. As I understood it the re-factoring Takashi was talking about is related to how this data is passed from the graphics driver to the audio driver. The way things work right now in HDA land is that the graphics driver reads the EDID from the monitor, converts it to ELD and writes it to a special memory region in the graphics controller. This generates an interrupt in the audio driver and the audio driver reads the ELD from the hardware and sets up the constraints based on that. And I think that the plan is to change this to pass the EDID directly from the graphics driver to the audio driver without taking the detour through the hardware. This is what we'll need for embedded systems anyway. A system that allows to associate a sound driver with a specific HDMI port and status updates for that port, e.g. new EDID or cable connected/disconnected are passed from the graphics driver handling that port to the audio driver.
- Lars
On Wed, Feb 05, 2014 at 02:31:09PM +0100, Lars-Peter Clausen wrote:
On 02/05/2014 12:18 PM, Mark Brown wrote:
...as previously requested. I know there was some discusion of broader moves to factor this stuff out but it'd still be good to keep it separated out even prior to a final non-EDID based solution so it's easier to refactor.
I think it will always be EDID (or ELD) based. As I understood it the re-factoring Takashi was talking about is related to how this data is passed from the graphics driver to the audio driver. The way
Right, the data of course has to come from there originally, it's about how we pass the data and updates to it around.
On Wed, 05 Feb 2014 10:19:22 +0100 Lars-Peter Clausen lars@metafoo.de wrote:
So, in the CODEC, I don't see how I could update the parameters dictated by the EDID otherwise in changing the DAI driver parameters.
The startup function is the right place. But instead of modifying the DAI use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to setup the additional constraints that come from the EDID.
It is more complicated, but it works. Nevertheless, I have 2 problems:
- snd_pcm_hw_constraint_list() keeps a pointer to the list, so, it cannot be in the stack. It fix this with static struct and rate array.
- snd_pcm_hw_constraint_mask64() is not exported. Is there an other way to set constraints on the formats/sample widths?
On 02/05/2014 07:07 PM, Jean-Francois Moine wrote:
On Wed, 05 Feb 2014 10:19:22 +0100 Lars-Peter Clausen lars@metafoo.de wrote:
So, in the CODEC, I don't see how I could update the parameters dictated by the EDID otherwise in changing the DAI driver parameters.
The startup function is the right place. But instead of modifying the DAI use snd_pcm_hw_constraint_mask64(), snd_pcm_hw_constraint_list(), etc. to setup the additional constraints that come from the EDID.
It is more complicated, but it works. Nevertheless, I have 2 problems:
- snd_pcm_hw_constraint_list() keeps a pointer to the list, so, it cannot be in the stack. It fix this with static struct and rate array.
Right. If the struct is modified though it should be per device and not global. I think the best way to implement this is to make the array static and specify a mask for the constraint based on the EDID. E.g.
static unsigned int hdmi_rates[] = { 32000, 44100, 48000, 88200, 96000, 176400, 192000, };
rate_mask = 0;
while (...) { ... rate_mask |= 1 << sad[1]; }
rate_constraints->list = hdmi_rates; rate_constraints->count = ARRAY_SIZE(hdmi_rates); rate_constraints->mask = rate_mask;
- snd_pcm_hw_constraint_mask64() is not exported. Is there an other way to set constraints on the formats/sample widths?
I think that's a bug. Both snd_pcm_hw_constraint_mask() and snd_pcm_hw_constraint_mask64() should be exported. Can you send a patch?
- Lars
In some boards, with I2S input, the NXP TDA998x HDMI transmitter did not play audio streams with a sample width lower than S16_32.
This patch adjusts the CTS_N predivider according to the used sample width.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- drivers/gpu/drm/i2c/tda998x_drv.c | 25 +++++++++++++++++++++---- include/drm/i2c/tda998x.h | 5 ++++- sound/soc/codecs/tda998x.c | 18 ++++++++++++++---- 3 files changed, 39 insertions(+), 9 deletions(-)
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c index b833fa5..66013ba 100644 --- a/drivers/gpu/drm/i2c/tda998x_drv.c +++ b/drivers/gpu/drm/i2c/tda998x_drv.c @@ -22,6 +22,7 @@ #include <linux/irq.h> #include <linux/of_platform.h> #include <sound/asoundef.h> +#include <sound/pcm_params.h>
#include <drm/drmP.h> #include <drm/drm_crtc_helper.h> @@ -48,6 +49,8 @@ struct tda998x_priv { volatile int wq_edid_wait; struct drm_encoder *encoder;
+ int audio_sample_format; + u8 *eld; };
@@ -664,7 +667,18 @@ tda998x_configure_audio(struct tda998x_priv *priv, reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S); clksel_aip = AIP_CLKSEL_AIP_I2S; clksel_fs = AIP_CLKSEL_FS_ACLK; - cts_n = CTS_N_M(3) | CTS_N_K(3); + switch (priv->audio_sample_format) { + case SNDRV_PCM_FORMAT_S16_LE: + cts_n = CTS_N_M(3) | CTS_N_K(1); + break; + case SNDRV_PCM_FORMAT_S24_LE: + cts_n = CTS_N_M(3) | CTS_N_K(2); + break; + default: + case SNDRV_PCM_FORMAT_S32_LE: + cts_n = CTS_N_M(3) | CTS_N_K(3); + break; + } aclk = 1; /* clock enable */ break;
@@ -745,13 +759,14 @@ EXPORT_SYMBOL_GPL(tda998x_audio_get_eld);
void tda998x_audio_update(struct i2c_client *client, int format, - int port) + int port, + struct snd_pcm_hw_params *params) { struct tda998x_priv *priv = i2c_get_clientdata(client); struct tda998x_encoder_params *p = &priv->params;
/* if the audio output is active, it may be a second start or a stop */ - if (format == 0 || priv->audio_active) { + if (format == 0 || !params || priv->audio_active) { if (format == 0) { priv->audio_active = 0; reg_write(priv, REG_ENA_AP, 0); @@ -763,11 +778,13 @@ void tda998x_audio_update(struct i2c_client *client, p->audio_cfg = port;
/* don't restart audio if same input format */ - if (format == p->audio_format) { + if (format == p->audio_format && + params_format(params) == priv->audio_sample_format) { reg_write(priv, REG_ENA_AP, p->audio_cfg); return; } p->audio_format = format; + priv->audio_sample_format = params_format(params);
tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p); } diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h index 99387ae..62b838f 100644 --- a/include/drm/i2c/tda998x.h +++ b/include/drm/i2c/tda998x.h @@ -27,8 +27,11 @@ struct tda998x_encoder_params { unsigned audio_sample_rate; };
+struct snd_pcm_hw_params; + u8 *tda998x_audio_get_eld(struct i2c_client *client); void tda998x_audio_update(struct i2c_client *client, int format, - int port); + int port, + struct snd_pcm_hw_params *params); #endif diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c index 0493163..a1de35d 100644 --- a/sound/soc/codecs/tda998x.c +++ b/sound/soc/codecs/tda998x.c @@ -47,7 +47,8 @@ static int tda_get_encoder(struct tda_priv *priv) return 0; }
-static int tda_start_stop(struct tda_priv *priv) +static int tda_start_stop(struct tda_priv *priv, + struct snd_pcm_hw_params *params) { int port;
@@ -56,7 +57,7 @@ static int tda_start_stop(struct tda_priv *priv) port = priv->ports[0]; else port = priv->ports[1]; - tda998x_audio_update(priv->i2c_client, priv->dai_id, port); + tda998x_audio_update(priv->i2c_client, priv->dai_id, port, params); return 0; }
@@ -136,9 +137,17 @@ static int tda_startup(struct snd_pcm_substream *substream, stream->channels_max = max_channels; stream->formats = formats; } + return 0; +} + +static int tda_hw_params(struct snd_pcm_substream *substream, + struct snd_pcm_hw_params *params, + struct snd_soc_dai *dai) +{ + struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
/* start the TDA998x audio */ - return tda_start_stop(priv); + return tda_start_stop(priv, params); }
static void tda_shutdown(struct snd_pcm_substream *substream, @@ -147,11 +156,12 @@ static void tda_shutdown(struct snd_pcm_substream *substream, struct tda_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
priv->dai_id = 0; /* streaming stop */ - tda_start_stop(priv); + tda_start_stop(priv, NULL); }
static const struct snd_soc_dai_ops tda_ops = { .startup = tda_startup, + .hw_params = tda_hw_params, .shutdown = tda_shutdown, };
On Thu, Jan 30, 2014 at 12:08:06PM +0100, Jean-Francois Moine wrote:
- if (format == p->audio_format) {
- if (format == p->audio_format &&
params_format(params) == priv->audio_sample_format) {
I find the above confusing and probably deserving of a comment explaining the logic. I think it's trying to skip noop configuration updates?
} p->audio_format = format;
- priv->audio_sample_format = params_format(params);
You should be using params_width() for the number of bits per sample.
This patch adds the DT documentation of the NXP TDA998x CODEC.
Signed-off-by: Jean-Francois Moine moinejf@free.fr --- Documentation/devicetree/bindings/drm/i2c/tda998x.txt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt index d7df01c..aa0d81b 100644 --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt @@ -15,6 +15,17 @@ Optional properties: - video-ports: 24 bits value which defines how the video controller output is wired to the TDA998x input - default: <0x230145>
+Optional subnodes: + + - codec: audio CODEC + +Required codec subnode properties: + - compatible: must be "nxp,tda998x-codec". + - audio-ports: one or two values corresponding to entries in + the audio-port-names property. + - audio-port-names: must contain "i2s", "spdif" entries + matching entries in the audio-ports property. + Example:
tda998x: hdmi-encoder { @@ -24,4 +35,10 @@ Example: interrupts = <27 2>; /* falling edge */ pinctrl-0 = <&pmx_camera>; pinctrl-names = "default"; + hdmi_codec: codec { + compatible = "nxp,tda998x-codec"; + audio-ports = <0x03>, <0x04>; + audio-port-names = "i2s", "spdif"; + #sound-dai-cells = <1>; + }; };
Hello.
On 01-02-2014 20:48, Jean-Francois Moine wrote:
This patch adds the DT documentation of the NXP TDA998x CODEC.
Signed-off-by: Jean-Francois Moine moinejf@free.fr
Documentation/devicetree/bindings/drm/i2c/tda998x.txt | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)
diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt index d7df01c..aa0d81b 100644 --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
[...]
@@ -24,4 +35,10 @@ Example: interrupts = <27 2>; /* falling edge */ pinctrl-0 = <&pmx_camera>; pinctrl-names = "default";
hdmi_codec: codec {
This line is indented too far to the right.
compatible = "nxp,tda998x-codec";audio-ports = <0x03>, <0x04>;audio-port-names = "i2s", "spdif";#sound-dai-cells = <1>;};
WBR, Sergei
On Sat, Feb 01, 2014 at 05:48:49PM +0100, Jean-Francois Moine wrote:
- compatible: must be "nxp,tda998x-codec".
It's not clear to me why there's a separate compatible here - as far as I can see this can only appear as part of one of these devices and there's no addressing or other information that'd account for chip variation so I'd not expect to need to bind this independently of the parent.
On Tue, 4 Feb 2014 18:12:13 +0000 Mark Brown broonie@kernel.org wrote:
On Sat, Feb 01, 2014 at 05:48:49PM +0100, Jean-Francois Moine wrote:
- compatible: must be "nxp,tda998x-codec".
It's not clear to me why there's a separate compatible here - as far as I can see this can only appear as part of one of these devices and there's no addressing or other information that'd account for chip variation so I'd not expect to need to bind this independently of the parent.
If there is no 'compatible', the CODEC module is not loaded, and, when the module is in the core, no CODEC device can be created from the DT.
On Tue, Feb 04, 2014 at 08:02:39PM +0100, Jean-Francois Moine wrote:
Mark Brown broonie@kernel.org wrote:
On Sat, Feb 01, 2014 at 05:48:49PM +0100, Jean-Francois Moine wrote:
- compatible: must be "nxp,tda998x-codec".
It's not clear to me why there's a separate compatible here - as far as I can see this can only appear as part of one of these devices and there's no addressing or other information that'd account for chip variation so I'd not expect to need to bind this independently of the parent.
If there is no 'compatible', the CODEC module is not loaded, and, when the module is in the core, no CODEC device can be created from the DT.
You're confusing implementation details with device tree specification here. We can easily handle loading a subdriver without having to put anything in the device tree, just create a platform device like we do with MFDs.
participants (4)
-
Jean-Francois Moine -
Lars-Peter Clausen -
Mark Brown -
Sergei Shtylyov