ASoC: PATCH: wm8731 - reinitialize regmap cache after hardware reset

ASoC: PATCH: wm8731 - reinitialize regmap cache after hardware reset

[Sergey Kiselev]

Hi,

I am working on a machine driver that uses wm8731 codec and I've
noticed that it will not work after rmmod/modprobe of that machine
driver. The issue is that wm8731_probe() resets the hardware, but it
doesn't reinitialize the regmap cache to the default values.

The patch below fixes this issue.

I also skimmed through other codecs, and some of them might have
similar issue (wm8510.c, wm8711.c, wm8750.c, wm8753.c, wm8940.c,
wm8960.c, wm8971.c, wm8974.c, wm8988.c, wm8990.c)

Another way to avoid this issue might be resetting the codec in the
*_spi_probe()/*_i2c_probe(), so that it only gets reset once when
codec is loading.

Thanks in advance for any comments/suggestions on the patch.

Thanks,
Sergey

Signed-off-by: Sergey Kiselev <skiselev@gmail.com>

diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 2245b6a..76af75b 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -84,7 +84,7 @@ static bool wm8731_writeable(struct device *dev,
unsigned int reg)
  return reg <= WM8731_RESET;
 }

-#define wm8731_reset(c) snd_soc_write(c, WM8731_RESET, 0)
+static int wm8731_reset(struct snd_soc_codec *codec);

 static const char *wm8731_input_select[] = {"Line In", "Mic"};

@@ -665,6 +665,21 @@ static const struct regmap_config wm8731_regmap = {
  .num_reg_defaults = ARRAY_SIZE(wm8731_reg_defaults),
 };

+static int wm8731_reset(struct snd_soc_codec *codec)
+{
+ int ret;
+ struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
+
+ ret = snd_soc_write(codec, WM8731_RESET, 0);
+
+ if (ret != 0)
+ return ret;
+
+ regmap_reinit_cache(wm8731->regmap, &wm8731_regmap);
+
+ return 0;
+}
+
 #if defined(CONFIG_SPI_MASTER)
 static int wm8731_spi_probe(struct spi_device *spi)
 {

Re: ASoC: PATCH: wm8731 - reinitialize regmap cache after hardware reset

[Liam Girdwood]

Hi Sergey,

Best to always CC Mark on any ASoC patches, I've also added Richard from
Wolfson. Best to read submitting patches from the Documentation
directory too :) 

On Thu, 2015-05-28 at 09:34 -0700, Sergey Kiselev wrote:
> Hi,
> 
> I am working on a machine driver that uses wm8731 codec and I've
> noticed that it will not work after rmmod/modprobe of that machine
> driver. The issue is that wm8731_probe() resets the hardware, but it
> doesn't reinitialize the regmap cache to the default values.
> 
> The patch below fixes this issue.
> 
> I also skimmed through other codecs, and some of them might have
> similar issue (wm8510.c, wm8711.c, wm8750.c, wm8753.c, wm8940.c,
> wm8960.c, wm8971.c, wm8974.c, wm8988.c, wm8990.c)
> 

Richard, can Wolfson take care of these ones. I guess some of the older
codecs have no way to reset the registers ?

> Another way to avoid this issue might be resetting the codec in the
> *_spi_probe()/*_i2c_probe(), so that it only gets reset once when
> codec is loading.
> 
> Thanks in advance for any comments/suggestions on the patch.
> 
> Thanks,
> Sergey
> 
> Signed-off-by: Sergey Kiselev <skiselev@gmail.com>
> 
> diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
> index 2245b6a..76af75b 100644
> --- a/sound/soc/codecs/wm8731.c
> +++ b/sound/soc/codecs/wm8731.c
> @@ -84,7 +84,7 @@ static bool wm8731_writeable(struct device *dev,
> unsigned int reg)
>   return reg <= WM8731_RESET;
>  }
> 
> -#define wm8731_reset(c) snd_soc_write(c, WM8731_RESET, 0)

I would have expected this to reset the register values so I'm not sure
why it's failing in your case ?

> +static int wm8731_reset(struct snd_soc_codec *codec);
> 
>  static const char *wm8731_input_select[] = {"Line In", "Mic"};
> 
> @@ -665,6 +665,21 @@ static const struct regmap_config wm8731_regmap = {
>   .num_reg_defaults = ARRAY_SIZE(wm8731_reg_defaults),
>  };
> 
> +static int wm8731_reset(struct snd_soc_codec *codec)
> +{
> + int ret;
> + struct wm8731_priv *wm8731 = snd_soc_codec_get_drvdata(codec);
> +
> + ret = snd_soc_write(codec, WM8731_RESET, 0);
> +
> + if (ret != 0)
> + return ret;
> +
> + regmap_reinit_cache(wm8731->regmap, &wm8731_regmap);
> +
> + return 0;
> +}
> +

Your mailer has screwed up the formatting here :(

>  #if defined(CONFIG_SPI_MASTER)
>  static int wm8731_spi_probe(struct spi_device *spi)
>  {

Liam

Re: ASoC: PATCH: wm8731 - reinitialize regmap cache after hardware reset

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 478 bytes --]

On Thu, May 28, 2015 at 05:52:46PM +0100, Liam Girdwood wrote:
> On Thu, 2015-05-28 at 09:34 -0700, Sergey Kiselev wrote:

> > Another way to avoid this issue might be resetting the codec in the
> > *_spi_probe()/*_i2c_probe(), so that it only gets reset once when
> > codec is loading.

This is the correct way to fix the issue, this is a result of a partial
conversion to the device model.  There is no point in reseting the
device if we also rewrite all the register values.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: ASoC: PATCH: wm8731 - reinitialize regmap cache after hardware reset

[Mark Brown]

[-- Attachment #1.1: Type: text/plain, Size: 670 bytes --]

On Thu, May 28, 2015 at 02:55:48PM -0700, Sergey Kiselev wrote:

Please fix your mail client to word wrap within paragraphs, it makes
your mail more readable.

> On Thu, May 28, 2015 at 12:26 PM, Mark Brown wrote:

> OK, I was trying to move the request supplies and reset code to
> wm8731_{i2c|spi}_probe(). And I noticed the following weirdness with
> the supplies: The current code in wm8731_probe() requests and enables
> regulators, resets the codec hardware, tweaks some registers, and then
> disables all the regulators:

That's totally normal, the new register values will be rewritten as part
of resyncing the register cache when the device is powered back up.

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: ASoC: PATCH: wm8731 - reinitialize regmap cache after hardware reset

[Charles Keepax]

On Thu, May 28, 2015 at 09:34:09AM -0700, Sergey Kiselev wrote:
> Hi,
> 
> I am working on a machine driver that uses wm8731 codec and I've
> noticed that it will not work after rmmod/modprobe of that machine
> driver. The issue is that wm8731_probe() resets the hardware, but it
> doesn't reinitialize the regmap cache to the default values.
> 
> The patch below fixes this issue.
> 
> I also skimmed through other codecs, and some of them might have
> similar issue (wm8510.c, wm8711.c, wm8750.c, wm8753.c, wm8940.c,
> wm8960.c, wm8971.c, wm8974.c, wm8988.c, wm8990.c)
> 
> Another way to avoid this issue might be resetting the codec in the
> *_spi_probe()/*_i2c_probe(), so that it only gets reset once when
> codec is loading.
> 
> Thanks in advance for any comments/suggestions on the patch.
> 
> Thanks,
> Sergey
> 
> Signed-off-by: Sergey Kiselev <skiselev@gmail.com>
> 

In addition to the comments from Mark and Liam, if you could CC
patches@opensource.wolfsonmicro.com when submitting patches to
the Wolfson stuff, ensures we will see them going through, it is
easy to miss stuff on the generic lists.

Thanks,
Charles