ALSA: ASoC: cs4720: use snd_soc_cache_sync()

PATCH Superseded Daniel Mack 3 participants 14 messages Nov 22, 2011

Subsystems (3)

Sound ASoC Cirrus Logic audio codecs

lore

Share & tools

lore

Daniel Mack

author

2011-11-22 13:45 UTC #0 lore raw

PATCH 1/1 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 <[email protected]>

Reported-and-tested-by Sven Neumann <[email protected]>

---

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

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

sound/soc/codecs/cs4270.c +1 -9

index f1f237e..73f46eb 100644
@@ -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);

Signature · 1 line

1.7.7.3

Mark Brown

2011-11-22 14:02 UTC #1 lore raw

Quoted 3 lines · 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.

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...

sound/soc/codecs/cs4270.c static int cs4270_soc_resume(struct snd_soc_codec *codec) · line 617

Quoted 4 lines

sound/soc/codecs/cs4270.c +0 -3

 	/* 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.

sound/soc/codecs/cs4270.c static int cs4270_soc_resume(struct snd_soc_codec *codec) · line 621

Quoted 4 lines

sound/soc/codecs/cs4270.c +0 -4

-		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.

Daniel Mack

author

2011-11-22 15:23 UTC #2 lore raw

Quoted 8 lines · Mark Brown

Quoted 3 lines · 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.
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.

Mark Brown

2011-11-22 15:35 UTC #3 lore raw

Quoted 6 lines · Daniel Mack

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.

Timur Tabi

2011-11-22 15:38 UTC #4 lore raw

Quoted 4 lines · Mark Brown

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.

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Daniel Mack

author

2011-11-22 15:41 UTC #5 lore raw

Quoted 10 lines · Timur Tabi

Quoted 4 lines · Mark Brown
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

Mark Brown

2011-11-22 15:43 UTC #6 lore raw

In reply to Timur Tabi

Quoted 3 lines · Timur Tabi

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).

Daniel Mack

author

2011-11-22 15:44 UTC #7 lore raw

In reply to Mark Brown

Quoted 9 lines · Mark Brown

Quoted 6 lines · Daniel Mack
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.

Quoted 4 lines

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.

Quoted 2 lines

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

Timur Tabi

2011-11-22 15:47 UTC #8 lore raw

Quoted 5 lines · Daniel Mack

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".

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Daniel Mack

author

2011-11-22 15:57 UTC #9 lore raw

Quoted 8 lines · Timur Tabi

Quoted 5 lines · Daniel Mack
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.

Quoted 4 lines

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.

Quoted 4 lines

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

Timur Tabi

2011-11-22 16:12 UTC #10 lore raw

Quoted 2 lines · Daniel Mack

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?

Signature · 2 lines

Timur Tabi
Linux kernel developer at Freescale

Mark Brown

2011-11-22 15:53 UTC #11 lore raw

In reply to Daniel Mack

Quoted 5 lines · Daniel Mack

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...

Quoted 1 line

We also think that the effect is actually rather new.

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

Quoted 5 lines

Quoted 2 lines
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.

Daniel Mack

author

2011-11-22 16:04 UTC #12 lore raw

Quoted 8 lines · Mark Brown

Quoted 5 lines · Daniel Mack
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.

Quoted 4 lines

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

Quoted 3 lines

Quoted 1 line
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?

Quoted 7 lines

Quoted 5 lines
Quoted 2 lines
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.

Mark Brown

2011-11-22 16:08 UTC #13 lore raw

Quoted 7 lines · Daniel Mack

Quoted 4 lines · Mark Brown
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...

Quoted 8 lines

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.

Quoted 3 lines

Quoted 1 line
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.