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

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.

Also, for most of the devices of the same DAI link, they use the same DAI formats, but I have found some drivers like : "sound/soc/pxa/magician.c" and "sound/soc/s6000/s6105-ipcam.c", etc, may have different DAI format settings for some reasons(maybe some formats are the CPU/Codec devices default setting that it needn't set it here or others).

I think maybe this is also the reason why the simple-card has common DAI format and CPU/CODEC private DAI formats at the same time.

Thanks,

-- Best Regards, Xiubo

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

On Tue, Dec 31, 2013 at 03:27:53AM +0000, Li.Xiubo@freescale.com wrote:

...

Also, for most of the devices of the same DAI link, they use the same DAI formats, but I have found some drivers like : "sound/soc/pxa/magician.c" and "sound/soc/s6000/s6105-ipcam.c", etc, may have different DAI format settings for some reasons(maybe some formats are the CPU/Codec devices default setting that it needn't set it here or others).

The ipcam still has that?

----------- /* set codec DAI configuration */ ret = snd_soc_dai_set_fmt(codec_dai, SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM); if (ret < 0) return ret;

/* set cpu DAI configuration */ ret = snd_soc_dai_set_fmt(cpu_dai, SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_NB_NF); if (ret < 0) return ret; ----------

... ?

...

I think maybe this is also the reason why the simple-card has common DAI format and CPU/CODEC private DAI formats at the same time.

I'd really want to see a real use case that actually was a simple card.

I'm also still trying.

Thanks,

-- Best Regards, Xiubo