[PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Daniel Mack]

Replace the manual register restore mechanism in cs4270.c and call
snd_soc_cache_sync() instead. The current is also wrong, as it doesn't
update the internal cache, leading to cache inconsitency after suspend.

Signed-off-by: Daniel Mack <zonque@gmail.com>
Reported-and-tested-by: Sven Neumann <s.neumann@raumfeld.com>
---

 Not sure if it's worth marking this one for stable@?

 sound/soc/codecs/cs4270.c |   10 +---------
 1 files changed, 1 insertions(+), 9 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index f1f237e..73f46eb 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -601,7 +601,6 @@ static int cs4270_soc_suspend(struct snd_soc_codec *codec, pm_message_t mesg)
 static int cs4270_soc_resume(struct snd_soc_codec *codec)
 {
 	struct cs4270_private *cs4270 = snd_soc_codec_get_drvdata(codec);
-	struct i2c_client *i2c_client = to_i2c_client(codec->dev);
 	int reg;
 
 	regulator_bulk_enable(ARRAY_SIZE(cs4270->supplies),
@@ -612,14 +611,7 @@ static int cs4270_soc_resume(struct snd_soc_codec *codec)
 	ndelay(500);
 
 	/* first restore the entire register cache ... */
-	for (reg = CS4270_FIRSTREG; reg <= CS4270_LASTREG; reg++) {
-		u8 val = snd_soc_read(codec, reg);
-
-		if (i2c_smbus_write_byte_data(i2c_client, reg, val)) {
-			dev_err(codec->dev, "i2c write failed\n");
-			return -EIO;
-		}
-	}
+	snd_soc_cache_sync(codec);
 
 	/* ... then disable the power-down bits */
 	reg = snd_soc_read(codec, CS4270_PWRCTL);
-- 
1.7.7.3

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Mark Brown]

On Tue, Nov 22, 2011 at 02:45:16PM +0100, Daniel Mack wrote:
> Replace the manual register restore mechanism in cs4270.c and call
> snd_soc_cache_sync() instead. The current is also wrong, as it doesn't
> update the internal cache, leading to cache inconsitency after suspend.

I don't understand why this is a bug fix - the code is writing the
values from the internal cache to the hardware and what's there doesn't
look obviously wrong...

>  	/* first restore the entire register cache ... */
> -	for (reg = CS4270_FIRSTREG; reg <= CS4270_LASTREG; reg++) {
> -		u8 val = snd_soc_read(codec, reg);
> -

This reads from the cache.

> -		if (i2c_smbus_write_byte_data(i2c_client, reg, val)) {
> -			dev_err(codec->dev, "i2c write failed\n");
> -			return -EIO;
> -		}

This writes the cached value to the hardware.

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Daniel Mack]

On 11/22/2011 03:02 PM, Mark Brown wrote:
> On Tue, Nov 22, 2011 at 02:45:16PM +0100, Daniel Mack wrote:
>> Replace the manual register restore mechanism in cs4270.c and call
>> snd_soc_cache_sync() instead. The current is also wrong, as it doesn't
>> update the internal cache, leading to cache inconsitency after suspend.
> 
> I don't understand why this is a bug fix - the code is writing the
> values from the internal cache to the hardware and what's there doesn't
> look obviously wrong...

Hmm, you're right, that isn't obvious. I was too fast in blaming the
code wrong as it stands. But as a matter of fact, the patch *does* fix
the problem, Sven successfully tested it on various devices.

The bug is reproducible by setting the output volume to 50% before
sending the device to suspend. After wakeup, amixer still reports the
old volume level, but the codec is in fact set to 100%. Changing it to
50% doesn't do anything, but to any other level has an effect. This is
clearly a cache sync bug, but the piece that's missing is the reason why
my patch does the right thing.

I'll look into this again - thanks for the heads-up.

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Mark Brown]

On Tue, Nov 22, 2011 at 04:23:05PM +0100, Daniel Mack wrote:

> The bug is reproducible by setting the output volume to 50% before
> sending the device to suspend. After wakeup, amixer still reports the
> old volume level, but the codec is in fact set to 100%. Changing it to
> 50% doesn't do anything, but to any other level has an effect. This is
> clearly a cache sync bug, but the piece that's missing is the reason why
> my patch does the right thing.

That's very odd, not ringing any bells at all.  It's not like the
existing code is trying to do anything clever with skipping writes, it
just blasts in all the register values and it's affecting two different
registers.

In any case, the patch is moving to factor code out in favour of core
facilities so it's a good idea anyway.

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Timur Tabi]

Mark Brown wrote:
> That's very odd, not ringing any bells at all.  It's not like the
> existing code is trying to do anything clever with skipping writes, it
> just blasts in all the register values and it's affecting two different
> registers.

Is there a quick-and-dirty way to test suspend/resume?  It's not officially supported on our board with a CS4270, so I never tried it, and I'm not familiar with it.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Daniel Mack]

On 11/22/2011 04:37 PM, Timur Tabi wrote:
> Mark Brown wrote:
>> That's very odd, not ringing any bells at all.  It's not like the 
>> existing code is trying to do anything clever with skipping writes,
>> it just blasts in all the register values and it's affecting two
>> different registers.
> 
> Is there a quick-and-dirty way to test suspend/resume?  It's not
> officially supported on our board with a CS4270, so I never tried it,
> and I'm not familiar with it.
> 

echo mem > /sys/power/state

But in our case, the codec is powered off completely, so you might have
a very different power regulator setup on your board. And that might
cause the bug not to show up, as it retains its register values over
suspend.


Daniel

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Mark Brown]

On Tue, Nov 22, 2011 at 09:37:58AM -0600, Timur Tabi wrote:

> Is there a quick-and-dirty way to test suspend/resume?  It's not
> officially supported on our board with a CS4270, so I never tried it,
> and I'm not familiar with it.

No, not really.  Nearest thing would be to enable idle_bias_off and then
do the stuff you currently do in suspend and resume in the bias
management (which might not be a bad idea actually, unless there's pops
when powering off and on it'll probably save some power).

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Daniel Mack]

On 11/22/2011 04:35 PM, Mark Brown wrote:
> On Tue, Nov 22, 2011 at 04:23:05PM +0100, Daniel Mack wrote:
> 
>> The bug is reproducible by setting the output volume to 50% before
>> sending the device to suspend. After wakeup, amixer still reports the
>> old volume level, but the codec is in fact set to 100%. Changing it to
>> 50% doesn't do anything, but to any other level has an effect. This is
>> clearly a cache sync bug, but the piece that's missing is the reason why
>> my patch does the right thing.
> 
> That's very odd, not ringing any bells at all.  It's not like the
> existing code is trying to do anything clever with skipping writes, it
> just blasts in all the register values and it's affecting two different
> registers.

The code is so simple that I'm starting to suspect
i2c_smbus_write_byte_data() is doing something very wrong, but I can't
trace it without a hardware I2C analyzer right now. The i2c-regmap
low-level implementation uses different access functions under the hood,
so maybe that's a regression.

We also think that the effect is actually rather new.

> In any case, the patch is moving to factor code out in favour of core
> facilities so it's a good idea anyway.

Hmm, we should really care for the root cause, but maybe we can still
commit this thing, with a more appropriate commit log?


Daniel

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Timur Tabi]

Daniel Mack wrote:
> The code is so simple that I'm starting to suspect
> i2c_smbus_write_byte_data() is doing something very wrong, but I can't
> trace it without a hardware I2C analyzer right now. The i2c-regmap
> low-level implementation uses different access functions under the hood,
> so maybe that's a regression.

What platform are you testing this on?  I never really understood the i2c vs. smbus thing (I can never tell which function an i2c device really needs), so I only know that the i2c code works on my PowerPC board.

Also keep in mind that a patch that affects i2c_smbus_write_byte_data() on PowerPC was recently posted, but not approved yet.  Check the thread, "i2c/busses: (mpc) Add support for SMBUS_READ_BLOCK_DATA".  

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Daniel Mack]

On 11/22/2011 04:47 PM, Timur Tabi wrote:
> Daniel Mack wrote:
>> The code is so simple that I'm starting to suspect 
>> i2c_smbus_write_byte_data() is doing something very wrong, but I
>> can't trace it without a hardware I2C analyzer right now. The
>> i2c-regmap low-level implementation uses different access functions
>> under the hood, so maybe that's a regression.
> 
> What platform are you testing this on?

Some Raumfeld devices feature this codec, connected to an ARM PXA3xx.

> I never really understood the
> i2c vs. smbus thing (I can never tell which function an i2c device
> really needs), so I only know that the i2c code works on my PowerPC
> board.

Yes, that's a mess. SMBUS is a higher level protocol that is based in
I2C and adds an awareness of "registers" and "values". However, it
really just resembles to 2-byte instructions on the bus, which is what I
needed when I implemented the code some years ago.

> Also keep in mind that a patch that affects
> i2c_smbus_write_byte_data() on PowerPC was recently posted, but not
> approved yet.  Check the thread, "i2c/busses: (mpc) Add support for
> SMBUS_READ_BLOCK_DATA".

Well, we're on a different platform and we don't use BLOCK_DATA
transfers but single byte instructions. You still think it's related?


Daniel

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Timur Tabi]

Daniel Mack wrote:
> Well, we're on a different platform and we don't use BLOCK_DATA
> transfers but single byte instructions. You still think it's related?

I have no idea, I just wanted to bring it to your attention, in case it sparked something.  The original code uses i2c_smbus_write_byte_data, but the new code uses i2c_transfer().  Maybe that function is smarter about handling the I2C bus?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Mark Brown]

On Tue, Nov 22, 2011 at 04:44:39PM +0100, Daniel Mack wrote:

> The code is so simple that I'm starting to suspect
> i2c_smbus_write_byte_data() is doing something very wrong, but I can't
> trace it without a hardware I2C analyzer right now. The i2c-regmap
> low-level implementation uses different access functions under the hood,
> so maybe that's a regression.

Could be, though I can't see any changes in the code here.  It might be
that a change between SMBus and regmap at runtime is upsetting the
device somehow but given that it's supposed to be coming back from a
power on reset...

> We also think that the effect is actually rather new.

Does reverting to the non-regmap soc-io.c help?

> > In any case, the patch is moving to factor code out in favour of core
> > facilities so it's a good idea anyway.

> Hmm, we should really care for the root cause, but maybe we can still
> commit this thing, with a more appropriate commit log?

I think so.

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Daniel Mack]

On 11/22/2011 04:53 PM, Mark Brown wrote:
> On Tue, Nov 22, 2011 at 04:44:39PM +0100, Daniel Mack wrote:
> 
>> The code is so simple that I'm starting to suspect
>> i2c_smbus_write_byte_data() is doing something very wrong, but I can't
>> trace it without a hardware I2C analyzer right now. The i2c-regmap
>> low-level implementation uses different access functions under the hood,
>> so maybe that's a regression.
> 
> Could be, though I can't see any changes in the code here.  It might be
> that a change between SMBus and regmap at runtime is upsetting the
> device somehow but given that it's supposed to be coming back from a
> power on reset...

That should affect more codecs then, right?

$ git grep -l i2c_smbus_write sound/
sound/aoa/codecs/onyx.c
sound/aoa/codecs/tas.c
sound/ppc/daca.c
sound/ppc/tumbler.c
sound/soc/codecs/tpa6130a2.c

>> We also think that the effect is actually rather new.
> 
> Does reverting to the non-regmap soc-io.c help?

Is there a single commit we can revert on top of 3.2-git to test this?

>>> In any case, the patch is moving to factor code out in favour of core
>>> facilities so it's a good idea anyway.
> 
>> Hmm, we should really care for the root cause, but maybe we can still
>> commit this thing, with a more appropriate commit log?
> 
> I think so.

Ok, I'll repost with a better log then.

Re: [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()

[Mark Brown]

On Tue, Nov 22, 2011 at 05:04:46PM +0100, Daniel Mack wrote:
> On 11/22/2011 04:53 PM, Mark Brown wrote:

> > Could be, though I can't see any changes in the code here.  It might be
> > that a change between SMBus and regmap at runtime is upsetting the
> > device somehow but given that it's supposed to be coming back from a
> > power on reset...

> That should affect more codecs then, right?

> $ git grep -l i2c_smbus_write sound/
> sound/aoa/codecs/onyx.c
> sound/aoa/codecs/tas.c
> sound/ppc/daca.c
> sound/ppc/tumbler.c
> sound/soc/codecs/tpa6130a2.c

None of these use regmap so wouldn't be affected by an interaction
between the two.  The tpa6130a2 isn't going through the standard ASoC
structures at all.

> > Does reverting to the non-regmap soc-io.c help?

> Is there a single commit we can revert on top of 3.2-git to test this?

Should be, or a small series anyway.  git log sound/soc/soc-io.c should
show.