[PATCH] ASoC: WM8985: Register notifier for the regulator being disabled

[PATCH] ASoC: WM8985: Register notifier for the regulator being disabled

[Dimitris Papastamos]

Make sure we get an event when the regulator is disabled so we know
we have to sync the cache afterwards.

Minor stylistic changes.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/codecs/wm8985.c |   51 +++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 47 insertions(+), 4 deletions(-)

diff --git a/sound/soc/codecs/wm8985.c b/sound/soc/codecs/wm8985.c
index 6116b42..bd109c4 100644
--- a/sound/soc/codecs/wm8985.c
+++ b/sound/soc/codecs/wm8985.c
@@ -126,11 +126,35 @@ static const int volume_update_regs[] = {
 
 struct wm8985_priv {
 	enum snd_soc_control_type control_type;
+	struct snd_soc_codec *codec;
 	struct regulator_bulk_data supplies[WM8985_NUM_SUPPLIES];
+	struct notifier_block disable_nb[WM8985_NUM_SUPPLIES];
 	unsigned int sysclk;
 	unsigned int bclk;
 };
 
+/*
+ * We can't use the same notifier block for more than one supply and
+ * there's no way I can see to get from a callback to the caller
+ * except container_of().
+ */
+#define WM8985_REGULATOR_EVENT(n) \
+static int wm8985_regulator_event_##n(struct notifier_block *nb, \
+				      unsigned long event, void *data)    \
+{ \
+	struct wm8985_priv *wm8985 = container_of(nb, struct wm8985_priv, \
+							  disable_nb[n]); \
+	if (event & REGULATOR_EVENT_DISABLE) { \
+		wm8985->codec->cache_sync = 1; \
+	} \
+	return 0; \
+}
+
+WM8985_REGULATOR_EVENT(0)
+WM8985_REGULATOR_EVENT(1)
+WM8985_REGULATOR_EVENT(2)
+WM8985_REGULATOR_EVENT(3)
+
 static const struct {
 	int div;
 	int ratio;
@@ -782,7 +806,7 @@ static int wm8985_set_pll(struct snd_soc_dai *dai, int pll_id,
 {
 	int ret;
 	struct snd_soc_codec *codec;
-	struct pll_div pll_div;
+	struct pll_div pll_div = { 0 };
 
 	codec = dai->codec;
 	if (freq_in && freq_out) {
@@ -794,7 +818,7 @@ static int wm8985_set_pll(struct snd_soc_dai *dai, int pll_id,
 	/* disable the PLL before reprogramming it */
 	snd_soc_update_bits(codec, WM8985_POWER_MANAGEMENT_1,
 			    WM8985_PLLEN_MASK, 0);
-	
+
 	if (!freq_in || !freq_out)
 		return 0;
 
@@ -932,8 +956,6 @@ static int wm8985_set_bias_level(struct snd_soc_codec *codec,
 		snd_soc_write(codec, WM8985_POWER_MANAGEMENT_2, 0);
 		snd_soc_write(codec, WM8985_POWER_MANAGEMENT_3, 0);
 
-		codec->cache_sync = 1;
-
 		regulator_bulk_disable(ARRAY_SIZE(wm8985->supplies),
 				       wm8985->supplies);
 		break;
@@ -963,9 +985,13 @@ static int wm8985_resume(struct snd_soc_codec *codec)
 static int wm8985_remove(struct snd_soc_codec *codec)
 {
 	struct wm8985_priv *wm8985;
+	int i;
 
 	wm8985 = snd_soc_codec_get_drvdata(codec);
 	wm8985_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	for (i = 0; i < ARRAY_SIZE(wm8985->supplies); i++)
+		regulator_unregister_notifier(wm8985->supplies[i].consumer,
+					      &wm8985->disable_nb[i]);
 	regulator_bulk_free(ARRAY_SIZE(wm8985->supplies), wm8985->supplies);
 	return 0;
 }
@@ -978,6 +1004,7 @@ static int wm8985_probe(struct snd_soc_codec *codec)
 	u16 *cache;
 
 	wm8985 = snd_soc_codec_get_drvdata(codec);
+	wm8985->codec = codec;
 
 	ret = snd_soc_codec_set_cache_io(codec, 7, 9, wm8985->control_type);
 	if (ret < 0) {
@@ -995,6 +1022,22 @@ static int wm8985_probe(struct snd_soc_codec *codec)
 		return ret;
 	}
 
+	wm8985->disable_nb[0].notifier_call = wm8985_regulator_event_0;
+	wm8985->disable_nb[1].notifier_call = wm8985_regulator_event_1;
+	wm8985->disable_nb[2].notifier_call = wm8985_regulator_event_2;
+	wm8985->disable_nb[3].notifier_call = wm8985_regulator_event_3;
+
+	/* This should really be moved into the regulator core */
+	for (i = 0; i < ARRAY_SIZE(wm8985->supplies); i++) {
+		ret = regulator_register_notifier(wm8985->supplies[i].consumer,
+						  &wm8985->disable_nb[i]);
+		if (ret != 0) {
+			dev_err(codec->dev,
+				"Failed to register regulator notifier: %d\n",
+				ret);
+		}
+	}
+
 	ret = regulator_bulk_enable(ARRAY_SIZE(wm8985->supplies),
 				    wm8985->supplies);
 	if (ret) {
-- 
1.7.3

Re: [PATCH] ASoC: WM8985: Register notifier for the regulator being disabled

[Mark Brown]

On Wed, Sep 29, 2010 at 11:38:37AM +0100, Dimitris Papastamos wrote:
> Make sure we get an event when the regulator is disabled so we know
> we have to sync the cache afterwards.

Note that the whole callback thing is more important for things with
idle_bias_off (which might get the regulators powered down at runtime)
than for other things which don't go down to _BIAS_OFF at runtime so
would only trigger the callback on suspend anyway.

Not a problem doing the work, it'll just be unlikely to get called at
runtime.

> Minor stylistic changes.

Always split different things out into separate patches unless they
overlap with each other a lot.

> +/*
> + * We can't use the same notifier block for more than one supply and
> + * there's no way I can see to get from a callback to the caller
> + * except container_of().
> + */

Jarkko's way of doing this was a bit nicer, but both aren't great :(

> @@ -782,7 +806,7 @@ static int wm8985_set_pll(struct snd_soc_dai *dai, int pll_id,
>  {
>  	int ret;
>  	struct snd_soc_codec *codec;
> -	struct pll_div pll_div;
> +	struct pll_div pll_div = { 0 };
>  
>  	codec = dai->codec;
>  	if (freq_in && freq_out) {

It's better to restructure the code so that the compiler is able to
follow the control flow and see that there's no uninitialised access -
this sort of change will just shut up legitimate warnings as well.  Some
of the other CODEC drivers (wm8400 is one) have had similar changes
which seem to do the trick.

Re: [PATCH] ASoC: WM8985: Register notifier for the regulator being disabled

[Dimitris Papastamos]

On Wed, 2010-09-29 at 10:43 -0700, Mark Brown wrote:
> Note that the whole callback thing is more important for things with
> idle_bias_off (which might get the regulators powered down at runtime)
> than for other things which don't go down to _BIAS_OFF at runtime so
> would only trigger the callback on suspend anyway.

Ok I will revert this patch then.  The reason why I did not set
idle_bias_off is that I could not guarantee that the driver would still
function properly without testing it.

> Always split different things out into separate patches unless they
> overlap with each other a lot.

Yup indeed.

> It's better to restructure the code so that the compiler is able to
> follow the control flow and see that there's no uninitialised access -
> this sort of change will just shut up legitimate warnings as well.  Some
> of the other CODEC drivers (wm8400 is one) have had similar changes
> which seem to do the trick.

My version of GCC (4.5.1) does not seem to have a problem figuring out
the flow of execution.  It is just that some of the people who use this
code and have an older version of GCC get these warnings.  I agree this
is not the way to fix it as it silences legitimate warnings.  However I
can't really test any changes I make in an attempt to fixing this due
to not having an older version of gcc.

Thanks,
Dimitrios

Re: [PATCH] ASoC: WM8985: Register notifier for the regulator being disabled

[Mark Brown]

On Thu, Sep 30, 2010 at 10:51:32AM +0100, Dimitris Papastamos wrote:
> On Wed, 2010-09-29 at 10:43 -0700, Mark Brown wrote:

> > It's better to restructure the code so that the compiler is able to
> > follow the control flow and see that there's no uninitialised access -
> > this sort of change will just shut up legitimate warnings as well.  Some
> > of the other CODEC drivers (wm8400 is one) have had similar changes
> > which seem to do the trick.

> My version of GCC (4.5.1) does not seem to have a problem figuring out
> the flow of execution.  It is just that some of the people who use this
> code and have an older version of GCC get these warnings.  I agree this
> is not the way to fix it as it silences legitimate warnings.  However I
> can't really test any changes I make in an attempt to fixing this due
> to not having an older version of gcc.

Yes, there's some particular version of GCC that has trouble with this.
Whatever the WM8400 did to avoid the issue should work when applied to
this driver, it's a very common pattern in the Wolfson drivers.