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

[Mark Rutland <mark.rutland@arm.com>]

Hi,

On Fri, Oct 04, 2013 at 01:04:41AM +0100, 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 component driver.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> ---
> v1 -> v2
> 
>  - add common clock support, system-clock-frequency can over-write it
>  - add some comment on code
> 
> v2 -> v3
> 
>  - add devicetree ML address
> 
>  .../devicetree/bindings/sound/simple-card.txt      |   85 ++++++++++++
>  sound/soc/generic/simple-card.c                    |  146 +++++++++++++++++++-
>  2 files changed, 226 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..75bbc5a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/sound/simple-card.txt
> @@ -0,0 +1,85 @@
> +Simple-Card:

It's really difficult to review this without a description. Please explain what
this binding represents.

What hardware is this applicable to?

Does this inherit from some class of binding?

> +
> +Required properties:
> +
> +- compatible				: "simple-audio"
> +- simple-audio,card-name		: simple-audio card name

What's this used for?

> +- simple-audio,cpu			: CPU sub-node, see below
> +- simple-audio,codec			: CODEC sub-node, see below

Describe these as required subnodes. Nodes are not properties.

> +
> +Optional properties:
> +
> +- simple-audio,format			: CPU/CODEC common format, see below
> +
> +Required cpu/codec subnode properties:
> +
> +- sound-dai				: phandle and port for CPU/CODEC
> +- #sound-dai-cells			: sound-dai phandle's node

This description makes no sense, the organisation seems structurally wrong.

What does this mean? What does it affect?

> +
> +Optional subnode properties:
> +
> +- format				: specific format if needed, see below
> +- frame-master				: frame master
> +- bitclock-master			: bitclock master
> +- bitclock-inversion			: clock inversion
> +- frame-inversion			: frame inversion

What do these mean? Repeating the name without a dash is completely unhelpful.
Describe what these imply.

What type are they?

Which subnode(s) do they apply to?

> +- clocks				: phandle for system clock rate

Just one clock?

Nit: clocks are specified with a phandle + clock-specifier pair, not just a phandle.

> +- system-clock-frequency		: system clock rate
> +					  it will overwrite clocks's rate

This seems very odd.

Why do you want to overwrite a clock's rate?

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

What do these mean? Why are they not described when the property was defined above?

Does this also apply for the "format" property?

> +
> +Example:
> +
> +clock {
> +	osc: oscillator {
> +		#clock-cells = <0>;
> +		compatible = "fixed-clock";
> +		clock-frequency = <11289600>;
> +	};
> +};
> +
> +sound {
> +	compatible = "simple-audio";
> +
> +	simple-audio,card-name = "FSI2A-AK4648";
> +	simple-audio,format = "left_j";
> +
> +	simple-audio,cpu {
> +		sound-dai = <&sh_fsi2 0>;
> +	};
> +
> +	simple-audio,codec {
> +		sound-dai = <&ak4648>;
> +		bitclock-master;
> +		frame-master;
> +		clocks = <&osc>;
> +		/* it can use this instead of clocks
> +		 * system-clock-frequency = <11289600>; */

This just confuses matters. If ou want to show this off, have two examples.

> +	};
> +};
> +
> +&i2c0 {
> +	ak4648: ak4648@0x12 {

Nit: remove the 0x on the unit-address

> +		#sound-dai-cells = <0>;
> +		compatible = "asahi-kasei,ak4648";
> +		reg = <0x12>;
> +	};
> +};
> +
> +sh_fsi2: sh_fsi2@0xec230000 {

Again, fix the unit-address please.

> +	#sound-dai-cells = <1>;
> +	compatible = "renesas,sh_fsi2";
> +	reg = <0xec230000 0x400>;
> +	interrupt-parent = <&gic>;
> +	interrupts = <0 146 0x4>;
> +};
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 8c49147..62befbd 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -9,6 +9,8 @@
>   * published by the Free Software Foundation.
>   */
>  
> +#include <linux/clk.h>
> +#include <linux/of.h>
>  #include <linux/platform_device.h>
>  #include <linux/module.h>
>  #include <sound/simple_card.h>
> @@ -52,11 +54,135 @@ static int asoc_simple_card_dai_init(struct snd_soc_pcm_runtime *rtd)
>  	return 0;
>  }
>  
> +static int
> +__asoc_simple_card_parse_of(struct device_node *np,
> +			    struct asoc_simple_dai *dai,
> +			    struct device_node **node)
> +{
> +	struct clk *clk;
> +	int ret;
> +
> +	/*
> +	 * get node via "sound-dai = <&phandle port>"
> +	 * it will be used as xxx_of_node on soc_bind_dai_link()
> +	 */
> +	*node = of_parse_phandle(np, "sound-dai", 0);
> +	if (!*node)
> +		return -ENODEV;
> +
> +	of_node_put(*node);

Why?

You're refrering to it, so why do you not want to have it refcounted?

It could disappear under your feet.

> +
> +	/* get dai->name */
> +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> +	if (ret < 0)
> +		return ret;
> +
> +	/*
> +	 * bitclock-inversion, frame-inversion
> +	 * bitclock-master,    frame-master
> +	 * and specific "format" if it has
> +	 */
> +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> +
> +	/* dai->sysclk via "clolks = <xxx>" */
> +	clk = of_clk_get(np, 0);
> +	if (IS_ERR(clk))
> +		dai->sysclk = 0;
> +	else
> +		dai->sysclk = clk_get_rate(clk);

This seems like an odd assumption to make. Why not error?

> +
> +	/*
> +	 * overwrite dai->sysclk if it has
> +	 * "system-clock-frequency = <xxx>"
> +	 */
> +	of_property_read_u32(np,
> +			     "system-clock-frequency",
> +			     &dai->sysclk);

Is dai->sysclk defined as a u32?

> +
> +	return 0;
> +}
> +
> +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;
> +	int ret = 0;
> +
> +	/* get card name */
> +	of_property_read_string(node, "simple-audio,card-name", &info->card);
> +	info->name = info->card;

What if the string is not in the DT?

Does the core code handle these being NULL?

> +
> +	/* get CPU/CODEC common format via simple-audio,format */
> +	info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio,") &
> +		(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);
> +
> +	/* CPU/CODEC sub-node */
> +	for_each_child_of_node(node, np) {
> +		if (0 == strcmp("simple-audio,cpu", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->cpu_dai,
> +							  of_cpu);
> +		if (0 == strcmp("simple-audio,codec", np->name))
> +			ret = __asoc_simple_card_parse_of(np,
> +							  &info->codec_dai,
> +							  of_codec);

of_get_child_by_name?

> +		if (ret < 0)
> +			return ret;
> +	}

What if there were no children?

> +
> +	/* simple-card assumes platform == cpu */
> +	*of_platform = *of_cpu;
> +
> +	dev_dbg(dev, "card-name : %s\n", info->card);
> +	dev_dbg(dev, "platform : %04x / %p\n",
> +		info->daifmt,
> +		*of_platform);

Why is the pointer helpful, rather than the info it points to?

Thanks,
Mark.