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);
/* everything off, dac mute, inactive */
/* Deactivate the digital interface */
tlv320aic23_write(codec, TLV320AIC23_PWR, 0xffff);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.