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@xxxxxxxxxx; perex@xxxxxxxx; lgirdwood@xxxxxxxxx; tiwai@xxxxxxx;
> moinejf@xxxxxxx; andrew@xxxxxxx; kuninori.morimoto.gx@xxxxxxxxxxx;
> jsarha@xxxxxx; devicetree@xxxxxxxxxxxxxxx; alsa-devel@xxxxxxxxxxxxxxxx;
> robh+dt@xxxxxxxxxx; pawel.moll@xxxxxxx; mark.rutland@xxxxxxx;
> ijc+devicetree@xxxxxxxxxxxxxx; galak@xxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> 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@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
