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

2013-07-23 Thread 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 broo...@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.


signature.asc
Description: Digital signature

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

2013-07-22 Thread Padma Venkat 
Hi Mark,

On Fri, Jul 12, 2013 at 2:19 PM, Mark Brown broo...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-07-12 Thread 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.


signature.asc
Description: Digital signature

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

2013-07-12 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-07-12 Thread 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.


signature.asc
Description: Digital signature

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

2013-07-11 Thread 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 padm...@samsung.com
[abrestic: style cleanup and documentation]
Signed-off-by: Andrew Bresticker abres...@chromium.org
Reviewed-on: https://gerrit-int.chromium.org/37840
Reviewed-by: Simon Glass s...@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 I2SLVL1ADDR0x34
 #define I2SLVL2ADDR0x38
 #define I2SLVL3ADDR0x3c
+#define I2SSTR10x40
+#define I2SVER 0x44
+#define I2SFIC20x48
+#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_LLOW0
+#define MOD_LR_RLOW1
+#define MOD_SDF_SHIFT  5
+#define MOD_SDF_IIS0
+#define MOD_SDF_MSB1
+#define MOD_SDF_LSB2
+#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;

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

2013-07-11 Thread 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 padm...@samsung.com
 [abrestic: style cleanup and documentation]
 Signed-off-by: Andrew Bresticker abres...@chromium.org
 Reviewed-on: https://gerrit-int.chromium.org/37840
 Reviewed-by: Simon Glass s...@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_SHIFT7
 +#define MOD_LR_LLOW  0
 +#define MOD_LR_RLOW  1
 +#define MOD_SDF_SHIFT5
 +#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_MASK3
 +#define MOD_BCLK_SHIFT   1
 +#define MOD_BCLK_32FS0
 +#define MOD_BCLK_48FS1
 +#define MOD_BCLK_16FS2
 +#define MOD_BCLK_24FS3
 +#define MOD_BCLK_MASK3
  #define MOD_8BIT (1  0)
 
 +#define EXYNOS5420_MOD_LRP_SHIFT 15
 +#define EXYNOS5420_MOD_SDF_SHIFT 6
 +#define EXYNOS5420_MOD_RCLK_SHIFT4
 +#define EXYNOS5420_MOD_BCLK_SHIFT0
 +#define EXYNOS5420_MOD_BCLK_64FS 4
 +#define EXYNOS5420_MOD_BCLK_96FS 5
 +#define EXYNOS5420_MOD_BCLK_128FS6
 +#define EXYNOS5420_MOD_BCLK_192FS7
 +#define EXYNOS5420_MOD_BCLK_256FS8
 +#define EXYNOS5420_MOD_BCLK_MASK 0xf
 +
  #define MOD_CDCLKCON (1  12)
 
  #define PSR_PSREN

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

2013-07-11 Thread 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_SHIFT5
 +#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.


signature.asc
Description: Digital signature

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

2013-07-11 Thread 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_LLOW0
  +#define MOD_LR_RLOW1
  +#define MOD_SDF_SHIFT  5
  +#define MOD_SDF_IIS0
  +#define MOD_SDF_MSB1
  +#define MOD_SDF_LSB2
  +#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

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-07-11 Thread 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 :(


signature.asc
Description: Digital signature

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

2013-07-11 Thread 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

--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-07-11 Thread 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.


signature.asc
Description: Digital signature

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

2013-07-11 Thread Padma Venkat 
On Thu, Jul 11, 2013 at 4:37 PM, Tomasz Figa tomasz.f...@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_LLOW0
  +#define MOD_LR_RLOW1
  +#define MOD_SDF_SHIFT  5
  +#define MOD_SDF_IIS0
  +#define MOD_SDF_MSB1
  +#define MOD_SDF_LSB2
  +#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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

2013-07-11 Thread Padma Venkat 
Hi,

On Thu, Jul 11, 2013 at 6:31 PM, Mark Brown broo...@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
--
To unsubscribe from this list: send the line unsubscribe linux-samsung-soc in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html