[alsa-devel] [PATCH] ALSA: ASoC: cs4720: use snd_soc_cache_sync()
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);
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.
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.
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.
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.
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
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).
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
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".
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
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?
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.
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.
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.
participants (3)
-
Daniel Mack -
Mark Brown -
Timur Tabi