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

[Dong Aisheng]

Hi Fabio,

On 31 October 2012 07:07, Fabio Estevam <festevam at gmail.com> wrote:
> From: Fabio Estevam <fabio.estevam at 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 at tc-helicon.com>
> Signed-off-by: Fabio Estevam <fabio.estevam at freescale.com>
> Tested-by: Dan Winner <DWinner at 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
>