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

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

[Xiubo Li]

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.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 sound/soc/generic/simple-card.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index d340320..8189e68 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -135,7 +135,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 
 	/* 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);
+		SND_SOC_DAIFMT_FORMAT_MASK;
 
 	ret = asoc_simple_card_get_component(node, info);
 	if (ret)
-- 
1.8.4

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 518 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Li.Xiubo]

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

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

[Li.Xiubo]

Hi Mark,


> -----
> 	......
> 
>         /* 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

Sorry the "bitclock-master" should be "bitclock-inversion" just for written
Mistake.



> "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

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

[Li.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

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

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1008 bytes --]

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?  It was supposed to have been fixed before it
was merged since it was identified as a bug in the CPU DAI during review.  
I don't know about magician since it was before my time but I suspect it
suffers from the same issue or that there's something else in there that
needs some other hardware more explicitly representing.

> 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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

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

[Li.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