| Message ID | 1394542472-16580-1-git-send-email-Guangyu.Chen@freescale.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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 ?
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.
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
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, }, };
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.
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
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(-)