[PATCH v13 0/3] ASoC: tda998x: add a codec to the HDMI transmitter

[PATCH v13 0/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

This patch series asks for Russell King's patch

	http://article.gmane.org/gmane.linux.ports.arm.kernel/411574
	drm/edid: add function to help find SADs

to be applayed.

v13:
	- rebase on 4.2.0-rc3
	- remove Russell's patches
v12:
	- use Russell King's DRM ELD helper (Mark Brown)
v11:
	- reduce the patch series to adding the tda998x codec only
v10:
	- add the generic dt-card
	- define the audio ports from a DT graph of ports (Russell King)
	- reuse HDMI constants (Andrew Jackson - Jyri Sarha)
	- alloc rate_constraints in codec (Jyri Sarha)
	- fix bad number of channels (Jyri Sarha)
	- correct codec generation from config (Russell King - Jyri Sarha)
	- no module init/exit (Russell King)
v9:
	- back to a TDA998x specific CODEC
	- more comments
	- change magic values to constants
v8:
	- change some comments about the patches
v7:
	- remove the change of the K predivider (Jyri Sarha)
	- add S24_3LE and S32_LE as possible audio formats (Jyri Sarha)
	- don't move the struct priv2 definition and use the
	  slave encoder private data as the device private data
	  (Russell King)
	- remove the useless request_module (Russell King/Mark Brown)
	- don't lock the HDMI module (Russell King)
	- use platform_device_unregister to remove the codec
	  (Russell King)
v6:
	- extend the HDMI CODEC instead of using a specific CODEC
v5:
	- use the TDA998x private data instead of a specific area
	  for the CODEC interface
	- the CODEC is TDA998x specific (Mark Brown)
v4:
	- remove all the TDA998x specific stuff from the CODEC
	- move the EDID scan from the CODEC to the TDA998x
	- move the CODEC to sound/soc (Mark Brown)
	- update the audio_sample_rate from the EDID (Andrew Jackson)
v3: fix bad rate (Andrew Jackson)
v2: check double stream start (Mark Brown)

Jean-Francois Moine (3):
  drm/i2c: tda998x: Add support of a DT graph of ports
  drm/i2c: tda998x: Change drvdata for audio extension
  ASoC: tda998x: add a codec to the HDMI transmitter

 .../devicetree/bindings/drm/i2c/tda998x.txt        |  51 ++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 178 +++++++++++++++++++--
 include/sound/tda998x.h                            |  23 +++
 sound/soc/codecs/Kconfig                           |   6 +
 sound/soc/codecs/Makefile                          |   2 +
 sound/soc/codecs/tda998x.c                         | 132 +++++++++++++++
 6 files changed, 381 insertions(+), 11 deletions(-)
 create mode 100644 include/sound/tda998x.h
 create mode 100644 sound/soc/codecs/tda998x.c

-- 
2.1.4

[PATCH v13 1/3] drm/i2c: tda998x: Add support of a DT graph of ports

[Jean-Francois Moine]

Two kinds of ports may be declared in a DT graph of ports: video and audio.
This patch accepts the port value from a video port as an alternative
to the video-ports property.
It also accepts audio ports in the case the transmitter is not used as
a slave encoder.
The new file include/sound/tda998x.h prepares to the definition of
a tda998x CODEC.

Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
---
 .../devicetree/bindings/drm/i2c/tda998x.txt        | 51 ++++++++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 90 +++++++++++++++++++---
 include/sound/tda998x.h                            |  8 ++
 3 files changed, 140 insertions(+), 9 deletions(-)
 create mode 100644 include/sound/tda998x.h

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index e9e4bce..386b6c3 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -16,6 +16,35 @@ Optional properties:
 
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
+	This property is not used when ports are defined.
+
+Optional nodes:
+
+  - port: up to three ports.
+	The ports are defined according to [1].
+
+    Video port.
+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"
+
+	and may contain the optional property:
+
+	- reg: 24 bits value which defines how the video controller
+		output is wired to the TDA998x input (video pins)
+		When absent, the default value is <0x230145>.
+
+    Audio ports.
+	There may be one or two audio ports.
+	These ones must contain the following properties:
+
+	- port-type: must be "i2s" or "spdif"
+
+	- reg: 8 bits value which defines how the audio controller
+		output is wired to the TDA998x input (audio pins)
+
+[1] Documentation/devicetree/bindings/graph.txt
 
 Example:
 
@@ -26,4 +55,26 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		port@230145 {
+			port-type = "rgb";
+			reg = <0x230145>;
+			hdmi_0: endpoint {
+				remote-endpoint = <&lcd0_0>;
+			};
+		};
+		port@3 {			/* AP1 = I2S */
+			port-type = "i2s";
+			reg = <0x03>;
+			tda998x_i2s: endpoint {
+				remote-endpoint = <&audio1_i2s>;
+			};
+		};
+		port@4 {			 /* AP2 = S/PDIF */
+			port-type = "spdif";
+			reg = <0x04>;
+			tda998x_spdif: endpoint {
+				remote-endpoint = <&audio1_spdif1>;
+			};
+		};
 	};
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 424228b..b0a730a 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -27,6 +27,7 @@
 #include <drm/drm_edid.h>
 #include <drm/drm_of.h>
 #include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 
@@ -47,6 +48,8 @@ struct tda998x_priv {
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
 	struct drm_encoder *encoder;
+
+	struct tda998x_audio_s audio;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -774,6 +777,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
 			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
 	priv->params = *p;
+	priv->audio.port_types[0] = p->audio_format;
+	priv->audio.ports[0] = p->audio_cfg;
 }
 
 static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)
@@ -1230,9 +1235,57 @@ static struct drm_encoder_slave_funcs tda998x_encoder_slave_funcs = {
 
 /* I2C driver functions */
 
+static int tda998x_parse_ports(struct tda998x_priv *priv,
+				struct device_node *np)
+{
+	struct device_node *of_port;
+	const char *port_type;
+	int ret, audio_index, reg, afmt;
+
+	audio_index = 0;
+	for_each_child_of_node(np, of_port) {
+		if (!of_port->name
+		 || of_node_cmp(of_port->name, "port") != 0)
+			continue;
+		ret = of_property_read_string(of_port, "port-type",
+					&port_type);
+		if (ret < 0)
+			continue;
+		ret = of_property_read_u32(of_port, "reg", &reg);
+		if (strcmp(port_type, "rgb") == 0) {
+			if (!ret) {		/* video reg is optional */
+				priv->vip_cntrl_0 = reg >> 16;
+				priv->vip_cntrl_1 = reg >> 8;
+				priv->vip_cntrl_2 = reg;
+			}
+			continue;
+		}
+		if (strcmp(port_type, "i2s") == 0)
+			afmt = AFMT_I2S;
+		else if (strcmp(port_type, "spdif") == 0)
+			afmt = AFMT_SPDIF;
+		else
+			continue;
+		if (ret < 0) {
+			dev_err(&priv->hdmi->dev, "missing reg for %s\n",
+				port_type);
+			return ret;
+		}
+		if (audio_index >= ARRAY_SIZE(priv->audio.ports)) {
+			dev_err(&priv->hdmi->dev, "too many audio ports\n");
+			break;
+		}
+		priv->audio.ports[audio_index] = reg;
+		priv->audio.port_types[audio_index] = afmt;
+		audio_index++;
+	}
+	return 0;
+}
+
 static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
+	struct device_node *of_port;
 	u32 video;
 	int rev_lo, rev_hi, ret;
 	unsigned short cec_addr;
@@ -1337,15 +1390,34 @@ 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);
 
-	if (!np)
-		return 0;		/* non-DT */
-
-	/* get the optional video properties */
-	ret = of_property_read_u32(np, "video-ports", &video);
-	if (ret == 0) {
-		priv->vip_cntrl_0 = video >> 16;
-		priv->vip_cntrl_1 = video >> 8;
-		priv->vip_cntrl_2 = video;
+	/* get the device tree parameters */
+	if (np) {
+		of_port = of_get_child_by_name(np, "port");
+		if (of_port) {				/* graph of ports */
+			of_node_put(of_port);
+			ret = tda998x_parse_ports(priv, np);
+			if (ret < 0)
+				goto fail;
+
+			/* initialize the default audio configuration */
+			if (priv->audio.ports[0]) {
+				priv->params.audio_cfg = priv->audio.ports[0];
+				priv->params.audio_format =
+						priv->audio.port_types[0];
+				priv->params.audio_clk_cfg =
+					priv->params.audio_format ==
+							AFMT_SPDIF ? 0 : 1;
+			}
+		} else {
+
+			/* optional video properties */
+			ret = of_property_read_u32(np, "video-ports", &video);
+			if (ret == 0) {
+				priv->vip_cntrl_0 = video >> 16;
+				priv->vip_cntrl_1 = video >> 8;
+				priv->vip_cntrl_2 = video;
+			}
+		}
 	}
 
 	return 0;
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
new file mode 100644
index 0000000..487a809
--- /dev/null
+++ b/include/sound/tda998x.h
@@ -0,0 +1,8 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+struct tda998x_audio_s {
+	u8 ports[2];			/* AP value */
+	u8 port_types[2];		/* AFMT_xxx */
+};
+#endif
-- 
2.1.4

--
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

[PATCH v13 2/3] drm/i2c: tda998x: Change drvdata for audio extension

[Jean-Francois Moine]

The device drvdata is used for component bind, but points to the
encoder/connector structure which is hidden from the slave encoder.
For audio extension, the slave encoder private data must be accessible,
so, this patch changes drvdata to the slave encoder private data and
sets it in case of slave encoder use.

Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b0a730a..655ebb0 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1453,6 +1453,8 @@ static int tda998x_encoder_init(struct i2c_client *client,
 	encoder_slave->slave_priv = priv;
 	encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
 
+	dev_set_drvdata(&client->dev, priv);
+
 	return 0;
 }
 
@@ -1580,7 +1582,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (!priv)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, priv);
+	dev_set_drvdata(dev, &priv->base);
 
 	if (dev->of_node)
 		crtcs = drm_of_find_possible_crtcs(drm, dev->of_node);
@@ -1639,7 +1641,9 @@ err_encoder:
 static void tda998x_unbind(struct device *dev, struct device *master,
 			   void *data)
 {
-	struct tda998x_priv2 *priv = dev_get_drvdata(dev);
+	struct tda998x_priv *priv_s = dev_get_drvdata(dev);
+	struct tda998x_priv2 *priv =
+			container_of(priv_s, struct tda998x_priv2, base);
 
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
-- 
2.1.4

--
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

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

[Jean-Francois Moine]

The tda998x CODEC maintains the audio constraints according to
the HDMI device parameters (EDID) and sets dynamically the input ports
in the TDA998x I2C driver on start/stop audio streaming.

Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
---
 drivers/gpu/drm/i2c/tda998x_drv.c |  80 +++++++++++++++++++++++
 include/sound/tda998x.h           |  15 +++++
 sound/soc/codecs/Kconfig          |   6 ++
 sound/soc/codecs/Makefile         |   2 +
 sound/soc/codecs/tda998x.c        | 132 ++++++++++++++++++++++++++++++++++++++
 5 files changed, 235 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 655ebb0..90a508f 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -758,6 +758,66 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 	tda998x_write_aif(priv, p);
 }
 
+#if IS_ENABLED(CONFIG_SND_SOC_TDA998X)
+/* tda998x audio codec interface */
+
+/* return the ELD to the CODEC */
+static u8 *tda998x_get_eld(struct device *dev)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct drm_encoder *encoder = priv->encoder;
+	struct drm_device *drm = encoder->dev;
+	struct drm_connector *connector;
+
+	if (!priv->is_hdmi_sink
+	 || !encoder->crtc)
+		return NULL;
+
+	list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
+		if (connector->encoder == encoder)
+			return connector->eld;
+	}
+	return NULL;
+}
+
+/* switch the audio port and initialize the audio parameters for streaming */
+static int tda998x_set_audio_input(struct device *dev,
+				int port_index,
+				unsigned sample_rate)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+	struct tda998x_encoder_params *p = &priv->params;
+
+	if (!priv->encoder->crtc)
+		return -ENODEV;
+
+	/* if no port, just disable the audio port */
+	if (port_index == PORT_NONE) {
+		reg_write(priv, REG_ENA_AP, 0);
+		return 0;
+	}
+
+	/* if same audio parameters, just enable the audio port */
+	if (p->audio_cfg == priv->audio.ports[port_index] &&
+	    p->audio_sample_rate == sample_rate) {
+		reg_write(priv, REG_ENA_AP, p->audio_cfg);
+		return 0;
+	}
+
+	p->audio_format = priv->audio.port_types[port_index];
+	p->audio_clk_cfg = p->audio_format == AFMT_SPDIF ? 0 : 1;
+	p->audio_cfg = priv->audio.ports[port_index];
+	p->audio_sample_rate = sample_rate;
+	tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
+	return 0;
+}
+
+static struct tda998x_ops_s tda998x_codec_ops = {
+	.get_eld = tda998x_get_eld,
+	.set_audio_input = tda998x_set_audio_input,
+};
+#endif /* SND_SOC */
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_set_config(struct tda998x_priv *priv,
@@ -1123,6 +1183,12 @@ tda998x_encoder_get_modes(struct tda998x_priv *priv,
 	drm_mode_connector_update_edid_property(connector, edid);
 	n = drm_add_edid_modes(connector, edid);
 	priv->is_hdmi_sink = drm_detect_hdmi_monitor(edid);
+
+#if IS_ENABLED(CONFIG_SND_SOC_TDA998X)
+	if (priv->is_hdmi_sink)
+		drm_edid_to_eld(connector, edid);
+#endif
+
 	kfree(edid);
 
 	return n;
@@ -1158,6 +1224,10 @@ static void tda998x_destroy(struct tda998x_priv *priv)
 		cancel_delayed_work_sync(&priv->dwork);
 	}
 
+#if IS_ENABLED(CONFIG_SND_SOC_TDA998X)
+	if (priv->audio.ports[0])
+		tda9998x_codec_unregister(&priv->hdmi->dev);
+#endif
 	i2c_unregister_device(priv->cec);
 }
 
@@ -1294,6 +1364,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 */
@@ -1407,6 +1480,13 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 				priv->params.audio_clk_cfg =
 					priv->params.audio_format ==
 							AFMT_SPDIF ? 0 : 1;
+
+#if IS_ENABLED(CONFIG_SND_SOC_TDA998X)
+				/* and create the audio CODEC */
+				tda9998x_codec_register(&client->dev,
+							&priv->audio,
+							&tda998x_codec_ops);
+#endif
 			}
 		} else {
 
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
index 487a809..8e89db4 100644
--- a/include/sound/tda998x.h
+++ b/include/sound/tda998x.h
@@ -1,8 +1,23 @@
 #ifndef SND_TDA998X_H
 #define SND_TDA998X_H
 
+/* port index for audio stream stop */
+#define PORT_NONE (-1)
+
 struct tda998x_audio_s {
 	u8 ports[2];			/* AP value */
 	u8 port_types[2];		/* AFMT_xxx */
 };
+
+struct tda998x_ops_s {
+	u8 *(*get_eld)(struct device *dev);
+	int (*set_audio_input)(struct device *dev,
+			int port_index,
+			unsigned sample_rate);
+};
+
+int tda9998x_codec_register(struct device *dev,
+			struct tda998x_audio_s *tda998x_audio_i,
+			struct tda998x_ops_s *tda998x_ops);
+void tda9998x_codec_unregister(struct device *dev);
 #endif
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index efaafce..08b73bd 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -105,6 +105,7 @@ config SND_SOC_ALL_CODECS
 	select SND_SOC_TAS2552 if I2C
 	select SND_SOC_TAS5086 if I2C
 	select SND_SOC_TAS571X if I2C
+	select SND_SOC_TDA998X if DRM_I2C_NXP_TDA998X
 	select SND_SOC_TFA9879 if I2C
 	select SND_SOC_TLV320AIC23_I2C if I2C
 	select SND_SOC_TLV320AIC23_SPI if SPI_MASTER
@@ -622,6 +623,11 @@ config SND_SOC_TAS571X
 	tristate "Texas Instruments TAS5711/TAS5717/TAS5719 power amplifiers"
 	depends on I2C
 
+config SND_SOC_TDA998X
+	def_tristate y
+	select SND_PCM_ELD
+	depends on DRM_I2C_NXP_TDA998X
+
 config SND_SOC_TFA9879
 	tristate "NXP Semiconductors TFA9879 amplifier"
 	depends on I2C
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index cf160d9..819d689 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -108,6 +108,7 @@ snd-soc-sta529-objs := sta529.o
 snd-soc-stac9766-objs := stac9766.o
 snd-soc-tas5086-objs := tas5086.o
 snd-soc-tas571x-objs := tas571x.o
+snd-soc-tda998x-objs := tda998x.o
 snd-soc-tfa9879-objs := tfa9879.o
 snd-soc-tlv320aic23-objs := tlv320aic23.o
 snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o
@@ -292,6 +293,7 @@ obj-$(CONFIG_SND_SOC_STAC9766)	+= snd-soc-stac9766.o
 obj-$(CONFIG_SND_SOC_TAS2552)	+= snd-soc-tas2552.o
 obj-$(CONFIG_SND_SOC_TAS5086)	+= snd-soc-tas5086.o
 obj-$(CONFIG_SND_SOC_TAS571X)	+= snd-soc-tas571x.o
+obj-$(CONFIG_SND_SOC_TDA998X)	+= snd-soc-tda998x.o
 obj-$(CONFIG_SND_SOC_TFA9879)	+= snd-soc-tfa9879.o
 obj-$(CONFIG_SND_SOC_TLV320AIC23)	+= snd-soc-tlv320aic23.o
 obj-$(CONFIG_SND_SOC_TLV320AIC23_I2C)	+= snd-soc-tlv320aic23-i2c.o
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
new file mode 100644
index 0000000..f588a75
--- /dev/null
+++ b/sound/soc/codecs/tda998x.c
@@ -0,0 +1,132 @@
+/*
+ * ALSA SoC TDA998x CODEC
+ *
+ * Copyright (C) 2015 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 <linux/of.h>
+#include <linux/of_device.h>
+#include <sound/pcm_drm_eld.h>
+#include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
+
+/* functions in tda998x_drv */
+static struct tda998x_ops_s *tda998x_ops;
+
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+			struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	u8 *eld;
+
+	eld = tda998x_ops->get_eld(dai->dev);
+	if (!eld)
+		return -ENODEV;
+	return snd_pcm_hw_constraint_eld(runtime, eld);
+}
+
+/* ask the HDMI transmitter to activate the audio input port */
+static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params,
+				   struct snd_soc_dai *dai)
+{
+	return tda998x_ops->set_audio_input(dai->dev, dai->id,
+					params_rate(params));
+}
+
+static void tda998x_codec_shutdown(struct snd_pcm_substream *substream,
+				   struct snd_soc_dai *dai)
+{
+	tda998x_ops->set_audio_input(dai->dev, PORT_NONE, 0);
+}
+
+static const struct snd_soc_dai_ops tda998x_codec_ops = {
+	.startup = tda998x_codec_startup,
+	.hw_params = tda998x_codec_hw_params,
+	.shutdown = tda998x_codec_shutdown,
+};
+
+#define TDA998X_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+			SNDRV_PCM_FMTBIT_S20_3LE | \
+			SNDRV_PCM_FMTBIT_S24_LE | \
+			SNDRV_PCM_FMTBIT_S32_LE)
+
+static struct snd_soc_dai_driver tda998x_dai_i2s = {
+	.name = "i2s-hifi",
+	.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 = &tda998x_codec_ops,
+};
+
+static const struct snd_soc_dapm_widget tda998x_widgets[] = {
+	SND_SOC_DAPM_OUTPUT("hdmi-out"),
+};
+static const struct snd_soc_dapm_route tda998x_routes[] = {
+	{ "hdmi-out", NULL, "HDMI I2S Playback" },
+	{ "hdmi-out", NULL, "HDMI SPDIF Playback" },
+};
+
+static struct snd_soc_codec_driver tda998x_codec_drv = {
+	.dapm_widgets = tda998x_widgets,
+	.num_dapm_widgets = ARRAY_SIZE(tda998x_widgets),
+	.dapm_routes = tda998x_routes,
+	.num_dapm_routes = ARRAY_SIZE(tda998x_routes),
+	.ignore_pmdown_time = true,
+};
+
+int tda9998x_codec_register(struct device *dev,
+			struct tda998x_audio_s *tda998x_audio,
+			struct tda998x_ops_s *tda998x_ops_i)
+{
+	struct snd_soc_dai_driver *dais, *p_dai;
+	int i, ndais;
+
+	tda998x_ops = tda998x_ops_i;
+
+	/* build the DAIs */
+	for (ndais = 0; ndais < ARRAY_SIZE(tda998x_audio->ports); ndais++) {
+		if (!tda998x_audio->ports[ndais])
+			break;
+	}
+	dais = devm_kzalloc(dev, sizeof(*dais) * ndais, GFP_KERNEL);
+	if (!dais)
+		return -ENOMEM;
+	for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
+		memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));
+		p_dai->id = i;
+		if (tda998x_audio->port_types[i] == AFMT_SPDIF) {
+			p_dai->name = "spdif-hifi";
+			p_dai->playback.stream_name = "HDMI SPDIF Playback";
+			p_dai->playback.channels_max = 2;
+			p_dai->playback.rate_min = 22050;
+		}
+	}
+
+	return snd_soc_register_codec(dev,
+				&tda998x_codec_drv,
+				dais, ndais);
+}
+EXPORT_SYMBOL(tda9998x_codec_register);
+
+void tda9998x_codec_unregister(struct device *dev)
+{
+	snd_soc_unregister_codec(dev);
+}
+EXPORT_SYMBOL(tda9998x_codec_unregister);
+
+MODULE_AUTHOR("Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>");
+MODULE_DESCRIPTION("TDA998X CODEC");
+MODULE_LICENSE("GPL");
-- 
2.1.4

--
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 v13 3/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Fri, May 08, 2015 at 10:41:12AM +0200, Jean-Francois Moine wrote:

> +
> +	if (!priv->is_hdmi_sink
> +	 || !encoder->crtc)
> +		return NULL;

That's weird indentation.

> +	list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
> +		if (connector->encoder == encoder)
> +			return connector->eld;
> +	}

What guarantees that connector->eld stays valid after it's returned?

> +struct tda998x_ops_s {
> +	u8 *(*get_eld)(struct device *dev);

Why _ops_s - what does the _s mean?  If it's sound perhaps it makes
sense to spell it out so people aren't guessing.

> +int tda9998x_codec_register(struct device *dev,
> +			struct tda998x_audio_s *tda998x_audio_i,
> +			struct tda998x_ops_s *tda998x_ops);
> +void tda9998x_codec_unregister(struct device *dev);

I'd expect these to be internal to the DRM driver.

> +config SND_SOC_TDA998X
> +	def_tristate y
> +	select SND_PCM_ELD
> +	depends on DRM_I2C_NXP_TDA998X
> +

def_tristate y?  Why?

> +/* functions in tda998x_drv */
> +static struct tda998x_ops_s *tda998x_ops;

I'd expect this to be stored in the driver data rather than a static
global, what if a system has two HDMI outputs?

> +static int tda998x_codec_startup(struct snd_pcm_substream *substream,
> +                       struct snd_soc_dai *dai)
> +{
> +       struct snd_pcm_runtime *runtime = substream->runtime;
> +       u8 *eld;
> +
> +       eld = tda998x_ops->get_eld(dai->dev);
> +       if (!eld)
> +               return -ENODEV;
> +       return snd_pcm_hw_constraint_eld(runtime, eld);
> +}

Do we really need a device specific mechanism for fishing the ELD out of
the graphics code?  I'd have expected this to be more generic.

> +/* ask the HDMI transmitter to activate the audio input port */
> +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
> +				   struct snd_pcm_hw_params *params,
> +				   struct snd_soc_dai *dai)
> +{
> +	return tda998x_ops->set_audio_input(dai->dev, dai->id,
> +					params_rate(params));
> +}

The set_audio_input() function doesn't appear to have anything that
checks if the device is busy before enabling things, what happens if the
user tries to switch between I2S and S/PDIF?  It looks like only one DAI
can be active at once.

> +	for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
> +		memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));
> +		p_dai->id = i;
> +		if (tda998x_audio->port_types[i] == AFMT_SPDIF) {
> +			p_dai->name = "spdif-hifi";
> +			p_dai->playback.stream_name = "HDMI SPDIF Playback";
> +			p_dai->playback.channels_max = 2;
> +			p_dai->playback.rate_min = 22050;
> +		}
> +	}

It would be a bit clearer if the template were just a template and this
copying initialised both I2S and S/PDIF specific settings.

> +	return snd_soc_register_codec(dev,
> +				&tda998x_codec_drv,
> +				dais, ndais);
> +}
> +EXPORT_SYMBOL(tda9998x_codec_register);

ASoC is all EXPORT_SYMBOL_GPL, you shouldn't reexport functionality with
plain EXPORT_SYMBOL.

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

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

[Jean-Francois Moine]

On Mon, 20 Jul 2015 19:06:06 +0100
Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> On Fri, May 08, 2015 at 10:41:12AM +0200, Jean-Francois Moine wrote:
> 
> > +
> > +	if (!priv->is_hdmi_sink
> > +	 || !encoder->crtc)
> > +		return NULL;
> 
> That's weird indentation.

But the conditions are aligned... This sequence will be removed.

> > +	list_for_each_entry(connector, &drm->mode_config.connector_list, head) {
> > +		if (connector->encoder == encoder)
> > +			return connector->eld;
> > +	}
> 
> What guarantees that connector->eld stays valid after it's returned?

You are right, the ELD content may change. I will use a pointer to a
stable content.

> > +struct tda998x_ops_s {
> > +	u8 *(*get_eld)(struct device *dev);
> 
> Why _ops_s - what does the _s mean?  If it's sound perhaps it makes
> sense to spell it out so people aren't guessing.

I will remove it.

> > +int tda9998x_codec_register(struct device *dev,
> > +			struct tda998x_audio_s *tda998x_audio_i,
> > +			struct tda998x_ops_s *tda998x_ops);
> > +void tda9998x_codec_unregister(struct device *dev);
> 
> I'd expect these to be internal to the DRM driver.

I don't see where. What is your idea?

> > +config SND_SOC_TDA998X
> > +	def_tristate y
> > +	select SND_PCM_ELD
> > +	depends on DRM_I2C_NXP_TDA998X
> > +
> 
> def_tristate y?  Why?

The TDA998x CODEC is always generated when the DRM TDA998x is generated
and when audio is wanted.
Its type, built-in or module, depends on the TDA998x driver type.

> > +/* functions in tda998x_drv */
> > +static struct tda998x_ops_s *tda998x_ops;
> 
> I'd expect this to be stored in the driver data rather than a static
> global, what if a system has two HDMI outputs?

Each TDA998x has only one HDMI output and there is only one driver.

I will put the pointer to the HDMI driver in the device private area.

> > +static int tda998x_codec_startup(struct snd_pcm_substream *substream,
> > +                       struct snd_soc_dai *dai)
> > +{
> > +       struct snd_pcm_runtime *runtime = substream->runtime;
> > +       u8 *eld;
> > +
> > +       eld = tda998x_ops->get_eld(dai->dev);
> > +       if (!eld)
> > +               return -ENODEV;
> > +       return snd_pcm_hw_constraint_eld(runtime, eld);
> > +}
> 
> Do we really need a device specific mechanism for fishing the ELD out of
> the graphics code?  I'd have expected this to be more generic.

I will put the ELD in the private data of the DRM driver.

> > +/* ask the HDMI transmitter to activate the audio input port */
> > +static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
> > +				   struct snd_pcm_hw_params *params,
> > +				   struct snd_soc_dai *dai)
> > +{
> > +	return tda998x_ops->set_audio_input(dai->dev, dai->id,
> > +					params_rate(params));
> > +}
> 
> The set_audio_input() function doesn't appear to have anything that
> checks if the device is busy before enabling things, what happens if the
> user tries to switch between I2S and S/PDIF?  It looks like only one DAI
> can be active at once.

Right. I will check this double streaming.

> > +	for (i = 0, p_dai = dais; i < ndais ; i++, p_dai++) {
> > +		memcpy(p_dai, &tda998x_dai_i2s, sizeof(*p_dai));
> > +		p_dai->id = i;
> > +		if (tda998x_audio->port_types[i] == AFMT_SPDIF) {
> > +			p_dai->name = "spdif-hifi";
> > +			p_dai->playback.stream_name = "HDMI SPDIF Playback";
> > +			p_dai->playback.channels_max = 2;
> > +			p_dai->playback.rate_min = 22050;
> > +		}
> > +	}
> 
> It would be a bit clearer if the template were just a template and this
> copying initialised both I2S and S/PDIF specific settings.

OK, I will change this.

> > +	return snd_soc_register_codec(dev,
> > +				&tda998x_codec_drv,
> > +				dais, ndais);
> > +}
> > +EXPORT_SYMBOL(tda9998x_codec_register);
> 
> ASoC is all EXPORT_SYMBOL_GPL, you shouldn't reexport functionality with
> plain EXPORT_SYMBOL.

Sorry, bug of mine.

-- 
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 v13 3/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Tue, Jul 28, 2015 at 12:19:45PM +0200, Jean-Francois Moine wrote:
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:

> > > +int tda9998x_codec_register(struct device *dev,
> > > +			struct tda998x_audio_s *tda998x_audio_i,
> > > +			struct tda998x_ops_s *tda998x_ops);
> > > +void tda9998x_codec_unregister(struct device *dev);

> > I'd expect these to be internal to the DRM driver.

> I don't see where. What is your idea?

What I said above - put them in the DRM driver.  They're just
registering a device IIRC.

> > > +config SND_SOC_TDA998X
> > > +	def_tristate y
> > > +	select SND_PCM_ELD
> > > +	depends on DRM_I2C_NXP_TDA998X

> > def_tristate y?  Why?

> The TDA998x CODEC is always generated when the DRM TDA998x is generated
> and when audio is wanted.
> Its type, built-in or module, depends on the TDA998x driver type.

Just make this like any other driver with no default specified.

> > > +/* functions in tda998x_drv */
> > > +static struct tda998x_ops_s *tda998x_ops;

> > I'd expect this to be stored in the driver data rather than a static
> > global, what if a system has two HDMI outputs?

> Each TDA998x has only one HDMI output and there is only one driver.

Someone might put two chips on the board.

> > > +static int tda998x_codec_startup(struct snd_pcm_substream *substream,
> > > +                       struct snd_soc_dai *dai)
> > > +{
> > > +       struct snd_pcm_runtime *runtime = substream->runtime;
> > > +       u8 *eld;
> > > +
> > > +       eld = tda998x_ops->get_eld(dai->dev);
> > > +       if (!eld)
> > > +               return -ENODEV;
> > > +       return snd_pcm_hw_constraint_eld(runtime, eld);
> > > +}

> > Do we really need a device specific mechanism for fishing the ELD out of
> > the graphics code?  I'd have expected this to be more generic.

> I will put the ELD in the private data of the DRM driver.

That's not the issue here - the issue is that the need to get the ELD
out of the video subsystem is not specific to this driver, it's pretty
common and something that it seems we should factor out.

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

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

[Jean-Francois Moine]

On Tue, 28 Jul 2015 11:24:10 +0100
Mark Brown <broonie@kernel.org> wrote:

> On Tue, Jul 28, 2015 at 12:19:45PM +0200, Jean-Francois Moine wrote:
> > Mark Brown <broonie@kernel.org> wrote:
> 
> > > > +int tda9998x_codec_register(struct device *dev,
> > > > +			struct tda998x_audio_s *tda998x_audio_i,
> > > > +			struct tda998x_ops_s *tda998x_ops);
> > > > +void tda9998x_codec_unregister(struct device *dev);
> 
> > > I'd expect these to be internal to the DRM driver.
> 
> > I don't see where. What is your idea?
> 
> What I said above - put them in the DRM driver.  They're just
> registering a device IIRC.

There is no CODEC device. The HDMI device supports both audio and video.

> > > > +config SND_SOC_TDA998X
> > > > +	def_tristate y
> > > > +	select SND_PCM_ELD
> > > > +	depends on DRM_I2C_NXP_TDA998X
> 
> > > def_tristate y?  Why?
> 
> > The TDA998x CODEC is always generated when the DRM TDA998x is generated
> > and when audio is wanted.
> > Its type, built-in or module, depends on the TDA998x driver type.
> 
> Just make this like any other driver with no default specified.

If I understand correctly, you want that the user might choose to use
the HDMI for video only and to have audio by some other mean.
No problem.

> > > > +/* functions in tda998x_drv */
> > > > +static struct tda998x_ops_s *tda998x_ops;
> 
> > > I'd expect this to be stored in the driver data rather than a static
> > > global, what if a system has two HDMI outputs?
> 
> > Each TDA998x has only one HDMI output and there is only one driver.
> 
> Someone might put two chips on the board.

The pointer was from the tda998x codec to the tda998x driver.
There might be any number of chips on the board.

> > > > +static int tda998x_codec_startup(struct snd_pcm_substream *substream,
> > > > +                       struct snd_soc_dai *dai)
> > > > +{
> > > > +       struct snd_pcm_runtime *runtime = substream->runtime;
> > > > +       u8 *eld;
> > > > +
> > > > +       eld = tda998x_ops->get_eld(dai->dev);
> > > > +       if (!eld)
> > > > +               return -ENODEV;
> > > > +       return snd_pcm_hw_constraint_eld(runtime, eld);
> > > > +}
> 
> > > Do we really need a device specific mechanism for fishing the ELD out of
> > > the graphics code?  I'd have expected this to be more generic.
> 
> > I will put the ELD in the private data of the DRM driver.
> 
> That's not the issue here - the issue is that the need to get the ELD
> out of the video subsystem is not specific to this driver, it's pretty
> common and something that it seems we should factor out.

Yes, but how?

The EDID arrives in the DRM connector when video starts. The built ELD
may be stored either in the connector itself (default), in the encoder
(TDA998x device) or in some DRM/encoder/connector private data.

On the audio side, the CODEC is supported by a driver which is either a
CODEC driver (as in the dummy HDMI codec) or the DRM encoder (TDA998x
device).

You may note that, in DRM, there is no relation between the encoder and
the connector except at video startup time, and no relation between the
DRM connector and the audio CODEC (no CODEC information is returned on
CODEC creation and the CODEC has no private data).

I tried many solutions to solve this problem, and the one of may latest
patchset (v14) seems the simpleset: set the audio/video common part at
the beginning of the TDA998x (HDMI) private data.

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

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

[Russell King - ARM Linux]

On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote:
> The EDID arrives in the DRM connector when video starts. The built ELD
> may be stored either in the connector itself (default), in the encoder
> (TDA998x device) or in some DRM/encoder/connector private data.

The ELD is stored in the DRM connector, and there _should_ be a system
of locking which ensures that you can get at that information safely.

The ELD is only updated when the connector's get_modes() method is called,
and the driver itself calls drm_edid_to_eld().  This all happens under
the drm_device's struct_mutex.

So, to safely read the ELD from outside DRM, you need to take the top-level
drm_device's struct_mutex.  That could be fraught, as that's quite a big
lock, so an alternative would be to introduce an 'eld' lock to the driver,
which protects the call to drm_edid_to_eld() and the reading of the ELD.

However, that doesn't solve the problem of passing the data to an audio
driver.  What's needed there is a notification system which allows a video
driver to broadcast HDMI events such as:

* connected
* new EDID available
* new ELD available
* disconnected

With such a system, different components driving the facilities of a HDMI
connector can make decisions on what to do - which not only includes things
like the audio driver, but also a driver for the CEC interface (which also
needs to see the EDID to get at its "address".)  This can be done in a safe
manner as the HDMI video driver would have control over the generation of
these messages.

The point that Mark is trying to get you to see is that this problem is
not specific to TDA998x.  The same problem exists for many other HDMI
interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware
mechanism of passing the ELD data from the video driver, through the
hardware to the audio driver.)

It needs solving in a driver-agnostic way, and the normal way that happens
is when someone comes along, wanting to add support in that area, they get
to do the hard work to propose that infrastructure.

> You may note that, in DRM, there is no relation between the encoder and
> the connector except at video startup time, and no relation between the
> DRM connector and the audio CODEC (no CODEC information is returned on
> CODEC creation and the CODEC has no private data).

In the case of the TDA998x, there is a 1:1 fixed relationship between the
connector and the encoder.  The connector part can't be used with any other
encoder, and the encoder part can't be used with any other connector.  The
same is true of many other HDMI implementations (such as the Synopsis
Designware HDMI interface found on iMX6 and Rockchip.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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 v13 3/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Takashi Iwai]

On Tue, 28 Jul 2015 15:53:58 +0200,
Russell King - ARM Linux wrote:
> 
> On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote:
> > The EDID arrives in the DRM connector when video starts. The built ELD
> > may be stored either in the connector itself (default), in the encoder
> > (TDA998x device) or in some DRM/encoder/connector private data.
> 
> The ELD is stored in the DRM connector, and there _should_ be a system
> of locking which ensures that you can get at that information safely.
> 
> The ELD is only updated when the connector's get_modes() method is called,
> and the driver itself calls drm_edid_to_eld().  This all happens under
> the drm_device's struct_mutex.
> 
> So, to safely read the ELD from outside DRM, you need to take the top-level
> drm_device's struct_mutex.  That could be fraught, as that's quite a big
> lock, so an alternative would be to introduce an 'eld' lock to the driver,
> which protects the call to drm_edid_to_eld() and the reading of the ELD.
> 
> However, that doesn't solve the problem of passing the data to an audio
> driver.  What's needed there is a notification system which allows a video
> driver to broadcast HDMI events such as:
> 
> * connected
> * new EDID available
> * new ELD available
> * disconnected
> 
> With such a system, different components driving the facilities of a HDMI
> connector can make decisions on what to do - which not only includes things
> like the audio driver, but also a driver for the CEC interface (which also
> needs to see the EDID to get at its "address".)  This can be done in a safe
> manner as the HDMI video driver would have control over the generation of
> these messages.
> 
> The point that Mark is trying to get you to see is that this problem is
> not specific to TDA998x.  The same problem exists for many other HDMI
> interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware
> mechanism of passing the ELD data from the video driver, through the
> hardware to the audio driver.)

FWIW, we're currently discussing about extending the i915/hda
component binding so that the audio driver gets a notification and
queries the ELD via callbacks instead of the flaky hardware access.

It'd be best if we have a common infrastructure for that, of course.
But currently it's a start, just a bit step forward there.


Takashi

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

[Russell King - ARM Linux]

On Tue, Jul 28, 2015 at 03:59:45PM +0200, Takashi Iwai wrote:
> FWIW, we're currently discussing about extending the i915/hda
> component binding so that the audio driver gets a notification and
> queries the ELD via callbacks instead of the flaky hardware access.
> 
> It'd be best if we have a common infrastructure for that, of course.
> But currently it's a start, just a bit step forward there.

Okay, so it sounds like i915/hda also needs this.

I think in addition to what I said above, we need whatever provides the
notification system to generate state notifications for new "listeners"
that would otherwise be unaware of the current state.

I'm thinking at the moment that something along these lines to go into
drivers/video/hdmi.c / include/linux/hdmi.h:

struct hdmi_state {
	struct mutex mutex;
	struct raw_notifier_head head;
	bool connected;
	bool edid_valid;
	u8 edid[MAX_EDID_SIZE];
};

enum {
	HDMI_CONNECTED,
	HDMI_DISCONNECTED,
	HDMI_NEW_EDID,
};

int hdmi_register_notifier(struct notifier_block *nb)
{
	struct hdmi_state *state = ...;
	int ret;

	mutex_lock(&state->mutex);
	if (state->connected) {
		nb->notifier_call(nb, HDMI_CONNECTED, NULL);
		if (state->edid_valid)
			nb->notifier_call(nb, HDMI_NEW_EDID, state->edid);
	} else {
		nb->notifier_call(nb, HDMI_DISCONNECTED, NULL);
	}
	ret = raw_notifier_chain_register(&state->head, nb);
	mutex_unlock(&state->mutex);

	return ret;
}

int hdmi_unregister_notifier(struct notifier_block *nb)
{
	struct hdmi_state *state = ...;
	int ret;

	mutex_lock(&state->mutex);
	ret = raw_notifier_chain_unregister(&state->head, nb);
	mutex_unlock(&state->mutex);

	return ret;
}

void hdmi_event_connect(struct device *dev)
{
	struct hdmi_state *state = lookup_state_from_dev(dev);

	mutex_lock(&state->mutex);
	state->connected = true;
	raw_notifier_call_chain(&state->head, HDMI_CONNECTED, NULL);
	mutex_unlock(&state->mutex);
}

void hdmi_event_disconnect(struct device *dev)
{
	struct hdmi_state *state = lookup_state_from_dev(dev);

	mutex_lock(&state->mutex);
	state->connected = false;
	state->valid_edid = false;
	raw_notifier_call_chain(&state->head, HDMI_DISCONNECTED, NULL);
	mutex_unlock(&state->mutex);
}

int hdmi_event_new_edid(struct device *dev, const void *edid, size_t size)
{
	struct hdmi_state *state = lookup_state_from_dev(dev);

	if (size > sizeof(state->edid))
		return -EINVAL;

	mutex_lock(&state->mutex);
	state->valid_edid = true;
	memcpy(state->edid, edid, size);
	raw_notifier_call_chain(&state->head, HDMI_NEW_EDID, state->edid);
	mutex_unlock(&state->mutex);

	return 0;
}

The problems here are: how to go from a struct device to the state (iow,
the implementation of lookup_state_from_dev()) and how to convey on the
client side which 'state' to attach to.  I'd rather not involve DT at
this level: DT is not the world's only firmware system, we have to cater
for x86 too here, so we need something that is firmware agnostic.

This is something I've just banged out in this email...

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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 v13 3/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

On Tue, 28 Jul 2015 14:53:58 +0100
Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:

> On Tue, Jul 28, 2015 at 03:23:29PM +0200, Jean-Francois Moine wrote:
> > The EDID arrives in the DRM connector when video starts. The built ELD
> > may be stored either in the connector itself (default), in the encoder
> > (TDA998x device) or in some DRM/encoder/connector private data.
> 
> The ELD is stored in the DRM connector, and there _should_ be a system
> of locking which ensures that you can get at that information safely.
> 
> The ELD is only updated when the connector's get_modes() method is called,
> and the driver itself calls drm_edid_to_eld().  This all happens under
> the drm_device's struct_mutex.
> 
> So, to safely read the ELD from outside DRM, you need to take the top-level
> drm_device's struct_mutex.  That could be fraught, as that's quite a big
> lock, so an alternative would be to introduce an 'eld' lock to the driver,
> which protects the call to drm_edid_to_eld() and the reading of the ELD.

I think that my solution should work:
- the CODEC uses a pointer in the driver private data to the ELD.
- when get_modes() is called, this pointer is reset to NULL.
  (no audio streaming is permitted).
- this function reads the EDID and this asks for hardware exchanges
  with the HDMI device.
- the EDID is then translated to ELD and the ELD pointer is set
  (audio streaming is permitted again).

Then, if audio streaming is started before get_modes() is called, the
memory treatment of the ELD may be safely done during the harware
exchanges.

> However, that doesn't solve the problem of passing the data to an audio
> driver.  What's needed there is a notification system which allows a video
> driver to broadcast HDMI events such as:
> 
> * connected
> * new EDID available
> * new ELD available
> * disconnected

My patch just wants to offer basic audio to the TDA998x.
Especially, the display device state is of no importance: audio
streaming may continue while there is no connected device.

> With such a system, different components driving the facilities of a HDMI
> connector can make decisions on what to do - which not only includes things
> like the audio driver, but also a driver for the CEC interface (which also
> needs to see the EDID to get at its "address".)  This can be done in a safe
> manner as the HDMI video driver would have control over the generation of
> these messages.
> 
> The point that Mark is trying to get you to see is that this problem is
> not specific to TDA998x.  The same problem exists for many other HDMI
> interfaces (except maybe Intel's i9x5/HDA stuff which has a hardware
> mechanism of passing the ELD data from the video driver, through the
> hardware to the audio driver.)

I know that, but I don't know enough about all the DRM and ASoC drivers
to propose a global mechanism.

> It needs solving in a driver-agnostic way, and the normal way that happens
> is when someone comes along, wanting to add support in that area, they get
> to do the hard work to propose that infrastructure.

OK. I'll stop here.

> > You may note that, in DRM, there is no relation between the encoder and
> > the connector except at video startup time, and no relation between the
> > DRM connector and the audio CODEC (no CODEC information is returned on
> > CODEC creation and the CODEC has no private data).
> 
> In the case of the TDA998x, there is a 1:1 fixed relationship between the
> connector and the encoder.  The connector part can't be used with any other
> encoder, and the encoder part can't be used with any other connector.  The
> same is true of many other HDMI implementations (such as the Synopsis
> Designware HDMI interface found on iMX6 and Rockchip.)

But there is no direct link (pointer) between the encoder and the
connector.

-- 
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 v13 3/3] ASoC: tda998x: add a codec to the HDMI transmitter

[Russell King - ARM Linux]

On Tue, Jul 28, 2015 at 04:39:12PM +0200, Jean-Francois Moine wrote:
> On Tue, 28 Jul 2015 14:53:58 +0100
> Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> > In the case of the TDA998x, there is a 1:1 fixed relationship between the
> > connector and the encoder.  The connector part can't be used with any other
> > encoder, and the encoder part can't be used with any other connector.  The
> > same is true of many other HDMI implementations (such as the Synopsis
> > Designware HDMI interface found on iMX6 and Rockchip.)
> 
> But there is no direct link (pointer) between the encoder and the
> connector.

There's no direct link from an encoder to a connector because the DRM
model allows a single encoder to be associated with multiple connectors.
Adding a link from the encoder to a connector breaks that ability (an
encoder would then need an array of connectors.)

If we kill the old drm_slave_encoder support in TDA998x, then this would
solve the problem - we can then get at the TDA998x's drm_connector easily
as it then becomes part of TDA998x's private data.

However, this doesn't matter.  All that we need from the DRM side of the
TDA998x is to know is the EDID or ELD.  That can be broadcast via a
notification (using something like the code I illustrated in a follow up
email) to any listeners, whether they be an audio driver or a CEC bus
driver.  The audio driver may not care about HDMI connect/disconnect
events, but that's no reason not to include them.  Some consumers of
these events need to know (CEC certainly needs to know.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
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