[PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_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

It's pretty fair to do it for DAIFMT_INV since two dais might have differnt
definitions in their drivers even though it'd be better to keep it same if
we could.

But for DAIFMT_MASTER bit, we should keep them identical. Otherwise, it'll
make one of dai work in an incorrect mode.

Thus this patch fixes it by overwriting the DAIFMT_MASTER bit of cpu_dai->fmt
with the one of codec_dai->fmt since we defined DAIFMT_MASTER basing on CODEC
at the first place.

Signed-off-by: Nicolin Chen <Guangyu.Chen@freescale.com>
---
 sound/soc/generic/simple-card.c | 18 ++++++++++++------
 1 file changed, 12 insertions(+), 6 deletions(-)

diff --git a/sound/soc/generic/simple-card.c b/sound/soc/generic/simple-card.c
index 5dd4769..f9a3c8a 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,16 @@ 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 DAIFMT_MASTER of cpu_dai since it's based on CODEC */
+	cpu_dai->fmt &= ~SND_SOC_DAIFMT_MASTER_MASK;
+	cpu_dai->fmt |= codec_dai->fmt & SND_SOC_DAIFMT_MASTER_MASK;
+
 	if (!dai_link->cpu_dai_name || !dai_link->codec_dai_name)
 		return -EINVAL;
 
@@ -227,12 +233,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] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Mark Brown]

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

On Tue, Mar 11, 2014 at 08:54:32PM +0800, Nicolin Chen wrote:

Adding Jyri who's been looking at this as well but not added anyone else
working on simple-card so you might've missed his mails.

> It's pretty fair to do it for DAIFMT_INV since two dais might have differnt
> definitions in their drivers even though it'd be better to keep it same if
> we could.

No, that's not at all OK - anything that requires this is broken.  The
same DAI format should be usable by both ends of the link unless the
board itself is inverting one of the signals or something.

> Thus this patch fixes it by overwriting the DAIFMT_MASTER bit of cpu_dai->fmt
> with the one of codec_dai->fmt since we defined DAIFMT_MASTER basing on CODEC
> at the first place.

This seems closer to what I'd expect for something like this but it does
mean that any format settings on the CPU DAI will be ignored (rather
than say warning or something).  I'm not sure this is a bad thing
though, probably wants the binding documenting at least.

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

Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Kuninori Morimoto]

Hi Nicolin

> 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

cpu   = 4003 : SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_LEFT_J
codec = 1003 : SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_LEFT_J

codec is master, cpu is slave...
what is problem ??
Am I misunderstanding ?

Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Mark Brown]

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

On Tue, Mar 11, 2014 at 06:32:32PM -0700, Kuninori Morimoto 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

> cpu   = 4003 : SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_LEFT_J
> codec = 1003 : SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_LEFT_J

> codec is master, cpu is slave...
> what is problem ??
> Am I misunderstanding ?

The C in those constants stands for CODEC and the values should be
identical for both ends of the link.

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

Re: [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Nicolin Chen]

On Tue, Mar 11, 2014 at 01:25:53PM +0000, Mark Brown wrote:
> On Tue, Mar 11, 2014 at 08:54:32PM +0800, Nicolin Chen wrote:
> 
> Adding Jyri who's been looking at this as well but not added anyone else
> working on simple-card so you might've missed his mails.
> 
> > It's pretty fair to do it for DAIFMT_INV since two dais might have differnt
> > definitions in their drivers even though it'd be better to keep it same if
> > we could.
> 
> No, that's not at all OK - anything that requires this is broken.  The
> same DAI format should be usable by both ends of the link unless the
> board itself is inverting one of the signals or something.

That makes sense to me. Okay, I'll overwrite them all...

> 
> > Thus this patch fixes it by overwriting the DAIFMT_MASTER bit of cpu_dai->fmt
> > with the one of codec_dai->fmt since we defined DAIFMT_MASTER basing on CODEC
> > at the first place.
> 
> This seems closer to what I'd expect for something like this but it does
> mean that any format settings on the CPU DAI will be ignored (rather
> than say warning or something).  I'm not sure this is a bad thing
> though, probably wants the binding documenting at least.

...along with the binding doc.

Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Nicolin Chen]

Hi Morimoto-san,

On Tue, Mar 11, 2014 at 08:36:22PM -0700, Kuninori Morimoto wrote:
> 
> Hi Mark
> 
> > > > 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
> > 
> > > cpu   = 4003 : SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_LEFT_J
> > > codec = 1003 : SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_LEFT_J
> > 
> > > codec is master, cpu is slave...
> > > what is problem ??
> > > Am I misunderstanding ?
> > 
> > The C in those constants stands for CODEC and the values should be
> > identical for both ends of the link.
> 
> Wow ! really ??
> Then, is this settiting wrong ??
> 
> ${LINUX}/arch/arm/mach-shmobile/board-armadillo800eva.c :: fsi_wm8978_info
> 
> static struct asoc_simple_card_info fsi_wm8978_info = {
> 	...
> 	.daifmt		= SND_SOC_DAIFMT_I2S,
> 	.cpu_dai = {
> 		...
> 		.fmt	= SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_IB_NF,
> 	},
> 	.codec_dai = {
> 		...
> 		.fmt	= SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_NB_NF,
> 	},
> };
> 
> It should be like this ?
> 
> static struct asoc_simple_card_info fsi_wm8978_info = {
> 	...
> 	.daifmt		= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM,
> 	.cpu_dai = {
> 		...
> 		.fmt	= SND_SOC_DAIFMT_IB_NF,
> 	},
> 	.codec_dai = {
> 		...
> 		.fmt	= SND_SOC_DAIFMT_NB_NF,
> 	},
> };

This would be better imo.

And ideally we should also keep the xB_xF identical like Mark said _identical_.
Just some cpu dai drivers might do an incorrect settings for it, like regarding
NB as sampling on rising edge and IF as active low (I'm saying this without a
careful check though), which results people need to re-set bitclock-invert and
frame-invert if they switch the DAI format from left_j to i2s for example.

Thank you,
Nicolin Chen

Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Kuninori Morimoto]

Hi Mark

> > > 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
> 
> > cpu   = 4003 : SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_LEFT_J
> > codec = 1003 : SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_LEFT_J
> 
> > codec is master, cpu is slave...
> > what is problem ??
> > Am I misunderstanding ?
> 
> The C in those constants stands for CODEC and the values should be
> identical for both ends of the link.

Wow ! really ??
Then, is this settiting wrong ??

${LINUX}/arch/arm/mach-shmobile/board-armadillo800eva.c :: fsi_wm8978_info

static struct asoc_simple_card_info fsi_wm8978_info = {
	...
	.daifmt		= SND_SOC_DAIFMT_I2S,
	.cpu_dai = {
		...
		.fmt	= SND_SOC_DAIFMT_CBS_CFS | SND_SOC_DAIFMT_IB_NF,
	},
	.codec_dai = {
		...
		.fmt	= SND_SOC_DAIFMT_CBM_CFM | SND_SOC_DAIFMT_NB_NF,
	},
};

It should be like this ?

static struct asoc_simple_card_info fsi_wm8978_info = {
	...
	.daifmt		= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM,
	.cpu_dai = {
		...
		.fmt	= SND_SOC_DAIFMT_IB_NF,
	},
	.codec_dai = {
		...
		.fmt	= SND_SOC_DAIFMT_NB_NF,
	},
};

Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt

[Kuninori Morimoto]

Hi Nicolin

> > static struct asoc_simple_card_info fsi_wm8978_info = {
> > 	...
> > 	.daifmt		= SND_SOC_DAIFMT_I2S | SND_SOC_DAIFMT_CBM_CFM,
> > 	.cpu_dai = {
> > 		...
> > 		.fmt	= SND_SOC_DAIFMT_IB_NF,
> > 	},
> > 	.codec_dai = {
> > 		...
> > 		.fmt	= SND_SOC_DAIFMT_NB_NF,
> > 	},
> > };
> 
> This would be better imo.
> 
> And ideally we should also keep the xB_xF identical like Mark said _identical_.
> Just some cpu dai drivers might do an incorrect settings for it, like regarding
> NB as sampling on rising edge and IF as active low (I'm saying this without a
> careful check though), which results people need to re-set bitclock-invert and
> frame-invert if they switch the DAI format from left_j to i2s for example.

Wow... I had misunderstood...
I need to send fixup patch after lunch.