[alsa-devel] [patch 1/1] sgtl5000: Fix suspend/resume
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
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.
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
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.
participants (2)
-
Arnaud Patard -
Mark Brown