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

["Li.Xiubo@freescale.com" <Li.Xiubo@freescale.com>]

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