Audio codec device tree entries

Audio codec device tree entries

[Jon Smirl]

Is this what the device tree entries should look like?

First example is ac97 audio:

ac97@2000 {           // PSC1
      device_type = "sound";
      compatible = "mpc5200b-psc-ac97\0mpc5200-psc-ac97";
      cell-index = <0>;
      reg = <2000 100>;
      interrupts = <2 1 0>;
      interrupt-parent = <&mpc5200_pic>;
      codec-handle = <&codec0>
};

pseudo-sound@0 { // use to trigger loading platform specific fabric driver
      device_type = "pseudo-sound"
};

codec0:codec@0 {
      device_type = "codec"
      compatible = "stac9766\0ac97"
};

Second is i2s/i2c connected:
How do I link this to the i2c bus?

i2s@2000 {           // PSC1
      device_type = "sound";
      compatible = "mpc5200b-psc-i2s\0mpc5200-psc-i2s";
      cell-index = <0>;
      reg = <2000 100>;
      interrupts = <2 1 0>;
      interrupt-parent = <&mpc5200_pic>;
      codec-handle = <&codec0>
};

i2c@3d00 {
      device_type = "i2c";
      compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
      cell-index = <0>;
      reg = <3d00 40>;
      interrupts = <2 f 0>;
      interrupt-parent = <&mpc5200_pic>;
      fsl5200-clocking;
};

pseudo-sound@0 { // use to trigger loading platform specific fabric driver
      device_type = "pseudo-sound"
};

codec0:codec@0 {
      device_type = "codec"
      compatible = "tas5508"
};





-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[David Gibson]

On Mon, Oct 22, 2007 at 09:59:00PM -0400, Jon Smirl wrote:
> Is this what the device tree entries should look like?
> 
> First example is ac97 audio:
> 
> ac97@2000 {           // PSC1
>       device_type = "sound";

Kill all these device_type values.

>       compatible = "mpc5200b-psc-ac97\0mpc5200-psc-ac97";

dtc now supports the more readable:
	compatible = "mpc5200b-psc-ac97", "mpc5200-psc-ac97";
Use it.

>       cell-index = <0>;

cell-index should only be present if the device is within a
system-on-chip *and* that device number is used to program some global
register somewhere.

>       reg = <2000 100>;
>       interrupts = <2 1 0>;
>       interrupt-parent = <&mpc5200_pic>;
>       codec-handle = <&codec0>
> };
> 
> pseudo-sound@0 { // use to trigger loading platform specific fabric driver
>       device_type = "pseudo-sound"
> };

This looks completely bogus.  The device tree should represent actual
hardware.

> codec0:codec@0 {

Space after the : please.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Audio codec device tree entries

[Grant Likely]

On 10/22/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> Is this what the device tree entries should look like?
>
> First example is ac97 audio:
>
> ac97@2000 {           // PSC1
>       device_type = "sound";
>       compatible = "mpc5200b-psc-ac97\0mpc5200-psc-ac97";

This format is so, like. 3 months ago.  :-)

dtc now supports a comma separated format:  property = "string1",
"string2", "string3"

Embedded nulls no longer needed.

>       cell-index = <0>;
>       reg = <2000 100>;
>       interrupts = <2 1 0>;
>       interrupt-parent = <&mpc5200_pic>;
>       codec-handle = <&codec0>
> };

Isn't this the ac97 bus node?  Can't the ac97 codecs simply be
children of this bus?

>
> pseudo-sound@0 { // use to trigger loading platform specific fabric driver
>       device_type = "pseudo-sound"
> };

I don't think this is a good idea.  The device tree is for describing
your hardware; so the layout should reflect that.  Make the platform
code do the right thing with the real nodes.

>
> codec0:codec@0 {
>       device_type = "codec"
>       compatible = "stac9766\0ac97"
> };
>
> Second is i2s/i2c connected:
> How do I link this to the i2c bus?
>
> i2s@2000 {           // PSC1
>       device_type = "sound";
>       compatible = "mpc5200b-psc-i2s\0mpc5200-psc-i2s";
>       cell-index = <0>;
>       reg = <2000 100>;
>       interrupts = <2 1 0>;
>       interrupt-parent = <&mpc5200_pic>;
>       codec-handle = <&codec0>
> };
>
> i2c@3d00 {
>       device_type = "i2c";
>       compatible = "mpc5200b-i2c\0mpc5200-i2c\0fsl-i2c";
>       cell-index = <0>;
>       reg = <3d00 40>;
>       interrupts = <2 f 0>;
>       interrupt-parent = <&mpc5200_pic>;
>       fsl5200-clocking;
> };
>
> pseudo-sound@0 { // use to trigger loading platform specific fabric driver
>       device_type = "pseudo-sound"
> };
>
> codec0:codec@0 {
>       device_type = "codec"
>       compatible = "tas5508"
> };

I think this is what you want:

ac97@2000 {           // PSC1
      compatible = "fsl,mpc5200b-psc-ac97","fsl,mpc5200-psc-ac97";
      cell-index = <0>;
      reg = <2000 100>;
      #address-cells = <1>;
      #size-cells = <0>;
      interrupts = <2 1 0>;
      interrupt-parent = <&mpc5200_pic>;
      // The following codec stuff could be a little off; It has been
a while since I've looked
      // at ac97 codec interfacing, but if I remember correctly you
can have multiple
      // codecs on an ac97 bus; an audio codec and a modem codec.  The following
      // reflects that understanding...
      ac97-codec@0 {
            // This could be the audio codec
            reg = <0>;
            compatible = "stac9766","ac97-audio"
      };
      ac97-codec@1 {
            // This could be the modem codec
            reg = <1>;
            compatible = "super-sexy-modem-codec","ac97-modem"
      };
};


// Now here's a big example for i2c.
// I've shown 3 audio interfaces; 2 i2s busses and 1 i2c controller.
// This may not be realistic on a 5200, but it is a possible scenario
and this shows
// that it can be handled elegantly.
i2s@2000 {           // PSC1
      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
      cell-index = <0>;
      reg = <2000 100>;
      interrupts = <2 1 0>;
      interrupt-parent = <&mpc5200_pic>;

      // There are 2 codecs on this i2s bus; either only one at a time
can be used, or
      // (if both the i2s controller and codecs support it) both at
the same time if audio
      // stream interleaving is supported.
      codec-handle = <&codec0 &codec2>;
};

i2s@2200 {           // PSC2
      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
      cell-index = <0>;
      reg = <2200 100>;
      interrupts = <2 2 0>;
      interrupt-parent = <&mpc5200_pic>;
      codec-handle = <&codec1>;
};

i2c@3d00 {
      compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
      #address-cells = <1>;
      #size-cells = <0>;
      cell-index = <0>;
      reg = <3d00 40>;
      interrupts = <2 f 0>;
      interrupt-parent = <&mpc5200_pic>;
      fsl5200-clocking;

      codec0: i2s-codec@0 {
            compatible = "<mfg>,tas5508";
            reg = <0>;
      };
      codec1: i2s-codec@1 {
            compatible = "<mfg>,tas5508";
            reg = <1>;
      };
      codec2: i2s-codec@2 {
            compatible = "<mfg>,tas5508";
            reg = <2>;
      };
};

So, this scheme describes the hardware, but it does not describe 2 of
your questions:

1. How does the device tree describe the myriad of possible circuits
that an audio codec can get dropped into, and
2. How do the drivers get probed

For question #1, I think the answer is simple... it doesn't try.  :-)

As was described in the previous thread, trying to describe the
circuit is very complex and figuring out what software would do with
such a description is even worse.  Instead, I propose the following:
- as much as possible, make the board firmware and the platform setup
code configure the GPIOs, port_config, and other 'fixed' configuration
- make the driver code look at either the board model/compatible
properties or add a board-unique value to the codec's compatible list.
 Either way the driver can apply board specific tweaks to its
behavious.  (I think the better solution is to add a value to the
front of the codec's compatible list because the same circuit can be
used on a different board and it can then claim compatibility with the
older board for just that part of the circuit).

(Segher, what are your thoughts on my above suggestion?)

As for question #2, I think you make the i2s driver probe on the i2s
bus node and the ac97 driver probe on the ac97 bus node.  In both
cases, the driver can find the link to the codec; determine what kind
of system it is running on, and instantiate the appropriate ASoC
fabric driver.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Segher Boessenkool]

> Isn't this the ac97 bus node?  Can't the ac97 codecs simply be
> children of this bus?

They should be, yes.

>> pseudo-sound@0 { // use to trigger loading platform specific fabric 
>> driver
>>       device_type = "pseudo-sound"
>> };
>
> I don't think this is a good idea.  The device tree is for describing
> your hardware; so the layout should reflect that.  Make the platform
> code do the right thing with the real nodes.

There _can_ be good reasons for using a "pseudo-device" node,
but I don't see any such reasons here.  The main reason for
doing a "pseudo" is if there is some hardware you need to
describe, but it doesn't fit anywhere in the device tree; one
example is certain weird interrupt routings.

> I think this is what you want:
>
> ac97@2000 {           // PSC1
>       compatible = "fsl,mpc5200b-psc-ac97","fsl,mpc5200-psc-ac97";
>

>       // The following codec stuff could be a little off; It has been
> a while since I've looked
>       // at ac97 codec interfacing, but if I remember correctly you
> can have multiple
>       // codecs on an ac97 bus; an audio codec and a modem codec.  The 
> following
>       // reflects that understanding...

You can have more than two, and then there is the weird stuff where
"hd" codecs have more than one address.  But this all can be neatly
described with "reg", so no problem.

>       ac97-codec@0 {

"sound@0", says the generic naming doc.

>       ac97-codec@1 {
>             // This could be the modem codec

So "modem@1".

> i2s@2000 {           // PSC1

>       codec-handle = <&codec0 &codec2>;

I would really do it the other way around: have the codec node point
to the I2S bus.

>       codec0: i2s-codec@0 {

>       codec1: i2s-codec@1 {

>       codec2: i2s-codec@2 {

0, 1, 2 aren't valid I2C addresses.  But this was only an example ;-)

> So, this scheme describes the hardware, but it does not describe 2 of
> your questions:
>
> 1. How does the device tree describe the myriad of possible circuits
> that an audio codec can get dropped into, and
> 2. How do the drivers get probed
>
> For question #1, I think the answer is simple... it doesn't try.  :-)
>
> As was described in the previous thread, trying to describe the
> circuit is very complex and figuring out what software would do with
> such a description is even worse.  Instead, I propose the following:
> - as much as possible, make the board firmware and the platform setup
> code configure the GPIOs, port_config, and other 'fixed' configuration

Certainly agreed.

> - make the driver code look at either the board model/compatible
> properties or add a board-unique value to the codec's compatible list.

Make the _platform_ code look at the board version properties, and have
_it_ instantiate the correct fabric driver, with the correct 
configuration
for it.  Some of that configuration could be probed from the device tree
("codec gpio pin 0 is used as a level-low headphone detect switch") but
I wouldn't go over the top with it, there are just way way too many
different ways all of this is put together.

>  Either way the driver can apply board specific tweaks to its
> behavious.  (I think the better solution is to add a value to the
> front of the codec's compatible list because the same circuit can be
> used on a different board and it can then claim compatibility with the
> older board for just that part of the circuit).

The "fabric" stuff isn't really a codec property, but a board-global
property.  So that's how this should be described in the device tree
as well: board-global.

> As for question #2, I think you make the i2s driver probe on the i2s
> bus node and the ac97 driver probe on the ac97 bus node.  In both
> cases, the driver can find the link to the codec; determine what kind
> of system it is running on, and instantiate the appropriate ASoC
> fabric driver.

In the i2s case, if the i2s driver has to know about the codecs at all,
it should get references to the codecs passed in by the platform driver.

Same goes for the fabric driver.

Or, that's the only sane way of doing things I can imagine, anyway :-)


Segher

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:
> Is this what the device tree entries should look like?
> 
> First example is ac97 audio:
> 
> ac97@2000 {           // PSC1
>       device_type = "sound";
>       compatible = "mpc5200b-psc-ac97\0mpc5200-psc-ac97";
>       cell-index = <0>;
>       reg = <2000 100>;
>       interrupts = <2 1 0>;
>       interrupt-parent = <&mpc5200_pic>;
>       codec-handle = <&codec0>
> };

I think the codec node should be a child of this node, not a separate node. 
Then there should be another codec node as a child of the i2c node.  The first 
codec node should reference the other one via a phandle (so this part you 
already have correct).

Re: Audio codec device tree entries

[Segher Boessenkool]

>> First example is ac97 audio:
>>
>> ac97@2000 {           // PSC1

> I think the codec node should be a child of this node, not a separate 
> node.
> Then there should be another codec node as a child of the i2c node.

AC'97 codecs don't typically have an I2C connection (if ever).

You shouldn't have two device nodes for the same device if ever you
can help it; instead, have the device's sole device node contain a
property that points to the device node of its secondary bus.  That
property could be called "i2s-handle" for a standard codec (which
probably is a direct child of an IIC bus, itself).


Segher

Re: Audio codec device tree entries

[Jon Smirl]

Is this consensus on how the tree should look?

There is no attempt to describe the codec connections inside the
device tree. Instead ASoC handles this by loading a platform specific
fabric driver. A simple example of this is that it is some codecs can
assign the six channel outputs under software control, the platform
specific driver tells which output is wired to the physical output
jacks so that alsa can label them correctly.

I'm still not clear on how to trigger the load of the fabric driver.
Right now I have a single kernel that works on Efika and my target
hardware.  This gets sorted out by define_machine(xxxx). I'll write
some code tonight to figure out how to load drivers and match on
codec0, codec1, etc. But how do I probe for the fabric driver I need
to figure out whether to load the Efika one or my target one.

ac97@2000 {           // PSC1
      compatible = "fsl,mpc5200b-psc-ac97","fsl,mpc5200-psc-ac97";
      cell-index = <0>;
      reg = <2000 100>;
      #address-cells = <1>;
      #size-cells = <0>;
      interrupts = <2 1 0>;
      interrupt-parent = <&mpc5200_pic>;
      sound@0 {
            reg = <0>;
            compatible = "idt,stac9766","ac97-audio"
      };
      modem@1 {
            reg = <1>;
            compatible = "idt,modem-codec","ac97-modem"
      };
};

------------------------

i2s@2000 {           // PSC1
      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
      cell-index = <0>;
      reg = <2000 100>;
      interrupts = <2 1 0>;
      interrupt-parent = <&mpc5200_pic>;
};

i2s@2200 {           // PSC2
      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
      cell-index = <1>;
      reg = <2200 100>;
      interrupts = <2 2 0>;
      interrupt-parent = <&mpc5200_pic>;
};

i2c@3d00 {
      compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
      #address-cells = <1>;
      #size-cells = <0>;
      cell-index = <0>;
      reg = <3d00 40>;
      interrupts = <2 f 0>;
      interrupt-parent = <&mpc5200_pic>;
      fsl5200-clocking;

      codec0: i2s-codec@0 {
            compatible = "ti,tas5508";
            reg = <0>;
            i2s-handle = <&i2s@2000>;
      };
      codec1: i2s-codec@1 {
            compatible = "ti,tas5508";
            reg = <1>;
            i2s-handle = <&i2s@2000>;
      };
      codec2: i2s-codec@2 {
            compatible = "wolson,wm8750";
            reg = <2>;
            i2s-handle = <&i2s@2200>;
      };
};




-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:
> Is this consensus on how the tree should look?
> 
> There is no attempt to describe the codec connections inside the
> device tree. 

I don't think I agree with that.  The device tree should indicate which codec is 
connected to which I2S/AC97 device.

I see that you do that for the AC97 node, but not the I2S node.  Why?

> I'm still not clear on how to trigger the load of the fabric driver.
> Right now I have a single kernel that works on Efika and my target
> hardware.  This gets sorted out by define_machine(xxxx). I'll write
> some code tonight to figure out how to load drivers and match on
> codec0, codec1, etc. But how do I probe for the fabric driver I need
> to figure out whether to load the Efika one or my target one.

I've been struggling with that one, too.  To keep it simple, I have the fabric 
driver just search for all the I2S nodes in my tree, and create ASoC objects for 
each one it finds.  There's some hackery there, but I don't think we need to 
solve all the problems at once.  The only thing that *has* to be right the first 
time is the device tree.

> i2s@2200 {           // PSC2
>       compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
>       cell-index = <1>;
>       reg = <2200 100>;
>       interrupts = <2 2 0>;
>       interrupt-parent = <&mpc5200_pic>;
> };
> 
> i2c@3d00 {
>       compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
>       #address-cells = <1>;
>       #size-cells = <0>;
>       cell-index = <0>;
>       reg = <3d00 40>;
>       interrupts = <2 f 0>;
>       interrupt-parent = <&mpc5200_pic>;
>       fsl5200-clocking;
> 
>       codec0: i2s-codec@0 {
>             compatible = "ti,tas5508";
>             reg = <0>;
>             i2s-handle = <&i2s@2000>;
>       };

I'd do this the other way around -- that is:

i2s@2200 {           // PSC2
       	compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
	...
	i2c-handle = <&codec0>;	 /* Or something like that */
};

The reason is because I think the I2S driver will be instantiated *first* as an 
I2S driver and then it will create the I2C instantiation.

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Jon Smirl wrote:
> > Is this consensus on how the tree should look?
> >
> > There is no attempt to describe the codec connections inside the
> > device tree.
>
> I don't think I agree with that.  The device tree should indicate which codec is
> connected to which I2S/AC97 device.

What I meant was that there is no attempt to describe how the codec is
connected to the external world. Those connections are described in
the fabric driver.

I'm getting conflicting opinions on how the devices should be linked
into the tree. We should pick one and add it to the documentation.

The DTC experts need to tell us which way to make the pointers between
i2s and i2c for the codec.  Here's a another way it could be done that
looks more like the ac97 model.

i2s@2000 {           // PSC1
     compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
     cell-index = <0>;
     reg = <2000 100>;
     interrupts = <2 1 0>;
     interrupt-parent = <&mpc5200_pic>;
     codec0: i2s-codec@0 {
           compatible = "ti,tas5508";
           reg = <0>;
           i2c-handle = <&i2c@3d00>;
     };
     codec1: i2s-codec@1 {
           compatible = "ti,tas5508";
           reg = <1>;
           i2c-handle = <&i2c@3d00>;
     };
};

i2s@2200 {           // PSC2
     compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
     cell-index = <1>;
     reg = <2200 100>;
     interrupts = <2 2 0>;
     interrupt-parent = <&mpc5200_pic>;
     codec2: i2s-codec@2 {
           compatible = "wolson,wm8750";
           reg = <2>;
           i2c-handle = <&i2c@3d00>;
     };
};

i2c@3d00 {
     compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
     #address-cells = <1>;
     #size-cells = <0>;
     cell-index = <0>;
     reg = <3d00 40>;
     interrupts = <2 f 0>;
     interrupt-parent = <&mpc5200_pic>;
     fsl5200-clocking;
}

----- or would this be better? ---------------------

i2s@2000 {           // PSC1
     compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
     cell-index = <0>;
     reg = <2000 100>;
     interrupts = <2 1 0>;
     interrupt-parent = <&mpc5200_pic>;
     i2s-codec@0 {
           compatible = "ti,tas5508";
           reg = <0>;
     };
     i2s-codec@1 {
           compatible = "ti,tas5508";
           reg = <1>;
     };
};

i2s@2200 {           // PSC2
     compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
     cell-index = <1>;
     reg = <2200 100>;
     interrupts = <2 2 0>;
     interrupt-parent = <&mpc5200_pic>;
     i2s-codec@2 {
           compatible = "wolson,wm8750";
           reg = <2>;
     };
};

i2c@3d00 {
     compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
     #address-cells = <1>;
     #size-cells = <0>;
     cell-index = <0>;
     reg = <3d00 40>;
     interrupts = <2 f 0>;
     interrupt-parent = <&mpc5200_pic>;
     fsl5200-clocking;
     i2c-handle = <&i2s-codec@0 &i2s-codec@1 &i2s-codec@2>
}



>
> I see that you do that for the AC97 node, but not the I2S node.  Why?
>
> > I'm still not clear on how to trigger the load of the fabric driver.
> > Right now I have a single kernel that works on Efika and my target
> > hardware.  This gets sorted out by define_machine(xxxx). I'll write
> > some code tonight to figure out how to load drivers and match on
> > codec0, codec1, etc. But how do I probe for the fabric driver I need
> > to figure out whether to load the Efika one or my target one.
>
> I've been struggling with that one, too.  To keep it simple, I have the fabric
> driver just search for all the I2S nodes in my tree, and create ASoC objects for
> each one it finds.  There's some hackery there, but I don't think we need to
> solve all the problems at once.  The only thing that *has* to be right the first
> time is the device tree.
>
> > i2s@2200 {           // PSC2
> >       compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
> >       cell-index = <1>;
> >       reg = <2200 100>;
> >       interrupts = <2 2 0>;
> >       interrupt-parent = <&mpc5200_pic>;
> > };
> >
> > i2c@3d00 {
> >       compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
> >       #address-cells = <1>;
> >       #size-cells = <0>;
> >       cell-index = <0>;
> >       reg = <3d00 40>;
> >       interrupts = <2 f 0>;
> >       interrupt-parent = <&mpc5200_pic>;
> >       fsl5200-clocking;
> >
> >       codec0: i2s-codec@0 {
> >             compatible = "ti,tas5508";
> >             reg = <0>;
> >             i2s-handle = <&i2s@2000>;
> >       };
>
> I'd do this the other way around -- that is:
>
> i2s@2200 {           // PSC2
>         compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
>         ...
>         i2c-handle = <&codec0>;  /* Or something like that */
> };
>
> The reason is because I think the I2S driver will be instantiated *first* as an
> I2S driver and then it will create the I2C instantiation.
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:

> What I meant was that there is no attempt to describe how the codec is
> connected to the external world. Those connections are described in
> the fabric driver.

Hmmm, I'm not sure I'm okay with that.  We can always add properties to those 
nodes if it's necessary.  However, now you're basically defining some parts of 
the board layout in the DTS, and some parts in the fabric driver.  On PowerPC 
platforms, the fabric driver is supposed to get board layout information from 
the device tree.

> I'm getting conflicting opinions on how the devices should be linked
> into the tree. We should pick one and add it to the documentation.

It's a battle of wills!

> The DTC experts need to tell us which way to make the pointers between
> i2s and i2c for the codec.  Here's a another way it could be done that
> looks more like the ac97 model.
> 
> i2s@2000 {           // PSC1
>      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
>      cell-index = <0>;
>      reg = <2000 100>;
>      interrupts = <2 1 0>;
>      interrupt-parent = <&mpc5200_pic>;
>      codec0: i2s-codec@0 {
>            compatible = "ti,tas5508";
>            reg = <0>;
>            i2c-handle = <&i2c@3d00>;
>      };
>      codec1: i2s-codec@1 {
>            compatible = "ti,tas5508";
>            reg = <1>;
>            i2c-handle = <&i2c@3d00>;
>      };
> };
> 
> i2s@2200 {           // PSC2
>      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
>      cell-index = <1>;
>      reg = <2200 100>;
>      interrupts = <2 2 0>;
>      interrupt-parent = <&mpc5200_pic>;
>      codec2: i2s-codec@2 {

This should probably be codec0, since it's the first code on this I2S bus.

>            compatible = "wolson,wm8750";
>            reg = <2>;
>            i2c-handle = <&i2c@3d00>;
>      };
> };
> 
> i2c@3d00 {
>      compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
>      #address-cells = <1>;
>      #size-cells = <0>;
>      cell-index = <0>;
>      reg = <3d00 40>;
>      interrupts = <2 f 0>;
>      interrupt-parent = <&mpc5200_pic>;
>      fsl5200-clocking;
> }

My vote is for this version.  In fact, I think it *has* to be this way.  If 
you're using a CS4270 codec (as I am), the I2C interface is *optional*.  So I 
want the I2S node to point to the I2C node if it exists.

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Jon Smirl wrote:
>
> > What I meant was that there is no attempt to describe how the codec is
> > connected to the external world. Those connections are described in
> > the fabric driver.
>
> Hmmm, I'm not sure I'm okay with that.  We can always add properties to those
> nodes if it's necessary.  However, now you're basically defining some parts of
> the board layout in the DTS, and some parts in the fabric driver.  On PowerPC
> platforms, the fabric driver is supposed to get board layout information from
> the device tree.

No, not always; if the description is too complex (like gpio's for
instance) then it is perfectly valid to build the knowledge into the
platform code.  It should be avoided; but the device tree doesn't need
to describe *everything*.

>
> > i2s@2200 {           // PSC2
> >      compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
> >      cell-index = <1>;
> >      reg = <2200 100>;
> >      interrupts = <2 2 0>;
> >      interrupt-parent = <&mpc5200_pic>;
> >      codec2: i2s-codec@2 {
>
> This should probably be codec0, since it's the first code on this I2S bus.
>
> >            compatible = "wolson,wm8750";
> >            reg = <2>;
> >            i2c-handle = <&i2c@3d00>;
> >      };
> > };
> >
> > i2c@3d00 {
> >      compatible = "fsl,mpc5200b-i2c", "fsl,mpc5200-i2c", "fsl-i2c";
> >      #address-cells = <1>;
> >      #size-cells = <0>;
> >      cell-index = <0>;
> >      reg = <3d00 40>;
> >      interrupts = <2 f 0>;
> >      interrupt-parent = <&mpc5200_pic>;
> >      fsl5200-clocking;
> > }
>
> My vote is for this version.  In fact, I think it *has* to be this way.  If
> you're using a CS4270 codec (as I am), the I2C interface is *optional*.  So I
> want the I2S node to point to the I2C node if it exists.

It doesn't have to be this way.  If the codec does not have a control
interface, then it can happily be a child of the i2s node.  But if it
*does*; don't break convention by separating it from it's control
interface.

I strongly recommend following the lead of ethernet phys and mdio busses here.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[David Gibson]

On Wed, Oct 24, 2007 at 09:28:42AM -0600, Grant Likely wrote:
> On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> > Jon Smirl wrote:
[snip]
> > My vote is for this version.  In fact, I think it *has* to be this way.  If
> > you're using a CS4270 codec (as I am), the I2C interface is *optional*.  So I
> > want the I2S node to point to the I2C node if it exists.
> 
> It doesn't have to be this way.  If the codec does not have a control
> interface, then it can happily be a child of the i2s node.  But if it
> *does*; don't break convention by separating it from it's control
> interface.
> 
> I strongly recommend following the lead of ethernet phys and mdio
> busses here.

Yes.  Devices should appear on the bus from which they're addressable,
that is from the control interface in this case.  Sometimes different
things need to be done for bus-bridges which are configured from a
different bus than the one they bridge, but this is not such a
situation.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> > Jon Smirl wrote:
> > > Is this consensus on how the tree should look?
> > >
> > > There is no attempt to describe the codec connections inside the
> > > device tree.
> >
> > I don't think I agree with that.  The device tree should indicate which codec is
> > connected to which I2S/AC97 device.
>
> What I meant was that there is no attempt to describe how the codec is
> connected to the external world. Those connections are described in
> the fabric driver.
>
> I'm getting conflicting opinions on how the devices should be linked
> into the tree. We should pick one and add it to the documentation.

Two valid methods have been proposed
1. a codec-

> The DTC experts need to tell us which way to make the pointers between
> i2s and i2c for the codec.  Here's a another way it could be done that
> looks more like the ac97 model.

I *really* don't think this is a good idea.  Put the node on the bus
that the device is addressed from.  I2S is the *data* path, not the
*control* path, but you cannot control the codec from there.

Your suggestion only looks more like the AC97 model if you're looking
at the data path.  If you're looking at the control path it looks
completely different.  The device tree convention is to orient around
the control path.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 10/24/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> > On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> > > Jon Smirl wrote:
> > > > Is this consensus on how the tree should look?
> > > >
> > > > There is no attempt to describe the codec connections inside the
> > > > device tree.
> > >
> > > I don't think I agree with that.  The device tree should indicate which codec is
> > > connected to which I2S/AC97 device.
> >
> > What I meant was that there is no attempt to describe how the codec is
> > connected to the external world. Those connections are described in
> > the fabric driver.
> >
> > I'm getting conflicting opinions on how the devices should be linked
> > into the tree. We should pick one and add it to the documentation.
>
> Two valid methods have been proposed
> 1. a codec-

oops

1. a codec-handle property in the i2s node
2. an i2s-handle property in the codec node

Either are reasonable.  I prefer putting the handle in the i2s node;
but I'm looking at it from the way that ethernet phys are being
described currently.  The other is also perfectly valid.

I suppose it depends on what point of view you see the system from; either:
a. the codec is supported by the i2s bus, in which case use the
i2s-handle property
b. the i2s bus is supported by the codec; in which case use the
codec-handle property.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > Two valid methods have been proposed
> > 1. a codec-
>
> oops
>
> 1. a codec-handle property in the i2s node
> 2. an i2s-handle property in the codec node
>
> Either are reasonable.  I prefer putting the handle in the i2s node;
> but I'm looking at it from the way that ethernet phys are being
> described currently.  The other is also perfectly valid.
>
> I suppose it depends on what point of view you see the system from; either:
> a. the codec is supported by the i2s bus, in which case use the
> i2s-handle property
> b. the i2s bus is supported by the codec; in which case use the
> codec-handle property.

Do you want to pick one and add it to the device tree documentation
with an example for i2s and ac97? I'll use which ever one is picked.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > Two valid methods have been proposed
> > > 1. a codec-
> >
> > oops
> >
> > 1. a codec-handle property in the i2s node
> > 2. an i2s-handle property in the codec node
> >
> > Either are reasonable.  I prefer putting the handle in the i2s node;
> > but I'm looking at it from the way that ethernet phys are being
> > described currently.  The other is also perfectly valid.
> >
> > I suppose it depends on what point of view you see the system from; either:
> > a. the codec is supported by the i2s bus, in which case use the
> > i2s-handle property
> > b. the i2s bus is supported by the codec; in which case use the
> > codec-handle property.
>
> Do you want to pick one and add it to the device tree documentation
> with an example for i2s and ac97? I'll use which ever one is picked.

Sure, I'll draft something up and post it for review.

On the device probing front; what about this method:

Rather than trying to figure things out from the board model, or the
combination of the codec and i2s bus; add another node to represent
the sound circuit.  All that node would need is a unique compatible
property and a phandle to either the i2s bus or the codec (depending
on which binding approach is used).  It could have additional
properties to represent optional features, etc.

For example:
sound@0 {
      compatible = "<mfg>,<board>,sound"   // The board might have
more than one sound i/f which could be wired differently
      codec-handle = <&codec0>;
};

This would give your fabric driver something unique to probe on; but
the i2c, i2s and codec nodes which actually describe interconnects
will still be present.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > Do you want to pick one and add it to the device tree documentation
> > with an example for i2s and ac97? I'll use which ever one is picked.
>
> Sure, I'll draft something up and post it for review.
>
> On the device probing front; what about this method:
>
> Rather than trying to figure things out from the board model, or the
> combination of the codec and i2s bus; add another node to represent
> the sound circuit.  All that node would need is a unique compatible
> property and a phandle to either the i2s bus or the codec (depending
> on which binding approach is used).  It could have additional
> properties to represent optional features, etc.

That's the pseudo-sound node proposal that other people objected to.

It makes sense to me, there needs to be some way to trigger loading
the fabric driver.

>
> For example:
> sound@0 {
>       compatible = "<mfg>,<board>,sound"   // The board might have
> more than one sound i/f which could be wired differently
>       codec-handle = <&codec0>;
> };

Do you even need the parameters,  how about simply this?

sound-fabric {
};

That will trigger loading all of the sound-fabric drivers built into
the kernel. In their probe functions they can look in the device tree
and extract the machine name and then decide to stay loaded or fail
the probe.

> This would give your fabric driver something unique to probe on; but
> the i2c, i2s and codec nodes which actually describe interconnects
> will still be present.
>
> Cheers,
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
> grant.likely@secretlab.ca
> (403) 399-0195
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:

> It makes sense to me, there needs to be some way to trigger loading
> the fabric driver.

Well, you only really two have choices:

1) Probe on the AC97/SSI nodes
2) When the driver initializes, just scan all the nodes in the device tree (no 
probing).

I think option #2 makes the most sense, because option #1 says that your fabric 
driver is really an AC97 driver, which it isn't.

You can try to make the fabric driver into a bus driver, but I think that just 
complicates things.

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Jon Smirl wrote:
>
> > It makes sense to me, there needs to be some way to trigger loading
> > the fabric driver.
>
> Well, you only really two have choices:
>
> 1) Probe on the AC97/SSI nodes
> 2) When the driver initializes, just scan all the nodes in the device tree (no
> probing).
>
> I think option #2 makes the most sense, because option #1 says that your fabric
> driver is really an AC97 driver, which it isn't.

If you need to scan the tree, don't do it when the driver registers,
do it in the platform code instead.  Otherwise you unconditionally
incur the penalty of scanning the whole device tree on every system
that loads the driver, regardless of whether or not the device is
present.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Timur Tabi]

Grant Likely wrote:

> If you need to scan the tree, don't do it when the driver registers,
> do it in the platform code instead.  Otherwise you unconditionally
> incur the penalty of scanning the whole device tree on every system
> that loads the driver, regardless of whether or not the device is
> present.

So your saying that the fabric driver should be embedded in the platform driver?

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
> > If you need to scan the tree, don't do it when the driver registers,
> > do it in the platform code instead.  Otherwise you unconditionally
> > incur the penalty of scanning the whole device tree on every system
> > that loads the driver, regardless of whether or not the device is
> > present.
>
> So your saying that the fabric driver should be embedded in the platform driver?

No; that's not what I mean.

But the scanning of the device tree to decide whether or not to
instantiate the driver should be called from platform code.  The
device tree should not be scanned on driver loading.

In other words: populate either a platform_device or an
of_platform_device that the driver can bind against.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > Do you want to pick one and add it to the device tree documentation
> > > with an example for i2s and ac97? I'll use which ever one is picked.
> >
> > Sure, I'll draft something up and post it for review.
> >
> > On the device probing front; what about this method:
> >
> > Rather than trying to figure things out from the board model, or the
> > combination of the codec and i2s bus; add another node to represent
> > the sound circuit.  All that node would need is a unique compatible
> > property and a phandle to either the i2s bus or the codec (depending
> > on which binding approach is used).  It could have additional
> > properties to represent optional features, etc.
>
> That's the pseudo-sound node proposal that other people objected to.
>
Heh, I'm one of the folks who objected to it; here's what was written:

> > >
> > > pseudo-sound@0 { // use to trigger loading platform specific fabric driver
> > >       device_type = "pseudo-sound"
> > > };
> >
> > I don't think this is a good idea.  The device tree is for describing
> > your hardware; so the layout should reflect that.  Make the platform
> > code do the right thing with the real nodes.

What I objected to was that the pseudo-sound node didn't contain any
real information.  It was just being a hook to trigger calling a probe
function.  If you're going to do that then you might as well just call
it directly from platform code.

> > For example:
> > sound@0 {
> >       compatible = "<mfg>,<board>,sound"   // The board might have
> > more than one sound i/f which could be wired differently
> >       codec-handle = <&codec0>;
> > };

The difference here is that the node provides real information about
the board.  It has a compatible field which tells you *exactly* what
sound circuit is present on the board.  It can have additional
information that does make sense to encode into the device tree (ie.
the codec that is used).  It's not addressable (no registers or
anything), but it does describe the board.

It would be possible and reasonable for a single fabric driver to work
with many different circuit layouts as long as it has the information
needed to adapt each instance.

>
> Do you even need the parameters,  how about simply this?
>
> sound-fabric {
> };

But this goes back to having nodes that don't provide any information.
 You don't want that.

>
> That will trigger loading all of the sound-fabric drivers built into
> the kernel. In their probe functions they can look in the device tree
> and extract the machine name and then decide to stay loaded or fail
> the probe.
>

...

Now is probably a good time to mention that there is *nothing* in the
device tree that enforces a 1:1 relationship between device tree nodes
and driver instances.  Sure, it make sense to register the i2s and
codec drivers from probing on the i2s and codec nodes.  However, there
is nothing that prevents the codec driver from *also* registering a
fabric driver based on a property in the codec node or the board-level
compatible property.

Fabric drivers are codec specific anyway.  It's not all that
unrealistic for the device tree binding for a codec to have a list of
fabric drivers that it can register.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Timur Tabi]

Grant Likely wrote:

> Now is probably a good time to mention that there is *nothing* in the
> device tree that enforces a 1:1 relationship between device tree nodes
> and driver instances.  Sure, it make sense to register the i2s and
> codec drivers from probing on the i2s and codec nodes.  However, there
> is nothing that prevents the codec driver from *also* registering a
> fabric driver based on a property in the codec node or the board-level
> compatible property.

Wouldn't it make more sense for the fabric driver to register the codec driver? 
  The fabric driver is the "master" of the other drivers, should it needs to 
load and run first.

> Fabric drivers are codec specific anyway.

Yes and no.  They're really platform-specific, but they should be able to scan 
the device tree, determine which codecs are actually on the system, and then 
link in the appropriate codec driver.

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Grant Likely wrote:
>
> > Now is probably a good time to mention that there is *nothing* in the
> > device tree that enforces a 1:1 relationship between device tree nodes
> > and driver instances.  Sure, it make sense to register the i2s and
> > codec drivers from probing on the i2s and codec nodes.  However, there
> > is nothing that prevents the codec driver from *also* registering a
> > fabric driver based on a property in the codec node or the board-level
> > compatible property.
>
> Wouldn't it make more sense for the fabric driver to register the codec driver?
>   The fabric driver is the "master" of the other drivers, should it needs to
> load and run first.

It doesn't really matter.  There doesn't need to be a 1:1 relationship
between driver instances and device nodes.  A probe on one device node
can cause the instantiation of both a fabric driver and a codec driver
(just put the appropriate calls in the of_platform_bus probe hook).

>
> > Fabric drivers are codec specific anyway.
>
> Yes and no.  They're really platform-specific, but they should be able to scan
> the device tree, determine which codecs are actually on the system, and then
> link in the appropriate codec driver.

My point is; most likely if you change the codec, you need to change
the fabric driver too.

There will be many fabric drivers using a single codec,
but there will not be many codec drivers using a single fabric.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> Heh, I'm one of the folks who objected to it; here's what was written:
>
> > > >
> > > > pseudo-sound@0 { // use to trigger loading platform specific fabric driver
> > > >       device_type = "pseudo-sound"
> > > > };
> > >
> > > I don't think this is a good idea.  The device tree is for describing
> > > your hardware; so the layout should reflect that.  Make the platform
> > > code do the right thing with the real nodes.
>
> What I objected to was that the pseudo-sound node didn't contain any
> real information.  It was just being a hook to trigger calling a probe
> function.  If you're going to do that then you might as well just call
> it directly from platform code.

Calling it directly from the platform code is an option, but where
does the fabric driver code live? It doesn't make a lot of sense to
put ALSA code into arch/powerpc/platforms/52xx. I could make a
function call from arch/powerpc/platforms/52xx over to
sound/soc/powerpc but that's not very pretty.

Another option is to make the fabric driver a "struct platform_driver"
instead of a "struct of_platform_driver". "struct platform_driver" is
not being probed in the current mpc5200 code. This option makes senses
to me, "struct platform_driver" will load without a device tree node.
The driver will still need to check and see if it is on the right
platform when more than one is compiled in.

>
> > > For example:
> > > sound@0 {
> > >       compatible = "<mfg>,<board>,sound"   // The board might have
> > > more than one sound i/f which could be wired differently
> > >       codec-handle = <&codec0>;
> > > };
>
> The difference here is that the node provides real information about
> the board.  It has a compatible field which tells you *exactly* what
> sound circuit is present on the board.  It can have additional
> information that does make sense to encode into the device tree (ie.
> the codec that is used).  It's not addressable (no registers or
> anything), but it does describe the board.
>
> It would be possible and reasonable for a single fabric driver to work
> with many different circuit layouts as long as it has the information
> needed to adapt each instance.

That is how the Apple driver is implemented. There is a single fabric
driver that uses layout-id to set everything up to match the physical
PCB.
sound/aoa/fabrics/snd-aoa-fabric-layout.c

But Apple put the layout id down inside the a sound node:
/proc/device-tree/pci@f2000000/mac-io@17/i2s@0/i2s-a@10000/sound:
name             "sound"
device_type      "soundchip"
compatible       "AOAbase"
built-in
layout-id        00000046 (70)
object-model-version 00000002
vendor-id        0000106b (4203)
platform-tas-codec-ref ff98cba8
linux,phandle    ff985d48


>
> >
> > Do you even need the parameters,  how about simply this?
> >
> > sound-fabric {
> > };
>
> But this goes back to having nodes that don't provide any information.
>  You don't want that.
>
> >
> > That will trigger loading all of the sound-fabric drivers built into
> > the kernel. In their probe functions they can look in the device tree
> > and extract the machine name and then decide to stay loaded or fail
> > the probe.
> >
>
> ...
>
> Now is probably a good time to mention that there is *nothing* in the
> device tree that enforces a 1:1 relationship between device tree nodes
> and driver instances.  Sure, it make sense to register the i2s and
> codec drivers from probing on the i2s and codec nodes.  However, there
> is nothing that prevents the codec driver from *also* registering a
> fabric driver based on a property in the codec node or the board-level
> compatible property.

But there is something in the kernel that enforces it. I haven't
checked the powerpc code, but the PCI code won't probe anymore drivers
once one has attached to a device. The rule of one driver per device
is a good one. Places where that rule has been broken have caused a
lot of problems (fbdev vs DRM).

> Fabric drivers are codec specific anyway.  It's not all that
> unrealistic for the device tree binding for a codec to have a list of
> fabric drivers that it can register.

The codec drivers in asoc are completely agnostic. The same codec
driver works on x86, arm, powerpc, etc.


>
> Cheers,
> g.
>
> --
> Grant Likely, B.Sc., P.Eng.
> Secret Lab Technologies Ltd.
> grant.likely@secretlab.ca
> (403) 399-0195
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > Heh, I'm one of the folks who objected to it; here's what was written:
> >
> > > > >
> > > > > pseudo-sound@0 { // use to trigger loading platform specific fabric driver
> > > > >       device_type = "pseudo-sound"
> > > > > };
> > > >
> > > > I don't think this is a good idea.  The device tree is for describing
> > > > your hardware; so the layout should reflect that.  Make the platform
> > > > code do the right thing with the real nodes.
> >
> > What I objected to was that the pseudo-sound node didn't contain any
> > real information.  It was just being a hook to trigger calling a probe
> > function.  If you're going to do that then you might as well just call
> > it directly from platform code.
>
> Calling it directly from the platform code is an option, but where
> does the fabric driver code live? It doesn't make a lot of sense to
> put ALSA code into arch/powerpc/platforms/52xx. I could make a
> function call from arch/powerpc/platforms/52xx over to
> sound/soc/powerpc but that's not very pretty.
>
> Another option is to make the fabric driver a "struct platform_driver"
> instead of a "struct of_platform_driver". "struct platform_driver" is
> not being probed in the current mpc5200 code. This option makes senses
> to me, "struct platform_driver" will load without a device tree node.
> The driver will still need to check and see if it is on the right
> platform when more than one is compiled in.

Yes, this is a good approach.

> > It would be possible and reasonable for a single fabric driver to work
> > with many different circuit layouts as long as it has the information
> > needed to adapt each instance.
>
> That is how the Apple driver is implemented. There is a single fabric
> driver that uses layout-id to set everything up to match the physical
> PCB.
> sound/aoa/fabrics/snd-aoa-fabric-layout.c
>
> But Apple put the layout id down inside the a sound node:
> /proc/device-tree/pci@f2000000/mac-io@17/i2s@0/i2s-a@10000/sound:
> name             "sound"
> device_type      "soundchip"
> compatible       "AOAbase"
> built-in
> layout-id        00000046 (70)
> object-model-version 00000002
> vendor-id        0000106b (4203)
> platform-tas-codec-ref ff98cba8
> linux,phandle    ff985d48

Yes, this is the same idea.  I don't think benh and segher were
particularly fond of it though.  I think Segher in particular had a
preference for the platform code probing approach.

> > Now is probably a good time to mention that there is *nothing* in the
> > device tree that enforces a 1:1 relationship between device tree nodes
> > and driver instances.  Sure, it make sense to register the i2s and
> > codec drivers from probing on the i2s and codec nodes.  However, there
> > is nothing that prevents the codec driver from *also* registering a
> > fabric driver based on a property in the codec node or the board-level
> > compatible property.
>
> But there is something in the kernel that enforces it. I haven't
> checked the powerpc code, but the PCI code won't probe anymore drivers
> once one has attached to a device. The rule of one driver per device
> is a good one. Places where that rule has been broken have caused a
> lot of problems (fbdev vs DRM).

heh; there's isn't a 1:1 relationship between device tree nodes and
device objects either.  You create the device objects that you need;
either on the platform bus or the of_platform bus.

The probe of an of_platform device can trigger the creation of a
plaform_device node for *another* driver.  This approach is safe.

>
> > Fabric drivers are codec specific anyway.  It's not all that
> > unrealistic for the device tree binding for a codec to have a list of
> > fabric drivers that it can register.
>
> The codec drivers in asoc are completely agnostic. The same codec
> driver works on x86, arm, powerpc, etc.

Yes the *driver* is agnostic; but the *binding* doesn't have to be.  :-)

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:

> Calling it directly from the platform code is an option, but where
> does the fabric driver code live? It doesn't make a lot of sense to
> put ALSA code into arch/powerpc/platforms/52xx. I could make a
> function call from arch/powerpc/platforms/52xx over to
> sound/soc/powerpc but that's not very pretty.

sound/soc/fsl is where the non-codec ASoC drivers for Freescale parts should go.

> The codec drivers in asoc are completely agnostic. The same codec
> driver works on x86, arm, powerpc, etc.

Well, in theory at least.  I never tried my cs4270 driver on anything but PowerPC.

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Jon Smirl wrote:
>
> > Calling it directly from the platform code is an option, but where
> > does the fabric driver code live? It doesn't make a lot of sense to
> > put ALSA code into arch/powerpc/platforms/52xx. I could make a
> > function call from arch/powerpc/platforms/52xx over to
> > sound/soc/powerpc but that's not very pretty.
>
> sound/soc/fsl is where the non-codec ASoC drivers for Freescale parts should go.

Why are you using a vendor named directory? I don't believe vendor
named directories are used anywhere in the kernel. The directories are
always named after the platform or architecture. Vendor directories
end up in a big mess if Freescale decides to sell a CPU to someone
else.


>
> > The codec drivers in asoc are completely agnostic. The same codec
> > driver works on x86, arm, powerpc, etc.
>
> Well, in theory at least.  I never tried my cs4270 driver on anything but PowerPC.
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:

> Why are you using a vendor named directory? I don't believe vendor
> named directories are used anywhere in the kernel. The directories are
> always named after the platform or architecture. Vendor directories
> end up in a big mess if Freescale decides to sell a CPU to someone
> else.

Two reasons:

1) The sound/soc directory already has names like "at91" and "pxa", so I thought 
"fsl" is appropriate.

2) There may not be any directories named "fsl", but there are plenty of files 
with that name:

./arch/powerpc/boot/fsl-soc.c
./arch/powerpc/boot/fsl-soc.h
./arch/powerpc/boot/fsl-soc.o
./arch/powerpc/mm/fsl_booke_mmu.c
./arch/powerpc/platforms/fsl_uli1575.c
./arch/powerpc/sysdev/fsl_pci.c
./arch/powerpc/sysdev/fsl_pci.h
./arch/powerpc/sysdev/fsl_soc.c
./arch/powerpc/sysdev/fsl_soc.h
./arch/powerpc/sysdev/fsl_soc.o
./arch/ppc/mm/fsl_booke_mmu.c
./drivers/usb/gadget/fsl_usb2_udc.c
./drivers/usb/gadget/fsl_usb2_udc.h
./include/linux/fsl_devices.h
./include/config/fsl

Having said all that, if you really think sound/soc/powerpc is better than 
sound/soc/fsl, I won't complain.

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Jon Smirl wrote:
>
> > Why are you using a vendor named directory? I don't believe vendor
> > named directories are used anywhere in the kernel. The directories are
> > always named after the platform or architecture. Vendor directories
> > end up in a big mess if Freescale decides to sell a CPU to someone
> > else.
>
> Two reasons:
>
> 1) The sound/soc directory already has names like "at91" and "pxa", so I thought
> "fsl" is appropriate.

pxa is the processor family. Intel sold the pxa cpus to Marvell six months ago.

>
> 2) There may not be any directories named "fsl", but there are plenty of files
> with that name:
>
> ./arch/powerpc/boot/fsl-soc.c
> ./arch/powerpc/boot/fsl-soc.h
> ./arch/powerpc/boot/fsl-soc.o
> ./arch/powerpc/mm/fsl_booke_mmu.c
> ./arch/powerpc/platforms/fsl_uli1575.c
> ./arch/powerpc/sysdev/fsl_pci.c
> ./arch/powerpc/sysdev/fsl_pci.h
> ./arch/powerpc/sysdev/fsl_soc.c
> ./arch/powerpc/sysdev/fsl_soc.h
> ./arch/powerpc/sysdev/fsl_soc.o
> ./arch/ppc/mm/fsl_booke_mmu.c
> ./drivers/usb/gadget/fsl_usb2_udc.c
> ./drivers/usb/gadget/fsl_usb2_udc.h
> ./include/linux/fsl_devices.h
> ./include/config/fsl
>
> Having said all that, if you really think sound/soc/powerpc is better than
> sound/soc/fsl, I won't complain.
>
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[David Gibson]

On Wed, Oct 24, 2007 at 10:38:11AM -0600, Grant Likely wrote:
> On 10/24/07, Jon Smirl <jonsmirl@gmail.com> wrote:
> > On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
[snip]
> > > For example:
> > > sound@0 {
> > >       compatible = "<mfg>,<board>,sound"   // The board might have
> > > more than one sound i/f which could be wired differently
> > >       codec-handle = <&codec0>;
> > > };
> 
> The difference here is that the node provides real information about
> the board.  It has a compatible field which tells you *exactly* what
> sound circuit is present on the board.  It can have additional
> information that does make sense to encode into the device tree (ie.
> the codec that is used).  It's not addressable (no registers or
> anything), but it does describe the board.
> 
> It would be possible and reasonable for a single fabric driver to work
> with many different circuit layouts as long as it has the information
> needed to adapt each instance.

This still seems nasty, since it seems to do little but duplicate the
platform information.

I'm afraid I still don't understand quite what information this
"fabric" driver is conveying.  Is it really inherently platform
specific, or is it something that can be encoded directly in a
sensible way.  If the latter we could have a general "device tree"
fabric driver that will handle all systems with the layout correctly
encoded in the device tree.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> I'm afraid I still don't understand quite what information this
> "fabric" driver is conveying.  Is it really inherently platform
> specific, or is it something that can be encoded directly in a
> sensible way.  If the latter we could have a general "device tree"
> fabric driver that will handle all systems with the layout correctly
> encoded in the device tree.

Codecs are like GPIOs, all of their pins are programmable. So the same
codec can be wired into various boards quite differently and then
software programmed to work the same. The fabric driver contains the
mapping information.

People were making a codec driver for each board, but this resulted in
lots of similar codec drivers for the same chips. I believe a common
Wolfson chip had eight drivers in the kernel. In the new model the
codec drivers are generic and the fabric driver describes the mapping.

A side effect of this is that we need to load the fabric driver which
doesn't have a device associated with it.

This is a complex mapping from a STAC9277 used with an Intel hda chip.

Codec: SigmaTel STAC9227
Address: 2
Vendor Id: 0x83847618
Subsystem Id: 0x102801db
Revision Id: 0x100201
No Modem Function Group found
Default PCM:
    rates [0x7e0]: 44100 48000 88200 96000 176400 192000
    bits [0xe]: 16 20 24
    formats [0x1]: PCM
Default Amp-In caps: ofs=0x00, nsteps=0x0e, stepsize=0x05, mute=0
Default Amp-Out caps: ofs=0x7f, nsteps=0x7f, stepsize=0x02, mute=1
Node 0x02 [Audio Output] wcaps 0xd0c05: Stereo Amp-Out
  Amp-Out caps: N/A
  Amp-Out vals:  [0x62 0x62]
  Power: 0x0
Node 0x03 [Audio Output] wcaps 0xd0c05: Stereo Amp-Out
  Amp-Out caps: N/A
  Amp-Out vals:  [0xe4 0xe4]
  Power: 0x0
Node 0x04 [Audio Output] wcaps 0xd0c05: Stereo Amp-Out
  Amp-Out caps: N/A
  Amp-Out vals:  [0x62 0x62]
  Power: 0x0
Node 0x05 [Audio Output] wcaps 0xd0c05: Stereo Amp-Out
  Amp-Out caps: N/A
  Amp-Out vals:  [0xe4 0xe4]
  Power: 0x0
Node 0x06 [Vendor Defined Widget] wcaps 0xfd0c05: Stereo Amp-Out
  Amp-Out caps: N/A
  Amp-Out vals:  [0xff 0xff]
  Power: 0x0
Node 0x07 [Audio Input] wcaps 0x1d0541: Stereo
  Power: 0x0
  Connection: 1
     0x1b
Node 0x08 [Audio Input] wcaps 0x1d0541: Stereo
  Power: 0x0
  Connection: 1
     0x1c
Node 0x09 [Audio Input] wcaps 0x1d0541: Stereo
  Power: 0x0
  Connection: 1
     0x1d
Node 0x0a [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x08173f: IN OUT HP Detect
  Pin Default 0x0221101f: [Jack] HP Out at Ext Front
    Conn = 1/8, Color = Black
  Pin-ctls: 0xc0: OUT HP
  Connection: 2
     0x02* 0x03
Node 0x0b [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x08173f: IN OUT HP Detect
  Pin Default 0x02a11020: [Jack] Mic at Ext Front
    Conn = 1/8, Color = Black
  Pin-ctls: 0x24: IN
  Connection: 2
     0x02* 0x03
Node 0x0c [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x081737: IN OUT Detect
  Pin Default 0x01a19021: [Jack] Mic at Ext Rear
    Conn = 1/8, Color = Pink
  Pin-ctls: 0x24: IN
  Connection: 1
     0x03
Node 0x0d [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x08173f: IN OUT HP Detect
  Pin Default 0x01014010: [Jack] Line Out at Ext Rear
    Conn = 1/8, Color = Green
  Pin-ctls: 0x40: OUT
  Connection: 1
     0x02
Node 0x0e [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x081737: IN OUT Detect
  Pin Default 0x01011012: [Jack] Line Out at Ext Rear
    Conn = 1/8, Color = Black
  Pin-ctls: 0x40: OUT
  Connection: 1
     0x04
Node 0x0f [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x081737: IN OUT Detect
  Pin Default 0x01016011: [Jack] Line Out at Ext Rear
    Conn = 1/8, Color = Orange
  Pin-ctls: 0x40: OUT
  Connection: 1
     0x05
Node 0x10 [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x0837: IN OUT Detect
  Pin Default 0x0181302e: [Jack] Line In at Ext Rear
    Conn = 1/8, Color = Blue
  Pin-ctls: 0x20: IN
  Connection: 1
     0x04
Node 0x11 [Pin Complex] wcaps 0x400181: Stereo
  Pincap 0x0837: IN OUT Detect
  Pin Default 0x01012014: [Jack] Line Out at Ext Rear
    Conn = 1/8, Color = Grey
  Pin-ctls: 0x40: OUT
  Connection: 1
     0x03
Node 0x12 [Pin Complex] wcaps 0x400001: Stereo
  Pincap 0x0820: IN
  Pin Default 0x40f000f1: [N/A] Other at Ext N/A
    Conn = Unknown, Color = Unknown
  Pin-ctls: 0x00:
Node 0x13 [Vendor Defined Widget] wcaps 0xf00001: Stereo
Node 0x14 [Vendor Defined Widget] wcaps 0xf00001: Stereo
Node 0x15 [Audio Selector] wcaps 0x30010d: Stereo Amp-Out
  Amp-Out caps: ofs=0x00, nsteps=0x04, stepsize=0x27, mute=0
  Amp-Out vals:  [0x00 0x00]
  Connection: 9
     0x0e 0x12 0x0f 0x0b 0x0c* 0x0d 0x0a 0x10 0x11
Node 0x16 [Audio Selector] wcaps 0x30010d: Stereo Amp-Out
  Amp-Out caps: ofs=0x00, nsteps=0x04, stepsize=0x27, mute=0
  Amp-Out vals:  [0x00 0x00]
  Connection: 9
     0x0e 0x12 0x0f 0x0b 0x0c* 0x0d 0x0a 0x10 0x11
Node 0x17 [Audio Selector] wcaps 0x30010d: Stereo Amp-Out
  Amp-Out caps: ofs=0x00, nsteps=0x04, stepsize=0x27, mute=0
  Amp-Out vals:  [0x00 0x00]
  Connection: 9
     0x0e 0x12 0x0f 0x0b 0x0c* 0x0d 0x0a 0x10 0x11
Node 0x18 [Audio Selector] wcaps 0x300103: Stereo Amp-In
  Amp-In caps: N/A
  Amp-In vals:  [0x00 0x00]
  Connection: 1
     0x15
Node 0x19 [Audio Selector] wcaps 0x300103: Stereo Amp-In
  Amp-In caps: N/A
  Amp-In vals:  [0x00 0x00]
  Connection: 1
     0x16
Node 0x1a [Audio Selector] wcaps 0x300103: Stereo Amp-In
  Amp-In caps: N/A
  Amp-In vals:  [0x00 0x00]
  Connection: 1
     0x17
Node 0x1b [Audio Selector] wcaps 0x30090d: Stereo Amp-Out
  Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
  Amp-Out vals:  [0x80 0x80]
  Connection: 1
     0x18
Node 0x1c [Audio Selector] wcaps 0x30090d: Stereo Amp-Out
  Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
  Amp-Out vals:  [0x80 0x80]
  Connection: 1
     0x19
Node 0x1d [Audio Selector] wcaps 0x30090d: Stereo Amp-Out
  Amp-Out caps: ofs=0x00, nsteps=0x00, stepsize=0x00, mute=1
  Amp-Out vals:  [0x80 0x80]
  Connection: 1
     0x1a
Node 0x1e [Audio Output] wcaps 0x40211: Stereo Digital
  PCM:
    rates [0x7e0]: 44100 48000 88200 96000 176400 192000
    bits [0xe]: 16 20 24
    formats [0x5]: PCM AC3
Node 0x1f [Vendor Defined Widget] wcaps 0xf30201: Stereo Digital
Node 0x20 [Audio Input] wcaps 0x140311: Stereo Digital
  PCM:
    rates [0x160]: 44100 48000 96000
    bits [0xe]: 16 20 24
    formats [0x5]: PCM AC3
  Connection: 1
     0x22
Node 0x21 [Pin Complex] wcaps 0x400301: Stereo Digital
  Pincap 0x0810: OUT
  Pin Default 0x014510a0: [Jack] SPDIF Out at Ext Rear
    Conn = Optical, Color = Black
  Pin-ctls: 0x40: OUT
  Connection: 5
     0x1e* 0x1f 0x1b 0x1c 0x1d
Node 0x22 [Pin Complex] wcaps 0x430681: Stereo Digital
  Pincap 0x0810024: IN EAPD Detect
  Pin Default 0x40f000f0: [N/A] Other at Ext N/A
    Conn = Unknown, Color = Unknown
  Pin-ctls: 0x00:
  Power: 0x0
Node 0x23 [Beep Generator Widget] wcaps 0x70000c: Mono Amp-Out
  Amp-Out caps: ofs=0x03, nsteps=0x03, stepsize=0x17, mute=0
  Amp-Out vals:  [0x00]
Node 0x24 [Volume Knob Widget] wcaps 0x600000: Mono



-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[David Gibson]

On Wed, Oct 24, 2007 at 08:17:57PM -0400, Jon Smirl wrote:
> On 10/24/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> > I'm afraid I still don't understand quite what information this
> > "fabric" driver is conveying.  Is it really inherently platform
> > specific, or is it something that can be encoded directly in a
> > sensible way.  If the latter we could have a general "device tree"
> > fabric driver that will handle all systems with the layout correctly
> > encoded in the device tree.
> 
> Codecs are like GPIOs, all of their pins are programmable. So the same
> codec can be wired into various boards quite differently and then
> software programmed to work the same. The fabric driver contains the
> mapping information.
> 
> People were making a codec driver for each board, but this resulted in
> lots of similar codec drivers for the same chips. I believe a common
> Wolfson chip had eight drivers in the kernel. In the new model the
> codec drivers are generic and the fabric driver describes the mapping.

Ok, but the fabric driver is about the wiring of a specific codec
chip, yes?  If a board was foolishly designed to have two identical
codec chips, but each wired differently, it would need two instances
of the same codec driver, plus one instance each of two different
fabric drivers?

If this is so, then the fabric information *must* be per-codec, and
should therefore go with the codec node.

> A side effect of this is that we need to load the fabric driver which
> doesn't have a device associated with it.

Well, it does have a device associated with it, it's just a question
of how to represent it.  There's not reason a single device node can't
cause instantiation of multiple driver instances.  Depending on the
complexity of typical fabric arrangements, one of the following
options might make sense:
	- the device node's compatible has enough information to
specify both fabric and codec driver.  The fabric driver is
instantiated from this node, and instantiates the codec driver itself
(since I gather fabric drivers are frequently codec specific in any
case).
	- both fabric and codec drivers are instantiated from the same
device node, and co-ordinate with each other.
	- The codec is represented as:
codec-fabric@... {
	compatible = "...";
	<properties describing the fabric>
	codec {
	      compatible = "...";
	      <properties describing the codec>
	}
}

This is different from a "pseudo" node, because the codec-fabric node
represents a real piece of hardware:  specifically the cluster of
wires between the sound bus and the codec.

Remember: the device tree describes the hardware, how the chooses to
structure its driver model to meet the demands of that hardware is up
to it.  Don't put the cart before the horse.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> On Wed, Oct 24, 2007 at 08:17:57PM -0400, Jon Smirl wrote:
> > On 10/24/07, David Gibson <david@gibson.dropbear.id.au> wrote:
> > > I'm afraid I still don't understand quite what information this
> > > "fabric" driver is conveying.  Is it really inherently platform
> > > specific, or is it something that can be encoded directly in a
> > > sensible way.  If the latter we could have a general "device tree"
> > > fabric driver that will handle all systems with the layout correctly
> > > encoded in the device tree.
> >
> > Codecs are like GPIOs, all of their pins are programmable. So the same
> > codec can be wired into various boards quite differently and then
> > software programmed to work the same. The fabric driver contains the
> > mapping information.
> >
> > People were making a codec driver for each board, but this resulted in
> > lots of similar codec drivers for the same chips. I believe a common
> > Wolfson chip had eight drivers in the kernel. In the new model the
> > codec drivers are generic and the fabric driver describes the mapping.
>
> Ok, but the fabric driver is about the wiring of a specific codec
> chip, yes?  If a board was foolishly designed to have two identical
> codec chips, but each wired differently, it would need two instances
> of the same codec driver, plus one instance each of two different
> fabric drivers?

AFAIK no one has built that case. My target board has two different
codec chips. I was handling them both in a single fabric driver but
there is no reason the code couldn't be split.

I was thinking that there was a single fabric for the board, but you
are right in observing that it is per codec chip.

The term fabric is coming from the Apple aoa driver. They only have a
single fabric per board. But there is no reason the Apple driver
couldn't also be adjusted.

> If this is so, then the fabric information *must* be per-codec, and
> should therefore go with the codec node.
>
> > A side effect of this is that we need to load the fabric driver which
> > doesn't have a device associated with it.
>
> Well, it does have a device associated with it, it's just a question
> of how to represent it.  There's not reason a single device node can't
> cause instantiation of multiple driver instances.  Depending on the
> complexity of typical fabric arrangements, one of the following
> options might make sense:
>         - the device node's compatible has enough information to
> specify both fabric and codec driver.  The fabric driver is
> instantiated from this node, and instantiates the codec driver itself
> (since I gather fabric drivers are frequently codec specific in any
> case).

This could work. The generic codec is a alsa soc_device_driver, not a
of_device_driver. The codec node could instantiate the fabric as a
of_device_driver which could then instantiate the soc_device_driver
for the generic codec.

The generic codecs are supposed to work cross platform so they can't
include code that munges the of device tree.


>         - both fabric and codec drivers are instantiated from the same
> device node, and co-ordinate with each other.
>         - The codec is represented as:
> codec-fabric@... {
>         compatible = "...";
>         <properties describing the fabric>
>         codec {
>               compatible = "...";
>               <properties describing the codec>
>         }
> }
>
> This is different from a "pseudo" node, because the codec-fabric node
> represents a real piece of hardware:  specifically the cluster of
> wires between the sound bus and the codec.
>
> Remember: the device tree describes the hardware, how the chooses to
> structure its driver model to meet the demands of that hardware is up
> to it.  Don't put the cart before the horse.
>
> --
> David Gibson                    | I'll have my music baroque, and my code
> david AT gibson.dropbear.id.au  | minimalist, thank you.  NOT _the_ _other_
>                                 | _way_ _around_!
> http://www.ozlabs.org/~dgibson
>


-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:

> This could work. The generic codec is a alsa soc_device_driver, not a
> of_device_driver. The codec node could instantiate the fabric as a
> of_device_driver which could then instantiate the soc_device_driver
> for the generic codec.
> 
> The generic codecs are supposed to work cross platform so they can't
> include code that munges the of device tree.

My understanding of ASoC is that the fabric driver is supposed to be OF-aware, 
and the codec and other drivers are not.  The other drivers have ASoC entry 
points that the fabric driver calls to provide information.

Example:

Fabric driver:

static int mpc8610hpcd_startup(struct snd_pcm_substream *substream)
{
	...
	if (codec_dai->dai_ops.set_fmt) {
		ret = codec_dai->dai_ops.set_fmt(codec_dai, machine_data->dai_format);
		if (ret < 0) {
			printk(KERN_ERR "mpc8610-hpcd: could not set codec "
				"driver audio format (ret=%i)\n", ret);
			return ret;
		}
	}

codec driver:

static int cs4270_set_dai_fmt(struct snd_soc_codec_dai *codec_dai,
			      unsigned int format)
{
	...

	switch (format & SND_SOC_DAIFMT_FORMAT_MASK) {
	case SND_SOC_DAIFMT_I2S:
	case SND_SOC_DAIFMT_LEFT_J:
		cs4270->mode = format & SND_SOC_DAIFMT_FORMAT_MASK;
		break;


In my device tree, the I2S node specifies the interface (this node layout is 
what I came up with 6 months ago when I started working on this.)

		ssi@16000 {
			compatible = "fsl,ssi";
			reg = <16000 100>;
			interrupt-parent = <&mpic>;
			interrupts = <3e 2>;
			/* This is probably not the right way to tell the
			   SSI driver how to configure the interface */
			fsl,mode = "i2s-slave";
			codec {
				compatible = "cs4270";
			};
		};


The fabric driver reads "i2s-slave" and converts that to SND_SOC_DAIFMT_I2S.

Re: Audio codec device tree entries

[David Gibson]

On Wed, Oct 24, 2007 at 11:54:23AM -0400, Jon Smirl wrote:
> On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > > Do you want to pick one and add it to the device tree documentation
> > > with an example for i2s and ac97? I'll use which ever one is picked.
> >
> > Sure, I'll draft something up and post it for review.
> >
> > On the device probing front; what about this method:
> >
> > Rather than trying to figure things out from the board model, or the
> > combination of the codec and i2s bus; add another node to represent
> > the sound circuit.  All that node would need is a unique compatible
> > property and a phandle to either the i2s bus or the codec (depending
> > on which binding approach is used).  It could have additional
> > properties to represent optional features, etc.
> 
> That's the pseudo-sound node proposal that other people objected to.
> 
> It makes sense to me, there needs to be some way to trigger loading
> the fabric driver.
> 
> >
> > For example:
> > sound@0 {
> >       compatible = "<mfg>,<board>,sound"   // The board might have
> > more than one sound i/f which could be wired differently
> >       codec-handle = <&codec0>;
> > };
> 
> Do you even need the parameters,  how about simply this?
> 
> sound-fabric {
> };
> 
> That will trigger loading all of the sound-fabric drivers built into
> the kernel. In their probe functions they can look in the device tree
> and extract the machine name and then decide to stay loaded or fail
> the probe.

We shouldn't be basing driver configuration on the machine name unless
we really have to.  We should be able to find a sane way to encode the
necessary information in the tree proper.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > The DTC experts need to tell us which way to make the pointers between
> > i2s and i2c for the codec.  Here's a another way it could be done that
> > looks more like the ac97 model.
>
> I *really* don't think this is a good idea.  Put the node on the bus
> that the device is addressed from.  I2S is the *data* path, not the
> *control* path, but you cannot control the codec from there.
>
> Your suggestion only looks more like the AC97 model if you're looking
> at the data path.  If you're looking at the control path it looks
> completely different.  The device tree convention is to orient around
> the control path.

It doesn't make any difference to me which one we pick. I'm just
listing all of the possible combinations. I just want to pick a model
so that we can write the code.

I see your point about putting the child node onto the control bus.
ac97 is both a control and data bus. For the i2s case the child should
go onto the i2c bus.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[Timur Tabi]

Jon Smirl wrote:

> I see your point about putting the child node onto the control bus.
> ac97 is both a control and data bus. For the i2s case the child should
> go onto the i2c bus.

I know AC97 is *also* a control bus, but treating I2S and AC97 differently is 
bad, IMHO.  If you're going to put the child node in the AC97 node, you should 
also put it in the I2S node.

The 8610 has an SSI that can operate as either AC97 or I2S.  If I want to switch 
from AC97 to I2S, I should not have to move the child node out of the AC97 node. 
  I should instead just add an I2C node and point to it.

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> Jon Smirl wrote:
>
> > I see your point about putting the child node onto the control bus.
> > ac97 is both a control and data bus. For the i2s case the child should
> > go onto the i2c bus.
>
> I know AC97 is *also* a control bus, but treating I2S and AC97 differently is
> bad, IMHO.  If you're going to put the child node in the AC97 node, you should
> also put it in the I2S node.

They *are* different.  The choice you're making is whether or not you
keep them similar in the control path or the data path; but you still
have to choose.

>
> The 8610 has an SSI that can operate as either AC97 or I2S.  If I want to switch
> from AC97 to I2S, I should not have to move the child node out of the AC97 node.
>   I should instead just add an I2C node and point to it.

But you need a different codec node regardless.  The board/system will
in the vast majority of cases designed to only use AC97 or only use
I2S.  It's not moving a node.  It's deleting an ac97 codec node and
adding an i2s codec node.

Besides; correctness is more important that how many device tree
changes need to be made to go from one board design to another.

Cheers,
g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Grant Likely]

On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> >       codec0: i2s-codec@0 {
> >             compatible = "ti,tas5508";
> >             reg = <0>;
> >             i2s-handle = <&i2s@2000>;
> >       };
>
> I'd do this the other way around -- that is:
>
> i2s@2200 {           // PSC2
>         compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
>         ...
>         i2c-handle = <&codec0>;  /* Or something like that */

i2c-handle is a poor property name here.  It should be 'codec-handle'.
 The codec could theoretically live on just about *any* control bus;
not just i2c.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.
grant.likely@secretlab.ca
(403) 399-0195

Re: Audio codec device tree entries

[Jon Smirl]

On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> > >       codec0: i2s-codec@0 {
> > >             compatible = "ti,tas5508";
> > >             reg = <0>;
> > >             i2s-handle = <&i2s@2000>;
> > >       };
> >
> > I'd do this the other way around -- that is:
> >
> > i2s@2200 {           // PSC2
> >         compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
> >         ...
> >         i2c-handle = <&codec0>;  /* Or something like that */
>
> i2c-handle is a poor property name here.  It should be 'codec-handle'.
>  The codec could theoretically live on just about *any* control bus;
> not just i2c.

That's one of the reasons I put the second option in the post.

In the second option the i2s driver would instantiate first. Next the
generic code would get loaded. The generic codec will know the control
but for the device and it can go look in the i2c node for the address.
i2c node still lists all of the devices on the i2c bus. But the codecs
are in the i2c-handle property so they don't trigger a second loading
of the codec.

A fundamental question is, which bus should trigger the loading of the
generic codec driver. The answer to this determines how the device
tree should look.

I'm using the model that a child node means load the driver and a
reference does not load a driver.

-- 
Jon Smirl
jonsmirl@gmail.com

Re: Audio codec device tree entries

[David Gibson]

On Wed, Oct 24, 2007 at 11:19:33AM -0400, Jon Smirl wrote:
> On 10/24/07, Grant Likely <grant.likely@secretlab.ca> wrote:
> > On 10/24/07, Timur Tabi <timur@freescale.com> wrote:
> > > >       codec0: i2s-codec@0 {
> > > >             compatible = "ti,tas5508";
> > > >             reg = <0>;
> > > >             i2s-handle = <&i2s@2000>;
> > > >       };
> > >
> > > I'd do this the other way around -- that is:
> > >
> > > i2s@2200 {           // PSC2
> > >         compatible = "fsl,mpc5200b-psc-i2s","fsl,mpc5200-psc-i2s";
> > >         ...
> > >         i2c-handle = <&codec0>;  /* Or something like that */
> >
> > i2c-handle is a poor property name here.  It should be 'codec-handle'.
> >  The codec could theoretically live on just about *any* control bus;
> > not just i2c.
> 
> That's one of the reasons I put the second option in the post.
> 
> In the second option the i2s driver would instantiate first. Next the
> generic code would get loaded. The generic codec will know the control
> but for the device and it can go look in the i2c node for the address.
> i2c node still lists all of the devices on the i2c bus. But the codecs
> are in the i2c-handle property so they don't trigger a second loading
> of the codec.
> 
> A fundamental question is, which bus should trigger the loading of the
> generic codec driver. The answer to this determines how the device
> tree should look.

No!  Device tree layout should not be determined by the instantiation
model used by Linux drivers right now.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson