Re: [PATCH v2 6/7] ASoC: sun4i-i2s: Adjust regmap settings

From: Maxime Ripard <maxime@cerno.tech>
To: "Clément Péron" <peron.clem@gmail.com>
Cc: Liam Girdwood <lgirdwood@gmail.com>,
	Mark Brown <broonie@kernel.org>, Rob Herring <robh+dt@kernel.org>,
	Chen-Yu Tsai <wens@csie.org>, Jaroslav Kysela <perex@perex.cz>,
	Takashi Iwai <tiwai@suse.com>,
	alsa-devel@alsa-project.org, devicetree@vger.kernel.org,
	linux-arm-kernel@lists.infradead.org,
	linux-kernel@vger.kernel.org,
	Marcus Cooper <codekipper@gmail.com>
Subject: Re: [PATCH v2 6/7] ASoC: sun4i-i2s: Adjust regmap settings
Date: Mon, 20 Apr 2020 14:43:10 +0200	[thread overview]
Message-ID: <20200420124310.u5475fgdkmidkvy5@gilmour.lan> (raw)
In-Reply-To: <20200418224435.23672-7-peron.clem@gmail.com>

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

Hi,

On Sun, Apr 19, 2020 at 12:44:34AM +0200, Clément Péron wrote:
> From: Marcus Cooper <codekipper@gmail.com>
> 
> Bypass the regmap cache when flushing the i2s FIFOs and modify the tables
> to reflect this.

I think that commit log requires a bit more work.

As far as I can see, you're doing several things here:

> Signed-off-by: Marcus Cooper <codekipper@gmail.com>
> Signed-off-by: Clément Péron <peron.clem@gmail.com>
> ---
>  sound/soc/sunxi/sun4i-i2s.c | 31 ++++++++-----------------------
>  1 file changed, 8 insertions(+), 23 deletions(-)
> 
> diff --git a/sound/soc/sunxi/sun4i-i2s.c b/sound/soc/sunxi/sun4i-i2s.c
> index 9778af37fbca..9053d290dd87 100644
> --- a/sound/soc/sunxi/sun4i-i2s.c
> +++ b/sound/soc/sunxi/sun4i-i2s.c
> @@ -921,7 +921,7 @@ 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 */
> -	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> +	regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_RX);

You change regmap_update_bits to regmap_write_bits, I assume this is what the
commit log means by "bypassing the cache", so that every write actually gets
done even if the bit is already set.

I'm not quite sure why it's needed though, since that bit is self-clearing.

> @@ -942,7 +942,7 @@ static void sun4i_i2s_start_capture(struct sun4i_i2s *i2s)
>  static void sun4i_i2s_start_playback(struct sun4i_i2s *i2s)
>  {
>  	/* Flush TX FIFO */
> -	regmap_update_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
> +	regmap_write_bits(i2s->regmap, SUN4I_I2S_FIFO_CTRL_REG,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX,
>  			   SUN4I_I2S_FIFO_CTRL_FLUSH_TX);

Ditto.

>  
> @@ -1096,13 +1096,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;
>  }

You seem to be allowing reads to FIFO_TX_REG now for some reason?

>  static bool sun4i_i2s_wr_reg(struct device *dev, unsigned int reg)
> @@ -1121,6 +1115,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:

I assume that your issue was that the flushed bit got cached since the register
wasn't volatile, and therefore each time we were supposed to flush, we actually
ended up doing nothing. Marking it as volatile would prevent the cache to catch
that write and make regmap_update_bits work, actually fixing what you mention in
the commit log.

Either way, it should be in the log itself, and it doesn't really explain the
rest of the patch either.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]