> -----Original Message----- > From: Mark Brown [mailto:broonie@xxxxxxxxxxxxxxxxxxxxxxxxxxx] > Sent: Thursday, November 26, 2009 1:10 AM > To: Aggarwal, Anuj > Cc: alsa-devel@xxxxxxxxxxxxxxxx; linux-omap@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang > > 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). Fine with me, I will push individual patches for all the issues. > > 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. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
