Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang

[Mark Brown <broonie@opensource.wolfsonmicro.com>]

On Thu, Nov 26, 2009 at 12:49:11AM +0530, Aggarwal, Anuj wrote:

> <snip>
>         int i;
>         u16 reg;
> 
>         /* Sync reg_cache with the hardware */
>         for (reg = 0; reg < ARRAY_SIZE(tlv320aic23_reg); i++) {
>                 u16 val = tlv320aic23_read_reg_cache(codec, reg);
>                 tlv320aic23_write(codec, reg, val);
>         }
> </snip>

> I can see 'i' getting incremented, instead of 'reg'. Isn't this an
> infinite loop?

Yes, that does look entirely buggy - it seems fairly clear that suspend
and resume have never worked with this driver.  The fact that you were
removing the loop entirely meant I didn't read the actual code for bugs.

> > > This patch fixes the problem by correcting the resume path and
> > > properly activating/deactivating the digital interface while
> > > doing the suspend / off transitions.

> > What do you mean by "properly activating/deactivating the digital
> > interface"?

> [Aggarwal, Anuj] The driver was only deactivating the digital
> interface and not activating it. Hence added that part. 

So this is a separate issue, and should ideally be a separate patch - it
looks like you've got three different changes here, one to fix the buggy
register cache restore, one to manage the PWR bit on suspend and another
to possibly fix the bias configuration.  You certainly need to identify
all three changes in the changelog, especially for things like this
which look like they should go in as bug fixes (and bearing in mind that
it's the end of the release cycle).

Glancing at the code there's much more management required here -
there's other places where it's managed and the code needs to be joined
up with them.  The setting should only be restored if the device was
active when suspended, not unconditionally.

> > >  	case SND_SOC_BIAS_STANDBY:
> > > -		/* everything off except vref/vmid, */
> > > -		tlv320aic23_write(codec, TLV320AIC23_PWR, reg | 0x0040);
> > > +		/* Activate the digital interface */
> > > +		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x1);
> > >  		break;
> > >  	case SND_SOC_BIAS_OFF:
> > > -		/* everything off, dac mute, inactive */
> > > +		/* Deactivate the digital interface */
> > >  		tlv320aic23_write(codec, TLV320AIC23_ACTIVE, 0x0);
> > > -		tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);
> > >  		break;
> > 
> > This looks wrong - the driver is now no longer managing bias levels at
> > all.
> [Aggarwal, Anuj] The bias levels were wrongly managed earlier. While doing 
> a suspend, it was powering off all the interfaces and while coming up, it
> was not switching on the required interfaces. Hence no sound was coming
> out if audio was played back. 

Note that this CODEC driver impelements DAPM and so all the bits in the
power register which are controlled by DAPM will be managed by the ASoC
core without any action by the driver.

> On each bias level, which are the interfaces which need to be switched
> on / off?

The biases and any other device-wide settings, everything else should be
managed via DAPM if it's in use.