diff mbox

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

Message ID 0084acea5a3475a77531d6a77483f36d3469111a.1420628786.git.moinejf@free.fr
State New, archived
Headers show

Commit Message

Jean-Francois Moine Jan. 7, 2015, 9:10 a.m. UTC
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(-)

Comments

Andrew Jackson Jan. 7, 2015, 2:39 p.m. UTC | #1
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;
> 

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Francois Moine Jan. 7, 2015, 5:08 p.m. UTC | #2
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?
Andrew Jackson Jan. 7, 2015, 5:18 p.m. UTC | #3
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mark Brown Jan. 7, 2015, 5:33 p.m. UTC | #4
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.
Jyri Sarha Jan. 8, 2015, 2:53 p.m. UTC | #5
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@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.
> +

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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Francois Moine Jan. 8, 2015, 4:42 p.m. UTC | #6
On Thu, 8 Jan 2015 16:53:41 +0200
Jyri Sarha <jsarha@ti.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.
> > +  
> 
> 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";
Mark Brown Jan. 8, 2015, 8:04 p.m. UTC | #7
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.
Andrew Jackson Jan. 9, 2015, 9:25 a.m. UTC | #8
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jyri Sarha Jan. 9, 2015, 10:13 a.m. UTC | #9
On 01/08/2015 06:42 PM, Jean-Francois Moine wrote:
> On Thu, 8 Jan 2015 16:53:41 +0200
> Jyri Sarha <jsarha@ti.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.
>>> +
>>
>> 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Jean-Francois Moine Jan. 9, 2015, 11:30 a.m. UTC | #10
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";
Russell King - ARM Linux Jan. 9, 2015, 11:45 a.m. UTC | #11
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.
Jean-Francois Moine Jan. 9, 2015, 12:54 p.m. UTC | #12
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.
Russell King - ARM Linux Jan. 9, 2015, 1:07 p.m. UTC | #13
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?
Andrew Jackson Jan. 9, 2015, 1:58 p.m. UTC | #14
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.

* 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 9, 2015, 2:57 p.m. UTC | #15
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.
Jean-Francois Moine Jan. 9, 2015, 5:38 p.m. UTC | #16
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>;
Russell King - ARM Linux Jan. 9, 2015, 8:01 p.m. UTC | #17
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.
Jean-Francois Moine Jan. 10, 2015, 3:47 p.m. UTC | #18
On Fri, 9 Jan 2015 20:01:27 +0000
Russell King - ARM Linux <linux@arm.linux.org.uk> 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.
Philipp Zabel Jan. 12, 2015, 9:25 a.m. UTC | #19
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 12, 2015, 12:25 p.m. UTC | #20
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.
Philipp Zabel Jan. 12, 2015, 1:59 p.m. UTC | #21
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 12, 2015, 2:04 p.m. UTC | #22
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.
Jean-Francois Moine Jan. 12, 2015, 5:13 p.m. UTC | #23
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.
Russell King - ARM Linux Jan. 12, 2015, 5:57 p.m. UTC | #24
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.
Jean-Francois Moine Jan. 12, 2015, 7:14 p.m. UTC | #25
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).
Philipp Zabel Jan. 13, 2015, 12:21 p.m. UTC | #26
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 13, 2015, 12:27 p.m. UTC | #27
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?
Jean-Francois Moine Jan. 13, 2015, 3:54 p.m. UTC | #28
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>;
		};
	};
Russell King - ARM Linux Jan. 13, 2015, 4:03 p.m. UTC | #29
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.
Jean-Francois Moine Jan. 13, 2015, 7:02 p.m. UTC | #30
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.
Russell King - ARM Linux Jan. 13, 2015, 7:26 p.m. UTC | #31
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.
Jyri Sarha Jan. 13, 2015, 7:41 p.m. UTC | #32
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.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 13, 2015, 7:54 p.m. UTC | #33
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.)
Jean-Francois Moine Jan. 14, 2015, 7:55 a.m. UTC | #34
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>;
                 };
	};
Philipp Zabel Jan. 14, 2015, 10:46 a.m. UTC | #35
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

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Russell King - ARM Linux Jan. 14, 2015, 12:12 p.m. UTC | #36
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?
Mark Brown Jan. 14, 2015, 12:50 p.m. UTC | #37
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.
Russell King - ARM Linux Jan. 14, 2015, 2:23 p.m. UTC | #38
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.
diff mbox

Patch

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;