[PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

This patch adds a CODEC function to the NXP TDA998x HDMI transmitter.

The CODEC handles both I2S and S/PDIF inputs and does dynamic input
switch in the TDA998x I2C driver on start/stop audio streaming.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
This patch applies against linux-next.
---
 .../devicetree/bindings/drm/i2c/tda998x.txt        |  13 ++
 drivers/gpu/drm/i2c/Makefile                       |   2 +-
 drivers/gpu/drm/i2c/tda998x_codec.c                | 241 +++++++++++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  |  74 +++++--
 drivers/gpu/drm/i2c/tda998x_drv.h                  |  31 +++
 include/drm/i2c/tda998x.h                          |   1 +
 6 files changed, 341 insertions(+), 21 deletions(-)
 create mode 100644 drivers/gpu/drm/i2c/tda998x_codec.c
 create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.h

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index d7df01c..7ce4eac 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -15,6 +15,15 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+  - 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.
+
+  - #sound-dai-cells: must be set to <1> for use with the simple-card.
+	The DAI 0 is the I2S input and the DAI 1 is the S/PDIF input.
+
 Example:
 
 	tda998x: hdmi-encoder {
@@ -24,4 +33,8 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		audio-ports = <0x03>, <0x04>;
+		audio-port-names = "i2s", "spdif";
+		#sound-dai-cells = <1>;
 	};
diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile
index 43aa33b..f2d625c 100644
--- a/drivers/gpu/drm/i2c/Makefile
+++ b/drivers/gpu/drm/i2c/Makefile
@@ -6,5 +6,5 @@ obj-$(CONFIG_DRM_I2C_CH7006) += ch7006.o
 sil164-y := sil164_drv.o
 obj-$(CONFIG_DRM_I2C_SIL164) += sil164.o
 
-tda998x-y := tda998x_drv.o
+tda998x-y := tda998x_drv.o tda998x_codec.o
 obj-$(CONFIG_DRM_I2C_NXP_TDA998X) += tda998x.o
diff --git a/drivers/gpu/drm/i2c/tda998x_codec.c b/drivers/gpu/drm/i2c/tda998x_codec.c
new file mode 100644
index 0000000..dff6afb
--- /dev/null
+++ b/drivers/gpu/drm/i2c/tda998x_codec.c
@@ -0,0 +1,241 @@
+/*
+ * ALSA SoC TDA998X CODEC
+ *
+ * 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 <sound/pcm_params.h>
+#include <linux/of.h>
+#include <linux/i2c.h>
+#include <drm/drm_encoder_slave.h>
+#include <drm/i2c/tda998x.h>
+
+#include "tda998x_drv.h"
+
+#define TDA998X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+			SNDRV_PCM_FMTBIT_S20_3LE | \
+			SNDRV_PCM_FMTBIT_S24_LE | \
+			SNDRV_PCM_FMTBIT_S32_LE)
+
+static int tda_startup(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai)
+{
+	struct tda998x_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+	u8 *eld = priv->eld;
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	u8 *sad;
+	int sad_count;
+	unsigned eld_ver, mnl, rate_mask;
+	unsigned max_channels, fmt;
+	u64 formats;
+	struct snd_pcm_hw_constraint_list *rate_constraints =
+			&priv->rate_constraints;
+	static const u32 hdmi_rates[] = {
+		32000, 44100, 48000, 88200, 9600, 176400, 192000
+	};
+
+	if (!eld)
+		return 0;
+
+	/* adjust the hw params from the ELD (EDID) */
+	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;
+	rate_mask = 0;
+	fmt = 0;
+	while (sad_count--) {
+		switch (sad[0] & 0x78) {
+		case 0x08: /* PCM */
+			max_channels = max(max_channels, (sad[0] & 7) + 1u);
+			rate_mask |= sad[1];
+			fmt |= sad[2] & 0x07;
+			break;
+		}
+		sad += 3;
+	}
+
+	/* set the constraints */
+	rate_constraints->list = hdmi_rates;
+	rate_constraints->count = ARRAY_SIZE(hdmi_rates);
+	rate_constraints->mask = rate_mask;
+	snd_pcm_hw_constraint_list(runtime, 0,
+					SNDRV_PCM_HW_PARAM_RATE,
+					rate_constraints);
+
+	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;
+	snd_pcm_hw_constraint_mask64(runtime,
+				SNDRV_PCM_HW_PARAM_FORMAT,
+				formats);
+
+	snd_pcm_hw_constraint_minmax(runtime,
+				SNDRV_PCM_HW_PARAM_CHANNELS,
+				1, max_channels);
+	return 0;
+}
+
+static int tda_hw_params(struct snd_pcm_substream *substream,
+			struct snd_pcm_hw_params *params,
+			struct snd_soc_dai *dai)
+{
+	struct tda998x_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+	/* Requires an attached display */
+	if (!priv->encoder->crtc)
+		return -ENODEV;
+
+	/* if same input and same parameters, do not do a full switch */
+	if (dai->id == priv->params.audio_format &&
+	    params_format(params) == priv->audio_sample_format) {
+		tda998x_audio_start(priv, 0);
+		return 0;
+	}
+	priv->params.audio_format = dai->id;
+	priv->audio_sample_format = params_format(params);
+	priv->params.audio_cfg =
+		priv->audio_ports[dai->id == AFMT_I2S ? 0 : 1];
+	tda998x_audio_start(priv, 1);
+	return 0;
+}
+
+static void tda_shutdown(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai)
+{
+	struct tda998x_priv *priv = snd_soc_codec_get_drvdata(dai->codec);
+
+	tda998x_audio_stop(priv);
+}
+
+static const struct snd_soc_dai_ops tda_ops = {
+	.startup = tda_startup,
+	.hw_params = tda_hw_params,
+	.shutdown = tda_shutdown,
+};
+
+static 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 i2c_client *i2c_client = to_i2c_client(codec->dev);
+	struct tda998x_priv *priv = i2c_get_clientdata(i2c_client);
+	struct device_node *np = codec->dev->of_node;
+	int i, j, ret;
+	const char *p;
+
+	if (!priv)
+		return -ENODEV;
+	snd_soc_codec_set_drvdata(codec, priv);
+
+	if (!np)
+		return 0;
+
+	/* get the audio input ports*/
+	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->audio_ports[j] = port;
+	}
+	return 0;
+}
+
+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),
+};
+
+int tda998x_codec_register(struct device *dev)
+{
+	return snd_soc_register_codec(dev,
+				&soc_codec_tda998x,
+				tda998x_dai, ARRAY_SIZE(tda998x_dai));
+}
+
+void tda998x_codec_unregister(struct device *dev)
+{
+	snd_soc_unregister_codec(dev);
+}
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 240c331..7e9f9dc 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -21,6 +21,7 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
+#include <sound/pcm_params.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
@@ -28,24 +29,9 @@
 #include <drm/drm_edid.h>
 #include <drm/i2c/tda998x.h>
 
-#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
+#include "tda998x_drv.h"
 
-struct tda998x_priv {
-	struct i2c_client *cec;
-	struct i2c_client *hdmi;
-	uint16_t rev;
-	uint8_t current_page;
-	int dpms;
-	bool is_hdmi_sink;
-	u8 vip_cntrl_0;
-	u8 vip_cntrl_1;
-	u8 vip_cntrl_2;
-	struct tda998x_encoder_params params;
-
-	wait_queue_head_t wq_edid;
-	volatile int wq_edid_wait;
-	struct drm_encoder *encoder;
-};
+#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
 
@@ -640,12 +626,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,13 +639,28 @@ 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:
 		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);
+		/* with I2S input, the CTS_N predivider depends on
+		 * the sample width */
+		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;
 
 	default:
@@ -672,6 +672,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 +729,24 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	tda998x_write_aif(priv, p);
 }
 
+/* tda998x codec interface */
+void tda998x_audio_start(struct tda998x_priv *priv,
+			 int full)
+{
+	struct tda998x_encoder_params *p = &priv->params;
+
+	if (!full) {
+		reg_write(priv, REG_ENA_AP, p->audio_cfg);
+		return;
+	}
+	tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
+}
+
+void tda998x_audio_stop(struct tda998x_priv *priv)
+{
+	reg_write(priv, REG_ENA_AP, 0);
+}
+
 /* DRM encoder functions */
 
 static void
@@ -1149,6 +1168,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);
 	}
 
@@ -1191,6 +1215,8 @@ tda998x_encoder_destroy(struct drm_encoder *encoder)
 	if (priv->hdmi->irq)
 		free_irq(priv->hdmi->irq, priv);
 
+	tda998x_codec_unregister(&priv->hdmi->dev);
+
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
 	kfree(priv);
@@ -1239,10 +1265,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);
@@ -1340,6 +1371,9 @@ tda998x_encoder_init(struct i2c_client *client,
 	/* enable EDID read irq: */
 	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
+	/* register the audio CODEC */
+	tda998x_codec_register(&client->dev);
+
 	if (!np)
 		return 0;		/* non-DT */
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.h b/drivers/gpu/drm/i2c/tda998x_drv.h
new file mode 100644
index 0000000..6db5160
--- /dev/null
+++ b/drivers/gpu/drm/i2c/tda998x_drv.h
@@ -0,0 +1,31 @@
+/* tda998x private data */
+
+struct tda998x_priv {
+	struct i2c_client *cec;
+	struct i2c_client *hdmi;
+	uint16_t rev;
+	uint8_t current_page;
+	int dpms;
+	bool is_hdmi_sink;
+	u8 vip_cntrl_0;
+	u8 vip_cntrl_1;
+	u8 vip_cntrl_2;
+	struct tda998x_encoder_params params;
+
+	wait_queue_head_t wq_edid;
+	volatile int wq_edid_wait;
+	struct drm_encoder *encoder;
+
+	u8 audio_ports[2];
+	int audio_sample_format;
+
+	u8 *eld;
+
+	struct snd_pcm_hw_constraint_list rate_constraints;
+};
+
+int tda998x_codec_register(struct device *dev);
+void tda998x_codec_unregister(struct device *dev);
+
+void tda998x_audio_start(struct tda998x_priv *priv, int full);
+void tda998x_audio_stop(struct tda998x_priv *priv);
diff --git a/include/drm/i2c/tda998x.h b/include/drm/i2c/tda998x.h
index 3e419d9..31757df 100644
--- a/include/drm/i2c/tda998x.h
+++ b/include/drm/i2c/tda998x.h
@@ -20,6 +20,7 @@ struct tda998x_encoder_params {
 	u8 audio_frame[6];
 
 	enum {
+		AFMT_NO_AUDIO = 0,
 		AFMT_SPDIF,
 		AFMT_I2S
 	} audio_format;
-- 
2.0.1


-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Andrew Lunn]

On Wed, Jul 02, 2014 at 06:38:41PM +0200, Jean-Francois Moine wrote:
> This patch adds a CODEC function to the NXP TDA998x HDMI transmitter.
> 
> The CODEC handles both I2S and S/PDIF inputs and does dynamic input
> switch in the TDA998x I2C driver on start/stop audio streaming.

Hi Jean-Francois

How well will this work with Russell concept of a front end and two
backends? Can you uses your CODEC twice, once with the I2S backend and
a second time with the S/PDIF backend?

	Andrew

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Wed, 2 Jul 2014 18:56:28 +0200
Andrew Lunn <andrew@lunn.ch> wrote:

> How well will this work with Russell concept of a front end and two
> backends? Can you uses your CODEC twice, once with the I2S backend and
> a second time with the S/PDIF backend?

Hi Andrew,

The TDA998x CODEC has two functions:
- it sets the possible formats and rates when the screen connects and
- it sets the audio input port when audio streaming starts.

I tested this CODEC with both DAPM and DPCM. If the audio subsystem
asks for streaming on both I2S and S/PDIF, only the last call is served
(this depends on the order of the DAI links in the audio card creation
table).

As I told to Russell, DPCM just asks for a 'system' DAI (front-end),
but it also asks for some additional code in the kirkwood DMA driver
because all PCMs are activated on streaming start.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Russell King - ARM Linux]

On Wed, Jul 02, 2014 at 07:51:54PM +0200, Jean-Francois Moine wrote:
> On Wed, 2 Jul 2014 18:56:28 +0200
> Andrew Lunn <andrew@lunn.ch> wrote:
> 
> > How well will this work with Russell concept of a front end and two
> > backends? Can you uses your CODEC twice, once with the I2S backend and
> > a second time with the S/PDIF backend?
> 
> Hi Andrew,
> 
> The TDA998x CODEC has two functions:
> - it sets the possible formats and rates when the screen connects and
> - it sets the audio input port when audio streaming starts.
> 
> I tested this CODEC with both DAPM and DPCM. If the audio subsystem
> asks for streaming on both I2S and S/PDIF, only the last call is served
> (this depends on the order of the DAI links in the audio card creation
> table).
> 
> As I told to Russell, DPCM just asks for a 'system' DAI (front-end),
> but it also asks for some additional code in the kirkwood DMA driver
> because all PCMs are activated on streaming start.

Well, as far as I'm concerned right now, it's for Mark to sort out this
situation, and tell us what he wants to do with the Kirkwood stuff.  I
suspect he's keeping quiet because he doesn't care about it.

Given that he's the one with the knowledge of DPCM, and he's the maintainer
of ASoC, he has the ultimate responsibility for sorting out this and
giving direction as to how he wants to proceed.

As I understand it though, my solution is the full and proper DPCM solution,
and I believe that what's currently in mainline is not a DPCM solution.

Mark needs to correct that, because he is one of the few who knows this
stuff.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Wed, Jul 02, 2014 at 07:02:12PM +0100, Russell King - ARM Linux wrote:

> Well, as far as I'm concerned right now, it's for Mark to sort out this
> situation, and tell us what he wants to do with the Kirkwood stuff.  I
> suspect he's keeping quiet because he doesn't care about it.

Well, that and the general level of stress around the support for these
boards in general.  

> Given that he's the one with the knowledge of DPCM, and he's the maintainer
> of ASoC, he has the ultimate responsibility for sorting out this and
> giving direction as to how he wants to proceed.

> As I understand it though, my solution is the full and proper DPCM solution,
> and I believe that what's currently in mainline is not a DPCM solution.

That's right, from what I remember your last set of patches looked like
it was pretty much there in terms of the code for the CPU.  Liam's
obviously the most in depth expert with DPCM but he's incredibly snowed
under with work right now.

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

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Wed, Jul 02, 2014 at 07:51:54PM +0200, Jean-Francois Moine wrote:

> I tested this CODEC with both DAPM and DPCM. If the audio subsystem
> asks for streaming on both I2S and S/PDIF, only the last call is served
> (this depends on the order of the DAI links in the audio card creation
> table).

I'd expect this to return an error for the busy DAI rather than just
silently ignore it failing to start or (better) implement some control
to let the user select which of the DAIs is active.

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

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Wed, 2 Jul 2014 20:42:52 +0100
Mark Brown <broonie@kernel.org> wrote:

> > I tested this CODEC with both DAPM and DPCM. If the audio subsystem
> > asks for streaming on both I2S and S/PDIF, only the last call is served
> > (this depends on the order of the DAI links in the audio card creation
> > table).  
> 
> I'd expect this to return an error for the busy DAI rather than just
> silently ignore it failing to start or (better) implement some control
> to let the user select which of the DAIs is active.

Mark,

This is not an error. If the audio subsystem (DPCM, not the user)
chooses to activate both I2S and S/PDIF, this means the HDMI audio may
be taken either from I2S or from S/PDIF: both inputs have the right
format and rate.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Thu, Jul 03, 2014 at 07:49:59AM +0200, Jean-Francois Moine wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > I'd expect this to return an error for the busy DAI rather than just
> > silently ignore it failing to start or (better) implement some control
> > to let the user select which of the DAIs is active.

> This is not an error. If the audio subsystem (DPCM, not the user)
> chooses to activate both I2S and S/PDIF, this means the HDMI audio may
> be taken either from I2S or from S/PDIF: both inputs have the right
> format and rate.

Your board happens to only be able to present the same input on both I2S
and S/PDIF but that might not apply to other boards, they may be able to
route different signals to each which would present a practical problem.

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

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Thu, 3 Jul 2014 11:44:32 +0100
Mark Brown <broonie@kernel.org> wrote:

> > > I'd expect this to return an error for the busy DAI rather than just
> > > silently ignore it failing to start or (better) implement some control
> > > to let the user select which of the DAIs is active.  
> 
> > This is not an error. If the audio subsystem (DPCM, not the user)
> > chooses to activate both I2S and S/PDIF, this means the HDMI audio may
> > be taken either from I2S or from S/PDIF: both inputs have the right
> > format and rate.  
> 
> Your board happens to only be able to present the same input on both I2S
> and S/PDIF but that might not apply to other boards, they may be able to
> route different signals to each which would present a practical problem.

If there are two different streamss on I2S and S/PDIF, and if the audio
subsystem wants to route these streams to the same connector (widget
'hdmi-out'), then, somewhere, there should be a software or a design
bug. No?

Anyway, the tda998x cannot know if the double route is wanted or not.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Thu, Jul 03, 2014 at 01:34:06PM +0200, Jean-Francois Moine wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > Your board happens to only be able to present the same input on both I2S
> > and S/PDIF but that might not apply to other boards, they may be able to
> > route different signals to each which would present a practical problem.

> If there are two different streamss on I2S and S/PDIF, and if the audio
> subsystem wants to route these streams to the same connector (widget
> 'hdmi-out'), then, somewhere, there should be a software or a design
> bug. No?

Yes, which is why the driver shouldn't silently ignore the situation.

> Anyway, the tda998x cannot know if the double route is wanted or not.

It doesn't need to know, it just needs to identify something it can't
support either by providing a way to pick which interface is used or by
rejecting the second interface.

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

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Thu, 3 Jul 2014 12:59:24 +0100
Mark Brown <broonie@kernel.org> wrote:

> > > Your board happens to only be able to present the same input on both I2S
> > > and S/PDIF but that might not apply to other boards, they may be able to
> > > route different signals to each which would present a practical problem.  
> 
> > If there are two different streamss on I2S and S/PDIF, and if the audio
> > subsystem wants to route these streams to the same connector (widget
> > 'hdmi-out'), then, somewhere, there should be a software or a design
> > bug. No?  
> 
> Yes, which is why the driver shouldn't silently ignore the situation.
> 
> > Anyway, the tda998x cannot know if the double route is wanted or not.  
> 
> It doesn't need to know, it just needs to identify something it can't
> support either by providing a way to pick which interface is used or by
> rejecting the second interface.

OK. no problem, I can do that: only the first stream is switched and
the second is rejected.

But, this means that there will be a lot of errors when DPCM will be
used, because, in most cases for the Cubox (kirkwood audio + tda998x),
both ways I2S and S/PDIF will be activated at the same time for a
single stream (you may note that the routes from the second input
cannot be blocked by the CODEC after it received the first input,
because these routes have already been computed).

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Russell King - ARM Linux]

On Thu, Jul 03, 2014 at 03:28:26PM +0200, Jean-Francois Moine wrote:
> OK. no problem, I can do that: only the first stream is switched and
> the second is rejected.
> 
> But, this means that there will be a lot of errors when DPCM will be
> used, because, in most cases for the Cubox (kirkwood audio + tda998x),
> both ways I2S and S/PDIF will be activated at the same time for a
> single stream (you may note that the routes from the second input
> cannot be blocked by the CODEC after it received the first input,
> because these routes have already been computed).

This is /very/ easy to solve on the Cubox, if only you would listen
to me - I've stated many times that I2S should not be used there.

Just because the hardware is wired up with both the SPDIF connected
and the I2S connected, it does *not* mean that we have to wire them
both up in software.

Indeed, *everything* works with just SPDIF.  The same is not true of
I2S.  So, what we do for the Cubox is we just wire up SPDIF in software
and be done with it - we then get a fully functional setup.  So using
I2S on the Cubox is mostly pointless - unless you wish to use I2S for
testing purposes.

Let me also point out that adding your changes - including the addition
of this codec patch - explicitly deny the possibility of:
* using compressed audio streams via the optical SPDIF out socket
* using compressed audio streams over HDMI
* using audio rates and formats not supported by the attached HDMI
  device via the optical SPDIF socket.

These are serious issues which thus far you have so far failed to
respond on.  People who use the Cubox as a media platform rather
than (presumably) just a music jukebox want things like compressed
audio out and SPDIF out to work.

Look, one reason to use the optical socket is because you want to
push out a stream at (eg) 96kHz to your audio system, but your TV
doesn't support it.  With your approach, you explicitly block that
because the TDA998x codec assocated with the Kirkwood CPU DAI
refuses to allow that sample rate.  Fine, if the attached device
does not support that rate, then the right thing to do may be to
disable audio transmission over HDMI, but with the Cubox hardware,
limiting the sample rates is totally the wrong solution.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Thu, 3 Jul 2014 14:43:46 +0100
Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:

> > But, this means that there will be a lot of errors when DPCM will be
> > used, because, in most cases for the Cubox (kirkwood audio + tda998x),
> > both ways I2S and S/PDIF will be activated at the same time for a
> > single stream (you may note that the routes from the second input
> > cannot be blocked by the CODEC after it received the first input,
> > because these routes have already been computed).  
> 
> This is /very/ easy to solve on the Cubox, if only you would listen
> to me - I've stated many times that I2S should not be used there.
> 
> Just because the hardware is wired up with both the SPDIF connected
> and the I2S connected, it does *not* mean that we have to wire them
> both up in software.
> 
> Indeed, *everything* works with just SPDIF.  The same is not true of
> I2S.  So, what we do for the Cubox is we just wire up SPDIF in software
> and be done with it - we then get a fully functional setup.  So using
> I2S on the Cubox is mostly pointless - unless you wish to use I2S for
> testing purposes.
> 
> Let me also point out that adding your changes - including the addition
> of this codec patch - explicitly deny the possibility of:
> * using compressed audio streams via the optical SPDIF out socket
> * using compressed audio streams over HDMI
> * using audio rates and formats not supported by the attached HDMI
>   device via the optical SPDIF socket.
> 
> These are serious issues which thus far you have so far failed to
> respond on.  People who use the Cubox as a media platform rather
> than (presumably) just a music jukebox want things like compressed
> audio out and SPDIF out to work.
> 
> Look, one reason to use the optical socket is because you want to
> push out a stream at (eg) 96kHz to your audio system, but your TV
> doesn't support it.  With your approach, you explicitly block that
> because the TDA998x codec assocated with the Kirkwood CPU DAI
> refuses to allow that sample rate.  Fine, if the attached device
> does not support that rate, then the right thing to do may be to
> disable audio transmission over HDMI, but with the Cubox hardware,
> limiting the sample rates is totally the wrong solution.

I think that you did not look at my code.

Both S/PDIF and I2S work with the TDA998x. It is a choice of the audio
subsystem to do this choice, not Russell King. If you want only S/PDIF,
it is easy: just declare the S/PDIF DAIs, on both sides, source
(kirkwood) and sinks (tda998x and S/PDIF):

	dai-link@1 {		/* S/PDIF - HDMI */
		cpu {
			sound-dai = <&audio1 1>;
		};
		,codec {
			sound-dai = <&hdmi 1>;
		};
	};

	dai-link@2 {		/* S/PDIF - S/PDIF */
		cpu {
			sound-dai = <&audio1 1>;
		};
		codec {
			sound-dai = <&spdif_codec>;
		};
	};

When DPCM will handle the formats and rates, the audio subsystem will
find by itself if such stream will go to the HDMI only or to the S/PDIF
only or to both. The kirwood audio driver will work as it is and the tda998x
CODEC will work as it is. There will be no need to change these drivers.
All we need actually is some more code in DPCM or multi-codec or
whatever mechanism which will route the stream according to the rates
and formats.

Actually, as DPCM does not work, the user has to choose which PCM to
use, otherwise, the application does a format and rate translation.

Anyway, if you want S/PDIF output with the actual kernel (3.16-rc3), in
the DT, put:

	spdif_codec: spdif-codec {
		compatible = "linux,spdif-dit";
		#sound-dai-cells = <0>;
	};

	sound {
		compatible = "simple-audio-card";
		simple-audio-card,name = "Cubox Audio";
		simple-audio-card,format = "i2s";

		simple-audio-card,dai-link {		/* S/PDIF - S/PDIF */
			simple-audio-card,cpu {
				sound-dai = <&audio1 1>;
			};
			simple-audio-card,codec {
				sound-dai = <&spdif_codec>;
			};
		};
	};

&audio1 {
	status = "okay";
	clocks = <&gate_clk 13>, <&si5351 2>;
	clock-names = "internal", "extclk";
	pinctrl-0 = <&pmx_audio1_i2s1_spdifo &pmx_audio1_extclk>;
	pinctrl-names = "default";
	#sound-dai-cells = <1>;
};

and the sound should get out of your S/PDIF optical connector.

Eventually, the TDA998x is used in other machines. Are you sure that
the audio controllers of all these machines have a S/PDIF output
connected to the S/PDIF input of the tda998x?

The tda998x CODEC I propose works in all configurations.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Russell King - ARM Linux]

On Thu, Jul 03, 2014 at 05:29:09PM +0200, Jean-Francois Moine wrote:
> I think that you did not look at my code.
> 
> Both S/PDIF and I2S work with the TDA998x. It is a choice of the audio
> subsystem to do this choice, not Russell King.

If you think I'm arguing because I personally want something, then we're
not going to make any progress.

What I want is for this to be _correct_ and _not_ to introduce silly
restrictions which break people's setups.

> When DPCM will handle the formats and rates, the audio subsystem will
> find by itself if such stream will go to the HDMI only or to the S/PDIF
> only or to both. The kirwood audio driver will work as it is and the tda998x
> CODEC will work as it is. There will be no need to change these drivers.

Except, as Mark has already pointed out, and as I keep pointing out to
you, and you keep refusing to accept, the kirkwood driver as it stands
is *not* a DPCM driver.  It makes no use of that stuff at all.

What you're doing in kirkwood-i2s is providing two plain DAI links,
and then insisting that only one can be active any any one time.

A DPCM solution provides at least one frontend DAI link and at least
one backend DAI link.  Your code does not do this.

The second point is that - and as I keep telling you - that as soon
as you couple your existing code (which restricts things like the
sample rates) you can no longer output anything else through the
optical out ON THE CUBOX.  For this statement, I'm not talking
_generally_,  I'm talking _specifically_ about _one_ _platform_.

> All we need actually is some more code in DPCM or multi-codec or
> whatever mechanism which will route the stream according to the rates
> and formats.

Yes, DPCM currently lacks full support, that's been known about for
some time - I've reported it to Mark, but it seems that this isn't
something that is going to get fixed until others put work into it.

> Eventually, the TDA998x is used in other machines. Are you sure that
> the audio controllers of all these machines have a S/PDIF output
> connected to the S/PDIF input of the tda998x?

Which bit of "we need to support both I2S and SPDIF" in my previous
emails was not clear.  Which bit of "We should only support SPDIF
on the Cubox" was not clear?

I *fully* acknowledge that we need to support both, but I'm putting
a _strong_ recommendation to you _with_ technical reasons why we
should _only_ support SPDIF on the Cubox.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

Re: [PATCH] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Thu, 3 Jul 2014 16:43:25 +0100
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> What you're doing in kirkwood-i2s is providing two plain DAI links,
> and then insisting that only one can be active any any one time.
> 
> A DPCM solution provides at least one frontend DAI link and at least
> one backend DAI link.  Your code does not do this.

You know why I did not insert the DPCM code in the kirkwood driver: it
does not work. But, as I tested it end 2013, I still have the code.
Do you want I propose a patch?

> Which bit of "we need to support both I2S and SPDIF" in my previous
> emails was not clear.  Which bit of "We should only support SPDIF
> on the Cubox" was not clear?
> 
> I *fully* acknowledge that we need to support both, but I'm putting
> a _strong_ recommendation to you _with_ technical reasons why we
> should _only_ support SPDIF on the Cubox.

Sorry, I still don't see why only S/PDIF should be supported on the
Cubox. I have both, and both are working on HDMI. I hope someone will
tell me it also works on S/PDIF: there is no reason it could not.

I don't see your technical reasons: you know the constraints of each
protocol I2S and S/PDIF. If you don't want I2S, just don't declare it
in the DT (see my previous mail). You may also note that I did not put
any change relative to the Cubox DT in my patch.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel