| Message ID | 20181221152110.17982-2-codekipper@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sun4i-i2s: Updates to the driver | expand |
On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote: > > From: Marcus Cooper <codekipper@gmail.com> > > Bypass the regmap cache when flushing the i2s FIFOs and modify the tables > to reflect this. > > Signed-off-by: Marcus Cooper <codekipper@gmail.com> > --- > sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++-------------------- > 1 file changed, 9 insertions(+), 20 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index d5ec1a20499d..64d073cb2aee 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -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. > > /* 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; 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 > } > > 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 >
On 2018-12-21 17:44, Chen-Yu Tsai wrote: > On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote: >> From: Marcus Cooper <codekipper@gmail.com> >> >> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables >> to reflect this. >> >> Signed-off-by: Marcus Cooper <codekipper@gmail.com> >> --- >> sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++-------------------- >> 1 file changed, 9 insertions(+), 20 deletions(-) >> >> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c >> index d5ec1a20499d..64d073cb2aee 100644 >> --- a/sound/soc/sunxi/sun4i-i2s.c >> +++ b/sound/soc/sunxi/sun4i-i2s.c >> @@ -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. 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. > > 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. > >> /* 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; > 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 > > 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 > >> } >> >> 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 >> > _______________________________________________ > Alsa-devel mailing list > Alsa-devel@alsa-project.org >
On Sun, Dec 23, 2018 at 6:12 AM Jonas Karlman <jonas@kwiboo.se> wrote: > > On 2018-12-21 17:44, Chen-Yu Tsai wrote: > > On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote: > >> From: Marcus Cooper <codekipper@gmail.com> > >> > >> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables > >> to reflect this. > >> > >> Signed-off-by: Marcus Cooper <codekipper@gmail.com> > >> --- > >> sound/soc/sunxi/sun4i-i2s.c | 29 +++++++++-------------------- > >> 1 file changed, 9 insertions(+), 20 deletions(-) > >> > >> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > >> index d5ec1a20499d..64d073cb2aee 100644 > >> --- a/sound/soc/sunxi/sun4i-i2s.c > >> +++ b/sound/soc/sunxi/sun4i-i2s.c > >> @@ -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. > > 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. > 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. > 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. > > > > 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. > > > >> /* 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; > > 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 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 > > > > 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 > > > >> } > >> > >> 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 > >> > > _______________________________________________ > > Alsa-devel mailing list > > Alsa-devel@alsa-project.org > >
On Sat, Dec 22, 2018 at 12:44:07AM +0800, Chen-Yu Tsai wrote: > On Fri, Dec 21, 2018 at 11:21 PM <codekipper@gmail.com> wrote: > > + 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. Yes, that should be the case. > 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).
On Sun, Dec 23, 2018 at 11:16:31AM +0800, Chen-Yu Tsai wrote: > 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. > 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.
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index d5ec1a20499d..64d073cb2aee 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -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, };