[alsa-devel] [PATCH] ASoC: mxs-saif: Fix channel swap for 24-bit format
From: Fabio Estevam fabio.estevam@freescale.com
Playing 24-bit format file leads to channel swap on mx28 and the reason is that the current driver performs one write/read to/from the SAIF_DATA register to trigger the transfer.
This approach works fine for S16_LE case because SAIF_DATA is a 32-bit register and thus is capable of storing the 16-bit left and right channels, but for the S24_LE case it can only store one channel, so in order to not lose the FIFO sync an extra read/write is needed.
Reported-by: Dan Winner DWinner@tc-helicon.com Signed-off-by: Fabio Estevam fabio.estevam@freescale.com Tested-by: Dan Winner DWinner@tc-helicon.com --- sound/soc/mxs/mxs-saif.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index aa037b2..05a875c 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -523,16 +523,22 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd,
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { /* - * write a data to saif data register to trigger - * the transfer + * write data to saif data register to trigger + * the transfer. + * For 24-bit format the 32-bit FIFO register stores + * only one channel, so we need to write twice. */ __raw_writel(0, saif->base + SAIF_DATA); + __raw_writel(0, saif->base + SAIF_DATA); } else { /* - * read a data from saif data register to trigger - * the receive + * read data from saif data register to trigger + * the receive. + * For 24-bit format the 32-bit FIFO register stores + * only one channel, so we need to read twice. */ __raw_readl(saif->base + SAIF_DATA); + __raw_readl(saif->base + SAIF_DATA); }
master_saif->ongoing = 1;
Hi Fabio,
On 31 October 2012 07:07, Fabio Estevam festevam@gmail.com wrote:
From: Fabio Estevam fabio.estevam@freescale.com
Playing 24-bit format file leads to channel swap on mx28 and the reason is that the current driver performs one write/read to/from the SAIF_DATA register to trigger the transfer.
This approach works fine for S16_LE case because SAIF_DATA is a 32-bit register and thus is capable of storing the 16-bit left and right channels, but for the S24_LE case it can only store one channel, so in order to not lose the FIFO sync an extra read/write is needed.
Reported-by: Dan Winner DWinner@tc-helicon.com Signed-off-by: Fabio Estevam fabio.estevam@freescale.com Tested-by: Dan Winner DWinner@tc-helicon.com
sound/soc/mxs/mxs-saif.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c index aa037b2..05a875c 100644 --- a/sound/soc/mxs/mxs-saif.c +++ b/sound/soc/mxs/mxs-saif.c @@ -523,16 +523,22 @@ static int mxs_saif_trigger(struct snd_pcm_substream *substream, int cmd,
if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) { /*
* write a data to saif data register to trigger* the transfer
* write data to saif data register to trigger* the transfer.* For 24-bit format the 32-bit FIFO register stores* only one channel, so we need to write twice. */ __raw_writel(0, saif->base + SAIF_DATA);__raw_writel(0, saif->base + SAIF_DATA);
This probably could a workaround for the customer, but i'm not sure Mark could accept it since the code is a bit confusing. (Mark, what's your suggestion on this?) Maybe the correct way to do is to get rid of this write for playback and the read for capture, since I did not see any place in datasheet to specify we must write/read to trigger the transfer, But the test result seems we indeed need this to trigger. And this code also exists in Freescale internal BSP for a long time. Maybe we need spend some more time to dig into why the trigger is needed, however, one bad thing is that the original owner of this IC module has left Freescale, i have no one to ask about hw details now.
Regards Dong Aisheng
} else { /*
* read a data from saif data register to trigger* the receive
* read data from saif data register to trigger* the receive.* For 24-bit format the 32-bit FIFO register stores* only one channel, so we need to read twice. */ __raw_readl(saif->base + SAIF_DATA);__raw_readl(saif->base + SAIF_DATA); } master_saif->ongoing = 1;-- 1.7.9.5
On Wed, Oct 31, 2012 at 04:50:50PM +0800, Dong Aisheng wrote:
On 31 October 2012 07:07, Fabio Estevam festevam@gmail.com wrote:
* write data to saif data register to trigger* the transfer.* For 24-bit format the 32-bit FIFO register stores* only one channel, so we need to write twice. */ __raw_writel(0, saif->base + SAIF_DATA);__raw_writel(0, saif->base + SAIF_DATA);
This probably could a workaround for the customer, but i'm not sure Mark could accept it since the code is a bit confusing. (Mark, what's your suggestion on this?)
I'm not really concerned about the code, the comment is fairly clear except that it'd be good to mention that this is safe for data sizes other than 24 bit. It's certainly really much more confusing than the existing code and if it works better that's obviously good!
On 1 November 2012 20:11, Mark Brown broonie@opensource.wolfsonmicro.com wrote:
On Wed, Oct 31, 2012 at 04:50:50PM +0800, Dong Aisheng wrote:
On 31 October 2012 07:07, Fabio Estevam festevam@gmail.com wrote:
* write data to saif data register to trigger* the transfer.* For 24-bit format the 32-bit FIFO register stores* only one channel, so we need to write twice. */ __raw_writel(0, saif->base + SAIF_DATA);__raw_writel(0, saif->base + SAIF_DATA);This probably could a workaround for the customer, but i'm not sure Mark could accept it since the code is a bit confusing. (Mark, what's your suggestion on this?)
I'm not really concerned about the code, the comment is fairly clear except that it'd be good to mention that this is safe for data sizes other than 24 bit. It's certainly really much more confusing than the existing code and if it works better that's obviously good!
If you're fine with it, i'm also ok. This probably is a suitable solution for now since finding out the strange IC behavior of this issue may need much longer time without IC people's help and it's still not on the plan.
Fabio, You can update the patch with Mark's suggestion and resend it.
Regards Dong Aisheng
participants (3)
-
Dong Aisheng -
Fabio Estevam -
Mark Brown