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

["Eric Bénard" <eric@eukrea.com>]

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