-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Wednesday, November 25, 2009 7:04 PM To: Aggarwal, Anuj Cc: alsa-devel@alsa-project.org; linux-omap@vger.kernel.org Subject: Re: [PATCH] ASoC: AM3517: Fix AIC23 suspend/resume hang
On Wed, Nov 25, 2009 at 06:40:31PM +0530, Anuj Aggarwal wrote:
System hang is observed While trying to do suspend. It was because of an infinite loop in the AIC23 resume path which was trying to restore AIC23 register values from the register cache.
This doesn't appear to tie in with the actual patch - where's the infinite loop? The register cache restore loop has a constant bound and I can't see anything that would make it spin infinitely.
[Aggarwal, Anuj] Here is the original code:
<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?
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.
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. Moreover, I referred twl4030 codec and there also the driver was only handling the codec power control bit in _STANDBY. So based my patch on that. On each bias level, which are the interfaces which need to be switched on / off?
- 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);- }
This also looks wrong, the register cache restore has been completely removed so if the power to the device is cut during suspend then the device will go back to register defaults and no longer function correctly after resume.
[Aggarwal, Anuj] I can add that register cache restore again with the necessary checks.