[PATCH v2] ASoC: simple-card: overwrite cpu_dai->fmt with codec_dai->fmt

[PATCH v2] ASoC: simple-card: overwrite cpu_dai->fmt with codec_dai->fmt

[Nicolin Chen]

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(-)

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
-- 
1.8.4

Re: [PATCH v2] ASoC: simple-card: overwrite cpu_dai->fmt with codec_dai->fmt

[Jyri Sarha]

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

Re: [alsa-devel] [PATCH v2] ASoC: simple-card: overwrite cpu_dai->fmt with codec_dai->fmt

[Nicolin Chen]

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-KZfg59tc24xl57MIdRCFDg@public.gmane.org>
> >---
> 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


--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [alsa-devel] [PATCH v2] ASoC: simple-card: overwrite cpu_dai->fmt with codec_dai->fmt

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1209 bytes --]

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?

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH v2] ASoC: simple-card: overwrite cpu_dai->fmt with codec_dai->fmt

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 464 bytes --]

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.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]