| Message ID | 20190814060854.26345-16-codekipper@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | ASoC: sun4i-i2s: Updates to the driver | expand |
On Wed, Aug 14, 2019 at 08:08:54AM +0200, 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> This patch looks like it's fixing something while the commit log doesn't mention what is being fixed. Having some context here would be great. Maxime -- Maxime Ripard, Bootlin Embedded Linux and Kernel engineering https://bootlin.com
Hi! Dne sreda, 14. avgust 2019 ob 08:08:54 CEST je codekipper@gmail.com napisal(a): > 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 | 31 ++++++++++--------------------- > 1 file changed, 10 insertions(+), 21 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index d3c8789f70bb..ecfc1ed79379 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -876,9 +876,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); Did you try with regmap_write_bits() instead? This function will unconditionally write bits so it's nicer solution, because you don't have to use regcache_cache_bypass(). > > /* Clear RX counter */ > regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0); > @@ -897,9 +899,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); Ditto. > > /* Clear TX counter */ > regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0); > @@ -1053,13 +1057,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; Why did you change this? Manual mentions that SUN4I_I2S_FIFO_TX_REG is write- only register. Even if it can be read, then it's better to remove whole function, which will automatically mean that all registers can be read. > } > > static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg) > @@ -1078,6 +1076,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: SUN4I_I2S_FIFO_CTRL_REG should be put here, because it has two bits which returns to 0 immediately after they are set to 1. Best regards, Jernej > @@ -1088,23 +1088,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); > } > @@ -1175,7 +1164,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, > }; > > @@ -1188,7 +1177,7 @@ static const struct regmap_config > sun50i_i2s_regmap_config = { .reg_defaults = sun50i_i2s_reg_defaults, > .num_reg_defaults = ARRAY_SIZE(sun50i_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, > };
On Wed, 14 Aug 2019 at 10:38, Jernej Škrabec <jernej.skrabec@gmail.com> wrote: > > Hi! > > Dne sreda, 14. avgust 2019 ob 08:08:54 CEST je codekipper@gmail.com > napisal(a): > > 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 | 31 ++++++++++--------------------- > > 1 file changed, 10 insertions(+), 21 deletions(-) > > > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > > index d3c8789f70bb..ecfc1ed79379 100644 > > --- a/sound/soc/sunxi/sun4i-i2s.c > > +++ b/sound/soc/sunxi/sun4i-i2s.c > > @@ -876,9 +876,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); > > Did you try with regmap_write_bits() instead? This function will > unconditionally write bits so it's nicer solution, because you don't have to > use regcache_cache_bypass(). I didn't....with all the rework I've avoided messing with this change. Now that the dust has settled, I can go back to look at this. Thanks, CK > > > > > /* Clear RX counter */ > > regmap_write(i2s->regmap, SUN4I_I2S_RX_CNT_REG, 0); > > @@ -897,9 +899,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); > > Ditto. > > > > > /* Clear TX counter */ > > regmap_write(i2s->regmap, SUN4I_I2S_TX_CNT_REG, 0); > > @@ -1053,13 +1057,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; > > Why did you change this? Manual mentions that SUN4I_I2S_FIFO_TX_REG is write- > only register. Even if it can be read, then it's better to remove whole > function, which will automatically mean that all registers can be read. > > > > } > > > > static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg) > > @@ -1078,6 +1076,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: > > SUN4I_I2S_FIFO_CTRL_REG should be put here, because it has two bits which > returns to 0 immediately after they are set to 1. > > Best regards, > Jernej > > > @@ -1088,23 +1088,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); > > } > > @@ -1175,7 +1164,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, > > }; > > > > @@ -1188,7 +1177,7 @@ static const struct regmap_config > > sun50i_i2s_regmap_config = { .reg_defaults = sun50i_i2s_reg_defaults, > > .num_reg_defaults = ARRAY_SIZE(sun50i_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, > > }; > > > >
Dne sreda, 14. avgust 2019 ob 09:20:07 CEST je Maxime Ripard napisal(a): > On Wed, Aug 14, 2019 at 08:08:54AM +0200, 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> > > This patch looks like it's fixing something while the commit log > doesn't mention what is being fixed. Main issue addressed here is that SUN4I_I2S_FIFO_CTRL_REG has two self-clear registers (SUN4I_I2S_FIFO_CTRL_FLUSH_RX and SUN4I_I2S_FIFO_CTRL_FLUSH_TX) and thus it should be marked as volatile. Best regards, Jernej > > Having some context here would be great. > > Maxime > > -- > Maxime Ripard, Bootlin > Embedded Linux and Kernel engineering > https://bootlin.com
On Wed, Aug 14, 2019 at 2:09 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 | 31 ++++++++++--------------------- > 1 file changed, 10 insertions(+), 21 deletions(-) > > diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c > index d3c8789f70bb..ecfc1ed79379 100644 > --- a/sound/soc/sunxi/sun4i-i2s.c > +++ b/sound/soc/sunxi/sun4i-i2s.c > @@ -876,9 +876,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); > @@ -897,9 +899,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); > @@ -1053,13 +1057,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; The commit log needs to explain why this is relevant. And I'm not sure why one would read back the TX FIFO. Also, if it's always true, just drop the callback. ChenYu > } > > static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg) > @@ -1078,6 +1076,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: > @@ -1088,23 +1088,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); > } > @@ -1175,7 +1164,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, > }; > > @@ -1188,7 +1177,7 @@ static const struct regmap_config sun50i_i2s_regmap_config = { > .reg_defaults = sun50i_i2s_reg_defaults, > .num_reg_defaults = ARRAY_SIZE(sun50i_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.22.0 >
diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c index d3c8789f70bb..ecfc1ed79379 100644 --- a/sound/soc/sunxi/sun4i-i2s.c +++ b/sound/soc/sunxi/sun4i-i2s.c @@ -876,9 +876,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); @@ -897,9 +899,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); @@ -1053,13 +1057,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) @@ -1078,6 +1076,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: @@ -1088,23 +1088,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); } @@ -1175,7 +1164,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, }; @@ -1188,7 +1177,7 @@ static const struct regmap_config sun50i_i2s_regmap_config = { .reg_defaults = sun50i_i2s_reg_defaults, .num_reg_defaults = ARRAY_SIZE(sun50i_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, };