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

[Stephen Warren <swarren@wwwdotorg.org>]

On 02/25/2013 01:56 AM, Kuninori Morimoto wrote:
> Support for loading the simple-card module via devicetree.
> It requests cpu/codec information,
> and .of_xlate_dai_name support on each driver for probing.

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


> +Optional properties:
...
> +- simple-audio,bitclock-inversion	: bit clock inversion for both CPU/CODEC
> +- simple-audio,frame-inversion		: frame inversion for both CPU/CODEC

I think that Mark pointed out those would be best stored in the CPU and
CODEC child nodes, since random buffering/inverting on the board might
make the values at the two DAIs the opposite of each-other.


> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC

I would be inclined to rename that simply "dai". "sound" is implicit
since the property is part of a sound-related node. Still, this isn't a
big deal.

> +- #sound-dai-cells			: sound-dai phandle's node

That property isn't part of the sound card's CPU/CODEC subnode, but
rather part of the CPU or CODEC that's referred to by the phandle.

This document needs to describe 3 sets of nodes:

1) The sound node itself.

2) The CPU and CODEC child nodes of the sound node.

3) The nodes representing the CPU and CODEC HW devices. It is these that
will contain #sound-dai-cells.

See for example how Documentation/devicetree/bindings/gpio/gpio.txt
describes each of these 3 cases separately.

Note that I think #sound-dai-cells is the correct name for this property
(and not #dai-cells), since the nodes they will appear in are more
generic HW device nodes, and not a dedicated sound-card node.

> +simple-audio,format
> +	"i2s"
> +	"right_j"
> +	"left_j"
> +	"dsp_a"
> +	"dsp_b"
> +	"ac97"
> +	"pdm"
> +	"msb"
> +	"lsb"

Now that we have a C pre-processor for device tree, I would be inclined
to use an integer for that property, rather than strings.

> +Example:

Overall, the example looks very much like what I hoped.

> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c

> +static int
> +__asoc_simple_card_parse_of(struct device_node *np,

__ is a bit odd. Rename the function to asoc_simple_card_parse_of_dai()
to avoid it having the same name as asoc_simple_card_parse_of() below,
once you remove the __.

> +	/* get "simple-audio,dev = <&phandle port>" */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;

I definitely think snd_soc_of_get_*_dai_name() should return this.
Otherwise, you're just duplicating work that happens inside there.

> +	of_node_put(*node);
> +
> +	/* get dai-name  */
> +	if (for_cpu)
> +		ret = snd_soc_of_get_cpu_dai_name(np, &dai->name);
> +	else
> +		ret = snd_soc_of_get_codec_dai_name(np, &dai->name);

Why can't there be a single function that returns both the target DT
node and the DAI name. It should simply search both the list of CPUs and
list of CODECs until a match is found. That way, machine drivers won't
need this "for_cpu" condition.

> +static int asoc_simple_card_parse_of(struct device_node *node,
> +				     struct asoc_simple_card_info *info,
> +				     struct device *dev,
> +				     struct device_node **of_cpu,
> +				     struct device_node **of_codec,
> +				     struct device_node **of_platform)
> +{
> +	struct device_node *np;
> +	u32 sysclk = 0;
> +	int ret = 0;
> +
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	info->name = info->card;
> +
> +	/* cpu/codec common daifmt */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* cpu/codec common clock */
> +	of_property_read_u32(node,
> +			"simple-audio,system-clock-frequency", &sysclk);
> +	info->cpu_dai.sysclk = sysclk;
> +	info->codec_dai.sysclk = sysclk;
> +
> +	/* cpu/codec sub-node */
> +	for_each_child_of_node(node, np) {

What if there are 3 nodes because the DT author put random cruft in there.

I think you want to use of_find_node_by_name() instead, to look up a
specific node name.

> +		if (0 == strcmp("simple-audio,cpu", np->name))

s/0 ==/!/. (and I'm sure others disagree, but I rather dislike comparing
a constant to see if it has a particular value, rather than comparing a
value to the constant that you care if it matches or not).

>  static int asoc_simple_card_probe(struct platform_device *pdev)
...
> +	struct asoc_simple_card_info *cinfo;
...
> +	if (np && of_device_is_available(np)) {
> +		cinfo = devm_kzalloc(dev, sizeof(*cinfo), GFP_KERNEL);
> +		if (cinfo) {
> +			int ret;
> +			ret = asoc_simple_card_parse_of(np, cinfo, dev,
> +							&of_cpu,
> +							&of_codec,
> +							&of_platform);
...
> +	cinfo->snd_link.cpu_of_node	= of_cpu;
> +	cinfo->snd_link.codec_of_node	= of_codec;
> +	cinfo->snd_link.platform_of_node = of_platform;

Why not just have asoc_simple_card_parse_of() directly fill in those
values, rather than writing them to temporary variables only to copy
them in here anyway. You pass cinfo to the function, so it could easily
do this.