[v3] ASoC: sgtl5000: Fix the cache handling

Commit Message

Fabio Estevam May 23, 2014, 2:48 p.m. UTC

From: Fabio Estevam <[e-mail]>

Since commit e5d80e82e32e (ASoC: sgtl5000: Convert to use regmap directly) a 
kernel oops is observed after a suspend/resume sequence.

According to Mark Brown:

"Yes, reg_cache isn't there if we're not using ASoC level caching.  The
fix should just be to replace the direct cache references with
snd_soc_read()s which will end up in a cache lookup if the register is
cached."

Do as suggested and also complete the cache array with the missing registers
so that sgtl5000_restore_regs() can properly cache the all the registers it 
needs.

Reported-by: Shawn Guo <[e-mail]>
Signed-off-by: Fabio Estevam <[e-mail]>
---
Changes since v2:
- Use regmap_update_bits 
Changes since v1:
- Also fix sgtl5000_set_bias_level so that suspend/resume/play sequence does
not fail after several interactions
 sound/soc/codecs/sgtl5000.c | 61 +++++++++++++++++++++++++--------------------
 sound/soc/codecs/sgtl5000.h |  2 ++
 2 files changed, 36 insertions(+), 27 deletions(-)

Comments

Mark Brown May 23, 2014, 6:33 p.m. UTC | #1

On Fri, May 23, 2014 at 11:48:58AM -0300, Fabio Estevam wrote:

>  static int sgtl5000_resume(struct snd_soc_codec *codec)
>  {
> +	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
>  	/* Bring the codec back up to standby to enable regulators */
>  	sgtl5000_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
>  
>  	/* Restore registers by cached in memory */
>  	sgtl5000_restore_regs(codec);
> +
> +	regcache_cache_only(sgtl5000->regmap, false);
>  	return 0;

Like Lars said this seems broken - why are we restoring the registers in
cache only mode, and what is actually doing the sync?  I expect the
errors you were seeing when not in cache only mode were due to the I/O
failing, most likely due to the device not being powered.

This appears to be another one of those confused things that the driver
has been doing since the early versions rather than something introduced
by your patch (or the regmap conversion for that matter) - I'd be
surprised if we're not just seeing the results of better error checking
here.

Looking at the code what I'd expect to happen here is that
set_bias_level() manages the cache enable, turning on cache only mode
just before it turns the regulators off and restoring normal mode just
after enabling them, then calling _restore_regs() after that.  The
resume call should just be a call to set_bias_level() then.

Fabio Estevam May 24, 2014, 7:13 p.m. UTC | #2

On Fri, May 23, 2014 at 3:33 PM, Mark Brown <[e-mail]> wrote:

> Looking at the code what I'd expect to happen here is that
> set_bias_level() manages the cache enable, turning on cache only mode
> just before it turns the regulators off and restoring normal mode just
> after enabling them, then calling _restore_regs() after that.  The
> resume call should just be a call to set_bias_level() then.

Thanks, Mark. Your suggestion works fine.

Patch

diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c
index 9626ee0..6b4bf9a 100644
--- a/sound/soc/codecs/sgtl5000.c
+++ b/sound/soc/codecs/sgtl5000.c
@@ -36,18 +36,32 @@ 
 
 /* default value of sgtl5000 registers */
 static const struct reg_default sgtl5000_reg_defaults[] = {
+	{ SGTL5000_CHIP_DIG_POWER,		0x0000 },
 	{ SGTL5000_CHIP_CLK_CTRL,		0x0008 },
 	{ SGTL5000_CHIP_I2S_CTRL,		0x0010 },
 	{ SGTL5000_CHIP_SSS_CTRL,		0x0010 },
+	{ SGTL5000_CHIP_ADCDAC_CTRL,		0x020c },
 	{ SGTL5000_CHIP_DAC_VOL,		0x3c3c },
 	{ SGTL5000_CHIP_PAD_STRENGTH,		0x015f },
+	{ SGTL5000_CHIP_ANA_ADC_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_ANA_HP_CTRL,		0x1818 },
 	{ SGTL5000_CHIP_ANA_CTRL,		0x0111 },
+	{ SGTL5000_CHIP_LINREG_CTRL,		0x0000 },
+	{ SGTL5000_CHIP_REF_CTRL,		0x0000 },
+	{ SGTL5000_CHIP_MIC_CTRL,		0x0000 },
+	{ SGTL5000_CHIP_LINE_OUT_CTRL,		0x0000 },
 	{ SGTL5000_CHIP_LINE_OUT_VOL,		0x0404 },
 	{ SGTL5000_CHIP_ANA_POWER,		0x7060 },
 	{ SGTL5000_CHIP_PLL_CTRL,		0x5000 },
+	{ SGTL5000_CHIP_CLK_TOP_CTRL,		0x0000 },
+	{ SGTL5000_CHIP_ANA_STATUS,		0x0000 },
+	{ SGTL5000_CHIP_SHORT_CTRL,		0x0000 },
+	{ SGTL5000_CHIP_ANA_TEST2,		0x0000 },
+	{ SGTL5000_DAP_CTRL,			0x0000 },
+	{ SGTL5000_DAP_PEQ,			0x0000 },
 	{ SGTL5000_DAP_BASS_ENHANCE,		0x0040 },
 	{ SGTL5000_DAP_BASS_ENHANCE_CTRL,	0x051f },
+	{ SGTL5000_DAP_AUDIO_EQ,		0x0000 },
 	{ SGTL5000_DAP_SURROUND,		0x0040 },
 	{ SGTL5000_DAP_EQ_BASS_BAND0,		0x002f },
 	{ SGTL5000_DAP_EQ_BASS_BAND1,		0x002f },
@@ -55,6 +69,7 @@  static const struct reg_default sgtl5000_reg_defaults[] = {
 	{ SGTL5000_DAP_EQ_BASS_BAND3,		0x002f },
 	{ SGTL5000_DAP_EQ_BASS_BAND4,		0x002f },
 	{ SGTL5000_DAP_MAIN_CHAN,		0x8000 },
+	{ SGTL5000_DAP_MIX_CHAN,		0x0000 },
 	{ SGTL5000_DAP_AVC_CTRL,		0x0510 },
 	{ SGTL5000_DAP_AVC_THRESHOLD,		0x1473 },
 	{ SGTL5000_DAP_AVC_ATTACK,		0x0028 },
@@ -924,20 +939,6 @@  static int sgtl5000_set_bias_level(struct snd_soc_codec *codec,
 			if (ret)
 				return ret;
 			udelay(10);
-
-			regcache_cache_only(sgtl5000->regmap, false);
-
-			ret = regcache_sync(sgtl5000->regmap);
-			if (ret != 0) {
-				dev_err(codec->dev,
-					"Failed to restore cache: %d\n", ret);
-
-				regcache_cache_only(sgtl5000->regmap, true);
-				regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
-						       sgtl5000->supplies);
-
-				return ret;
-			}
 		}
 
 		break;
@@ -1063,7 +1064,10 @@  static bool sgtl5000_readable(struct device *dev, unsigned int reg)
 #ifdef CONFIG_SUSPEND
 static int sgtl5000_suspend(struct snd_soc_codec *codec)
 {
+	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
+
 	sgtl5000_set_bias_level(codec, SND_SOC_BIAS_OFF);
+	regcache_cache_only(sgtl5000->regmap, true);
 
 	return 0;
 }
@@ -1075,8 +1079,8 @@  static int sgtl5000_suspend(struct snd_soc_codec *codec)
  */
 static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
 {
-	u16 *cache = codec->reg_cache;
 	u16 reg;
+	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
 
 	/* restore regular registers */
 	for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) {
@@ -1089,12 +1093,12 @@  static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
 			reg == SGTL5000_CHIP_REF_CTRL)
 			continue;
 
-		snd_soc_write(codec, reg, cache[reg]);
+		regmap_update_bits(sgtl5000->regmap, reg, reg, 0);
 	}
 
 	/* restore dap registers */
 	for (reg = SGTL5000_DAP_REG_OFFSET; reg < SGTL5000_MAX_REG_OFFSET; reg += 2)
-		snd_soc_write(codec, reg, cache[reg]);
+		regmap_update_bits(sgtl5000->regmap, reg, reg, 0);
 
 	/*
 	 * restore these regs according to the power setting sequence in
@@ -1109,30 +1113,33 @@  static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
 	 * 3. SGTL5000_CHIP_REF_CTRL controls Analog Ground Voltage,
 	 *    prefer to resotre it after SGTL5000_CHIP_ANA_POWER restored
 	 */
-	snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL,
-			cache[SGTL5000_CHIP_LINREG_CTRL]);
+	regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_LINREG_CTRL,
+			   SGTL5000_CHIP_LINREG_CTRL, 0);
 
-	snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER,
-			cache[SGTL5000_CHIP_ANA_POWER]);
+	regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_ANA_POWER,
+			   SGTL5000_PLL_POWERUP_BIT, 0);
 
-	snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL,
-			cache[SGTL5000_CHIP_CLK_CTRL]);
+	regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_CLK_CTRL,
+			   SGTL5000_MCLK_FREQ_PLL, 0);
 
-	snd_soc_write(codec, SGTL5000_CHIP_REF_CTRL,
-			cache[SGTL5000_CHIP_REF_CTRL]);
+	regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_REF_CTRL,
+			   SGTL5000_CHIP_REF_CTRL, 0);
 
-	snd_soc_write(codec, SGTL5000_CHIP_LINE_OUT_CTRL,
-			cache[SGTL5000_CHIP_LINE_OUT_CTRL]);
+	regmap_update_bits(sgtl5000->regmap, SGTL5000_CHIP_LINE_OUT_CTRL,
+			   SGTL5000_CHIP_LINE_OUT_CTRL, 0);
 	return 0;
 }
 
 static int sgtl5000_resume(struct snd_soc_codec *codec)
 {
+	struct sgtl5000_priv *sgtl5000 = snd_soc_codec_get_drvdata(codec);
 	/* Bring the codec back up to standby to enable regulators */
 	sgtl5000_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 
 	/* Restore registers by cached in memory */
 	sgtl5000_restore_regs(codec);
+
+	regcache_cache_only(sgtl5000->regmap, false);
 	return 0;
 }
 #else
diff --git a/sound/soc/codecs/sgtl5000.h b/sound/soc/codecs/sgtl5000.h
index 2f8c889..72b68c0 100644
--- a/sound/soc/codecs/sgtl5000.h
+++ b/sound/soc/codecs/sgtl5000.h
@@ -341,6 +341,8 @@ 
 #define SGTL5000_ADC_POWERUP			0x0002
 #define SGTL5000_LINE_OUT_POWERUP		0x0001
 
+#define SGTL5000_PLL_POWERUP_BIT		10
+
 /*
  * SGTL5000_CHIP_PLL_CTRL
  */