[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padmavathi Venna]

Exynos5420 added support for I2S TDM mode. For this, there are some
register changes in the I2S controller. This patch adds the relevant
register changes to support I2S in normal mode. This patch adds a
quirk for TDM mode and if TDM mode is present all the relevent changes
will be applied.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
[abrestic: style cleanup and documentation]
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-on: https://gerrit-int.chromium.org/37840
Reviewed-by: Simon Glass <sjg@google.com>
---
 .../devicetree/bindings/sound/samsung-i2s.txt      |    2 +
 include/linux/platform_data/asoc-s3c.h             |    1 +
 sound/soc/samsung/i2s-regs.h                       |   51 ++++++---
 sound/soc/samsung/i2s.c                            |  117 ++++++++++++++++----
 4 files changed, 132 insertions(+), 39 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
index 025e66b..b8593d5 100644
--- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
@@ -28,6 +28,8 @@ Optional SoC Specific Properties:
   enabled or disabled based on need.
 - samsung,supports-secdai:If I2S block has a secondary FIFO and internal DMA,
   then this flag is enabled.
+- samsung,supports-tdm: If the I2S controller supports TDM, then this flag
+  must be enabled.
 - samsung,idma-addr: Internal DMA register base address of the audio
   sub system(used in secondary sound source).
 - pinctrl-0: Should specify pin control groups used for this controller.
diff --git a/include/linux/platform_data/asoc-s3c.h b/include/linux/platform_data/asoc-s3c.h
index 8827259..9efc04d 100644
--- a/include/linux/platform_data/asoc-s3c.h
+++ b/include/linux/platform_data/asoc-s3c.h
@@ -36,6 +36,7 @@ struct samsung_i2s {
  */
 #define QUIRK_NO_MUXPSR		(1 << 2)
 #define QUIRK_NEED_RSTCLR	(1 << 3)
+#define QUIRK_SUPPORTS_TDM	(1 << 4)
 	/* Quirks of the I2S controller */
 	u32 quirks;
 	dma_addr_t idma_addr;
diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
index c0e6d9a..821a502 100644
--- a/sound/soc/samsung/i2s-regs.h
+++ b/sound/soc/samsung/i2s-regs.h
@@ -31,6 +31,10 @@
 #define I2SLVL1ADDR	0x34
 #define I2SLVL2ADDR	0x38
 #define I2SLVL3ADDR	0x3c
+#define I2SSTR1		0x40
+#define I2SVER		0x44
+#define I2SFIC2		0x48
+#define I2STDM		0x4c
 
 #define CON_RSTCLR		(1 << 31)
 #define CON_FRXOFSTATUS		(1 << 26)
@@ -95,24 +99,39 @@
 #define MOD_RXONLY		(1 << 8)
 #define MOD_TXRX		(2 << 8)
 #define MOD_MASK		(3 << 8)
-#define MOD_LR_LLOW		(0 << 7)
-#define MOD_LR_RLOW		(1 << 7)
-#define MOD_SDF_IIS		(0 << 5)
-#define MOD_SDF_MSB		(1 << 5)
-#define MOD_SDF_LSB		(2 << 5)
-#define MOD_SDF_MASK		(3 << 5)
-#define MOD_RCLK_256FS		(0 << 3)
-#define MOD_RCLK_512FS		(1 << 3)
-#define MOD_RCLK_384FS		(2 << 3)
-#define MOD_RCLK_768FS		(3 << 3)
-#define MOD_RCLK_MASK		(3 << 3)
-#define MOD_BCLK_32FS		(0 << 1)
-#define MOD_BCLK_48FS		(1 << 1)
-#define MOD_BCLK_16FS		(2 << 1)
-#define MOD_BCLK_24FS		(3 << 1)
-#define MOD_BCLK_MASK		(3 << 1)
+#define MOD_LRP_SHIFT		7
+#define MOD_LR_LLOW		0
+#define MOD_LR_RLOW		1
+#define MOD_SDF_SHIFT		5
+#define MOD_SDF_IIS		0
+#define MOD_SDF_MSB		1
+#define MOD_SDF_LSB		2
+#define MOD_SDF_MASK		3
+#define MOD_RCLK_SHIFT		3
+#define MOD_RCLK_256FS		0
+#define MOD_RCLK_512FS		1
+#define MOD_RCLK_384FS		2
+#define MOD_RCLK_768FS		3
+#define MOD_RCLK_MASK		3
+#define MOD_BCLK_SHIFT		1
+#define MOD_BCLK_32FS		0
+#define MOD_BCLK_48FS		1
+#define MOD_BCLK_16FS		2
+#define MOD_BCLK_24FS		3
+#define MOD_BCLK_MASK		3
 #define MOD_8BIT		(1 << 0)
 
+#define EXYNOS5420_MOD_LRP_SHIFT	15
+#define EXYNOS5420_MOD_SDF_SHIFT	6
+#define EXYNOS5420_MOD_RCLK_SHIFT	4
+#define EXYNOS5420_MOD_BCLK_SHIFT	0
+#define EXYNOS5420_MOD_BCLK_64FS	4
+#define EXYNOS5420_MOD_BCLK_96FS	5
+#define EXYNOS5420_MOD_BCLK_128FS	6
+#define EXYNOS5420_MOD_BCLK_192FS	7
+#define EXYNOS5420_MOD_BCLK_256FS	8
+#define EXYNOS5420_MOD_BCLK_MASK	0xf
+
 #define MOD_CDCLKCON		(1 << 12)
 
 #define PSR_PSREN		(1 << 15)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 3fcf8d7..398f8db 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -198,7 +198,13 @@ static inline bool is_manager(struct i2s_dai *i2s)
 /* Read RCLK of I2S (in multiples of LRCLK) */
 static inline unsigned get_rfs(struct i2s_dai *i2s)
 {
-	u32 rfs = (readl(i2s->addr + I2SMOD) >> 3) & 0x3;
+	u32 rfs;
+
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
+		rfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_RCLK_SHIFT;
+	else
+		rfs = readl(i2s->addr + I2SMOD) >> MOD_RCLK_SHIFT;
+	rfs &= MOD_RCLK_MASK;
 
 	switch (rfs) {
 	case 3:	return 768;
@@ -212,21 +218,26 @@ static inline unsigned get_rfs(struct i2s_dai *i2s)
 static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs)
 {
 	u32 mod = readl(i2s->addr + I2SMOD);
+	int rfs_shift;
 
-	mod &= ~MOD_RCLK_MASK;
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
+		rfs_shift = EXYNOS5420_MOD_RCLK_SHIFT;
+	else
+		rfs_shift = MOD_RCLK_SHIFT;
+	mod &= ~(MOD_RCLK_MASK << rfs_shift);
 
 	switch (rfs) {
 	case 768:
-		mod |= MOD_RCLK_768FS;
+		mod |= (MOD_RCLK_768FS << rfs_shift);
 		break;
 	case 512:
-		mod |= MOD_RCLK_512FS;
+		mod |= (MOD_RCLK_512FS << rfs_shift);
 		break;
 	case 384:
-		mod |= MOD_RCLK_384FS;
+		mod |= (MOD_RCLK_384FS << rfs_shift);
 		break;
 	default:
-		mod |= MOD_RCLK_256FS;
+		mod |= (MOD_RCLK_256FS << rfs_shift);
 		break;
 	}
 
@@ -236,9 +247,22 @@ static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs)
 /* Read Bit-Clock of I2S (in multiples of LRCLK) */
 static inline unsigned get_bfs(struct i2s_dai *i2s)
 {
-	u32 bfs = (readl(i2s->addr + I2SMOD) >> 1) & 0x3;
+	u32 bfs;
+
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
+		bfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_BCLK_SHIFT;
+		bfs &= EXYNOS5420_MOD_BCLK_MASK;
+	} else {
+		bfs = readl(i2s->addr + I2SMOD) >> MOD_BCLK_SHIFT;
+		bfs &= MOD_BCLK_MASK;
+	}
 
 	switch (bfs) {
+	case 8: return 256;
+	case 7: return 192;
+	case 6: return 128;
+	case 5: return 96;
+	case 4: return 64;
 	case 3: return 24;
 	case 2: return 16;
 	case 1:	return 48;
@@ -250,21 +274,50 @@ static inline unsigned get_bfs(struct i2s_dai *i2s)
 static inline void set_bfs(struct i2s_dai *i2s, unsigned bfs)
 {
 	u32 mod = readl(i2s->addr + I2SMOD);
+	int bfs_shift;
+	int tdm = i2s->quirks & QUIRK_SUPPORTS_TDM;
 
-	mod &= ~MOD_BCLK_MASK;
+	if (tdm) {
+		bfs_shift = EXYNOS5420_MOD_BCLK_SHIFT;
+		mod &= ~(EXYNOS5420_MOD_BCLK_MASK << bfs_shift);
+	} else {
+		bfs_shift = MOD_BCLK_SHIFT;
+		mod &= ~(MOD_BCLK_MASK << bfs_shift);
+	}
+
+	/* Non-TDM I2S controllers do not support BCLK > 48 * FS */
+	if (!tdm && bfs > 48) {
+		dev_err(&i2s->pdev->dev, "Unsupported BCLK divider\n");
+		return;
+	}
 
 	switch (bfs) {
 	case 48:
-		mod |= MOD_BCLK_48FS;
+		mod |= (MOD_BCLK_48FS << bfs_shift);
 		break;
 	case 32:
-		mod |= MOD_BCLK_32FS;
+		mod |= (MOD_BCLK_32FS << bfs_shift);
 		break;
 	case 24:
-		mod |= MOD_BCLK_24FS;
+		mod |= (MOD_BCLK_24FS << bfs_shift);
 		break;
 	case 16:
-		mod |= MOD_BCLK_16FS;
+		mod |= (MOD_BCLK_16FS << bfs_shift);
+		break;
+	case 64:
+		mod |= (EXYNOS5420_MOD_BCLK_64FS << bfs_shift);
+		break;
+	case 96:
+		mod |= (EXYNOS5420_MOD_BCLK_96FS << bfs_shift);
+		break;
+	case 128:
+		mod |= (EXYNOS5420_MOD_BCLK_128FS << bfs_shift);
+		break;
+	case 192:
+		mod |= (EXYNOS5420_MOD_BCLK_192FS << bfs_shift);
+		break;
+	case 256:
+		mod |= (EXYNOS5420_MOD_BCLK_256FS << bfs_shift);
 		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Wrong BCLK Divider!\n");
@@ -492,19 +545,31 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
 	struct i2s_dai *i2s = to_info(dai);
 	u32 mod = readl(i2s->addr + I2SMOD);
 	u32 tmp = 0;
+	int lrp_shift, sdf_shift, sdf_mask, lrp_rlow;
+
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
+		lrp_shift = EXYNOS5420_MOD_LRP_SHIFT;
+		sdf_shift = EXYNOS5420_MOD_SDF_SHIFT;
+	} else {
+		lrp_shift = MOD_LRP_SHIFT;
+		sdf_shift = MOD_SDF_SHIFT;
+	}
+
+	sdf_mask = MOD_SDF_MASK << sdf_shift;
+	lrp_rlow = MOD_LR_RLOW << lrp_shift;
 
 	/* Format is priority */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_RIGHT_J:
-		tmp |= MOD_LR_RLOW;
-		tmp |= MOD_SDF_MSB;
+		tmp |= lrp_rlow;
+		tmp |= (MOD_SDF_MSB << sdf_shift);
 		break;
 	case SND_SOC_DAIFMT_LEFT_J:
-		tmp |= MOD_LR_RLOW;
-		tmp |= MOD_SDF_LSB;
+		tmp |= lrp_rlow;
+		tmp |= (MOD_SDF_LSB << sdf_shift);
 		break;
 	case SND_SOC_DAIFMT_I2S:
-		tmp |= MOD_SDF_IIS;
+		tmp |= (MOD_SDF_IIS << sdf_shift);
 		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Format not supported\n");
@@ -519,10 +584,10 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
 	case SND_SOC_DAIFMT_NB_NF:
 		break;
 	case SND_SOC_DAIFMT_NB_IF:
-		if (tmp & MOD_LR_RLOW)
-			tmp &= ~MOD_LR_RLOW;
+		if (tmp & lrp_rlow)
+			tmp &= ~lrp_rlow;
 		else
-			tmp |= MOD_LR_RLOW;
+			tmp |= lrp_rlow;
 		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Polarity not supported\n");
@@ -544,15 +609,18 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
 		return -EINVAL;
 	}
 
+	/*
+	 * Don't change the I2S mode if any controller is active on this
+	 * channel.
+	 */
 	if (any_active(i2s) &&
-			((mod & (MOD_SDF_MASK | MOD_LR_RLOW
-				| MOD_SLAVE)) != tmp)) {
+	    ((mod & (sdf_mask | lrp_rlow | MOD_SLAVE)) != tmp)) {
 		dev_err(&i2s->pdev->dev,
 				"%s:%d Other DAI busy\n", __func__, __LINE__);
 		return -EAGAIN;
 	}
 
-	mod &= ~(MOD_SDF_MASK | MOD_LR_RLOW | MOD_SLAVE);
+	mod &= ~(sdf_mask | lrp_rlow | MOD_SLAVE);
 	mod |= tmp;
 	writel(mod, i2s->addr + I2SMOD);
 
@@ -1170,6 +1238,9 @@ static int samsung_i2s_probe(struct platform_device *pdev)
 		if (of_find_property(np, "samsung,supports-rstclr", NULL))
 			quirks |= QUIRK_NEED_RSTCLR;
 
+		if (of_find_property(np, "samsung,supports-tdm", NULL))
+			quirks |= QUIRK_SUPPORTS_TDM;
+
 		if (of_property_read_u32(np, "samsung,idma-addr",
 					 &idma_addr)) {
 			if (quirks & QUIRK_SEC_DAI) {
-- 
1.7.4.4

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padmavathi Venna]

Exynos5420 added support for I2S TDM mode. For this, there are some
register changes in the I2S controller. This patch adds the relevant
register changes to support I2S in normal mode. This patch adds a
quirk for TDM mode and if TDM mode is present all the relevent changes
will be applied.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
[abrestic: style cleanup and documentation]
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-on: https://gerrit-int.chromium.org/37840
Reviewed-by: Simon Glass <sjg@google.com>
---
 .../devicetree/bindings/sound/samsung-i2s.txt      |    2 +
 include/linux/platform_data/asoc-s3c.h             |    1 +
 sound/soc/samsung/i2s-regs.h                       |   51 ++++++---
 sound/soc/samsung/i2s.c                            |  117 ++++++++++++++++----
 4 files changed, 132 insertions(+), 39 deletions(-)

diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
index 025e66b..b8593d5 100644
--- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
+++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
@@ -28,6 +28,8 @@ Optional SoC Specific Properties:
   enabled or disabled based on need.
 - samsung,supports-secdai:If I2S block has a secondary FIFO and internal DMA,
   then this flag is enabled.
+- samsung,supports-tdm: If the I2S controller supports TDM, then this flag
+  must be enabled.
 - samsung,idma-addr: Internal DMA register base address of the audio
   sub system(used in secondary sound source).
 - pinctrl-0: Should specify pin control groups used for this controller.
diff --git a/include/linux/platform_data/asoc-s3c.h b/include/linux/platform_data/asoc-s3c.h
index 8827259..9efc04d 100644
--- a/include/linux/platform_data/asoc-s3c.h
+++ b/include/linux/platform_data/asoc-s3c.h
@@ -36,6 +36,7 @@ struct samsung_i2s {
  */
 #define QUIRK_NO_MUXPSR		(1 << 2)
 #define QUIRK_NEED_RSTCLR	(1 << 3)
+#define QUIRK_SUPPORTS_TDM	(1 << 4)
 	/* Quirks of the I2S controller */
 	u32 quirks;
 	dma_addr_t idma_addr;
diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
index c0e6d9a..821a502 100644
--- a/sound/soc/samsung/i2s-regs.h
+++ b/sound/soc/samsung/i2s-regs.h
@@ -31,6 +31,10 @@
 #define I2SLVL1ADDR	0x34
 #define I2SLVL2ADDR	0x38
 #define I2SLVL3ADDR	0x3c
+#define I2SSTR1		0x40
+#define I2SVER		0x44
+#define I2SFIC2		0x48
+#define I2STDM		0x4c
 
 #define CON_RSTCLR		(1 << 31)
 #define CON_FRXOFSTATUS		(1 << 26)
@@ -95,24 +99,39 @@
 #define MOD_RXONLY		(1 << 8)
 #define MOD_TXRX		(2 << 8)
 #define MOD_MASK		(3 << 8)
-#define MOD_LR_LLOW		(0 << 7)
-#define MOD_LR_RLOW		(1 << 7)
-#define MOD_SDF_IIS		(0 << 5)
-#define MOD_SDF_MSB		(1 << 5)
-#define MOD_SDF_LSB		(2 << 5)
-#define MOD_SDF_MASK		(3 << 5)
-#define MOD_RCLK_256FS		(0 << 3)
-#define MOD_RCLK_512FS		(1 << 3)
-#define MOD_RCLK_384FS		(2 << 3)
-#define MOD_RCLK_768FS		(3 << 3)
-#define MOD_RCLK_MASK		(3 << 3)
-#define MOD_BCLK_32FS		(0 << 1)
-#define MOD_BCLK_48FS		(1 << 1)
-#define MOD_BCLK_16FS		(2 << 1)
-#define MOD_BCLK_24FS		(3 << 1)
-#define MOD_BCLK_MASK		(3 << 1)
+#define MOD_LRP_SHIFT		7
+#define MOD_LR_LLOW		0
+#define MOD_LR_RLOW		1
+#define MOD_SDF_SHIFT		5
+#define MOD_SDF_IIS		0
+#define MOD_SDF_MSB		1
+#define MOD_SDF_LSB		2
+#define MOD_SDF_MASK		3
+#define MOD_RCLK_SHIFT		3
+#define MOD_RCLK_256FS		0
+#define MOD_RCLK_512FS		1
+#define MOD_RCLK_384FS		2
+#define MOD_RCLK_768FS		3
+#define MOD_RCLK_MASK		3
+#define MOD_BCLK_SHIFT		1
+#define MOD_BCLK_32FS		0
+#define MOD_BCLK_48FS		1
+#define MOD_BCLK_16FS		2
+#define MOD_BCLK_24FS		3
+#define MOD_BCLK_MASK		3
 #define MOD_8BIT		(1 << 0)
 
+#define EXYNOS5420_MOD_LRP_SHIFT	15
+#define EXYNOS5420_MOD_SDF_SHIFT	6
+#define EXYNOS5420_MOD_RCLK_SHIFT	4
+#define EXYNOS5420_MOD_BCLK_SHIFT	0
+#define EXYNOS5420_MOD_BCLK_64FS	4
+#define EXYNOS5420_MOD_BCLK_96FS	5
+#define EXYNOS5420_MOD_BCLK_128FS	6
+#define EXYNOS5420_MOD_BCLK_192FS	7
+#define EXYNOS5420_MOD_BCLK_256FS	8
+#define EXYNOS5420_MOD_BCLK_MASK	0xf
+
 #define MOD_CDCLKCON		(1 << 12)
 
 #define PSR_PSREN		(1 << 15)
diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 3fcf8d7..398f8db 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -198,7 +198,13 @@ static inline bool is_manager(struct i2s_dai *i2s)
 /* Read RCLK of I2S (in multiples of LRCLK) */
 static inline unsigned get_rfs(struct i2s_dai *i2s)
 {
-	u32 rfs = (readl(i2s->addr + I2SMOD) >> 3) & 0x3;
+	u32 rfs;
+
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
+		rfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_RCLK_SHIFT;
+	else
+		rfs = readl(i2s->addr + I2SMOD) >> MOD_RCLK_SHIFT;
+	rfs &= MOD_RCLK_MASK;
 
 	switch (rfs) {
 	case 3:	return 768;
@@ -212,21 +218,26 @@ static inline unsigned get_rfs(struct i2s_dai *i2s)
 static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs)
 {
 	u32 mod = readl(i2s->addr + I2SMOD);
+	int rfs_shift;
 
-	mod &= ~MOD_RCLK_MASK;
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
+		rfs_shift = EXYNOS5420_MOD_RCLK_SHIFT;
+	else
+		rfs_shift = MOD_RCLK_SHIFT;
+	mod &= ~(MOD_RCLK_MASK << rfs_shift);
 
 	switch (rfs) {
 	case 768:
-		mod |= MOD_RCLK_768FS;
+		mod |= (MOD_RCLK_768FS << rfs_shift);
 		break;
 	case 512:
-		mod |= MOD_RCLK_512FS;
+		mod |= (MOD_RCLK_512FS << rfs_shift);
 		break;
 	case 384:
-		mod |= MOD_RCLK_384FS;
+		mod |= (MOD_RCLK_384FS << rfs_shift);
 		break;
 	default:
-		mod |= MOD_RCLK_256FS;
+		mod |= (MOD_RCLK_256FS << rfs_shift);
 		break;
 	}
 
@@ -236,9 +247,22 @@ static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs)
 /* Read Bit-Clock of I2S (in multiples of LRCLK) */
 static inline unsigned get_bfs(struct i2s_dai *i2s)
 {
-	u32 bfs = (readl(i2s->addr + I2SMOD) >> 1) & 0x3;
+	u32 bfs;
+
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
+		bfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_BCLK_SHIFT;
+		bfs &= EXYNOS5420_MOD_BCLK_MASK;
+	} else {
+		bfs = readl(i2s->addr + I2SMOD) >> MOD_BCLK_SHIFT;
+		bfs &= MOD_BCLK_MASK;
+	}
 
 	switch (bfs) {
+	case 8: return 256;
+	case 7: return 192;
+	case 6: return 128;
+	case 5: return 96;
+	case 4: return 64;
 	case 3: return 24;
 	case 2: return 16;
 	case 1:	return 48;
@@ -250,21 +274,50 @@ static inline unsigned get_bfs(struct i2s_dai *i2s)
 static inline void set_bfs(struct i2s_dai *i2s, unsigned bfs)
 {
 	u32 mod = readl(i2s->addr + I2SMOD);
+	int bfs_shift;
+	int tdm = i2s->quirks & QUIRK_SUPPORTS_TDM;
 
-	mod &= ~MOD_BCLK_MASK;
+	if (tdm) {
+		bfs_shift = EXYNOS5420_MOD_BCLK_SHIFT;
+		mod &= ~(EXYNOS5420_MOD_BCLK_MASK << bfs_shift);
+	} else {
+		bfs_shift = MOD_BCLK_SHIFT;
+		mod &= ~(MOD_BCLK_MASK << bfs_shift);
+	}
+
+	/* Non-TDM I2S controllers do not support BCLK > 48 * FS */
+	if (!tdm && bfs > 48) {
+		dev_err(&i2s->pdev->dev, "Unsupported BCLK divider\n");
+		return;
+	}
 
 	switch (bfs) {
 	case 48:
-		mod |= MOD_BCLK_48FS;
+		mod |= (MOD_BCLK_48FS << bfs_shift);
 		break;
 	case 32:
-		mod |= MOD_BCLK_32FS;
+		mod |= (MOD_BCLK_32FS << bfs_shift);
 		break;
 	case 24:
-		mod |= MOD_BCLK_24FS;
+		mod |= (MOD_BCLK_24FS << bfs_shift);
 		break;
 	case 16:
-		mod |= MOD_BCLK_16FS;
+		mod |= (MOD_BCLK_16FS << bfs_shift);
+		break;
+	case 64:
+		mod |= (EXYNOS5420_MOD_BCLK_64FS << bfs_shift);
+		break;
+	case 96:
+		mod |= (EXYNOS5420_MOD_BCLK_96FS << bfs_shift);
+		break;
+	case 128:
+		mod |= (EXYNOS5420_MOD_BCLK_128FS << bfs_shift);
+		break;
+	case 192:
+		mod |= (EXYNOS5420_MOD_BCLK_192FS << bfs_shift);
+		break;
+	case 256:
+		mod |= (EXYNOS5420_MOD_BCLK_256FS << bfs_shift);
 		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Wrong BCLK Divider!\n");
@@ -492,19 +545,31 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
 	struct i2s_dai *i2s = to_info(dai);
 	u32 mod = readl(i2s->addr + I2SMOD);
 	u32 tmp = 0;
+	int lrp_shift, sdf_shift, sdf_mask, lrp_rlow;
+
+	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
+		lrp_shift = EXYNOS5420_MOD_LRP_SHIFT;
+		sdf_shift = EXYNOS5420_MOD_SDF_SHIFT;
+	} else {
+		lrp_shift = MOD_LRP_SHIFT;
+		sdf_shift = MOD_SDF_SHIFT;
+	}
+
+	sdf_mask = MOD_SDF_MASK << sdf_shift;
+	lrp_rlow = MOD_LR_RLOW << lrp_shift;
 
 	/* Format is priority */
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_RIGHT_J:
-		tmp |= MOD_LR_RLOW;
-		tmp |= MOD_SDF_MSB;
+		tmp |= lrp_rlow;
+		tmp |= (MOD_SDF_MSB << sdf_shift);
 		break;
 	case SND_SOC_DAIFMT_LEFT_J:
-		tmp |= MOD_LR_RLOW;
-		tmp |= MOD_SDF_LSB;
+		tmp |= lrp_rlow;
+		tmp |= (MOD_SDF_LSB << sdf_shift);
 		break;
 	case SND_SOC_DAIFMT_I2S:
-		tmp |= MOD_SDF_IIS;
+		tmp |= (MOD_SDF_IIS << sdf_shift);
 		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Format not supported\n");
@@ -519,10 +584,10 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
 	case SND_SOC_DAIFMT_NB_NF:
 		break;
 	case SND_SOC_DAIFMT_NB_IF:
-		if (tmp & MOD_LR_RLOW)
-			tmp &= ~MOD_LR_RLOW;
+		if (tmp & lrp_rlow)
+			tmp &= ~lrp_rlow;
 		else
-			tmp |= MOD_LR_RLOW;
+			tmp |= lrp_rlow;
 		break;
 	default:
 		dev_err(&i2s->pdev->dev, "Polarity not supported\n");
@@ -544,15 +609,18 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
 		return -EINVAL;
 	}
 
+	/*
+	 * Don't change the I2S mode if any controller is active on this
+	 * channel.
+	 */
 	if (any_active(i2s) &&
-			((mod & (MOD_SDF_MASK | MOD_LR_RLOW
-				| MOD_SLAVE)) != tmp)) {
+	    ((mod & (sdf_mask | lrp_rlow | MOD_SLAVE)) != tmp)) {
 		dev_err(&i2s->pdev->dev,
 				"%s:%d Other DAI busy\n", __func__, __LINE__);
 		return -EAGAIN;
 	}
 
-	mod &= ~(MOD_SDF_MASK | MOD_LR_RLOW | MOD_SLAVE);
+	mod &= ~(sdf_mask | lrp_rlow | MOD_SLAVE);
 	mod |= tmp;
 	writel(mod, i2s->addr + I2SMOD);
 
@@ -1170,6 +1238,9 @@ static int samsung_i2s_probe(struct platform_device *pdev)
 		if (of_find_property(np, "samsung,supports-rstclr", NULL))
 			quirks |= QUIRK_NEED_RSTCLR;
 
+		if (of_find_property(np, "samsung,supports-tdm", NULL))
+			quirks |= QUIRK_SUPPORTS_TDM;
+
 		if (of_property_read_u32(np, "samsung,idma-addr",
 					 &idma_addr)) {
 			if (quirks & QUIRK_SEC_DAI) {
-- 
1.7.4.4

[PATCH] ASoC: Samsung: Set RFS and BFS in slave mode

[Padmavathi Venna]

As per the User Manual, the RFS and BFS should be set in slave mode
for correct operation.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-on: https://gerrit-int.chromium.org/37841
Reviewed-by: Simon Glass <sjg@google.com>
---
 sound/soc/samsung/i2s.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 82ebb1a..3fcf8d7 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -742,13 +742,13 @@ static int config_setup(struct i2s_dai *i2s)
 		return -EAGAIN;
 	}
 
-	/* Don't bother RFS, BFS & PSR in Slave mode */
-	if (is_slave(i2s))
-		return 0;
-
 	set_bfs(i2s, bfs);
 	set_rfs(i2s, rfs);
 
+	/* Don't bother with PSR in Slave mode */
+	if (is_slave(i2s))
+		return 0;
+
 	if (!(i2s->quirks & QUIRK_NO_MUXPSR)) {
 		psr = i2s->rclk_srcrate / i2s->frmclk / rfs;
 		writel(((psr - 1) << 8) | PSR_PSREN, i2s->addr + I2SPSR);
-- 
1.7.4.4

[PATCH] ASoC: Samsung: Set RFS and BFS in slave mode

[Padmavathi Venna]

As per the User Manual, the RFS and BFS should be set in slave mode
for correct operation.

Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
Reviewed-on: https://gerrit-int.chromium.org/37841
Reviewed-by: Simon Glass <sjg@google.com>
---
 sound/soc/samsung/i2s.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
index 82ebb1a..3fcf8d7 100644
--- a/sound/soc/samsung/i2s.c
+++ b/sound/soc/samsung/i2s.c
@@ -742,13 +742,13 @@ static int config_setup(struct i2s_dai *i2s)
 		return -EAGAIN;
 	}
 
-	/* Don't bother RFS, BFS & PSR in Slave mode */
-	if (is_slave(i2s))
-		return 0;
-
 	set_bfs(i2s, bfs);
 	set_rfs(i2s, rfs);
 
+	/* Don't bother with PSR in Slave mode */
+	if (is_slave(i2s))
+		return 0;
+
 	if (!(i2s->quirks & QUIRK_NO_MUXPSR)) {
 		psr = i2s->rclk_srcrate / i2s->frmclk / rfs;
 		writel(((psr - 1) << 8) | PSR_PSREN, i2s->addr + I2SPSR);
-- 
1.7.4.4

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

Hi Padmavathi,

On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote:
> Exynos5420 added support for I2S TDM mode. For this, there are some
> register changes in the I2S controller. This patch adds the relevant
> register changes to support I2S in normal mode. This patch adds a
> quirk for TDM mode and if TDM mode is present all the relevent changes
> will be applied.
> 
> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> [abrestic: style cleanup and documentation]
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Reviewed-on: https://gerrit-int.chromium.org/37840
> Reviewed-by: Simon Glass <sjg@google.com>
> ---
>  .../devicetree/bindings/sound/samsung-i2s.txt      |    2 +
>  include/linux/platform_data/asoc-s3c.h             |    1 +
>  sound/soc/samsung/i2s-regs.h                       |   51 ++++++---
>  sound/soc/samsung/i2s.c                            |  117
> ++++++++++++++++---- 4 files changed, 132 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
> b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index
> 025e66b..b8593d5 100644
> --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
> +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
> @@ -28,6 +28,8 @@ Optional SoC Specific Properties:
>    enabled or disabled based on need.
>  - samsung,supports-secdai:If I2S block has a secondary FIFO and internal
> DMA, then this flag is enabled.
> +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag
> +  must be enabled.

I think this should be rather handled by a different compatible value, not a 
flag. Also a word about what this TDM mode is would be nice.

Best regards,
Tomasz

>  - samsung,idma-addr: Internal DMA register base address of the audio
>    sub system(used in secondary sound source).
>  - pinctrl-0: Should specify pin control groups used for this controller.
> diff --git a/include/linux/platform_data/asoc-s3c.h
> b/include/linux/platform_data/asoc-s3c.h index 8827259..9efc04d 100644
> --- a/include/linux/platform_data/asoc-s3c.h
> +++ b/include/linux/platform_data/asoc-s3c.h
> @@ -36,6 +36,7 @@ struct samsung_i2s {
>   */
>  #define QUIRK_NO_MUXPSR		(1 << 2)
>  #define QUIRK_NEED_RSTCLR	(1 << 3)
> +#define QUIRK_SUPPORTS_TDM	(1 << 4)
>  	/* Quirks of the I2S controller */
>  	u32 quirks;
>  	dma_addr_t idma_addr;
> diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
> index c0e6d9a..821a502 100644
> --- a/sound/soc/samsung/i2s-regs.h
> +++ b/sound/soc/samsung/i2s-regs.h
> @@ -31,6 +31,10 @@
>  #define I2SLVL1ADDR	0x34
>  #define I2SLVL2ADDR	0x38
>  #define I2SLVL3ADDR	0x3c
> +#define I2SSTR1		0x40
> +#define I2SVER		0x44
> +#define I2SFIC2		0x48
> +#define I2STDM		0x4c
> 
>  #define CON_RSTCLR		(1 << 31)
>  #define CON_FRXOFSTATUS		(1 << 26)
> @@ -95,24 +99,39 @@
>  #define MOD_RXONLY		(1 << 8)
>  #define MOD_TXRX		(2 << 8)
>  #define MOD_MASK		(3 << 8)
> -#define MOD_LR_LLOW		(0 << 7)
> -#define MOD_LR_RLOW		(1 << 7)
> -#define MOD_SDF_IIS		(0 << 5)
> -#define MOD_SDF_MSB		(1 << 5)
> -#define MOD_SDF_LSB		(2 << 5)
> -#define MOD_SDF_MASK		(3 << 5)
> -#define MOD_RCLK_256FS		(0 << 3)
> -#define MOD_RCLK_512FS		(1 << 3)
> -#define MOD_RCLK_384FS		(2 << 3)
> -#define MOD_RCLK_768FS		(3 << 3)
> -#define MOD_RCLK_MASK		(3 << 3)
> -#define MOD_BCLK_32FS		(0 << 1)
> -#define MOD_BCLK_48FS		(1 << 1)
> -#define MOD_BCLK_16FS		(2 << 1)
> -#define MOD_BCLK_24FS		(3 << 1)
> -#define MOD_BCLK_MASK		(3 << 1)
> +#define MOD_LRP_SHIFT		7
> +#define MOD_LR_LLOW		0
> +#define MOD_LR_RLOW		1
> +#define MOD_SDF_SHIFT		5
> +#define MOD_SDF_IIS		0
> +#define MOD_SDF_MSB		1
> +#define MOD_SDF_LSB		2
> +#define MOD_SDF_MASK		3
> +#define MOD_RCLK_SHIFT		3
> +#define MOD_RCLK_256FS		0
> +#define MOD_RCLK_512FS		1
> +#define MOD_RCLK_384FS		2
> +#define MOD_RCLK_768FS		3
> +#define MOD_RCLK_MASK		3
> +#define MOD_BCLK_SHIFT		1
> +#define MOD_BCLK_32FS		0
> +#define MOD_BCLK_48FS		1
> +#define MOD_BCLK_16FS		2
> +#define MOD_BCLK_24FS		3
> +#define MOD_BCLK_MASK		3
>  #define MOD_8BIT		(1 << 0)
> 
> +#define EXYNOS5420_MOD_LRP_SHIFT	15
> +#define EXYNOS5420_MOD_SDF_SHIFT	6
> +#define EXYNOS5420_MOD_RCLK_SHIFT	4
> +#define EXYNOS5420_MOD_BCLK_SHIFT	0
> +#define EXYNOS5420_MOD_BCLK_64FS	4
> +#define EXYNOS5420_MOD_BCLK_96FS	5
> +#define EXYNOS5420_MOD_BCLK_128FS	6
> +#define EXYNOS5420_MOD_BCLK_192FS	7
> +#define EXYNOS5420_MOD_BCLK_256FS	8
> +#define EXYNOS5420_MOD_BCLK_MASK	0xf
> +
>  #define MOD_CDCLKCON		(1 << 12)
> 
>  #define PSR_PSREN		(1 << 15)
> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
> index 3fcf8d7..398f8db 100644
> --- a/sound/soc/samsung/i2s.c
> +++ b/sound/soc/samsung/i2s.c
> @@ -198,7 +198,13 @@ static inline bool is_manager(struct i2s_dai *i2s)
>  /* Read RCLK of I2S (in multiples of LRCLK) */
>  static inline unsigned get_rfs(struct i2s_dai *i2s)
>  {
> -	u32 rfs = (readl(i2s->addr + I2SMOD) >> 3) & 0x3;
> +	u32 rfs;
> +
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
> +		rfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_RCLK_SHIFT;
> +	else
> +		rfs = readl(i2s->addr + I2SMOD) >> MOD_RCLK_SHIFT;
> +	rfs &= MOD_RCLK_MASK;
> 
>  	switch (rfs) {
>  	case 3:	return 768;
> @@ -212,21 +218,26 @@ static inline unsigned get_rfs(struct i2s_dai *i2s)
>  static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs)
>  {
>  	u32 mod = readl(i2s->addr + I2SMOD);
> +	int rfs_shift;
> 
> -	mod &= ~MOD_RCLK_MASK;
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
> +		rfs_shift = EXYNOS5420_MOD_RCLK_SHIFT;
> +	else
> +		rfs_shift = MOD_RCLK_SHIFT;
> +	mod &= ~(MOD_RCLK_MASK << rfs_shift);
> 
>  	switch (rfs) {
>  	case 768:
> -		mod |= MOD_RCLK_768FS;
> +		mod |= (MOD_RCLK_768FS << rfs_shift);
>  		break;
>  	case 512:
> -		mod |= MOD_RCLK_512FS;
> +		mod |= (MOD_RCLK_512FS << rfs_shift);
>  		break;
>  	case 384:
> -		mod |= MOD_RCLK_384FS;
> +		mod |= (MOD_RCLK_384FS << rfs_shift);
>  		break;
>  	default:
> -		mod |= MOD_RCLK_256FS;
> +		mod |= (MOD_RCLK_256FS << rfs_shift);
>  		break;
>  	}
> 
> @@ -236,9 +247,22 @@ static inline void set_rfs(struct i2s_dai *i2s,
> unsigned rfs) /* Read Bit-Clock of I2S (in multiples of LRCLK) */
>  static inline unsigned get_bfs(struct i2s_dai *i2s)
>  {
> -	u32 bfs = (readl(i2s->addr + I2SMOD) >> 1) & 0x3;
> +	u32 bfs;
> +
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
> +		bfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_BCLK_SHIFT;
> +		bfs &= EXYNOS5420_MOD_BCLK_MASK;
> +	} else {
> +		bfs = readl(i2s->addr + I2SMOD) >> MOD_BCLK_SHIFT;
> +		bfs &= MOD_BCLK_MASK;
> +	}
> 
>  	switch (bfs) {
> +	case 8: return 256;
> +	case 7: return 192;
> +	case 6: return 128;
> +	case 5: return 96;
> +	case 4: return 64;
>  	case 3: return 24;
>  	case 2: return 16;
>  	case 1:	return 48;
> @@ -250,21 +274,50 @@ static inline unsigned get_bfs(struct i2s_dai *i2s)
>  static inline void set_bfs(struct i2s_dai *i2s, unsigned bfs)
>  {
>  	u32 mod = readl(i2s->addr + I2SMOD);
> +	int bfs_shift;
> +	int tdm = i2s->quirks & QUIRK_SUPPORTS_TDM;
> 
> -	mod &= ~MOD_BCLK_MASK;
> +	if (tdm) {
> +		bfs_shift = EXYNOS5420_MOD_BCLK_SHIFT;
> +		mod &= ~(EXYNOS5420_MOD_BCLK_MASK << bfs_shift);
> +	} else {
> +		bfs_shift = MOD_BCLK_SHIFT;
> +		mod &= ~(MOD_BCLK_MASK << bfs_shift);
> +	}
> +
> +	/* Non-TDM I2S controllers do not support BCLK > 48 * FS */
> +	if (!tdm && bfs > 48) {
> +		dev_err(&i2s->pdev->dev, "Unsupported BCLK divider\n");
> +		return;
> +	}
> 
>  	switch (bfs) {
>  	case 48:
> -		mod |= MOD_BCLK_48FS;
> +		mod |= (MOD_BCLK_48FS << bfs_shift);
>  		break;
>  	case 32:
> -		mod |= MOD_BCLK_32FS;
> +		mod |= (MOD_BCLK_32FS << bfs_shift);
>  		break;
>  	case 24:
> -		mod |= MOD_BCLK_24FS;
> +		mod |= (MOD_BCLK_24FS << bfs_shift);
>  		break;
>  	case 16:
> -		mod |= MOD_BCLK_16FS;
> +		mod |= (MOD_BCLK_16FS << bfs_shift);
> +		break;
> +	case 64:
> +		mod |= (EXYNOS5420_MOD_BCLK_64FS << bfs_shift);
> +		break;
> +	case 96:
> +		mod |= (EXYNOS5420_MOD_BCLK_96FS << bfs_shift);
> +		break;
> +	case 128:
> +		mod |= (EXYNOS5420_MOD_BCLK_128FS << bfs_shift);
> +		break;
> +	case 192:
> +		mod |= (EXYNOS5420_MOD_BCLK_192FS << bfs_shift);
> +		break;
> +	case 256:
> +		mod |= (EXYNOS5420_MOD_BCLK_256FS << bfs_shift);
>  		break;
>  	default:
>  		dev_err(&i2s->pdev->dev, "Wrong BCLK Divider!\n");
> @@ -492,19 +545,31 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>  	struct i2s_dai *i2s = to_info(dai);
>  	u32 mod = readl(i2s->addr + I2SMOD);
>  	u32 tmp = 0;
> +	int lrp_shift, sdf_shift, sdf_mask, lrp_rlow;
> +
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
> +		lrp_shift = EXYNOS5420_MOD_LRP_SHIFT;
> +		sdf_shift = EXYNOS5420_MOD_SDF_SHIFT;
> +	} else {
> +		lrp_shift = MOD_LRP_SHIFT;
> +		sdf_shift = MOD_SDF_SHIFT;
> +	}
> +
> +	sdf_mask = MOD_SDF_MASK << sdf_shift;
> +	lrp_rlow = MOD_LR_RLOW << lrp_shift;
> 
>  	/* Format is priority */
>  	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>  	case SND_SOC_DAIFMT_RIGHT_J:
> -		tmp |= MOD_LR_RLOW;
> -		tmp |= MOD_SDF_MSB;
> +		tmp |= lrp_rlow;
> +		tmp |= (MOD_SDF_MSB << sdf_shift);
>  		break;
>  	case SND_SOC_DAIFMT_LEFT_J:
> -		tmp |= MOD_LR_RLOW;
> -		tmp |= MOD_SDF_LSB;
> +		tmp |= lrp_rlow;
> +		tmp |= (MOD_SDF_LSB << sdf_shift);
>  		break;
>  	case SND_SOC_DAIFMT_I2S:
> -		tmp |= MOD_SDF_IIS;
> +		tmp |= (MOD_SDF_IIS << sdf_shift);
>  		break;
>  	default:
>  		dev_err(&i2s->pdev->dev, "Format not supported\n");
> @@ -519,10 +584,10 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>  	case SND_SOC_DAIFMT_NB_NF:
>  		break;
>  	case SND_SOC_DAIFMT_NB_IF:
> -		if (tmp & MOD_LR_RLOW)
> -			tmp &= ~MOD_LR_RLOW;
> +		if (tmp & lrp_rlow)
> +			tmp &= ~lrp_rlow;
>  		else
> -			tmp |= MOD_LR_RLOW;
> +			tmp |= lrp_rlow;
>  		break;
>  	default:
>  		dev_err(&i2s->pdev->dev, "Polarity not supported\n");
> @@ -544,15 +609,18 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>  		return -EINVAL;
>  	}
> 
> +	/*
> +	 * Don't change the I2S mode if any controller is active on this
> +	 * channel.
> +	 */
>  	if (any_active(i2s) &&
> -			((mod & (MOD_SDF_MASK | MOD_LR_RLOW
> -				| MOD_SLAVE)) != tmp)) {
> +	    ((mod & (sdf_mask | lrp_rlow | MOD_SLAVE)) != tmp)) {
>  		dev_err(&i2s->pdev->dev,
>  				"%s:%d Other DAI busy\n", __func__, __LINE__);
>  		return -EAGAIN;
>  	}
> 
> -	mod &= ~(MOD_SDF_MASK | MOD_LR_RLOW | MOD_SLAVE);
> +	mod &= ~(sdf_mask | lrp_rlow | MOD_SLAVE);
>  	mod |= tmp;
>  	writel(mod, i2s->addr + I2SMOD);
> 
> @@ -1170,6 +1238,9 @@ static int samsung_i2s_probe(struct platform_device
> *pdev) if (of_find_property(np, "samsung,supports-rstclr", NULL))
>  			quirks |= QUIRK_NEED_RSTCLR;
> 
> +		if (of_find_property(np, "samsung,supports-tdm", NULL))
> +			quirks |= QUIRK_SUPPORTS_TDM;
> +
>  		if (of_property_read_u32(np, "samsung,idma-addr",
>  					 &idma_addr)) {
>  			if (quirks & QUIRK_SEC_DAI) {

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

Hi Padmavathi,

On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote:
> Exynos5420 added support for I2S TDM mode. For this, there are some
> register changes in the I2S controller. This patch adds the relevant
> register changes to support I2S in normal mode. This patch adds a
> quirk for TDM mode and if TDM mode is present all the relevent changes
> will be applied.
> 
> Signed-off-by: Padmavathi Venna <padma.v@samsung.com>
> [abrestic: style cleanup and documentation]
> Signed-off-by: Andrew Bresticker <abrestic@chromium.org>
> Reviewed-on: https://gerrit-int.chromium.org/37840
> Reviewed-by: Simon Glass <sjg@google.com>
> ---
>  .../devicetree/bindings/sound/samsung-i2s.txt      |    2 +
>  include/linux/platform_data/asoc-s3c.h             |    1 +
>  sound/soc/samsung/i2s-regs.h                       |   51 ++++++---
>  sound/soc/samsung/i2s.c                            |  117
> ++++++++++++++++---- 4 files changed, 132 insertions(+), 39 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
> b/Documentation/devicetree/bindings/sound/samsung-i2s.txt index
> 025e66b..b8593d5 100644
> --- a/Documentation/devicetree/bindings/sound/samsung-i2s.txt
> +++ b/Documentation/devicetree/bindings/sound/samsung-i2s.txt
> @@ -28,6 +28,8 @@ Optional SoC Specific Properties:
>    enabled or disabled based on need.
>  - samsung,supports-secdai:If I2S block has a secondary FIFO and internal
> DMA, then this flag is enabled.
> +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag
> +  must be enabled.

I think this should be rather handled by a different compatible value, not a 
flag. Also a word about what this TDM mode is would be nice.

Best regards,
Tomasz

>  - samsung,idma-addr: Internal DMA register base address of the audio
>    sub system(used in secondary sound source).
>  - pinctrl-0: Should specify pin control groups used for this controller.
> diff --git a/include/linux/platform_data/asoc-s3c.h
> b/include/linux/platform_data/asoc-s3c.h index 8827259..9efc04d 100644
> --- a/include/linux/platform_data/asoc-s3c.h
> +++ b/include/linux/platform_data/asoc-s3c.h
> @@ -36,6 +36,7 @@ struct samsung_i2s {
>   */
>  #define QUIRK_NO_MUXPSR		(1 << 2)
>  #define QUIRK_NEED_RSTCLR	(1 << 3)
> +#define QUIRK_SUPPORTS_TDM	(1 << 4)
>  	/* Quirks of the I2S controller */
>  	u32 quirks;
>  	dma_addr_t idma_addr;
> diff --git a/sound/soc/samsung/i2s-regs.h b/sound/soc/samsung/i2s-regs.h
> index c0e6d9a..821a502 100644
> --- a/sound/soc/samsung/i2s-regs.h
> +++ b/sound/soc/samsung/i2s-regs.h
> @@ -31,6 +31,10 @@
>  #define I2SLVL1ADDR	0x34
>  #define I2SLVL2ADDR	0x38
>  #define I2SLVL3ADDR	0x3c
> +#define I2SSTR1		0x40
> +#define I2SVER		0x44
> +#define I2SFIC2		0x48
> +#define I2STDM		0x4c
> 
>  #define CON_RSTCLR		(1 << 31)
>  #define CON_FRXOFSTATUS		(1 << 26)
> @@ -95,24 +99,39 @@
>  #define MOD_RXONLY		(1 << 8)
>  #define MOD_TXRX		(2 << 8)
>  #define MOD_MASK		(3 << 8)
> -#define MOD_LR_LLOW		(0 << 7)
> -#define MOD_LR_RLOW		(1 << 7)
> -#define MOD_SDF_IIS		(0 << 5)
> -#define MOD_SDF_MSB		(1 << 5)
> -#define MOD_SDF_LSB		(2 << 5)
> -#define MOD_SDF_MASK		(3 << 5)
> -#define MOD_RCLK_256FS		(0 << 3)
> -#define MOD_RCLK_512FS		(1 << 3)
> -#define MOD_RCLK_384FS		(2 << 3)
> -#define MOD_RCLK_768FS		(3 << 3)
> -#define MOD_RCLK_MASK		(3 << 3)
> -#define MOD_BCLK_32FS		(0 << 1)
> -#define MOD_BCLK_48FS		(1 << 1)
> -#define MOD_BCLK_16FS		(2 << 1)
> -#define MOD_BCLK_24FS		(3 << 1)
> -#define MOD_BCLK_MASK		(3 << 1)
> +#define MOD_LRP_SHIFT		7
> +#define MOD_LR_LLOW		0
> +#define MOD_LR_RLOW		1
> +#define MOD_SDF_SHIFT		5
> +#define MOD_SDF_IIS		0
> +#define MOD_SDF_MSB		1
> +#define MOD_SDF_LSB		2
> +#define MOD_SDF_MASK		3
> +#define MOD_RCLK_SHIFT		3
> +#define MOD_RCLK_256FS		0
> +#define MOD_RCLK_512FS		1
> +#define MOD_RCLK_384FS		2
> +#define MOD_RCLK_768FS		3
> +#define MOD_RCLK_MASK		3
> +#define MOD_BCLK_SHIFT		1
> +#define MOD_BCLK_32FS		0
> +#define MOD_BCLK_48FS		1
> +#define MOD_BCLK_16FS		2
> +#define MOD_BCLK_24FS		3
> +#define MOD_BCLK_MASK		3
>  #define MOD_8BIT		(1 << 0)
> 
> +#define EXYNOS5420_MOD_LRP_SHIFT	15
> +#define EXYNOS5420_MOD_SDF_SHIFT	6
> +#define EXYNOS5420_MOD_RCLK_SHIFT	4
> +#define EXYNOS5420_MOD_BCLK_SHIFT	0
> +#define EXYNOS5420_MOD_BCLK_64FS	4
> +#define EXYNOS5420_MOD_BCLK_96FS	5
> +#define EXYNOS5420_MOD_BCLK_128FS	6
> +#define EXYNOS5420_MOD_BCLK_192FS	7
> +#define EXYNOS5420_MOD_BCLK_256FS	8
> +#define EXYNOS5420_MOD_BCLK_MASK	0xf
> +
>  #define MOD_CDCLKCON		(1 << 12)
> 
>  #define PSR_PSREN		(1 << 15)
> diff --git a/sound/soc/samsung/i2s.c b/sound/soc/samsung/i2s.c
> index 3fcf8d7..398f8db 100644
> --- a/sound/soc/samsung/i2s.c
> +++ b/sound/soc/samsung/i2s.c
> @@ -198,7 +198,13 @@ static inline bool is_manager(struct i2s_dai *i2s)
>  /* Read RCLK of I2S (in multiples of LRCLK) */
>  static inline unsigned get_rfs(struct i2s_dai *i2s)
>  {
> -	u32 rfs = (readl(i2s->addr + I2SMOD) >> 3) & 0x3;
> +	u32 rfs;
> +
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
> +		rfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_RCLK_SHIFT;
> +	else
> +		rfs = readl(i2s->addr + I2SMOD) >> MOD_RCLK_SHIFT;
> +	rfs &= MOD_RCLK_MASK;
> 
>  	switch (rfs) {
>  	case 3:	return 768;
> @@ -212,21 +218,26 @@ static inline unsigned get_rfs(struct i2s_dai *i2s)
>  static inline void set_rfs(struct i2s_dai *i2s, unsigned rfs)
>  {
>  	u32 mod = readl(i2s->addr + I2SMOD);
> +	int rfs_shift;
> 
> -	mod &= ~MOD_RCLK_MASK;
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM)
> +		rfs_shift = EXYNOS5420_MOD_RCLK_SHIFT;
> +	else
> +		rfs_shift = MOD_RCLK_SHIFT;
> +	mod &= ~(MOD_RCLK_MASK << rfs_shift);
> 
>  	switch (rfs) {
>  	case 768:
> -		mod |= MOD_RCLK_768FS;
> +		mod |= (MOD_RCLK_768FS << rfs_shift);
>  		break;
>  	case 512:
> -		mod |= MOD_RCLK_512FS;
> +		mod |= (MOD_RCLK_512FS << rfs_shift);
>  		break;
>  	case 384:
> -		mod |= MOD_RCLK_384FS;
> +		mod |= (MOD_RCLK_384FS << rfs_shift);
>  		break;
>  	default:
> -		mod |= MOD_RCLK_256FS;
> +		mod |= (MOD_RCLK_256FS << rfs_shift);
>  		break;
>  	}
> 
> @@ -236,9 +247,22 @@ static inline void set_rfs(struct i2s_dai *i2s,
> unsigned rfs) /* Read Bit-Clock of I2S (in multiples of LRCLK) */
>  static inline unsigned get_bfs(struct i2s_dai *i2s)
>  {
> -	u32 bfs = (readl(i2s->addr + I2SMOD) >> 1) & 0x3;
> +	u32 bfs;
> +
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
> +		bfs = readl(i2s->addr + I2SMOD) >> EXYNOS5420_MOD_BCLK_SHIFT;
> +		bfs &= EXYNOS5420_MOD_BCLK_MASK;
> +	} else {
> +		bfs = readl(i2s->addr + I2SMOD) >> MOD_BCLK_SHIFT;
> +		bfs &= MOD_BCLK_MASK;
> +	}
> 
>  	switch (bfs) {
> +	case 8: return 256;
> +	case 7: return 192;
> +	case 6: return 128;
> +	case 5: return 96;
> +	case 4: return 64;
>  	case 3: return 24;
>  	case 2: return 16;
>  	case 1:	return 48;
> @@ -250,21 +274,50 @@ static inline unsigned get_bfs(struct i2s_dai *i2s)
>  static inline void set_bfs(struct i2s_dai *i2s, unsigned bfs)
>  {
>  	u32 mod = readl(i2s->addr + I2SMOD);
> +	int bfs_shift;
> +	int tdm = i2s->quirks & QUIRK_SUPPORTS_TDM;
> 
> -	mod &= ~MOD_BCLK_MASK;
> +	if (tdm) {
> +		bfs_shift = EXYNOS5420_MOD_BCLK_SHIFT;
> +		mod &= ~(EXYNOS5420_MOD_BCLK_MASK << bfs_shift);
> +	} else {
> +		bfs_shift = MOD_BCLK_SHIFT;
> +		mod &= ~(MOD_BCLK_MASK << bfs_shift);
> +	}
> +
> +	/* Non-TDM I2S controllers do not support BCLK > 48 * FS */
> +	if (!tdm && bfs > 48) {
> +		dev_err(&i2s->pdev->dev, "Unsupported BCLK divider\n");
> +		return;
> +	}
> 
>  	switch (bfs) {
>  	case 48:
> -		mod |= MOD_BCLK_48FS;
> +		mod |= (MOD_BCLK_48FS << bfs_shift);
>  		break;
>  	case 32:
> -		mod |= MOD_BCLK_32FS;
> +		mod |= (MOD_BCLK_32FS << bfs_shift);
>  		break;
>  	case 24:
> -		mod |= MOD_BCLK_24FS;
> +		mod |= (MOD_BCLK_24FS << bfs_shift);
>  		break;
>  	case 16:
> -		mod |= MOD_BCLK_16FS;
> +		mod |= (MOD_BCLK_16FS << bfs_shift);
> +		break;
> +	case 64:
> +		mod |= (EXYNOS5420_MOD_BCLK_64FS << bfs_shift);
> +		break;
> +	case 96:
> +		mod |= (EXYNOS5420_MOD_BCLK_96FS << bfs_shift);
> +		break;
> +	case 128:
> +		mod |= (EXYNOS5420_MOD_BCLK_128FS << bfs_shift);
> +		break;
> +	case 192:
> +		mod |= (EXYNOS5420_MOD_BCLK_192FS << bfs_shift);
> +		break;
> +	case 256:
> +		mod |= (EXYNOS5420_MOD_BCLK_256FS << bfs_shift);
>  		break;
>  	default:
>  		dev_err(&i2s->pdev->dev, "Wrong BCLK Divider!\n");
> @@ -492,19 +545,31 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>  	struct i2s_dai *i2s = to_info(dai);
>  	u32 mod = readl(i2s->addr + I2SMOD);
>  	u32 tmp = 0;
> +	int lrp_shift, sdf_shift, sdf_mask, lrp_rlow;
> +
> +	if (i2s->quirks & QUIRK_SUPPORTS_TDM) {
> +		lrp_shift = EXYNOS5420_MOD_LRP_SHIFT;
> +		sdf_shift = EXYNOS5420_MOD_SDF_SHIFT;
> +	} else {
> +		lrp_shift = MOD_LRP_SHIFT;
> +		sdf_shift = MOD_SDF_SHIFT;
> +	}
> +
> +	sdf_mask = MOD_SDF_MASK << sdf_shift;
> +	lrp_rlow = MOD_LR_RLOW << lrp_shift;
> 
>  	/* Format is priority */
>  	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
>  	case SND_SOC_DAIFMT_RIGHT_J:
> -		tmp |= MOD_LR_RLOW;
> -		tmp |= MOD_SDF_MSB;
> +		tmp |= lrp_rlow;
> +		tmp |= (MOD_SDF_MSB << sdf_shift);
>  		break;
>  	case SND_SOC_DAIFMT_LEFT_J:
> -		tmp |= MOD_LR_RLOW;
> -		tmp |= MOD_SDF_LSB;
> +		tmp |= lrp_rlow;
> +		tmp |= (MOD_SDF_LSB << sdf_shift);
>  		break;
>  	case SND_SOC_DAIFMT_I2S:
> -		tmp |= MOD_SDF_IIS;
> +		tmp |= (MOD_SDF_IIS << sdf_shift);
>  		break;
>  	default:
>  		dev_err(&i2s->pdev->dev, "Format not supported\n");
> @@ -519,10 +584,10 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>  	case SND_SOC_DAIFMT_NB_NF:
>  		break;
>  	case SND_SOC_DAIFMT_NB_IF:
> -		if (tmp & MOD_LR_RLOW)
> -			tmp &= ~MOD_LR_RLOW;
> +		if (tmp & lrp_rlow)
> +			tmp &= ~lrp_rlow;
>  		else
> -			tmp |= MOD_LR_RLOW;
> +			tmp |= lrp_rlow;
>  		break;
>  	default:
>  		dev_err(&i2s->pdev->dev, "Polarity not supported\n");
> @@ -544,15 +609,18 @@ static int i2s_set_fmt(struct snd_soc_dai *dai,
>  		return -EINVAL;
>  	}
> 
> +	/*
> +	 * Don't change the I2S mode if any controller is active on this
> +	 * channel.
> +	 */
>  	if (any_active(i2s) &&
> -			((mod & (MOD_SDF_MASK | MOD_LR_RLOW
> -				| MOD_SLAVE)) != tmp)) {
> +	    ((mod & (sdf_mask | lrp_rlow | MOD_SLAVE)) != tmp)) {
>  		dev_err(&i2s->pdev->dev,
>  				"%s:%d Other DAI busy\n", __func__, __LINE__);
>  		return -EAGAIN;
>  	}
> 
> -	mod &= ~(MOD_SDF_MASK | MOD_LR_RLOW | MOD_SLAVE);
> +	mod &= ~(sdf_mask | lrp_rlow | MOD_SLAVE);
>  	mod |= tmp;
>  	writel(mod, i2s->addr + I2SMOD);
> 
> @@ -1170,6 +1238,9 @@ static int samsung_i2s_probe(struct platform_device
> *pdev) if (of_find_property(np, "samsung,supports-rstclr", NULL))
>  			quirks |= QUIRK_NEED_RSTCLR;
> 
> +		if (of_find_property(np, "samsung,supports-tdm", NULL))
> +			quirks |= QUIRK_SUPPORTS_TDM;
> +
>  		if (of_property_read_u32(np, "samsung,idma-addr",
>  					 &idma_addr)) {
>  			if (quirks & QUIRK_SEC_DAI) {

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 972 bytes --]

On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote:

> -#define MOD_LR_LLOW		(0 << 7)
> -#define MOD_LR_RLOW		(1 << 7)
> -#define MOD_SDF_IIS		(0 << 5)
> -#define MOD_SDF_MSB		(1 << 5)
> -#define MOD_SDF_LSB		(2 << 5)
> -#define MOD_SDF_MASK		(3 << 5)

> +#define MOD_LR_LLOW		0
> +#define MOD_LR_RLOW		1
> +#define MOD_SDF_SHIFT		5
> +#define MOD_SDF_IIS		0
> +#define MOD_SDF_MSB		1
> +#define MOD_SDF_LSB		2
> +#define MOD_SDF_MASK		3

This patch has an awful lot of coding style changes like this which
are just coding style changes and not implementing TDM support.  These
should be done separately, not as part of the same patch, in order to
make the code easier to review.

>  	case 768:
> -		mod |= MOD_RCLK_768FS;
> +		mod |= (MOD_RCLK_768FS << rfs_shift);
>  		break;

This stuff is another example.

I think the change itself should be fine but I'm not 100% sure I'm
correctly identifying what's a stylistic change and what's a functional
change.

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

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote:

> -#define MOD_LR_LLOW		(0 << 7)
> -#define MOD_LR_RLOW		(1 << 7)
> -#define MOD_SDF_IIS		(0 << 5)
> -#define MOD_SDF_MSB		(1 << 5)
> -#define MOD_SDF_LSB		(2 << 5)
> -#define MOD_SDF_MASK		(3 << 5)

> +#define MOD_LR_LLOW		0
> +#define MOD_LR_RLOW		1
> +#define MOD_SDF_SHIFT		5
> +#define MOD_SDF_IIS		0
> +#define MOD_SDF_MSB		1
> +#define MOD_SDF_LSB		2
> +#define MOD_SDF_MASK		3

This patch has an awful lot of coding style changes like this which
are just coding style changes and not implementing TDM support.  These
should be done separately, not as part of the same patch, in order to
make the code easier to review.

>  	case 768:
> -		mod |= MOD_RCLK_768FS;
> +		mod |= (MOD_RCLK_768FS << rfs_shift);
>  		break;

This stuff is another example.

I think the change itself should be fine but I'm not 100% sure I'm
correctly identifying what's a stylistic change and what's a functional
change.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130711/2aad456e/attachment.sig>

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

On Thursday 11 of July 2013 11:48:04 Mark Brown wrote:
> On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote:
> > -#define MOD_LR_LLOW		(0 << 7)
> > -#define MOD_LR_RLOW		(1 << 7)
> > -#define MOD_SDF_IIS		(0 << 5)
> > -#define MOD_SDF_MSB		(1 << 5)
> > -#define MOD_SDF_LSB		(2 << 5)
> > -#define MOD_SDF_MASK		(3 << 5)
> > 
> > +#define MOD_LR_LLOW		0
> > +#define MOD_LR_RLOW		1
> > +#define MOD_SDF_SHIFT		5
> > +#define MOD_SDF_IIS		0
> > +#define MOD_SDF_MSB		1
> > +#define MOD_SDF_LSB		2
> > +#define MOD_SDF_MASK		3
> 
> This patch has an awful lot of coding style changes like this which
> are just coding style changes and not implementing TDM support.  These
> should be done separately, not as part of the same patch, in order to
> make the code easier to review.
> 
> >  	case 768:
> > -		mod |= MOD_RCLK_768FS;
> > +		mod |= (MOD_RCLK_768FS << rfs_shift);
> > 
> >  		break;
> 
> This stuff is another example.
> 
> I think the change itself should be fine but I'm not 100% sure I'm
> correctly identifying what's a stylistic change and what's a functional
> change.

Right. This could be split into two patches, first reworking the style to give 
more flexibility with operations on registers and another one adding TDM 
specific changes, like new bitfield definitions and conditional handling of 
register accesses to account for new bitfield locations.

Best regards,
Tomasz

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

On Thursday 11 of July 2013 11:48:04 Mark Brown wrote:
> On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote:
> > -#define MOD_LR_LLOW		(0 << 7)
> > -#define MOD_LR_RLOW		(1 << 7)
> > -#define MOD_SDF_IIS		(0 << 5)
> > -#define MOD_SDF_MSB		(1 << 5)
> > -#define MOD_SDF_LSB		(2 << 5)
> > -#define MOD_SDF_MASK		(3 << 5)
> > 
> > +#define MOD_LR_LLOW		0
> > +#define MOD_LR_RLOW		1
> > +#define MOD_SDF_SHIFT		5
> > +#define MOD_SDF_IIS		0
> > +#define MOD_SDF_MSB		1
> > +#define MOD_SDF_LSB		2
> > +#define MOD_SDF_MASK		3
> 
> This patch has an awful lot of coding style changes like this which
> are just coding style changes and not implementing TDM support.  These
> should be done separately, not as part of the same patch, in order to
> make the code easier to review.
> 
> >  	case 768:
> > -		mod |= MOD_RCLK_768FS;
> > +		mod |= (MOD_RCLK_768FS << rfs_shift);
> > 
> >  		break;
> 
> This stuff is another example.
> 
> I think the change itself should be fine but I'm not 100% sure I'm
> correctly identifying what's a stylistic change and what's a functional
> change.

Right. This could be split into two patches, first reworking the style to give 
more flexibility with operations on registers and another one adding TDM 
specific changes, like new bitfield definitions and conditional handling of 
register accesses to account for new bitfield locations.

Best regards,
Tomasz

Re: [PATCH] ASoC: Samsung: Set RFS and BFS in slave mode

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 570 bytes --]

On Thu, Jul 11, 2013 at 12:38:25PM +0530, Padmavathi Venna wrote:
> As per the User Manual, the RFS and BFS should be set in slave mode
> for correct operation.

Applied, thanks.  Since this is a fix it should have been the first
patch in the series - this allows fixes to be sent to 

> Reviewed-on: https://gerrit-int.chromium.org/37841

Including things like this isn't terribly helpful - this is the Chromium
internal system and nobody in the community can view the review.  A link
to a public system might be useful but a private one should be omitted.

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

[PATCH] ASoC: Samsung: Set RFS and BFS in slave mode

[Mark Brown]

On Thu, Jul 11, 2013 at 12:38:25PM +0530, Padmavathi Venna wrote:
> As per the User Manual, the RFS and BFS should be set in slave mode
> for correct operation.

Applied, thanks.  Since this is a fix it should have been the first
patch in the series - this allows fixes to be sent to 

> Reviewed-on: https://gerrit-int.chromium.org/37841

Including things like this isn't terribly helpful - this is the Chromium
internal system and nobody in the community can view the review.  A link
to a public system might be useful but a private one should be omitted.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130711/486b9d13/attachment.sig>

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 460 bytes --]

On Thu, Jul 11, 2013 at 12:43:22PM +0200, Tomasz Figa wrote:
> On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote:

> > +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag
> > +  must be enabled.

> I think this should be rather handled by a different compatible value, not a 

This is a bit of a larger project sadly, there was resistance to doing
the DT bindings based on compatible strings using the IP version :(

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

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

On Thu, Jul 11, 2013 at 12:43:22PM +0200, Tomasz Figa wrote:
> On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote:

> > +- samsung,supports-tdm: If the I2S controller supports TDM, then this flag
> > +  must be enabled.

> I think this should be rather handled by a different compatible value, not a 

This is a bit of a larger project sadly, there was resistance to doing
the DT bindings based on compatible strings using the IP version :(
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130711/5d790561/attachment.sig>

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:
> On Thu, Jul 11, 2013 at 12:43:22PM +0200, Tomasz Figa wrote:
> > On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote:
> > > +- samsung,supports-tdm: If the I2S controller supports TDM, then this
> > > flag
> > > +  must be enabled.
> > 
> > I think this should be rather handled by a different compatible value, not
> > a
> This is a bit of a larger project sadly, there was resistance to doing
> the DT bindings based on compatible strings using the IP version :(

I'm not sure if this case is related to this problem in any way. I just 
suggested introducing a new include like samsung,exynos5420-i2s, a opposed to 
using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is 
the first one to have this TDM or whatever mode. (I'd still like to hear what 
it is...)

Best regards,
Tomasz

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:
> On Thu, Jul 11, 2013 at 12:43:22PM +0200, Tomasz Figa wrote:
> > On Thursday 11 of July 2013 12:38:24 Padmavathi Venna wrote:
> > > +- samsung,supports-tdm: If the I2S controller supports TDM, then this
> > > flag
> > > +  must be enabled.
> > 
> > I think this should be rather handled by a different compatible value, not
> > a
> This is a bit of a larger project sadly, there was resistance to doing
> the DT bindings based on compatible strings using the IP version :(

I'm not sure if this case is related to this problem in any way. I just 
suggested introducing a new include like samsung,exynos5420-i2s, a opposed to 
using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is 
the first one to have this TDM or whatever mode. (I'd still like to hear what 
it is...)

Best regards,
Tomasz

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 887 bytes --]

On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote:
> On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:

> > This is a bit of a larger project sadly, there was resistance to doing
> > the DT bindings based on compatible strings using the IP version :(

> I'm not sure if this case is related to this problem in any way. I just 
> suggested introducing a new include like samsung,exynos5420-i2s, a opposed to 
> using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is 

Well, it *should* be samsung,i2s-vN where N is the version number of the
IP but documentation on the versioning has been patchy hence this whole
thinng.

> the first one to have this TDM or whatever mode. (I'd still like to hear what 
> it is...)

What it *should* be is the option to send more than one audio stream
down a link using time division multiplexing.

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

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote:
> On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:

> > This is a bit of a larger project sadly, there was resistance to doing
> > the DT bindings based on compatible strings using the IP version :(

> I'm not sure if this case is related to this problem in any way. I just 
> suggested introducing a new include like samsung,exynos5420-i2s, a opposed to 
> using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is 

Well, it *should* be samsung,i2s-vN where N is the version number of the
IP but documentation on the versioning has been patchy hence this whole
thinng.

> the first one to have this TDM or whatever mode. (I'd still like to hear what 
> it is...)

What it *should* be is the option to send more than one audio stream
down a link using time division multiplexing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130711/36342c14/attachment.sig>

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padma Venkat]

On Thu, Jul 11, 2013 at 4:37 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Thursday 11 of July 2013 11:48:04 Mark Brown wrote:
>> On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote:
>> > -#define MOD_LR_LLOW                (0 << 7)
>> > -#define MOD_LR_RLOW                (1 << 7)
>> > -#define MOD_SDF_IIS                (0 << 5)
>> > -#define MOD_SDF_MSB                (1 << 5)
>> > -#define MOD_SDF_LSB                (2 << 5)
>> > -#define MOD_SDF_MASK               (3 << 5)
>> >
>> > +#define MOD_LR_LLOW                0
>> > +#define MOD_LR_RLOW                1
>> > +#define MOD_SDF_SHIFT              5
>> > +#define MOD_SDF_IIS                0
>> > +#define MOD_SDF_MSB                1
>> > +#define MOD_SDF_LSB                2
>> > +#define MOD_SDF_MASK               3
>>
>> This patch has an awful lot of coding style changes like this which
>> are just coding style changes and not implementing TDM support.  These
>> should be done separately, not as part of the same patch, in order to
>> make the code easier to review.
>>
>> >     case 768:
>> > -           mod |= MOD_RCLK_768FS;
>> > +           mod |= (MOD_RCLK_768FS << rfs_shift);
>> >
>> >             break;
>>
>> This stuff is another example.
>>
>> I think the change itself should be fine but I'm not 100% sure I'm
>> correctly identifying what's a stylistic change and what's a functional
>> change.
>
> Right. This could be split into two patches, first reworking the style to give
> more flexibility with operations on registers and another one adding TDM
> specific changes, like new bitfield definitions and conditional handling of
> register accesses to account for new bitfield locations.
>
> Best regards,
> Tomasz
>

Ok. I will bisect the changes into two patches as suggested.

Thanks
Padma

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padma Venkat]

On Thu, Jul 11, 2013 at 4:37 PM, Tomasz Figa <tomasz.figa@gmail.com> wrote:
> On Thursday 11 of July 2013 11:48:04 Mark Brown wrote:
>> On Thu, Jul 11, 2013 at 12:38:24PM +0530, Padmavathi Venna wrote:
>> > -#define MOD_LR_LLOW                (0 << 7)
>> > -#define MOD_LR_RLOW                (1 << 7)
>> > -#define MOD_SDF_IIS                (0 << 5)
>> > -#define MOD_SDF_MSB                (1 << 5)
>> > -#define MOD_SDF_LSB                (2 << 5)
>> > -#define MOD_SDF_MASK               (3 << 5)
>> >
>> > +#define MOD_LR_LLOW                0
>> > +#define MOD_LR_RLOW                1
>> > +#define MOD_SDF_SHIFT              5
>> > +#define MOD_SDF_IIS                0
>> > +#define MOD_SDF_MSB                1
>> > +#define MOD_SDF_LSB                2
>> > +#define MOD_SDF_MASK               3
>>
>> This patch has an awful lot of coding style changes like this which
>> are just coding style changes and not implementing TDM support.  These
>> should be done separately, not as part of the same patch, in order to
>> make the code easier to review.
>>
>> >     case 768:
>> > -           mod |= MOD_RCLK_768FS;
>> > +           mod |= (MOD_RCLK_768FS << rfs_shift);
>> >
>> >             break;
>>
>> This stuff is another example.
>>
>> I think the change itself should be fine but I'm not 100% sure I'm
>> correctly identifying what's a stylistic change and what's a functional
>> change.
>
> Right. This could be split into two patches, first reworking the style to give
> more flexibility with operations on registers and another one adding TDM
> specific changes, like new bitfield definitions and conditional handling of
> register accesses to account for new bitfield locations.
>
> Best regards,
> Tomasz
>

Ok. I will bisect the changes into two patches as suggested.

Thanks
Padma

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padma Venkat]

Hi,

On Thu, Jul 11, 2013 at 6:31 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote:
>> On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:
>
>> > This is a bit of a larger project sadly, there was resistance to doing
>> > the DT bindings based on compatible strings using the IP version :(
>
>> I'm not sure if this case is related to this problem in any way. I just
>> suggested introducing a new include like samsung,exynos5420-i2s, a opposed to
>> using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is
>
> Well, it *should* be samsung,i2s-vN where N is the version number of the
> IP but documentation on the versioning has been patchy hence this whole
> thinng.
>

A new version number is added when a there was some change in the IP
like adding a internal mux to the IP, adding multi channel support,
adding reset control bit. This was done in the older code with
platform device.
Same thing is followed here. So as previously done, adding a new
compatible name like "samsung,i2s-v6" with new quirk
"samsung,supports-tdm" is okey?

I will mention about this versioning info in the Documentation in the
next patch.

>> the first one to have this TDM or whatever mode. (I'd still like to hear what
>> it is...)
>
> What it *should* be is the option to send more than one audio stream
> down a link using time division multiplexing.

Thanks
Padma

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padma Venkat]

Hi,

On Thu, Jul 11, 2013 at 6:31 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote:
>> On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:
>
>> > This is a bit of a larger project sadly, there was resistance to doing
>> > the DT bindings based on compatible strings using the IP version :(
>
>> I'm not sure if this case is related to this problem in any way. I just
>> suggested introducing a new include like samsung,exynos5420-i2s, a opposed to
>> using samsung,exynos5250-i2s and adding flags, since the I2S in Exynos5420 is
>
> Well, it *should* be samsung,i2s-vN where N is the version number of the
> IP but documentation on the versioning has been patchy hence this whole
> thinng.
>

A new version number is added when a there was some change in the IP
like adding a internal mux to the IP, adding multi channel support,
adding reset control bit. This was done in the older code with
platform device.
Same thing is followed here. So as previously done, adding a new
compatible name like "samsung,i2s-v6" with new quirk
"samsung,supports-tdm" is okey?

I will mention about this versioning info in the Documentation in the
next patch.

>> the first one to have this TDM or whatever mode. (I'd still like to hear what
>> it is...)
>
> What it *should* be is the option to send more than one audio stream
> down a link using time division multiplexing.

Thanks
Padma

Re: [PATCH] ASoC: Samsung: Set RFS and BFS in slave mode

[Padma Venkat]

Hi,

On Thu, Jul 11, 2013 at 4:41 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 11, 2013 at 12:38:25PM +0530, Padmavathi Venna wrote:
>> As per the User Manual, the RFS and BFS should be set in slave mode
>> for correct operation.
>
> Applied, thanks.  Since this is a fix it should have been the first
> patch in the series - this allows fixes to be sent to
>
>> Reviewed-on: https://gerrit-int.chromium.org/37841
>
> Including things like this isn't terribly helpful - this is the Chromium
> internal system and nobody in the community can view the review.  A link
> to a public system might be useful but a private one should be omitted.

I thought it was link to the public gerrit review system. Will remove next time.

Thanks
Padma

[PATCH] ASoC: Samsung: Set RFS and BFS in slave mode

[Padma Venkat]

Hi,

On Thu, Jul 11, 2013 at 4:41 PM, Mark Brown <broonie@kernel.org> wrote:
> On Thu, Jul 11, 2013 at 12:38:25PM +0530, Padmavathi Venna wrote:
>> As per the User Manual, the RFS and BFS should be set in slave mode
>> for correct operation.
>
> Applied, thanks.  Since this is a fix it should have been the first
> patch in the series - this allows fixes to be sent to
>
>> Reviewed-on: https://gerrit-int.chromium.org/37841
>
> Including things like this isn't terribly helpful - this is the Chromium
> internal system and nobody in the community can view the review.  A link
> to a public system might be useful but a private one should be omitted.

I thought it was link to the public gerrit review system. Will remove next time.

Thanks
Padma

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 564 bytes --]

On Fri, Jul 12, 2013 at 10:07:22AM +0530, Padma Venkat wrote:

> A new version number is added when a there was some change in the IP
> like adding a internal mux to the IP, adding multi channel support,
> adding reset control bit. This was done in the older code with
> platform device.
> Same thing is followed here. So as previously done, adding a new
> compatible name like "samsung,i2s-v6" with new quirk
> "samsung,supports-tdm" is okey?

Yes, this is good but the quirk should just be found by software based
on the compatible string rather than in the DT.

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

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

On Fri, Jul 12, 2013 at 10:07:22AM +0530, Padma Venkat wrote:

> A new version number is added when a there was some change in the IP
> like adding a internal mux to the IP, adding multi channel support,
> adding reset control bit. This was done in the older code with
> platform device.
> Same thing is followed here. So as previously done, adding a new
> compatible name like "samsung,i2s-v6" with new quirk
> "samsung,supports-tdm" is okey?

Yes, this is good but the quirk should just be found by software based
on the compatible string rather than in the DT.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130712/957b5b54/attachment.sig>

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

On Thursday 11 of July 2013 14:01:39 Mark Brown wrote:
> On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote:
> > On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:
> > > This is a bit of a larger project sadly, there was resistance to doing
> > > the DT bindings based on compatible strings using the IP version :(
> > 
> > I'm not sure if this case is related to this problem in any way. I just
> > suggested introducing a new include like samsung,exynos5420-i2s, a opposed
> > to using samsung,exynos5250-i2s and adding flags, since the I2S in
> > Exynos5420 is
> Well, it *should* be samsung,i2s-vN where N is the version number of the
> IP but documentation on the versioning has been patchy hence this whole
> thinng.

Oh, the binding for samsung-i2s uses IP versions. Isn't that slightly broken?

At least with Samsung bindings, I think we happened to agree on a single 
convention of naming compatible values after the first SoC in which such block 
first appeared.

I wonder what happened with this i2s binding that made it end up outside this 
convention.

> > the first one to have this TDM or whatever mode. (I'd still like to hear
> > what it is...)
> 
> What it *should* be is the option to send more than one audio stream
> down a link using time division multiplexing.

OK.

Best regards,
Tomasz

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Tomasz Figa]

On Thursday 11 of July 2013 14:01:39 Mark Brown wrote:
> On Thu, Jul 11, 2013 at 01:41:40PM +0200, Tomasz Figa wrote:
> > On Thursday 11 of July 2013 12:20:23 Mark Brown wrote:
> > > This is a bit of a larger project sadly, there was resistance to doing
> > > the DT bindings based on compatible strings using the IP version :(
> > 
> > I'm not sure if this case is related to this problem in any way. I just
> > suggested introducing a new include like samsung,exynos5420-i2s, a opposed
> > to using samsung,exynos5250-i2s and adding flags, since the I2S in
> > Exynos5420 is
> Well, it *should* be samsung,i2s-vN where N is the version number of the
> IP but documentation on the versioning has been patchy hence this whole
> thinng.

Oh, the binding for samsung-i2s uses IP versions. Isn't that slightly broken?

At least with Samsung bindings, I think we happened to agree on a single 
convention of naming compatible values after the first SoC in which such block 
first appeared.

I wonder what happened with this i2s binding that made it end up outside this 
convention.

> > the first one to have this TDM or whatever mode. (I'd still like to hear
> > what it is...)
> 
> What it *should* be is the option to send more than one audio stream
> down a link using time division multiplexing.

OK.

Best regards,
Tomasz

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 874 bytes --]

On Fri, Jul 12, 2013 at 12:13:17PM +0200, Tomasz Figa wrote:
> On Thursday 11 of July 2013 14:01:39 Mark Brown wrote:

> > Well, it *should* be samsung,i2s-vN where N is the version number of the
> > IP but documentation on the versioning has been patchy hence this whole
> > thinng.

> Oh, the binding for samsung-i2s uses IP versions. Isn't that slightly broken?

> At least with Samsung bindings, I think we happened to agree on a single 
> convention of naming compatible values after the first SoC in which such block 
> first appeared.

> I wonder what happened with this i2s binding that made it end up outside this 
> convention.

I think it predates that convention, plus it's been quite common to put
two different I2S blocks in the same SoC (for totally sensible reasons)
so it's not always been clear that the SoC name is sufficiently unique.

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

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

On Fri, Jul 12, 2013 at 12:13:17PM +0200, Tomasz Figa wrote:
> On Thursday 11 of July 2013 14:01:39 Mark Brown wrote:

> > Well, it *should* be samsung,i2s-vN where N is the version number of the
> > IP but documentation on the versioning has been patchy hence this whole
> > thinng.

> Oh, the binding for samsung-i2s uses IP versions. Isn't that slightly broken?

> At least with Samsung bindings, I think we happened to agree on a single 
> convention of naming compatible values after the first SoC in which such block 
> first appeared.

> I wonder what happened with this i2s binding that made it end up outside this 
> convention.

I think it predates that convention, plus it's been quite common to put
two different I2S blocks in the same SoC (for totally sensible reasons)
so it's not always been clear that the SoC name is sufficiently unique.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130712/1898d978/attachment.sig>

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padma Venkat]

Hi Mark,

On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jul 12, 2013 at 10:07:22AM +0530, Padma Venkat wrote:
>
>> A new version number is added when a there was some change in the IP
>> like adding a internal mux to the IP, adding multi channel support,
>> adding reset control bit. This was done in the older code with
>> platform device.
>> Same thing is followed here. So as previously done, adding a new
>> compatible name like "samsung,i2s-v6" with new quirk
>> "samsung,supports-tdm" is okey?
>
> Yes, this is good but the quirk should just be found by software based
> on the compatible string rather than in the DT.

You mean I just need to initialize the quirks in the driver based on
the compatible string?
So what about the quirks that I added previously on 5250? Should I
also follow the same thing for other quirks?

Or it need to be passed via driver data as it is done in spi driver?
Please clarify me.

Thanks
Padma

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Padma Venkat]

Hi Mark,

On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown <broonie@kernel.org> wrote:
> On Fri, Jul 12, 2013 at 10:07:22AM +0530, Padma Venkat wrote:
>
>> A new version number is added when a there was some change in the IP
>> like adding a internal mux to the IP, adding multi channel support,
>> adding reset control bit. This was done in the older code with
>> platform device.
>> Same thing is followed here. So as previously done, adding a new
>> compatible name like "samsung,i2s-v6" with new quirk
>> "samsung,supports-tdm" is okey?
>
> Yes, this is good but the quirk should just be found by software based
> on the compatible string rather than in the DT.

You mean I just need to initialize the quirks in the driver based on
the compatible string?
So what about the quirks that I added previously on 5250? Should I
also follow the same thing for other quirks?

Or it need to be passed via driver data as it is done in spi driver?
Please clarify me.

Thanks
Padma

Re: [PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 712 bytes --]

On Tue, Jul 23, 2013 at 09:38:00AM +0530, Padma Venkat wrote:
> On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown <broonie@kernel.org> wrote:

> > Yes, this is good but the quirk should just be found by software based
> > on the compatible string rather than in the DT.

> You mean I just need to initialize the quirks in the driver based on
> the compatible string?

That's the gold standard.

> So what about the quirks that I added previously on 5250? Should I
> also follow the same thing for other quirks?

Yes, ideally.

> Or it need to be passed via driver data as it is done in spi driver?
> Please clarify me.

This is a good way to initialise things based on the name - the core
will do the lookups for you.

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

[PATCH] ASoC: Samsung: Modify the I2S driver to support I2S on Exynos5420

[Mark Brown]

On Tue, Jul 23, 2013 at 09:38:00AM +0530, Padma Venkat wrote:
> On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown <broonie@kernel.org> wrote:

> > Yes, this is good but the quirk should just be found by software based
> > on the compatible string rather than in the DT.

> You mean I just need to initialize the quirks in the driver based on
> the compatible string?

That's the gold standard.

> So what about the quirks that I added previously on 5250? Should I
> also follow the same thing for other quirks?

Yes, ideally.

> Or it need to be passed via driver data as it is done in spi driver?
> Please clarify me.

This is a good way to initialise things based on the name - the core
will do the lookups for you.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20130723/09c53da6/attachment.sig>