| Message ID | 1394593331-22867-1-git-send-email-Guangyu.Chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 03/12/2014 05:02 AM, Nicolin Chen wrote: > The current simple-card driver separates the daimft for cpu_dai and codec_dai. > So we might get different values for them (0x4003 and 0x1003 for example): > > asoc-simple-card sound-cs42888.12: cpu : 2024000.esai / 4003 / 132000000 > asoc-simple-card sound-cs42888.12: codec : cs42888 / 1003 / 24576000 > asoc-simple-card sound-cs42888.12: cs42888 <-> 2024000.esai mapping ok > > This is not allowed at all as we need to keep the DAIFMT settings identical > for both the ends of the link. > > Thus this patch fixes it by overwriting the cpu_dai->fmt with codec_dai->fmt > since we defined the DAIFMT_MASTER basing on CODEC at the first place while > the other bits are same. > > Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com> > --- Hi Nicolin, This patch is an improvement, but in my opinion the binding is still a bit confusing. How about changing 'frame-master' and 'bitclock-master' to 'codec-frame-master' and 'codec-bitclock-master'. We could possibly keep the old names as aliases until all the .dts files out there have been fixed. At the same go we could add SND_SOC_DAIFMT_MASTER_MASK here: > /* get CPU/CODEC common format via simple-audio-card,format */ > priv->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") & > (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK); or leave the masking out all together. Can't see why SND_SOC_DAIFMT_CONT/GATED could not be defined at dai-link level too. This way the norm would be defining the daifmt at link level. We could still keep the possibility to overwrite the setting at dai level if there is need for that. Best regards, Jyri
On Wed, Mar 12, 2014 at 11:25:32AM +0200, Jyri Sarha wrote: > On 03/12/2014 05:02 AM, Nicolin Chen wrote: > >The current simple-card driver separates the daimft for cpu_dai and codec_dai. > >So we might get different values for them (0x4003 and 0x1003 for example): > > > >asoc-simple-card sound-cs42888.12: cpu : 2024000.esai / 4003 / 132000000 > >asoc-simple-card sound-cs42888.12: codec : cs42888 / 1003 / 24576000 > >asoc-simple-card sound-cs42888.12: cs42888 <-> 2024000.esai mapping ok > > > >This is not allowed at all as we need to keep the DAIFMT settings identical > >for both the ends of the link. > > > >Thus this patch fixes it by overwriting the cpu_dai->fmt with codec_dai->fmt > >since we defined the DAIFMT_MASTER basing on CODEC at the first place while > >the other bits are same. > > > >Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com> > >--- > Hi Nicolin, > This patch is an improvement, but in my opinion the binding is still > a bit confusing. > > How about changing 'frame-master' and 'bitclock-master' to > 'codec-frame-master' and 'codec-bitclock-master'. We could possibly > keep the old names as aliases until all the .dts files out there > have been fixed. > > At the same go we could add SND_SOC_DAIFMT_MASTER_MASK here: > > > /* get CPU/CODEC common format via simple-audio-card,format */ > > priv->daifmt = snd_soc_of_parse_daifmt(node, "simple-audio-card,") & > > (SND_SOC_DAIFMT_FORMAT_MASK | SND_SOC_DAIFMT_INV_MASK); > > or leave the masking out all together. Can't see why > SND_SOC_DAIFMT_CONT/GATED could not be defined at dai-link level > too. > > This way the norm would be defining the daifmt at link level. We > could still keep the possibility to overwrite the setting at dai > level if there is need for that. Hi Jyri, After looking at Morimoto-san's mail, I think there might be a better solution for this issue as he suggested: We may move the fmt from struct asoc_simple_dai to a common place for both CPU and CODEC. I'm not sure if there's any defect from this idea, but as long as we keep DAIFMT settings identical for both dai-link ends, it'll be a neater way. So I'd like to pend this patch and wait for further solution from that topic. Thank you, Nicolin Chen
On Wed, Mar 12, 2014 at 05:33:54PM +0800, Nicolin Chen wrote: > I'm not sure if there's any defect from this idea, but as long as we > keep DAIFMT settings identical for both dai-link ends, it'll be a > neater way. > So I'd like to pend this patch and wait for further solution from that > topic. My understanding of the binding had been that exactly one of the devices on the link would have each master flag set and the code was parsing both devices together in order to come up with the setting. Obviously I didn't check the code closely enough here. It seems that the Renesas devices actually do this, though just due to a bug in the master/slave handling. The patch you posted would essentially implement that (without validating the settings on the CPU side, but that could be added). The above suggestion would also work though it *is* a change in the binding and right now we don't have code for it. We need something here very soon, the merge window is going to open shortly and we ought to avoid having the problematic code in v3.15 if we can - indeed I'd really prefer to get something into v3.14 if possible (though this patch doesn't apply cleanly there and it's *very* late). Thoughts?
On Wed, Mar 12, 2014 at 11:02:11AM +0800, Nicolin Chen wrote: > The current simple-card driver separates the daimft for cpu_dai and codec_dai. > So we might get different values for them (0x4003 and 0x1003 for example): Since I really want to see some sort of fix in v3.15 that allows sensible looking DTs to be written I've gone ahead and applied this. I'd expect we'll change it again but this at least lets us only set the master flag on one end of the link.
diff --git a/Documentation/devicetree/bindings/sound/simple-card.txt b/Documentation/devicetree/bindings/sound/simple-card.txt index b30c222..881914b1 100644 --- a/Documentation/devicetree/bindings/sound/simple-card.txt +++ b/Documentation/devicetree/bindings/sound/simple-card.txt @@ -43,6 +43,12 @@ Optional CPU/CODEC subnodes properties: clock node (= common clock), or "system-clock-frequency" (if system doens't support common clock) +Note: + * For 'format', 'frame-master', 'bitclock-master', 'bitclock-inversion' and + 'frame-inversion', the simple card will use the settings of CODEC for both + CPU and CODEC sides as we need to keep the settings identical for both ends + of the link. + Example: sound { diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c index 5dd4769..09591ab 100644 --- a/sound/soc/generic/simple-card.c +++ b/sound/soc/generic/simple-card.c @@ -157,6 +157,8 @@ static int asoc_simple_card_parse_of(struct device_node *node, struct device *dev) { struct snd_soc_dai_link *dai_link = priv->snd_card.dai_link; + struct asoc_simple_dai *codec_dai = &priv->codec_dai; + struct asoc_simple_dai *cpu_dai = &priv->cpu_dai; struct device_node *np; char *name; int ret; @@ -189,7 +191,7 @@ static int asoc_simple_card_parse_of(struct device_node *node, np = of_get_child_by_name(node, "simple-audio-card,cpu"); if (np) ret = asoc_simple_card_sub_parse_of(np, priv->daifmt, - &priv->cpu_dai, + cpu_dai, &dai_link->cpu_of_node, &dai_link->cpu_dai_name); if (ret < 0) @@ -200,12 +202,18 @@ static int asoc_simple_card_parse_of(struct device_node *node, np = of_get_child_by_name(node, "simple-audio-card,codec"); if (np) ret = asoc_simple_card_sub_parse_of(np, priv->daifmt, - &priv->codec_dai, + codec_dai, &dai_link->codec_of_node, &dai_link->codec_dai_name); if (ret < 0) return ret; + /* + * overwrite cpu_dai->fmt as its DAIFMT_MASTER bit is based on CODEC + * while the other bits should be identical unless buggy SW/HW design. + */ + cpu_dai->fmt = codec_dai->fmt; + if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name) return -EINVAL; @@ -227,12 +235,12 @@ static int asoc_simple_card_parse_of(struct device_node *node, dev_dbg(dev, "platform : %04x\n", priv->daifmt); dev_dbg(dev, "cpu : %s / %04x / %d\n", dai_link->cpu_dai_name, - priv->cpu_dai.fmt, - priv->cpu_dai.sysclk); + cpu_dai->fmt, + cpu_dai->sysclk); dev_dbg(dev, "codec : %s / %04x / %d\n", dai_link->codec_dai_name, - priv->codec_dai.fmt, - priv->codec_dai.sysclk); + codec_dai->fmt, + codec_dai->sysclk); /* * soc_bind_dai_link() will check cpu name
The current simple-card driver separates the daimft for cpu_dai and codec_dai. So we might get different values for them (0x4003 and 0x1003 for example): asoc-simple-card sound-cs42888.12: cpu : 2024000.esai / 4003 / 132000000 asoc-simple-card sound-cs42888.12: codec : cs42888 / 1003 / 24576000 asoc-simple-card sound-cs42888.12: cs42888 <-> 2024000.esai mapping ok This is not allowed at all as we need to keep the DAIFMT settings identical for both the ends of the link. Thus this patch fixes it by overwriting the cpu_dai->fmt with codec_dai->fmt since we defined the DAIFMT_MASTER basing on CODEC at the first place while the other bits are same. Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com> --- Changelog v2: * Keep the fmt identical for both CPU and CODEC sides. * Appended warning to binding doc. .../devicetree/bindings/sound/simple-card.txt | 6 ++++++ sound/soc/generic/simple-card.c | 20 ++++++++++++++------ 2 files changed, 20 insertions(+), 6 deletions(-)