Patchwork [1/5] ASoC: wm8731: rework power management

login
register
mail settings
Submitter Nicolas Ferre
Date June 29, 2011, 6 p.m.
Message ID <ccd4b125244271af7978948689cf1dcef37bc06b.1309368631.git.nicolas.ferre@atmel.com>
Download mbox | patch
Permalink /patch/102633/
State New
Headers show

Comments

Mark Brown - June 29, 2011, 5:11 p.m.
On Wed, Jun 29, 2011 at 08:00:15PM +0200, Nicolas Ferre wrote:

Don't mix multiple changes into a single patch!  There's no perceptible
code overlap between these so I don't understand why you've merged them,
it just makes review harder and the changelog less descriptive.

> - preserve crystal oscillator across suspend/resume sequence:
>   enabled by default, it should be kept enabled on resume.

This isn't what your code does...

> - if codec is in active state: set the active bit at resume time.

Your changelog doesn't explain what the actual problem is here...  I can
see it by studying the diff but the changelog makes me think the change
is bad as the subsystem shuts everything down before it enters CODEC
suspend...

>  		snd_soc_write(codec, WM8731_ACTIVE, 0x0);
> -		snd_soc_write(codec, WM8731_PWR, 0xffff);
> +		/* standby: keep crystal oscillator enabled */
> +		snd_soc_write(codec, WM8731_PWR, 0x00df);

This doesn't keep the crystal oscillator enabled, this forces it on in
suspend (and without looking at the datasheet it also changes way more
than the one register bit I'd expect to be changed).  If the system
isn't using the oscillator then that's not good.

I'd expect to see a change to using snd_soc_update_bits() based on your
description, or more likely something more involved.

> @@ -533,6 +534,8 @@ static int wm8731_suspend(struct snd_soc_codec *codec, pm_message_t state)
>  static int wm8731_resume(struct snd_soc_codec *codec)
>  {
>  	wm8731_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> +	if (codec->active)
> +		snd_soc_write(codec, WM8731_ACTIVE, 0x0001);

This should be handled as part of the bias level managemnent and cache
sync, if the disable happens in bias level management so should the
restore.
Nicolas Ferre - June 29, 2011, 6 p.m.
- preserve crystal oscillator across suspend/resume sequence:
  enabled by default, it should be kept enabled on resume.
- if codec is in active state: set the active bit at resume time.

Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 sound/soc/codecs/wm8731.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)
Nicolas Ferre - June 30, 2011, 1:57 p.m.
Le 29/06/2011 19:11, Mark Brown :
> On Wed, Jun 29, 2011 at 08:00:15PM +0200, Nicolas Ferre wrote:
> 
> Don't mix multiple changes into a single patch!  There's no perceptible
> code overlap between these so I don't understand why you've merged them,
> it just makes review harder and the changelog less descriptive.

Ok.

>> - preserve crystal oscillator across suspend/resume sequence:
>>   enabled by default, it should be kept enabled on resume.
> 
> This isn't what your code does...
> 
>>  		snd_soc_write(codec, WM8731_ACTIVE, 0x0);
>> -		snd_soc_write(codec, WM8731_PWR, 0xffff);
>> +		/* standby: keep crystal oscillator enabled */
>> +		snd_soc_write(codec, WM8731_PWR, 0x00df);
> 
> This doesn't keep the crystal oscillator enabled, this forces it on in
> suspend (and without looking at the datasheet it also changes way more
> than the one register bit I'd expect to be changed).  If the system
> isn't using the oscillator then that's not good.
> 
> I'd expect to see a change to using snd_soc_update_bits() based on your
> description, or more likely something more involved.

First of all, I experienced issues while not having OSC enabled during suspend/resume cycle. Am I right supposing that, if using oscillator to clock the codec, I have to keep it running during a suspend/resume cycle?

Is something like this sounds like an acceptable option or we need something more sophisticated?

@@ -481,7 +481,10 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
                break;
        case SND_SOC_BIAS_OFF:
                snd_soc_write(codec, WM8731_ACTIVE, 0x0);
-               snd_soc_write(codec, WM8731_PWR, 0xffff);
+               reg = 0xdf;
+               if (wm8731->sysclk_type == WM8731_SYSCLK_XTAL)
+                               reg |= 1 << 0x5;
+               snd_soc_update_bits(codec, WM8731_PWR, 0x00ff, reg);
                regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
                                       wm8731->supplies);
                break;

And, yes, there is only 8 bits dedicated to power down control in this register.

Best regards,
Nicolas Ferre - June 30, 2011, 3:04 p.m.
Le 30/06/2011 15:57, Nicolas Ferre :
> Le 29/06/2011 19:11, Mark Brown :
>> On Wed, Jun 29, 2011 at 08:00:15PM +0200, Nicolas Ferre wrote:
>>
>> Don't mix multiple changes into a single patch!  There's no perceptible
>> code overlap between these so I don't understand why you've merged them,
>> it just makes review harder and the changelog less descriptive.
> 
> Ok.
> 
>>> - preserve crystal oscillator across suspend/resume sequence:
>>>   enabled by default, it should be kept enabled on resume.
>>
>> This isn't what your code does...
>>
>>>  		snd_soc_write(codec, WM8731_ACTIVE, 0x0);
>>> -		snd_soc_write(codec, WM8731_PWR, 0xffff);
>>> +		/* standby: keep crystal oscillator enabled */
>>> +		snd_soc_write(codec, WM8731_PWR, 0x00df);
>>
>> This doesn't keep the crystal oscillator enabled, this forces it on in
>> suspend (and without looking at the datasheet it also changes way more
>> than the one register bit I'd expect to be changed).  If the system
>> isn't using the oscillator then that's not good.
>>
>> I'd expect to see a change to using snd_soc_update_bits() based on your
>> description, or more likely something more involved.
> 
> First of all, I experienced issues while not having OSC enabled during suspend/resume cycle. Am I right supposing that, if using oscillator to clock the codec, I have to keep it running during a suspend/resume cycle?
> 
> Is something like this sounds like an acceptable option or we need something more sophisticated?
> 
> @@ -481,7 +481,10 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
>                 break;
>         case SND_SOC_BIAS_OFF:
>                 snd_soc_write(codec, WM8731_ACTIVE, 0x0);
> -               snd_soc_write(codec, WM8731_PWR, 0xffff);
> +               reg = 0xdf;
> +               if (wm8731->sysclk_type == WM8731_SYSCLK_XTAL)

Actually it is: if... != WM8731_SYSCLK_XTAL

> +                               reg |= 1 << 0x5;
> +               snd_soc_update_bits(codec, WM8731_PWR, 0x00ff, reg);

And maybe here a simple snd_soc_write() is sufficient...

>                 regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
>                                        wm8731->supplies);
>                 break;
> 
> And, yes, there is only 8 bits dedicated to power down control in this register.
> 
> Best regards,
Mark Brown - June 30, 2011, 3:22 p.m.
On Thu, Jun 30, 2011 at 03:57:12PM +0200, Nicolas Ferre wrote:

Please fix your mailer to line wrap within paragraphs.

> First of all, I experienced issues while not having OSC enabled during
> suspend/resume cycle. Am I right supposing that, if using oscillator
> to clock the codec, I have to keep it running during a suspend/resume
> cycle?

No, not at all.  What makes you believe that you would you need to do
that?
Nicolas Ferre - July 1, 2011, 9:31 a.m.
Le 30/06/2011 17:22, Mark Brown :
> On Thu, Jun 30, 2011 at 03:57:12PM +0200, Nicolas Ferre wrote:
> 
> Please fix your mailer to line wrap within paragraphs.
> 
>> First of all, I experienced issues while not having OSC enabled during
>> suspend/resume cycle. Am I right supposing that, if using oscillator
>> to clock the codec, I have to keep it running during a suspend/resume
>> cycle?
> 
> No, not at all.  What makes you believe that you would you need to do
> that?

Ok, I re-tested and for sure I was totally wrong! So we forget this part
of the patch.
I guess that this belief may come from my first implementation on 2.6.35
where OSC was not part of the DAPM... Sory for the noise.

I repost the "active bit" part real-soon-now.

Thanks for your help, best regards,
Liam Girdwood - July 1, 2011, 2:18 p.m.
On 01/07/11 11:43, Nicolas Ferre wrote:
> Resuming from a suspend/resume cycle, the ACTIVE bit was not set even if a
> stream had actually been processing.
> Setting this bit at entering SND_SOC_BIAS_ON level will ensure this.
> 
> Signed-off-by: Nicolas Ferre <nicolas.ferre@atmel.com>
> ---
> Supersed the wrong patch:
> "[PATCH 1/5] ASoC: wm8731: rework power management"
> 
> now based on broonie/sound-2.6 git tree branch: for-3.1.
> 
>  sound/soc/codecs/wm8731.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
> index 2dc964b..ee4ec31 100644
> --- a/sound/soc/codecs/wm8731.c
> +++ b/sound/soc/codecs/wm8731.c
> @@ -453,6 +453,7 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
>  
>  	switch (level) {
>  	case SND_SOC_BIAS_ON:
> +		snd_soc_write(codec, WM8731_ACTIVE, 0x0001);
>  		break;
>  	case SND_SOC_BIAS_PREPARE:
>  		break;


Acked-by: Liam Girdwood <lrg@ti.com>
Mark Brown - July 1, 2011, 4:15 p.m.
On Fri, Jul 01, 2011 at 12:43:54PM +0200, Nicolas Ferre wrote:
> Resuming from a suspend/resume cycle, the ACTIVE bit was not set even if a
> stream had actually been processing.
> Setting this bit at entering SND_SOC_BIAS_ON level will ensure this.

This will set the bit even for analogue bypass paths which don't need
it.  I'd suggest making the active bit a supply widget supplying the DAC
and ADC rather than something open coded, that should do the right thing
I think.
Nicolas Ferre - July 4, 2011, 9:46 a.m.
Le 01/07/2011 18:15, Mark Brown :
> On Fri, Jul 01, 2011 at 12:43:54PM +0200, Nicolas Ferre wrote:
>> Resuming from a suspend/resume cycle, the ACTIVE bit was not set even if a
>> stream had actually been processing.
>> Setting this bit at entering SND_SOC_BIAS_ON level will ensure this.
> 
> This will set the bit even for analogue bypass paths which don't need
> it.  I'd suggest making the active bit a supply widget supplying the DAC
> and ADC rather than something open coded, that should do the right thing
> I think.

Ah, ok... but I fear that I do not know the whole system enough for this
addition ;-(
Do you think you can code this modification, I will be happy to test it,
if you want...

Best regards,
Liam Girdwood - July 4, 2011, 5:34 p.m.
On 04/07/11 18:29, Mark Brown wrote:
> On Mon, Jul 04, 2011 at 11:46:30AM +0200, Nicolas Ferre wrote:
> 
>> Ah, ok... but I fear that I do not know the whole system enough for this
>> addition ;-(
>> Do you think you can code this modification, I will be happy to test it,
>> if you want...
> 
> It's pretty trivial - try the below (Liam, are you OK with this if it
> tests OK?):
> 

All fine by me, It's been so long I've forgotten most things about the WM8731 now ;)
Nicolas Ferre - July 5, 2011, 7:58 a.m.
Le 04/07/2011 19:29, Mark Brown :
> On Mon, Jul 04, 2011 at 11:46:30AM +0200, Nicolas Ferre wrote:
> 
>> Ah, ok... but I fear that I do not know the whole system enough for this
>> addition ;-(
>> Do you think you can code this modification, I will be happy to test it,
>> if you want...
> 
> It's pretty trivial - try the below (Liam, are you OK with this if it
> tests OK?):
> 
>>From 1dd054eb530641dadbf71525ee517c847f19e161 Mon Sep 17 00:00:00 2001
> From: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Date: Mon, 4 Jul 2011 10:27:51 -0700
> Subject: [PATCH] ASoC: Manage WM8731 ACTIVE bit as a supply widget
> 
> Now we have supply widgets there's no need to open code the handling of
> the ACTIVE bit.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  sound/soc/codecs/wm8731.c |   29 +++--------------------------
>  1 files changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
> index 2dc964b..59d9e59 100644
> --- a/sound/soc/codecs/wm8731.c
> +++ b/sound/soc/codecs/wm8731.c
> @@ -175,6 +175,7 @@ static const struct snd_kcontrol_new wm8731_input_mux_controls =
>  SOC_DAPM_ENUM("Input Select", wm8731_insel_enum);
>  
>  static const struct snd_soc_dapm_widget wm8731_dapm_widgets[] = {
> +SND_SOC_DAPM_SUPPLY("Active",WM8731_ACTIVE, 0, 0, NULL, 0),

------------------------^^^^^^^
I have replaced this with ACTIVE to match the route declaration.

*With this modification* it works, so you can add my:

Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>


>  SND_SOC_DAPM_SUPPLY("OSC", WM8731_PWR, 5, 1, NULL, 0),
>  SND_SOC_DAPM_MIXER("Output Mixer", WM8731_PWR, 4, 1,
>  	&wm8731_output_mixer_controls[0],
> @@ -204,6 +205,8 @@ static int wm8731_check_osc(struct snd_soc_dapm_widget *source,
>  static const struct snd_soc_dapm_route wm8731_intercon[] = {
>  	{"DAC", NULL, "OSC", wm8731_check_osc},
>  	{"ADC", NULL, "OSC", wm8731_check_osc},
> +	{"DAC", NULL, "ACTIVE"},
> +	{"ADC", NULL, "ACTIVE"},
>  
>  	/* output mixer */
>  	{"Output Mixer", "Line Bypass Switch", "Line Input"},
> @@ -315,29 +318,6 @@ static int wm8731_hw_params(struct snd_pcm_substream *substream,
>  	return 0;
>  }
>  
> -static int wm8731_pcm_prepare(struct snd_pcm_substream *substream,
> -			      struct snd_soc_dai *dai)
> -{
> -	struct snd_soc_codec *codec = dai->codec;
> -
> -	/* set active */
> -	snd_soc_write(codec, WM8731_ACTIVE, 0x0001);
> -
> -	return 0;
> -}
> -
> -static void wm8731_shutdown(struct snd_pcm_substream *substream,
> -			    struct snd_soc_dai *dai)
> -{
> -	struct snd_soc_codec *codec = dai->codec;
> -
> -	/* deactivate */
> -	if (!codec->active) {
> -		udelay(50);
> -		snd_soc_write(codec, WM8731_ACTIVE, 0x0);
> -	}
> -}
> -
>  static int wm8731_mute(struct snd_soc_dai *dai, int mute)
>  {
>  	struct snd_soc_codec *codec = dai->codec;
> @@ -480,7 +460,6 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
>  		snd_soc_write(codec, WM8731_PWR, reg | 0x0040);
>  		break;
>  	case SND_SOC_BIAS_OFF:
> -		snd_soc_write(codec, WM8731_ACTIVE, 0x0);
>  		snd_soc_write(codec, WM8731_PWR, 0xffff);
>  		regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
>  				       wm8731->supplies);
> @@ -496,9 +475,7 @@ static int wm8731_set_bias_level(struct snd_soc_codec *codec,
>  	SNDRV_PCM_FMTBIT_S24_LE)
>  
>  static struct snd_soc_dai_ops wm8731_dai_ops = {
> -	.prepare	= wm8731_pcm_prepare,
>  	.hw_params	= wm8731_hw_params,
> -	.shutdown	= wm8731_shutdown,
>  	.digital_mute	= wm8731_mute,
>  	.set_sysclk	= wm8731_set_dai_sysclk,
>  	.set_fmt	= wm8731_set_dai_fmt,
Mark Brown - July 5, 2011, 8:27 p.m.
On Tue, Jul 05, 2011 at 09:58:30AM +0200, Nicolas Ferre wrote:

> I have replaced this with ACTIVE to match the route declaration.

> *With this modification* it works, so you can add my:

> Tested-by: Nicolas Ferre <nicolas.ferre@atmel.com>

Great, thanks.

Patch

diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 2dc964b..e65af3d 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -481,7 +481,8 @@  static int wm8731_set_bias_level(struct snd_soc_codec *codec,
 		break;
 	case SND_SOC_BIAS_OFF:
 		snd_soc_write(codec, WM8731_ACTIVE, 0x0);
-		snd_soc_write(codec, WM8731_PWR, 0xffff);
+		/* standby: keep crystal oscillator enabled */
+		snd_soc_write(codec, WM8731_PWR, 0x00df);
 		regulator_bulk_disable(ARRAY_SIZE(wm8731->supplies),
 				       wm8731->supplies);
 		break;
@@ -533,6 +534,8 @@  static int wm8731_suspend(struct snd_soc_codec *codec, pm_message_t state)
 static int wm8731_resume(struct snd_soc_codec *codec)
 {
 	wm8731_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
+	if (codec->active)
+		snd_soc_write(codec, WM8731_ACTIVE, 0x0001);
 
 	return 0;
 }