codecs/tlv320aic23: fix suspend/resume

PATCH Superseded Eric Bénard 3 participants 7 messages Jun 19, 2010 – Jun 23, 2010

Subsystems (3)

Sound ASoC Texas Instruments audio (ASoC/HDA) drivers

lore

Share & tools

lore

Eric Bénard

author

2010-06-19 08:57 UTC #0 lore raw

PATCH 1/1 codecs/tlv320aic23: fix suspend/resume

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 <[email protected]>

Cc [email protected]

---
 sound/soc/codecs/tlv320aic23.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

sound/soc/codecs/tlv320aic23.c +6 -2

index b0bae35..75e320b 100644
@@ -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;
 }

Signature · 6 lines

1.6.3.3

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Mark Brown

2010-06-19 10:59 UTC #1 lore raw

Quoted 4 lines · 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).

What is the actual problem here?

sound/soc/codecs/tlv320aic23.c static int tlv320aic23_suspend(struct platform_device *pdev, · line 623

Quoted 4 lines

sound/soc/codecs/tlv320aic23.c +3 -0

+	/* 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...

sound/soc/codecs/tlv320aic23.c static int tlv320aic23_resume(struct platform_device *pdev) · line 637

Quoted 2 lines

sound/soc/codecs/tlv320aic23.c +0 -2
-	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.

Eric Bénard

author

2010-06-19 12:07 UTC #2 lore raw

Quoted 8 lines · Mark Brown

Quoted 4 lines · 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).
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).

Quoted 7 lines

Review
Quoted 4 lines
(quoted diff) +3 -0
+	/* 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...

Quoted 8 lines

Review
Quoted 2 lines
(quoted diff) +0 -2
-	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

sound/soc/codecs/tlv320aic23.c +0 -0

@@ -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 */

Eric Bénard

author

2010-06-19 17:33 UTC #3 lore raw

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

In reply to Mark Brown

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 <[email protected]>

Cc [email protected]

---
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(-)

sound/soc/codecs/tlv320aic23.c +4 -3

index b0bae35..0a4b0fe 100644
@@ -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;

Signature · 6 lines

1.6.3.3

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Liam Girdwood

Acked-by on v2 1/1

2010-06-22 10:07 UTC #4 lore raw

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

Quoted 13 lines · 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 <[email protected]>
Cc [email protected]
Cc [email protected]
Cc [email protected]

Quoted 8 lines

---
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 <[email protected]>

Signature · 8 lines

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

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Mark Brown

Acked-by on v2 1/1

2010-06-22 23:17 UTC #5 lore raw

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

In reply to Eric Bénard

Quoted 4 lines · 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.

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

Acked-by Mark Brown <[email protected]>

Liam Girdwood

Applied

2010-06-23 10:35 UTC #6 lore raw

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

Quoted 7 lines · Mark Brown

Quoted 4 lines · 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.

Quoted 5 lines

Sorry - I should've registered that this was one that should go through Liam's tree
Acked-by Mark Brown <[email protected]>

Applied

Liam

Signature · 8 lines

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

_______________________________________________
Alsa-devel mailing list
[email protected]
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel