Skip to contentSkip to conversation

ASoC: sun4i-i2s: Updates to the driver

PATCH Superseded [email protected] 4 participants 20 messages Dec 21, 2018 – Jan 9, 2019

Subsystems (3)
Sound ASoC Arm/Allwinner sunXi SoC support

v3 of 6 Superseded Latest is v6 →

lore
Share & tools
lore

[email protected]

author
#0 lore raw

v3 cover ASoC: sun4i-i2s: Updates to the driver

From: Marcus Cooper <[email protected]>

Hi All,

here is a patch series to add some improvements to the sun4i-i2s driver found whilst getting slave clocking and hdmi audio working on the newer SoCs. Since the last push there has been some activity getting surround sound working and this is included.

The functionality included with the new patch set has been extended to cover more sample resolutions, multi-lane data output for HDMI audio and some bug fixes that have been discovered along the way.

I can see more usage of the tdm property since I last attempted to push these patches and the examples currently in mainline sort of the opposite to what I'm trying to achieve. When we first started looking at the i2s driver, the codecs that we were using allowed for the frame width to be determined based on the sampling resolution but in most use cases it seems that a fixed width is required(my highest priority should be to get HDMI audio support in). We're using the tdm property to override the old way to calculate the frame width. What I've seen in what has already been mainlined is that the i2s driver has a frame width that is fixed to 32 bits and this can be overridden using the tdm property.

Anyway, I've moved the more controversial patches to the top of the stack as I'm expecting feedback.

Have a great Xmas and New Year,

BR,
CK

---

v3 changes compared to v2 are:
- added back slave mode changes
- added back the use of tdm properties
- changes to regmap and caching
- removed loopback functionality
- fixes to the channel offset mask

v2 changes compared to v1 are:
- removed slave mode changes which didn't set mclk and bclk div.
- removed use of tdm and now use a dedicated property.
- fix commit message to better explain reason for sign extending
- add divider calculations for newer SoCs.
- add support for multi-lane i2s data output.
- add support for 20, 24 and 32 bit samples.
- add loopback property so blocks can be tested without a codec.

---

Marcus Cooper (9):
ASoC: sun4i-i2s: Adjust regmap settings
ASoC: sun4i-i2s: Add regmap field to sign extend sample
ASoc: sun4i-i2s: Add 20, 24 and 32 bit support
ASoC: sun4i-i2s: Fix offset mask
ASoC: sun4i-i2s: Correct divider calculations
ASoC: sun4i-i2s: Add multi-lane functionality
ASoC: sun4i-i2s: Do not divide clocks when slave
ASoC: sun4i-i2s: Add set_tdm_slot functionality
ASoC: sun4i-i2s: Add multichannel functionality

 sound/soc/sunxi/sun4i-i2s.c | 399 ++++++++++++++++++++++++------------
 1 file changed, 267 insertions(+), 132 deletions(-)
Signature · 1 line
2.20.1
author
#1 lore raw

PATCH v3 1/9 ASoC: sun4i-i2s: Adjust regmap settings

From: Marcus Cooper <[email protected]>

Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +9 -20
index d5ec1a20499d..64d073cb2aee 100644
@@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 {
 	/* Flush RX FIFO */
+	regcache_cache_bypass(i2s->regmap, true);
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
+	regcache_cache_bypass(i2s->regmap, false);
 
 	/* Clear RX counter */
 	regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);

@@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
 {
 	/* Flush TX FIFO */
+	regcache_cache_bypass(i2s->regmap, true);
 	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
 			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
+	regcache_cache_bypass(i2s->regmap, false);
 
 	/* Clear TX counter */
 	regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);

@@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
 
 static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
 {
-	switch (reg) {
-	case SUN4I_I2S_FIFO_TX_REG:
-		return false;
-
-	default:
-		return true;
-	}
+	return true;
 }
 
 static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)

@@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
 	switch (reg) {
 	case SUN4I_I2S_FIFO_RX_REG:
+	case SUN4I_I2S_FIFO_TX_REG:
+	case SUN4I_I2S_FIFO_STA_REG:
 	case SUN4I_I2S_INT_STA_REG:
 	case SUN4I_I2S_RX_CNT_REG:
 	case SUN4I_I2S_TX_CNT_REG:

@@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 	}
 }
 
-static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
-{
-	switch (reg) {
-	case SUN8I_I2S_FIFO_TX_REG:
-		return false;
-
-	default:
-		return true;
-	}
-}
-
 static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
 	if (reg == SUN8I_I2S_INT_STA_REG)
 		return true;
 	if (reg == SUN8I_I2S_FIFO_TX_REG)
-		return false;
+		return true;
 
 	return sun4i_i2s_volatile_reg(dev, reg);
 }

@@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
 	.reg_defaults	= sun8i_i2s_reg_defaults,
 	.num_reg_defaults	= ARRAY_SIZE(sun8i_i2s_reg_defaults),
 	.writeable_reg	= sun4i_i2s_wr_reg,
-	.readable_reg	= sun8i_i2s_rd_reg,
+	.readable_reg	= sun4i_i2s_rd_reg,
 	.volatile_reg	= sun8i_i2s_volatile_reg,
 };
Signature · 1 line
2.20.1
#2 lore raw

Re: [PATCH v3 1/9] ASoC: sun4i-i2s: Adjust regmap settings

Quoted 16 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]…

From: Marcus Cooper <[email protected]>

Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +0 -0
index d5ec1a20499d..64d073cb2aee 100644

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 555

Quoted 8 lines
sound/soc/sunxi/sun4i-i2s.c +2 -0
@@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 {
        /* Flush RX FIFO */
+       regcache_cache_bypass(i2s->regmap, true);
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
+       regcache_cache_bypass(i2s->regmap, false);

IIRC the flush cache bit is self-clearing. So you likely want to mark this register as volatile. If it is marked as volatile, then all access to that register bypasses the cache, so the regcache_cache_bypass calls are unneeded.

However, looking at the code, the write would seem to be ignored if the regmap is in the cache_only state. We only set this when the bus clock is disabled. Under such a condition, bypassing the cache and forcing a write would be unwise, as the system either drops the write, or stalls altogether.

Quoted 17 lines
sound/soc/sunxi/sun4i-i2s.c +2 -0
        /* Clear RX counter */
        regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);

@@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
 {
        /* Flush TX FIFO */
+       regcache_cache_bypass(i2s->regmap, true);
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
+       regcache_cache_bypass(i2s->regmap, false);
 
        /* Clear TX counter */
        regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);

@@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {

sound/soc/sunxi/sun4i-i2s.c static const struct snd_soc_component_driver sun4i_i2s_component = { · line 710

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +1 -7
 static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
 {
-       switch (reg) {
-       case SUN4I_I2S_FIFO_TX_REG:
-               return false;
-
-       default:
-               return true;
-       }
+       return true;

I don't understand why this is relevant. Do you need to read back from the TX FIFO?

If so, just drop the call-back altogether. If no callback is provided and no rd_table is provided either, it defaults to all registers under max_register (if max_register < 0) are readable.

However this seems like it deserves to be a separate patch (where you explain in the commit log why it's needed).

Regards
ChenYu

Quoted 49 lines
sound/soc/sunxi/sun4i-i2s.c +4 -14
 }
 
 static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)

@@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
        switch (reg) {
        case SUN4I_I2S_FIFO_RX_REG:
+       case SUN4I_I2S_FIFO_TX_REG:
+       case SUN4I_I2S_FIFO_STA_REG:
        case SUN4I_I2S_INT_STA_REG:
        case SUN4I_I2S_RX_CNT_REG:
        case SUN4I_I2S_TX_CNT_REG:

@@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
        }
 }
 
-static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
-{
-       switch (reg) {
-       case SUN8I_I2S_FIFO_TX_REG:
-               return false;
-
-       default:
-               return true;
-       }
-}
-
 static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
        if (reg == SUN8I_I2S_INT_STA_REG)
                return true;
        if (reg == SUN8I_I2S_FIFO_TX_REG)
-               return false;
+               return true;
 
        return sun4i_i2s_volatile_reg(dev, reg);
 }

@@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
        .reg_defaults   = sun8i_i2s_reg_defaults,
        .num_reg_defaults       = ARRAY_SIZE(sun8i_i2s_reg_defaults),
        .writeable_reg  = sun4i_i2s_wr_reg,
-       .readable_reg   = sun8i_i2s_rd_reg,
+       .readable_reg   = sun4i_i2s_rd_reg,
        .volatile_reg   = sun8i_i2s_volatile_reg,
 };
 
--

2.20.1

#3 lore raw

Re: [alsa-devel] [PATCH v3 1/9] ASoC: sun4i-i2s: Adjust regmap settings

Quoted 20 lines · Chen-Yu Tsai
Quoted 19 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:

From: Marcus Cooper <[email protected]>

Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +1 -0
index d5ec1a20499d..64d073cb2aee 100644
@@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 {
        /* Flush RX FIFO */
+       regcache_cache_bypass(i2s->regmap, true);
Quoted 8 lines

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 555

Quoted 4 lines
sound/soc/sunxi/sun4i-i2s.c +1 -0
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
+       regcache_cache_bypass(i2s->regmap, false);

IIRC the flush cache bit is self-clearing. So you likely want to mark this register as volatile. If it is marked as volatile, then all access to that register bypasses the cache, so the regcache_cache_bypass calls are unneeded.

I helped test this together with Marcus and when I tested with this register marked
as volatile audio started to stutter, still unclear why audio starts to stutter.

Without this cache bypass the flush TX/RX bits gets cached and flush happens unexpectedly
resulting in multi channel audio getting mapped to wrong speaker. Other ASoC codecs and fsl_spdif.c seems to use similar cache bypass for reset/flush.

Quoted 23 lines

However, looking at the code, the write would seem to be ignored if the regmap is in the cache_only state. We only set this when the bus clock is disabled. Under such a condition, bypassing the cache and forcing a write would be unwise, as the system either drops the write, or stalls altogether.

Quoted 16 lines
sound/soc/sunxi/sun4i-i2s.c +2 -0
        /* Clear RX counter */
        regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);

@@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
 {
        /* Flush TX FIFO */
+       regcache_cache_bypass(i2s->regmap, true);
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
+       regcache_cache_bypass(i2s->regmap, false);
 
        /* Clear TX counter */
        regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);

@@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
Quoted 12 lines

sound/soc/sunxi/sun4i-i2s.c static const struct snd_soc_component_driver sun4i_i2s_component = { · line 710

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +1 -7
 static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
 {
-       switch (reg) {
-       case SUN4I_I2S_FIFO_TX_REG:
-               return false;
-
-       default:
-               return true;
-       }
+       return true;

I don't understand why this is relevant. Do you need to read back from the TX FIFO?

This change was inspired by c66234cfedfc "ASoC: rockchip: i2s: fix playback after runtime resume" [1]
On resume a cached sample would be written to FIFO_TX_REG unless it is marked volatile,
the rockchip commit indicated that read is needed for volatile regs.

[1]
https://github.com/torvalds/linux/commit/c66234cfedfc3e6e3b62563a5f2c1562be09a35d

Quoted 64 lines

If so, just drop the call-back altogether. If no callback is provided and no rd_table is provided either, it defaults to all registers under max_register (if max_register < 0) are readable.

However this seems like it deserves to be a separate patch (where you explain in the commit log why it's needed).

Regards
ChenYu

Quoted 47 lines
sound/soc/sunxi/sun4i-i2s.c +4 -14
 }
 
 static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)

@@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
        switch (reg) {
        case SUN4I_I2S_FIFO_RX_REG:
+       case SUN4I_I2S_FIFO_TX_REG:
+       case SUN4I_I2S_FIFO_STA_REG:
        case SUN4I_I2S_INT_STA_REG:
        case SUN4I_I2S_RX_CNT_REG:
        case SUN4I_I2S_TX_CNT_REG:

@@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
        }
 }
 
-static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
-{
-       switch (reg) {
-       case SUN8I_I2S_FIFO_TX_REG:
-               return false;
-
-       default:
-               return true;
-       }
-}
-
 static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
        if (reg == SUN8I_I2S_INT_STA_REG)
                return true;
        if (reg == SUN8I_I2S_FIFO_TX_REG)
-               return false;
+               return true;
 
        return sun4i_i2s_volatile_reg(dev, reg);
 }

@@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
        .reg_defaults   = sun8i_i2s_reg_defaults,
        .num_reg_defaults       = ARRAY_SIZE(sun8i_i2s_reg_defaults),
        .writeable_reg  = sun4i_i2s_wr_reg,
-       .readable_reg   = sun8i_i2s_rd_reg,
+       .readable_reg   = sun4i_i2s_rd_reg,
        .volatile_reg   = sun8i_i2s_volatile_reg,
 };
 
--
Quoted 2 lines

2.20.1

_______________________________________________
Alsa-devel mailing list
[email protected]

#4 lore raw

Re: [alsa-devel] [PATCH v3 1/9] ASoC: sun4i-i2s: Adjust regmap settings

Quoted 31 lines · Jonas Karlman
Quoted 20 lines · Chen-Yu Tsai
Quoted 19 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]…

From: Marcus Cooper <[email protected]>

Bypass the regmap cache when flushing the i2s FIFOs and modify the tables to reflect this.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++--------------------
 1 file changed, 9 insertions(+), 20 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +1 -0
index d5ec1a20499d..64d073cb2aee 100644
@@ -548,9 +548,11 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 {
        /* Flush RX FIFO */
+       regcache_cache_bypass(i2s->regmap, true);
Quoted 8 lines

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 555

Quoted 4 lines
sound/soc/sunxi/sun4i-i2s.c +1 -0
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
+       regcache_cache_bypass(i2s->regmap, false);

IIRC the flush cache bit is self-clearing. So you likely want to mark this register as volatile. If it is marked as volatile, then all access to that register bypasses the cache, so the regcache_cache_bypass calls are unneeded.

Quoted 4 lines

I helped test this together with Marcus and when I tested with this register marked
as volatile audio started to stutter, still unclear why audio starts to stutter.

Sounds like we're missing something. However the only other time we update this register is to set the FIFO mode.

Quoted 3 lines

Without this cache bypass the flush TX/RX bits gets cached and flush happens unexpectedly
resulting in multi channel audio getting mapped to wrong speaker.

This sounds like the flush is happening after DMA transfers and/or I2S operations have started, disrupting the order of the audio samples. I think that might be the case since the regcache is synced sequentially, and the FIFO control register is after the enable bits. That would imply that the device is taken out of runtime suspend after the .start_capture or .start_playback callbacks. Not sure if that's the case, but that would mean the bus clocks are still off at this point, and bypassing the cache and updating the bits is basically moot.

I think there's something else happening here, but we need to figure it out instead of papering over it with something that "just works" but we don't know why it works.

Quoted 2 lines

Other ASoC codecs and fsl_spdif.c seems to use similar cache bypass for reset/flush.

fsl_spdif.c does it when forcing a soft reset, which would reset all the registers. It then does a cache sync, restoring any values set up. That's not what we're doing here.

Quoted 36 lines
Quoted 23 lines

However, looking at the code, the write would seem to be ignored if the regmap is in the cache_only state. We only set this when the bus clock is disabled. Under such a condition, bypassing the cache and forcing a write would be unwise, as the system either drops the write, or stalls altogether.

Quoted 16 lines
sound/soc/sunxi/sun4i-i2s.c +2 -0
        /* Clear RX counter */
        regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0);

@@ -569,9 +571,11 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
 static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
 {
        /* Flush TX FIFO */
+       regcache_cache_bypass(i2s->regmap, true);
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_TX);
+       regcache_cache_bypass(i2s->regmap, false);
 
        /* Clear TX counter */
        regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0);

@@ -703,13 +707,7 @@ static const struct snd_soc_component_driver sun4i_i2s_component = {
Quoted 12 lines

sound/soc/sunxi/sun4i-i2s.c static const struct snd_soc_component_driver sun4i_i2s_component = { · line 710

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +1 -7
 static bool sun4i_i2s_rd_reg(struct device *dev, unsigned int reg)
 {
-       switch (reg) {
-       case SUN4I_I2S_FIFO_TX_REG:
-               return false;
-
-       default:
-               return true;
-       }
+       return true;

I don't understand why this is relevant. Do you need to read back from the TX FIFO?

Quoted 8 lines

This change was inspired by c66234cfedfc "ASoC: rockchip: i2s: fix playback after runtime resume" [1]
On resume a cached sample would be written to FIFO_TX_REG unless it is marked volatile,
the rockchip commit indicated that read is needed for volatile regs.

[1]
https://github.com/torvalds/linux/commit/c66234cfedfc3e6e3b62563a5f2c1562be09a35d

What about simply marking the FIFO registers as both unreadable and unwritable, thus excluding them from the regmap? That should get rid of any unwanted syncs. We are doing DMA transfers to/from them. Do we need regmap access?

Either way this context should be provided in the commit log.

Regards
ChenYu

Quoted 64 lines
Quoted 64 lines

If so, just drop the call-back altogether. If no callback is provided and no rd_table is provided either, it defaults to all registers under max_register (if max_register < 0) are readable.

However this seems like it deserves to be a separate patch (where you explain in the commit log why it's needed).

Regards
ChenYu

Quoted 47 lines
sound/soc/sunxi/sun4i-i2s.c +4 -14
 }
 
 static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)

@@ -728,6 +726,8 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
        switch (reg) {
        case SUN4I_I2S_FIFO_RX_REG:
+       case SUN4I_I2S_FIFO_TX_REG:
+       case SUN4I_I2S_FIFO_STA_REG:
        case SUN4I_I2S_INT_STA_REG:
        case SUN4I_I2S_RX_CNT_REG:
        case SUN4I_I2S_TX_CNT_REG:

@@ -738,23 +738,12 @@ static bool sun4i_i2s_volatile_reg(struct device *dev, unsigned int reg)
        }
 }
 
-static bool sun8i_i2s_rd_reg(struct device *dev, unsigned int reg)
-{
-       switch (reg) {
-       case SUN8I_I2S_FIFO_TX_REG:
-               return false;
-
-       default:
-               return true;
-       }
-}
-
 static bool sun8i_i2s_volatile_reg(struct device *dev, unsigned int reg)
 {
        if (reg == SUN8I_I2S_INT_STA_REG)
                return true;
        if (reg == SUN8I_I2S_FIFO_TX_REG)
-               return false;
+               return true;
 
        return sun4i_i2s_volatile_reg(dev, reg);
 }

@@ -809,7 +798,7 @@ static const struct regmap_config sun8i_i2s_regmap_config = {
        .reg_defaults   = sun8i_i2s_reg_defaults,
        .num_reg_defaults       = ARRAY_SIZE(sun8i_i2s_reg_defaults),
        .writeable_reg  = sun4i_i2s_wr_reg,
-       .readable_reg   = sun8i_i2s_rd_reg,
+       .readable_reg   = sun4i_i2s_rd_reg,
        .volatile_reg   = sun8i_i2s_volatile_reg,
 };
 
--
Quoted 2 lines

2.20.1

_______________________________________________
Alsa-devel mailing list Alsa-devel-K7yf7f+aM1XWsZ/[email protected]

maintainer
#5 lore raw

Re: [alsa-devel] [PATCH v3 1/9] ASoC: sun4i-i2s: Adjust regmap settings

Quoted 8 lines · Chen-Yu Tsai

This sounds like the flush is happening after DMA transfers and/or I2S operations have started, disrupting the order of the audio samples. I think that might be the case since the regcache is synced sequentially, and the FIFO control register is after the enable bits. That would imply that the device is taken out of runtime suspend after the .start_capture or .start_playback callbacks. Not sure if that's the case, but that would mean the bus clocks are still off at this point, and bypassing the cache and updating the bits is basically moot.

I would expect that the device needs to be resumed from suspend before we start actually trying to transfer audio - there is stuff in the ASoC core which is supposed to have appropriate gets but it's possible something is going wrong there.

Quoted 3 lines

I think there's something else happening here, but we need to figure it out instead of papering over it with something that "just works" but we don't know why it works.

I agree.

maintainer
#6 lore raw

Re: [PATCH v3 1/9] ASoC: sun4i-i2s: Adjust regmap settings

In reply to Chen-Yu Tsai
Quoted 8 lines · Chen-Yu Tsai

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt) · line 555

Quoted 5 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:
sound/soc/sunxi/sun4i-i2s.c +2 -0
+       regcache_cache_bypass(i2s->regmap, true);
        regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
                           SUN4I_I2S_FIFO_CTRL_FLUSH_RX);
+       regcache_cache_bypass(i2s->regmap, false);
Quoted 4 lines

IIRC the flush cache bit is self-clearing. So you likely want to mark this register as volatile. If it is marked as volatile, then all access to that register bypasses the cache, so the regcache_cache_bypass calls are unneeded.

Yes, that should be the case.

Quoted 5 lines

However, looking at the code, the write would seem to be ignored if the regmap is in the cache_only state. We only set this when the bus clock is disabled. Under such a condition, bypassing the cache and forcing a write would be unwise, as the system either drops the write, or stalls altogether.

Right, access to a cache only register while the device is in cache only mode is not a great idea - the usual reason we're in cache only mode is that the device is in a state where I/O isn't going to work. One thing that can work for this if you need the register to be cached (but is a bit gross) is to do a write setting the self clearing bit then another immediately after resetting it back to the cleared state. That works OK for cases where the bit is a strobe and never retains state, though if the device isn't operational then needing to write to the register might indicate a bigger picture logic error (or it could be that the register map mixes random things into one register).

_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

author
#7 lore raw

PATCH v3 2/9 ASoC: sun4i-i2s: Add regmap field to sign extend sample

In reply to [email protected]

From: Marcus Cooper <[email protected]>

On the newer SoCs this is set by default to transfer a 0 after each sample in each slot. Add the regmap field to configure this and set it so that it pads the sample with 0s.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 15 +++++++++++++++
 1 file changed, 15 insertions(+)
sound/soc/sunxi/sun4i-i2s.c +15 -0
index 64d073cb2aee..80bf29e0cc86 100644
@@ -138,6 +138,7 @@
  * @field_fmt_bclk: regmap field to set clk polarity.
  * @field_fmt_lrclk: regmap field to set frame polarity.
  * @field_fmt_mode: regmap field to set the operational mode.
+ * @field_fmt_sext: regmap field to set the sign extension.
  * @field_txchanmap: location of the tx channel mapping register.
  * @field_rxchanmap: location of the rx channel mapping register.
  * @field_txchansel: location of the tx channel select bit fields.

@@ -163,6 +164,7 @@ struct sun4i_i2s_quirks {
 	struct reg_field		field_fmt_bclk;
 	struct reg_field		field_fmt_lrclk;
 	struct reg_field		field_fmt_mode;
+	struct reg_field		field_fmt_sext;
 	struct reg_field		field_txchanmap;
 	struct reg_field		field_rxchanmap;
 	struct reg_field		field_txchansel;

@@ -187,6 +189,7 @@ struct sun4i_i2s {
 	struct regmap_field	*field_fmt_bclk;
 	struct regmap_field	*field_fmt_lrclk;
 	struct regmap_field	*field_fmt_mode;
+	struct regmap_field	*field_fmt_sext;
 	struct regmap_field	*field_txchanmap;
 	struct regmap_field	*field_rxchanmap;
 	struct regmap_field	*field_txchansel;

@@ -346,6 +349,9 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
 				   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
 				   SUN8I_I2S_FMT0_LRCK_PERIOD(32));
 
+	/* Set sign extension to pad out LSB with 0 */
+	regmap_field_write(i2s->field_fmt_sext, 0);
+
 	return 0;
 }
 

@@ -876,6 +882,7 @@ static const struct sun4i_i2s_quirks sun4i_a10_i2s_quirks = {
 	.field_fmt_lrclk	= REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7),
 	.has_slave_select_bit	= true,
 	.field_fmt_mode		= REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1),
+	.field_fmt_sext		= REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8),
 	.field_txchanmap	= REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31),
 	.field_rxchanmap	= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
 	.field_txchansel	= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),

@@ -893,6 +900,7 @@ static const struct sun4i_i2s_quirks sun6i_a31_i2s_quirks = {
 	.field_fmt_lrclk	= REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7),
 	.has_slave_select_bit	= true,
 	.field_fmt_mode		= REG_FIELD(SUN4I_I2S_FMT0_REG, 0, 1),
+	.field_fmt_sext		= REG_FIELD(SUN4I_I2S_FMT1_REG, 8, 8),
 	.field_txchanmap	= REG_FIELD(SUN4I_I2S_TX_CHAN_MAP_REG, 0, 31),
 	.field_rxchanmap	= REG_FIELD(SUN4I_I2S_RX_CHAN_MAP_REG, 0, 31),
 	.field_txchansel	= REG_FIELD(SUN4I_I2S_TX_CHAN_SEL_REG, 0, 2),

@@ -933,6 +941,7 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = {
 	.field_fmt_bclk		= REG_FIELD(SUN4I_I2S_FMT0_REG, 7, 7),
 	.field_fmt_lrclk	= REG_FIELD(SUN4I_I2S_FMT0_REG, 19, 19),
 	.field_fmt_mode		= REG_FIELD(SUN4I_I2S_CTRL_REG, 4, 5),
+	.field_fmt_sext		= REG_FIELD(SUN4I_I2S_FMT1_REG, 4, 5),
 	.field_txchanmap	= REG_FIELD(SUN8I_I2S_TX_CHAN_MAP_REG, 0, 31),
 	.field_rxchanmap	= REG_FIELD(SUN8I_I2S_RX_CHAN_MAP_REG, 0, 31),
 	.field_txchansel	= REG_FIELD(SUN8I_I2S_TX_CHAN_SEL_REG, 0, 2),

@@ -995,6 +1004,12 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,
 	if (IS_ERR(i2s->field_fmt_mode))
 		return PTR_ERR(i2s->field_fmt_mode);
 
+	i2s->field_fmt_sext =
+			devm_regmap_field_alloc(dev, i2s->regmap,
+						i2s->variant->field_fmt_sext);
+	if (IS_ERR(i2s->field_fmt_sext))
+		return PTR_ERR(i2s->field_fmt_sext);
+
 	i2s->field_txchanmap =
 			devm_regmap_field_alloc(dev, i2s->regmap,
 						i2s->variant->field_txchanmap);
Signature · 1 line
2.20.1
#8 lore raw

Re: [PATCH v3 2/9] ASoC: sun4i-i2s: Add regmap field to sign extend sample

Quoted 8 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]…

From: Marcus Cooper <[email protected]>

On the newer SoCs this is set by default to transfer a 0 after each sample in each slot. Add the regmap field to configure this and set it so that it pads the sample with 0s.

Signed-off-by Marcus Cooper <[email protected]>

The code matches the description, however I lack the in depth knowledge to understand the difference.

ChenYu

author
#9 lore raw

PATCH v3 3/9 ASoc: sun4i-i2s: Add 20, 24 and 32 bit support

In reply to [email protected]

From: Marcus Cooper <[email protected]>

Extend the functionality of the driver to include support of 20 and 24 bits per sample for the earlier SoCs.

Newer SoCs can also handle 32bit samples.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 41 ++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +38 -3
index 80bf29e0cc86..adb988ae9ac5 100644
@@ -399,6 +399,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	case 16:
 		width = DMA_SLAVE_BUSWIDTH_2_BYTES;
 		break;
+	case 20:
+	case 24:
+	case 32:
+		width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+		break;
 	default:
 		dev_err(dai->dev, "Unsupported physical sample width: %d\n",
 			params_physical_width(params));

@@ -411,7 +416,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 		sr = 0;
 		wss = 0;
 		break;
-
+	case 20:
+		sr = 1;
+		wss = 1;
+		break;
+	case 24:
+		sr = 2;
+		wss = 2;
+		break;
+	case 32:
+		sr = 4;
+		wss = 4;
+		break;
 	default:
 		dev_err(dai->dev, "Unsupported sample width: %d\n",
 			params_width(params));

@@ -687,6 +703,13 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
 	return 0;
 }
 
+#define SUN4I_FORMATS	(SNDRV_PCM_FMTBIT_S16_LE | \
+			 SNDRV_PCM_FMTBIT_S20_3LE | \
+			 SNDRV_PCM_FMTBIT_S24_LE)
+
+#define SUN8I_FORMATS	(SUN4I_FORMATS | \
+			 SNDRV_PCM_FMTBIT_S32_LE)
+
 static struct snd_soc_dai_driver sun4i_i2s_dai = {
 	.probe = sun4i_i2s_dai_probe,
 	.capture = {

@@ -694,14 +717,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_192000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SUN4I_FORMATS,
 	},
 	.playback = {
 		.stream_name = "Playback",
 		.channels_min = 2,
 		.channels_max = 2,
 		.rates = SNDRV_PCM_RATE_8000_192000,
-		.formats = SNDRV_PCM_FMTBIT_S16_LE,
+		.formats = SUN4I_FORMATS,
 	},
 	.ops = &sun4i_i2s_dai_ops,
 	.symmetric_rates = 1,

@@ -1106,6 +1129,18 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 	i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;
 	i2s->capture_dma_data.maxburst = 8;
 
+	soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
+			       sizeof(*soc_dai), GFP_KERNEL);
+	if (!soc_dai) {
+		ret = -ENOMEM;
+		goto err_pm_disable;
+	}
+
+	if (i2s->variant->has_fmt_set_lrck_period) {
+		soc_dai->playback.formats = SUN8I_FORMATS;
+		soc_dai->capture.formats = SUN8I_FORMATS;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = sun4i_i2s_runtime_resume(&pdev->dev);
Signature · 1 line
2.20.1
#10 lore raw

Re: [PATCH v3 3/9] ASoc: sun4i-i2s: Add 20, 24 and 32 bit support

Quoted 50 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]…

From: Marcus Cooper <[email protected]>

Extend the functionality of the driver to include support of 20 and 24 bits per sample for the earlier SoCs.

Newer SoCs can also handle 32bit samples.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 41 ++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +17 -1
index 80bf29e0cc86..adb988ae9ac5 100644
@@ -399,6 +399,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
        case 16:
                width = DMA_SLAVE_BUSWIDTH_2_BYTES;
                break;
+       case 20:
+       case 24:
+       case 32:
+               width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+               break;
        default:
                dev_err(dai->dev, "Unsupported physical sample width: %d\n",
                        params_physical_width(params));

@@ -411,7 +416,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
                sr = 0;
                wss = 0;
                break;
-
+       case 20:
+               sr = 1;
+               wss = 1;
+               break;
+       case 24:
+               sr = 2;
+               wss = 2;
+               break;
+       case 32:
+               sr = 4;
+               wss = 4;
+               break;
        default:
                dev_err(dai->dev, "Unsupported sample width: %d\n",
                        params_width(params));

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai) · line 708

Quoted 6 lines
sound/soc/sunxi/sun4i-i2s.c +3 -0
@@ -687,6 +703,13 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
        return 0;
 }
 
+#define SUN4I_FORMATS  (SNDRV_PCM_FMTBIT_S16_LE | \
+                        SNDRV_PCM_FMTBIT_S20_3LE | \
+                        SNDRV_PCM_FMTBIT_S24_LE)

IIRC SNDRV_PCM_FMTBIT_S24_LE and SNDRV_PCM_FMTBIT_S32_LE with .sig_bits = 24 are different things, in regards to how the sample is aligned in a 4 byte space. Have you checked that things actually work as expected? This was something that did not work using SNDRV_PCM_FMTBIT_S24_LE on sun4i-codec.

Quoted 26 lines
sound/soc/sunxi/sun4i-i2s.c +6 -2
+
+#define SUN8I_FORMATS  (SUN4I_FORMATS | \
+                        SNDRV_PCM_FMTBIT_S32_LE)
+
 static struct snd_soc_dai_driver sun4i_i2s_dai = {
        .probe = sun4i_i2s_dai_probe,
        .capture = {

@@ -694,14 +717,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
                .channels_min = 2,
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_8000_192000,
-               .formats = SNDRV_PCM_FMTBIT_S16_LE,
+               .formats = SUN4I_FORMATS,
        },
        .playback = {
                .stream_name = "Playback",
                .channels_min = 2,
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_8000_192000,
-               .formats = SNDRV_PCM_FMTBIT_S16_LE,
+               .formats = SUN4I_FORMATS,
        },
        .ops = &sun4i_i2s_dai_ops,
        .symmetric_rates = 1,

@@ -1106,6 +1129,18 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
        i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_probe(struct platform_device *pdev) · line 1139

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +8 -0
        i2s->capture_dma_data.maxburst = 8;
 
+       soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
+                              sizeof(*soc_dai), GFP_KERNEL);
+       if (!soc_dai) {
+               ret = -ENOMEM;
+               goto err_pm_disable;
+       }
+
+       if (i2s->variant->has_fmt_set_lrck_period) {

This condition deserves a comment, particularly as to why you use it in place of having a separate quirk field to represent 32 bit support.

Regards
ChenYu

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +4 -1
+               soc_dai->playback.formats = SUN8I_FORMATS;
+               soc_dai->capture.formats = SUN8I_FORMATS;
+       }
+
        pm_runtime_enable(&pdev->dev);
        if (!pm_runtime_enabled(&pdev->dev)) {
                ret = sun4i_i2s_runtime_resume(&pdev->dev);
--

2.20.1

#11 lore raw

Re: [PATCH v3 3/9] ASoc: sun4i-i2s: Add 20, 24 and 32 bit support

Quoted 55 lines · Chen-Yu Tsai
Quoted 53 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]…

From: Marcus Cooper <[email protected]>

Extend the functionality of the driver to include support of 20 and 24 bits per sample for the earlier SoCs.

Newer SoCs can also handle 32bit samples.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 41 ++++++++++++++++++++++++++++++++++---
 1 file changed, 38 insertions(+), 3 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +17 -1
index 80bf29e0cc86..adb988ae9ac5 100644
@@ -399,6 +399,11 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
        case 16:
                width = DMA_SLAVE_BUSWIDTH_2_BYTES;
                break;
+       case 20:
+       case 24:
+       case 32:
+               width = DMA_SLAVE_BUSWIDTH_4_BYTES;
+               break;
        default:
                dev_err(dai->dev, "Unsupported physical sample width: %d\n",
                        params_physical_width(params));

@@ -411,7 +416,18 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
                sr = 0;
                wss = 0;
                break;
-
+       case 20:
+               sr = 1;
+               wss = 1;
+               break;
+       case 24:
+               sr = 2;
+               wss = 2;
+               break;
+       case 32:
+               sr = 4;
+               wss = 4;
+               break;
        default:
                dev_err(dai->dev, "Unsupported sample width: %d\n",
                        params_width(params));

@@ -687,6 +703,13 @@ static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
        return 0;
 }
Quoted 8 lines

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai) · line 708

Quoted 3 lines
sound/soc/sunxi/sun4i-i2s.c +3 -0
+#define SUN4I_FORMATS  (SNDRV_PCM_FMTBIT_S16_LE | \
+                        SNDRV_PCM_FMTBIT_S20_3LE | \
+                        SNDRV_PCM_FMTBIT_S24_LE)

IIRC SNDRV_PCM_FMTBIT_S24_LE and SNDRV_PCM_FMTBIT_S32_LE with .sig_bits = 24 are different things, in regards to how the sample is aligned in a 4 byte space. Have you checked that things actually work as expected? This was something that did not work using SNDRV_PCM_FMTBIT_S24_LE on sun4i-codec.

OK. I see the I2S driver is using FIFO mode 1, which expects the sample to be in the lower portion of the 4 bytes. However it looks like you would want SNDRV_PCM_FMTBIT_S20_LE (20 bits LSB justified in 4 bytes) instead of SNDRV_PCM_FMTBIT_S20_3LE (20 bits LSB justified in 3 bytes).

ChenYu

Quoted 53 lines
Quoted 26 lines
sound/soc/sunxi/sun4i-i2s.c +6 -2
+
+#define SUN8I_FORMATS  (SUN4I_FORMATS | \
+                        SNDRV_PCM_FMTBIT_S32_LE)
+
 static struct snd_soc_dai_driver sun4i_i2s_dai = {
        .probe = sun4i_i2s_dai_probe,
        .capture = {

@@ -694,14 +717,14 @@ static struct snd_soc_dai_driver sun4i_i2s_dai = {
                .channels_min = 2,
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_8000_192000,
-               .formats = SNDRV_PCM_FMTBIT_S16_LE,
+               .formats = SUN4I_FORMATS,
        },
        .playback = {
                .stream_name = "Playback",
                .channels_min = 2,
                .channels_max = 2,
                .rates = SNDRV_PCM_RATE_8000_192000,
-               .formats = SNDRV_PCM_FMTBIT_S16_LE,
+               .formats = SUN4I_FORMATS,
        },
        .ops = &sun4i_i2s_dai_ops,
        .symmetric_rates = 1,

@@ -1106,6 +1129,18 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
        i2s->capture_dma_data.addr = res->start + SUN4I_I2S_FIFO_RX_REG;

sound/soc/sunxi/sun4i-i2s.c static int sun4i_i2s_probe(struct platform_device *pdev) · line 1139

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +8 -0
        i2s->capture_dma_data.maxburst = 8;
 
+       soc_dai = devm_kmemdup(&pdev->dev, &sun4i_i2s_dai,
+                              sizeof(*soc_dai), GFP_KERNEL);
+       if (!soc_dai) {
+               ret = -ENOMEM;
+               goto err_pm_disable;
+       }
+
+       if (i2s->variant->has_fmt_set_lrck_period) {

This condition deserves a comment, particularly as to why you use it in place of having a separate quirk field to represent 32 bit support.

Regards
ChenYu

Quoted 10 lines
sound/soc/sunxi/sun4i-i2s.c +4 -1
+               soc_dai->playback.formats = SUN8I_FORMATS;
+               soc_dai->capture.formats = SUN8I_FORMATS;
+       }
+
        pm_runtime_enable(&pdev->dev);
        if (!pm_runtime_enabled(&pdev->dev)) {
                ret = sun4i_i2s_runtime_resume(&pdev->dev);
--

2.20.1

author
#12 lore raw

PATCH v3 4/9 ASoC: sun4i-i2s: Fix offset mask

In reply to [email protected]

From: Marcus Cooper <[email protected]>

Also add offset to RX channel select

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
sound/soc/sunxi/sun4i-i2s.c +5 -1
index adb988ae9ac5..93a484d7e228 100644
@@ -110,7 +110,7 @@
 
 #define SUN8I_I2S_TX_CHAN_MAP_REG	0x44
 #define SUN8I_I2S_TX_CHAN_SEL_REG	0x34
-#define SUN8I_I2S_TX_CHAN_OFFSET_MASK		GENMASK(13, 11)
+#define SUN8I_I2S_TX_CHAN_OFFSET_MASK		GENMASK(13, 12)
 #define SUN8I_I2S_TX_CHAN_OFFSET(offset)	(offset << 12)
 #define SUN8I_I2S_TX_CHAN_EN_MASK		GENMASK(11, 4)
 #define SUN8I_I2S_TX_CHAN_EN(num_chan)		(((1 << num_chan) - 1) << 4)

@@ -482,6 +482,10 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
 				   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
 				   SUN8I_I2S_TX_CHAN_OFFSET(offset));
+
+		regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
+				   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
+				   SUN8I_I2S_TX_CHAN_OFFSET(offset));
 	}
 
 	regmap_field_write(i2s->field_fmt_mode, val);
Signature · 1 line
2.20.1
#13 lore raw

Re: [PATCH v3 4/9] ASoC: sun4i-i2s: Fix offset mask

Quoted 6 lines · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]…

From: Marcus Cooper <[email protected]>

Also add offset to RX channel select

Signed-off-by Marcus Cooper <[email protected]>

Commit log seems a bit lacking. You could probably explain how you found this, either when comparing datasheet macros, or some actual error manifested and you "heard" it. You could also state what the fixed outcome should be.

Also please add a fixes tag.

The RX channel select offset should be added as a separate patch, also with an explanation on how you found this missing, and a fixes tag.

ChenYu

Quoted 31 lines
---
 sound/soc/sunxi/sun4i-i2s.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)
sound/soc/sunxi/sun4i-i2s.c +5 -2
index adb988ae9ac5..93a484d7e228 100644
@@ -110,7 +110,7 @@
 
 #define SUN8I_I2S_TX_CHAN_MAP_REG      0x44
 #define SUN8I_I2S_TX_CHAN_SEL_REG      0x34
-#define SUN8I_I2S_TX_CHAN_OFFSET_MASK          GENMASK(13, 11)
+#define SUN8I_I2S_TX_CHAN_OFFSET_MASK          GENMASK(13, 12)
 #define SUN8I_I2S_TX_CHAN_OFFSET(offset)       (offset << 12)
 #define SUN8I_I2S_TX_CHAN_EN_MASK              GENMASK(11, 4)
 #define SUN8I_I2S_TX_CHAN_EN(num_chan)         (((1 << num_chan) - 1) << 4)

@@ -482,6 +482,10 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
                regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
                                   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
                                   SUN8I_I2S_TX_CHAN_OFFSET(offset));
+
+               regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
+                                  SUN8I_I2S_TX_CHAN_OFFSET_MASK,
+                                  SUN8I_I2S_TX_CHAN_OFFSET(offset));
        }
 
        regmap_field_write(i2s->field_fmt_mode, val);
--

2.20.1

maintainer
#14 lore raw

Re: [PATCH v3 4/9] ASoC: sun4i-i2s: Fix offset mask

Quoted 10 lines · Chen-Yu Tsai
Quoted 1 line · On Fri, Dec 21, 2018 at 11:21 PM <[email protected]> wrote:

Also add offset to RX channel select

Commit log seems a bit lacking. You could probably explain how you found this, either when comparing datasheet macros, or some actual error manifested and you "heard" it. You could also state what the fixed outcome should be.

Also please add a fixes tag.

Plus send this at the start of the series so it can be applied as a fix and sent to Linus during the development cycle.

author
#15 lore raw

PATCH v3 5/9 ASoC: sun4i-i2s: Correct divider calculations

In reply to [email protected]

From: Marcus Cooper <[email protected]>

The clock division circuitry is different on the H3 and later SoCs. The division of bclk is now based on pll2.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 82 +++++++++++++++++++++++++------------
 1 file changed, 56 insertions(+), 26 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +56 -26
index 93a484d7e228..b31f84787218 100644
@@ -129,10 +129,10 @@
  * @has_chsel_offset: SoC uses offset for selecting dai operational mode.
  * @reg_offset_txdata: offset of the tx fifo.
  * @sun4i_i2s_regmap: regmap config to use.
- * @mclk_offset: Value by which mclkdiv needs to be adjusted.
- * @bclk_offset: Value by which bclkdiv needs to be adjusted.
  * @fmt_offset: Value by which wss and sr needs to be adjusted.
  * @field_clkdiv_mclk_en: regmap field to enable mclk output.
+ * @field_clkdiv_mclk: regmap field for mclkdiv.
+ * @field_clkdiv_bclk: regmap field for bclkdiv.
  * @field_fmt_wss: regmap field to set word select size.
  * @field_fmt_sr: regmap field to set sample resolution.
  * @field_fmt_bclk: regmap field to set clk polarity.

@@ -153,8 +153,6 @@ struct sun4i_i2s_quirks {
 	bool				has_chsel_offset;
 	unsigned int			reg_offset_txdata;	/* TX FIFO */
 	const struct regmap_config	*sun4i_i2s_regmap;
-	unsigned int			mclk_offset;
-	unsigned int			bclk_offset;
 	unsigned int			fmt_offset;
 
 	/* Register fields for i2s */

@@ -210,7 +208,25 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_bclk_div[] = {
 	{ .div = 8, .val = 3 },
 	{ .div = 12, .val = 4 },
 	{ .div = 16, .val = 5 },
-	/* TODO - extend divide ratio supported by newer SoCs */
+};
+
+static const struct sun4i_i2s_clk_div sun8i_i2s_clk_div[] = {
+	{ .div = 0, .val = 0 },
+	{ .div = 1, .val = 1 },
+	{ .div = 2, .val = 2 },
+	{ .div = 4, .val = 3 },
+	{ .div = 6, .val = 4 },
+	{ .div = 8, .val = 5 },
+	{ .div = 12, .val = 6 },
+	{ .div = 16, .val = 7 },
+	{ .div = 24, .val = 8 },
+	{ .div = 32, .val = 9 },
+	{ .div = 48, .val = 10 },
+	{ .div = 64, .val = 11 },
+	{ .div = 96, .val = 12 },
+	{ .div = 128, .val = 13 },
+	{ .div = 176, .val = 14 },
+	{ .div = 192, .val = 15 },
 };
 
 static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] = {

@@ -222,21 +238,21 @@ static const struct sun4i_i2s_clk_div sun4i_i2s_mclk_div[] = {
 	{ .div = 12, .val = 5 },
 	{ .div = 16, .val = 6 },
 	{ .div = 24, .val = 7 },
-	/* TODO - extend divide ratio supported by newer SoCs */
 };
 
 static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i2s,
 				  unsigned int oversample_rate,
-				  unsigned int word_size)
+				  unsigned int word_size,
+				  const struct sun4i_i2s_clk_div *bdiv,
+				  unsigned int size)
 {
 	int div = oversample_rate / word_size / 2;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(sun4i_i2s_bclk_div); i++) {
-		const struct sun4i_i2s_clk_div *bdiv = &sun4i_i2s_bclk_div[i];
-
+	for (i = 0; i < size; i++) {
 		if (bdiv->div == div)
 			return bdiv->val;
+		bdiv++;
 	}
 
 	return -EINVAL;

@@ -245,16 +261,17 @@ static int sun4i_i2s_get_bclk_div(struct sun4i_i2s *i2s,
 static int sun4i_i2s_get_mclk_div(struct sun4i_i2s *i2s,
 				  unsigned int oversample_rate,
 				  unsigned int module_rate,
-				  unsigned int sampling_rate)
+				  unsigned int sampling_rate,
+				  const struct sun4i_i2s_clk_div *mdiv,
+				  unsigned int size)
 {
 	int div = module_rate / sampling_rate / oversample_rate;
 	int i;
 
-	for (i = 0; i < ARRAY_SIZE(sun4i_i2s_mclk_div); i++) {
-		const struct sun4i_i2s_clk_div *mdiv = &sun4i_i2s_mclk_div[i];
-
+	for (i = 0; i < size; i++) {
 		if (mdiv->div == div)
 			return mdiv->val;
+		mdiv++;
 	}
 
 	return -EINVAL;

@@ -319,24 +336,39 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
 		return -EINVAL;
 	}
 
-	bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
-					  word_size);
+	if (i2s->variant->has_fmt_set_lrck_period)
+		bclk_div = sun4i_i2s_get_bclk_div(i2s, clk_rate / rate,
+						  word_size,
+						  sun8i_i2s_clk_div,
+						  ARRAY_SIZE(sun8i_i2s_clk_div));
+	else
+		bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
+						  word_size,
+						  sun4i_i2s_bclk_div,
+						  ARRAY_SIZE(sun4i_i2s_bclk_div));
 	if (bclk_div < 0) {
-		dev_err(dai->dev, "Unsupported BCLK divider: %d\n", bclk_div);
+		dev_err(dai->dev, "Unsupported BCLK divider: %d\n",
+			bclk_div);
 		return -EINVAL;
 	}
 
-	mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
-					  clk_rate, rate);
+
+	if (i2s->variant->has_fmt_set_lrck_period)
+		mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+						  clk_rate, rate,
+						  sun8i_i2s_clk_div,
+						  ARRAY_SIZE(sun8i_i2s_clk_div));
+	else
+		mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+						  clk_rate, rate,
+						  sun4i_i2s_mclk_div,
+						  ARRAY_SIZE(sun4i_i2s_mclk_div));
 	if (mclk_div < 0) {
-		dev_err(dai->dev, "Unsupported MCLK divider: %d\n", mclk_div);
+		dev_err(dai->dev, "Unsupported MCLK divider: %d\n",
+			mclk_div);
 		return -EINVAL;
 	}
 
-	/* Adjust the clock division values if needed */
-	bclk_div += i2s->variant->bclk_offset;
-	mclk_div += i2s->variant->mclk_offset;
-
 	regmap_write(i2s->regmap, SUN4I_I2S_CLK_DIV_REG,
 		     SUN4I_I2S_CLK_DIV_BCLK(bclk_div) |
 		     SUN4I_I2S_CLK_DIV_MCLK(mclk_div));

@@ -955,8 +987,6 @@ static const struct sun4i_i2s_quirks sun8i_h3_i2s_quirks = {
 	.has_reset		= true,
 	.reg_offset_txdata	= SUN8I_I2S_FIFO_TX_REG,
 	.sun4i_i2s_regmap	= &sun8i_i2s_regmap_config,
-	.mclk_offset		= 1,
-	.bclk_offset		= 2,
 	.fmt_offset		= 3,
 	.has_fmt_set_lrck_period = true,
 	.has_chcfg		= true,
Signature · 1 line
2.20.1
author
#16 lore raw

PATCH v3 6/9 ASoC: sun4i-i2s: Add multi-lane functionality

In reply to [email protected]

From: Marcus Cooper <[email protected]>

The i2s block supports multi-lane i2s output however this functionality is only possible in earlier SoCs where the pins are exposed and for the i2s block used for HDMI audio on the later SoCs.

To enable this functionality, an optional property has been added to the bindings.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 37 ++++++++++++++++++++++++++++++++-----
 1 file changed, 32 insertions(+), 5 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +32 -5
index b31f84787218..e85789d84c0c 100644
@@ -27,7 +27,7 @@
 
 #define SUN4I_I2S_CTRL_REG		0x00
 #define SUN4I_I2S_CTRL_SDO_EN_MASK		GENMASK(11, 8)
-#define SUN4I_I2S_CTRL_SDO_EN(sdo)			BIT(8 + (sdo))
+#define SUN4I_I2S_CTRL_SDO_EN(lines)		(((1 << lines) - 1) << 8)
 #define SUN4I_I2S_CTRL_MODE_MASK		BIT(5)
 #define SUN4I_I2S_CTRL_MODE_SLAVE			(1 << 5)
 #define SUN4I_I2S_CTRL_MODE_MASTER			(0 << 5)

@@ -394,14 +394,23 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 	int sr, wss, channels;
 	u32 width;
+	int lines;
 
 	channels = params_channels(params);
-	if (channels != 2) {
+	if ((channels > dai->driver->playback.channels_max) ||
+		(channels < dai->driver->playback.channels_min)) {
 		dev_err(dai->dev, "Unsupported number of channels: %d\n",
 			channels);
 		return -EINVAL;
 	}
 
+	lines = (channels + 1) / 2;
+
+	/* Enable the required output lines */
+	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+			   SUN4I_I2S_CTRL_SDO_EN_MASK,
+			   SUN4I_I2S_CTRL_SDO_EN(lines));
+
 	if (i2s->variant->has_chcfg) {
 		regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
 				   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,

@@ -412,8 +421,19 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	}
 
 	/* Map the channels for playback and capture */
-	regmap_field_write(i2s->field_txchanmap, 0x76543210);
 	regmap_field_write(i2s->field_rxchanmap, 0x00003210);
+	regmap_field_write(i2s->field_txchanmap, 0x10);
+	if (i2s->variant->has_chsel_tx_chen) {
+		if (channels > 2)
+			regmap_write(i2s->regmap,
+				     SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
+		if (channels > 4)
+			regmap_write(i2s->regmap,
+				     SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
+		if (channels > 6)
+			regmap_write(i2s->regmap,
+				     SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
+	}
 
 	/* Configure the channels */
 	regmap_field_write(i2s->field_txchansel,

@@ -1094,9 +1114,10 @@ static int sun4i_i2s_init_regmap_fields(struct device *dev,
 static int sun4i_i2s_probe(struct platform_device *pdev)
 {
 	struct sun4i_i2s *i2s;
+	struct snd_soc_dai_driver *soc_dai;
 	struct resource *res;
 	void __iomem *regs;
-	int irq, ret;
+	int irq, ret, val;
 
 	i2s = devm_kzalloc(&pdev->dev, sizeof(*i2s), GFP_KERNEL);
 	if (!i2s)

@@ -1175,6 +1196,12 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 		soc_dai->capture.formats = SUN8I_FORMATS;
 	}
 
+	if (!of_property_read_u32(pdev->dev.of_node,
+				  "allwinner,playback-channels", &val)) {
+		if (val >= 2 && val <= 8)
+			soc_dai->playback.channels_max = val;
+	}
+
 	pm_runtime_enable(&pdev->dev);
 	if (!pm_runtime_enabled(&pdev->dev)) {
 		ret = sun4i_i2s_runtime_resume(&pdev->dev);

@@ -1184,7 +1211,7 @@ static int sun4i_i2s_probe(struct platform_device *pdev)
 
 	ret = devm_snd_soc_register_component(&pdev->dev,
 					      &sun4i_i2s_component,
-					      &sun4i_i2s_dai, 1);
+					      soc_dai, 1);
 	if (ret) {
 		dev_err(&pdev->dev, "Could not register DAI\n");
 		goto err_suspend;
Signature · 1 line
2.20.1
author
#17 lore raw

PATCH v3 7/9 ASoC: sun4i-i2s: Do not divide clocks when slave

In reply to [email protected]

From: Marcus Cooper <[email protected]>

There is no need to set the clock and calculate the division of the audio pll for the bclk and sync signals when they are not required.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 144 +++++++++++++++++++-----------------
 1 file changed, 77 insertions(+), 67 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +77 -67
index e85789d84c0c..8cec2f42c94e 100644
@@ -194,6 +194,8 @@ struct sun4i_i2s {
 	struct regmap_field	*field_rxchansel;
 
 	const struct sun4i_i2s_quirks	*variant;
+
+	bool bit_clk_master;
 };
 
 struct sun4i_i2s_clk_div {

@@ -298,82 +300,86 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
 	int bclk_div, mclk_div;
 	int ret;
 
-	switch (rate) {
-	case 176400:
-	case 88200:
-	case 44100:
-	case 22050:
-	case 11025:
-		clk_rate = 22579200;
-		break;
+	if (i2s->bit_clk_master) {
+		switch (rate) {
+		case 176400:
+		case 88200:
+		case 44100:
+		case 22050:
+		case 11025:
+			clk_rate = 22579200;
+			break;
 
-	case 192000:
-	case 128000:
-	case 96000:
-	case 64000:
-	case 48000:
-	case 32000:
-	case 24000:
-	case 16000:
-	case 12000:
-	case 8000:
-		clk_rate = 24576000;
-		break;
+		case 192000:
+		case 128000:
+		case 96000:
+		case 64000:
+		case 48000:
+		case 32000:
+		case 24000:
+		case 16000:
+		case 12000:
+		case 8000:
+			clk_rate = 24576000;
+			break;
 
-	default:
-		dev_err(dai->dev, "Unsupported sample rate: %u\n", rate);
-		return -EINVAL;
-	}
+		default:
+			dev_err(dai->dev, "Unsupported sample rate: %u\n", rate);
+			return -EINVAL;
+		}
 
-	ret = clk_set_rate(i2s->mod_clk, clk_rate);
-	if (ret)
-		return ret;
+		ret = clk_set_rate(i2s->mod_clk, clk_rate);
+		if (ret) {
+			dev_err(dai->dev, "Unable to set clock\n");
+			return ret;
+		}
 
-	oversample_rate = i2s->mclk_freq / rate;
-	if (!sun4i_i2s_oversample_is_valid(oversample_rate)) {
-		dev_err(dai->dev, "Unsupported oversample rate: %d\n",
-			oversample_rate);
-		return -EINVAL;
-	}
+		oversample_rate = i2s->mclk_freq / rate;
+		if (!sun4i_i2s_oversample_is_valid(oversample_rate)) {
+			dev_err(dai->dev, "Unsupported oversample rate: %d\n",
+				oversample_rate);
+			return -EINVAL;
+		}
 
-	if (i2s->variant->has_fmt_set_lrck_period)
-		bclk_div = sun4i_i2s_get_bclk_div(i2s, clk_rate / rate,
-						  word_size,
-						  sun8i_i2s_clk_div,
-						  ARRAY_SIZE(sun8i_i2s_clk_div));
-	else
-		bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
-						  word_size,
-						  sun4i_i2s_bclk_div,
-						  ARRAY_SIZE(sun4i_i2s_bclk_div));
-	if (bclk_div < 0) {
-		dev_err(dai->dev, "Unsupported BCLK divider: %d\n",
-			bclk_div);
-		return -EINVAL;
-	}
+		if (i2s->variant->has_fmt_set_lrck_period)
+			bclk_div = sun4i_i2s_get_bclk_div(i2s, clk_rate / rate,
+							  word_size,
+							  sun8i_i2s_clk_div,
+							  ARRAY_SIZE(sun8i_i2s_clk_div));
+		else
+			bclk_div = sun4i_i2s_get_bclk_div(i2s, oversample_rate,
+							  word_size,
+							  sun4i_i2s_bclk_div,
+							  ARRAY_SIZE(sun4i_i2s_bclk_div));
+		if (bclk_div < 0) {
+			dev_err(dai->dev, "Unsupported BCLK divider: %d\n",
+				bclk_div);
+			return -EINVAL;
+		}
 
 
-	if (i2s->variant->has_fmt_set_lrck_period)
-		mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
-						  clk_rate, rate,
-						  sun8i_i2s_clk_div,
-						  ARRAY_SIZE(sun8i_i2s_clk_div));
-	else
-		mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
-						  clk_rate, rate,
-						  sun4i_i2s_mclk_div,
-						  ARRAY_SIZE(sun4i_i2s_mclk_div));
-	if (mclk_div < 0) {
-		dev_err(dai->dev, "Unsupported MCLK divider: %d\n",
-			mclk_div);
-		return -EINVAL;
-	}
+		if (i2s->variant->has_fmt_set_lrck_period)
+			mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+							  clk_rate, rate,
+							  sun8i_i2s_clk_div,
+							  ARRAY_SIZE(sun8i_i2s_clk_div));
+		else
+			mclk_div = sun4i_i2s_get_mclk_div(i2s, oversample_rate,
+							  clk_rate, rate,
+							  sun4i_i2s_mclk_div,
+							  ARRAY_SIZE(sun4i_i2s_mclk_div));
+		if (mclk_div < 0) {
+			dev_err(dai->dev, "Unsupported MCLK divider: %d\n",
+				mclk_div);
+			return -EINVAL;
+		}
 
-	regmap_write(i2s->regmap, SUN4I_I2S_CLK_DIV_REG,
-		     SUN4I_I2S_CLK_DIV_BCLK(bclk_div) |
-		     SUN4I_I2S_CLK_DIV_MCLK(mclk_div));
+		regmap_write(i2s->regmap, SUN4I_I2S_CLK_DIV_REG,
+			     SUN4I_I2S_CLK_DIV_BCLK(bclk_div) |
+			     SUN4I_I2S_CLK_DIV_MCLK(mclk_div));
 
-	regmap_field_write(i2s->field_clkdiv_mclk_en, 1);
+		regmap_field_write(i2s->field_clkdiv_mclk_en, 1);
+	}
 
 	/* Set sync period */
 	if (i2s->variant->has_fmt_set_lrck_period)

@@ -574,10 +580,12 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		case SND_SOC_DAIFMT_CBS_CFS:
 			/* BCLK and LRCLK master */
 			val = SUN4I_I2S_CTRL_MODE_MASTER;
+			i2s->bit_clk_master = true;
 			break;
 		case SND_SOC_DAIFMT_CBM_CFM:
 			/* BCLK and LRCLK slave */
 			val = SUN4I_I2S_CTRL_MODE_SLAVE;
+			i2s->bit_clk_master = false;
 			break;
 		default:
 			dev_err(dai->dev, "Unsupported slave setting: %d\n",

@@ -598,10 +606,12 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 			/* BCLK and LRCLK master */
 			val = SUN8I_I2S_CTRL_BCLK_OUT |
 				SUN8I_I2S_CTRL_LRCK_OUT;
+			i2s->bit_clk_master = true;
 			break;
 		case SND_SOC_DAIFMT_CBM_CFM:
 			/* BCLK and LRCLK slave */
 			val = 0;
+			i2s->bit_clk_master = false;
 			break;
 		default:
 			dev_err(dai->dev, "Unsupported slave setting: %d\n",
Signature · 1 line
2.20.1
author
#18 lore raw

PATCH v3 8/9 ASoC: sun4i-i2s: Add set_tdm_slot functionality

In reply to [email protected]

From: Marcus Cooper <[email protected]>

Some codecs require a different amount of a bit clocks per frame than what is calculated by the sample width. Use the tdm slot bindings to provide this mechanism.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 22 ++++++++++++++++++++--
 1 file changed, 20 insertions(+), 2 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +20 -2
index 8cec2f42c94e..cfcf427270fd 100644
@@ -196,6 +196,9 @@ struct sun4i_i2s {
 	const struct sun4i_i2s_quirks	*variant;
 
 	bool bit_clk_master;
+
+	unsigned int	tdm_slots;
+	unsigned int	slot_width;
 };
 
 struct sun4i_i2s_clk_div {

@@ -385,7 +388,7 @@ static int sun4i_i2s_set_clk_rate(struct snd_soc_dai *dai,
 	if (i2s->variant->has_fmt_set_lrck_period)
 		regmap_update_bits(i2s->regmap, SUN4I_I2S_FMT0_REG,
 				   SUN8I_I2S_FMT0_LRCK_PERIOD_MASK,
-				   SUN8I_I2S_FMT0_LRCK_PERIOD(32));
+				   SUN8I_I2S_FMT0_LRCK_PERIOD(word_size));
 
 	/* Set sign extension to pad out LSB with 0 */
 	regmap_field_write(i2s->field_fmt_sext, 0);

@@ -498,7 +501,8 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 			   sr + i2s->variant->fmt_offset);
 
 	return sun4i_i2s_set_clk_rate(dai, params_rate(params),
-				      params_width(params));
+				      i2s->tdm_slots ?
+				      i2s->slot_width : params_width(params));
 }
 
 static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)

@@ -749,11 +753,25 @@ static int sun4i_i2s_set_sysclk(struct snd_soc_dai *dai, int clk_id,
 	return 0;
 }
 
+static int sun4i_i2s_set_dai_tdm_slot(struct snd_soc_dai *dai,
+	unsigned int tx_mask, unsigned int rx_mask,
+	int slots, int width)
+{
+	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
+
+	i2s->tdm_slots = slots;
+
+	i2s->slot_width = width;
+
+	return 0;
+}
+
 static const struct snd_soc_dai_ops sun4i_i2s_dai_ops = {
 	.hw_params	= sun4i_i2s_hw_params,
 	.set_fmt	= sun4i_i2s_set_fmt,
 	.set_sysclk	= sun4i_i2s_set_sysclk,
 	.trigger	= sun4i_i2s_trigger,
+	.set_tdm_slot	= sun4i_i2s_set_dai_tdm_slot,
 };
 
 static int sun4i_i2s_dai_probe(struct snd_soc_dai *dai)
Signature · 1 line
2.20.1
author
#19 lore raw

PATCH v3 9/9 ASoC: sun4i-i2s: Add multichannel functionality

In reply to [email protected]

From: Marcus Cooper <[email protected]>

The i2s block can be used to pass PCM data over multiple channels and is sometimes used for the audio side of an HDMI connection.

Signed-off-by Marcus Cooper <[email protected]>
---
 sound/soc/sunxi/sun4i-i2s.c | 121 +++++++++++++++++++-----------------
 1 file changed, 64 insertions(+), 57 deletions(-)
sound/soc/sunxi/sun4i-i2s.c +64 -57
index cfcf427270fd..2be3a3e7a3c0 100644
@@ -199,6 +199,7 @@ struct sun4i_i2s {
 
 	unsigned int	tdm_slots;
 	unsigned int	slot_width;
+	unsigned int	offset;
 };
 
 struct sun4i_i2s_clk_div {

@@ -406,56 +407,71 @@ static int sun4i_i2s_hw_params(struct snd_pcm_substream *substream,
 	int lines;
 
 	channels = params_channels(params);
-	if ((channels > dai->driver->playback.channels_max) ||
-		(channels < dai->driver->playback.channels_min)) {
-		dev_err(dai->dev, "Unsupported number of channels: %d\n",
-			channels);
-		return -EINVAL;
-	}
-
-	lines = (channels + 1) / 2;
+	if (substream->stream == SNDRV_PCM_STREAM_PLAYBACK) {
+		if ((channels > dai->driver->playback.channels_max) ||
+			(channels < dai->driver->playback.channels_min)) {
+			dev_err(dai->dev, "Unsupported number of channels: %d\n",
+				channels);
+			return -EINVAL;
+		}
 
-	/* Enable the required output lines */
-	regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
-			   SUN4I_I2S_CTRL_SDO_EN_MASK,
-			   SUN4I_I2S_CTRL_SDO_EN(lines));
-
-	if (i2s->variant->has_chcfg) {
-		regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
-				   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
-				   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels));
-		regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
-				   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK,
-				   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels));
-	}
+		lines = (channels + 1) / 2;
 
-	/* Map the channels for playback and capture */
-	regmap_field_write(i2s->field_rxchanmap, 0x00003210);
-	regmap_field_write(i2s->field_txchanmap, 0x10);
-	if (i2s->variant->has_chsel_tx_chen) {
-		if (channels > 2)
-			regmap_write(i2s->regmap,
-				     SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
-		if (channels > 4)
-			regmap_write(i2s->regmap,
-				     SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
-		if (channels > 6)
-			regmap_write(i2s->regmap,
-				     SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
+		/* Enable the required output lines */
+		regmap_update_bits(i2s->regmap, SUN4I_I2S_CTRL_REG,
+				   SUN4I_I2S_CTRL_SDO_EN_MASK,
+				   SUN4I_I2S_CTRL_SDO_EN(lines));
+
+		if (i2s->variant->has_chcfg)
+			regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
+					   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM_MASK,
+					   SUN8I_I2S_CHAN_CFG_TX_SLOT_NUM(channels));
+
+		regmap_field_write(i2s->field_txchanmap, 0x10);
+		/* Configure the channels */
+		regmap_field_write(i2s->field_txchansel, SUN4I_I2S_CHAN_SEL(2));
+
+		if (i2s->variant->has_chsel_tx_chen) {
+			u32 chan_sel = SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset) | 0x1;
+			regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
+				     chan_sel | 0x30);
+
+			if (channels > 2) {
+				regmap_write(i2s->regmap,
+					     SUN8I_I2S_TX_CHAN_MAP_REG+4, 0x32);
+				regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG+4,
+					     chan_sel | 0x30);
+			}
+			if (channels > 4) {
+				regmap_write(i2s->regmap,
+					     SUN8I_I2S_TX_CHAN_MAP_REG+8, 0x54);
+				regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG+8,
+					     chan_sel | 0x30);
+			}
+			if (channels > 6) {
+				regmap_write(i2s->regmap,
+					     SUN8I_I2S_TX_CHAN_MAP_REG+12, 0x76);
+				regmap_write(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG+12,
+					     chan_sel | 0x30);
+			}
+		}
+	} else {
+		/* Map the channels for capture */
+		regmap_field_write(i2s->field_rxchanmap, 0x00003210);
+		regmap_field_write(i2s->field_rxchansel,
+				   SUN4I_I2S_CHAN_SEL(params_channels(params)));
+
+		if (i2s->variant->has_chcfg)
+			regmap_update_bits(i2s->regmap, SUN8I_I2S_CHAN_CFG_REG,
+					   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM_MASK,
+					   SUN8I_I2S_CHAN_CFG_RX_SLOT_NUM(channels));
+
+		if (i2s->variant->has_chsel_tx_chen)
+			regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
+					   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
+					   SUN8I_I2S_TX_CHAN_OFFSET(i2s->offset));
 	}
 
-	/* Configure the channels */
-	regmap_field_write(i2s->field_txchansel,
-			   SUN4I_I2S_CHAN_SEL(params_channels(params)));
-
-	regmap_field_write(i2s->field_rxchansel,
-			   SUN4I_I2S_CHAN_SEL(params_channels(params)));
-
-	if (i2s->variant->has_chsel_tx_chen)
-		regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
-				   SUN8I_I2S_TX_CHAN_EN_MASK,
-				   SUN8I_I2S_TX_CHAN_EN(channels));
-
 	switch (params_physical_width(params)) {
 	case 16:
 		width = DMA_SLAVE_BUSWIDTH_2_BYTES;

@@ -509,7 +525,6 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 {
 	struct sun4i_i2s *i2s = snd_soc_dai_get_drvdata(dai);
 	u32 val;
-	u32 offset = 0;
 	u32 bclk_polarity = SUN4I_I2S_FMT0_POLARITY_NORMAL;
 	u32 lrclk_polarity = SUN4I_I2S_FMT0_POLARITY_NORMAL;
 

@@ -517,7 +532,7 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 	switch (fmt & SND_SOC_DAIFMT_FORMAT_MASK) {
 	case SND_SOC_DAIFMT_I2S:
 		val = SUN4I_I2S_FMT0_FMT_I2S;
-		offset = 1;
+		i2s->offset = 1;
 		break;
 	case SND_SOC_DAIFMT_LEFT_J:
 		val = SUN4I_I2S_FMT0_FMT_LEFT_J;

@@ -538,16 +553,8 @@ static int sun4i_i2s_set_fmt(struct snd_soc_dai *dai, unsigned int fmt)
 		 * i2s shares the same setting with the LJ format. Increment
 		 * val so that the bit to value to write is correct.
 		 */
-		if (offset > 0)
+		if (i2s->offset > 0)
 			val++;
-		/* blck offset determines whether i2s or LJ */
-		regmap_update_bits(i2s->regmap, SUN8I_I2S_TX_CHAN_SEL_REG,
-				   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
-				   SUN8I_I2S_TX_CHAN_OFFSET(offset));
-
-		regmap_update_bits(i2s->regmap, SUN8I_I2S_RX_CHAN_SEL_REG,
-				   SUN8I_I2S_TX_CHAN_OFFSET_MASK,
-				   SUN8I_I2S_TX_CHAN_OFFSET(offset));
 	}
 
 	regmap_field_write(i2s->field_fmt_mode, val);
Signature · 1 line
2.20.1