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@xxxxxxxxxxx>
> 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@xxxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
