diff mbox

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

Message ID 87egvt7dmd.wl%kuninori.morimoto.gx@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Kuninori Morimoto Sept. 3, 2014, 12:26 a.m. UTC
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>

---------

Best regards
---
Kuninori Morimoto

Comments

Xiubo Li Sept. 3, 2014, 2:21 a.m. UTC | #1
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
Kuninori Morimoto Sept. 3, 2014, 3:36 a.m. UTC | #2
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
Xiubo Li Sept. 3, 2014, 3:37 a.m. UTC | #3
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@kernel.org; perex@perex.cz; lgirdwood@gmail.com; tiwai@suse.de;
> moinejf@free.fr; andrew@lunn.ch; kuninori.morimoto.gx@renesas.com;
> jsarha@ti.com; devicetree@vger.kernel.org; alsa-devel@alsa-project.org;
> robh+dt@kernel.org; pawel.moll@arm.com; mark.rutland@arm.com;
> ijc+devicetree@hellion.org.uk; galak@codeaurora.org; linux-
> kernel@vger.kernel.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@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
Xiubo Li Sept. 3, 2014, 3:41 a.m. UTC | #4
> 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
Kuninori Morimoto Sept. 3, 2014, 4:13 a.m. UTC | #5
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
Xiubo Li Sept. 3, 2014, 5:33 a.m. UTC | #6
> 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
Jean-Francois Moine Sept. 3, 2014, 6:48 a.m. UTC | #7
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...
diff mbox

Patch

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;
 }