[PATCH v2 1/2] ASoC: mxs-saif: Fix channel swap for 24-bit format

[PATCH v2 1/2] ASoC: mxs-saif: Fix channel swap for 24-bit format

[Fabio Estevam]

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>
---
Changes since v1:
- Clarify that the extra read/write is also safe for non 24-bit formats.

 sound/soc/mxs/mxs-saif.c |   16 ++++++++++++----
 1 file changed, 12 insertions(+), 4 deletions(-)

diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index aa037b2..0eb2883 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -523,16 +523,24 @@ 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.
+			 * This is also safe for the other non 24-bit formats.
 			 */
 			__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.
+			 * This is also safe for the other non 24-bit formats.
 			 */
 			__raw_readl(saif->base + SAIF_DATA);
+			__raw_readl(saif->base + SAIF_DATA);
 		}
 
 		master_saif->ongoing = 1;
-- 
1.7.9.5

[PATCH v4 2/2] ASoC: mxs-saif: Remove platform data

[Fabio Estevam]

All MXS users have been converted to device tree and the board files have been
removed.

No need to keep platform data in the driver.
  
Signed-off-by: Fabio Estevam <fabio.estevam@freescale.com>
Acked-by: Dong Aisheng <dong.aisheng@linaro.org>
Acked-by: Shawn Guo <shawn.guo@linaro.org>
---
Changes since v3:
- Remove pdev->id check
Changes since v2:
- Also remove 'if (np)' as dt is the only probing mechanism.
Changes since v1:
- Also remove mxs_saif_platform_data and sound/saif.h, which was used only
to include mxs_saif_platform_data definition.
 include/sound/saif.h     |   16 ----------------
 sound/soc/mxs/mxs-saif.c |   44 ++++++++++++++++----------------------------
 2 files changed, 16 insertions(+), 44 deletions(-)
 delete mode 100644 include/sound/saif.h

diff --git a/include/sound/saif.h b/include/sound/saif.h
deleted file mode 100644
index f22f3e1..0000000
diff --git a/sound/soc/mxs/mxs-saif.c b/sound/soc/mxs/mxs-saif.c
index c294fbb..05dbadb 100644
--- a/sound/soc/mxs/mxs-saif.c
+++ b/sound/soc/mxs/mxs-saif.c
@@ -32,7 +32,6 @@
 #include <sound/pcm.h>
 #include <sound/pcm_params.h>
 #include <sound/soc.h>
-#include <sound/saif.h>
 #include <asm/mach-types.h>
 #include <mach/hardware.h>
 #include <mach/mxs.h>
@@ -660,43 +659,32 @@ static int __devinit mxs_saif_probe(struct platform_device *pdev)
 	struct device_node *np = pdev->dev.of_node;
 	struct resource *iores, *dmares;
 	struct mxs_saif *saif;
-	struct mxs_saif_platform_data *pdata;
 	struct pinctrl *pinctrl;
 	int ret = 0;
+	struct device_node *master;
 
-
-	if (!np && pdev->id >= ARRAY_SIZE(mxs_saif))
+	if (!np)
 		return -EINVAL;
 
 	saif = devm_kzalloc(&pdev->dev, sizeof(*saif), GFP_KERNEL);
 	if (!saif)
 		return -ENOMEM;
 
-	if (np) {
-		struct device_node *master;
-		saif->id = of_alias_get_id(np, "saif");
-		if (saif->id < 0)
-			return saif->id;
-		/*
-		 * If there is no "fsl,saif-master" phandle, it's a saif
-		 * master.  Otherwise, it's a slave and its phandle points
-		 * to the master.
-		 */
-		master = of_parse_phandle(np, "fsl,saif-master", 0);
-		if (!master) {
-			saif->master_id = saif->id;
-		} else {
-			saif->master_id = of_alias_get_id(master, "saif");
-			if (saif->master_id < 0)
-				return saif->master_id;
-		}
+	saif->id = of_alias_get_id(np, "saif");
+	if (saif->id < 0)
+		return saif->id;
+	/*
+	 * If there is no "fsl,saif-master" phandle, it's a saif
+	 * master.  Otherwise, it's a slave and its phandle points
+	 * to the master.
+	 */
+	master = of_parse_phandle(np, "fsl,saif-master", 0);
+	if (!master) {
+		saif->master_id = saif->id;
 	} else {
-		saif->id = pdev->id;
-		pdata = pdev->dev.platform_data;
-		if (pdata && !pdata->master_mode)
-			saif->master_id = pdata->master_id;
-		else
-			saif->master_id = saif->id;
+		saif->master_id = of_alias_get_id(master, "saif");
+		if (saif->master_id < 0)
+			return saif->master_id;
 	}
 
 	if (saif->master_id < 0 || saif->master_id >= ARRAY_SIZE(mxs_saif)) {
-- 
1.7.9.5

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

[Dong Aisheng]

On 2 November 2012 01:57, Fabio Estevam <fabio.estevam@freescale.com> wrote:
> 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>

Acked-by: Dong Aisheng <dong.aisheng@linaro.org>

Regards
Dong Aisheng

Re: [PATCH v4 2/2] ASoC: mxs-saif: Remove platform data

[Mark Brown]

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

On Thu, Nov 01, 2012 at 03:57:12PM -0200, Fabio Estevam wrote:
> All MXS users have been converted to device tree and the board files have been
> removed.
> 
> No need to keep platform data in the driver.

This doesn't apply against v3.7-rc3 (or against my fix/mxs branch) -
please check what's going on.

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

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

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

[Mark Brown]

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

On Thu, Nov 01, 2012 at 03:57:11PM -0200, Fabio Estevam wrote:
> 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.

Applied, thanks.

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

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

Re: [PATCH v4 2/2] ASoC: mxs-saif: Remove platform data

[Fabio Estevam]

Hi Mark,

On Fri, Nov 2, 2012 at 1:03 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Thu, Nov 01, 2012 at 03:57:12PM -0200, Fabio Estevam wrote:
>> All MXS users have been converted to device tree and the board files have been
>> removed.
>>
>> No need to keep platform data in the driver.
>
> This doesn't apply against v3.7-rc3 (or against my fix/mxs branch) -
> please check what's going on.

fix/mxs branch misses commit 730963f81 (ASoC: mxs-saif: Use devm_clk_get()).

This patch applies cleanly against your for-next and also against linux-next.

Thanks,

Fabio Estevam

Re: [PATCH v4 2/2] ASoC: mxs-saif: Remove platform data

[Mark Brown]

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

On Tue, Nov 13, 2012 at 10:25:06AM -0200, Fabio Estevam wrote:

> fix/mxs branch misses commit 730963f81 (ASoC: mxs-saif: Use devm_clk_get()).

> This patch applies cleanly against your for-next and also against linux-next.

That commit appears to have been added somewhere prior to v3.6 so should
be in mainline now in which case it's surprising that it's not in a
branch based off v3.7-rc3, and indeed when I look at both the fix/mxs
branch and mainline I can see the commit.  This suggests that it isn't
the problem.

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

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