[alsa-devel] [RFC][PATCH 2/2 v2] ASoC: simple-card: add Device Tree support

[Stephen Warren]

On 01/29/2013 03:00 AM, Kuninori Morimoto wrote:
> 
> Hi Mark, Stephen
> 
>>> 	simple-audio,codec {
>>> 		simple-audio,dev = &phandle;
>>> 		simple-audio,system-clock-frequency = 122880000;
>>> 	};
> 
> I would like to ask you before creating v3 patch.
> I got some opinions from you.
>  - cpu/codec style (above)
>  - "controller" naming
>  - it is using "name" property (it is must item for matching)
> 
> Then, what do you think about this style ?
> 
> fai_ak4642 {

The node would usually be named after the type of object, not the
identity of the object, so more like "sound".

>         compatible = "simple-audio";
> 
>         simple-audio,card-name = "FSI2A-AK4648";

OK.

>         /* platform */
>         simple-audio,platform {

I still don't understand why you need to even represent the platform in DT.

Introducing sub-nodes seems to just add to the complexity without much gain.

>              compatible = "ak4642-hifi";  /* platform name (if needed) */

The "compatible" property has a very specific meaning in DT already. I
don't think we should re-use that property name for other purposes.

>              simple-audio,dev = <&sh_fsi2>;


>              /* format and inversion are here */
>              simple-audio,format = "left_j";
>              simple-audio,bitclock-inversion;

Those properties appear more relevant to the I2S link than the ASoC
platform object.

>         }
> 
>         /* codec and cpu are same style */
> 
>         /* codec */
>         simple-audio,codec {
>              compatible = "ak4642-hifi";  /* codec dai name (this is must item on ASoC) */

If you absolutely must use named-based binding here rather than the
standard phandle+args style, rename that property to something more like
simple-audio,dai-name.

>              simple-audio,dev = <&ak4648>;
>              simple-audio,bitclock-master;
>  
>              /* clock gating is here */
>             simple-audio,clock-gating = "continuous";

I think this should be a Boolean property; present means yes, missing
means no.

>         }
> 
>         /* cpu */
>         simple-audio,cpu {
>              compatible = "fsia-dai"; /* cpu dai name (if needed) */
>              simple-audio,dev = <&sh_fsi2>;
>              simple-audio,system-clock-frequency = <11289600>;
>         }
> }