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

[Lars-Peter Clausen <lars@metafoo.de>]

On 02/14/2013 10:24 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.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v5 -> v6
> 
>  - cpu/codec become sub-node
>  - it is using new .of_xlate_dai_name based
>    snd_soc_of_get_port_dai_name()
>  - update Documentation
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   75 +++++++++++++
>  sound/soc/generic/simple-card.c                    |  118 +++++++++++++++++++-
>  2 files changed, 188 insertions(+), 5 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/sound/simple-card.txt
> 
> diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
> new file mode 100644
> index 0000000..86e0d9f
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,75 @@
> +Simple-Card:
> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name
> +- simple-audio,format			: see below
> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below
> +
> +Optional properties:
> +
> +- simple-audio,system-clock-frequency	: system clock rate if it is connected to both CPU/CODEC
> +- simple-audio,bitclock-inversion	: bit clock inversion for both CPU/CODEC
> +- simple-audio,frame-inversion		: frame inversion for both CPU/CODEC
> +
> +Required cpu/codec subnode properties:
> +
> +- simple-audio,dev			: phandle and port for CPU/CODEC
> +- simple-audio,frame-master		: frame master
> +- simple-audio,bitclock-master		: bitclock master
> +#sound-dai-cells integer is required on simple-audio,dev phandle's node

Shouldn't the names of '#sound-dai-cells' and 'simple-audio,dev' kind of
match? E.g. '#sound-dai-cells' and 'sound-dai'. Maybe drop the sound, since
the a in dai kind of implies this.

[...]
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 6cf8355..e50e415 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,7 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +53,108 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static struct device_node*
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    struct asoc_simple_dai *dai)
> +{
> +	struct device_node *node;
> +	char prop[128];
> +
> +	/* get "simple-audio,dev = <&phandle port>" */
> +	snprintf(prop, sizeof(prop), "simple-audio,dev");

Why do you need the extra buffer? Can you just pass, "simple-audio,dev"
directly to of_parse_phandle?

> +	node = of_parse_phandle(np, prop, 0);
> +	if (!node)
> +		return NULL;
> +
> +	of_node_put(node);

You shouldn't drop the reference until you are done processing it. Which in
this case is only after the device has been unregistered, since you pass the
node on to the ASoC core.

> +
> +	/* get dai-name  */
> +	dai->name = snd_soc_of_get_port_dai_name(np, prop);
> +
> +	/* get dai specific format */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, "simple-audio,");
> +
> +	/* get "simple-audio,system-clock-frequency = <xxx>" */
> +	snprintf(prop, sizeof(prop), "simple-audio,system-clock-frequency");
> +	of_property_read_u32(np, prop, &dai->sysclk);
> +
> +	return node;
> +}