[PATCH] codecs/tlv320aic23: fix suspend/resume

[PATCH] codecs/tlv320aic23: fix suspend/resume

[Eric Bénard]

The PWR register actually can't be restored at resume because its
cached value was set to 0xFFFF when suspending and thus the initial
state won't be restored. Thus, sound doesn't work after resume (tested
on an i.MX27).

To workaround this, this patch saves the PWR value before turning
the codec's power off and store this value in the cached registers.

Signed-off-by: Eric Bénard <eric@eukrea.com>
Cc: broonie@opensource.wolfsonmicro.com
Cc: anuj.aggarwal@ti.com
Cc: lrg@slimlogic.co.uk
---
 sound/soc/codecs/tlv320aic23.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic23.c b/sound/soc/codecs/tlv320aic23.c
index b0bae35..75e320b 100644
--- a/sound/soc/codecs/tlv320aic23.c
+++ b/sound/soc/codecs/tlv320aic23.c
@@ -614,9 +614,15 @@ static int tlv320aic23_suspend(struct platform_device *pdev,
 {
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = socdev->card->codec;
+	u16 tmp;
 
 	tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
+	/* backup PWR register */
+	tmp = tlv320aic23_read_reg_cache(codec, TLV320AIC23_PWR);
+	/* turn off the codec */
 	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	/* restore PWR register in the cache */
+	tlv320aic23_write_reg_cache(codec, TLV320AIC23_PWR, tmp);
 
 	return 0;
 }
@@ -633,8 +639,6 @@ static int tlv320aic23_resume(struct platform_device *pdev)
 		tlv320aic23_write(codec, reg, val);
 	}
 
-	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
-
 	return 0;
 }
 
-- 
1.6.3.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] codecs/tlv320aic23: fix suspend/resume

[Mark Brown]

On Sat, Jun 19, 2010 at 10:56:54AM +0200, Eric Bénard wrote:
> The PWR register actually can't be restored at resume because its
> cached value was set to 0xFFFF when suspending and thus the initial
> state won't be restored. Thus, sound doesn't work after resume (tested
> on an i.MX27).

What is the actual problem here?

> +	/* backup PWR register */
> +	tmp = tlv320aic23_read_reg_cache(codec, TLV320AIC23_PWR);
> +	/* turn off the codec */
>  	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);

On power down we put the CODEC into BIAS_OFF...

> -	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
> -

...then we bring the power back up on resume.  If there's a problem here
it looks like the bias management functions need fixing but without
knowing what the actual problem with the system is it's not clear what
this is supposed to fix.

Re: [PATCH] codecs/tlv320aic23: fix suspend/resume

[Eric Bénard]

Le 19/06/2010 12:59, Mark Brown a écrit :
> On Sat, Jun 19, 2010 at 10:56:54AM +0200, Eric Bénard wrote:
>> The PWR register actually can't be restored at resume because its
>> cached value was set to 0xFFFF when suspending and thus the initial
>> state won't be restored. Thus, sound doesn't work after resume (tested
>> on an i.MX27).
>
> What is the actual problem here?
>
aplay test.wav => sound
echo mem > /sys/power/state => system suspended
press button to wake up => system resumed
aplay test.wav => no sound

at wake up, the codec doesn't produce any sound and checking with a 
scope shows that the I2S interface (codec is master and thus generates 
the signals) is not working (no clock produced by the codec).

>> +	/* backup PWR register */
>> +	tmp = tlv320aic23_read_reg_cache(codec, TLV320AIC23_PWR);
>> +	/* turn off the codec */
>>   	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
>
> On power down we put the CODEC into BIAS_OFF...
>
>> -	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
>> -
>
> ...then we bring the power back up on resume.  If there's a problem here
> it looks like the bias management functions need fixing but without
> knowing what the actual problem with the system is it's not clear what
> this is supposed to fix.
>
my understanding of the actual code is :

- at suspend :
tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
will set PWR to 0xFFFF (all off) which is the expected behaviour

- at resume :
tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
will set PWR to 0xff7f (all off except DAC) as its value is restored 
from the register cache, which doesn't seems to be the expected 
behaviour unless I'm one more time mistaken.


You will find hereinafter log added to tlv320aic23_write concerning PWR 
register and reg value in tlv320aic23_set_bias_level (patch copy and 
pasted bellow so it may be line wrapped) :

boot :
tlv320aic23_set_bias_level : reg is 7
tlv320aic23_write write 047 to register R6

aplay :
tlv320aic23_set_bias_level : reg is 5f
tlv320aic23_write write 057 to register R6
(so DEVICE + OSC + DAC are on)
tlv320aic23_set_bias_level : reg is 47
tlv320aic23_write write 047 to register R6
(so DEVICE + OSC + OUT + DAC are on)

suspend :
tlv320aic23_set_bias_level : reg is 5f
tlv320aic23_write write ffff to register R6

resume :
tlv320aic23_write write ffff to register R6
tlv320aic23_set_bias_level : reg is ff7f
tlv320aic23_write write ff7f to register R6

aplay (after resume) :
tlv320aic23_set_bias_level : reg is ff7f
tlv320aic23_write write ff7f to register R6
(so DEVICE is on but DAC and OSC are off)
tlv320aic23_set_bias_level : reg is ff67
tlv320aic23_write write ff67 to register R6
(so DEVICE + OUT + DAC are on but OSC is off)

So do unless I'm mistaken the actual handling of PWR register is wrong 
as OSC bit stays OFF even after resume.

Eric

--- a/sound/soc/codecs/tlv320aic23.c
+++ b/sound/soc/codecs/tlv320aic23.c
@@ -95,6 +95,8 @@ static int tlv320aic23_write(struct snd_soc_codec 
*codec, unsigned int reg,
         data[1] = value & 0xff;

         tlv320aic23_write_reg_cache(codec, reg, value);
+printk("%s write %03x to register R%u\n", __func__,
+              value, reg);

         if (codec->hw_write(codec->control_data, data, 2) == 2)
                 return 0;
@@ -556,7 +558,7 @@ static int tlv320aic23_set_bias_level(struct 
snd_soc_codec *codec,
                                       enum snd_soc_bias_level level)
  {
         u16 reg = tlv320aic23_read_reg_cache(codec, TLV320AIC23_PWR) & 
0xff7f;
-
+printk("%s : reg is %x\n", __FUNCTION__, reg);
         switch (level) {
         case SND_SOC_BIAS_ON:
                 /* vref/mid, osc on, dac unmute */

[PATCH v2] codecs/tlv320aic23: fix bias management for suspend/resume

[Eric Bénard]

in tlv320aic23_set_bias_level, for the case SND_SOC_BIAS_ON, the
comment says "vref/mid, osc on, dac unmute" but the code doesn't
clear the corresponding bits, thus when resuming, several bits are
not cleared preventing the codec from working.

in tlv320aic23_suspend, clearing the active register is not needed
as it will be done by tlv320aic23_set_bias_level, when setting
bias to SND_SOC_BIAS_OFF

Signed-off-by: Eric Bénard <eric@eukrea.com>
Cc: broonie@opensource.wolfsonmicro.com
Cc: anuj.aggarwal@ti.com
Cc: lrg@slimlogic.co.uk
---
v2:
	fix the problem in tlv320aic23_set_bias_level instead of the
	previous bad workaround in suspend.

 sound/soc/codecs/tlv320aic23.c |    7 ++++---
 1 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/sound/soc/codecs/tlv320aic23.c b/sound/soc/codecs/tlv320aic23.c
index b0bae35..0a4b0fe 100644
--- a/sound/soc/codecs/tlv320aic23.c
+++ b/sound/soc/codecs/tlv320aic23.c
@@ -560,13 +560,16 @@ static int tlv320aic23_set_bias_level(struct snd_soc_codec *codec,
 	switch (level) {
 	case SND_SOC_BIAS_ON:
 		/* vref/mid, osc on, dac unmute */
+		reg &= ~(TLV320AIC23_DEVICE_PWR_OFF | TLV320AIC23_OSC_OFF | \
+			TLV320AIC23_DAC_OFF);
 		tlv320aic23_write(codec, TLV320AIC23_PWR, reg);
 		break;
 	case SND_SOC_BIAS_PREPARE:
 		break;
 	case SND_SOC_BIAS_STANDBY:
 		/* everything off except vref/vmid, */
-		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
+		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | \
+			TLV320AIC23_CLK_OFF);
 		break;
 	case SND_SOC_BIAS_OFF:
 		/* everything off, dac mute, inactive */
@@ -615,7 +618,6 @@ static int tlv320aic23_suspend(struct platform_device *pdev,
 	struct snd_soc_device *socdev = platform_get_drvdata(pdev);
 	struct snd_soc_codec *codec = socdev->card->codec;
 
-	tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
 	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_OFF);
 
 	return 0;
@@ -632,7 +634,6 @@ static int tlv320aic23_resume(struct platform_device *pdev)
 		u16 val = tlv320aic23_read_reg_cache(codec, reg);
 		tlv320aic23_write(codec, reg, val);
 	}
-
 	tlv320aic23_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 
 	return 0;
-- 
1.6.3.3

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] codecs/tlv320aic23: fix bias management for suspend/resume

[Liam Girdwood]

On Sat, 2010-06-19 at 19:33 +0200, Eric Bénard wrote:
> in tlv320aic23_set_bias_level, for the case SND_SOC_BIAS_ON, the
> comment says "vref/mid, osc on, dac unmute" but the code doesn't
> clear the corresponding bits, thus when resuming, several bits are
> not cleared preventing the codec from working.
> 
> in tlv320aic23_suspend, clearing the active register is not needed
> as it will be done by tlv320aic23_set_bias_level, when setting
> bias to SND_SOC_BIAS_OFF
> 
> Signed-off-by: Eric Bénard <eric@eukrea.com>
> Cc: broonie@opensource.wolfsonmicro.com
> Cc: anuj.aggarwal@ti.com
> Cc: lrg@slimlogic.co.uk
> ---
> v2:
> 	fix the problem in tlv320aic23_set_bias_level instead of the
> 	previous bad workaround in suspend.
> 
>  sound/soc/codecs/tlv320aic23.c |    7 ++++---
>  1 files changed, 4 insertions(+), 3 deletions(-)
> 

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH v2] codecs/tlv320aic23: fix bias management for suspend/resume

[Mark Brown]

On Sat, Jun 19, 2010 at 07:33:39PM +0200, Eric Bénard wrote:

> in tlv320aic23_set_bias_level, for the case SND_SOC_BIAS_ON, the
> comment says "vref/mid, osc on, dac unmute" but the code doesn't
> clear the corresponding bits, thus when resuming, several bits are
> not cleared preventing the codec from working.

Sorry - I should've registered that this was one that should go through
Liam's tree

Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>

Re: [PATCH v2] codecs/tlv320aic23: fix bias management for suspend/resume

[Liam Girdwood]

On Wed, 2010-06-23 at 00:17 +0100, Mark Brown wrote:
> On Sat, Jun 19, 2010 at 07:33:39PM +0200, Eric Bénard wrote:
> 
> > in tlv320aic23_set_bias_level, for the case SND_SOC_BIAS_ON, the
> > comment says "vref/mid, osc on, dac unmute" but the code doesn't
> > clear the corresponding bits, thus when resuming, several bits are
> > not cleared preventing the codec from working.
> 
> Sorry - I should've registered that this was one that should go through
> Liam's tree
> 
> Acked-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> 
Applied

Liam
-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel