[alsa-devel] [PATCH v4] ASoC: simple-card: add Device Tree support

[Kuninori Morimoto]

Hi Mark Rutland

Thank you for your feedback

> > +- frame-master				: bool property. add this if subnode was frame master
> > +- bitclock-master			: bool property. add this if subnode was bitclock master
> 
> s/was/is/

Oops, I will fix it on v5

> > +- bitclock-inversion			: bool property. add this if subnode has clock inversion
> > +- frame-inversion			: bool property. add this if subnode has frame inversion
> > +- clocks / system-clock-frequency	: specify subnode's clock if needed.
> > +					  it can be specified via "clocks" if system has clock node,
> > +                                          or "system-clock-frequency" if system doesn't have it.
> 
> What does "if system doesn't have it" mean? If it doesn't have a clock,
> how does said non-existent clock have a frequency?

It means "if system doesn't support common clock".
I will fix it

> > +static int
> > +asoc_simple_card_sub_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;
> > +
> > +	/* get dai->name */
> > +	ret = snd_soc_of_get_dai_name(np, &dai->name);
> > +	if (ret < 0)
> > +		goto parse_error;
> > +
> > +	/*
> > +	 * bitclock-inversion, frame-inversion
> > +	 * bitclock-master,    frame-master
> > +	 * and specific "format" if it has
> > +	 */
> 
> This comment looks confusing to me. I'm not sure it's all that helpful.
> 
> > +	dai->fmt = snd_soc_of_parse_daifmt(np, NULL);
> 
> As a general note, I'm surprised there isn't a helper that does all of
> the above, from of_parse_phandle to here.

snd_soc_of_parse_daifmt() is the helper fucntion,
and above comment is explaining it.
Or, am I misunderstanding your review ?

> > +	/*
> > +	 * dai->sysclk come from
> > +	 *  "clolks = <&xxx>" or "system-clock-frequency = <xxx>"
> 
> s/clolks/clocks/

Grr, thank you

> > +	clk = of_clk_get(np, 0);
> > +	if (IS_ERR(clk))
> > +		of_property_read_u32(np,
> > +				     "system-clock-frequency",
> > +				     &dai->sysclk);
> 
> What it this isn't present?

If sysclk doesn't have common clock support

> > +	/* simple-card assumes platform == cpu */
> > +	*of_platform = *of_cpu;
> 
> Why? What does this imply?

This is the one of reason why this driver is "simple" card

Best regards
---
Kuninori Morimoto