[PATCH 0/3] tda998x: add sound support

[PATCH 0/3] tda998x: add sound support

[Joao Pinto]

This patch adds sound over HDMI feature in TDA998x.
The work was inspired in a Linaro repository:
https://git.linaro.org/kernel/linux-linaro-tracking.git/tree/4b131c9bf8af9bc7a25441251f60a8ca7fc69c07

Joao Pinto (3):
  tda998x: adding sound support
  tda998: adding sound support for Juno in the DT
  tda998: add HPD delay to avoid disabling sound when EDID checksum fails.

 arch/arm64/boot/dts/arm/juno-base.dtsi |  92 ++++++--
 drivers/gpu/drm/i2c/Makefile           |   2 +-
 drivers/gpu/drm/i2c/tda998x_codec.c    | 248 ++++++++++++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c      | 115 +++++++---
 drivers/gpu/drm/i2c/tda998x_drv.h      |  41 ++++
 include/drm/i2c/tda998x.h              |   1 +
 6 files changed, 453 insertions(+), 46 deletions(-)
 create mode 100644 drivers/gpu/drm/i2c/tda998x_codec.c
 create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.h

-- 
1.8.1.5

[PATCH 1/3] tda998x: adding sound support

[Joao Pinto]

This patch adds the sound over HDMI feature to the TDA998x driver.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
 drivers/gpu/drm/i2c/Makefile        |   2 +-
 drivers/gpu/drm/i2c/tda998x_codec.c | 248 +++++++++++++++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c   | 103 ++++++----
 drivers/gpu/drm/i2c/tda998x_drv.h   |  41 ++++
 include/drm/i2c/tda998x.h           |   1 +
 5 files changed, 363 insertions(+), 32 deletions(-)
 create mode 100644 drivers/gpu/drm/i2c/tda998x_codec.c
 create mode 100644 drivers/gpu/drm/i2c/tda998x_drv.h

diff --git a/drivers/gpu/drm/i2c/Makefile b/drivers/gpu/drm/i2c/Makefile
index 2c72eb5..f897c2c 100644
--- a/drivers/gpu/drm/i2c/Makefile
+++ b/drivers/gpu/drm/i2c/Makefile
@@ -8,5 +8,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..c6e275d
--- /dev/null
+++ b/drivers/gpu/drm/i2c/tda998x_codec.c
@@ -0,0 +1,248 @@
+/*
+ * 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 int eld_ver, mnl, rate_mask;
+	unsigned int 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, 96000, 176400, 192000
+	};
+
+	/* check if streaming is already active */
+	if (priv->dai_id != AFMT_NO_AUDIO)
+		return -EBUSY;
+	priv->dai_id = dai->id;
+
+	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_sample_rate = params_rate(params);
+	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);
+	priv->dai_id = AFMT_NO_AUDIO;
+}
+
+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 f4315bc..7020b50 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,6 +20,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_atomic_helper.h>
@@ -28,32 +29,9 @@
 #include <drm/drm_of.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;
-	struct mutex mutex;
-	u16 rev;
-	u8 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 work_struct detect_work;
-	struct timer_list edid_delay_timer;
-	wait_queue_head_t edid_delay_waitq;
-	bool edid_delay_active;
-
-	struct drm_encoder encoder;
-	struct drm_connector connector;
-};
+#define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
 #define conn_to_tda998x_priv(x) \
 	container_of(x, struct tda998x_priv, connector)
@@ -714,8 +692,8 @@ static void
 tda998x_configure_audio(struct tda998x_priv *priv,
 		struct drm_display_mode *mode, struct tda998x_encoder_params *p)
 {
-	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv;
-	u32 n;
+	u8 buf[6], clksel_aip, clksel_fs, cts_n, adiv, aclk;
+	u32 n, cts;
 
 	/* Enable audio ports */
 	reg_write(priv, REG_ENA_AP, p->audio_cfg);
@@ -728,13 +706,29 @@ 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:
@@ -746,6 +739,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
@@ -771,9 +765,14 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	n = 128 * p->audio_sample_rate / 1000;
 
 	/* Write the CTS and N values */
-	buf[0] = 0x44;
-	buf[1] = 0x42;
-	buf[2] = 0x01;
+	if ((n > 0) && (mode->clock > 0))
+		cts = mode->clock;
+	else
+		cts = 82500;
+
+	buf[0] = cts;
+	buf[1] = cts >> 8;
+	buf[2] = cts >> 16;
 	buf[3] = n;
 	buf[4] = n >> 8;
 	buf[5] = n >> 16;
@@ -802,6 +812,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 tda998x_encoder_set_config(struct tda998x_priv *priv,
@@ -1159,6 +1187,11 @@ static int tda998x_connector_get_modes(struct drm_connector *connector)
 	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);
 
 	return n;
@@ -1186,6 +1219,8 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 	del_timer_sync(&priv->edid_delay_timer);
 	cancel_work_sync(&priv->detect_work);
 
+	tda998x_codec_unregister(&priv->hdmi->dev);
+
 	i2c_unregister_device(priv->cec);
 }
 
@@ -1202,6 +1237,9 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	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;
 	/* CEC I2C address bound to TDA998x I2C addr by configuration pins */
@@ -1301,6 +1339,9 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 	/* 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..25c1380
--- /dev/null
+++ b/drivers/gpu/drm/i2c/tda998x_drv.h
@@ -0,0 +1,36 @@
+/* tda998x private data */
+
+struct tda998x_priv {
+	struct i2c_client *cec;
+	struct i2c_client *hdmi;
+	struct mutex mutex;
+	struct delayed_work dwork;
+	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;
+	int wq_edid_wait;
+
+	struct work_struct detect_work;
+	struct timer_list edid_delay_timer;
+	wait_queue_head_t edid_delay_waitq;
+	bool edid_delay_active;
+	struct drm_encoder encoder;
+	struct drm_connector connector;
+	u8 audio_ports[2];
+	int audio_sample_format;
+	int dai_id;			/* DAI ID when streaming active */
+	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;
-- 
1.8.1.5

[PATCH 2/3] tda998x: adding sound support for Juno in the DT

[Joao Pinto]

This patch adds the sound over HDMI feature to the Juno Device Tree.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
 arch/arm64/boot/dts/arm/juno-base.dtsi | 92 +++++++++++++++++++----
 1 file changed, 78 insertions(+), 14 deletions(-)

diff --git a/arch/arm64/boot/dts/arm/juno-base.dtsi b/arch/arm64/boot/dts/arm/juno-base.dtsi
index 68ccc39..50c0c9a 100644
--- a/arch/arm64/boot/dts/arm/juno-base.dtsi
+++ b/arch/arm64/boot/dts/arm/juno-base.dtsi
@@ -105,17 +105,17 @@
 		clocks {
 			compatible = "arm,scpi-clocks";
 
-			scpi_dvfs: scpi-dvfs {
+			scpi_dvfs: scpi_clocks@0 {
 				compatible = "arm,scpi-dvfs-clocks";
 				#clock-cells = <1>;
 				clock-indices = <0>, <1>, <2>;
-				clock-output-names = "atlclk", "aplclk","gpuclk";
+				clock-output-names = "atlclk", "aplclk","clk_mali";
 			};
-			scpi_clk: scpi-clk {
+			scpi_clk: scpi_clocks@3 {
 				compatible = "arm,scpi-variable-clocks";
 				#clock-cells = <1>;
-				clock-indices = <3>;
-				clock-output-names = "pxlclk";
+				clock-indices = <3>, <4>, <5>;
+				clock-output-names = "pxlclk0", "pxlclk1", "i2sclk";
 			};
 		};
 
@@ -127,7 +127,7 @@
 
 	/include/ "juno-clocks.dtsi"
 
-	dma@7ff00000 {
+	dma0: dma@7ff00000 {
 		compatible = "arm,pl330", "arm,primecell";
 		reg = <0x0 0x7ff00000 0 0x1000>;
 		#dma-cells = <1>;
@@ -144,8 +144,10 @@
 			     <GIC_SPI 111 IRQ_TYPE_LEVEL_HIGH>;
 		clocks = <&soc_faxiclk>;
 		clock-names = "apb_pclk";
+		dmas = <&dma0 1>, <&dma0 2>;
+		dma-names = "rx", "tx";
 	};
-
+/*
 	hdlcd@7ff50000 {
 		compatible = "arm,hdlcd";
 		reg = <0 0x7ff50000 0 0x1000>;
@@ -154,21 +156,21 @@
 		clock-names = "pxlclk";
 
 		port {
-			hdlcd1_output: hdlcd1-endpoint {
+			hdlcd1_output: endpoint@0 {
 				remote-endpoint = <&tda998x_1_input>;
 			};
 		};
 	};
-
+*/
 	hdlcd@7ff60000 {
 		compatible = "arm,hdlcd";
 		reg = <0 0x7ff60000 0 0x1000>;
 		interrupts = <GIC_SPI 85 IRQ_TYPE_LEVEL_HIGH>;
-		clocks = <&scpi_clk 3>;
+		clocks = <&scpi_clk 4>;
 		clock-names = "pxlclk";
 
 		port {
-			hdlcd0_output: hdlcd0-endpoint {
+			hdlcd0_output: endpoint@0 {
 				remote-endpoint = <&tda998x_0_input>;
 			};
 		};
@@ -192,13 +194,20 @@
 		i2c-sda-hold-time-ns = <500>;
 		clocks = <&soc_smc50mhz>;
 
-		hdmi-transmitter@70 {
+		hdmi_transmitter0: hdmi-transmitter@70 {
 			compatible = "nxp,tda998x";
 			reg = <0x70>;
+			audio-ports = <0x03>, <0x04>;
+			audio-port-names = "i2s", "spdif";
+			#sound-dai-cells = <1>;
 			port {
-				tda998x_0_input: tda998x-0-endpoint {
+				tda998x_0_input: endpoint@0 {
 					remote-endpoint = <&hdlcd0_output>;
 				};
+
+				tda998x_0_output: endpoint@1 {
+					remote-endpoint = <&hdmi0_connector_output>;
+				};
 			};
 		};
 
@@ -206,9 +215,64 @@
 			compatible = "nxp,tda998x";
 			reg = <0x71>;
 			port {
-				tda998x_1_input: tda998x-1-endpoint {
+/*				tda998x_1_input: endpoint@0 {
 					remote-endpoint = <&hdlcd1_output>;
 				};
+*/
+				tda998x_1_output: endpoint@1 {
+					remote-endpoint = <&hdmi1_connector_output>;
+				};
+			};
+		};
+	};
+
+	soc_i2s: i2s@7ff90000 {
+		compatible = "snps,designware-i2s";
+		reg = <0x0 0x7ff90000 0x0 0x1000>;
+		clocks = <&scpi_clk 5>, <&soc_refclk100mhz>;
+		clock-names = "i2sclk", "apb_pclk";
+		#sound-dai-cells = <0>;
+		dmas = <&dma0 5>;
+		dma-names = "tx";
+	};
+
+	hdmi_audio: hdmi_audio@0 {
+		compatible = "linux,hdmi-audio";
+		#sound-dai-cells = <0>;
+		status = "okay";
+	};
+
+	sound {
+		compatible = "simple-audio-card";
+
+		simple-audio-card,format = "i2s";
+
+		simple-audio-card,cpu {
+			sound-dai = <&soc_i2s>;
+		};
+
+		simple-audio-card,codec {
+			sound-dai = <&hdmi_transmitter0 0>;
+		};
+
+	};
+
+	hdmi0: connector@0 {
+		compatible = "hdmi-connector";
+		type = "a";
+		port {
+			hdmi0_connector_output: endpoint {
+				remote-endpoint = <&tda998x_0_output>;
+			};
+		};
+	};
+
+	hdmi1: connector@1 {
+		compatible = "hdmi-connector";
+		type = "a";
+		port {
+			hdmi1_connector_output: endpoint {
+				remote-endpoint = <&tda998x_1_output>;
 			};
 		};
 	};
-- 
1.8.1.5

[PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.

[Joao Pinto]

When using ffplay to reproduce video+sound it was noticed that sometimes the
sound was disabled. The cause was an initial EDID checksum error that disabled
the HDMI sound. By adding this tweak, it was noticed that the sound is not
even when initial EDID checksum error ocurres.

Signed-off-by: Joao Pinto <jpinto@synopsys.com>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 7020b50..fc7c1c3 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -587,6 +587,17 @@ static void tda998x_detect_work(struct work_struct *work)
 		drm_kms_helper_hotplug_event(dev);
 }
 
+/* handle HDMI connect/disconnect */
+static void tda998x_hpd(struct work_struct *work)
+{
+	struct delayed_work *dwork = to_delayed_work(work);
+	struct tda998x_priv *priv =
+			container_of(dwork, struct tda998x_priv, dwork);
+
+	if (&priv->encoder && priv->encoder.dev)
+		drm_kms_helper_hotplug_event(priv->encoder.dev);
+}
+
 /*
  * only 2 interrupts may occur: screen plug/unplug and EDID read
  */
@@ -1313,6 +1324,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 
 		/* init read EDID waitqueue and HDP work */
 		init_waitqueue_head(&priv->wq_edid);
+		INIT_DELAYED_WORK(&priv->dwork, tda998x_hpd);
 
 		/* clear pending interrupts */
 		reg_read(priv, REG_INT_FLAGS_0);
-- 
1.8.1.5

Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.

[Russell King - ARM Linux]

On Mon, May 30, 2016 at 04:15:54PM +0100, Joao Pinto wrote:
> When using ffplay to reproduce video+sound it was noticed that sometimes the
> sound was disabled. The cause was an initial EDID checksum error that disabled
> the HDMI sound. By adding this tweak, it was noticed that the sound is not
> even when initial EDID checksum error ocurres.
> 
> Signed-off-by: Joao Pinto <jpinto@synopsys.com>
> ---
>  drivers/gpu/drm/i2c/tda998x_drv.c | 12 ++++++++++++
>  1 file changed, 12 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 7020b50..fc7c1c3 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -587,6 +587,17 @@ static void tda998x_detect_work(struct work_struct *work)
>  		drm_kms_helper_hotplug_event(dev);
>  }
>  
> +/* handle HDMI connect/disconnect */
> +static void tda998x_hpd(struct work_struct *work)
> +{
> +	struct delayed_work *dwork = to_delayed_work(work);
> +	struct tda998x_priv *priv =
> +			container_of(dwork, struct tda998x_priv, dwork);
> +
> +	if (&priv->encoder && priv->encoder.dev)
> +		drm_kms_helper_hotplug_event(priv->encoder.dev);
> +}
> +
>  /*
>   * only 2 interrupts may occur: screen plug/unplug and EDID read
>   */
> @@ -1313,6 +1324,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
>  
>  		/* init read EDID waitqueue and HDP work */
>  		init_waitqueue_head(&priv->wq_edid);
> +		INIT_DELAYED_WORK(&priv->dwork, tda998x_hpd);
>  
>  		/* clear pending interrupts */
>  		reg_read(priv, REG_INT_FLAGS_0);

Clearly, this patch is incomplete.  There's nothing that schedules this
work to be run.

In any case, this is reintroducing the code which I deleted when I fixed
the (rather crappy) previous implemention of delaying the EDID read after
a hotplug event.  You should not need this patch.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.

[Joao Pinto]

Hi Russell,

On 5/30/2016 8:10 PM, Russell King - ARM Linux wrote:
> On Mon, May 30, 2016 at 04:15:54PM +0100, Joao Pinto wrote:
>> When using ffplay to reproduce video+sound it was noticed that sometimes the
>> sound was disabled. The cause was an initial EDID checksum error that disabled

(...)

>> @@ -1313,6 +1324,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
>>  
>>  		/* init read EDID waitqueue and HDP work */
>>  		init_waitqueue_head(&priv->wq_edid);
>> +		INIT_DELAYED_WORK(&priv->dwork, tda998x_hpd);
>>  
>>  		/* clear pending interrupts */
>>  		reg_read(priv, REG_INT_FLAGS_0);
> 
> Clearly, this patch is incomplete.  There's nothing that schedules this
> work to be run.

You are right, forgot to include the schedule in the patch!

> 
> In any case, this is reintroducing the code which I deleted when I fixed
> the (rather crappy) previous implemention of delaying the EDID read after
> a hotplug event.  You should not need this patch.
> 

If a checksum validation fails the video reproduction is done muted if you use a
simple app like ffplay. This does not happen if using mplayer.

Could you please check the sound support patch?

Thanks,
Joao

Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.

[Russell King - ARM Linux]

On Tue, May 31, 2016 at 05:58:31PM +0100, Joao Pinto wrote:
> Hi Russell,
> 
> On 5/30/2016 8:10 PM, Russell King - ARM Linux wrote:
> > On Mon, May 30, 2016 at 04:15:54PM +0100, Joao Pinto wrote:
> >> When using ffplay to reproduce video+sound it was noticed that sometimes the
> >> sound was disabled. The cause was an initial EDID checksum error that disabled
> 
> (...)
> 
> >> @@ -1313,6 +1324,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
> >>  
> >>  		/* init read EDID waitqueue and HDP work */
> >>  		init_waitqueue_head(&priv->wq_edid);
> >> +		INIT_DELAYED_WORK(&priv->dwork, tda998x_hpd);
> >>  
> >>  		/* clear pending interrupts */
> >>  		reg_read(priv, REG_INT_FLAGS_0);
> > 
> > Clearly, this patch is incomplete.  There's nothing that schedules this
> > work to be run.
> 
> You are right, forgot to include the schedule in the patch!
> 
> > 
> > In any case, this is reintroducing the code which I deleted when I fixed
> > the (rather crappy) previous implemention of delaying the EDID read after
> > a hotplug event.  You should not need this patch.
> > 
> 
> If a checksum validation fails the video reproduction is done muted if
> you use a simple app like ffplay. This does not happen if using mplayer.

So?

We already delay the EDID read to work around reading a corrupted EDID.
If you need an additional delay, then it seems that the existing delay
is not long enough, and maybe we should extend it a little more.

What we should not do is to delay the HPD signalling.  That is definitely
the wrong solution.

In any case, I'm having a hard time understanding what the problem here
is.  You've unplugged the connected HDMI sink, so there's no longer a
display device connected, and the display hardware is shut down.  The
EDID becomes invalid.  You then plug in a HDMI sink, and now you have a
display device connected.  The EDID is read, and the display hardware is
configured according to the EDID details.  Video output is resumed, and
it takes a second or so for the sink to lock on and display the resulting
video.

At what point does the problem occur?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 3/3] tda998x: add HPD delay to avoid disabling sound when EDID checksum fails.

[Joao Pinto]

On 6/1/2016 5:32 PM, Russell King - ARM Linux wrote:
> On Tue, May 31, 2016 at 05:58:31PM +0100, Joao Pinto wrote:
>> Hi Russell,
>>
>> On 5/30/2016 8:10 PM, Russell King - ARM Linux wrote:
>>> On Mon, May 30, 2016 at 04:15:54PM +0100, Joao Pinto wrote:
>>>> When using ffplay to reproduce video+sound it was noticed that sometimes the
>>>> sound was disabled. The cause was an initial EDID checksum error that disabled
>>
>> (...)
>>
>>>> @@ -1313,6 +1324,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
>>>>  
>>>>  		/* init read EDID waitqueue and HDP work */
>>>>  		init_waitqueue_head(&priv->wq_edid);
>>>> +		INIT_DELAYED_WORK(&priv->dwork, tda998x_hpd);
>>>>  
>>>>  		/* clear pending interrupts */
>>>>  		reg_read(priv, REG_INT_FLAGS_0);
>>>
>>> Clearly, this patch is incomplete.  There's nothing that schedules this
>>> work to be run.
>>
>> You are right, forgot to include the schedule in the patch!
>>
>>>
>>> In any case, this is reintroducing the code which I deleted when I fixed
>>> the (rather crappy) previous implemention of delaying the EDID read after
>>> a hotplug event.  You should not need this patch.
>>>
>>
>> If a checksum validation fails the video reproduction is done muted if
>> you use a simple app like ffplay. This does not happen if using mplayer.
> 
> So?
> 
> We already delay the EDID read to work around reading a corrupted EDID.
> If you need an additional delay, then it seems that the existing delay
> is not long enough, and maybe we should extend it a little more.

I increased the delay to 200ms (x2) and the problem persisted.

> 
> What we should not do is to delay the HPD signalling.  That is definitely
> the wrong solution.
> 
> In any case, I'm having a hard time understanding what the problem here
> is.  You've unplugged the connected HDMI sink, so there's no longer a
> display device connected, and the display hardware is shut down.  The
> EDID becomes invalid.  You then plug in a HDMI sink, and now you have a
> display device connected.  The EDID is read, and the display hardware is
> configured according to the EDID details.  Video output is resumed, and
> it takes a second or so for the sink to lock on and display the resulting
> video.
> 
> At what point does the problem occur?

The problem happens at the kernel boot. If when the driver is instantiated the
EDID checksum is ok, ffplay plays well video+sound. If the checksum is not ok,
it assumes that the output is DVI (I think that this is the reason why we don't
have video output with sound), producing video without sound.
I have tested this and it is always the same.

If you use mplayer, the problem does not happen, since the app surely makes some
tweak that leads the driver to enable the sound.

Joao