ASoC: tda998x: add a codec to the HDMI transmitter

This patch permits the definition of the audio ports from the device tree.

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

---
 .../devicetree/bindings/drm/i2c/tda998x.txt        | 18 +++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 60 ++++++++++++++++++----
 2 files changed, 69 insertions(+), 9 deletions(-)

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

index e9e4bce..e50e7cd 100644

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

+
+  - audio-port-names: must contain entries matching the entries in
+	the audio-ports property.
+	Each value may be "i2s" or "spdif", giving the type of
+	the audio source.
+
+  - #sound-dai-cells: must be set to <1> for use with the simple-card.
+	The TDA998x audio CODEC always defines two DAIs.
+	The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input.
+
 Example:
 
 	tda998x: hdmi-encoder {

@@ -26,4 +40,8 @@ Example:
 		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
+
+		audio-ports = <0x04>, <0x03>;
+		audio-port-names = "spdif", "i2s";
+		#sound-dai-cells = <1>;
 	};

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

index 70658af..9d9b072 100644
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
+#include <linux/platform_device.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>

@@ -44,6 +45,8 @@ struct tda998x_priv {
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
 	struct drm_encoder *encoder;
+
+	u8 audio_ports[2];
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)

@@ -1254,12 +1257,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 {
 	struct device_node *np = client->dev.of_node;
 	u32 video;
-	int rev_lo, rev_hi, ret;
+	int i, rev_lo, rev_hi, ret;
+	const char *p;
 
 	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
 	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
 	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
 
+	priv->params.audio_frame[1] = 1;		/* channels - 1 */
+	priv->params.audio_sample_rate = 48000;		/* 48kHz */
+
 	priv->current_page = 0xff;
 	priv->hdmi = client;
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);

@@ -1351,15 +1358,50 @@ 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 device tree parameters */
+	if (np) {
+
+		/* 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;
+		}
+
+		/* optional audio properties */
+		for (i = 0; i < 2; i++) {
+			u32 port;
 
-	/* 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;
+			ret = of_property_read_u32_index(np, "audio-ports", i, &port);
+			if (ret)
+				break;
+			ret = of_property_read_string_index(np, "audio-port-names",
+							i, &p);
+			if (ret) {
+				dev_err(&client->dev,
+					"missing audio-port-names[%d]\n", i);
+				break;
+			}
+			if (strcmp(p, "spdif") == 0) {
+				priv->audio_ports[0] = port;
+			} else if (strcmp(p, "i2s") == 0) {
+				priv->audio_ports[1] = port;
+			} else {
+				dev_err(&client->dev,
+					"bad audio-port-names '%s'\n", p);
+				break;
+			}
+		}
+		if (priv->audio_ports[0]) {
+			priv->params.audio_cfg = priv->audio_ports[0];
+			priv->params.audio_format = AFMT_SPDIF;
+			priv->params.audio_clk_cfg = 0;
+		} else {
+			priv->params.audio_cfg = priv->audio_ports[1];
+			priv->params.audio_format = AFMT_I2S;
+			priv->params.audio_clk_cfg = 1;
+		}
 	}
 
 	return 0;

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

Quoted 4 lines

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

+  - audio-ports: must contain one or two values selecting the source
+	in the audio port.
+	The source type is given by the corresponding entry in
+	the audio-port-names property.  

I think that this entry might benefit from a little more explanation. The value specified here selects which pins on the chip provide the audio input doesn't it? In the outline datasheet that I have these are listed in table 17:

Audio port Input configuration

		S/PDIF 		I2S-bus

AP0 - WS (word select)
AP1 S/PDIF input I2S-bus channel 0
AP2 S/PDIF input I2S-bus channel 1
AP3[1] I2S-bus channel 2
AP4[1] I2S-bus channel 3
ACLK - SCK (I2S-bus clock)

[1] Depending on package.

Quoted 12 lines · On Wed, 07 Jan 2015 14:39:13 +0000 Andrew Jackson <[email protected]>…

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

Quoted 4 lines

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

+  - audio-ports: must contain one or two values selecting the source
+	in the audio port.
+	The source type is given by the corresponding entry in
+	the audio-port-names property.  

I think that this entry might benefit from a little more explanation. The value specified here selects which pins on the chip provide the audio input doesn't it? In the outline datasheet that I have these are listed in table 17:

Audio port Input configuration

		S/PDIF 		I2S-bus

Quoted 8 lines

AP0 - WS (word select)
AP1 S/PDIF input I2S-bus channel 0
AP2 S/PDIF input I2S-bus channel 1
AP3[1] I2S-bus channel 2
AP4[1] I2S-bus channel 3
ACLK - SCK (I2S-bus clock)

[1] Depending on package.

Your table is close to the one in the TDA9983B documentation I have, but the pins are not exactly the same:

AP0 WS (word select)
AP1 I2S-bus port 0
AP2 I2S-bus port 1
AP3 I2S-bus port 2
AP4 I2S-bus port 3
AP5 MCLK (master clock for S/PDIF)
AP6 S/PDIF input
AP7 AUX (internal test)
ACLK SCK (I2S-bus clock)

That's why I did not know clearly why I had to set AP2 for S/PDIF input and (AP0 + AP1) for I2S input in the Cubox.

Then, the only more explanation I could give is "have a look at the audio input format and at the register 0x1e page 0 in the documentation of the TDA998x chip".

BTW, the tda998x driver supports only the TDA9989, TDA19988 and TDA19989 chips. If the TDA9983B would be supported, the audio port definitions would be of no use.

So, what would you see as an explanation?

I understand your difficulty! I was just wanting something to clarify the meaning of the value without reference to the driver source.

You could add something like this to your existing explanation: "The value describes which audio input pins are selected; this varies depending on chip type so consult the section on audio port configuration in the relevant datasheet.".

This patch permits the definition of the audio ports from the device tree.

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

---
  .../devicetree/bindings/drm/i2c/tda998x.txt        | 18 +++++++
  drivers/gpu/drm/i2c/tda998x_drv.c                  | 60 ++++++++++++++++++----
  2 files changed, 69 insertions(+), 9 deletions(-)

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

index e9e4bce..e50e7cd 100644

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

+  - audio-port-names: must contain entries matching the entries in
+	the audio-ports property.
+	Each value may be "i2s" or "spdif", giving the type of
+	the audio source.
+
+  - #sound-dai-cells: must be set to <1> for use with the simple-card.
+	The TDA998x audio CODEC always defines two DAIs.
+	The DAI 0 is the S/PDIF input and the DAI 1 is the I2S input.
+
  Example:
 
  	tda998x: hdmi-encoder {

@@ -26,4 +40,8 @@ Example:
  		interrupts = <27 2>;		/* falling edge */
  		pinctrl-0 = <&pmx_camera>;
  		pinctrl-names = "default";
+
+		audio-ports = <0x04>, <0x03>;
+		audio-port-names = "spdif", "i2s";
+		#sound-dai-cells = <1>;
  	};

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

index 70658af..9d9b072 100644
@@ -20,6 +20,7 @@
  #include <linux/module.h>
  #include <linux/irq.h>
  #include <sound/asoundef.h>
+#include <linux/platform_device.h>
 
  #include <drm/drmP.h>
  #include <drm/drm_crtc_helper.h>

@@ -44,6 +45,8 @@ struct tda998x_priv {
  	wait_queue_head_t wq_edid;
  	volatile int wq_edid_wait;
  	struct drm_encoder *encoder;
+
+	u8 audio_ports[2];
  };
 
  #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)

@@ -1254,12 +1257,16 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
  {
  	struct device_node *np = client->dev.of_node;
  	u32 video;
-	int rev_lo, rev_hi, ret;
+	int i, rev_lo, rev_hi, ret;
+	const char *p;
 
  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(2) | VIP_CNTRL_0_SWAP_B(3);
  	priv->vip_cntrl_1 = VIP_CNTRL_1_SWAP_C(0) | VIP_CNTRL_1_SWAP_D(1);
  	priv->vip_cntrl_2 = VIP_CNTRL_2_SWAP_E(4) | VIP_CNTRL_2_SWAP_F(5);
 
+	priv->params.audio_frame[1] = 1;		/* channels - 1 */
+	priv->params.audio_sample_rate = 48000;		/* 48kHz */
+
  	priv->current_page = 0xff;
  	priv->hdmi = client;
  	priv->cec = i2c_new_dummy(client->adapter, 0x34);

@@ -1351,15 +1358,50 @@ 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 device tree parameters */
+	if (np) {
+
+		/* 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;
+		}
+
+		/* optional audio properties */
+		for (i = 0; i < 2; i++) {
+			u32 port;
 
-	/* 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;
+			ret = of_property_read_u32_index(np, "audio-ports", i, &port);
+			if (ret)
+				break;
+			ret = of_property_read_string_index(np, "audio-port-names",
+							i, &p);
+			if (ret) {
+				dev_err(&client->dev,
+					"missing audio-port-names[%d]\n", i);
+				break;
+			}
+			if (strcmp(p, "spdif") == 0) {
+				priv->audio_ports[0] = port;
+			} else if (strcmp(p, "i2s") == 0) {
+				priv->audio_ports[1] = port;
+			} else {
+				dev_err(&client->dev,
+					"bad audio-port-names '%s'\n", p);
+				break;
+			}
+		}
+		if (priv->audio_ports[0]) {
+			priv->params.audio_cfg = priv->audio_ports[0];

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

Quoted 5 lines

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

+  - audio-ports: must contain one or two values selecting the source
+	in the audio port.
+	The source type is given by the corresponding entry in
+	the audio-port-names property.
+  

This binding does not allow multi channel i2s setups with multiple i2s pins. It would be nice to support that in the DT binding, even if the code is not yet ready for it.

How about having these two optional properties instead of audio-ports and audio-port-names:

audio-port-i2s: Upto 4 values for selecting pins for i2s port
audio-port-spdif: Value for selecting input pin for spdif port

Presence of one of the properties would be mandatory and both are allowed.

Sorry to notice this only now, but I have not yet looked the drm side changes too closely.

From Andrew's datasheet, the TDA998x's which are handled by the tda998x

Examples:

- for the Cubox:

	audio-inputs = "i2s", "spdif";

- for some other board with I2S on the pins 3 and 4 only:

	audio-inputs = "none", "none", "i2s", "i2s";

- for a fully wired TDA9983B (no driver yet):

	audio-inputs = "i2s", "i2s", "i2s", "i2s", "spdif";

Quoted 9 lines · Jean-Francois Moine

Examples:

- for the Cubox:

	audio-inputs = "i2s", "spdif";

- for some other board with I2S on the pins 3 and 4 only:

	audio-inputs = "none", "none", "i2s", "i2s";

Quoted 3 lines

- for a fully wired TDA9983B (no driver yet):

	audio-inputs = "i2s", "i2s", "i2s", "i2s", "spdif";

I think that mostly works, though I do wonder if we need a way to specify the ordering of the pins (if you can make pin 3 be the first two I2S channels for example)? Someone might choose a strange mapping for board routing reasons for example.

Quoted 16 lines · On Thu, 8 Jan 2015 16:53:41 +0200 Jyri Sarha…

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

Quoted 5 lines

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

+  - audio-ports: must contain one or two values selecting the source
+	in the audio port.
+	The source type is given by the corresponding entry in
+	the audio-port-names property.
+

This binding does not allow multi channel i2s setups with multiple i2s pins. It would be nice to support that in the DT binding, even if the code is not yet ready for it.

How about having these two optional properties instead of audio-ports and audio-port-names:

audio-port-i2s: Upto 4 values for selecting pins for i2s port
audio-port-spdif: Value for selecting input pin for spdif port

Quoted 4 lines

Presence of one of the properties would be mandatory and both are allowed.

Sorry to notice this only now, but I have not yet looked the drm side changes too closely.

From Andrew's datasheet, the TDA998x's which are handled by the tda998x
driver have only 4 input audio pins, the first two ones being either S/PDIF or I2s, the last ones being I2S only.

So, the DT description could be reduced to a simple list indexed by the pin number (= DAI number) and defining the protocol type.

Examples:

- for the Cubox:

	audio-inputs = "i2s", "spdif";

- for some other board with I2S on the pins 3 and 4 only:

	audio-inputs = "none", "none", "i2s", "i2s";

- for a fully wired TDA9983B (no driver yet):

	audio-inputs = "i2s", "i2s", "i2s", "i2s", "spdif";

Quoted 3 lines · Jean-Francois Moine

From Andrew's datasheet, the TDA998x's which are handled by the tda998x
driver have only 4 input audio pins, the first two ones being either S/PDIF or I2s, the last ones being I2S only.

AFAIK, SPDIF is always a single pin connection so only one pin needs to be selected. But i2s need for pins for full 8 channel output.

Quoted 9 lines

So, the DT description could be reduced to a simple list indexed by the pin number (= DAI number) and defining the protocol type.

Examples:

- for the Cubox:

	audio-inputs = "i2s", "spdif";

Quoted 8 lines

- for some other board with I2S on the pins 3 and 4 only:

	audio-inputs = "none", "none", "i2s", "i2s";

- for a fully wired TDA9983B (no driver yet):

	audio-inputs = "i2s", "i2s", "i2s", "i2s", "spdif";

If you want to go closer to the original paradigm, then how about defining following audio-port-names: i2s0, i2s1, i2s2, i2s3, and spdif. With this approach we could go with your original binding with only minor changes. A binding in your stereo i2s or spdif case would look like this:

	audio-ports = <0x04>, <0x03>;
	audio-port-names = "spdif", "i2s0";

A full 8 channel i2s or spdif output would look like this:

	audio-ports = <0x04>, <0x03>, <0x02>, <0x01>, <0x00>;
	audio-port-names = "spdif", "i2s0", "i2s1", "i2s2", "i2s3";

This would also indicate the channel mapping to the audio pins (i2s0 for first two channels, i2s1 for 3-4, etc.)

The code could for now just look for "i2s0" and the port names for channels 3-8 could be ignored until they are needed.

In my original version, the audio-ports are a bitmap of the pins, the bit 0 being the WS used for I2S. A fully wired tda998x would have been as:

	audio-ports = <0x03>, <0x04>, <0x09>, <0x11>;
	audio-port-names = "i2s", "spdif", "i2s", "i2s";

With the new version, it would simply become:

	audio-inputs = "i2s", "spdif", "i2s", "i2s";

Quoted 10 lines · Jean-Francois Moine

In my original version, the audio-ports are a bitmap of the pins, the bit 0 being the WS used for I2S. A fully wired tda998x would have been as:

	audio-ports = <0x03>, <0x04>, <0x09>, <0x11>;
	audio-port-names = "i2s", "spdif", "i2s", "i2s";

With the new version, it would simply become:

	audio-inputs = "i2s", "spdif", "i2s", "i2s";

How do you know which i2s inputs to enable?

Does it make sense for the audio inputs to be mixed like that?

You will need to enable one i2s for front L+R, and increasingly others for the additional channels.

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

Quoted 5 lines · On Fri, 9 Jan 2015 11:45:29 +0000 Russell King - ARM Linux…

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

The DT describes the hardware configuration.

A fully wired tda998x could be a chip with the audio pins connected to:
- a kirkwood-like audio device with one I2S and one S/PDIF output,
- two other audio devices with one I2S each.

With S/PDIF, only one stereo channel may be sent, but with I2S, up to 4 stereo channels may be sent.

These channels are extracted by the devices connected on the HDMI bus.

An example could be the playing of a multi-language movie: each audio channel carries one language. From the computer view, the playing application sends each language to one sound card.

So, this means that the tda998x driver should check that S/PDIF and I2S are not active at the same time, and it should also do a pin OR/AND on I2S start/stop.

Quoted 9 lines · Jean-Francois Moine

Quoted 5 lines · On Fri, 9 Jan 2015 11:45:29 +0000 Russell King - ARM Linux…

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

The DT describes the hardware configuration.

You're missing my point.

How does the driver know which of the I2S pins to enable in I2S mode?

My question is: how do we know which I2S inputs to enable, or are you suggesting that all I2S inputs should be enabled if operating in I2S mode irrespective of whether they may be active?

Quoted 14 lines · Russell King - ARM Linux

Quoted 9 lines · Jean-Francois Moine

Quoted 5 lines · On Fri, 9 Jan 2015 11:45:29 +0000 Russell King - ARM Linux…

I think we need to understand exactly how the 998x map I2S inputs to the HDMI channels to avoid making a mistake with the binding; remember, the binding isn't something that can be easily "bug fixed" at a later date as anything we come up with now has to be supported long term by the kernel.

The DT describes the hardware configuration.

You're missing my point.

How does the driver know which of the I2S pins to enable in I2S mode?

[snip]

Quoted 4 lines

My question is: how do we know which I2S inputs to enable, or are you suggesting that all I2S inputs should be enabled if operating in I2S mode irrespective of whether they may be active?

Isn't it the case that for I2S:

* Word Select (WS) is always required to disambiguate left/right. So WS need not be specified in any device tree configuration.

* The audio inputs depend on a particular board but at least one is required. Fortunately, the TDA998x devices have all the same pin/input numbering so they /could/ be described as i2s0, i2s1, i2s2 and i2s3 as per J-F's earlier email. But tt isn't clear from my reading of the TDA19988 datasheet (for example) whether one can skip channels (so does channel 0 always have to be present?). If the channels must be enabled from 0 then one could simply specify the number of inputs. (The datasheets that I have don't indicate whether there's any channel remapping performed internally by the TDA998x).

Well, if we look at the HDMI specs, there is a field in the audio info frame (CA bits) which identifies the speaker allocation between the HDMI PCM channels and the physical speakers.

It describes a limited set - essentially though, channel 0 is always front left, and channel 1 is always front right. Channel 2 is always LFE, and channel 3 is always front centre.

Channel 0 and 1 are always allocated, but it's possible to have channels above 2 to be allocated independently of each other (eg, you could have 7, 6, 1, 0 allocated to front right centre, front left centre, right, left speakers - in that order.)

As you point out, we don't have documentation which tells us know how these PCM channels map to I2S inputs.

What we do know is that there is a fixed mapping between AP pins and I2S channels (which are not PCM channels), but as you point out, we don't have any documentation which describes how the I2S channels (each with their own L+R words) map to the PCM channels - and we don't know whether the CA_I2S bits in that same register in the TDA998x have an effect on this.

Does anyone have a TDA998x setup which has an I2S source connected to the TDA998x I2S channel 1, and who has a HDMI sink which will accept the LFE/FC channels? If so, producing a description of how the CA_I2S bits and enabling I2S input pins influences the mapping would be a good idea.

If we don't have that, I'd recommend splitting the DT property into "audio inputs for I2S" and "audio input for SPDIF" (only one can be active with SPDIF).

If we want to support more than one SPDIF input (which must be mutually exclusive) I'd recommend to look at the OF graph stuff we use in DRM - one port for each "mode" - eg, I2S, SPDIF in on AP2, SPDIF in on AP3. Each port node can specify the AP pins which should be enabled.

Quoted 6 lines · On Fri, 9 Jan 2015 14:57:41 +0000 Russell King - ARM Linux…

What we do know is that there is a fixed mapping between AP pins and I2S channels (which are not PCM channels), but as you point out, we don't have any documentation which describes how the I2S channels (each with their own L+R words) map to the PCM channels - and we don't know whether the CA_I2S bits in that same register in the TDA998x have an effect on this.

HDMI talks about LPCM (Linear PCM) channels and TDA19988 talks about I2S-bus (stereo) channels. For me, it seems obvious that these channels are correlated:
- LPCM-0 is I2S-bus-0-left (FL)
- LPCM-1 is I2S-bus-0-right (FR)
- LPCM-2 is I2S-bus-1-left (LFE)
- LPCM-3 is I2S-bus-1-right (FC)
...

Quoted 5 lines

Does anyone have a TDA998x setup which has an I2S source connected to the TDA998x I2S channel 1, and who has a HDMI sink which will accept the LFE/FC channels? If so, producing a description of how the CA_I2S bits and enabling I2S input pins influences the mapping would be a good idea.

I could not find a description of these CA_I2S bits.

Quoted 8 lines

If we don't have that, I'd recommend splitting the DT property into "audio inputs for I2S" and "audio input for SPDIF" (only one can be active with SPDIF).

If we want to support more than one SPDIF input (which must be mutually exclusive) I'd recommend to look at the OF graph stuff we use in DRM - one port for each "mode" - eg, I2S, SPDIF in on AP2, SPDIF in on AP3. Each port node can specify the AP pins which should be enabled.

I agree. There are one S/PDIF port and one I2S port.

So, which syntax?

I proposed:

	audio-ports = <0x04>, <0x03>;
	audio-port-names = "spdif", "i2s";

Do you better like:

	audio-spdif-port = <0x04>;
	audio-i2s-port = <0x03>;

Quoted 9 lines

I proposed:

	audio-ports = <0x04>, <0x03>;
	audio-port-names = "spdif", "i2s";

Do you better like:

	audio-spdif-port = <0x04>;
	audio-i2s-port = <0x03>;  

Neither; we know that there are TDA998x devices which allow SPDIF to be input via two separate pins, but only one to be active at any one time. Given that the hardware is flexible in that regard, a binding which artificially restricts that flexibility would seem unwise.

If we were to come across a setup which did route two SPDIF streams to the TDA998x, and we had to make the decision at run time which to route to the HDMI sink, we'd have to rework the binding, and we'd have to support the new binding and the old binding in the driver.

Can you please look at Documentation/devicetree/bindings/graph.txt ?

I think we may be able to use something like this:

tda998x: hdmi-encoder {
	compatible = "nxp,tda998x";
	reg = <0x70>;
	video-ports = <0x234501>;

	port {
		tda998x_video: endpoint {
			remote-endpoint = <&lcd0_rgb>;
		};
	};

	port {
		#address-cells = <1>;
		#size-cells = <0>;

		tda998x_spdif0: endpoint@02 {
			reg = <0x02>;
			remote-endpoint = <&spdif0>;
		};

		tda998x_spdif1: endpoint@04 {
			reg = <0x04>;
			remote-endpoint = <&spdif0>;
		};

		tda998x_i2s: endpoint@03 {
			reg = <0x03>;
			remote-endpoint = <&i2s>;
		};
	};
};

I'm adding Philipp Zabel for comment. The issue I see with this is that we have two ports here which are not mutually exclusive, and it's not obvious how they are dealt with.

Neither; we know that there are TDA998x devices which allow SPDIF to be input via two separate pins, but only one to be active at any one time. Given that the hardware is flexible in that regard, a binding which artificially restricts that flexibility would seem unwise.

If we were to come across a setup which did route two SPDIF streams to the TDA998x, and we had to make the decision at run time which to route to the HDMI sink, we'd have to rework the binding, and we'd have to support the new binding and the old binding in the driver.

Can you please look at Documentation/devicetree/bindings/graph.txt ?

I think we may be able to use something like this:

tda998x: hdmi-encoder {
	compatible = "nxp,tda998x";
	reg = <0x70>;
	video-ports = <0x234501>;

	port {
		tda998x_video: endpoint {
			remote-endpoint = <&lcd0_rgb>;
		};
	};

	port {
		#address-cells = <1>;
		#size-cells = <0>;

		tda998x_spdif0: endpoint@02 {
			reg = <0x02>;
			remote-endpoint = <&spdif0>;
		};

		tda998x_spdif1: endpoint@04 {
			reg = <0x04>;
			remote-endpoint = <&spdif0>;
		};

		tda998x_i2s: endpoint@03 {
			reg = <0x03>;
			remote-endpoint = <&i2s>;
		};
	};
};

I'm adding Philipp Zabel for comment. The issue I see with this is that we have two ports here which are not mutually exclusive, and it's not obvious how they are dealt with.

Jean-Francois' reply already reflects this, but the 'port' nodes should correspond to physical ports of the device if possible. If you can configure the device to have dedicated input pins for I2S, SPDIF0, and SPDIF1 at the same time, they should appear in the device tree as separate ports:

tda998x: hdmi-encoder {
	port@0 { /* pixel data according to video-ports */
		reg = <0x00>;
	};
	port@1 { /* AP1: SPDIF0 */
		reg = <0x01>;
	};
	port@2 { /* AP2: SPDIF1 */
		reg = <0x02>;
	};
	port@3 { /* AP3: I2S */
		reg = <0x03>;
	};
};

The tda998x binding would define how the ports are numbered, some correspondence to the AP pin numbers would be good.

Quoted 21 lines

Jean-Francois' reply already reflects this, but the 'port' nodes should correspond to physical ports of the device if possible. If you can configure the device to have dedicated input pins for I2S, SPDIF0, and SPDIF1 at the same time, they should appear in the device tree as separate ports:

tda998x: hdmi-encoder {
	port@0 { /* pixel data according to video-ports */
		reg = <0x00>;
	};
	port@1 { /* AP1: SPDIF0 */
		reg = <0x01>;
	};
	port@2 { /* AP2: SPDIF1 */
		reg = <0x02>;
	};
	port@3 { /* AP3: I2S */
		reg = <0x03>;
	};
};

Quoted 2 lines

The tda998x binding would define how the ports are numbered, some correspondence to the AP pin numbers would be good.

It's not quite that simple, because the SPDIF AP pins are multiplexed with the I2S pins - and there is variation between chip models and packages.

So, it's probably best if port@0 is the video port, and then [email protected] can describe the audio inputs, including a property which specifies whether they are I2S or SPDIF, and the value to be programmed into the AP enable register (which is a bit field of the AP pins which should be unmasked.) I guess we can re-use the reg= property for that value, since video will always be zero.

Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:

Quoted 10 lines

It's not quite that simple, because the SPDIF AP pins are multiplexed with the I2S pins - and there is variation between chip models and packages.

So, it's probably best if port@0 is the video port, and then [email protected] can describe the audio inputs, including a property which specifies whether they are I2S or SPDIF, and the value to be programmed into the AP enable register (which is a bit field of the AP pins which should be unmasked.) I guess we can re-use the reg= property for that value, since video will always be zero.

Note that of_graph_parse_endpoint interprets the port node's reg property as port id. And the unit address part of the node name should match the first address in the reg property.

Quoted 12 lines · Philipp Zabel

Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:

Quoted 10 lines

It's not quite that simple, because the SPDIF AP pins are multiplexed with the I2S pins - and there is variation between chip models and packages.

So, it's probably best if port@0 is the video port, and then [email protected] can describe the audio inputs, including a property which specifies whether they are I2S or SPDIF, and the value to be programmed into the AP enable register (which is a bit field of the AP pins which should be unmasked.) I guess we can re-use the reg= property for that value, since video will always be zero.

Quoted 3 lines

Note that of_graph_parse_endpoint interprets the port node's reg property as port id. And the unit address part of the node name should match the first address in the reg property.

So that's not going to work very well... because the AP register is a bitmask.

I guess we could specify a node unit and reg, which the code otherwise ignores, and specify a philipps,ap-mask = property for the audio ports instead.

Quoted 4 lines · On Mon, 12 Jan 2015 14:04:56 +0000 Russell King - ARM Linux…

Quoted 3 lines · Philipp Zabel

Note that of_graph_parse_endpoint interprets the port node's reg property as port id. And the unit address part of the node name should match the first address in the reg property.

This is not the case in vexpress-v2p-ca15_a7.dts.

Quoted 6 lines

So that's not going to work very well... because the AP register is a bitmask.

I guess we could specify a node unit and reg, which the code otherwise ignores, and specify a philipps,ap-mask = property for the audio ports instead.

Also, the video and audio ports must be distinguished. They could be defined in different port groups.

Example from the Cubox:

	video-ports: ports@0 {
		port {
			tda998x_video: endpoint {
				remote-endpoint = <&lcd0_0>;
				nxp,video-port = <0x230145>;
			};
		};
	};
	audio-ports: ports@1 {
		port@0 {			/* AP1 = I2S */
			tda998x_i2s: endpoint@0 {
				remote-endpoint = <&audio1_i2s>;
				nxp,audio-port = <0x03>;
			};
		};
		port@1 {			 /* AP2 = S/PDIF */
			tda998x_spdif: endpoint@1 {
				remote-endpoint = <&audio1_spdif1>;
				nxp,audio-port = <0x04>;
			};
		};
	};

The port type is identified by the bit AP0.

I don't particularly like that - that makes the assumption that AP0 always means I2S. What if a future chip decides to allow SPDIF on AP0? Why should we need to re-invent the binding?

IMHO, it would be much better to make this explicit.

Note that the "video-ports" and "audio-ports" are just labels in the DT file; they aren't carried through to the resulting DT binary file, so they don't have any meaning to the kernel.

Quoted 7 lines

Quoted 4 lines · On Mon, 12 Jan 2015 14:04:56 +0000 Russell King - ARM Linux…

Quoted 3 lines · Philipp Zabel

Note that of_graph_parse_endpoint interprets the port node's reg property as port id. And the unit address part of the node name should match the first address in the reg property.

Quoted 1 line

This is not the case in vexpress-v2p-ca15_a7.dts.

Hmm... as the DT binding doc doesn't specify this restriction, and we have a DT file which violates what Philipp has said, I think we ought to document that reg vs unit node name does not need to match each other, thereby making that official.

Quoted 26 lines

Quoted 6 lines

So that's not going to work very well... because the AP register is a bitmask.

I guess we could specify a node unit and reg, which the code otherwise ignores, and specify a philipps,ap-mask = property for the audio ports instead.

Also, the video and audio ports must be distinguished. They could be defined in different port groups.

Example from the Cubox:

	video-ports: ports@0 {
		port {
			tda998x_video: endpoint {
				remote-endpoint = <&lcd0_0>;
				nxp,video-port = <0x230145>;
			};
		};
	};
	audio-ports: ports@1 {
		port@0 {			/* AP1 = I2S */
			tda998x_i2s: endpoint@0 {
				remote-endpoint = <&audio1_i2s>;
				nxp,audio-port = <0x03>;
			};

Quoted 8 lines

		};
		port@1 {			 /* AP2 = S/PDIF */
			tda998x_spdif: endpoint@1 {
				remote-endpoint = <&audio1_spdif1>;
				nxp,audio-port = <0x04>;
			};
		};
	};

Quoted 1 line

The port type is identified by the bit AP0.

I don't particularly like that - that makes the assumption that AP0 always means I2S. What if a future chip decides to allow SPDIF on AP0? Why should we need to re-invent the binding?

IMHO, it would be much better to make this explicit.

I wonder if it wouldn't be nicer to have the AP# and type in the device tree and then calculate the register value from that in the driver.

	port@1 {
		reg = <1>; /* AP1 */
		nxp,audio-port = "i2s";
		tda998x_i2s: endpoint {
			remote-endpoint = <&audio1_i2s>;
		};
	};

Quoted 10 lines · Philipp Zabel

I wonder if it wouldn't be nicer to have the AP# and type in the device tree and then calculate the register value from that in the driver.

	port@1 {
		reg = <1>; /* AP1 */
		nxp,audio-port = "i2s";
		tda998x_i2s: endpoint {
			remote-endpoint = <&audio1_i2s>;
		};
	};

What about the case where we have 4 I2S streams being supplied to the device on four separate AP inputs?

4 streams on 4 different APs (sources) should work, but 4 streams from a same source should be detailed.

Quoted 2 lines · Jean-Francois Moine

4 streams on 4 different APs (sources) should work, but 4 streams from a same source should be detailed.

I'd like to know how you intend to wire up four different I2S sources to the TDA998x.

Remember, an I2S source produces the I2S data and the word clock - that's two outputs. You can't electronically wire the word clocks together.

Quoted 9 lines · On Tue, 13 Jan 2015 16:03:13 +0000 Russell King - ARM Linux…

Quoted 2 lines · Jean-Francois Moine

4 streams on 4 different APs (sources) should work, but 4 streams from a same source should be detailed.

I'd like to know how you intend to wire up four different I2S sources to the TDA998x.

Remember, an I2S source produces the I2S data and the word clock - that's two outputs. You can't electronically wire the word clocks together.

From the spec, the tda998x gets independently the serial clock and the serial word select from each I2S (stereo) input channel (= audio pin), so, you may have 4 audio chips giving 4 independent audio streams. I don't know what can be the result in HDMI if these streams are actived at the same time!

In the other configuration, an audio chip may have 4 synchronized stereo channels (software PCMs). These ones may be considered as one link (one port out from the audio chip to one port in to the tda998x), the AP configuration being 0x1f.

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_
WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~
I2S1: llmm............................llmm............................llm
I2S2: llmm............................llmm............................llm
I2S3: llmm............................llmm............................llm
I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is_impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

Quoted 9 lines · Russell King - ARM Linux

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_
WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~
I2S1: llmm............................llmm............................llm
I2S2: llmm............................llmm............................llm
I2S3: llmm............................llmm............................llm
I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is_impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

This is besides the point, but it is possible that one of the multiple I2S blocks is the bit-clock and frame-clock master to the i2s bus and the others are slaves to it (banging their bits according to SCLK and WS of the I2S master). However, in this situation there really is only one i2s bus with multiple data pins.

Just my 0.02€ to this discussion.

Quoted 11 lines · Jyri Sarha

Quoted 9 lines · Russell King - ARM Linux

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_
WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~
I2S1: llmm............................llmm............................llm
I2S2: llmm............................llmm............................llm
I2S3: llmm............................llmm............................llm
I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is_impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

Quoted 7 lines

This is besides the point, but it is possible that one of the multiple I2S blocks is the bit-clock and frame-clock master to the i2s bus and the others are slaves to it (banging their bits according to SCLK and WS of the I2S master). However, in this situation there really is only one i2s bus with multiple data pins.

Just my 0.02€ to this discussion.

Right, that's about the only way it could work.

To represent that in DT, I would imagine we'd need something like this:

	#address-cells = <1>;
	#size-cells = <0>;
	...
        port@1 {                        /* AP1,2 = I2S */
		#address-cells = <1>;
		#size-cells = <0>;
                port-type = "i2s";
                reg = <0x01>;		/* WS */
                tda998x_i2s1: endpoint@2 {
			reg = <0x02>;	/* AP1 */
                        remote-endpoint = <&audio1_i2s>;
                };
                tda998x_i2s2: endpoint@4 {
			reg = <0x04>;	/* AP2 */
                        remote-endpoint = <&audio2_i2s>;
                };
        };

where audio1_i2s is operating in master mode, and audio2_i2s is operating in slave mode for both WS and SCLK.

If we can agree on that, then I'm happy with the proposed binding. (Remember that #address-cells and #size-cells are required in the parent where we have reg= in the child.)

Quoted 45 lines · On Tue, 13 Jan 2015 19:54:15 +0000 Russell King - ARM Linux…

Quoted 11 lines · Jyri Sarha

Quoted 9 lines · Russell King - ARM Linux

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_
WS: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~
I2S1: llmm............................llmm............................llm
I2S2: llmm............................llmm............................llm
I2S3: llmm............................llmm............................llm
I2S4: llmm............................llmm............................llm

So, what I'm saying is that it is_impossible_ to drive the TDA998x using multiple I2S streams which are not produced by the same I2S block.

Quoted 7 lines

This is besides the point, but it is possible that one of the multiple I2S blocks is the bit-clock and frame-clock master to the i2s bus and the others are slaves to it (banging their bits according to SCLK and WS of the I2S master). However, in this situation there really is only one i2s bus with multiple data pins.

Just my 0.02€ to this discussion.

Right, that's about the only way it could work.

To represent that in DT, I would imagine we'd need something like this:

	#address-cells = <1>;
	#size-cells = <0>;
	...
        port@1 {                        /* AP1,2 = I2S */
		#address-cells = <1>;
		#size-cells = <0>;
                port-type = "i2s";
                reg = <0x01>;		/* WS */
                tda998x_i2s1: endpoint@2 {
			reg = <0x02>;	/* AP1 */
                        remote-endpoint = <&audio1_i2s>;
                };
                tda998x_i2s2: endpoint@4 {
			reg = <0x04>;	/* AP2 */
                        remote-endpoint = <&audio2_i2s>;
                };
        };

where audio1_i2s is operating in master mode, and audio2_i2s is operating in slave mode for both WS and SCLK.

Quoted 3 lines

If we can agree on that, then I'm happy with the proposed binding. (Remember that #address-cells and #size-cells are required in the parent where we have reg= in the child.)

#address-cells and #size-cells may be defined in any of the upper parent, so, as it is defined in the device, it is not needed in the port (see of_n_addr_cells in drivers/of/base.c).

Well, I am a bit lost between (only one source / many channels - I2S busses) and (many sources / one I2s bus!).

Anyway, I don't understand your example.
I'd expect that a port would be a DAI.

If yes, when playing is active, both endpoints receive an audio stream from the remote audio devices, and the AP register is always set to 0x07 (= 0x01 | 0x02 | 0x04).
Then, it would have been simpler to have:

(quoted diff) +0 -0

 	#address-cells = <1>;
 	#size-cells = <0>;
...
         port@7 {                        /* AP1,2 = I2S */
                 port-type = "i2s";
                 reg = <0x07>;		/* WS + AP1 + AP2 */
                 tda998x_i2s1: endpoint@1 {
                         remote-endpoint = <&audio1_i2s>;
                 };
                 tda998x_i2s2: endpoint@2 {
                         remote-endpoint = <&audio2_i2s>;
                 };
         };

If no, the two DAIs must be created from the endpoints, permitting streaming on i2s1 alone, i2s2 alone or both i2s1+i2s2 at the same time.

To represent that in DT, I would imagine we'd need something like this:

	#address-cells = <1>;
	#size-cells = <0>;
	...
        port@1 {                        /* AP1,2 = I2S */
		#address-cells = <1>;
		#size-cells = <0>;
                port-type = "i2s";
                reg = <0x01>;		/* WS */
                tda998x_i2s1: endpoint@2 {
			reg = <0x02>;	/* AP1 */
                        remote-endpoint = <&audio1_i2s>;
                };
                tda998x_i2s2: endpoint@4 {
			reg = <0x04>;	/* AP2 */
                        remote-endpoint = <&audio2_i2s>;
                };
        };

where audio1_i2s is operating in master mode, and audio2_i2s is operating in slave mode for both WS and SCLK.

If we can agree on that, then I'm happy with the proposed binding. (Remember that #address-cells and #size-cells are required in the parent where we have reg= in the child.)

So the question is mostly whether four I2S data pins with a single shared WS/SCK input should be called "four I2S ports with shared clocks" or "one I2S port with up to four data lanes". I'd lean towards the latter.

How audio2_i2s is forced to synchronize its clock output to audio1_i2s is a problem their bindings will have to handle.

Trying to hook up a controller that doesn't natively support this format to a device that uses it is definitely tricky, as well as describing the physical hookup we also need to worry about how things look to userspace
- it's ideally going to want a single multi-channel audio stream.

I think from a binding point of view we need a way to say "this endpoint on the link is constructed from these DAIs on the device side" and say if this is TDM or multi-data, and what's driving the clocks.

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

The CODEC handles both I2S and S/PDIF inputs.
It maintains the audio format and rate 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 <[email protected]>

---
 drivers/gpu/drm/i2c/Kconfig       |   1 +
 drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++--
 include/sound/tda998x.h           |  23 +++++
 sound/soc/codecs/Kconfig          |   4 +
 sound/soc/codecs/Makefile         |   2 +
 sound/soc/codecs/tda998x.c        | 175 ++++++++++++++++++++++++++++++++++++++
 6 files changed, 339 insertions(+), 5 deletions(-)
 create mode 100644 include/sound/tda998x.h
 create mode 100644 sound/soc/codecs/tda998x.c

drivers/gpu/drm/i2c/Kconfig +0 -0

index 22c7ed6..24fc975 100644

drivers/gpu/drm/i2c/Kconfig +0 -0

        help
          Support for NXP Semiconductors TDA998X HDMI encoders.
 

drivers/gpu/drm/i2c/tda998x_drv.c +134 -5

index ce1478d..a26a516 100644
@@ -27,6 +27,11 @@
 #include <drm/drm_encoder_slave.h>
 #include <drm/drm_edid.h>
 #include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
+
+#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE)
+#define WITH_CODEC 1
+#endif
 
 #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 

@@ -47,6 +52,10 @@ struct tda998x_priv {
        struct drm_encoder *encoder;
 
        u8 audio_ports[2];
+#ifdef WITH_CODEC
+       u8 sad[3];                      /* Short Audio Descriptor */
+       struct snd_pcm_hw_constraint_list rate_constraints;
+#endif
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)

@@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv,
        tda998x_write_aif(priv, p);
 }
 
+#ifdef WITH_CODEC
+/* tda998x audio codec interface */
+
+/* return the audio parameters extracted from the last EDID */
+static int tda998x_get_audio_var(struct device *dev,
+                               u8 **sad,
+                       struct snd_pcm_hw_constraint_list **rate_constraints)
+{
+       struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+       if (!priv->encoder->crtc
+        || !priv->is_hdmi_sink)
+               return -ENODEV;
+
+       *sad = priv->sad;
+       *rate_constraints = &priv->rate_constraints;
+       return 0;
+}
+
+/* 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,
+                               int sample_format)
+{
+       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 = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S;
+       p->audio_clk_cfg = port_index == PORT_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;
+}
+
+/* get the audio capabilities from the EDID */
+static void tda998x_get_audio_caps(struct tda998x_priv *priv,
+                               struct drm_connector *connector)
+{
+       u8 *eld = connector->eld;
+       u8 *sad;
+       int sad_count;
+       unsigned eld_ver, mnl;
+       u8 max_channels, rate_mask, fmt;
+
+       /* adjust the hw params from the ELD (EDID) */
+       eld_ver = eld[0] >> 3;
+       if (eld_ver != 2 && eld_ver != 31)
+               return;
+
+       mnl = eld[4] & 0x1f;
+       if (mnl > 16)
+               return;
+
+       sad_count = eld[5] >> 4;
+       sad = eld + 20 + mnl;
+
+       /* Start from the basic audio settings */
+       max_channels = 1;
+       rate_mask = 0;
+       fmt = 0;
+       while (sad_count--) {
+               switch (sad[0] & 0x78) {
+               case 0x08:                      /* SAD uncompressed audio */
+                       if ((sad[0] & 7) > max_channels)
+                               max_channels = sad[0] & 7;
+                       rate_mask |= sad[1];
+                       fmt |= sad[2] & 0x07;
+                       break;
+               }
+               sad += 3;
+       }
+
+       priv->sad[0] = max_channels;
+       priv->sad[1] = rate_mask;
+       priv->sad[2] = fmt;
+}
+#endif /* WITH_CODEC */
+
 /* DRM encoder functions */
 
 static void tda998x_encoder_set_config(struct tda998x_priv *priv,
                                       const struct tda998x_encoder_params *p)
 {
+       int port_index;
+
        priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
                            (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
                            VIP_CNTRL_0_SWAP_B(p->swap_b) |

@@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
                            (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
        priv->params = *p;
+       port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S;
+       priv->audio_ports[port_index] = p->audio_cfg;
 }
 
 static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)

@@ -1142,6 +1251,15 @@ 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);
+
+#ifdef WITH_CODEC
+               /* set the audio parameters from the EDID */
+               if (priv->is_hdmi_sink) {
+                       drm_edid_to_eld(connector, edid);
+                       tda998x_get_audio_caps(priv, connector);
+               }
+#endif
+
                kfree(edid);
        }
 

@@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv)
        if (priv->hdmi->irq)
                free_irq(priv->hdmi->irq, priv);
 
+#ifdef WITH_CODEC
+       tda9998x_codec_unregister(&priv->hdmi->dev);
+#endif
+
        i2c_unregister_device(priv->cec);
 }
 

@@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
                                break;
                        }
                        if (strcmp(p, "spdif") == 0) {
-                               priv->audio_ports[0] = port;
+                               priv->audio_ports[PORT_SPDIF] = port;
                        } else if (strcmp(p, "i2s") == 0) {
-                               priv->audio_ports[1] = port;
+                               priv->audio_ports[PORT_I2S] = port;
                        } else {
                                dev_err(&client->dev,
                                        "bad audio-port-names '%s'\n", p);
                                break;
                        }
                }
-               if (priv->audio_ports[0]) {
-                       priv->params.audio_cfg = priv->audio_ports[0];
+               if (priv->audio_ports[PORT_SPDIF]) {
+                       priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF];
                        priv->params.audio_format = AFMT_SPDIF;
                        priv->params.audio_clk_cfg = 0;
                } else {
-                       priv->params.audio_cfg = priv->audio_ports[1];
+                       priv->params.audio_cfg = priv->audio_ports[PORT_I2S];
                        priv->params.audio_format = AFMT_I2S;
                        priv->params.audio_clk_cfg = 1;
                }
        }
 
+#ifdef WITH_CODEC
+       /* create the audio CODEC */
+       if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S])
+               tda9998x_codec_register(&client->dev,
+                                       tda998x_get_audio_var,
+                                       tda998x_set_audio_input);
+#endif
        return 0;
 
 fail:

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

new file mode 100644
index 0000000..97cff30
@@ -0,0 +1,23 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+#include <sound/pcm.h>
+
+/* port indexes */
+#define PORT_NONE (-1)
+#define PORT_SPDIF 0
+#define PORT_I2S 1
+
+typedef int (*get_audio_var_t)(struct device *dev,
+                               u8 **sad,
+                       struct snd_pcm_hw_constraint_list **rate_constraints);
+typedef int (*set_audio_input_t)(struct device *dev,
+                               int port_index,
+                               unsigned sample_rate,
+                               int sample_format);
+
+int tda9998x_codec_register(struct device *dev,
+                       get_audio_var_t get_audio_var_i,
+                       set_audio_input_t set_audio_input_i);
+void tda9998x_codec_unregister(struct device *dev);
+#endif

sound/soc/codecs/Kconfig +4 -0

index 8349f98..456c549 100644
@@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS
        select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
        select SND_SOC_TAS2552 if I2C
        select SND_SOC_TAS5086 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

@@ -600,6 +601,9 @@ config SND_SOC_TAS5086
        tristate "Texas Instruments TAS5086 speaker amplifier"
        depends on I2C
 
+config SND_SOC_TDA998X
+       tristate
+
 config SND_SOC_TFA9879
        tristate "NXP Semiconductors TFA9879 amplifier"
        depends on I2C

sound/soc/codecs/Makefile +2 -0

index bbdfd1e..44b4fda 100644
@@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o
 snd-soc-sta529-objs := sta529.o
 snd-soc-stac9766-objs := stac9766.o
 snd-soc-tas5086-objs := tas5086.o
+snd-soc-tda998x-objs := tda998x.o
 snd-soc-tfa9879-objs := tfa9879.o
 snd-soc-tlv320aic23-objs := tlv320aic23.o
 snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o

@@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529)   += snd-soc-sta529.o
 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_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

sound/soc/codecs/tda998x.c new file +23 -0

new file mode 100644
index 0000000..b055570
@@ -0,0 +1,175 @@
+/*
+ * 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_params.h>
+#include <sound/tda998x.h>
+
+/* functions in tda998x_drv */
+static get_audio_var_t get_audio_var;
+static set_audio_input_t set_audio_input;
+
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+                                struct snd_soc_dai *dai)

sound/soc/codecs/tda998x.c new file +142 -1

+       u64 formats;
+       int ret;
+       static const u32 hdmi_rates[] = {
+               32000, 44100, 48000, 88200, 96000, 176400, 192000
+       };
+
+       /* get the EDID values and the rate constraints buffer */
+       ret = get_audio_var(dai->dev, &sad, &rate_constraints);
+       if (ret < 0)
+               return ret;                     /* no screen */
+
+       /* convert the EDID values to audio constraints */
+       rate_constraints->list = hdmi_rates;
+       rate_constraints->count = ARRAY_SIZE(hdmi_rates);
+       rate_constraints->mask = sad[SAD_RATE_MASK_I];
+       snd_pcm_hw_constraint_list(runtime, 0,
+                                  SNDRV_PCM_HW_PARAM_RATE,
+                                  rate_constraints);
+
+       formats = 0;
+       if (sad[SAD_FMT_I] & SAD_FMT_16)
+               formats |= SNDRV_PCM_FMTBIT_S16_LE;
+       if (sad[SAD_FMT_I] & SAD_FMT_20)
+               formats |= SNDRV_PCM_FMTBIT_S20_3LE;
+       if (sad[SAD_FMT_I] & SAD_FMT_24)
+               formats |= SNDRV_PCM_FMTBIT_S24_LE |
+                          SNDRV_PCM_FMTBIT_S24_3LE |
+                          SNDRV_PCM_FMTBIT_S32_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, sad[SAD_MX_CHAN_I]);
+       return 0;
+}
+
+static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
+                                  struct snd_pcm_hw_params *params,
+                                  struct snd_soc_dai *dai)
+{
+       return set_audio_input(dai->dev, dai->id,
+                               params_rate(params),
+                               params_format(params));
+}
+
+static void tda998x_codec_shutdown(struct snd_pcm_substream *substream,
+                                  struct snd_soc_dai *dai)
+{
+       set_audio_input(dai->dev, PORT_NONE, 0, 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_dais[] = {
+       {
+               .name = "spdif-hifi",
+               .id = PORT_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 = &tda998x_codec_ops,
+       },
+       {
+               .name = "i2s-hifi",
+               .id = PORT_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 = &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,
+                       get_audio_var_t get_audio_var_i,
+                       set_audio_input_t set_audio_input_i)
+{
+       get_audio_var = get_audio_var_i;
+       set_audio_input = set_audio_input_i;
+       return snd_soc_register_codec(dev,
+                               &tda998x_codec_drv,
+                               tda998x_dais, ARRAY_SIZE(tda998x_dais));
+}
+EXPORT_SYMBOL(tda9998x_codec_register);
+
+void tda9998x_codec_unregister(struct device *dev)
+{
+       snd_soc_unregister_codec(dev);
+}
+EXPORT_SYMBOL(tda9998x_codec_unregister);
+
+static int __init tda998x_codec_init(void)
+{
+       return 0;
+}
+static void __exit tda998x_codec_exit(void)
+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);
+
+MODULE_AUTHOR("Jean-Francois Moine <[email protected]>");
+MODULE_DESCRIPTION("TDA998X CODEC");
+MODULE_LICENSE("GPL");
--

2.1.4

drivers/gpu/drm/i2c/Kconfig config DRM_I2C_SIL164 · line 31

Quoted 9 lines · Jean-Francois Moine

drivers/gpu/drm/i2c/Kconfig +1 -0

index 22c7ed6..24fc975 100644
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
 config DRM_I2C_NXP_TDA998X
        tristate "NXP Semiconductors TDA998X HDMI encoder"
        default m if DRM_TILCDC
+       select SND_SOC_TDA998X

Do you need this since sound/soc/codecs/Kconfig conditionally brings in the CODEC driver?

drivers/gpu/drm/i2c/Kconfig config DRM_I2C_SIL164

Quoted 9 lines

drivers/gpu/drm/i2c/Kconfig +9 -0

+static int __init tda998x_codec_init(void)
+{
+       return 0;
+}
+static void __exit tda998x_codec_exit(void)
+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);

Quoted 13 lines · Andrew Jackson

drivers/gpu/drm/i2c/Kconfig config DRM_I2C_SIL164 · line 31

Quoted 9 lines · Jean-Francois Moine

drivers/gpu/drm/i2c/Kconfig +1 -0

index 22c7ed6..24fc975 100644
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
 config DRM_I2C_NXP_TDA998X
        tristate "NXP Semiconductors TDA998X HDMI encoder"
        default m if DRM_TILCDC
+       select SND_SOC_TDA998X

Do you need this since sound/soc/codecs/Kconfig conditionally brings in the CODEC driver?

More importantly, it's broken, because it'll cause Kconfig to complain if you enable DRM_I2C_NXP_TDA998X, but have ASoC disabled.

Moreover, if you decide you want the sound and ASoC built as a module, but want to build in DRM and TDA998x support (so you can get video output working on boot before initramfs/rootfs), Kconfig may well complain.

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i2c/Kconfig +5 -0

+static int __init tda998x_codec_init(void)
+{
+       return 0;
+}
+static void __exit tda998x_codec_exit(void)

Quoted 4 lines

drivers/gpu/drm/i2c/Kconfig

Quoted 4 lines

drivers/gpu/drm/i2c/Kconfig +4 -0

+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);

There's no need for these if they remain as the above. Modules can have both init/exit functions, or neither, and they are still unloadable. Only modules which provide only an init function get locked in on load.

Quoted 14 lines · On Wed, 7 Jan 2015 15:41:38 +0000 Russell King - ARM Linux…

Quoted 13 lines · Andrew Jackson

drivers/gpu/drm/i2c/Kconfig config DRM_I2C_SIL164 · line 31

Quoted 9 lines · Jean-Francois Moine

drivers/gpu/drm/i2c/Kconfig +1 -0

index 22c7ed6..24fc975 100644
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
  config DRM_I2C_NXP_TDA998X
         tristate "NXP Semiconductors TDA998X HDMI encoder"
         default m if DRM_TILCDC
+       select SND_SOC_TDA998X

Do you need this since sound/soc/codecs/Kconfig conditionally brings in the CODEC driver?

For SND_SOC_ALL_CODECS only.

Quoted 3 lines

More importantly, it's broken, because it'll cause Kconfig to complain if you enable DRM_I2C_NXP_TDA998X, but have ASoC disabled.

Quoted 4 lines

Moreover, if you decide you want the sound and ASoC built as a module, but want to build in DRM and TDA998x support (so you can get video output working on boot before initramfs/rootfs), Kconfig may well complain.

select SND_SOC_TDA998X if SND_SOC

should work in all cases.

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

drivers/gpu/drm/i2c/Kconfig +5 -0

+static int __init tda998x_codec_init(void)
+{
+       return 0;
+}
+static void __exit tda998x_codec_exit(void)

Quoted 8 lines

Quoted 4 lines

drivers/gpu/drm/i2c/Kconfig

Quoted 4 lines

drivers/gpu/drm/i2c/Kconfig +4 -0

+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);

There's no need for these if they remain as the above. Modules can have both init/exit functions, or neither, and they are still unloadable. Only modules which provide only an init function get locked in on load.

Thanks.

Quoted 4 lines

select SND_SOC_TDA998X if SND_SOC

should work in all cases.

I think that would still fail if DRM and TDA998x is built in and SND_SOC is built as modules. A request_module() call before tda9998x_codec_register() should help. Or could could write:

select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y)

I think that would still fail if DRM and TDA998x is built in and SND_SOC is built as modules. A request_module() call before tda9998x_codec_register() should help.

Or could could write:

select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y)

Would it be more sensible to make SND_SOC_TDA998X depend on DRM_I2C_NXP_TDA998X instead, maybe with a 'default y' - which is a kinder way to have SND_SOC_TDA998X be enabled. If SND_SOC_TDA998X doesn't have a prompt, then it'll automatically enable itself too this way when all its dependencies are satisfied.

sound/soc/codecs/Kconfig +5 -0

index b33b45d..747e387 100644
@@ -352,6 +352,12 @@ config SND_SOC_STAC9766
 config SND_SOC_TAS5086
 	tristate
 
+config SND_SOC_TDA998X
+	tristate
+	default y if DRM_I2C_NXP_TDA998X=y
+	default m if DRM_I2C_NXP_TDA998X=m
+

Quoted 5 lines · On Fri, 9 Jan 2015 11:19:35 +0000 Russell King - ARM Linux…

Would it be more sensible to make SND_SOC_TDA998X depend on DRM_I2C_NXP_TDA998X instead, maybe with a 'default y' - which is a kinder way to have SND_SOC_TDA998X be enabled. If SND_SOC_TDA998X doesn't have a prompt, then it'll automatically enable itself too this way when all its dependencies are satisfied.

You are right, I will set back the way I had in the version 3:

Quoted 8 lines

sound/soc/codecs/Kconfig +0 -0

index b33b45d..747e387 100644
@@ -352,6 +352,12 @@ config SND_SOC_STAC9766
 config SND_SOC_TAS5086
 	tristate
 

sound/soc/codecs/Kconfig config SND_SOC_STAC9766 · line 359

Quoted 5 lines

sound/soc/codecs/Kconfig +5 -0

+config SND_SOC_TDA998X
+	tristate
+	default y if DRM_I2C_NXP_TDA998X=y
+	default m if DRM_I2C_NXP_TDA998X=m
+

Quoted 3 lines · Jean-Francois Moine

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

The CODEC handles both I2S and S/PDIF inputs.

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

The CODEC handles both I2S and S/PDIF inputs.
It maintains the audio format and rate 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 <[email protected]>

---
  drivers/gpu/drm/i2c/Kconfig       |   1 +
  drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++--
  include/sound/tda998x.h           |  23 +++++
  sound/soc/codecs/Kconfig          |   4 +
  sound/soc/codecs/Makefile         |   2 +
  sound/soc/codecs/tda998x.c        | 175 ++++++++++++++++++++++++++++++++++++++
  6 files changed, 339 insertions(+), 5 deletions(-)

create mode 100644 include/sound/tda998x.h
create mode 100644 sound/soc/codecs/tda998x.c

drivers/gpu/drm/i2c/Kconfig +1 -0

index 22c7ed6..24fc975 100644
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
  config DRM_I2C_NXP_TDA998X
  	tristate "NXP Semiconductors TDA998X HDMI encoder"
  	default m if DRM_TILCDC
+	select SND_SOC_TDA998X
  	help
  	  Support for NXP Semiconductors TDA998X HDMI encoders.
 

drivers/gpu/drm/i2c/tda998x_drv.c +0 -0

index ce1478d..a26a516 100644

drivers/gpu/drm/i2c/tda998x_drv.c +2 -0

+	struct snd_pcm_hw_constraint_list rate_constraints;
+#endif
  };
 
  #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)

drivers/gpu/drm/i2c/tda998x_drv.c +40 -0

+			struct snd_pcm_hw_constraint_list **rate_constraints)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	if (!priv->encoder->crtc
+	 || !priv->is_hdmi_sink)
+		return -ENODEV;
+
+	*sad = priv->sad;
+	*rate_constraints = &priv->rate_constraints;
+	return 0;
+}
+
+/* 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,
+				int sample_format)
+{
+	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 = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S;
+	p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1;

drivers/gpu/drm/i2c/tda998x_drv.c +69 -5

+	u8 *eld = connector->eld;
+	u8 *sad;
+	int sad_count;
+	unsigned eld_ver, mnl;
+	u8 max_channels, rate_mask, fmt;
+
+	/* adjust the hw params from the ELD (EDID) */
+	eld_ver = eld[0] >> 3;
+	if (eld_ver != 2 && eld_ver != 31)
+		return;
+
+	mnl = eld[4] & 0x1f;
+	if (mnl > 16)
+		return;
+
+	sad_count = eld[5] >> 4;
+	sad = eld + 20 + mnl;
+
+	/* Start from the basic audio settings */
+	max_channels = 1;
+	rate_mask = 0;
+	fmt = 0;
+	while (sad_count--) {
+		switch (sad[0] & 0x78) {
+		case 0x08:			/* SAD uncompressed audio */
+			if ((sad[0] & 7) > max_channels)
+				max_channels = sad[0] & 7;
+			rate_mask |= sad[1];
+			fmt |= sad[2] & 0x07;
+			break;
+		}
+		sad += 3;
+	}
+
+	priv->sad[0] = max_channels;
+	priv->sad[1] = rate_mask;
+	priv->sad[2] = fmt;
+}
+#endif /* WITH_CODEC */
+
  /* DRM encoder functions */
 
  static void tda998x_encoder_set_config(struct tda998x_priv *priv,
  				       const struct tda998x_encoder_params *p)
  {
+	int port_index;
+
  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
  			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
  			    VIP_CNTRL_0_SWAP_B(p->swap_b) |

@@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
  			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
  	priv->params = *p;
+	port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S;
+	priv->audio_ports[port_index] = p->audio_cfg;
  }
 
  static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)

@@ -1142,6 +1251,15 @@ 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);
+
+#ifdef WITH_CODEC
+		/* set the audio parameters from the EDID */
+		if (priv->is_hdmi_sink) {
+			drm_edid_to_eld(connector, edid);
+			tda998x_get_audio_caps(priv, connector);
+		}
+#endif
+
  		kfree(edid);
  	}
 

@@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv)
  	if (priv->hdmi->irq)
  		free_irq(priv->hdmi->irq, priv);
 
+#ifdef WITH_CODEC
+	tda9998x_codec_unregister(&priv->hdmi->dev);
+#endif
+
  	i2c_unregister_device(priv->cec);
  }
 

@@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
  				break;
  			}
  			if (strcmp(p, "spdif") == 0) {
-				priv->audio_ports[0] = port;
+				priv->audio_ports[PORT_SPDIF] = port;
  			} else if (strcmp(p, "i2s") == 0) {
-				priv->audio_ports[1] = port;
+				priv->audio_ports[PORT_I2S] = port;
  			} else {
  				dev_err(&client->dev,
  					"bad audio-port-names '%s'\n", p);
  				break;
  			}
  		}
-		if (priv->audio_ports[0]) {
-			priv->params.audio_cfg = priv->audio_ports[0];
+		if (priv->audio_ports[PORT_SPDIF]) {
+			priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF];
  			priv->params.audio_format = AFMT_SPDIF;
  			priv->params.audio_clk_cfg = 0;
  		} else {
-			priv->params.audio_cfg = priv->audio_ports[1];
+			priv->params.audio_cfg = priv->audio_ports[PORT_I2S];
  			priv->params.audio_format = AFMT_I2S;
  			priv->params.audio_clk_cfg = 1;
  		}
  	}
 
+#ifdef WITH_CODEC
+	/* create the audio CODEC */
+	if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S])
+		tda9998x_codec_register(&client->dev,
+					tda998x_get_audio_var,
+					tda998x_set_audio_input);
+#endif
  	return 0;
 
  fail:

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

new file mode 100644
index 0000000..97cff30
@@ -0,0 +1,23 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H

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

+			struct snd_pcm_hw_constraint_list **rate_constraints);
+typedef int (*set_audio_input_t)(struct device *dev,
+				int port_index,
+				unsigned sample_rate,
+				int sample_format);
+
+int tda9998x_codec_register(struct device *dev,
+			get_audio_var_t get_audio_var_i,
+			set_audio_input_t set_audio_input_i);
+void tda9998x_codec_unregister(struct device *dev);
+#endif

sound/soc/codecs/Kconfig +4 -0

index 8349f98..456c549 100644
@@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS
  	select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
  	select SND_SOC_TAS2552 if I2C
  	select SND_SOC_TAS5086 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

@@ -600,6 +601,9 @@ config SND_SOC_TAS5086
  	tristate "Texas Instruments TAS5086 speaker amplifier"
  	depends on I2C
 
+config SND_SOC_TDA998X
+	tristate
+
  config SND_SOC_TFA9879
  	tristate "NXP Semiconductors TFA9879 amplifier"
  	depends on I2C

sound/soc/codecs/Makefile +2 -0

index bbdfd1e..44b4fda 100644
@@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o
  snd-soc-sta529-objs := sta529.o
  snd-soc-stac9766-objs := stac9766.o
  snd-soc-tas5086-objs := tas5086.o
+snd-soc-tda998x-objs := tda998x.o
  snd-soc-tfa9879-objs := tfa9879.o
  snd-soc-tlv320aic23-objs := tlv320aic23.o
  snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o

@@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529)   += snd-soc-sta529.o
  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_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

sound/soc/codecs/tda998x.c new file +23 -0

new file mode 100644
index 0000000..b055570
@@ -0,0 +1,175 @@
+/*
+ * 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_params.h>
+#include <sound/tda998x.h>
+
+/* functions in tda998x_drv */
+static get_audio_var_t get_audio_var;
+static set_audio_input_t set_audio_input;
+
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+				 struct snd_soc_dai *dai)

sound/soc/codecs/tda998x.c new file +142 -0

+	u64 formats;
+	int ret;
+	static const u32 hdmi_rates[] = {
+		32000, 44100, 48000, 88200, 96000, 176400, 192000
+	};
+
+	/* get the EDID values and the rate constraints buffer */
+	ret = get_audio_var(dai->dev, &sad, &rate_constraints);
+	if (ret < 0)
+		return ret;			/* no screen */
+
+	/* convert the EDID values to audio constraints */
+	rate_constraints->list = hdmi_rates;
+	rate_constraints->count = ARRAY_SIZE(hdmi_rates);
+	rate_constraints->mask = sad[SAD_RATE_MASK_I];
+	snd_pcm_hw_constraint_list(runtime, 0,
+				   SNDRV_PCM_HW_PARAM_RATE,
+				   rate_constraints);
+
+	formats = 0;
+	if (sad[SAD_FMT_I] & SAD_FMT_16)
+		formats |= SNDRV_PCM_FMTBIT_S16_LE;
+	if (sad[SAD_FMT_I] & SAD_FMT_20)
+		formats |= SNDRV_PCM_FMTBIT_S20_3LE;
+	if (sad[SAD_FMT_I] & SAD_FMT_24)
+		formats |= SNDRV_PCM_FMTBIT_S24_LE |
+			   SNDRV_PCM_FMTBIT_S24_3LE |
+			   SNDRV_PCM_FMTBIT_S32_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, sad[SAD_MX_CHAN_I]);
+	return 0;
+}
+
+static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params,
+				   struct snd_soc_dai *dai)
+{
+	return set_audio_input(dai->dev, dai->id,
+				params_rate(params),
+				params_format(params));
+}
+
+static void tda998x_codec_shutdown(struct snd_pcm_substream *substream,
+				   struct snd_soc_dai *dai)
+{
+	set_audio_input(dai->dev, PORT_NONE, 0, 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_dais[] = {
+	{
+		.name = "spdif-hifi",
+		.id = PORT_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 = &tda998x_codec_ops,
+	},
+	{
+		.name = "i2s-hifi",
+		.id = PORT_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 = &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,
+			get_audio_var_t get_audio_var_i,
+			set_audio_input_t set_audio_input_i)
+{
+	get_audio_var = get_audio_var_i;
+	set_audio_input = set_audio_input_i;
+	return snd_soc_register_codec(dev,
+				&tda998x_codec_drv,
+				tda998x_dais, ARRAY_SIZE(tda998x_dais));
+}
+EXPORT_SYMBOL(tda9998x_codec_register);
+
+void tda9998x_codec_unregister(struct device *dev)
+{
+	snd_soc_unregister_codec(dev);
+}
+EXPORT_SYMBOL(tda9998x_codec_unregister);
+
+static int __init tda998x_codec_init(void)
+{
+	return 0;
+}
+static void __exit tda998x_codec_exit(void)
+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);
+
+MODULE_AUTHOR("Jean-Francois Moine <[email protected]>");
+MODULE_DESCRIPTION("TDA998X CODEC");
+MODULE_LICENSE("GPL");

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

The CODEC handles both I2S and S/PDIF inputs.
It maintains the audio format and rate 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 <[email protected]>

---

sound/soc/codecs/tda998x.c new file +6 -0

+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+                                struct snd_soc_dai *dai)
+{
+       struct snd_pcm_runtime *runtime = substream->runtime;
+       struct snd_pcm_hw_constraint_list *rate_constraints;
+       u8 *sad;                /* Short Audio Descriptor (3 bytes) */

In the light of our discussions elsewhere [1], shouldn't this be constrained by the number of hardware channels that the TDA998x supports too? That is, the maximum number of channels should be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels (or S/PDIF channels if so configured).

sound/soc/codecs/tda998x.c line 68

Quoted 3 lines

sound/soc/codecs/tda998x.c new file +3 -0

+       snd_pcm_hw_constraint_minmax(runtime,
+                               SNDRV_PCM_HW_PARAM_CHANNELS,
+                               1, sad[SAD_MX_CHAN_I]);  

In the light of our discussions elsewhere [1], shouldn't this be constrained by the number of hardware channels that the TDA998x supports too? That is, the maximum number of channels should be the lesser of sd[SAD_MX_CHAN_I] and number_of_I2S channels (or S/PDIF channels if so configured).

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

The CODEC handles both I2S and S/PDIF inputs.
It maintains the audio format and rate 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 <[email protected]>

---
  drivers/gpu/drm/i2c/Kconfig       |   1 +
  drivers/gpu/drm/i2c/tda998x_drv.c | 139 ++++++++++++++++++++++++++++--
  include/sound/tda998x.h           |  23 +++++
  sound/soc/codecs/Kconfig          |   4 +
  sound/soc/codecs/Makefile         |   2 +
  sound/soc/codecs/tda998x.c        | 175 ++++++++++++++++++++++++++++++++++++++
  6 files changed, 339 insertions(+), 5 deletions(-)

create mode 100644 include/sound/tda998x.h
create mode 100644 sound/soc/codecs/tda998x.c

drivers/gpu/drm/i2c/Kconfig +1 -0

index 22c7ed6..24fc975 100644
@@ -28,6 +28,7 @@ config DRM_I2C_SIL164
  config DRM_I2C_NXP_TDA998X
  	tristate "NXP Semiconductors TDA998X HDMI encoder"
  	default m if DRM_TILCDC
+	select SND_SOC_TDA998X
  	help
  	  Support for NXP Semiconductors TDA998X HDMI encoders.
 

drivers/gpu/drm/i2c/tda998x_drv.c +5 -0

index ce1478d..a26a516 100644
@@ -27,6 +27,11 @@
  #include <drm/drm_encoder_slave.h>
  #include <drm/drm_edid.h>
  #include <drm/i2c/tda998x.h>
+#include <sound/tda998x.h>
+
+#if defined(CONFIG_SND_SOC_TDA998X) || defined(CONFIG_SND_SOC_TDA998X_MODULE)
+#define WITH_CODEC 1
+#endif
 
  #define DBG(fmt, ...) DRM_DEBUG(fmt"\n", ##__VA_ARGS__)
 

drivers/gpu/drm/i2c/tda998x_drv.c +125 -5

  };
 
  #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)

@@ -730,11 +739,109 @@ tda998x_configure_audio(struct tda998x_priv *priv,
  	tda998x_write_aif(priv, p);
  }
 
+#ifdef WITH_CODEC
+/* tda998x audio codec interface */
+
+/* return the audio parameters extracted from the last EDID */
+static int tda998x_get_audio_var(struct device *dev,
+				u8 **sad,
+			struct snd_pcm_hw_constraint_list **rate_constraints)
+{
+	struct tda998x_priv *priv = dev_get_drvdata(dev);
+
+	if (!priv->encoder->crtc
+	 || !priv->is_hdmi_sink)
+		return -ENODEV;
+
+	*sad = priv->sad;
+	*rate_constraints = &priv->rate_constraints;
+	return 0;
+}
+
+/* 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,
+				int sample_format)
+{
+	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 = port_index == PORT_SPDIF ? AFMT_SPDIF : AFMT_I2S;
+	p->audio_clk_cfg = port_index == PORT_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;
+}
+
+/* get the audio capabilities from the EDID */
+static void tda998x_get_audio_caps(struct tda998x_priv *priv,
+				struct drm_connector *connector)
+{
+	u8 *eld = connector->eld;
+	u8 *sad;
+	int sad_count;
+	unsigned eld_ver, mnl;
+	u8 max_channels, rate_mask, fmt;
+
+	/* adjust the hw params from the ELD (EDID) */
+	eld_ver = eld[0] >> 3;
+	if (eld_ver != 2 && eld_ver != 31)
+		return;
+
+	mnl = eld[4] & 0x1f;
+	if (mnl > 16)
+		return;
+
+	sad_count = eld[5] >> 4;
+	sad = eld + 20 + mnl;
+SNDRV_PCM_RATE_CONTINUOUS
+	/* Start from the basic audio settings */
+	max_channels = 1;
+	rate_mask = 0;
+	fmt = 0;
+	while (sad_count--) {
+		switch (sad[0] & 0x78) {
+		case 0x08:			/* SAD uncompressed audio */
+			if ((sad[0] & 7) > max_channels)
+				max_channels = sad[0] & 7;
+			rate_mask |= sad[1];
+			fmt |= sad[2] & 0x07;
+			break;
+		}
+		sad += 3;
+	}
+
+	priv->sad[0] = max_channels;
+	priv->sad[1] = rate_mask;
+	priv->sad[2] = fmt;
+}
+#endif /* WITH_CODEC */
+
  /* DRM encoder functions */
 
  static void tda998x_encoder_set_config(struct tda998x_priv *priv,
  				       const struct tda998x_encoder_params *p)
  {
+	int port_index;
+
  	priv->vip_cntrl_0 = VIP_CNTRL_0_SWAP_A(p->swap_a) |
  			    (p->mirr_a ? VIP_CNTRL_0_MIRR_A : 0) |
  			    VIP_CNTRL_0_SWAP_B(p->swap_b) |

@@ -749,6 +856,8 @@ static void tda998x_encoder_set_config(struct tda998x_priv *priv,
  			    (p->mirr_f ? VIP_CNTRL_2_MIRR_F : 0);
 
  	priv->params = *p;
+	port_index = p->audio_format == AFMT_SPDIF ? PORT_SPDIF : PORT_I2S;
+	priv->audio_ports[port_index] = p->audio_cfg;
  }
 
  static void tda998x_encoder_dpms(struct tda998x_priv *priv, int mode)

@@ -1142,6 +1251,15 @@ 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);
+
+#ifdef WITH_CODEC
+		/* set the audio parameters from the EDID */
+		if (priv->is_hdmi_sink) {
+			drm_edid_to_eld(connector, edid);
+			tda998x_get_audio_caps(priv, connector);
+		}
+#endif
+
  		kfree(edid);
  	}
 

@@ -1176,6 +1294,10 @@ static void tda998x_destroy(struct tda998x_priv *priv)
  	if (priv->hdmi->irq)
  		free_irq(priv->hdmi->irq, priv);
 
+#ifdef WITH_CODEC
+	tda9998x_codec_unregister(&priv->hdmi->dev);
+#endif
+
  	i2c_unregister_device(priv->cec);
  }
 

@@ -1384,26 +1506,33 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
  				break;
  			}
  			if (strcmp(p, "spdif") == 0) {
-				priv->audio_ports[0] = port;
+				priv->audio_ports[PORT_SPDIF] = port;
  			} else if (strcmp(p, "i2s") == 0) {
-				priv->audio_ports[1] = port;
+				priv->audio_ports[PORT_I2S] = port;
  			} else {
  				dev_err(&client->dev,
  					"bad audio-port-names '%s'\n", p);
  				break;
  			}
  		}
-		if (priv->audio_ports[0]) {
-			priv->params.audio_cfg = priv->audio_ports[0];
+		if (priv->audio_ports[PORT_SPDIF]) {
+			priv->params.audio_cfg = priv->audio_ports[PORT_SPDIF];
  			priv->params.audio_format = AFMT_SPDIF;
  			priv->params.audio_clk_cfg = 0;
  		} else {
-			priv->params.audio_cfg = priv->audio_ports[1];
+			priv->params.audio_cfg = priv->audio_ports[PORT_I2S];
  			priv->params.audio_format = AFMT_I2S;
  			priv->params.audio_clk_cfg = 1;
  		}
  	}
 
+#ifdef WITH_CODEC
+	/* create the audio CODEC */
+	if (priv->audio_ports[PORT_SPDIF] || priv->audio_ports[PORT_I2S])
+		tda9998x_codec_register(&client->dev,
+					tda998x_get_audio_var,
+					tda998x_set_audio_input);
+#endif
  	return 0;
 
  fail:

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

new file mode 100644
index 0000000..97cff30
@@ -0,0 +1,23 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+#include <sound/pcm.h>
+
+/* port indexes */
+#define PORT_NONE (-1)
+#define PORT_SPDIF 0

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

+int tda9998x_codec_register(struct device *dev,
+			get_audio_var_t get_audio_var_i,
+			set_audio_input_t set_audio_input_i);
+void tda9998x_codec_unregister(struct device *dev);
+#endif

sound/soc/codecs/Kconfig +4 -0

index 8349f98..456c549 100644
@@ -102,6 +102,7 @@ config SND_SOC_ALL_CODECS
  	select SND_SOC_STAC9766 if SND_SOC_AC97_BUS
  	select SND_SOC_TAS2552 if I2C
  	select SND_SOC_TAS5086 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

@@ -600,6 +601,9 @@ config SND_SOC_TAS5086
  	tristate "Texas Instruments TAS5086 speaker amplifier"
  	depends on I2C
 
+config SND_SOC_TDA998X
+	tristate
+
  config SND_SOC_TFA9879
  	tristate "NXP Semiconductors TFA9879 amplifier"
  	depends on I2C

sound/soc/codecs/Makefile +2 -0

index bbdfd1e..44b4fda 100644
@@ -104,6 +104,7 @@ snd-soc-sta350-objs := sta350.o
  snd-soc-sta529-objs := sta529.o
  snd-soc-stac9766-objs := stac9766.o
  snd-soc-tas5086-objs := tas5086.o
+snd-soc-tda998x-objs := tda998x.o
  snd-soc-tfa9879-objs := tfa9879.o
  snd-soc-tlv320aic23-objs := tlv320aic23.o
  snd-soc-tlv320aic23-i2c-objs := tlv320aic23-i2c.o

@@ -282,6 +283,7 @@ obj-$(CONFIG_SND_SOC_STA529)   += snd-soc-sta529.o
  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_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

sound/soc/codecs/tda998x.c new file +58 -0

new file mode 100644
index 0000000..b055570
@@ -0,0 +1,175 @@
+/*
+ * 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_params.h>
+#include <sound/tda998x.h>
+
+/* functions in tda998x_drv */
+static get_audio_var_t get_audio_var;
+static set_audio_input_t set_audio_input;
+
+static int tda998x_codec_startup(struct snd_pcm_substream *substream,
+				 struct snd_soc_dai *dai)
+{
+	struct snd_pcm_runtime *runtime = substream->runtime;
+	struct snd_pcm_hw_constraint_list *rate_constraints;
+	u8 *sad;		/* Short Audio Descriptor (3 bytes) */
+#define SAD_MX_CHAN_I 0			/* max number of chennels - 1 */
+#define SAD_RATE_MASK_I 1		/* rate mask */
+#define SAD_FMT_I 2			/* formats */
+#define  SAD_FMT_16 0x01
+#define  SAD_FMT_20 0x02
+#define  SAD_FMT_24 0x04
+	u64 formats;
+	int ret;
+	static const u32 hdmi_rates[] = {
+		32000, 44100, 48000, 88200, 96000, 176400, 192000
+	};
+
+	/* get the EDID values and the rate constraints buffer */
+	ret = get_audio_var(dai->dev, &sad, &rate_constraints);
+	if (ret < 0)
+		return ret;			/* no screen */
+
+	/* convert the EDID values to audio constraints */
+	rate_constraints->list = hdmi_rates;
+	rate_constraints->count = ARRAY_SIZE(hdmi_rates);
+	rate_constraints->mask = sad[SAD_RATE_MASK_I];
+	snd_pcm_hw_constraint_list(runtime, 0,
+				   SNDRV_PCM_HW_PARAM_RATE,
+				   rate_constraints);
+
+	formats = 0;
+	if (sad[SAD_FMT_I] & SAD_FMT_16)
+		formats |= SNDRV_PCM_FMTBIT_S16_LE;
+	if (sad[SAD_FMT_I] & SAD_FMT_20)
+		formats |= SNDRV_PCM_FMTBIT_S20_3LE;
+	if (sad[SAD_FMT_I] & SAD_FMT_24)

sound/soc/codecs/tda998x.c new file +49 -0

+	return 0;
+}
+
+static int tda998x_codec_hw_params(struct snd_pcm_substream *substream,
+				   struct snd_pcm_hw_params *params,
+				   struct snd_soc_dai *dai)
+{
+	return set_audio_input(dai->dev, dai->id,
+				params_rate(params),
+				params_format(params));
+}
+
+static void tda998x_codec_shutdown(struct snd_pcm_substream *substream,
+				   struct snd_soc_dai *dai)
+{
+	set_audio_input(dai->dev, PORT_NONE, 0, 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_dais[] = {
+	{
+		.name = "spdif-hifi",
+		.id = PORT_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 = &tda998x_codec_ops,
+	},
+	{
+		.name = "i2s-hifi",
+		.id = PORT_I2S,
+		.playback = {
+			.stream_name	= "HDMI I2S Playback",

sound/soc/codecs/tda998x.c new file +16 -0

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

sound/soc/codecs/tda998x.c new file +22 -0

+}
+EXPORT_SYMBOL(tda9998x_codec_register);
+
+void tda9998x_codec_unregister(struct device *dev)
+{
+	snd_soc_unregister_codec(dev);
+}
+EXPORT_SYMBOL(tda9998x_codec_unregister);
+
+static int __init tda998x_codec_init(void)
+{
+	return 0;
+}
+static void __exit tda998x_codec_exit(void)
+{
+}
+module_init(tda998x_codec_init);
+module_exit(tda998x_codec_exit);
+
+MODULE_AUTHOR("Jean-Francois Moine <[email protected]>");
+MODULE_DESCRIPTION("TDA998X CODEC");
+MODULE_LICENSE("GPL");

Some more comments based on my - finally successful - attempt to use this code with BBB HDMI audio.

Quoted 1 line · Jean-Francois Moine

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

drivers/gpu/drm/i2c/tda998x_drv.c struct tda998x_priv { · line 58

Quoted 8 lines

drivers/gpu/drm/i2c/tda998x_drv.c +4 -0

@@ -47,6 +52,10 @@ struct tda998x_priv {
  	struct drm_encoder *encoder;
 
  	u8 audio_ports[2];
+#ifdef WITH_CODEC
+	u8 sad[3];			/* Short Audio Descriptor */
+	struct snd_pcm_hw_constraint_list rate_constraints;
+#endif

It feels a bit backwards to me to store these audio side variables here in DRM side driver - especially the rate_constraint - and provide a function (tda998x_get_audio_var()) for getting their individual addresses to ASoC side. Wouldn't it be more straight forward to provide functions for setting and getting the audio side data from a void pointer in DRM side device drvdata?

Quoted 22 lines

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

new file mode 100644
index 0000000..97cff30
@@ -0,0 +1,23 @@
+#ifndef SND_TDA998X_H
+#define SND_TDA998X_H
+
+#include <sound/pcm.h>
+
+/* port indexes */
+#define PORT_NONE (-1)
+#define PORT_SPDIF 0
+#define PORT_I2S 1
+
+typedef int (*get_audio_var_t)(struct device *dev,
+				u8 **sad,
+			struct snd_pcm_hw_constraint_list **rate_constraints);
+typedef int (*set_audio_input_t)(struct device *dev,
+				int port_index,
+				unsigned sample_rate,

include/sound/tda998x.h line 18

Quoted 2 lines

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

+				int sample_format);
+

Why don't you simply declare tda998x_get_audio_var() and tda998x_set_audio_input() here and export them in DRM side driver? This is a tda998x specific codec after all. I see no point to this this function pointer typedef game, when the pointers are anyway stored to global variables in ASoC the side module.

include/sound/tda998x.h line 21

Quoted 3 lines

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

+	snd_pcm_hw_constraint_minmax(runtime,
+				SNDRV_PCM_HW_PARAM_CHANNELS,
+				1, sad[SAD_MX_CHAN_I]);

SAD byte 1 bits 2-0 provides the maximum channel count _minus_one_. You should _add_one_ to sad[SAD_MX_CHAN_I] when setting the constraint.

Quoted 25 lines

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

+static struct snd_soc_dai_driver tda998x_dais[] = {
+	{
+		.name = "spdif-hifi",
+		.id = PORT_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 = &tda998x_codec_ops,
+	},
+	{
+		.name = "i2s-hifi",
+		.id = PORT_I2S,
+		.playback = {
+			.stream_name	= "HDMI I2S Playback",
+			.channels_min	= 1,
+			.channels_max	= 8,
+			.rates		= SNDRV_PCM_RATE_CONTINUOUS,
+			.rate_min	= 5512,
+			.rate_max	= 192000,

include/sound/tda998x.h

Quoted 5 lines

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

+			.formats	= TDA998X_FORMATS,
+		},
+		.ops = &tda998x_codec_ops,
+	},
+};

Why do you use SNDRV_PCM_RATE_CONTINUOUS, when HDMI standard only supports 32000, 44100, 48000, 88200, 96000, 176400, and 192000 Hz-rates?

Quoted 16 lines

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

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

include/sound/tda998x.h

Quoted 10 lines

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

+
+int tda9998x_codec_register(struct device *dev,
+			get_audio_var_t get_audio_var_i,
+			set_audio_input_t set_audio_input_i)
+{
+	get_audio_var = get_audio_var_i;
+	set_audio_input = set_audio_input_i;
+	return snd_soc_register_codec(dev,
+				&tda998x_codec_drv,
+				tda998x_dais, ARRAY_SIZE(tda998x_dais));

So this always registers a two DAI codec whether or not both the i2s and spdif is used. The DT binding document could state that the DAI index 0 is always the spdif DAI and index 1 is the i2s DAI, or even better you could #define them under include/dt-bindings/.

While you are at it, you could also mention the DAPM output name "hdmi-out" for the codec in the DT-binding document.

Based on 3.19.0-rc3.

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)int drm_edid_to_sad(struct edid *edid, struct cea_sad **sads);

v2: check double stream start (Mark Brown)

Jean-Francois Moine (4):
drm/i2c: tda998x: Add DT support for audio drm/i2c: tda998x: Change drvdata for audio extension
ASoC: tda998x: add a codec to the HDMI transmitter drm/i2c: tda998x: set cts_n according to the sample width

  .../devicetree/bindings/drm/i2c/tda998x.txt        |  18 ++
  drivers/gpu/drm/i2c/Kconfig                        |   1 +
  drivers/gpu/drm/i2c/tda998x_drv.c                  | 213 +++++++++++++++++++--
  include/sound/tda998x.h                            |  23 +++
  sound/soc/codecs/Kconfig                           |   4 +
  sound/soc/codecs/Makefile                          |   2 +
  sound/soc/codecs/tda998x.c                         | 175 +++++++++++++++++
  7 files changed, 424 insertions(+), 12 deletions(-)

create mode 100644 include/sound/tda998x.h
create mode 100644 sound/soc/codecs/tda998x.c

BTW, the HDMI codec is now completely unused, since my OMAP4+ HDMI audio patches were merged (it uses dummy codec) and my BBB HDMI audio patches were never merged.

Quoted 3 lines · Jyri Sarha

BTW, the HDMI codec is now completely unused, since my OMAP4+ HDMI audio patches were merged (it uses dummy codec) and my BBB HDMI audio patches were never merged.

Might those BBB ones turn up sometime?