RE: [PATCH] ASoC: simple-card: cleanup asoc_simple_card_parse_of()code

From: Li.Xiubo@xxxxxxxxxxxxx
Date: Mon Dec 30 2013 - 20:35:56 EST


Hi Mark and all,

Merry Christmas.


> Subject: Re: [PATCH] ASoC: simple-card: cleanup asoc_simple_card_parse_of()
> code
>
> On Fri, Dec 27, 2013 at 11:14:38AM +0800, Xiubo Li wrote:
> > Since there is only one common format via "simple-audio-card,format"
> > for simple card driver(CPU & CODEC DAI), there is no need to do the
> > INV mask.
>
> I don't understand this change, how is the fact that the CPU and CODEC
> DAI share a common format related to using the inversion? The inversion
> specifies the format relative to the spec rather than relative to the
> other end of the link and for any DAI link the two devices should be
> using the same format.

Sorry, sorry for confusion.

>From the "Documentation/devicetree/bindings/sound/simple-card.txt":
------
Optional properties:

- simple-audio-card,format : CPU/CODEC common audio format.
"i2s", "right_j", "left_j" , "dsp_a"
"dsp_b", "ac97", "pdm", "msb", "lsb"
------
There is only one common daifmt properties in simple-card parent dt node, and
its bits mask is SND_SOC_DAIFMT_FORMAT_MASK.

And also in the same binding documents:
------
Optional CPU/CODEC subnodes properties:

- format : CPU/CODEC specific audio format if needed.
see simple-audio-card,format
- frame-master : bool property. add this if subnode is frame master
- bitclock-master : bool property. add this if subnode is bitclock master
- 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 (= common clock), or "system-clock-frequency"
(if system doens't support common clock)
------

And in "sound/soc/generic/simple-card.c", function : asoc_simple_card_parse_of()
-----
......

/* get CPU/CODEC common format via simple-audio-card,format */
info->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") &
(SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK);

......

ret = asoc_simple_card_sub_parse_of(cpu, ...);

...

ret = asoc_simple_card_sub_parse_of(codec, ...);

......
-----
The SND_SOC_DAIFMT_INV_MASK bits mask should be for "bitclock-master" and
"frame-inversion" in subnodes' daifmt parsing if need, or why shouldn't
SND_SOC_DAIFMT_CLOCK_MASK be here like SND_SOC_DAIFMT_INV_MASK ?

IMO, the SND_SOC_DAIFMT_INV_MASK bits mask here maybe a little confused.

Certainly, these two and other subnode properties can also be common properties
like "[prefix]format" for other drivers.

It's the limitation of snd_soc_of_parse_daifmt() that the simple-card driver must
do the mask(except SND_SOC_DAIFMT_MASTER_MASK, because it default value is not zero)
operation.

Or should we change "bitclock-master" and "frame-master" as one string property
like "[prefix]master = XXX", and set the default value as zero like others. And then
we can remove all the bits mask operations in simple-card and other drivers.
If need, I will send this patch of this.


Thanks,

--
Best Regards,
Xiubo

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/