Re: [PATCH 5/5] ASoC: simple-card: add Device Tree support

[Stephen Warren <swarren@wwwdotorg.org>]

On 12/25/2012 11:53 PM, Kuninori Morimoto wrote:
> Support for loading the simple-card module via devicetree.
> It requests platform/codec driver's DT blob for probing.

This binding proposal should be sent to
devicetree-discuss@lists.ozlabs.org; that's where all DT bindings are
dicussed.

> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt

> +Required properties:
> +for simple-card
> +- compatible			: "asoc,simple-card"

I don't think "asoc," is a good vendor prefix; DT should strive to
describe the hardware, not a particular OS's driver structure.

Perhaps "simple-audio" would be better.

> +- simple,asoc,name		: simple card name

What is "simple,asoc,"? I've never seen properties with two vendor
prefixes before... Perhaps "simple-audio," would be better.

What kind of name? Perhaps "simple-audio,model" would be better, and
more consistent with the use of the word "model" in other audio-related
DT bindings? Describe it as "user-visible card name"?

> +- simple,asoc,cpu		: phandle for platform

"cpu" isn't a very good name; it couuld mean anything. "platform" is an
ASoC-specific term. I think this would be better written as:

simple-audio,cpu-audio-controller : phandle of the CPU's audio controller

or perhaps:

simple-audio,cpu-port : phandle of the CPU's audio port

or since I assume this binding assumes I2S rather than say AC'97 or
SlimBus, perhaps:

simple-audio,i2c-controller : phandle of the CPU's I2S controller

> +- simple,asoc,codec		: phandle for codec

CODEC should be capitalized in the plain-text description (but not DT
property name). Same for DAI below.

> +for platform
> +- simple,asoc,dai		: dai name
> +
> +for codec
> +- simple,asoc,dai		: dai name
> +- simple,asoc,name		: codec name

Why are names needed? The existing audio-related DT bindings don't put
any names into the device tree; everything binds with phandles.

> +both platform/codec
> +- simple,asoc,daifmt,fmt		: see snd_soc_of_parse_daifmt()
> +- simple,asoc,daifmt,clock		: see snd_soc_of_parse_daifmt()
> +- simple,asoc,daifmt,bitclock_inversion	: see snd_soc_of_parse_daifmt()
> +- simple,asoc,daifmt,bitclock_master	: see snd_soc_of_parse_daifmt()
> +- simple,asoc,daifmt,frame_inversion	: see snd_soc_of_parse_daifmt()
> +- simple,asoc,daifmt,frame_master	: see snd_soc_of_parse_daifmt()

DT bindings should be self-contained, i.e. they should describe
everything required to write the device tree (with the possible
exception of referring to other existing manuals or documentation for
the HW). In particular, they shouldn't refer to code in some specific
OS. As such, please explain what all those mean here.

It is more typical to use - not _ as a word separator in DT property names.

"bitclock_master" doesn't really describe who is the master. Perhaps
"cpu-bitclk-master"?

What is "clock"?

> +- simple,asoc,sysclk			: system clock rate

"clock-frequency" rather than "sysclk" would be more consistent with
other bindings.

> +Example:
> +
> +fsi_ak4642 {
> +	compatible = "asoc,simple-card";
> +
> +	simple,asoc,name = "FSI2A-AK4648";
> +	simple,asoc,cpu = <&sh_fsi2>;
> +	simple,asoc,codec = <&ak4648>;
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +
> +		simple,asoc,dai = "ak4642-hifi";
> +		simple,asoc,name = "ak4642-codec.0-0012";
> +		simple,asoc,daifmt,fmt = "left_j";
> +		simple,asoc,daifmt,bitclock_master;
> +		simple,asoc,daifmt,frame_master;
> +		simple,asoc,sysclk = <11289600>;
> +	};
> +};
> +
> +&sh_fsi2 {
> +	simple,asoc,dai = "fsia-dai";
> +	simple,asoc,daifmt,fmt = "left_j";
> +};

Oh, I see. Nothing in the text above implies that the DAI format
properties would exist in the CODEC and I2S controller nodes. That's
rather an imposition on the DT bindings for the CODEC and I2S
controller; the individual bindings for those devices should define all
the properties that go into those nodes, and those devices won't always
be used with a "simple" sound card. It feels to me like the DAI
configuration properties should be part of the sound node, not the
I2S/CODEC nodes.