[PATCH] ASoC: mxs-saif: Fix channel swap for 24-bit format

[PATCH] ASoC: mxs-saif: Fix channel swap for 24-bit format

[Fabio Estevam]

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;
-- 
1.7.9.5

Re: [PATCH] ASoC: mxs-saif: Fix channel swap for 24-bit format

[Dong Aisheng]

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
>

Re: [PATCH] ASoC: mxs-saif: Fix channel swap for 24-bit format

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 1021 bytes --]

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!

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

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH] ASoC: mxs-saif: Fix channel swap for 24-bit format

[Dong Aisheng]

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