[alsa-devel] [PATCH 0/7] ASoC:davinci-mcasp: bc polarity fix and proper format support
Hi,
The bit clock polarity has been configured incorrectly in the McASP driver for a long time. IB_NF, NB_IF and IB_IF was not correct on the receive side since they were selecting the same edge as we used for the tx.
The driver only had support for DSP_B mode (and probably AC97, but I can not test that). All other formats were broken (DSP_A, I2S, LEFT_J, etc). With this series the following formats will be supported by the McASP driver: - DSP_B - AC97 - DSP_A (new) - I2S (new) - LEFT_J (new)
Non supported modes will return error from now on.
Regards, Peter --- Peter Ujfalusi (7): ASoC: davinci-mcasp: Fix bit clock polarity settings ASoC: davinci-mcasp: Format data delay configuration enhancement ASoC: davinci-mcasp: Support for DSP_A format ASoC: davinci-mcasp: Move the FS polarity change out from the switch case ASoC: davinci-mcasp: Add support for I2S format ASoC: davinci-mcasp: Support for LEFT_J format ASoC: davinci-mcasp: Remove excess empty lines from davinci_mcasp_set_dai_fmt()
sound/soc/davinci/davinci-mcasp.c | 76 +++++++++++++++++++++++++-------------- 1 file changed, 50 insertions(+), 26 deletions(-)
IB_NF, NB_IF and IB_IF configured the bc polarity incorrectly. The receive polarity was set to the same edge as the TX in these cases.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index aae7d69af699..95b38f74e2af 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -331,7 +331,7 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL);
- mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); + mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); break;
@@ -339,7 +339,7 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL);
- mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); + mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); break;
@@ -347,7 +347,7 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL);
- mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); + mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); break;
On Fri, Apr 04, 2014 at 02:31:41PM +0300, Peter Ujfalusi wrote:
IB_NF, NB_IF and IB_IF configured the bc polarity incorrectly. The receive polarity was set to the same edge as the TX in these cases.
Applied, thanks. I'll apply the rest after -rc1 comes out since there's an interdependency between this fix and the rest and I don't want to rebase at the minute.
Use intermediate variable for the data delay needed for the specific format and write the register after the format configuration at once. This will help to control the number of lines as support for more formats going to be added. Also fixes a case when we switch between two formats with different delay requirements.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 14 +++++++++++--- 1 file changed, 11 insertions(+), 3 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 95b38f74e2af..d9ea709cb6d0 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -264,6 +264,7 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, { struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(cpu_dai); int ret = 0; + u32 data_delay;
pm_runtime_get_sync(mcasp->dev); switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { @@ -271,18 +272,25 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, case SND_SOC_DAIFMT_AC97: mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR); + + /* No delay after FS */ + data_delay = 0; break; default: /* configure a full-word SYNC pulse (LRCLK) */ mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR);
- /* make 1st data bit occur one ACLK cycle after the frame sync */ - mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, FSXDLY(1)); - mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMT_REG, FSRDLY(1)); + /* 1st data bit occur one ACLK cycle after the frame sync */ + data_delay = 1; break; }
+ mcasp_mod_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, FSXDLY(data_delay), + FSXDLY(3)); + mcasp_mod_bits(mcasp, DAVINCI_MCASP_RXFMT_REG, FSRDLY(data_delay), + FSRDLY(3)); + switch (fmt & SND_SOC_DAIFMT_MASTER_MASK) { case SND_SOC_DAIFMT_CBS_CFS: /* codec is clock and frame slave */
DSP_A is like DSP_B mode but with one bit delay after the FS.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index d9ea709cb6d0..7076dba66031 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -268,6 +268,13 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai,
pm_runtime_get_sync(mcasp->dev); switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { + case SND_SOC_DAIFMT_DSP_A: + mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); + mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR); + + /* 1st data bit occur one ACLK cycle after the frame sync */ + data_delay = 1; + break; case SND_SOC_DAIFMT_DSP_B: case SND_SOC_DAIFMT_AC97: mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR);
FS polarity can be either rising or falling edge in McASP. Instead of accessing the registers in every switch/case set a flag and write the registers after the switch for the invert configuration. This change will help when adding support for more formats also.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 27 ++++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 7076dba66031..3f67e374829e 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -265,6 +265,7 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, struct davinci_mcasp *mcasp = snd_soc_dai_get_drvdata(cpu_dai); int ret = 0; u32 data_delay; + bool fs_pol_rising;
pm_runtime_get_sync(mcasp->dev); switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { @@ -344,39 +345,39 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, switch (fmt & SND_SOC_DAIFMT_INV_MASK) { case SND_SOC_DAIFMT_IB_NF: mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); - mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); - mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); - mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); + fs_pol_rising = true; break;
case SND_SOC_DAIFMT_NB_IF: mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); - mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); - mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); - mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); + fs_pol_rising = false; break;
case SND_SOC_DAIFMT_IB_IF: mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); - mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); - mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); - mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); + fs_pol_rising = false; break;
case SND_SOC_DAIFMT_NB_NF: mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); - mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); - mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); - mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); + fs_pol_rising = true; break;
default: ret = -EINVAL; - break; + goto out; + } + + if (fs_pol_rising) { + mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); + mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); + } else { + mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); + mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL); } out: pm_runtime_put_sync(mcasp->dev);
The FS needs to be inverted in McASP compared to other supported formats. Use a flag to indicate if the FS needs to be inverted. At the same time fail when non supported format is asked since the default case was anyways configuring McASP to a not valid format.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index 3f67e374829e..af6cca3e61e4 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -266,6 +266,7 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, int ret = 0; u32 data_delay; bool fs_pol_rising; + bool inv_fs = false;
pm_runtime_get_sync(mcasp->dev); switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) { @@ -284,14 +285,19 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, /* No delay after FS */ data_delay = 0; break; - default: + case SND_SOC_DAIFMT_I2S: /* configure a full-word SYNC pulse (LRCLK) */ mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR);
/* 1st data bit occur one ACLK cycle after the frame sync */ data_delay = 1; + /* FS need to be inverted */ + inv_fs = true; break; + default: + ret = -EINVAL; + goto out; }
mcasp_mod_bits(mcasp, DAVINCI_MCASP_TXFMT_REG, FSXDLY(data_delay), @@ -372,6 +378,9 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, goto out; }
+ if (inv_fs) + fs_pol_rising = !fs_pol_rising; + if (fs_pol_rising) { mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXPOL); mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRPOL);
Configuration for LEFT_J format.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 7 +++++++ 1 file changed, 7 insertions(+)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index af6cca3e61e4..da9533b8e2dd 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -295,6 +295,13 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, /* FS need to be inverted */ inv_fs = true; break; + case SND_SOC_DAIFMT_LEFT_J: + /* configure a full-word SYNC pulse (LRCLK) */ + mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); + mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR); + /* No delay after FS */ + data_delay = 0; + break; default: ret = -EINVAL; goto out;
To make the code look uniform.
Signed-off-by: Peter Ujfalusi peter.ujfalusi@ti.com --- sound/soc/davinci/davinci-mcasp.c | 8 -------- 1 file changed, 8 deletions(-)
diff --git a/sound/soc/davinci/davinci-mcasp.c b/sound/soc/davinci/davinci-mcasp.c index da9533b8e2dd..ce0e55a4304c 100644 --- a/sound/soc/davinci/davinci-mcasp.c +++ b/sound/soc/davinci/davinci-mcasp.c @@ -273,7 +273,6 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, case SND_SOC_DAIFMT_DSP_A: mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR); - /* 1st data bit occur one ACLK cycle after the frame sync */ data_delay = 1; break; @@ -281,7 +280,6 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, case SND_SOC_DAIFMT_AC97: mcasp_clr_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); mcasp_clr_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR); - /* No delay after FS */ data_delay = 0; break; @@ -289,7 +287,6 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, /* configure a full-word SYNC pulse (LRCLK) */ mcasp_set_bits(mcasp, DAVINCI_MCASP_TXFMCTL_REG, FSXDUR); mcasp_set_bits(mcasp, DAVINCI_MCASP_RXFMCTL_REG, FSRDUR); - /* 1st data bit occur one ACLK cycle after the frame sync */ data_delay = 1; /* FS need to be inverted */ @@ -349,7 +346,6 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, ACLKX | AHCLKX | AFSX | ACLKR | AHCLKR | AFSR); mcasp->bclk_master = 0; break; - default: ret = -EINVAL; goto out; @@ -361,25 +357,21 @@ static int davinci_mcasp_set_dai_fmt(struct snd_soc_dai *cpu_dai, mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); fs_pol_rising = true; break; - case SND_SOC_DAIFMT_NB_IF: mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); fs_pol_rising = false; break; - case SND_SOC_DAIFMT_IB_IF: mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); mcasp_clr_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); fs_pol_rising = false; break; - case SND_SOC_DAIFMT_NB_NF: mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKXCTL_REG, ACLKXPOL); mcasp_set_bits(mcasp, DAVINCI_MCASP_ACLKRCTL_REG, ACLKRPOL); fs_pol_rising = true; break; - default: ret = -EINVAL; goto out;
Hi Peter,
On 04/04/2014 01:31 PM, Peter Ujfalusi wrote:
The bit clock polarity has been configured incorrectly in the McASP driver for a long time. IB_NF, NB_IF and IB_IF was not correct on the receive side since they were selecting the same edge as we used for the tx.
The driver only had support for DSP_B mode (and probably AC97, but I can not test that). All other formats were broken (DSP_A, I2S, LEFT_J, etc).
Well, we're using this driver in I2S mode for a number of devices since a while, so the above statement is not entirely true :)
The cleanups look sane, though. I can test them on AM33xx based hardware early next week. If you don't want to hold them off until then, no problem. I can also send fixups in case I spot a regression.
Out of interest: which hardware and which dai format are you testing this with?
Thanks, Daniel
Hi Daniel,
On 04/04/2014 04:24 PM, Daniel Mack wrote:
Hi Peter,
On 04/04/2014 01:31 PM, Peter Ujfalusi wrote:
The bit clock polarity has been configured incorrectly in the McASP driver for a long time. IB_NF, NB_IF and IB_IF was not correct on the receive side since they were selecting the same edge as we used for the tx.
The driver only had support for DSP_B mode (and probably AC97, but I can not test that). All other formats were broken (DSP_A, I2S, LEFT_J, etc).
Well, we're using this driver in I2S mode for a number of devices since a while, so the above statement is not entirely true :)
True, you could select I2S and ask for inverted frame sync with the old code. However if you ask for DSP_A, LEFT_J, etc it was doing the wrong thing.
The cleanups look sane, though. I can test them on AM33xx based hardware early next week. If you don't want to hold them off until then, no problem. I can also send fixups in case I spot a regression.
That would be great if you could also test these. It seams you have quite good array of codecs available.
Out of interest: which hardware and which dai format are you testing this with?
I have one AM335x board, one AM437x and one DRA7 where I can test right now. I just go one OMAP-l138 board which I need to set up. It is good for legacy (non DT boot) debugging.
Thanks, Daniel
Hi Peter,
On 04/04/2014 06:38 PM, Peter Ujfalusi wrote:
On 04/04/2014 04:24 PM, Daniel Mack wrote:
The cleanups look sane, though. I can test them on AM33xx based hardware early next week. If you don't want to hold them off until then, no problem. I can also send fixups in case I spot a regression.
That would be great if you could also test these. It seams you have quite good array of codecs available.
I tested this series with a CS4271 codec in I2S mode, and things still work as expected, except for the fact that the LRCLK is now inverted in comparison to the old state.
After some debugging, however, I have confidence that the behaviour with the patches applied is in fact correct, we just need to flip the bits in our ASoC machine driver, which is no big deal.
Thanks for the cleanups again! Daniel
Hi Daniel,
On 04/09/2014 03:12 PM, Daniel Mack wrote:
Hi Peter,
On 04/04/2014 06:38 PM, Peter Ujfalusi wrote:
On 04/04/2014 04:24 PM, Daniel Mack wrote:
The cleanups look sane, though. I can test them on AM33xx based hardware early next week. If you don't want to hold them off until then, no problem. I can also send fixups in case I spot a regression.
That would be great if you could also test these. It seams you have quite good array of codecs available.
I tested this series with a CS4271 codec in I2S mode, and things still work as expected, except for the fact that the LRCLK is now inverted in comparison to the old state.
Yes, this is expected. I think you were asking for NB_IF with the old code, right? The old code was - wrongly IMHO - configured the I2S mode's FS in inverted mode: NB_NF would end up to send the left channel when the FS is high. This is not correct. NB_NF means in I2S is that left channel is when FS is low, right channel is when FS is high. Most of the daVinci device used DSP_B, which was correct in terms of FS. The NB_IF with cs4271 was fine since the codec driver did not checked for invert it only cares about the protocol. Page 20 of the cs4271 shows the I2S. With this series we can use the correct DAIFMT_I2S + NB_NF with standard compliant codecs.
After some debugging, however, I have confidence that the behaviour with the patches applied is in fact correct, we just need to flip the bits in our ASoC machine driver, which is no big deal.
Thanks for the cleanups again!
Thanks for testing it!
On Fri, Apr 04, 2014 at 02:31:40PM +0300, Peter Ujfalusi wrote:
Hi,
The bit clock polarity has been configured incorrectly in the McASP driver for a long time. IB_NF, NB_IF and IB_IF was not correct on the receive side since they were selecting the same edge as we used for the tx.
Applied the rest, thanks.
participants (3)
-
Daniel Mack -
Mark Brown -
Peter Ujfalusi