Implement generic ASoC HDMI codec and use it in tda998x

Add IEC958 channel status helper that gets the audio properties from snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to produce the channel status bits already in audio stream configuration phase.

Signed-off-by Jyri Sarha <[email protected]>

---
 include/sound/pcm_iec958.h |  2 ++
 sound/core/pcm_iec958.c    | 52 +++++++++++++++++++++++++++++++---------------

 2 files changed, 37 insertions(+), 17 deletions(-)

include/sound/pcm_iec958.h +2 -0

index 0eed397..36f023a 100644
@@ -6,4 +6,6 @@
 int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	size_t len);
 
+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
+					     u8 *cs, size_t len);
 #endif

sound/core/pcm_iec958.c +35 -17

index 36b2d7a..c9f8b66 100644
@@ -9,30 +9,18 @@
 #include <linux/types.h>
 #include <sound/asoundef.h>
 #include <sound/pcm.h>
+#include <sound/pcm_params.h>
 #include <sound/pcm_iec958.h>
 
-/**
- * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
- * @runtime: pcm runtime structure with ->rate filled in
- * @cs: channel status buffer, at least four bytes
- * @len: length of channel status buffer
- *
- * Create the consumer format channel status data in @cs of maximum size
- * @len corresponding to the parameters of the PCM runtime @runtime.
- *
- * Drivers may wish to tweak the contents of the buffer after creation.
- *
- * Returns: length of buffer, or negative error code if something failed.
- */
-int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
-	size_t len)
+static int create_iec958_consumer(uint rate, uint sample_width,
+				  u8 *cs, size_t len)
 {
 	unsigned int fs, ws;
 
 	if (len < 4)
 		return -EINVAL;
 
-	switch (runtime->rate) {
+	switch (rate) {
 	case 32000:
 		fs = IEC958_AES3_CON_FS_32000;
 		break;

@@ -59,7 +47,7 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 	}
 
 	if (len > 4) {
-		switch (snd_pcm_format_width(runtime->format)) {
+		switch (sample_width) {
 		case 16:
 			ws = IEC958_AES4_CON_WORDLEN_20_16;
 			break;

@@ -92,4 +80,34 @@ int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
 
 	return len;
 }
+
+/**
+ * snd_pcm_create_iec958_consumer - create consumer format IEC958 channel status
+ * @runtime: pcm runtime structure with ->rate filled in
+ * @cs: channel status buffer, at least four bytes
+ * @len: length of channel status buffer
+ *
+ * Create the consumer format channel status data in @cs of maximum size
+ * @len corresponding to the parameters of the PCM runtime @runtime.
+ *
+ * Drivers may wish to tweak the contents of the buffer after creation.
+ *
+ * Returns: length of buffer, or negative error code if something failed.
+ */
+int snd_pcm_create_iec958_consumer(struct snd_pcm_runtime *runtime, u8 *cs,
+	size_t len)
+{
+	return create_iec958_consumer(runtime->rate,
+				      snd_pcm_format_width(runtime->format),
+				      cs, len);
+}
 EXPORT_SYMBOL(snd_pcm_create_iec958_consumer);
+
+
+int snd_pcm_create_iec958_consumer_hw_params(struct snd_pcm_hw_params *params,
+					     u8 *cs, size_t len)
+{
+	return create_iec958_consumer(params_rate(params), params_width(params),
+				      cs, len);
+}
+EXPORT_SYMBOL(snd_pcm_create_iec958_consumer_hw_params);

Signature · 1 line

1.9.1

Add IEC958 channel status helper that gets the audio properties from snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to produce the channel status bits already in audio stream configuration phase.

Quoted 4 lines · Jyri Sarha

Add IEC958 channel status helper that gets the audio properties from snd_pcm_hw_params instead of snd_pcm_runtime. This is needed to produce the channel status bits already in audio stream configuration phase.

What is the reason for doing this early?

ALSA documentation (which may be out of date) says that the hw_params callback can be called multiple times during stream setup. Do we want to be repeatedly programming the HDMI infoframe with different settings, potentially confusing the attached device, or would it be better to do it slightly later (in the prepare callback) after the parameters have been fully negotiated?

Quoted 6 lines · Russell King - ARM Linux

ALSA documentation (which may be out of date) says that the hw_params callback can be called multiple times during stream setup. Do we want to be repeatedly programming the HDMI infoframe with different settings, potentially confusing the attached device, or would it be better to do it slightly later (in the prepare callback) after the parameters have been fully negotiated?

If it is possible that hw_params() can be called multiple times, at least it does not happen very often. I wonder if it would ever happen again after a successful hw_params() call.

But there is no downside in using prepare callback.

Quoted 1 line

Quoted 1 line

But there is no downside in using prepare callback.

The prepare callback also can be called multiple times. This certainly happens when the stream is stopped/restarted multiple times.

If something does not need to be done again when a stream is restarted, then it should be done in the hw_params callback.

From: Jean-Francois Moine <[email protected]>

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 <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

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

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +2 -0

index e9e4bce..35f6a80 100644
@@ -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.
+

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +2 -0

+
+[1] Documentation/devicetree/bindings/graph.txt
 
 Example:
 

@@ -26,4 +55,26 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +13 -0

+			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>;
+			};
+		};
 	};

drivers/gpu/drm/i2c/tda998x_drv.c +81 -9

index 424228b..0952eac 100644
@@ -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 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;

include/sound/tda998x.h new file +0 -0

new file mode 100644
index 0000000..bef1da7

Quoted 10 lines · Jyri Sarha

From: Jean-Francois Moine <[email protected]>

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.

Quoted 3 lines

Signed-off-by Jean-Francois Moine <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---

Quoted 15 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +7 -0

index e9e4bce..35f6a80 100644
@@ -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].
+

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Optional properties: · line 30

Quoted 5 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +5 -0

+    Video port.
+	There may be only one video port.
+	This one must contain the following property:
+
+	- port-type: must be "rgb"  

Why do you need this if there is no other choice? The port number should define which one is video.

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Optional properties: · line 36

Quoted 6 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +6 -0

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

I'm failing to decode how this value works. In any case, I would keep this as a vendor specific property rather than abusing reg.

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Optional properties: · line 45

Quoted 9 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +9 -0

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

reg is 32-bits.

Quoted 5 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +2 -0

+
+[1] Documentation/devicetree/bindings/graph.txt
 
 Example:
 

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Example: · line 66

Quoted 13 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +9 -0

@@ -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 */  

Is 3 significant? What happened to 0-2?

Quoted 6 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +6 -0

+			port-type = "i2s";
+			reg = <0x03>;
+			tda998x_i2s: endpoint {
+				remote-endpoint = <&audio1_i2s>;
+			};
+		};

Quoted 11 lines · On Mon, 21 Sep 2015 10:19:18 -0500 Rob Herring <[email protected]> wrote:

Quoted 10 lines · Jyri Sarha

From: Jean-Francois Moine <[email protected]>

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.

Quoted 3 lines

Quoted 3 lines

Signed-off-by Jean-Francois Moine <[email protected]>

Signed-off-by Jyri Sarha <[email protected]>

---

        [snip]

Quoted 15 lines

Quoted 15 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +7 -0

index e9e4bce..35f6a80 100644
@@ -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].
+

Quoted 8 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Optional properties: · line 30

Quoted 5 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +5 -0

+    Video port.
+   There may be only one video port.
+   This one must contain the following property:
+
+   - port-type: must be "rgb"

Why do you need this if there is no other choice? The port number should define which one is video.

There is no specific port number.
One of the ports is video and two other ones are audio.

Quoted 10 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Optional properties: · line 36

Quoted 7 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +6 -0

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

I'm failing to decode how this value works. In any case, I would keep this as a vendor specific property rather than abusing reg.

This has been explained in
https://lkml.org/lkml/2014/1/20/86

Quoted 12 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt Optional properties: · line 45

Quoted 10 lines

Documentation/devicetree/bindings/drm/i2c/tda998x.txt +9 -0

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

reg is 32-bits.

This port has only 8 significant bits as explained in
https://lkml.org/lkml/2015/1/7/362

Quoted 10 lines · Jyri Sarha

sound/soc/codecs/hdmi-codec.c new file +10 -0

+#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
+
+#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\

sound/soc/codecs/hdmi-codec.c line 293

Quoted 1 line

sound/soc/codecs/hdmi-codec.c new file +1 -0

+			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)

I'm not sure how abstracted this I2S and S/PDIF DAI business is - it doesn't feel like something that's going to be a property of all HDMI devices, and the specific sets of formats above cause me to raise a bit of an eyebrow. Should this be an interface where the HDMI chip registers multiple interfaces if it has them instead of automatically getting this split as is?

Quoted 11 lines · Mark Brown

Quoted 10 lines · Jyri Sarha

sound/soc/codecs/hdmi-codec.c new file +10 -0

+#define SPDIF_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE)
+
+#define I2S_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | SNDRV_PCM_FMTBIT_S16_BE |\
+			 SNDRV_PCM_FMTBIT_S18_3LE | SNDRV_PCM_FMTBIT_S18_3BE |\
+			 SNDRV_PCM_FMTBIT_S20_3LE | SNDRV_PCM_FMTBIT_S20_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_3LE | SNDRV_PCM_FMTBIT_S24_3BE |\
+			 SNDRV_PCM_FMTBIT_S24_LE | SNDRV_PCM_FMTBIT_S24_BE |\

Quoted 8 lines

sound/soc/codecs/hdmi-codec.c line 293

Quoted 1 line

sound/soc/codecs/hdmi-codec.c new file +1 -0

+			 SNDRV_PCM_FMTBIT_S32_LE | SNDRV_PCM_FMTBIT_S32_BE)

I'm not sure how abstracted this I2S and S/PDIF DAI business is - it doesn't feel like something that's going to be a property of all HDMI devices, and the specific sets of formats above cause me to raise a bit of an eyebrow. Should this be an interface where the HDMI chip registers multiple interfaces if it has them instead of automatically getting this split as is?

The inclusion of the 32-bit formats does raise an eyebrow here too. Audio transmission across the HDMI link is S/PDIF, supporting up to 24-bit uncompressed audio (aka L-PCM).

The device may accept 32 bit I2S, but it would have to be truncated to 24 bit before transmitting it to the sink. This should be mentioned somewhere.

Quoted 3 lines · Russell King - ARM Linux

The device may accept 32 bit I2S, but it would have to be truncated to 24 bit before transmitting it to the sink. This should be mentioned somewhere.

There is ".sig_bits = 24" in hdmi_i2s_dai, but I can add an explicit comment about it.

We needed 32bit format in practice until Peter got 24_LE properly working with McASP. I just thought the may still be some platforms out there that can not produce 24bit i2s samples for some reason, but work just fine with 32bit samples. Those platforms would be limited to less than 24bit precision without 32bit formats. But as said, it is not critical to us any more and I can drop the 32bit formats as well.