[PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jean-Francois Moine]

There may be many couples of CPU/CODEC DAI links.
The example 2 is extracted from the Cubox DT.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 .../devicetree/bindings/sound/simple-card.txt      | 34 +++++++++++++++++++++-
 1 file changed, 33 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index b30c222..a872e7b 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -26,6 +26,9 @@ Required subnodes:
 - simple-audio-card,cpu			: CPU   sub-node
 - simple-audio-card,codec		: CODEC sub-node
 
+  There may be one or many couples (simple-audio-card,cpu, simple-audio-card,codec)
+  (see example 2).
+
 Required CPU/CODEC subnodes properties:
 
 - sound-dai				: phandle and port of CPU/CODEC
@@ -43,7 +46,7 @@ Optional CPU/CODEC subnodes properties:
 					  clock node (= common clock), or "system-clock-frequency"
 					  (if system doens't support common clock)
 
-Example:
+Example 1:
 
 sound {
 	compatible = "simple-audio-card";
@@ -88,3 +91,32 @@ sh_fsi2: sh_fsi2@ec230000 {
 	interrupt-parent = <&gic>;
 	interrupts = <0 146 0x4>;
 };
+
+Example 2:
+
+sound {
+	compatible = "simple-audio-card";
+	simple-audio-card,name = "Cubox Audio";
+
+	simple-audio-card,cpu@0 {		/* I2S - HDMI */
+		sound-dai = <&audio1 0>;
+		format = "i2s";
+	};
+	simple-audio-card,codec@0 {
+		sound-dai = <&tda998x 0>;
+	};
+
+	simple-audio-card,cpu@1 {		/* S/PDIF - HDMI */
+		sound-dai = <&audio1 1>;
+	};
+	simple-audio-card,codec@1 {
+		sound-dai = <&tda998x 1>;
+	};
+
+	simple-audio-card,cpu@2 {		/* S/PDIF - S/PDIF */
+		sound-dai = <&audio1 1>;
+	};
+	simple-audio-card,codec@2 {
+		sound-dai = <&spdif_codec>;
+	};
+};
-- 
1.9.0

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 2014-03-15 13:30, Jean-Francois Moine wrote:
> There may be many couples of CPU/CODEC DAI links.
> The example 2 is extracted from the Cubox DT.
> 
> Signed-off-by: Jean-Francois Moine <moinejf-GANU6spQydw@public.gmane.org>
> ---
[...]

This binding forces all the dai links to share the same card level 
properties. I find it problematic in these cases:

>> - simple-audio-card,format		: CPU/CODEC common audio format.
>> 					  "i2s", "right_j", "left_j" , "dsp_a"
>> 					  "dsp_b", "ac97", "pdm", "msb", "lsb"

The code allows having the format parameter in sub nodes too, but the 
document does no mention it. Adding a mention would solve this problem 
at least partly.

Neither does the document mention that 
"simple-audio-card,bitclock-inversion" and 
"simple-audio-card,frame-inversion" are also allowed in card level. 
Currently the code uses simple bit-wise-or from card-level and dai-level 
daifmt parameters, which may lead to broken daifmt setting. However, 
this on directly related to this patch.

>> - dai-tdm-slot-num			: Please refer to tdm-slot.txt.
>> - dai-tdm-slot-width			: Please refer to tdm-slot.txt.

These properties are actually only taken from sub-nodes so the document 
is broken but the code is ok.

In general this binding would look better if another sub-node level 
would added for dai-link related properties, including the cpu and codec 
sub-nodes, but that would break the backwards compatibility.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Sat, Mar 15, 2014 at 12:30:05PM +0100, Jean-Francois Moine wrote:
> There may be many couples of CPU/CODEC DAI links.
> The example 2 is extracted from the Cubox DT.

Oh, here's some documentation - please include the documentation
before the code, without the documentation the reader is going to have
no idea what the code is supposed to be implementing.

> +  There may be one or many couples (simple-audio-card,cpu, simple-audio-card,codec)
> +  (see example 2).

This doesn't mention how they're matched up.

> +sound {
> +	compatible = "simple-audio-card";
> +	simple-audio-card,name = "Cubox Audio";
> +
> +	simple-audio-card,cpu@0 {		/* I2S - HDMI */
> +		sound-dai = <&audio1 0>;
> +		format = "i2s";
> +	};
> +	simple-audio-card,codec@0 {
> +		sound-dai = <&tda998x 0>;
> +	};
> +
> +	simple-audio-card,cpu@1 {		/* S/PDIF - HDMI */
> +		sound-dai = <&audio1 1>;
> +	};
> +	simple-audio-card,codec@1 {
> +		sound-dai = <&tda998x 1>;
> +	};

So, this is not exactly pretty as a binding.  I would expect to see the
links explicitly represented in the DT so you see the two DAIs in each
link grouped into a container, the above isn't very easy to read and as
Jyri says this lack of clarity also causes practical problems in that
there's nowhere to place link specific parameters.

I think what I'd expect to see here is that the simple card can either
be a container with a link in it directly or a container of links.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jean-Francois Moine]

On Mon, 17 Mar 2014 16:43:39 +0000
Mark Brown <broonie@kernel.org> wrote:

> On Sat, Mar 15, 2014 at 12:30:05PM +0100, Jean-Francois Moine wrote:
	[snip]
> > +sound {
> > +	compatible = "simple-audio-card";
> > +	simple-audio-card,name = "Cubox Audio";
> > +
> > +	simple-audio-card,cpu@0 {		/* I2S - HDMI */
> > +		sound-dai = <&audio1 0>;
> > +		format = "i2s";
> > +	};
> > +	simple-audio-card,codec@0 {
> > +		sound-dai = <&tda998x 0>;
> > +	};
> > +
> > +	simple-audio-card,cpu@1 {		/* S/PDIF - HDMI */
> > +		sound-dai = <&audio1 1>;
> > +	};
> > +	simple-audio-card,codec@1 {
> > +		sound-dai = <&tda998x 1>;
> > +	};
> 
> So, this is not exactly pretty as a binding.  I would expect to see the
> links explicitly represented in the DT so you see the two DAIs in each
> link grouped into a container, the above isn't very easy to read and as
> Jyri says this lack of clarity also causes practical problems in that
> there's nowhere to place link specific parameters.
> 
> I think what I'd expect to see here is that the simple card can either
> be a container with a link in it directly or a container of links.

I agree. I see two possible syntaxes:

1) keep the same definitions in the containers:

sound {
	compatible = "simple-audio-card";
	simple-audio-card,name = "Cubox Audio";

	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
		simple-audio-card,cpu {
			sound-dai = <&audio1 0>;
			format = "i2s";
		};
		simple-audio-card,codec {
			sound-dai = <&tda998x 0>;
		};
	};

	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
		simple-audio-card,cpu {
			sound-dai = <&audio1 1>;
		};
		simple-audio-card,codec {
			sound-dai = <&tda998x 1>;
		}
	};
	...

2) new definitions in the container

sound {
	compatible = "simple-audio-card";
	simple-audio-card,name = "Cubox Audio";

	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
		format = "i2s";
		cpu-dai  = <&audio1 0>;
		codec-dai = <&tda998x 0>;
	};

	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
		cpu-dai = <&audio1 1>;
		codec-dai = <&tda998x 1>;
	};
	...

The 2nd syntax is simpler and clearer, but the properties of the CPU
DAI and of the CODEC DAI are the same in the container (format, clock
and PCM slots). Is this a problem?

BTW, there is a 'dai_fmt' in the DAI link, but this format is not used
in the simple-card. Why?

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Tue, Mar 18, 2014 at 09:17:28AM +0100, Jean-Francois Moine wrote:

> The 2nd syntax is simpler and clearer, but the properties of the CPU
> DAI and of the CODEC DAI are the same in the container (format, clock
> and PCM slots). Is this a problem?

Yes, things like clocks might be different.  We need a way to specify
DAI specific properties.

> BTW, there is a 'dai_fmt' in the DAI link, but this format is not used
> in the simple-card. Why?

Could you be more specific please?  If you mean the dai_fmt feature of
the generic dai_link struct then because of the confusion about what
the DAI formats mean which you'll have seen on the list.

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

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/18/2014 10:17 AM, Jean-Francois Moine wrote:
> On Mon, 17 Mar 2014 16:43:39 +0000
> Mark Brown <broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org> wrote:
>
>> On Sat, Mar 15, 2014 at 12:30:05PM +0100, Jean-Francois Moine wrote:
> 	[snip]
...

>
> I agree. I see two possible syntaxes:
>
> 1) keep the same definitions in the containers:
>
> sound {
> 	compatible = "simple-audio-card";
> 	simple-audio-card,name = "Cubox Audio";
>
> 	simple-audio-card,dai-link@0 {		/* I2S - HDMI */
> 		simple-audio-card,cpu {
> 			sound-dai = <&audio1 0>;
> 			format = "i2s";
> 		};
> 		simple-audio-card,codec {
> 			sound-dai = <&tda998x 0>;
> 		};
> 	};
>
> 	simple-audio-card,dai-link@1 {		/* S/PDIF - HDMI */
> 		simple-audio-card,cpu {
> 			sound-dai = <&audio1 1>;
> 		};
> 		simple-audio-card,codec {
> 			sound-dai = <&tda998x 1>;
> 		}
> 	};
> 	...
>

I vote for the version above. As Mark said there is need for dai 
specific properties.

While we are at it we could update the bitclock-master and frame-master 
syntax to be like this:

bitclock-master = "cpu"
frame-master = "codec"

With the above explicit definition all the daifmt settings could be 
defined in link level. For backwards compatibility we could still define 
that omitting the value equals "codec" and omitting the property equals 
"cpu".

It may sometimes be helpful to allow overwriting link level settings in 
dai level. In order to do that it should be possible to write all daifmt 
settings explicitly like this:

bitclock-inversion = <0>; /* <0> = no bitclock-inversion */

If backward compatibility is necessary we could recognize the syntax 
version from the existence dai-link node.

sound {
	compatible = "simple-audio-card";
	simple-audio-card,name = "Simple Audio";
	simple-audio-card,widgets = ...
	simple-audio-card,routing = ...

	simple-audio-card,dai-link@0 {		/* I2S - codec */
		format = "i2s";
		bitclock-master = "codec";
		frame-master = "codec";
		bitclock-inversion = <1>;
		simple-audio-card,cpu {
			sound-dai = <&audio1 0>;
			bitclock-inversion = <0>;
		};
		simple-audio-card,codec {
			sound-dai = <&codec 0>;
			system-clock-frequency = <12000000>;
		};
	};
...

I can participate in the implementation too.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Wed, Mar 19, 2014 at 12:08:55PM +0200, Jyri Sarha wrote:

> While we are at it we could update the bitclock-master and frame-master
> syntax to be like this:

> bitclock-master = "cpu"
> frame-master = "codec"

> With the above explicit definition all the daifmt settings could be defined
> in link level. For backwards compatibility we could still define that
> omitting the value equals "codec" and omitting the property equals "cpu".

It seems it'd be a bit more idiomatic to do that with a phandle rather
than with a string in order to allow extensions for things like TDM (the
I2S to mono speaker driver use case for example).

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jean-Francois Moine]

On Wed, 19 Mar 2014 12:08:55 +0200
Jyri Sarha <jsarha-l0cyMroinI0@public.gmane.org> wrote:

> sound {
> 	compatible = "simple-audio-card";
> 	simple-audio-card,name = "Simple Audio";
> 	simple-audio-card,widgets = ...
> 	simple-audio-card,routing = ...
> 
> 	simple-audio-card,dai-link@0 {		/* I2S - codec */
> 		format = "i2s";
> 		bitclock-master = "codec";
> 		frame-master = "codec";
> 		bitclock-inversion = <1>;
> 		simple-audio-card,cpu {
> 			sound-dai = <&audio1 0>;
> 			bitclock-inversion = <0>;
> 		};
> 		simple-audio-card,codec {
> 			sound-dai = <&codec 0>;
> 			system-clock-frequency = <12000000>;
> 		};
> 	};
> ...
> 
> I can participate in the implementation too.

Thanks. I will prepare the multi DAI links and send you the first
patches.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/19/2014 03:46 PM, Mark Brown wrote:
> On Wed, Mar 19, 2014 at 12:08:55PM +0200, Jyri Sarha wrote:
>
>> While we are at it we could update the bitclock-master and frame-master
>> syntax to be like this:
>
>> bitclock-master = "cpu"
>> frame-master = "codec"
>
>> With the above explicit definition all the daifmt settings could be defined
>> in link level. For backwards compatibility we could still define that
>> omitting the value equals "codec" and omitting the property equals "cpu".
>
> It seems it'd be a bit more idiomatic to do that with a phandle rather
> than with a string in order to allow extensions for things like TDM (the
> I2S to mono speaker driver use case for example).
>

You mean a like this:

sound {
     compatible = "simple-audio-card";
     simple-audio-card,name = "Simple Audio";
     simple-audio-card,widgets = ...
     simple-audio-card,routing = ...

     simple-audio-card,dai-link@0 {        /* I2S - codec */
         format = "i2s";
         bitclock-master = <&codec 0>
         frame-master = <&codec 0>;
         bitclock-inversion = <1>;
         simple-audio-card,cpu {
             sound-dai = <&audio1 0>;
             bitclock-inversion = <0>;
         };
         simple-audio-card,codec {
             sound-dai = <&codec 0>;
             system-clock-frequency = <12000000>;
         };
     };
...

Yep, that makes sense when considering tdm setups with multiple codecs 
on the same wires.

Best regards,
Jyri

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Lars-Peter Clausen]

On 03/19/2014 11:08 AM, Jyri Sarha wrote:
[...]
> It may sometimes be helpful to allow overwriting link level settings in dai
> level. In order to do that it should be possible to write all daifmt
> settings explicitly like this:
>
> bitclock-inversion = <0>; /* <0> = no bitclock-inversion */

When does this make sense? Either the bitclock is inverted for all of them 
or for none.

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

Re: [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Wed, Mar 19, 2014 at 08:32:06PM +0200, Jyri Sarha wrote:
> On 03/19/2014 03:46 PM, Mark Brown wrote:

> >It seems it'd be a bit more idiomatic to do that with a phandle rather
> >than with a string in order to allow extensions for things like TDM (the
> >I2S to mono speaker driver use case for example).

> You mean a like this:

> sound {

>         bitclock-master = <&codec 0>
>         frame-master = <&codec 0>;
>         bitclock-inversion = <1>;
>         simple-audio-card,cpu {

Possibly referring to the subnodes rather than the device but yeah.

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

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

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:
> On 03/19/2014 11:08 AM, Jyri Sarha wrote:
> [...]
>> It may sometimes be helpful to allow overwriting link level settings
>> in dai
>> level. In order to do that it should be possible to write all daifmt
>> settings explicitly like this:
>>
>> bitclock-inversion = <0>; /* <0> = no bitclock-inversion */
>
> When does this make sense? Either the bitclock is inverted for all of
> them or for none.
>

Definition of clock signal and it's inversion varies between chip 
manufacturers and sometimes it may not be possible to get all the dai 
drivers to work identically in this respect. Because of this in some 
cases there may be a need to set the inversion bit only at one end of 
the link.

I think there was an example of this in some mail regarding the 
simple-card DT-bidings, but I can't find it ATM.

Best regards,
Jyri

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Mark Brown]

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

On Wed, Mar 19, 2014 at 09:15:14PM +0200, Jyri Sarha wrote:
> On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:

> >When does this make sense? Either the bitclock is inverted for all of
> >them or for none.

> Definition of clock signal and it's inversion varies between chip
> manufacturers and sometimes it may not be possible to get all the dai
> drivers to work identically in this respect. Because of this in some cases
> there may be a need to set the inversion bit only at one end of the link.

No, Linux has a definition of all the clock modes which applies to all
devices regardless of what the manufacturer documents in their
datasheet.

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

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Lars-Peter Clausen]

On 03/19/2014 08:21 PM, Mark Brown wrote:
> On Wed, Mar 19, 2014 at 09:15:14PM +0200, Jyri Sarha wrote:
>> On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:
>
>>> When does this make sense? Either the bitclock is inverted for all of
>>> them or for none.
>
>> Definition of clock signal and it's inversion varies between chip
>> manufacturers and sometimes it may not be possible to get all the dai
>> drivers to work identically in this respect. Because of this in some cases
>> there may be a need to set the inversion bit only at one end of the link.
>
> No, Linux has a definition of all the clock modes which applies to all
> devices regardless of what the manufacturer documents in their
> datasheet.
>

Yep. The clock properties are well defined for the different modes that can 
be specified in the format property. It's up to the driver to translate this 
to driver specific settings. If two drivers behave differently for the same 
mode one of them (or both) are broken.

We should probably add the definitions for the different formats to the DT 
bindings. E.g. what is default, what is inverted polarity, etc.

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

Re: [alsa-devel] [PATCH v3 4/4] ASoC: simple-card: Add DT documentation for multi-DAI links

[Jyri Sarha]

On 03/19/2014 09:31 PM, Lars-Peter Clausen wrote:
> On 03/19/2014 08:21 PM, Mark Brown wrote:
>> On Wed, Mar 19, 2014 at 09:15:14PM +0200, Jyri Sarha wrote:
>>> On 03/19/2014 08:51 PM, Lars-Peter Clausen wrote:
>>
>>>> When does this make sense? Either the bitclock is inverted for all of
>>>> them or for none.
>>
>>> Definition of clock signal and it's inversion varies between chip
>>> manufacturers and sometimes it may not be possible to get all the dai
>>> drivers to work identically in this respect. Because of this in some
>>> cases
>>> there may be a need to set the inversion bit only at one end of the
>>> link.
>>
>> No, Linux has a definition of all the clock modes which applies to all
>> devices regardless of what the manufacturer documents in their
>> datasheet.
>>
>
> Yep. The clock properties are well defined for the different modes that
> can be specified in the format property. It's up to the driver to
> translate this to driver specific settings. If two drivers behave
> differently for the same mode one of them (or both) are broken.
>

After a little thinking it is clear to me too that only reason to have 
this overwrite capability is a badly written dai driver. Even with buggy 
HW it should always be possible to present the working modes in daifmt 
terms. Ok, let's remove it.

Here is an updated DT example with updated phandle for master settings:

sound {
     compatible = "simple-audio-card";
     simple-audio-card,name = "Simple Audio";
     simple-audio-card,widgets = ...
     simple-audio-card,routing = ...

     simple-audio-card,dai-link@0 {        /* I2S - codec */
         format = "i2s";
         bitclock-master = <&dai_link_master>
         frame-master = <&dai_link_master>;
         bitclock-inversion;
         simple-audio-card,cpu {
             sound-dai = <&audio1 0>;
         };
         dai_link_master: simple-audio-card,codec {
             sound-dai = <&codec 0>;
             system-clock-frequency = <12000000>;
         };
     };
...


> We should probably add the definitions for the different formats to the
> DT bindings. E.g. what is default, what is inverted polarity, etc.
>

That is a good idea.

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