[PATCH v3 1/5] drm/i2c: tda998x: add a function for dynamic audio input switch

[PATCH v3 1/5] drm/i2c: tda998x: add a function for dynamic audio input switch

[Jean-Francois Moine]

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
-- 
1.9.rc1

[PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Jean-Francois Moine]

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");
-- 
1.9.rc1

[PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Jean-Francois Moine]

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)
-- 
1.9.rc1

[PATCH v3 5/5] ASoC: tda998x: adjust the audio CTS_N pre-divider from audio format

[Jean-Francois Moine]

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,
 };
 
-- 
1.9.rc1

[PATCH v3 3/5] ASoC: tda998x: add DT documentation of the tda998x CODEC

[Jean-Francois Moine]

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>;
+		};
 	};
-- 
1.9.rc1

[PATCH v3 0/5] add a TDA998x CODEC

[Jean-Francois Moine]

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

-- 
1.9.rc1

Re: [PATCH v3 3/5] ASoC: tda998x: add DT documentation of the tda998x CODEC

[Sergei Shtylyov]

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

Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1884 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Lars-Peter Clausen]

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

Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Jean-Francois Moine]

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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 519 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 3070 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 696 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 5/5] ASoC: tda998x: adjust the audio CTS_N pre-divider from audio format

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 573 bytes --]

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) {
>  		reg_write(priv, REG_ENA_AP, p->audio_cfg);
>  		return;

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 3/5] ASoC: tda998x: add DT documentation of the tda998x CODEC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Jean-Francois Moine]

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?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v3 3/5] ASoC: tda998x: add DT documentation of the tda998x CODEC

[Jean-Francois Moine]

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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v3 2/5] ASoC: tda998x: add a codec driver for the TDA998x

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1701 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 3/5] ASoC: tda998x: add DT documentation of the tda998x CODEC

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 911 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Jean-Francois Moine]

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.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Lars-Peter Clausen]

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

Re: [alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 859 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Lars-Peter Clausen]

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

Re: [alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 678 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Jean-Francois Moine]

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?

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [alsa-devel] [PATCH v3 4/5] ASoC: tda998x: adjust the audio hw parameters from EDID

[Lars-Peter Clausen]

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