* [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
@ 2014-03-11 12:54 Nicolin Chen
2014-03-11 13:25 ` Mark Brown
2014-03-12 1:32 ` [alsa-devel] " Kuninori Morimoto
0 siblings, 2 replies; 8+ messages in thread
From: Nicolin Chen @ 2014-03-11 12:54 UTC (permalink / raw)
To: broonie
Cc: alsa-devel, linux-kernel, kuninori.morimoto.gx, moinejf,
Li.Xiubo, lgirdwood
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
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-11 12:54 [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt Nicolin Chen
@ 2014-03-11 13:25 ` Mark Brown
2014-03-12 2:01 ` Nicolin Chen
2014-03-12 1:32 ` [alsa-devel] " Kuninori Morimoto
1 sibling, 1 reply; 8+ messages in thread
From: Mark Brown @ 2014-03-11 13:25 UTC (permalink / raw)
To: Nicolin Chen
Cc: alsa-devel, linux-kernel, kuninori.morimoto.gx, moinejf,
Li.Xiubo, lgirdwood, Jyri Sarha
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-11 13:25 ` Mark Brown
@ 2014-03-12 2:01 ` Nicolin Chen
0 siblings, 0 replies; 8+ messages in thread
From: Nicolin Chen @ 2014-03-12 2:01 UTC (permalink / raw)
To: Mark Brown
Cc: alsa-devel, linux-kernel, kuninori.morimoto.gx, moinejf,
Li.Xiubo, lgirdwood, Jyri Sarha
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-11 12:54 [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt Nicolin Chen
2014-03-11 13:25 ` Mark Brown
@ 2014-03-12 1:32 ` Kuninori Morimoto
2014-03-12 1:49 ` Mark Brown
1 sibling, 1 reply; 8+ messages in thread
From: Kuninori Morimoto @ 2014-03-12 1:32 UTC (permalink / raw)
To: Nicolin Chen
Cc: broonie, moinejf, alsa-devel, kuninori.morimoto.gx, linux-kernel,
lgirdwood, Li.Xiubo
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 ?
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-12 1:32 ` [alsa-devel] " Kuninori Morimoto
@ 2014-03-12 1:49 ` Mark Brown
2014-03-12 3:36 ` Kuninori Morimoto
0 siblings, 1 reply; 8+ messages in thread
From: Mark Brown @ 2014-03-12 1:49 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: Nicolin Chen, moinejf, alsa-devel, kuninori.morimoto.gx,
linux-kernel, lgirdwood, Li.Xiubo
[-- 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 --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-12 1:49 ` Mark Brown
@ 2014-03-12 3:36 ` Kuninori Morimoto
2014-03-12 3:26 ` Nicolin Chen
0 siblings, 1 reply; 8+ messages in thread
From: Kuninori Morimoto @ 2014-03-12 3:36 UTC (permalink / raw)
To: Mark Brown
Cc: Nicolin Chen, moinejf, alsa-devel, kuninori.morimoto.gx,
linux-kernel, lgirdwood, Li.Xiubo
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,
},
};
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-12 3:36 ` Kuninori Morimoto
@ 2014-03-12 3:26 ` Nicolin Chen
2014-03-12 4:01 ` Kuninori Morimoto
0 siblings, 1 reply; 8+ messages in thread
From: Nicolin Chen @ 2014-03-12 3:26 UTC (permalink / raw)
To: Kuninori Morimoto
Cc: Mark Brown, moinejf, alsa-devel, kuninori.morimoto.gx,
linux-kernel, lgirdwood, Li.Xiubo
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [alsa-devel] [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt
2014-03-12 3:26 ` Nicolin Chen
@ 2014-03-12 4:01 ` Kuninori Morimoto
0 siblings, 0 replies; 8+ messages in thread
From: Kuninori Morimoto @ 2014-03-12 4:01 UTC (permalink / raw)
To: Nicolin Chen
Cc: Mark Brown, moinejf, alsa-devel, kuninori.morimoto.gx,
linux-kernel, lgirdwood, Li.Xiubo
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.
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-03-12 4:01 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-03-11 12:54 [PATCH] ASoC: simple-card: overwrite DAIFMT_MASTER of cpu_dai->fmt Nicolin Chen
2014-03-11 13:25 ` Mark Brown
2014-03-12 2:01 ` Nicolin Chen
2014-03-12 1:32 ` [alsa-devel] " Kuninori Morimoto
2014-03-12 1:49 ` Mark Brown
2014-03-12 3:36 ` Kuninori Morimoto
2014-03-12 3:26 ` Nicolin Chen
2014-03-12 4:01 ` Kuninori Morimoto
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).