| Message ID | d054780a0edf4b2338a52e48bff9144e19aa614f.1394883134.git.moinejf@free.fr (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@free.fr> > --- [...] 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
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.
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?
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.
On 03/18/2014 10:17 AM, Jean-Francois Moine wrote: > 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] ... > > 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
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).
On Wed, 19 Mar 2014 12:08:55 +0200 Jyri Sarha <jsarha@ti.com> 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.
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
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.
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.
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
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.
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
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
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>; + }; +};
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(-)