[PATCH v9 0/4] ASoC: tda998x: add a codec to the HDMI transmitter

[PATCH v9 0/4] ASoC: tda998x: add a codec to the HDMI transmitter

[Jean-Francois Moine]

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

-- 
2.1.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

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

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/drm/i2c/tda998x.txt        | 18 +++++++
 drivers/gpu/drm/i2c/tda998x_drv.c                  | 60 ++++++++++++++++++----
 2 files changed, 69 insertions(+), 9 deletions(-)

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index e9e4bce..e50e7cd 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -17,6 +17,20 @@ Optional properties:
   - video-ports: 24 bits value which defines how the video controller
 	output is wired to the TDA998x input - default: <0x230145>
 
+  - 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.
+
+  - 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>;
 	};
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 70658af..9d9b072 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -20,6 +20,7 @@
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <sound/asoundef.h>
+#include <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;
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Andrew Jackson]

On 01/07/15 09:10, Jean-Francois Moine wrote:
> This patch permits the definition of the audio ports from the device tree.
> 
> Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
> ---
>  .../devicetree/bindings/drm/i2c/tda998x.txt        | 18 +++++++
>  drivers/gpu/drm/i2c/tda998x_drv.c                  | 60 ++++++++++++++++++----
>  2 files changed, 69 insertions(+), 9 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> index e9e4bce..e50e7cd 100644
> --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -17,6 +17,20 @@ Optional properties:
>    - video-ports: 24 bits value which defines how the video controller
>  	output is wired to the TDA998x input - default: <0x230145>
>  
> +  - 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.

	Andrew

> +
> +  - 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>;
>  	};
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 70658af..9d9b072 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -20,6 +20,7 @@
>  #include <linux/module.h>
>  #include <linux/irq.h>
>  #include <sound/asoundef.h>
> +#include <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;
> 

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Wed, 07 Jan 2015 14:39:13 +0000
Andrew Jackson <Andrew.Jackson@arm.com> wrote:

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

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?

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Andrew Jackson]

On 01/07/15 17:08, Jean-Francois Moine wrote:
> On Wed, 07 Jan 2015 14:39:13 +0000
> Andrew Jackson <Andrew.Jackson@arm.com> wrote:
> 
>>> +  - 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.
> 
> 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.".  

	Andrew

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Mark Brown]

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

On Wed, Jan 07, 2015 at 05:18:20PM +0000, Andrew Jackson wrote:

> 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 is commonly done by just saying that the value will be written into
a given bitfield for such and such a purpose and then relying on the
chip documentation for that; it's a more direct way of saying the above.

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jyri Sarha]

On 01/07/2015 11:10 AM, Jean-Francois Moine wrote:
> This patch permits the definition of the audio ports from the device tree.
>
> Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> ---
>   .../devicetree/bindings/drm/i2c/tda998x.txt        | 18 +++++++
>   drivers/gpu/drm/i2c/tda998x_drv.c                  | 60 ++++++++++++++++++----
>   2 files changed, 69 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> index e9e4bce..e50e7cd 100644
> --- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> +++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
> @@ -17,6 +17,20 @@ Optional properties:
>     - video-ports: 24 bits value which defines how the video controller
>   	output is wired to the TDA998x input - default: <0x230145>
>
> +  - 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.

> +  - 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>;
>   	};
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index 70658af..9d9b072 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -20,6 +20,7 @@
>   #include <linux/module.h>
>   #include <linux/irq.h>
>   #include <sound/asoundef.h>
> +#include <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;
>

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Thu, 8 Jan 2015 16:53:41 +0200
Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org> wrote:

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

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Mark Brown]

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

On Thu, Jan 08, 2015 at 05:42:57PM +0100, Jean-Francois Moine wrote:

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

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.

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Andrew Jackson]

On 01/08/15 20:04, Mark Brown wrote:
> On Thu, Jan 08, 2015 at 05:42:57PM +0100, Jean-Francois Moine wrote:
> 
>> 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";
> 
> 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.
> 

If it helps, I've collated the pin assignments given in the various TDA datasheets that I can find:

Chip>	9983B	9989		19988		19989	
Mode>	-	S/PDIF	I2S	S/PDIF	I2S	S/PDIF	I2S
Pin
AP0	WS	-	WS	-	WS	-	WS
AP1	I2S#0	S/PDIF	I2S#0	S/PDIF	I2S#0	S/PDIF	I2S#0
AP2	I2S#1	-	-	S/PDIF	I2S#1	S/PDIF	I2S#1
AP3	I2S#2	-	-	-	I2S#2*	MCLK	-
AP4	I2S#3	-	-	-	I2S#3*	-	-
AP5	MCLK	-	-	-	-	-	-
AP6	S/PDIF	-	-	-	-	-	-
AP7	AUX	-	-	-	-	-	-

WS = I2S Word Select
* Depends on package

The 9983B differs from the other devices in that the I2S and S/PDIF functionality is not multiplexed 
onto various pins.

	Andrew

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jyri Sarha]

On 01/08/2015 06:42 PM, Jean-Francois Moine wrote:
> On Thu, 8 Jan 2015 16:53:41 +0200
> Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org> wrote:
>
>>> +  - 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
> 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.

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

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.

Best regards,
Jyri
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Fri, 9 Jan 2015 12:13:04 +0200
Jyri Sarha <jsarha@ti.com> wrote:

> On 01/08/2015 06:42 PM, Jean-Francois Moine wrote:
> >  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.

There are 2 possible S/PDIF pins in the tda998x, and there may be many
audio chips with S/PDIF outputs.

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

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 12:30:36PM +0100, Jean-Francois Moine wrote:
> 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.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Fri, 9 Jan 2015 11:45:29 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Fri, Jan 09, 2015 at 12:30:36PM +0100, Jean-Francois Moine wrote:
> > 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.

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.

The relation between an audio device and the associated pin is done by
the definition of the sound card.

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. There is no mixing.
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.

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 01:54:01PM +0100, Jean-Francois Moine wrote:
> On Fri, 9 Jan 2015 11:45:29 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 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?

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

I don't think it's that simple.  Since there is only one WS input to
the 998x, the four I2S sources would need to synchronise somehow, and
since the I2S source generates WS, connecting the 998x to multiple
independent I2S sources, each with their own WS output, presents a
hardware problem.

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

That statement is not entirely accurate.  Yes, with S/PDIF, only one
PCM L+R channel can be sent.  However, S/PDIF also supports sending
more channels using "non-audio" streams, with the data encoded as
MPEG or AAC.  This uses the HDMI data islands which would've been
occupied by the Front L+R PCM channel.

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

That's incorrect.  Please read the HDMI 1.3 specification; channels are
allocated for Front L+R, Centre, Sub, Rear L+R and there's no
identification to indicate that there are two Front L+R channels which
are different languages.

If you feel differently, please provide a reference to the HDMI
specification which describes this feature.

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

The selection of the language is done at the player, not by the display
device.

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

That's correct: the TDA998x can operate in one of two modes: either
S/PDIF _or_ I2S, but never both.

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?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Andrew Jackson]

On 01/09/15 13:07, Russell King - ARM Linux wrote:
> On Fri, Jan 09, 2015 at 01:54:01PM +0100, Jean-Francois Moine wrote:
>> On Fri, 9 Jan 2015 11:45:29 +0000
>> Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
>>> 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]

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

* What the driver does need to take account of is the number of inputs
  that are active so that the sound CODEC can be properly configured.


	Andrew


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 01:58:37PM +0000, Andrew Jackson wrote:
> On 01/09/15 13:07, Russell King - ARM Linux wrote:
> > On Fri, Jan 09, 2015 at 01:54:01PM +0100, Jean-Francois Moine wrote:
> >> On Fri, 9 Jan 2015 11:45:29 +0000
> >> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> >>> 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]
> 
> > 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.

Yep.

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

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Fri, 9 Jan 2015 14:57:41 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

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

I had a look at the TDA19988 (small) datasheet I have and at the HDMI
specs, and, yes, you are right: multi (stereo) channels implies speaker
mapping, and, then, that the I2S streams come from a same media source.

While, as said in the TDA19988 datasheet,
"The I2S-bus input interface receives an I2S-bus signal including
 serial data, word select and serial clock",
it would be strange that these I2S buses come from different audio devices.

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

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

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

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 06:38:57PM +0100, Jean-Francois Moine wrote:
> On Fri, 9 Jan 2015 14:57:41 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > 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)
> ...

That's certainly a reasonable possibility, but we don't have a way to
confirm it as I don't think anyone has access to a setup which uses
I2S bus 1.

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

I'm willing to bet that when the audio is configured for layout 1, CA_I2S
affects the mapping of individual I2S channels (what I call "words") to
the HDMI channels.

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

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [alsa-devel] [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Fri, 9 Jan 2015 20:01:27 +0000
Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:

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

Very interesting idea!

That's a detail, but I don't fully agree your endpoint description:

- pin description
	AP1 is the first S/PDIF input - ok
	AP2 is the second S/PDIF input - ok
	then, the I2S input may go only to AP3 or AP4.

- source input nodes
	You declare the same external S/PDIF source on AP1 and AP2.
	I would have better seen a kirkwood-like audio device as one of
	the S/PDIF input, say the second one.

To complexify a bit, I also connect the spdif wire of the kirkwood-like
device to a S/PDIF optical output ;)

spdif_in -------------+
                      v
       + i2s -----> tda998x ---> HDMI
audio1 |              ^
       + spdif -------+--------> spdif_out


Here are the modified ports of
- the HDMI transmitter (I removed the #xxx_cells):

	port@0 { // AP 1
		tda998x_spdif0: endpoint@02 {
			reg = <0x02>;
			remote-endpoint = <&spdif_in_port>;
		};
	};
	port@1 { //AP 2
		tda998x_spdif1: endpoint@04 {
			reg = <0x04>;
			remote-endpoint = <&audio1_spdif0>;
		};
	};
	port@2 { //AP 3
		tda998x_i2s: endpoint@08 {
			reg = <0x09>;
			remote-endpoint = <&audio1_i2s>;
		};
	};

- the (internal) audio device (audio1):

	port@0 {
		audio1_spdif0: endpoint@0 {
			reg = 0;
			remote-endpoint = <&tda998x_spdif1>;
		};
		audio1_spdif1: endpoint@1 {
			reg = 0;
			remote-endpoint = <&spdif_out_port>;
		};
	};
	port@1 {
		audio1_i2s: endpoint@1 {
			reg = 1;
			remote-endpoint = <&tda998x_i2s>;
		};
	};

- and the S/PDIF external input (spdif_in):

	port {
		spdif_in_port: endpoint@0 {
			reg = 0;
			remote-endpoint = <&tda998x_spdif0>;
		};
	};

- and the S/PDIF external output (spdif_out):

	port {
		spdif_out_port: endpoint@0 {
			reg = 0;
			remote-endpoint = <&audio1_spdif1>;
		};
	};

All the hardware is described. There is nothing more to add in the DT.

Especially, there is no 'simple-card' which is pure software and rather
Linux specific.

And, now, from this DT description, ASoC expects a sound card to be
built.

It seems that this creation should be done in the same way the video
cards are built, i.e. from the source devices, i.e. the kirkwood-like
device, and also from the spdif_in (which could be some other internal
audio device outputting s/pdif instead of a simple S/PDIF input
connector)! Is there one or two cards, and if 2 cards, how do they
share the tda998x?

Well, I will have a look at how to get audio out of my machine with
these new DT definitions (hopefully, there is only one audio source!).

Mark, you may forget about my other patch adding multi-codecs in the
simple-card...

Thanks.

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Philipp Zabel]

Am Freitag, den 09.01.2015, 20:01 +0000 schrieb Russell King - ARM
Linux:
[...]
> 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.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Mon, Jan 12, 2015 at 10:25:28AM +0100, Philipp Zabel wrote:
> 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.

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 port@1..n
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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Philipp Zabel]

Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:
> On Mon, Jan 12, 2015 at 10:25:28AM +0100, Philipp Zabel wrote:
> > 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.
> 
> 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 port@1..n
> 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.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Mon, Jan 12, 2015 at 02:59:57PM +0100, Philipp Zabel wrote:
> Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:
> > 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 port@1..n
> > 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.

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Mon, 12 Jan 2015 14:04:56 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Mon, Jan 12, 2015 at 02:59:57PM +0100, Philipp Zabel wrote:
> > Am Montag, den 12.01.2015, 12:25 +0000 schrieb Russell King - ARM Linux:
> > > 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 port@1..n
> > > 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.

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

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

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Mon, Jan 12, 2015 at 06:13:41PM +0100, Jean-Francois Moine wrote:
> On Mon, 12 Jan 2015 14:04:56 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > On Mon, Jan 12, 2015 at 02:59:57PM +0100, Philipp Zabel wrote:
> > > 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.

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.

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

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Mon, 12 Jan 2015 17:57:06 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

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

OK.

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

Right, so, either the port type must be explicitly defined, or the name
of the property giving the port value also gives the port type
(nxp,video-port / nxp,audio-port).

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Philipp Zabel]

Am Montag, den 12.01.2015, 17:57 +0000 schrieb Russell King - ARM Linux:
> On Mon, Jan 12, 2015 at 06:13:41PM +0100, Jean-Francois Moine wrote:
> > On Mon, 12 Jan 2015 14:04:56 +0000
> > Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> > > On Mon, Jan 12, 2015 at 02:59:57PM +0100, Philipp Zabel wrote:
> > > > 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.
> 
> 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.

The (unit address part == first reg property value) is from the ePAPR
and still documented in http://www.devicetree.org/Device_Tree_Usage. It
isn't explicitly stated as a hard requirement, but it is worded in such
a way that I'd expect it to hold true most of the time :/

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

Please don't add the complexity of multiple 'ports' nodes to the OF
graph bindings. I'd rather have the driver determine the type of the
port. Ideally it could know that port 0 always is video and all other
ports are audio, otherwise checking the existence of a custom property
in the 'port' node should work, for example 'nxp,audio-port' or
'nxp,video-port'.
Why are those located in the 'endpoint' nodes in your example? Are you
expecting to dynamically reconfigure the port type of a given AP from
i2s to spdif depending on the activated endpoint?

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

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Tue, Jan 13, 2015 at 01:21:58PM +0100, Philipp Zabel wrote:
> 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?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Tue, 13 Jan 2015 12:27:15 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Jan 13, 2015 at 01:21:58PM +0100, Philipp Zabel wrote:
> > 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.

In an other way, the unit address (== first reg) does not need to be a
sequence number. It is the I/O base in most DTs.
So, it could be the port value: 

	port@230145 {
		port-type = "rgb";
		reg = <0x230145>;
		hdmi_0: endpoint {
			remote-endpoint = <&lcd0_0>;
		};
	};
	port@3 {			/* AP1 = I2S */
		port-type = "i2s";
		reg = <0x03>;
		tda998x_i2s: endpoint {
			remote-endpoint = <&audio1_i2s>;
		};
	};
	port@4 {			 /* AP2 = S/PDIF */
		port-type = "spdif";
		reg = <0x04>;
		tda998x_spdif: endpoint {
			remote-endpoint = <&audio1_spdif1>;
		};
	};

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Tue, Jan 13, 2015 at 04:54:11PM +0100, Jean-Francois Moine wrote:
> 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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Tue, 13 Jan 2015 16:03:13 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Jan 13, 2015 at 04:54:11PM +0100, Jean-Francois Moine wrote:
> > 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.

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Tue, Jan 13, 2015 at 08:02:52PM +0100, Jean-Francois Moine wrote:
> On Tue, 13 Jan 2015 16:03:13 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Tue, Jan 13, 2015 at 04:54:11PM +0100, Jean-Francois Moine wrote:
> > > 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.

Let me try to be clearer.  Here's four independent I2S blocks which
have been arranged to be clocked by the same I2S bus clock.  Each
I2S block produces its own word clock and data stream:

SCLK
|  +-------+
+->| I2S   +------> WS1
|  | src 1 +------> I2S1
|  +-------+
|
|  +-------+
+->| I2S   +------> WS2
|  | src 2 +------> I2S2
|  +-------+
|
|  +-------+
+->| I2S   +------> WS3
|  | src 3 +------> I2S3
|  +-------+
|
|  +-------+
+->| I2S   +------> WS4
|  | src 4 +------> I2S4
|  +-------+
|
|        +---------+
|        | TDA998x |
`------->+ ACLK    |
  WS --->+ AP0     |
I2S1 --->+ AP1     |
I2S2 --->+ AP2     |
I2S3 --->+ AP3     |
I2S4 --->+ AP4     |
         +---------+

The arrows point from the output (driver of the signal) to the input.
It is illegal in electronics to wire two outputs (which are not able to
disable their output drivers) together.

How do you arrange for WS1..WS4 to be connected to the TDA998x WS input?

My answer is: this is an impossible setup.

Since these are independent blocks, there is no guarantee that the
WS1..WS4 outputs will be synchronised to each other, since that depends
upon the SCLK edge that each unit becomes enabled.  That also determines
the bit position relative to SCLK of the first bit output.

Hence, you could very well end up with something like this for a 16-bit
I2S output from each I2S block:

SCLK: _~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_~_
 WS1: __~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~________________________________~
I2S1: llmm............................llmm............................llm
 WS2: ________________~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~___________________
I2S2: ..............llmm............................llmm.................
 WS3: ________~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~___________________________
I2S3: ......llmm............................llmm.........................
 WS4: ______~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~_____________________________
I2S4: ....llmm............................llmm...........................

where 'l' indicates the LSB of the sample, and 'm' indicates the MSB.

The problem is that the TDA998x expects each I2S input to be synchronised.
In other words, the WS input is shared between each and every separate I2S
input, which means the MSB of each I2S input needs to be supplied at
exactly the same SCLK edge.  In other words:

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.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jyri Sarha]

On 01/13/2015 09:26 PM, Russell King - ARM Linux wrote:
> 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.

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Tue, Jan 13, 2015 at 09:41:01PM +0200, Jyri Sarha wrote:
> On 01/13/2015 09:26 PM, Russell King - ARM Linux wrote:
> >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.

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

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Jean-Francois Moine]

On Tue, 13 Jan 2015 19:54:15 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Tue, Jan 13, 2015 at 09:41:01PM +0200, Jyri Sarha wrote:
> > On 01/13/2015 09:26 PM, Russell King - ARM Linux wrote:
> > >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.
> 
> 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.)

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

 	#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.
Then, it would have been simpler to define the DAIs from the ports:

	#address-cells = <1>;
 	#size-cells = <0>;
 	...
         port@3 {                        /* AP1 = I2S */
                 port-type = "i2s";
                 reg = <0x03>;		/* WS + AP1 */
                 tda998x_i2s1: endpoint {
                         remote-endpoint = <&audio1_i2s>;
                 };
         };
         port@5 {                        /* AP2 = I2S */
                 port-type = "i2s";
                 reg = <0x05>;		/* WS + AP2 */
                 tda998x_i2s1: endpoint {
                         remote-endpoint = <&audio1_i2s>;
                 };
	};

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

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Wed, Jan 14, 2015 at 08:55:01AM +0100, Jean-Francois Moine wrote:
> On Tue, 13 Jan 2015 19:54:15 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
> 
> > On Tue, Jan 13, 2015 at 09:41:01PM +0200, Jyri Sarha wrote:
> > > On 01/13/2015 09:26 PM, Russell King - ARM Linux wrote:
> > > >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.
> > 
> > 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.)
> 
> #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).

That may be, but the documentation specifies that it should be given.
See Documentation/devicetree/bindings/graph.txt - maybe the docs need
fixing?

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

I think that's a matter of how the problem is viewed. :)

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

I view a DAI as a Linux abstraction, which doesn't really have any place
in DT.  I'm looking at this problem from the electronics point of view.

> 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:
> 
>  	#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.

How does that work - I mean, if we have i2s1 as the SCLK and WS master,
how can i2s2 run without i2s1?

I suppose I2S2 could be reprogrammed to be the master for both signals,
provided that I2S1 is programmed to be a slave before hand, but that
seems to be making things excessively complex (we'd need some way for
I2S1 and I2S2 to comunicate that state between themselves and negotiate
which to be the master.)

However, I'd suggest that if audio1_i2s always maps to HDMI front
left/right, audio1_i2s must always be enabled when audio playback is
required; there is no CA bit combination provided in HDMI where the
front L/R channels are optional.  Hence, I'd suggest that audio1_i2s
must always be the master.

As we don't know, I'd suggest that we permit flexibility here.  We
can use the reg property as you have below, and we just bitwise-or
the of_endpoint port/id together, along with that result from other
active audio endpoints.  The advantage of that is we can use any of
these representations, and it should result in a working setup - and
when we do have the necessary information to make a properly informed
decision, we can update the DT and binding doc accordingly and we
won't have to update the driver (nor will we break backwards compat.)

Sound sane?

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Philipp Zabel]

Hi Russell,

thanks for the clarification. 

Am Dienstag, den 13.01.2015, 19:54 +0000 schrieb Russell King - ARM
Linux:
[...]
> 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.

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1123 bytes --]

On Wed, Jan 14, 2015 at 11:46:58AM +0100, Philipp Zabel wrote:

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

Yes, this is what we're doing for the Samsung CPU side controllers which
implement this natively and seems to make sense here too - the different
channels can't really work as separate interfaces in any practical way.

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

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH v9 1/4] drm/i2c: tda998x: Add DT support for audio

[Russell King - ARM Linux]

On Wed, Jan 14, 2015 at 12:50:56PM +0000, Mark Brown wrote:
> 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.

Trying to get the synchronisation correct for proper surround audio
reproduction would also be interesting.  I suspect that userspace
would have to be taught specifically about a hardware setup like this -
in order to ensure that the slave I2S blocks are ready before the master
starts outputting the I2S clocks.

I'm /not/ that thrilled with the whole idea; I'd much rather suggest
that we should concentrate on sane setups, but we know that hardware
engineers do create some silly setups "because they can".

> 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 seems to favour the "one DT port for I2S" model - possibly with
multiple endpoints for each I2S channel (if desired).  There's nothing
in that model which would prevent having one end point for all four
channels.  To put that another way, we can remain flexible.

Maybe we should document the binding indicating that for I2S, one port
and one endpoint connected to one I2S block which supplies all I2S
channels is the preferred model, but others are permitted but not
necessarily guaranteed to work correctly.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v9 2/4] drm/i2c: tda998x: Change drvdata for audio extension

[Jean-Francois Moine]

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

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

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index 9d9b072..ce1478d 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -1437,6 +1437,8 @@ static int tda998x_encoder_init(struct i2c_client *client,
 	encoder_slave->slave_priv = priv;
 	encoder_slave->slave_funcs = &tda998x_encoder_slave_funcs;
 
+	dev_set_drvdata(&client->dev, priv);
+
 	return 0;
 }
 
@@ -1563,7 +1565,7 @@ static int tda998x_bind(struct device *dev, struct device *master, void *data)
 	if (!priv)
 		return -ENOMEM;
 
-	dev_set_drvdata(dev, priv);
+	dev_set_drvdata(dev, &priv->base);
 
 	priv->base.encoder = &priv->encoder;
 	priv->connector.interlace_allowed = 1;
@@ -1613,7 +1615,9 @@ err_encoder:
 static void tda998x_unbind(struct device *dev, struct device *master,
 			   void *data)
 {
-	struct tda998x_priv2 *priv = dev_get_drvdata(dev);
+	struct tda998x_priv *priv_s = dev_get_drvdata(dev);
+	struct tda998x_priv2 *priv =
+			container_of(priv_s, struct tda998x_priv2, base);
 
 	drm_connector_cleanup(&priv->connector);
 	drm_encoder_cleanup(&priv->encoder);
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

[Jean-Francois Moine]

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 <moinejf@free.fr>
---
 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

diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
index 22c7ed6..24fc975 100644
--- a/drivers/gpu/drm/i2c/Kconfig
+++ b/drivers/gpu/drm/i2c/Kconfig
@@ -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.
 
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index ce1478d..a26a516 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -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:
diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
new file mode 100644
index 0000000..97cff30
--- /dev/null
+++ b/include/sound/tda998x.h
@@ -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
diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index 8349f98..456c549 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -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
diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
index bbdfd1e..44b4fda 100644
--- a/sound/soc/codecs/Makefile
+++ b/sound/soc/codecs/Makefile
@@ -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
diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
new file mode 100644
index 0000000..b055570
--- /dev/null
+++ b/sound/soc/codecs/tda998x.c
@@ -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)
+		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 <moinejf@free.fr>");
+MODULE_DESCRIPTION("TDA998X CODEC");
+MODULE_LICENSE("GPL");
-- 
2.1.4

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

[Andrew Jackson]

On 01/07/15 10:51, Jean-Francois Moine wrote:
> 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 <moinejf@free.fr>
> ---
>  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
> 
> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> index 22c7ed6..24fc975 100644
> --- a/drivers/gpu/drm/i2c/Kconfig
> +++ b/drivers/gpu/drm/i2c/Kconfig
> @@ -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?

>         help
>           Support for NXP Semiconductors TDA998X HDMI encoders.
> 
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index ce1478d..a26a516 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -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:
> diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
> new file mode 100644
> index 0000000..97cff30
> --- /dev/null
> +++ b/include/sound/tda998x.h
> @@ -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
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 8349f98..456c549 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -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
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index bbdfd1e..44b4fda 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -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
> diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
> new file mode 100644
> index 0000000..b055570
> --- /dev/null
> +++ b/sound/soc/codecs/tda998x.c
> @@ -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

Wouldn't these defines be better in sound/tda998x.h, with an appropriate prefix, then
you could use them in gpu/drm/i2c/tda998x_drv.c.

	Andrew

> +       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 <moinejf@free.fr>");
> +MODULE_DESCRIPTION("TDA998X CODEC");
> +MODULE_LICENSE("GPL");
> --
> 2.1.4
> 
> 

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

[Russell King - ARM Linux]

On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote:
> On 01/07/15 10:51, Jean-Francois Moine wrote:
> > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> > index 22c7ed6..24fc975 100644
> > --- a/drivers/gpu/drm/i2c/Kconfig
> > +++ b/drivers/gpu/drm/i2c/Kconfig
> > @@ -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.

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

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.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

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

[Jean-Francois Moine]

On Wed, 7 Jan 2015 15:41:38 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

> On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote:
> > On 01/07/15 10:51, Jean-Francois Moine wrote:
> > > diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> > > index 22c7ed6..24fc975 100644
> > > --- a/drivers/gpu/drm/i2c/Kconfig
> > > +++ b/drivers/gpu/drm/i2c/Kconfig
> > > @@ -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.

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

	select SND_SOC_TDA998X if SND_SOC

should work in all cases.

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

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

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

[Jyri Sarha]

On 01/07/2015 08:02 PM, Jean-Francois Moine wrote:
> On Wed, 7 Jan 2015 15:41:38 +0000
> Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:
>
>> On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote:
>>> On 01/07/15 10:51, Jean-Francois Moine wrote:
>>>> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
>>>> index 22c7ed6..24fc975 100644
>>>> --- a/drivers/gpu/drm/i2c/Kconfig
>>>> +++ b/drivers/gpu/drm/i2c/Kconfig
>>>> @@ -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.
>
>> 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.
>
> 	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)

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

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

[Jean-Francois Moine]

On Fri, 9 Jan 2015 12:24:12 +0200
Jyri Sarha <jsarha@ti.com> wrote:

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

request_module() is not needed because there are functions calls from
the transmitter to the codec.

With the simple "if SND_SOC", make menuconfig refuses to have TDA998x
built in and SND_SOC built as modules, as it should.

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

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

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 12:24:12PM +0200, Jyri Sarha wrote:
> 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.

That doesn't fix the problem.  If the DRM driver is built in, but the
codec is not, and the DRM driver has a reference to
tda9998x_codec_register(), then the vmlinux file will fail to link,
and you'll never get the opportunity to call request_module().

> Or could could write:
> 
> select SND_SOC_TDA998X if (SND_SOC=DRM_I2C_NXP_TDA998X || SND_SOC=y)

I'm not sure that's right either.

Let's go back and think about this: why should SND_SOC_TDA998X be
*selected*.  Let me put that a different way: why should this symbol
be forced on just because we're building the DRM TDA998x driver?

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.

IMHO "select" is a very over-used, and in many cases an evil
construct because its very hard to avoid breaking dependencies
with it.

-- 
FTTC broadband for 0.8mile line: currently at 9.5Mbps down 400kbps up
according to speedtest.net.

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

[Jean-Francois Moine]

On Fri, 9 Jan 2015 11:19:35 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> wrote:

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

> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index b33b45d..747e387 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -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
> +

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

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

[Russell King - ARM Linux]

On Fri, Jan 09, 2015 at 12:45:31PM +0100, Jean-Francois Moine wrote:
> On Fri, 9 Jan 2015 11:19:35 +0000
> Russell King - ARM Linux <linux-lFZ/pmaqli7XmaaqVzeoHQ@public.gmane.org> wrote:
> 
> > 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:
> 
> > diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> > index b33b45d..747e387 100644
> > --- a/sound/soc/codecs/Kconfig
> > +++ b/sound/soc/codecs/Kconfig
> > @@ -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
> > +

An easier way to write this is:

	config SND_SOC_TDA998X
		def_tristate y
		depends on DRM_I2C_NXP_TDA998X

That's because internally, Kconfig knows how to do ternary operations.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 354 bytes --]

On Wed, Jan 07, 2015 at 03:10:47PM +0000, Andrew Jackson wrote:
> On 01/07/15 10:51, Jean-Francois Moine wrote:
> > This patch adds a CODEC to the NXP TDA998x HDMI transmitter.
> > 
> > The CODEC handles both I2S and S/PDIF inputs.

Please delete unneeded context from your messages, it makes it much
easier to find any new content you've added.

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

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

[Jyri Sarha]

On 01/07/2015 12:51 PM, Jean-Francois Moine wrote:
> 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 <moinejf@free.fr>
> ---
>   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
>
> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> index 22c7ed6..24fc975 100644
> --- a/drivers/gpu/drm/i2c/Kconfig
> +++ b/drivers/gpu/drm/i2c/Kconfig
> @@ -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.
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index ce1478d..a26a516 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -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

Why not simply #if IS_ENABLED(CONFIG_SND_SOC_TDA998X), no need for 
WITH_CODEC at all IMO.

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

Why not use struct cea_sad and friends from drm/drm_edid.h ?

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

See comment above about struct cea_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)
> +{

See comment above about struct cea_sad.

> +	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:
> diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
> new file mode 100644
> index 0000000..97cff30
> --- /dev/null
> +++ b/include/sound/tda998x.h
> @@ -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,

See comment above about struct cea_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
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 8349f98..456c549 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -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
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index bbdfd1e..44b4fda 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -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
> diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
> new file mode 100644
> index 0000000..b055570
> --- /dev/null
> +++ b/sound/soc/codecs/tda998x.c
> @@ -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

See comment above about struct cea_sad.

> +	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 <moinejf@free.fr>");
> +MODULE_DESCRIPTION("TDA998X CODEC");
> +MODULE_LICENSE("GPL");
>

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

[Andrew Jackson]

On 01/07/15 10:51, Jean-Francois Moine wrote:
> 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 <moinejf@free.fr>
> ---

[snip]

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

[...]

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

	Andrew

[1] http://mailman.alsa-project.org/pipermail/alsa-devel/2015-January/086090.html

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

[Mark Brown]

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

On Fri, Jan 09, 2015 at 05:39:08PM +0000, Andrew Jackson wrote:

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

Yes, that's a good idea in general - we should do more of this.

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

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

[Jean-Francois Moine]

On Fri, 09 Jan 2015 17:39:08 +0000
Andrew Jackson <Andrew.Jackson@arm.com> wrote:

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

snd_pcm_hw_constraint_minmax() does the job from the min/max numbers of
channels in the DAI definition and the real max number of channels will
be set from audio ports declared in the DT.

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

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

[Jyri Sarha]

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

On 01/07/2015 12:51 PM, Jean-Francois Moine wrote:
> 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 <moinejf@free.fr>
> ---
>   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
>
> diff --git a/drivers/gpu/drm/i2c/Kconfig b/drivers/gpu/drm/i2c/Kconfig
> index 22c7ed6..24fc975 100644
> --- a/drivers/gpu/drm/i2c/Kconfig
> +++ b/drivers/gpu/drm/i2c/Kconfig
> @@ -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.
>
> diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
> index ce1478d..a26a516 100644
> --- a/drivers/gpu/drm/i2c/tda998x_drv.c
> +++ b/drivers/gpu/drm/i2c/tda998x_drv.c
> @@ -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

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?

>   };
>
>   #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:
> diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
> new file mode 100644
> index 0000000..97cff30
> --- /dev/null
> +++ b/include/sound/tda998x.h
> @@ -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);
> +

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.

> +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
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index 8349f98..456c549 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -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
> diff --git a/sound/soc/codecs/Makefile b/sound/soc/codecs/Makefile
> index bbdfd1e..44b4fda 100644
> --- a/sound/soc/codecs/Makefile
> +++ b/sound/soc/codecs/Makefile
> @@ -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
> diff --git a/sound/soc/codecs/tda998x.c b/sound/soc/codecs/tda998x.c
> new file mode 100644
> index 0000000..b055570
> --- /dev/null
> +++ b/sound/soc/codecs/tda998x.c
> @@ -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)
> +		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]);

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.

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

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

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

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.

> +}
> +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 <moinejf@free.fr>");
> +MODULE_DESCRIPTION("TDA998X CODEC");
> +MODULE_LICENSE("GPL");
>

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

[Jean-Francois Moine]

On Sun, 11 Jan 2015 23:03:14 +0200
Jyri Sarha <jsarha@ti.com> wrote:

> Some more comments based on my - finally successful - attempt to use 
> this code with BBB HDMI audio.
> 
> On 01/07/2015 12:51 PM, Jean-Francois Moine wrote:
> > This patch adds a CODEC to the NXP TDA998x HDMI transmitter.
	[snip]
> > @@ -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?

Good idea. The rate_constraints would be allocated by the codec and set by a transmitter function.

	[snip]
> > diff --git a/include/sound/tda998x.h b/include/sound/tda998x.h
> > new file mode 100644
> > index 0000000..97cff30
> > --- /dev/null
> > +++ b/include/sound/tda998x.h
> > @@ -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);
> > +
> 
> 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.

There cannot be both ways of function call with modules:
the transmitter may call exported functions of the codec,
but the codec cannot call exported functions of the transmitter.

	[snip]
> > +	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.

Right. Thanks.

	[snip]
> > +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,
> > +	},
> > +};
> 
> Why do you use SNDRV_PCM_RATE_CONTINUOUS, when HDMI standard only 
> supports 32000, 44100, 48000, 88200, 96000, 176400, and 192000 Hz-rates?

Before I treated the EDID, I directly used these definitions,
and my TV display accepted many more rates as 22050 with S/PDIF input
or 7875 with I2S input.

> > +
> > +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));
> 
> 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.
	[snip]

This will be changed when using the graph of ports: the DAIs will be
dynamically created from the DT.

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

[PATCH v9 4/4] drm/i2c: tda998x: set cts_n according to the sample width

[Jean-Francois Moine]

With I2S input, the CTS_N predivider depends on the sample width.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index a26a516..8af84cd 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -50,6 +50,7 @@ struct tda998x_priv {
 	wait_queue_head_t wq_edid;
 	volatile int wq_edid_wait;
 	struct drm_encoder *encoder;
 
+	int audio_sample_format;
 	u8 audio_ports[2];
 #ifdef WITH_CODEC
@@ -671,7 +672,17 @@ tda998x_configure_audio(struct tda998x_priv *priv,
 		reg_write(priv, REG_MUX_AP, MUX_AP_SELECT_I2S);
 		clksel_aip = AIP_CLKSEL_AIP_I2S;
 		clksel_fs = AIP_CLKSEL_FS_ACLK;
-		cts_n = CTS_N_M(3) | CTS_N_K(3);
+
+		/* with I2S input, the CTS_N predivider depends on
+		 * the sample width */
+		switch (priv->audio_sample_format) {
+		case SNDRV_PCM_FORMAT_S16_LE:
+			cts_n = CTS_N_M(3) | CTS_N_K(1);
+			break;
+		default:
+			cts_n = CTS_N_M(3) | CTS_N_K(3);
+			break;
+		}
 		break;
 
 	default:
@@ -778,7 +789,8 @@ static int tda998x_set_audio_input(struct device *dev,
 
 	/* if same audio parameters, just enable the audio port */
 	if (p->audio_cfg == priv->audio_ports[port_index] &&
-	    p->audio_sample_rate == sample_rate) {
+	    p->audio_sample_rate == sample_rate &&
+	    priv->audio_sample_format == sample_format) {
 		reg_write(priv, REG_ENA_AP, p->audio_cfg);
 		return 0;
 	}
@@ -787,6 +799,7 @@ static int tda998x_set_audio_input(struct device *dev,
 	p->audio_clk_cfg = port_index == PORT_SPDIF ? 0 : 1;
 	p->audio_cfg = priv->audio_ports[port_index];
 	p->audio_sample_rate = sample_rate;
+	priv->audio_sample_format = sample_format;
 	tda998x_configure_audio(priv, &priv->encoder->crtc->hwmode, p);
 	return 0;
 }
@@ -1388,6 +1401,7 @@ static int tda998x_create(struct i2c_client *client, struct tda998x_priv *priv)
 
 	priv->params.audio_frame[1] = 1;		/* channels - 1 */
 	priv->params.audio_sample_rate = 48000;		/* 48kHz */
+	priv->audio_sample_format = SNDRV_PCM_FORMAT_S24_LE;
 
 	priv->current_page = 0xff;
 	priv->hdmi = client;
-- 
2.1.4

Re: [PATCH v9 0/4] ASoC: tda998x: add a codec to the HDMI transmitter

[Jyri Sarha]

On 01/07/2015 01:06 PM, Jean-Francois Moine wrote:
> Based on 3.19.0-rc3.
>
> v9:
> 	- back to a TDA998x specific CODEC

I did not mean that you could not make your shot at generic codec. It 
will just need more work before it is generic enough for other HDMI 
encoders to fully utilize it.

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.

But then again if you'll rather make a tda998x specific codec for now, I 
am fine with that. Let's see later when(/if) I get my generic HDMI codec 
ready - I'll eventually need something for SiI9022 - if it makes sense 
to start using it with tda998x too.

Best regards,
Jyri

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

Re: [PATCH v9 0/4] ASoC: tda998x: add a codec to the HDMI transmitter

[Mark Brown]

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

On Thu, Jan 08, 2015 at 04:53:04PM +0200, Jyri Sarha wrote:

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

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

Re: [PATCH v9 0/4] ASoC: tda998x: add a codec to the HDMI transmitter

[Jyri Sarha]

On 01/08/2015 10:05 PM, Mark Brown wrote:
> On Thu, Jan 08, 2015 at 04:53:04PM +0200, Jyri Sarha wrote:
>
>> 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?
>

My intention is to take Jean-Francoises patches into use (I am currently 
working on it). I abandon my generic hdmi codec for now.
In any case I should not need the dummy HDMI codec for anything.

Best regards,
Jyri