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
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
