Advanced Linux Sound System development

Arnaud Patard | 4 Apr 2011 17:13

[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 <at> 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
 <at>  <at>  -34,37 +34,67  <at>  <at> 
 #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,

(Continue reading)

headers

Mark Brown | 5 Apr 2011 02:18

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

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.

headers

Arnaud Patard | 5 Apr 2011 09:56

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

Mark Brown <broonie <at> 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

(Continue reading)

headers

Mark Brown | 6 Apr 2011 01:23

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

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.