[PATCHv2 0/4] simple-card: simplify the code.

[PATCHv2 0/4] simple-card: simplify the code.

[Xiubo Li]

And comment and advice are welcome.


Change in v2:
- Maintian compatibility with the old DTs.

Change in v1:
- Add simple-card dts node patches.
- Fix format parsing bug from Jean-Francois's comment.
- Rebase to Kuninori-san's newest changes in next branch.

Xiubo Li (4):
  ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the
    code.
  ASoC: simple-card: Merge single and muti DAI link(s) code.
  ASoC: simple-card: Adjust the comments of simple card.
  ASoC: simple-card: binding: update binding to support the new style.

 .../devicetree/bindings/sound/simple-card.txt      | 184 ++++++++++++++-------
 sound/soc/generic/simple-card.c                    | 131 ++++++++-------
 2 files changed, 191 insertions(+), 124 deletions(-)

-- 
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Xiubo Li]

Signed-off-by: Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
---
 sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 32 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 986d2c7..cad2b30 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	return 0;
 }
 
+static inline unsigned int
+asoc_simple_card_fmt_master(struct device_node *np,
+			    struct device_node *bitclkmaster,
+			    struct device_node *framemaster)
+{
+	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
+	case 0x11:
+		return SND_SOC_DAIFMT_CBS_CFS;
+	case 0x10:
+		return SND_SOC_DAIFMT_CBS_CFM;
+	case 0x01:
+		return SND_SOC_DAIFMT_CBM_CFS;
+	default:
+		return SND_SOC_DAIFMT_CBM_CFM;
+	}
+
+	/* Shouldn't be here */
+	return -EINVAL;
+}
+
 static int asoc_simple_card_dai_link_of(struct device_node *node,
 					struct device *dev,
 					struct snd_soc_dai_link *dai_link,
@@ -172,7 +192,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	struct device_node *np = NULL;
 	struct device_node *bitclkmaster = NULL;
 	struct device_node *framemaster = NULL;
-	unsigned int daifmt;
+	unsigned int daifmt, fmt;
 	char *name;
 	char prop[128];
 	char *prefix = "";
@@ -185,6 +205,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 					 &bitclkmaster, &framemaster);
 	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
 
+	/* Parse CPU DAI sub-node */
 	snprintf(prop, sizeof(prop), "%scpu", prefix);
 	np = of_get_child_by_name(node, prop);
 	if (!np) {
@@ -199,23 +220,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	if (ret < 0)
 		goto dai_link_of_err;
 
-	dai_props->cpu_dai.fmt = daifmt;
-	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
-	case 0x11:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
-		break;
-	case 0x10:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
-		break;
-	case 0x01:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
-		break;
-	default:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
-		break;
-	}
-
+	fmt = asoc_simple_card_fmt_master(np, bitclkmaster, framemaster);
+	dai_props->cpu_dai.fmt = daifmt | fmt;
 	of_node_put(np);
+
+	/* Parse CODEC DAI sub-node */
 	snprintf(prop, sizeof(prop), "%scodec", prefix);
 	np = of_get_child_by_name(node, prop);
 	if (!np) {
@@ -240,21 +249,9 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
 			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
 	} else {
-		dai_props->codec_dai.fmt = daifmt;
-		switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
-		case 0x11:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
-			break;
-		case 0x10:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
-			break;
-		case 0x01:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
-			break;
-		default:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
-			break;
-		}
+		fmt = asoc_simple_card_fmt_master(np, bitclkmaster,
+						  framemaster);
+		dai_props->codec_dai.fmt = daifmt | fmt;
 	}
 
 	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
-- 
1.8.4

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Varka Bhadram]

On 09/02/2014 02:56 PM, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>   sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++---------------------
>   1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 986d2c7..cad2b30 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>   	return 0;
>   }
>   
> +static inline unsigned int
> +asoc_simple_card_fmt_master(struct device_node *np,
> +			    struct device_node *bitclkmaster,
> +			    struct device_node *framemaster)
> +{
> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> +	case 0x11:
> +		return SND_SOC_DAIFMT_CBS_CFS;
> +	case 0x10:
> +		return SND_SOC_DAIFMT_CBS_CFM;
> +	case 0x01:
> +		return SND_SOC_DAIFMT_CBM_CFS;
> +	default:
> +		return SND_SOC_DAIFMT_CBM_CFM;
> +	}
> +
> +	/* Shouldn't be here */
> +	return -EINVAL;
> +}

It will be nice if we declare the switch case numbers as macros (specific name)...



-- 
Regards,
Varka Bhadram.

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Jean-Francois Moine]

On Tue, 02 Sep 2014 15:51:41 +0530
Varka Bhadram <varkabhadram@gmail.com> wrote:

> > +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> > +	case 0x11:
> > +		return SND_SOC_DAIFMT_CBS_CFS;
> > +	case 0x10:
> > +		return SND_SOC_DAIFMT_CBS_CFM;
> > +	case 0x01:
> > +		return SND_SOC_DAIFMT_CBM_CFS;
> > +	default:
> > +		return SND_SOC_DAIFMT_CBM_CFM;
> > +	}
> > +
> > +	/* Shouldn't be here */
> > +	return -EINVAL;
> > +}  
> 
> It will be nice if we declare the switch case numbers as macros (specific name)...

I don't see which macros: the values are just 2 booleans.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Varka Bhadram]

On 09/02/2014 04:08 PM, Jean-Francois Moine wrote:
> On Tue, 02 Sep 2014 15:51:41 +0530
> Varka Bhadram <varkabhadram-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
>
>>> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
>>> +	case 0x11:
>>> +		return SND_SOC_DAIFMT_CBS_CFS;
>>> +	case 0x10:
>>> +		return SND_SOC_DAIFMT_CBS_CFM;
>>> +	case 0x01:
>>> +		return SND_SOC_DAIFMT_CBM_CFS;
>>> +	default:
>>> +		return SND_SOC_DAIFMT_CBM_CFM;
>>> +	}
>>> +
>>> +	/* Shouldn't be here */
>>> +	return -EINVAL;
>>> +}
>> It will be nice if we declare the switch case numbers as macros (specific name)...
> I don't see which macros: the values are just 2 booleans.
>
I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable
names to those...?


-- 
Regards,
Varka Bhadram.

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Takashi Iwai]

At Tue, 02 Sep 2014 16:12:40 +0530,
Varka Bhadram wrote:
> 
> On 09/02/2014 04:08 PM, Jean-Francois Moine wrote:
> > On Tue, 02 Sep 2014 15:51:41 +0530
> > Varka Bhadram <varkabhadram@gmail.com> wrote:
> >
> >>> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> >>> +	case 0x11:
> >>> +		return SND_SOC_DAIFMT_CBS_CFS;
> >>> +	case 0x10:
> >>> +		return SND_SOC_DAIFMT_CBS_CFM;
> >>> +	case 0x01:
> >>> +		return SND_SOC_DAIFMT_CBM_CFS;
> >>> +	default:
> >>> +		return SND_SOC_DAIFMT_CBM_CFM;
> >>> +	}
> >>> +
> >>> +	/* Shouldn't be here */
> >>> +	return -EINVAL;
> >>> +}
> >> It will be nice if we declare the switch case numbers as macros (specific name)...
> > I don't see which macros: the values are just 2 booleans.
> >
> I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable
> names to those...?

The whole switch block is too hackish, makes unnecessarily complex.
It can be more strightforwardly like:

	if (np == bitclkmaster)
		return np == framemater ?
			SND_SOC_DAIFMT_CBS_CFS : SND_SOC_DAIFMT_CBS_CFM;
	else
		return np == framemaster ?
			SND_SOC_DAIFMT_CBM_CFS : SND_SOC_DAIFMT_CBM_CFM;

Or, if you love efficiency and complexity, something like:

#define SND_SOC_DAIFMT(_np, _clk, _frame) \
	((((_np) != (_clk)) | (((_np) != (_frame)) << 1) << 12) + (1 << 12)

Then
	return SND_SOC_DAIFMT(np, blkclkmaster, framemaster);

Takashi

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Jean-Francois Moine]

On Tue, 02 Sep 2014 16:12:40 +0530
Varka Bhadram <varkabhadram-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> >>> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> >>> +	case 0x11:
> >>> +		return SND_SOC_DAIFMT_CBS_CFS;
> >>> +	case 0x10:
> >>> +		return SND_SOC_DAIFMT_CBS_CFM;
> >>> +	case 0x01:
> >>> +		return SND_SOC_DAIFMT_CBM_CFS;
> >>> +	default:
> >>> +		return SND_SOC_DAIFMT_CBM_CFM;
> >>> +	}
> >>> +
> >>> +	/* Shouldn't be here */
> >>> +	return -EINVAL;
> >>> +}  
> >> It will be nice if we declare the switch case numbers as macros (specific name)...  
> > I don't see which macros: the values are just 2 booleans.
> >  
> I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable
> names to those...?

#define TRUE_TRUE 0x11
#define TRUE_FALSE 0x10
#define FALSE_TRUE 0x01

or

	case ((TRUE << 4) | TRUE:
		...
	case ((TRUE << 4) | FALSE:
		...
	case ((FALSE << 4) | TRUE:
		...

??

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Jyri Sarha]

On 09/02/2014 02:09 PM, Jean-Francois Moine wrote:
> On Tue, 02 Sep 2014 16:12:40 +0530
> Varka Bhadram <varkabhadram@gmail.com> wrote:
>
>>>>> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
>>>>> +	case 0x11:
>>>>> +		return SND_SOC_DAIFMT_CBS_CFS;
>>>>> +	case 0x10:
>>>>> +		return SND_SOC_DAIFMT_CBS_CFM;
>>>>> +	case 0x01:
>>>>> +		return SND_SOC_DAIFMT_CBM_CFS;
>>>>> +	default:
>>>>> +		return SND_SOC_DAIFMT_CBM_CFM;
>>>>> +	}
>>>>> +
>>>>> +	/* Shouldn't be here */
>>>>> +	return -EINVAL;
>>>>> +}
>>>> It will be nice if we declare the switch case numbers as macros (specific name)...
>>> I don't see which macros: the values are just 2 booleans.
>>>
>> I am talking about 0x11, 0x10, 0x01 values.. We can give any understandable
>> names to those...?
>
> #define TRUE_TRUE 0x11
> #define TRUE_FALSE 0x10
> #define FALSE_TRUE 0x01
>
> or
>
> 	case ((TRUE << 4) | TRUE:
> 		...
> 	case ((TRUE << 4) | FALSE:
> 		...
> 	case ((FALSE << 4) | TRUE:
> 		...
>

I would vote for this. Even over the options suggested by Takashi, but 
then again this really a matter of taste.

The fact that frame and bit-clock master boolean values are bundled into 
a single "enum" field, instead of two dedicated bits, makes all options 
bit inconvenient.

Best regards,
Jyri

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Jyri Sarha]

On 09/02/2014 12:26 PM, Xiubo Li wrote:
> Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> ---
>   sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++---------------------
>   1 file changed, 29 insertions(+), 32 deletions(-)
>
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index 986d2c7..cad2b30 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>   	return 0;
>   }
>
> +static inline unsigned int
> +asoc_simple_card_fmt_master(struct device_node *np,
> +			    struct device_node *bitclkmaster,
> +			    struct device_node *framemaster)
> +{
> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> +	case 0x11:
> +		return SND_SOC_DAIFMT_CBS_CFS;
> +	case 0x10:
> +		return SND_SOC_DAIFMT_CBS_CFM;
> +	case 0x01:
> +		return SND_SOC_DAIFMT_CBM_CFS;
> +	default:
> +		return SND_SOC_DAIFMT_CBM_CFM;
> +	}
> +
> +	/* Shouldn't be here */
> +	return -EINVAL;
> +}
> +
....
> +	fmt = asoc_simple_card_fmt_master(np, bitclkmaster, framemaster);
> +	dai_props->cpu_dai.fmt = daifmt | fmt;
...
> +		fmt = asoc_simple_card_fmt_master(np, bitclkmaster,
> +						  framemaster);
> +		dai_props->codec_dai.fmt = daifmt | fmt;

This won't work. The logic for cpu node needs to be negated for codec node.

Best regards,
Jyri

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Li.Xiubo]

> Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master()
> to simplify the code.
> 
> On 09/02/2014 12:26 PM, Xiubo Li wrote:
> > Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
> > ---
> >   sound/soc/generic/simple-card.c | 61 ++++++++++++++++++++-----------------
> ----
> >   1 file changed, 29 insertions(+), 32 deletions(-)
> >
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
> card.c
> > index 986d2c7..cad2b30 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -163,6 +163,26 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
> >   	return 0;
> >   }
> >
> > +static inline unsigned int
> > +asoc_simple_card_fmt_master(struct device_node *np,
> > +			    struct device_node *bitclkmaster,
> > +			    struct device_node *framemaster)
> > +{
> > +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> > +	case 0x11:
> > +		return SND_SOC_DAIFMT_CBS_CFS;
> > +	case 0x10:
> > +		return SND_SOC_DAIFMT_CBS_CFM;
> > +	case 0x01:
> > +		return SND_SOC_DAIFMT_CBM_CFS;
> > +	default:
> > +		return SND_SOC_DAIFMT_CBM_CFM;
> > +	}
> > +
> > +	/* Shouldn't be here */
> > +	return -EINVAL;
> > +}
> > +
> ....
> > +	fmt = asoc_simple_card_fmt_master(np, bitclkmaster, framemaster);
> > +	dai_props->cpu_dai.fmt = daifmt | fmt;
> ...
> > +		fmt = asoc_simple_card_fmt_master(np, bitclkmaster,
> > +						  framemaster);
> > +		dai_props->codec_dai.fmt = daifmt | fmt;
> 
> This won't work. The logic for cpu node needs to be negated for codec node.
> 

Yes, actually it should be.

As my previous patches about this:
----
Since from the DAI format micro SND_SOC_DAIFMT_CBx_CFx, the 'CBx'
mean Codec's bit clock is as master/slave and the 'CFx' mean Codec's
frame clock is as master/slave.

So these same DAI formats should be informed to CPU and CODE DAIs at
the same time. For the Codec driver will set the bit clock and frame
clock as the DAI formats said, but for the CPU driver, if the the
bit clock or frame clock is as Codec master, so it should be set CPU
DAI device as bit clock or frame clock as slave, and vice versa.

The old code will cause confusion, and we should be clear that the
letter 'C' here mean to Codec.
----

For the master format, no matter for CPU or CODEC, it always means Codec
is master or slave for bit/frame clock, not means the local DAI device's
bit/frame clock as master or slave.

So your CPU DAI device driver should negate this locally as the existed
Ones do.

Thanks,

BRs
Xiubo

Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Jyri Sarha]

On 09/03/2014 05:37 AM, Li.Xiubo@freescale.com wrote:
>> Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master()
...
>>
>> This won't work. The logic for cpu node needs to be negated for codec node.
>>
>
> Yes, actually it should be.
>
> As my previous patches about this:
> ----
> Since from the DAI format micro SND_SOC_DAIFMT_CBx_CFx, the 'CBx'
> mean Codec's bit clock is as master/slave and the 'CFx' mean Codec's
> frame clock is as master/slave.
>
> So these same DAI formats should be informed to CPU and CODE DAIs at
> the same time. For the Codec driver will set the bit clock and frame
> clock as the DAI formats said, but for the CPU driver, if the the
> bit clock or frame clock is as Codec master, so it should be set CPU
> DAI device as bit clock or frame clock as slave, and vice versa.
>
> The old code will cause confusion, and we should be clear that the
> letter 'C' here mean to Codec.
> ----
>
> For the master format, no matter for CPU or CODEC, it always means Codec
> is master or slave for bit/frame clock, not means the local DAI device's
> bit/frame clock as master or slave.
>
> So your CPU DAI device driver should negate this locally as the existed
> Ones do.
>


Yes, but there is double negation in this patch. The switch-case
assignments depend on whether the bitclkmaster and framemaster
DT-node pointers are compared to a cpu-dai-node or
codec-dai-node. When your patch compares the codec-node, it does
the decisions like it was a cpu-node, which produces inverted CBM
and CFM setting.

However, Kurinori-san's patch fixes this problem because it just
uses the daifmt generated by comparing to codec node for both cpu
and codec nodes.

The reason why I did the comparison per node basis, was to make
the code more ready for tdm setups with multiple codecs on a same
wire. But writing code for something that is not really needed
yet is usually a bad idea, like it was this time too.

Kurinori-san's version of the fix should be fine and it cleans up
the code quite nicely.

Best regards,
Jyri

RE: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master()
> to simplify the code.
> 
> On 09/03/2014 05:37 AM, Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org wrote:
> >> Subject: Re: [PATCHv2 1/4] ASoC: simple-card: add
> asoc_simple_card_fmt_master()
> ...
> >>
> >> This won't work. The logic for cpu node needs to be negated for codec node.
> >>
> >
> > Yes, actually it should be.
> >
> > As my previous patches about this:
> > ----
> > Since from the DAI format micro SND_SOC_DAIFMT_CBx_CFx, the 'CBx'
> > mean Codec's bit clock is as master/slave and the 'CFx' mean Codec's
> > frame clock is as master/slave.
> >
> > So these same DAI formats should be informed to CPU and CODE DAIs at
> > the same time. For the Codec driver will set the bit clock and frame
> > clock as the DAI formats said, but for the CPU driver, if the the
> > bit clock or frame clock is as Codec master, so it should be set CPU
> > DAI device as bit clock or frame clock as slave, and vice versa.
> >
> > The old code will cause confusion, and we should be clear that the
> > letter 'C' here mean to Codec.
> > ----
> >
> > For the master format, no matter for CPU or CODEC, it always means Codec
> > is master or slave for bit/frame clock, not means the local DAI device's
> > bit/frame clock as master or slave.
> >
> > So your CPU DAI device driver should negate this locally as the existed
> > Ones do.
> >
> 
> 
> Yes, but there is double negation in this patch. The switch-case
> assignments depend on whether the bitclkmaster and framemaster
> DT-node pointers are compared to a cpu-dai-node or
> codec-dai-node. When your patch compares the codec-node, it does
> the decisions like it was a cpu-node, which produces inverted CBM
> and CFM setting.
> 
> However, Kurinori-san's patch fixes this problem because it just
> uses the daifmt generated by comparing to codec node for both cpu
> and codec nodes.
> 
> The reason why I did the comparison per node basis, was to make
> the code more ready for tdm setups with multiple codecs on a same
> wire. But writing code for something that is not really needed
> yet is usually a bad idea, like it was this time too.
> 
> Kurinori-san's version of the fix should be fine and it cleans up
> the code quite nicely.
> 

Yes, agree.

So I just removed this patch from my patch series list.

Kuninori-san will post his local patch about this later.

Thanks,

BRs
Xiubo
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Kuninori Morimoto]

Hi Xiubo

I was very surprised about this patch
because the idea is same as my local patch
(I was planned to send it to ML :)

I attached my local patch to sharing idea.

> +static inline unsigned int
> +asoc_simple_card_fmt_master(struct device_node *np,
> +			    struct device_node *bitclkmaster,
> +			    struct device_node *framemaster)
> +{
> +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> +	case 0x11:
> +		return SND_SOC_DAIFMT_CBS_CFS;
> +	case 0x10:
> +		return SND_SOC_DAIFMT_CBS_CFM;
> +	case 0x01:
> +		return SND_SOC_DAIFMT_CBM_CFS;
> +	default:
> +		return SND_SOC_DAIFMT_CBM_CFM;
> +	}
> +
> +	/* Shouldn't be here */
> +	return -EINVAL;
> +}

I think this concept is nice,
but setting all fmt in this function is good for me
see my local patch

----------
>From 85562eb1587e5c184e4f4e0b183bd7063aaa81b7 Mon Sep 17 00:00:00 2001
From: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
Date: Thu, 28 Aug 2014 19:20:14 +0900
Subject: [PATCH] ASoC: simple-card: add asoc_simple_card_parse_daifmt()

Current daifmt setting method in simple-card driver is
placed to many places, and using un-readable/confusable method.
This patch adds new asoc_simple_card_parse_daifmt()
and tidyup code.

Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index bea5901..c932103 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -167,6 +167,64 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	return 0;
 }
 
+static int asoc_simple_card_parse_daifmt(struct device_node *node,
+					 struct simple_card_data *priv,
+					 struct device_node *cpu,
+					 struct device_node *codec,
+					 char *prefix, int idx)
+{
+	struct device *dev = simple_priv_to_dev(priv);
+	struct device_node *bitclkmaster = NULL;
+	struct device_node *framemaster = NULL;
+	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
+	struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
+	struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
+	unsigned int daifmt;
+
+	daifmt = snd_soc_of_parse_daifmt(node, prefix,
+					 &bitclkmaster, &framemaster);
+	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
+
+	if (strlen(prefix) && !bitclkmaster && !framemaster) {
+		/*
+		 * No dai-link level and master setting was not found from
+		 * sound node level, revert back to legacy DT parsing and
+		 * take the settings from codec node.
+		 */
+		dev_dbg(dev, "Revert to legacy daifmt parsing\n");
+
+		cpu_dai->fmt = codec_dai->fmt =
+			snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
+			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
+	} else {
+
+		switch (((codec == bitclkmaster) << 4) | (codec == framemaster)) {
+		case 0x11:
+			daifmt |= SND_SOC_DAIFMT_CBM_CFM;
+			break;
+		case 0x10:
+			daifmt |= SND_SOC_DAIFMT_CBM_CFS;
+			break;
+		case 0x01:
+			daifmt |= SND_SOC_DAIFMT_CBS_CFM;
+			break;
+		default:
+			daifmt |= SND_SOC_DAIFMT_CBS_CFS;
+			break;
+		}
+
+		cpu_dai->fmt = daifmt;
+		codec_dai->fmt = daifmt;
+	}
+
+	if (bitclkmaster)
+		of_node_put(bitclkmaster);
+	if (framemaster)
+		of_node_put(framemaster);
+
+	return 0;
+}
+
 static int asoc_simple_card_dai_link_of(struct device_node *node,
 					struct simple_card_data *priv,
 					int idx,
@@ -175,10 +233,8 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	struct device *dev = simple_priv_to_dev(priv);
 	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx);
 	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
-	struct device_node *np = NULL;
-	struct device_node *bitclkmaster = NULL;
-	struct device_node *framemaster = NULL;
-	unsigned int daifmt;
+	struct device_node *cpu = NULL;
+	struct device_node *codec = NULL;
 	char *name;
 	char prop[128];
 	char *prefix = "";
@@ -187,82 +243,35 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	if (is_top_level_node)
 		prefix = "simple-audio-card,";
 
-	daifmt = snd_soc_of_parse_daifmt(node, prefix,
-					 &bitclkmaster, &framemaster);
-	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
-
 	snprintf(prop, sizeof(prop), "%scpu", prefix);
-	np = of_get_child_by_name(node, prop);
-	if (!np) {
+	cpu = of_get_child_by_name(node, prop);
+
+	snprintf(prop, sizeof(prop), "%scodec", prefix);
+	codec = of_get_child_by_name(node, prop);
+
+	if (!cpu || !codec) {
 		ret = -EINVAL;
 		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
 		goto dai_link_of_err;
 	}
 
-	ret = asoc_simple_card_sub_parse_of(np, &dai_props->cpu_dai,
-					    &dai_link->cpu_of_node,
-					    &dai_link->cpu_dai_name);
+	ret = asoc_simple_card_parse_daifmt(node, priv,
+					    cpu, codec, prefix, idx);
 	if (ret < 0)
 		goto dai_link_of_err;
 
-	dai_props->cpu_dai.fmt = daifmt;
-	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
-	case 0x11:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
-		break;
-	case 0x10:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
-		break;
-	case 0x01:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
-		break;
-	default:
-		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
-		break;
-	}
-
-	of_node_put(np);
-	snprintf(prop, sizeof(prop), "%scodec", prefix);
-	np = of_get_child_by_name(node, prop);
-	if (!np) {
-		ret = -EINVAL;
-		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
+	ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai,
+					    &dai_link->cpu_of_node,
+					    &dai_link->cpu_dai_name);
+	if (ret < 0)
 		goto dai_link_of_err;
-	}
 
-	ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
+	ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai,
 					    &dai_link->codec_of_node,
 					    &dai_link->codec_dai_name);
 	if (ret < 0)
 		goto dai_link_of_err;
 
-	if (strlen(prefix) && !bitclkmaster && !framemaster) {
-		/* No dai-link level and master setting was not found from
-		   sound node level, revert back to legacy DT parsing and
-		   take the settings from codec node. */
-		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
-			__func__);
-		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
-			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
-			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
-	} else {
-		dai_props->codec_dai.fmt = daifmt;
-		switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
-		case 0x11:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
-			break;
-		case 0x10:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
-			break;
-		case 0x01:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
-			break;
-		default:
-			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
-			break;
-		}
-	}
-
 	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
 		ret = -EINVAL;
 		goto dai_link_of_err;
@@ -304,12 +313,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	dai_link->cpu_dai_name = NULL;
 
 dai_link_of_err:
-	if (np)
-		of_node_put(np);
-	if (bitclkmaster)
-		of_node_put(bitclkmaster);
-	if (framemaster)
-		of_node_put(framemaster);
+	if (cpu)
+		of_node_put(cpu);
+	if (codec)
+		of_node_put(codec);
 	return ret;
 }
 
---------

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Li.Xiubo]

Hi Kuninori-san,

Yes, I think it make sense to set all fmt in one function, and will
Be more readable.

I agree with you, could you please just wait, because there has many
Replications and good Ideas about this patch, and I will revise it.
Then you can improve it as your patch blow.


Thanks,

BRs
Xiubo


> Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add
> asoc_simple_card_fmt_master() to simplify the code.
> 
> 
> Hi Xiubo
> 
> I was very surprised about this patch
> because the idea is same as my local patch
> (I was planned to send it to ML :)
> 
> I attached my local patch to sharing idea.
> 
> > +static inline unsigned int
> > +asoc_simple_card_fmt_master(struct device_node *np,
> > +			    struct device_node *bitclkmaster,
> > +			    struct device_node *framemaster)
> > +{
> > +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> > +	case 0x11:
> > +		return SND_SOC_DAIFMT_CBS_CFS;
> > +	case 0x10:
> > +		return SND_SOC_DAIFMT_CBS_CFM;
> > +	case 0x01:
> > +		return SND_SOC_DAIFMT_CBM_CFS;
> > +	default:
> > +		return SND_SOC_DAIFMT_CBM_CFM;
> > +	}
> > +
> > +	/* Shouldn't be here */
> > +	return -EINVAL;
> > +}
> 
> I think this concept is nice,
> but setting all fmt in this function is good for me
> see my local patch
> 
> ----------
> From 85562eb1587e5c184e4f4e0b183bd7063aaa81b7 Mon Sep 17 00:00:00 2001
> From: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> Date: Thu, 28 Aug 2014 19:20:14 +0900
> Subject: [PATCH] ASoC: simple-card: add asoc_simple_card_parse_daifmt()
> 
> Current daifmt setting method in simple-card driver is
> placed to many places, and using un-readable/confusable method.
> This patch adds new asoc_simple_card_parse_daifmt()
> and tidyup code.
> 
> Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx@renesas.com>
> 
> diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
> index bea5901..c932103 100644
> --- a/sound/soc/generic/simple-card.c
> +++ b/sound/soc/generic/simple-card.c
> @@ -167,6 +167,64 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
>  	return 0;
>  }
> 
> +static int asoc_simple_card_parse_daifmt(struct device_node *node,
> +					 struct simple_card_data *priv,
> +					 struct device_node *cpu,
> +					 struct device_node *codec,
> +					 char *prefix, int idx)
> +{
> +	struct device *dev = simple_priv_to_dev(priv);
> +	struct device_node *bitclkmaster = NULL;
> +	struct device_node *framemaster = NULL;
> +	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
> +	struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
> +	struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
> +	unsigned int daifmt;
> +
> +	daifmt = snd_soc_of_parse_daifmt(node, prefix,
> +					 &bitclkmaster, &framemaster);
> +	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
> +
> +	if (strlen(prefix) && !bitclkmaster && !framemaster) {
> +		/*
> +		 * No dai-link level and master setting was not found from
> +		 * sound node level, revert back to legacy DT parsing and
> +		 * take the settings from codec node.
> +		 */
> +		dev_dbg(dev, "Revert to legacy daifmt parsing\n");
> +
> +		cpu_dai->fmt = codec_dai->fmt =
> +			snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
> +			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
> +	} else {
> +
> +		switch (((codec == bitclkmaster) << 4) | (codec == framemaster))
> {
> +		case 0x11:
> +			daifmt |= SND_SOC_DAIFMT_CBM_CFM;
> +			break;
> +		case 0x10:
> +			daifmt |= SND_SOC_DAIFMT_CBM_CFS;
> +			break;
> +		case 0x01:
> +			daifmt |= SND_SOC_DAIFMT_CBS_CFM;
> +			break;
> +		default:
> +			daifmt |= SND_SOC_DAIFMT_CBS_CFS;
> +			break;
> +		}
> +
> +		cpu_dai->fmt = daifmt;
> +		codec_dai->fmt = daifmt;
> +	}
> +
> +	if (bitclkmaster)
> +		of_node_put(bitclkmaster);
> +	if (framemaster)
> +		of_node_put(framemaster);
> +
> +	return 0;
> +}
> +
>  static int asoc_simple_card_dai_link_of(struct device_node *node,
>  					struct simple_card_data *priv,
>  					int idx,
> @@ -175,10 +233,8 @@ static int asoc_simple_card_dai_link_of(struct
> device_node *node,
>  	struct device *dev = simple_priv_to_dev(priv);
>  	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx);
>  	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
> -	struct device_node *np = NULL;
> -	struct device_node *bitclkmaster = NULL;
> -	struct device_node *framemaster = NULL;
> -	unsigned int daifmt;
> +	struct device_node *cpu = NULL;
> +	struct device_node *codec = NULL;
>  	char *name;
>  	char prop[128];
>  	char *prefix = "";
> @@ -187,82 +243,35 @@ static int asoc_simple_card_dai_link_of(struct
> device_node *node,
>  	if (is_top_level_node)
>  		prefix = "simple-audio-card,";
> 
> -	daifmt = snd_soc_of_parse_daifmt(node, prefix,
> -					 &bitclkmaster, &framemaster);
> -	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
> -
>  	snprintf(prop, sizeof(prop), "%scpu", prefix);
> -	np = of_get_child_by_name(node, prop);
> -	if (!np) {
> +	cpu = of_get_child_by_name(node, prop);
> +
> +	snprintf(prop, sizeof(prop), "%scodec", prefix);
> +	codec = of_get_child_by_name(node, prop);
> +
> +	if (!cpu || !codec) {
>  		ret = -EINVAL;
>  		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
>  		goto dai_link_of_err;
>  	}
> 
> -	ret = asoc_simple_card_sub_parse_of(np, &dai_props->cpu_dai,
> -					    &dai_link->cpu_of_node,
> -					    &dai_link->cpu_dai_name);
> +	ret = asoc_simple_card_parse_daifmt(node, priv,
> +					    cpu, codec, prefix, idx);
>  	if (ret < 0)
>  		goto dai_link_of_err;
> 
> -	dai_props->cpu_dai.fmt = daifmt;
> -	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> -	case 0x11:
> -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> -		break;
> -	case 0x10:
> -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> -		break;
> -	case 0x01:
> -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> -		break;
> -	default:
> -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> -		break;
> -	}
> -
> -	of_node_put(np);
> -	snprintf(prop, sizeof(prop), "%scodec", prefix);
> -	np = of_get_child_by_name(node, prop);
> -	if (!np) {
> -		ret = -EINVAL;
> -		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
> +	ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai,
> +					    &dai_link->cpu_of_node,
> +					    &dai_link->cpu_dai_name);
> +	if (ret < 0)
>  		goto dai_link_of_err;
> -	}
> 
> -	ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
> +	ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai,
>  					    &dai_link->codec_of_node,
>  					    &dai_link->codec_dai_name);
>  	if (ret < 0)
>  		goto dai_link_of_err;
> 
> -	if (strlen(prefix) && !bitclkmaster && !framemaster) {
> -		/* No dai-link level and master setting was not found from
> -		   sound node level, revert back to legacy DT parsing and
> -		   take the settings from codec node. */
> -		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
> -			__func__);
> -		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
> -			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
> -			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
> -	} else {
> -		dai_props->codec_dai.fmt = daifmt;
> -		switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> -		case 0x11:
> -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> -			break;
> -		case 0x10:
> -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> -			break;
> -		case 0x01:
> -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> -			break;
> -		default:
> -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> -			break;
> -		}
> -	}
> -
>  	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
>  		ret = -EINVAL;
>  		goto dai_link_of_err;
> @@ -304,12 +313,10 @@ static int asoc_simple_card_dai_link_of(struct
> device_node *node,
>  	dai_link->cpu_dai_name = NULL;
> 
>  dai_link_of_err:
> -	if (np)
> -		of_node_put(np);
> -	if (bitclkmaster)
> -		of_node_put(bitclkmaster);
> -	if (framemaster)
> -		of_node_put(framemaster);
> +	if (cpu)
> +		of_node_put(cpu);
> +	if (codec)
> +		of_node_put(codec);
>  	return ret;
>  }
> 
> ---------
> 
> Best regards
> ---
> Kuninori Morimoto

Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Kuninori Morimoto]

Hi Xiubo

> Yes, I think it make sense to set all fmt in one function, and will
> Be more readable.
> 
> I agree with you, could you please just wait, because there has many
> Replications and good Ideas about this patch, and I will revise it.
> Then you can improve it as your patch blow.

Thank you for your help
I don't care so much about my local patch.
You can re-use it if you want.
Of course I can do it if you want

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add
> asoc_simple_card_fmt_master() to simplify the code.
> 
> 
> Hi Xiubo
> 
> > Yes, I think it make sense to set all fmt in one function, and will
> > Be more readable.
> >
> > I agree with you, could you please just wait, because there has many
> > Replications and good Ideas about this patch, and I will revise it.
> > Then you can improve it as your patch blow.
> 
> Thank you for your help
> I don't care so much about my local patch.
> You can re-use it if you want.
> Of course I can do it if you want
> 

Please send it out of your local patch.

Please also consider the ideas about Jyri, Jean-Francios, Varka and
Takashi's advice as previous emails about my patch.


BRs
Xiubo

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

Hi Kuninori-san,

Yes your patch has fixed the bug Jyri has pointed out.

So I has discard my [PATCHv2 1/4] patch.

Please send your patch out to replace this one.

Thanks,

BRs
Xiubo



> -----Original Message-----
> From: Xiubo Li-B47053
> Sent: Wednesday, September 03, 2014 10:22 AM
> To: 'Kuninori Morimoto'
> Cc: broonie-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; perex-/Fr2/VpizcU@public.gmane.org; lgirdwood-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org; tiwai-l3A5Bk7waGM@public.gmane.org;
> moinejf-GANU6spQydw@public.gmane.org; andrew-g2DYL2Zd6BY@public.gmane.org; kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org;
> jsarha-l0cyMroinI0@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; alsa-devel-K7yf7f+aM1XWsZ/bQMPhNw@public.gmane.org;
> robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; pawel.moll-5wv7dgnIgG8@public.gmane.org; mark.rutland-5wv7dgnIgG8@public.gmane.org;
> ijc+devicetree-KcIKpvwj1kUDXYZnReoRVg@public.gmane.org; galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org; linux-
> kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: RE: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add
> asoc_simple_card_fmt_master() to simplify the code.
> 
> Hi Kuninori-san,
> 
> Yes, I think it make sense to set all fmt in one function, and will
> Be more readable.
> 
> I agree with you, could you please just wait, because there has many
> Replications and good Ideas about this patch, and I will revise it.
> Then you can improve it as your patch blow.
> 
> 
> Thanks,
> 
> BRs
> Xiubo
> 
> 
> > Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add
> > asoc_simple_card_fmt_master() to simplify the code.
> >
> >
> > Hi Xiubo
> >
> > I was very surprised about this patch
> > because the idea is same as my local patch
> > (I was planned to send it to ML :)
> >
> > I attached my local patch to sharing idea.
> >
> > > +static inline unsigned int
> > > +asoc_simple_card_fmt_master(struct device_node *np,
> > > +			    struct device_node *bitclkmaster,
> > > +			    struct device_node *framemaster)
> > > +{
> > > +	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> > > +	case 0x11:
> > > +		return SND_SOC_DAIFMT_CBS_CFS;
> > > +	case 0x10:
> > > +		return SND_SOC_DAIFMT_CBS_CFM;
> > > +	case 0x01:
> > > +		return SND_SOC_DAIFMT_CBM_CFS;
> > > +	default:
> > > +		return SND_SOC_DAIFMT_CBM_CFM;
> > > +	}
> > > +
> > > +	/* Shouldn't be here */
> > > +	return -EINVAL;
> > > +}
> >
> > I think this concept is nice,
> > but setting all fmt in this function is good for me
> > see my local patch
> >
> > ----------
> > From 85562eb1587e5c184e4f4e0b183bd7063aaa81b7 Mon Sep 17 00:00:00 2001
> > From: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> > Date: Thu, 28 Aug 2014 19:20:14 +0900
> > Subject: [PATCH] ASoC: simple-card: add asoc_simple_card_parse_daifmt()
> >
> > Current daifmt setting method in simple-card driver is
> > placed to many places, and using un-readable/confusable method.
> > This patch adds new asoc_simple_card_parse_daifmt()
> > and tidyup code.
> >
> > Signed-off-by: Kuninori Morimoto <kuninori.morimoto.gx-zM6kxYcvzFBBDgjK7y7TUQ@public.gmane.org>
> >
> > diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-
> card.c
> > index bea5901..c932103 100644
> > --- a/sound/soc/generic/simple-card.c
> > +++ b/sound/soc/generic/simple-card.c
> > @@ -167,6 +167,64 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
> >  	return 0;
> >  }
> >
> > +static int asoc_simple_card_parse_daifmt(struct device_node *node,
> > +					 struct simple_card_data *priv,
> > +					 struct device_node *cpu,
> > +					 struct device_node *codec,
> > +					 char *prefix, int idx)
> > +{
> > +	struct device *dev = simple_priv_to_dev(priv);
> > +	struct device_node *bitclkmaster = NULL;
> > +	struct device_node *framemaster = NULL;
> > +	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
> > +	struct asoc_simple_dai *cpu_dai = &dai_props->cpu_dai;
> > +	struct asoc_simple_dai *codec_dai = &dai_props->codec_dai;
> > +	unsigned int daifmt;
> > +
> > +	daifmt = snd_soc_of_parse_daifmt(node, prefix,
> > +					 &bitclkmaster, &framemaster);
> > +	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
> > +
> > +	if (strlen(prefix) && !bitclkmaster && !framemaster) {
> > +		/*
> > +		 * No dai-link level and master setting was not found from
> > +		 * sound node level, revert back to legacy DT parsing and
> > +		 * take the settings from codec node.
> > +		 */
> > +		dev_dbg(dev, "Revert to legacy daifmt parsing\n");
> > +
> > +		cpu_dai->fmt = codec_dai->fmt =
> > +			snd_soc_of_parse_daifmt(codec, NULL, NULL, NULL) |
> > +			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
> > +	} else {
> > +
> > +		switch (((codec == bitclkmaster) << 4) | (codec == framemaster))
> > {
> > +		case 0x11:
> > +			daifmt |= SND_SOC_DAIFMT_CBM_CFM;
> > +			break;
> > +		case 0x10:
> > +			daifmt |= SND_SOC_DAIFMT_CBM_CFS;
> > +			break;
> > +		case 0x01:
> > +			daifmt |= SND_SOC_DAIFMT_CBS_CFM;
> > +			break;
> > +		default:
> > +			daifmt |= SND_SOC_DAIFMT_CBS_CFS;
> > +			break;
> > +		}
> > +
> > +		cpu_dai->fmt = daifmt;
> > +		codec_dai->fmt = daifmt;
> > +	}
> > +
> > +	if (bitclkmaster)
> > +		of_node_put(bitclkmaster);
> > +	if (framemaster)
> > +		of_node_put(framemaster);
> > +
> > +	return 0;
> > +}
> > +
> >  static int asoc_simple_card_dai_link_of(struct device_node *node,
> >  					struct simple_card_data *priv,
> >  					int idx,
> > @@ -175,10 +233,8 @@ static int asoc_simple_card_dai_link_of(struct
> > device_node *node,
> >  	struct device *dev = simple_priv_to_dev(priv);
> >  	struct snd_soc_dai_link *dai_link = simple_priv_to_link(priv, idx);
> >  	struct simple_dai_props *dai_props = simple_priv_to_props(priv, idx);
> > -	struct device_node *np = NULL;
> > -	struct device_node *bitclkmaster = NULL;
> > -	struct device_node *framemaster = NULL;
> > -	unsigned int daifmt;
> > +	struct device_node *cpu = NULL;
> > +	struct device_node *codec = NULL;
> >  	char *name;
> >  	char prop[128];
> >  	char *prefix = "";
> > @@ -187,82 +243,35 @@ static int asoc_simple_card_dai_link_of(struct
> > device_node *node,
> >  	if (is_top_level_node)
> >  		prefix = "simple-audio-card,";
> >
> > -	daifmt = snd_soc_of_parse_daifmt(node, prefix,
> > -					 &bitclkmaster, &framemaster);
> > -	daifmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
> > -
> >  	snprintf(prop, sizeof(prop), "%scpu", prefix);
> > -	np = of_get_child_by_name(node, prop);
> > -	if (!np) {
> > +	cpu = of_get_child_by_name(node, prop);
> > +
> > +	snprintf(prop, sizeof(prop), "%scodec", prefix);
> > +	codec = of_get_child_by_name(node, prop);
> > +
> > +	if (!cpu || !codec) {
> >  		ret = -EINVAL;
> >  		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
> >  		goto dai_link_of_err;
> >  	}
> >
> > -	ret = asoc_simple_card_sub_parse_of(np, &dai_props->cpu_dai,
> > -					    &dai_link->cpu_of_node,
> > -					    &dai_link->cpu_dai_name);
> > +	ret = asoc_simple_card_parse_daifmt(node, priv,
> > +					    cpu, codec, prefix, idx);
> >  	if (ret < 0)
> >  		goto dai_link_of_err;
> >
> > -	dai_props->cpu_dai.fmt = daifmt;
> > -	switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> > -	case 0x11:
> > -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> > -		break;
> > -	case 0x10:
> > -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> > -		break;
> > -	case 0x01:
> > -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> > -		break;
> > -	default:
> > -		dai_props->cpu_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> > -		break;
> > -	}
> > -
> > -	of_node_put(np);
> > -	snprintf(prop, sizeof(prop), "%scodec", prefix);
> > -	np = of_get_child_by_name(node, prop);
> > -	if (!np) {
> > -		ret = -EINVAL;
> > -		dev_err(dev, "%s: Can't find %s DT node\n", __func__, prop);
> > +	ret = asoc_simple_card_sub_parse_of(cpu, &dai_props->cpu_dai,
> > +					    &dai_link->cpu_of_node,
> > +					    &dai_link->cpu_dai_name);
> > +	if (ret < 0)
> >  		goto dai_link_of_err;
> > -	}
> >
> > -	ret = asoc_simple_card_sub_parse_of(np, &dai_props->codec_dai,
> > +	ret = asoc_simple_card_sub_parse_of(codec, &dai_props->codec_dai,
> >  					    &dai_link->codec_of_node,
> >  					    &dai_link->codec_dai_name);
> >  	if (ret < 0)
> >  		goto dai_link_of_err;
> >
> > -	if (strlen(prefix) && !bitclkmaster && !framemaster) {
> > -		/* No dai-link level and master setting was not found from
> > -		   sound node level, revert back to legacy DT parsing and
> > -		   take the settings from codec node. */
> > -		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
> > -			__func__);
> > -		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
> > -			snd_soc_of_parse_daifmt(np, NULL, NULL, NULL) |
> > -			(daifmt & ~SND_SOC_DAIFMT_CLOCK_MASK);
> > -	} else {
> > -		dai_props->codec_dai.fmt = daifmt;
> > -		switch (((np == bitclkmaster) << 4) | (np == framemaster)) {
> > -		case 0x11:
> > -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFM;
> > -			break;
> > -		case 0x10:
> > -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBM_CFS;
> > -			break;
> > -		case 0x01:
> > -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFM;
> > -			break;
> > -		default:
> > -			dai_props->codec_dai.fmt |= SND_SOC_DAIFMT_CBS_CFS;
> > -			break;
> > -		}
> > -	}
> > -
> >  	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) {
> >  		ret = -EINVAL;
> >  		goto dai_link_of_err;
> > @@ -304,12 +313,10 @@ static int asoc_simple_card_dai_link_of(struct
> > device_node *node,
> >  	dai_link->cpu_dai_name = NULL;
> >
> >  dai_link_of_err:
> > -	if (np)
> > -		of_node_put(np);
> > -	if (bitclkmaster)
> > -		of_node_put(bitclkmaster);
> > -	if (framemaster)
> > -		of_node_put(framemaster);
> > +	if (cpu)
> > +		of_node_put(cpu);
> > +	if (codec)
> > +		of_node_put(codec);
> >  	return ret;
> >  }
> >
> > ---------
> >
> > Best regards
> > ---
> > Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Kuninori Morimoto]

Hi Xiubo

> Yes your patch has fixed the bug Jyri has pointed out.
> 
> So I has discard my [PATCHv2 1/4] patch.
> 
> Please send your patch out to replace this one.
(snip)
> Please send it out of your local patch.
>
> Please also consider the ideas about Jyri, Jean-Francios, Varka and
> Takashi's advice as previous emails about my patch.

OK, will do.
To avoid confusion/conflict, I will post it after Mark applied it.
Because many simple-card patches are posted in these days...

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add
> asoc_simple_card_fmt_master() to simplify the code.
> 
> 
> Hi Xiubo
> 
> > Yes your patch has fixed the bug Jyri has pointed out.
> >
> > So I has discard my [PATCHv2 1/4] patch.
> >
> > Please send your patch out to replace this one.
> (snip)
> > Please send it out of your local patch.
> >
> > Please also consider the ideas about Jyri, Jean-Francios, Varka and
> > Takashi's advice as previous emails about my patch.
> 
> OK, will do.
> To avoid confusion/conflict, I will post it after Mark applied it.
> Because many simple-card patches are posted in these days...
> 

Yes, Get it.

Thanks,

BRs
Xiubo

--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCHv2 1/4] ASoC: simple-card: add asoc_simple_card_fmt_master() to simplify the code.

[Jean-Francois Moine]

On Tue, 02 Sep 2014 21:13:52 -0700 (PDT)
Kuninori Morimoto <kuninori.morimoto.gx@gmail.com> wrote:

> OK, will do.
> To avoid confusion/conflict, I will post it after Mark applied it.
> Because many simple-card patches are posted in these days...

Yes, I have one more awaiting, about multi-CODECs...

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/

[PATCHv2 2/4] ASoC: simple-card: Merge single and muti DAI link(s) code.

[Xiubo Li]

This patch will split the DT node into old style and new style:
The new style will merge the single DAI link and muti DAI links code
together, the new style will be easier to add muti DAI links from old
single DAI link DTs.

This patch will maintian compatibility with the old DTs.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 sound/soc/generic/simple-card.c | 22 ++++++++++++----------
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index cad2b30..667fa49 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -198,6 +198,7 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 	char *prefix = "";
 	int ret;
 
+	/* For single DAI link & old style of DT node */
 	if (is_top_level_node)
 		prefix = "simple-audio-card,";
 
@@ -306,14 +307,16 @@ dai_link_of_err:
 
 static int asoc_simple_card_parse_of(struct device_node *node,
 				     struct simple_card_data *priv,
-				     struct device *dev,
-				     int multi)
+				     struct device *dev)
 {
 	struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link;
 	struct simple_dai_props *dai_props = priv->dai_props;
 	u32 val;
 	int ret;
 
+	if (!node)
+		return -EINVAL;
+
 	/* parsing the card name from DT */
 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
 
@@ -341,7 +344,8 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	dev_dbg(dev, "New simple-card: %s\n", priv->snd_card.name ?
 		priv->snd_card.name : "");
 
-	if (multi) {
+	/* Single/Muti DAI link(s) & New style of DT node */
+	if (of_get_child_by_name(node, "simple-audio-card,dai-link")) {
 		struct device_node *np = NULL;
 		int i = 0;
 
@@ -358,6 +362,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 			i++;
 		}
 	} else {
+		/* For single DAI link & old style of DT node */
 		ret = asoc_simple_card_dai_link_of(node, dev,
 						   dai_link, dai_props, true);
 		if (ret < 0)
@@ -397,16 +402,13 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	struct snd_soc_dai_link *dai_link;
 	struct device_node *np = pdev->dev.of_node;
 	struct device *dev = &pdev->dev;
-	int num_links, multi, ret;
+	int num_links, ret;
 
 	/* get the number of DAI links */
-	if (np && of_get_child_by_name(np, "simple-audio-card,dai-link")) {
+	if (np && of_get_child_by_name(np, "simple-audio-card,dai-link"))
 		num_links = of_get_child_count(np);
-		multi = 1;
-	} else {
+	else
 		num_links = 1;
-		multi = 0;
-	}
 
 	/* allocate the private data and the DAI link array */
 	priv = devm_kzalloc(dev,
@@ -433,7 +435,7 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 
 	if (np && of_device_is_available(np)) {
 
-		ret = asoc_simple_card_parse_of(np, priv, dev, multi);
+		ret = asoc_simple_card_parse_of(np, priv, dev);
 		if (ret < 0) {
 			if (ret != -EPROBE_DEFER)
 				dev_err(dev, "parse error %d\n", ret);
-- 
1.8.4

[PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.

[Xiubo Li]

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

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 667fa49..e6976a0 100644
--- a/sound/soc/generic/simple-card.c
+++ b/sound/soc/generic/simple-card.c
@@ -120,7 +120,7 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 	int ret;
 
 	/*
-	 * get node via "sound-dai = <&phandle port>"
+	 * 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);
@@ -128,19 +128,19 @@ asoc_simple_card_sub_parse_of(struct device_node *np,
 		return -ENODEV;
 	*p_node = node;
 
-	/* get dai->name */
+	/* Get dai->name */
 	ret = snd_soc_of_get_dai_name(np, name);
 	if (ret < 0)
 		return ret;
 
-	/* parse TDM slot */
+	/* Parse TDM slot */
 	ret = snd_soc_of_parse_tdm_slot(np, &dai->slots, &dai->slot_width);
 	if (ret)
 		return ret;
 
 	/*
-	 * dai->sysclk come from
-	 *  "clocks = <&xxx>" (if system has common clock)
+	 * Parse dai->sysclk come from "clocks = <&xxx>"
+	 * (if system has common clock)
 	 *  or "system-clock-frequency = <xxx>"
 	 *  or device's module clock.
 	 */
@@ -241,9 +241,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 		goto dai_link_of_err;
 
 	if (strlen(prefix) && !bitclkmaster && !framemaster) {
-		/* No dai-link level and master setting was not found from
-		   sound node level, revert back to legacy DT parsing and
-		   take the settings from codec node. */
+		/*
+		 * No DAI link level and master setting was found
+		 * from sound node level, revert back to legacy DT
+		 * parsing and take the settings from codec node.
+		 */
 		dev_dbg(dev, "%s: Revert to legacy daifmt parsing\n",
 			__func__);
 		dai_props->cpu_dai.fmt = dai_props->codec_dai.fmt =
@@ -260,10 +262,10 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 		goto dai_link_of_err;
 	}
 
-	/* simple-card assumes platform == cpu */
+	/* Simple Card assumes platform == cpu */
 	dai_link->platform_of_node = dai_link->cpu_of_node;
 
-	/* Link name is created from CPU/CODEC dai name */
+	/* DAI link name is created from CPU/CODEC dai name */
 	name = devm_kzalloc(dev,
 			    strlen(dai_link->cpu_dai_name)   +
 			    strlen(dai_link->codec_dai_name) + 2,
@@ -285,11 +287,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
 		dai_props->codec_dai.sysclk);
 
 	/*
-	 * soc_bind_dai_link() will check cpu name
-	 * after of_node matching if dai_link has cpu_dai_name.
-	 * but, it will never match if name was created by fmt_single_name()
-	 * remove cpu_dai_name to escape name matching.
-	 * see
+	 * In soc_bind_dai_link() will check cpu name after
+	 * of_node matching if dai_link has cpu_dai_name.
+	 * but, it will never match if name was created by
+	 * fmt_single_name() remove cpu_dai_name to escape
+	 * name matching. Please see:
 	 *	fmt_single_name()
 	 *	fmt_multiple_name()
 	 */
@@ -317,10 +319,10 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	if (!node)
 		return -EINVAL;
 
-	/* parsing the card name from DT */
+	/* Parse the card name from DT */
 	snd_soc_of_parse_card_name(&priv->snd_card, "simple-audio-card,name");
 
-	/* off-codec widgets */
+	/* The off-codec widgets */
 	if (of_property_read_bool(node, "simple-audio-card,widgets")) {
 		ret = snd_soc_of_parse_audio_simple_widgets(&priv->snd_card,
 					"simple-audio-card,widgets");
@@ -375,7 +377,7 @@ static int asoc_simple_card_parse_of(struct device_node *node,
 	return 0;
 }
 
-/* update the reference count of the devices nodes at end of probe */
+/* Decrease the reference count of the device nodes */
 static int asoc_simple_card_unref(struct platform_device *pdev)
 {
 	struct snd_soc_card *card = platform_get_drvdata(pdev);
@@ -404,29 +406,27 @@ static int asoc_simple_card_probe(struct platform_device *pdev)
 	struct device *dev = &pdev->dev;
 	int num_links, ret;
 
-	/* get the number of DAI links */
+	/* Get the number of DAI links */
 	if (np && of_get_child_by_name(np, "simple-audio-card,dai-link"))
 		num_links = of_get_child_count(np);
 	else
 		num_links = 1;
 
-	/* allocate the private data and the DAI link array */
+	/* Allocate the private data and the DAI link array */
 	priv = devm_kzalloc(dev,
 			sizeof(*priv) + sizeof(*dai_link) * num_links,
 			GFP_KERNEL);
 	if (!priv)
 		return -ENOMEM;
 
-	/*
-	 * init snd_soc_card
-	 */
+	/* Init snd_soc_card */
 	priv->snd_card.owner = THIS_MODULE;
 	priv->snd_card.dev = dev;
 	dai_link = priv->dai_link;
 	priv->snd_card.dai_link = dai_link;
 	priv->snd_card.num_links = num_links;
 
-	/* get room for the other properties */
+	/* Get room for the other properties */
 	priv->dai_props = devm_kzalloc(dev,
 			sizeof(*priv->dai_props) * num_links,
 			GFP_KERNEL);
-- 
1.8.4

Re: [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.

[Jean-Francois Moine]

On Tue, 2 Sep 2014 17:26:08 +0800
Xiubo Li <Li.Xiubo-KZfg59tc24xl57MIdRCFDg@public.gmane.org> wrote:

> @@ -285,11 +287,11 @@ static int asoc_simple_card_dai_link_of(struct device_node *node,
>  		dai_props->codec_dai.sysclk);
>  
>  	/*
> -	 * soc_bind_dai_link() will check cpu name
> -	 * after of_node matching if dai_link has cpu_dai_name.
> -	 * but, it will never match if name was created by fmt_single_name()
> -	 * remove cpu_dai_name to escape name matching.
> -	 * see
> +	 * In soc_bind_dai_link() will check cpu name after
> +	 * of_node matching if dai_link has cpu_dai_name.
> +	 * but, it will never match if name was created by
> +	 * fmt_single_name() remove cpu_dai_name to escape
> +	 * name matching. Please see:
>  	 *	fmt_single_name()
>  	 *	fmt_multiple_name()
>  	 */

The patch done by Kuninori, setting the cpu_dai_name to NULL in all
cases, does not work. This sequence should be replaced where is was
previously.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: Re: [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple
> card.
> 
> On Tue, 2 Sep 2014 17:26:08 +0800
> Xiubo Li <Li.Xiubo@freescale.com> wrote:
> 
> > @@ -285,11 +287,11 @@ static int asoc_simple_card_dai_link_of(struct
> device_node *node,
> >  		dai_props->codec_dai.sysclk);
> >
> >  	/*
> > -	 * soc_bind_dai_link() will check cpu name
> > -	 * after of_node matching if dai_link has cpu_dai_name.
> > -	 * but, it will never match if name was created by fmt_single_name()
> > -	 * remove cpu_dai_name to escape name matching.
> > -	 * see
> > +	 * In soc_bind_dai_link() will check cpu name after
> > +	 * of_node matching if dai_link has cpu_dai_name.
> > +	 * but, it will never match if name was created by
> > +	 * fmt_single_name() remove cpu_dai_name to escape
> > +	 * name matching. Please see:
> >  	 *	fmt_single_name()
> >  	 *	fmt_multiple_name()
> >  	 */
> 
> The patch done by Kuninori, setting the cpu_dai_name to NULL in all
> cases, does not work. This sequence should be replaced where is was
> previously.
> 

If so, it should be another issue here, should we send another patch for
It ?


Thanks,

BRs
Xiubo

Re: [alsa-devel] [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.

[Kuninori Morimoto]

Hi Xiubo

> > >  	/*
> > > -	 * soc_bind_dai_link() will check cpu name
> > > -	 * after of_node matching if dai_link has cpu_dai_name.
> > > -	 * but, it will never match if name was created by fmt_single_name()
> > > -	 * remove cpu_dai_name to escape name matching.
> > > -	 * see
> > > +	 * In soc_bind_dai_link() will check cpu name after
> > > +	 * of_node matching if dai_link has cpu_dai_name.
> > > +	 * but, it will never match if name was created by
> > > +	 * fmt_single_name() remove cpu_dai_name to escape
> > > +	 * name matching. Please see:
> > >  	 *	fmt_single_name()
> > >  	 *	fmt_multiple_name()
> > >  	 */
> > 
> > The patch done by Kuninori, setting the cpu_dai_name to NULL in all
> > cases, does not work. This sequence should be replaced where is was
> > previously.
> > 
> 
> If so, it should be another issue here, should we send another patch for
> It ?

I posted patch yesterday, and Jean tested it

Subject: [PATCH][RFC] ASoC: simple-card: fixup cpu_dai_name clear case
Date: Tue, 02 Sep 2014 20:05:32 +0900

Best regards
---
Kuninori Morimoto
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [alsa-devel] [PATCHv2 3/4] ASoC: simple-card: Adjust the comments of simple card.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: Re: [alsa-devel] [PATCHv2 3/4] ASoC: simple-card: Adjust the comments
> of simple card.
> 
> 
> Hi Xiubo
> 
> > > >  	/*
> > > > -	 * soc_bind_dai_link() will check cpu name
> > > > -	 * after of_node matching if dai_link has cpu_dai_name.
> > > > -	 * but, it will never match if name was created by
> fmt_single_name()
> > > > -	 * remove cpu_dai_name to escape name matching.
> > > > -	 * see
> > > > +	 * In soc_bind_dai_link() will check cpu name after
> > > > +	 * of_node matching if dai_link has cpu_dai_name.
> > > > +	 * but, it will never match if name was created by
> > > > +	 * fmt_single_name() remove cpu_dai_name to escape
> > > > +	 * name matching. Please see:
> > > >  	 *	fmt_single_name()
> > > >  	 *	fmt_multiple_name()
> > > >  	 */
> > >
> > > The patch done by Kuninori, setting the cpu_dai_name to NULL in all
> > > cases, does not work. This sequence should be replaced where is was
> > > previously.
> > >
> >
> > If so, it should be another issue here, should we send another patch for
> > It ?
> 
> I posted patch yesterday, and Jean tested it
> 
> Subject: [PATCH][RFC] ASoC: simple-card: fixup cpu_dai_name clear case
> Date: Tue, 02 Sep 2014 20:05:32 +0900
> 

Nice.

I think I just missed it.

Thanks,

BRs
Xiubo


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCHv2 4/4] ASoC: simple-card: binding: update binding to support the new style.

[Xiubo Li]

This update patch will split the DT node into old style and new style:
The new style will will be easier to add muti DAI links from old single
DAI link DTs.

This patch will maintian compatibility with the old DTs.

Signed-off-by: Xiubo Li <Li.Xiubo@freescale.com>
---
 .../devicetree/bindings/sound/simple-card.txt      | 184 ++++++++++++++-------
 1 file changed, 126 insertions(+), 58 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt
index c2e9841..6fb8966 100644
--- a/Documentation/devicetree/bindings/sound/simple-card.txt
+++ b/Documentation/devicetree/bindings/sound/simple-card.txt
@@ -1,15 +1,19 @@
-Simple-Card:
+Device-Tree bindings for Simple Card
 
 Simple-Card specifies audio DAI connections of SoC <-> codec.
 
-Required properties:
+=== Top level's properties and subnodes ===
 
+*** Required properties ***
 - compatible				: "simple-audio-card"
 
-Optional properties:
-
+*** Optional properties ***
 - simple-audio-card,name		: User specified audio sound card name, one string
 					  property.
+- simple-audio-card,format		: CPU/CODEC common audio format.
+					  "i2s", "right_j", "left_j" , "dsp_a"
+					  "dsp_b", "ac97", "pdm", "msb", "lsb"
+					  (This is used for single DAI link & old style.)
 - simple-audio-card,widgets		: Please refer to widgets.txt.
 - simple-audio-card,routing		: A list of the connections between audio components.
 					  Each entry is a pair of strings, the first being the
@@ -17,63 +21,85 @@ Optional properties:
 					  source.
 - simple-audio-card,mclk-fs             : Multiplication factor between stream rate and codec
   					  mclk.
-
-Optional subnodes:
-
-- simple-audio-card,dai-link		: Container for dai-link level
-					  properties and the CPU and CODEC
-					  sub-nodes. This container may be
-					  omitted when the card has only one
-					  DAI link. See the examples and the
-					  section bellow.
-
-Dai-link subnode properties and subnodes:
-
-If dai-link subnode is omitted and the subnode properties are directly
-under "sound"-node the subnode property and subnode names have to be
-prefixed with "simple-audio-card,"-prefix.
-
-Required dai-link subnodes:
-
-- cpu					: CPU   sub-node
-- codec					: CODEC sub-node
-
-Optional dai-link subnode properties:
-
+- simple-audio-card,frame-master	: Indicates DAI link frame master. One phandle to a cpu
+					  or codec subnode.
+					  (This is used for single DAI link & old style.)
+- simple-audio-card,bitclock-master	: Indicates DAI link bit clock master. One phandle to a
+					  cpu or codec subnode.
+					  (This is used for single DAI link & old style.)
+
+*** Optional subnodes ***
+- simple-audio-card,dai-link		: Container for DAI link level properties and the CPU
+					  and CODEC sub-nodes. This container may be omitted
+					  when the card has only one DAI link and using the old
+					  style. See the examples and the section bellow.
+- simple-audio-card,cpu			: CPU DAI sub-node.
+					  (This is used for single DAI link & old style.)
+- simple-audio-card,codec		: CODEC DAI sub-node.
+					  (This is used for single DAI link & old style.)
+
+=== DAI link node's properties and its subnodes ===
+
+*** Required subnodes ***
+- cpu					: CPU DAI sub-node
+- codec					: CODEC DAI sub-node
+
+*** Optional properties ***
 - format				: CPU/CODEC common audio format.
 					  "i2s", "right_j", "left_j" , "dsp_a"
 					  "dsp_b", "ac97", "pdm", "msb", "lsb"
-- frame-master				: Indicates dai-link frame master.
-					  phandle to a cpu or codec subnode.
-- bitclock-master			: Indicates dai-link bit clock master.
-					  phandle to a cpu or codec subnode.
-- bitclock-inversion			: bool property. Add this if the
-					  dai-link uses bit clock inversion.
-- frame-inversion			: bool property. Add this if the
-					  dai-link uses frame clock inversion.
+- frame-master				: Indicates DAI link frame master. One phandle to a cpu
+					  or codec subnode.
+					  (This is One boolean property for old style.)
+- bitclock-master			: Indicates DAI link bit clock master. One phandle to a
+					  cpu or codec subnode.
+					  (This is one boolean property for old style.)
 
 For backward compatibility the frame-master and bitclock-master
 properties can be used as booleans in codec subnode to indicate if the
-codec is the dai-link frame or bit clock master. In this case there
-should be no dai-link node, the same properties should not be present
+codec is the DAI link frame or bit clock master. In this case there
+should be no DAI link node, the same properties should not be present
 at sound-node level, and the bitclock-inversion and frame-inversion
 properties should also be placed in the codec node if needed.
 
-Required CPU/CODEC subnodes properties:
 
-- sound-dai				: phandle and port of CPU/CODEC
+=== CPU/CODEC DAI node's properties and its subnodes ===
 
-Optional CPU/CODEC subnodes properties:
+*** Required properties ***
+- sound-dai				: One phandle and port of CPU/CODEC
 
+*** Optional properties ***
+- bitclock-inversion			: Boolean property. Add this if the DAI device uses bit
+					  clock inversion.
+- frame-inversion			: Boolean property. Add this if the DAI device uses frame
+					  clock inversion.
 - dai-tdm-slot-num			: Please refer to tdm-slot.txt.
 - dai-tdm-slot-width			: Please refer to tdm-slot.txt.
-- 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)
+- clocks / system-clock-frequency	: specify CPU/CODEC DAI node'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)
 
-Example 1 - single DAI link:
+=== Examples ===
+*** CPU & CODEC DAI DT nodes ***
+&i2c0 {
+	ak4648: ak4648@12 {
+		#sound-dai-cells = <0>;
+		compatible = "asahi-kasei,ak4648";
+		reg = <0x12>;
+	};
+};
 
+sh_fsi2: sh_fsi2@ec230000 {
+	#sound-dai-cells = <1>;
+	compatible = "renesas,sh_fsi2";
+	reg = <0xec230000 0x400>;
+	interrupt-parent = <&gic>;
+	interrupts = <0 146 0x4>;
+};
+
+Example 1 - single DAI link & old style:
+bitclock-master and frame-master as phandles.
 sound {
 	compatible = "simple-audio-card";
 	simple-audio-card,name = "VF610-Tower-Sound-Card";
@@ -91,6 +117,7 @@ sound {
 
 	simple-audio-card,cpu {
 		sound-dai = <&sh_fsi2 0>;
+		bitclock-inversion;
 	};
 
 	dailink0_master: simple-audio-card,codec {
@@ -99,24 +126,64 @@ sound {
 	};
 };
 
-&i2c0 {
-	ak4648: ak4648@12 {
-		#sound-dai-cells = <0>;
-		compatible = "asahi-kasei,ak4648";
-		reg = <0x12>;
+Example 2 - single DAI link & old style:
+bitclock-master and frame-master as boolean properties.
+sound {
+	compatible = "simple-audio-card";
+	simple-audio-card,name = "VF610-Tower-Sound-Card";
+	simple-audio-card,format = "left_j";
+	simple-audio-card,widgets =
+		"Microphone", "Microphone Jack",
+		"Headphone", "Headphone Jack",
+		"Speaker", "External Speaker";
+	simple-audio-card,routing =
+		"MIC_IN", "Microphone Jack",
+		"Headphone Jack", "HP_OUT",
+		"External Speaker", "LINE_OUT";
+
+	simple-audio-card,cpu {
+		sound-dai = <&sh_fsi2 0>;
 	};
-};
 
-sh_fsi2: sh_fsi2@ec230000 {
-	#sound-dai-cells = <1>;
-	compatible = "renesas,sh_fsi2";
-	reg = <0xec230000 0x400>;
-	interrupt-parent = <&gic>;
-	interrupts = <0 146 0x4>;
+	simple-audio-card,codec {
+		sound-dai = <&ak4648>;
+		clocks = <&osc>;
+		bitclock-master;
+		frame-master;
+		bitclock-inversion;
+	};
 };
 
-Example 2 - many DAI links:
+Example 3 - single DAI link & new style:
+sound {
+	compatible = "simple-audio-card";
+	simple-audio-card,name = "VF610-Tower-Sound-Card";
+	simple-audio-card,widgets =
+		"Microphone", "Microphone Jack",
+		"Headphone", "Headphone Jack",
+		"Speaker", "External Speaker";
+	simple-audio-card,routing =
+		"MIC_IN", "Microphone Jack",
+		"Headphone Jack", "HP_OUT",
+		"External Speaker", "LINE_OUT";
+
+	simple-audio-card,dai-link {
+		format = "i2s";
+		bitclock-master = <&dailink0_master>;
+		frame-master = <&dailink0_master>;
+		cpu {
+			sound-dai = <&sh_fsi2 0>;
+			frame-inversion;
+		};
+		dailink0_master: codec {
+			sound-dai = <&ak4648>;
+			clocks = <&osc>;
+			frame-inversion;
+		};
+	};
+};
 
+Example 4 - many DAI links:
 sound {
 	compatible = "simple-audio-card";
 	simple-audio-card,name = "Cubox Audio";
@@ -128,6 +195,7 @@ sound {
 		};
 		codec {
 			sound-dai = <&tda998x 0>;
+			frame-inversion;
 		};
 	};
 
-- 
1.8.4

Re: [PATCHv2 4/4] ASoC: simple-card: binding: update binding to support the new style.

[Jean-Francois Moine]

On Tue, 2 Sep 2014 17:26:09 +0800
Xiubo Li <Li.Xiubo@freescale.com> wrote:

> +Example 4 - many DAI links:
>  sound {
>  	compatible = "simple-audio-card";
>  	simple-audio-card,name = "Cubox Audio";
> @@ -128,6 +195,7 @@ sound {
>  		};
>  		codec {
>  			sound-dai = <&tda998x 0>;
> +			frame-inversion;
>  		};
>  	};

This is not useful: there is no clock/frame handling in the kirkwood
audio controller.

-- 
Ken ar c'hentañ	|	      ** Breizh ha Linux atav! **
Jef		|		http://moinejf.free.fr/
_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

RE: [PATCHv2 4/4] ASoC: simple-card: binding: update binding to support the new style.

[Li.Xiubo-KZfg59tc24xl57MIdRCFDg]

> Subject: Re: [PATCHv2 4/4] ASoC: simple-card: binding: update binding to
> support the new style.
> 
> On Tue, 2 Sep 2014 17:26:09 +0800
> Xiubo Li <Li.Xiubo@freescale.com> wrote:
> 
> > +Example 4 - many DAI links:
> >  sound {
> >  	compatible = "simple-audio-card";
> >  	simple-audio-card,name = "Cubox Audio";
> > @@ -128,6 +195,7 @@ sound {
> >  		};
> >  		codec {
> >  			sound-dai = <&tda998x 0>;
> > +			frame-inversion;
> >  		};
> >  	};
> 
> This is not useful: there is no clock/frame handling in the kirkwood
> audio controller.
> 


Okay, just for one example, if it really matter here I will remove this.

Thanks,

BRs
Xiubo