[patch 1/1] sgtl5000: Fix suspend/resume

[patch 1/1] sgtl5000: Fix suspend/resume

[Arnaud Patard]

[-- Attachment #1: sgtl5000_suspend.patch --]
[-- Type: text/plain, Size: 6447 bytes --]

sgtl5000 codec driver is assuming a wrong cache layout, leading to a broken
suspend/resume support. This patch declares properly the default cache buffer
and fix the function used to restore the regs from the cache. I've also moved
it were it belongs, in the set_bias_level function.

Signed-off-by: Arnaud Patard <arnaud.patard@rtp-net.org>
Index: imx-test/sound/soc/codecs/sgtl5000.c
===================================================================
--- imx-test.orig/sound/soc/codecs/sgtl5000.c
+++ imx-test/sound/soc/codecs/sgtl5000.c
@@ -34,37 +34,67 @@
 #define SGTL5000_MAX_REG_OFFSET	0x013A
 
 /* default value of sgtl5000 registers except DAP */
-static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET >> 1] =  {
+static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET] =  {
 	0xa011, /* 0x0000, CHIP_ID. 11 stand for revison 17 */
+	0,
 	0x0000, /* 0x0002, CHIP_DIG_POWER. */
+	0,
 	0x0008, /* 0x0004, CHIP_CKL_CTRL */
+	0,
 	0x0010, /* 0x0006, CHIP_I2S_CTRL */
+	0,
 	0x0000, /* 0x0008, reserved */
+	0,
 	0x0008, /* 0x000A, CHIP_SSS_CTRL */
+	0,
 	0x0000, /* 0x000C, reserved */
+	0,
 	0x020c, /* 0x000E, CHIP_ADCDAC_CTRL */
+	0,
 	0x3c3c, /* 0x0010, CHIP_DAC_VOL */
+	0,
 	0x0000, /* 0x0012, reserved */
+	0,
 	0x015f, /* 0x0014, CHIP_PAD_STRENGTH */
+	0,
 	0x0000, /* 0x0016, reserved */
+	0,
 	0x0000, /* 0x0018, reserved */
+	0,
 	0x0000, /* 0x001A, reserved */
+	0,
 	0x0000, /* 0x001E, reserved */
+	0,
 	0x0000, /* 0x0020, CHIP_ANA_ADC_CTRL */
+	0,
 	0x1818, /* 0x0022, CHIP_ANA_HP_CTRL */
+	0,
 	0x0111, /* 0x0024, CHIP_ANN_CTRL */
+	0,
 	0x0000, /* 0x0026, CHIP_LINREG_CTRL */
+	0,
 	0x0000, /* 0x0028, CHIP_REF_CTRL */
+	0,
 	0x0000, /* 0x002A, CHIP_MIC_CTRL */
+	0,
 	0x0000, /* 0x002C, CHIP_LINE_OUT_CTRL */
+	0,
 	0x0404, /* 0x002E, CHIP_LINE_OUT_VOL */
+	0,
 	0x7060, /* 0x0030, CHIP_ANA_POWER */
+	0,
 	0x5000, /* 0x0032, CHIP_PLL_CTRL */
+	0,
 	0x0000, /* 0x0034, CHIP_CLK_TOP_CTRL */
+	0,
 	0x0000, /* 0x0036, CHIP_ANA_STATUS */
+	0,
 	0x0000, /* 0x0038, reserved */
+	0,
 	0x0000, /* 0x003A, CHIP_ANA_TEST2 */
+	0,
 	0x0000, /* 0x003C, CHIP_SHORT_CTRL */
+	0,
 	0x0000, /* reserved */
 };
 
@@ -903,6 +933,63 @@ static int ldo_regulator_remove(struct s
 }
 
 /*
+ * restore all sgtl5000 registers,
+ * since a big hole between dap and regular registers,
+ * we will restore them respectively.
+ */
+static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
+{
+	u16 *cache = codec->reg_cache;
+	int i, step = codec->driver->reg_cache_step;
+	int regular_regs = SGTL5000_CHIP_SHORT_CTRL;
+
+	if (!codec->cache_sync)
+		return 0;
+
+	codec->cache_only = 0;
+	/*
+	 * restore power and other regs according
+	 * to set_power() and set_clock()
+	 */
+	snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL,
+			cache[SGTL5000_CHIP_LINREG_CTRL]);
+
+	snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER,
+			cache[SGTL5000_CHIP_ANA_POWER]);
+
+	msleep(10);
+	snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL,
+			cache[SGTL5000_CHIP_CLK_CTRL]);
+
+	snd_soc_write(codec, SGTL5000_CHIP_REF_CTRL,
+			cache[SGTL5000_CHIP_REF_CTRL]);
+
+	snd_soc_write(codec, SGTL5000_CHIP_LINE_OUT_CTRL,
+			cache[SGTL5000_CHIP_LINE_OUT_CTRL]);
+
+	/* restore regular registers */
+	for (i = 0; i < regular_regs; i += step) {
+		/* this regs depends on the others */
+		if (i == SGTL5000_CHIP_ANA_POWER ||
+			i == SGTL5000_CHIP_CLK_CTRL ||
+			i == SGTL5000_CHIP_LINREG_CTRL ||
+			i == SGTL5000_CHIP_LINE_OUT_CTRL ||
+			i == SGTL5000_CHIP_CLK_CTRL)
+			continue;
+
+		snd_soc_write(codec, i, cache[i]);
+	}
+
+	/* restore dap registers */
+	for (i = SGTL5000_DAP_REG_OFFSET;
+			i < SGTL5000_MAX_REG_OFFSET; i += step)
+		snd_soc_write(codec, i, cache[i]);
+
+	codec->cache_sync = 0;
+	return 0;
+}
+
+/*
  * set dac bias
  * common state changes:
  * startup:
@@ -927,15 +1014,22 @@ static int sgtl5000_set_bias_level(struc
 			ret = regulator_bulk_enable(
 						ARRAY_SIZE(sgtl5000->supplies),
 						sgtl5000->supplies);
-			if (ret)
+			if (ret) {
+				dev_err(codec->dev,
+					"Unable en enable regulators: %d\n",
+					ret);
 				return ret;
+			}
 			udelay(10);
+			/* Restore registers by cached in memory */
+			sgtl5000_restore_regs(codec);
 		}
 
 		break;
 	case SND_SOC_BIAS_OFF:
 		regulator_bulk_disable(ARRAY_SIZE(sgtl5000->supplies),
 					sgtl5000->supplies);
+		codec->cache_sync = 1;
 		break;
 	}
 
@@ -995,74 +1089,13 @@ static int sgtl5000_volatile_register(st
 #ifdef CONFIG_SUSPEND
 static int sgtl5000_suspend(struct snd_soc_codec *codec, pm_message_t state)
 {
-	sgtl5000_set_bias_level(codec, SND_SOC_BIAS_OFF);
-
-	return 0;
-}
-
-/*
- * restore all sgtl5000 registers,
- * since a big hole between dap and regular registers,
- * we will restore them respectively.
- */
-static int sgtl5000_restore_regs(struct snd_soc_codec *codec)
-{
-	u16 *cache = codec->reg_cache;
-	int i;
-	int regular_regs = SGTL5000_CHIP_SHORT_CTRL >> 1;
-
-	/* restore regular registers */
-	for (i = 0; i < regular_regs; i++) {
-		int reg = i << 1;
-
-		/* this regs depends on the others */
-		if (reg == SGTL5000_CHIP_ANA_POWER ||
-			reg == SGTL5000_CHIP_CLK_CTRL ||
-			reg == SGTL5000_CHIP_LINREG_CTRL ||
-			reg == SGTL5000_CHIP_LINE_OUT_CTRL ||
-			reg == SGTL5000_CHIP_CLK_CTRL)
-			continue;
-
-		snd_soc_write(codec, reg, cache[i]);
-	}
-
-	/* restore dap registers */
-	for (i = SGTL5000_DAP_REG_OFFSET >> 1;
-			i < SGTL5000_MAX_REG_OFFSET >> 1; i++) {
-		int reg = i << 1;
-
-		snd_soc_write(codec, reg, cache[i]);
-	}
-
-	/*
-	 * restore power and other regs according
-	 * to set_power() and set_clock()
-	 */
-	snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL,
-			cache[SGTL5000_CHIP_LINREG_CTRL >> 1]);
-
-	snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER,
-			cache[SGTL5000_CHIP_ANA_POWER >> 1]);
-
-	snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL,
-			cache[SGTL5000_CHIP_CLK_CTRL >> 1]);
-
-	snd_soc_write(codec, SGTL5000_CHIP_REF_CTRL,
-			cache[SGTL5000_CHIP_REF_CTRL >> 1]);
-
-	snd_soc_write(codec, SGTL5000_CHIP_LINE_OUT_CTRL,
-			cache[SGTL5000_CHIP_LINE_OUT_CTRL >> 1]);
-	return 0;
+	return sgtl5000_set_bias_level(codec, SND_SOC_BIAS_OFF);
 }
 
 static int sgtl5000_resume(struct snd_soc_codec *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);
-	return 0;
+	return sgtl5000_set_bias_level(codec, SND_SOC_BIAS_STANDBY);
 }
 #else
 #define sgtl5000_suspend NULL

Re: [patch 1/1] sgtl5000: Fix suspend/resume

[Mark Brown]

On Mon, Apr 04, 2011 at 05:13:00PM +0200, Arnaud Patard wrote:

>  /* default value of sgtl5000 registers except DAP */
> -static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET >> 1] =  {
> +static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET] =  {
>  	0xa011, /* 0x0000, CHIP_ID. 11 stand for revison 17 */
> +	0,

It's not immediately obvious that this is the best fix - the driver does
declare a step of 2 so I'd expect it to be able to lay out the cahce
defaults like this.  Also, it'd be easier to review the patch if you
could split out the cache stride fix from the rest of the changes,
there's quite a few things going on in this patch.

Re: [patch 1/1] sgtl5000: Fix suspend/resume

[Arnaud Patard]

Mark Brown <broonie@opensource.wolfsonmicro.com> writes:

> On Mon, Apr 04, 2011 at 05:13:00PM +0200, Arnaud Patard wrote:
>
>>  /* default value of sgtl5000 registers except DAP */
>> -static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET >> 1] =  {
>> +static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET] =  {
>>  	0xa011, /* 0x0000, CHIP_ID. 11 stand for revison 17 */
>> +	0,
>
> It's not immediately obvious that this is the best fix - the driver does
> declare a step of 2 so I'd expect it to be able to lay out the cahce
> defaults like this.  Also, it'd be easier to review the patch if you

The sgtl5000 driver is using:

static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET >> 1] =  {
       ...
};

...

.reg_cache_size = ARRAY_SIZE(sgtl5000_regs),

Which means that reg cache size is SGTL5000_MAX_REG_OFFSET / 2 and
register values stored without any "holes" in the array, except that in
flat cache case, ASoC does :

static int snd_soc_flat_cache_read(struct snd_soc_codec *codec,
                                   unsigned int reg, unsigned int
                                   *value)
{
        *value = snd_soc_get_cache_val(codec->reg_cache, reg,
                                       codec->driver->reg_word_size);
        return 0;
}

and snd_soc_get_cache_val returns cache[reg] which means that the cache
size must have a length of SGTL5000_MAX_REG_OFFSET and values stored
with "holes".

Also, without this change, the codec_reg debugfs files is returning
lines like :
0000: a0
instead of :
0000: a011

This is also the reason why resume was broken: the restored values were
not the right ones. Look at the restore regs code: instead of reading
cache[reg] like snd_soc_get_cache_val, it was reading cache[reg>>1].

Arnaud

Re: [patch 1/1] sgtl5000: Fix suspend/resume

[Mark Brown]

On Tue, Apr 05, 2011 at 09:56:12AM +0200, Arnaud Patard wrote:

> Which means that reg cache size is SGTL5000_MAX_REG_OFFSET / 2 and
> register values stored without any "holes" in the array, except that in
> flat cache case, ASoC does :

> static int snd_soc_flat_cache_read(struct snd_soc_codec *codec,
>                                    unsigned int reg, unsigned int
>                                    *value)
> {
>         *value = snd_soc_get_cache_val(codec->reg_cache, reg,
>                                        codec->driver->reg_word_size);
>         return 0;
> }

> and snd_soc_get_cache_val returns cache[reg] which means that the cache
> size must have a length of SGTL5000_MAX_REG_OFFSET and values stored
> with "holes".

But this means that _get_cache_val() needs to be fixed to take account
of the register cache step size.  We shouldn't just pad the array, that
doesn't help.