[alsa-devel] [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR
Replace the magic 0x80 value with a suitable macro definition.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Timur Tabi timur@freescale.com Cc: Mark Brown broonie@sirena.org.uk --- sound/soc/codecs/cs4270.c | 3 ++- 1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index 0f453a0..bc338df 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -56,6 +56,7 @@ #define CS4270_FIRSTREG 0x01 #define CS4270_LASTREG 0x08 #define CS4270_NUMREGS (CS4270_LASTREG - CS4270_FIRSTREG + 1) +#define CS4270_I2C_INCR 0x80
/* Bit masks for the CS4270 registers */ #define CS4270_CHIPID_ID 0xF0 @@ -296,7 +297,7 @@ static int cs4270_fill_cache(struct snd_soc_codec *codec) s32 length;
length = i2c_smbus_read_i2c_block_data(i2c_client, - CS4270_FIRSTREG | 0x80, CS4270_NUMREGS, cache); + CS4270_FIRSTREG | CS4270_I2C_INCR, CS4270_NUMREGS, cache);
if (length != CS4270_NUMREGS) { dev_err(codec->dev, "i2c read failure, addr=0x%x\n",
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Timur Tabi timur@freescale.com Cc: Mark Brown broonie@sirena.org.uk --- sound/soc/codecs/cs4270.c | 61 ++++++++++++++++++++++++++++++++++++++++++++- 1 files changed, 60 insertions(+), 1 deletions(-)
diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c index bc338df..80c7282 100644 --- a/sound/soc/codecs/cs4270.c +++ b/sound/soc/codecs/cs4270.c @@ -18,7 +18,7 @@ * - The machine driver's 'startup' function must call * cs4270_set_dai_sysclk() with the value of MCLK. * - Only I2S and left-justified modes are supported - * - Power management is not supported + * - Power management is supported */
#include <linux/module.h> @@ -27,6 +27,7 @@ #include <sound/soc.h> #include <sound/initval.h> #include <linux/i2c.h> +#include <linux/delay.h>
#include "cs4270.h"
@@ -65,6 +66,8 @@ #define CS4270_PWRCTL_PDN_ADC 0x20 #define CS4270_PWRCTL_PDN_DAC 0x02 #define CS4270_PWRCTL_PDN 0x01 +#define CS4270_PWRCTL_PDN_ALL \ + (CS4270_PWRCTL_PDN_ADC | CS4270_PWRCTL_PDN_DAC | CS4270_PWRCTL_PDN) #define CS4270_MODE_SPEED_MASK 0x30 #define CS4270_MODE_1X 0x00 #define CS4270_MODE_2X 0x10 @@ -788,6 +791,60 @@ static struct i2c_device_id cs4270_id[] = { }; MODULE_DEVICE_TABLE(i2c, cs4270_id);
+#ifdef CONFIG_PM + +/* This suspend/resume implementation can handle both - a simple standby + * where the codec remains powered, and a full suspend, where the voltage + * domain the codec is connected to is teared down and/or any other hardware + * reset condition is asserted. + * + * The codec's own power saving features are enabled in the suspend callback, + * and all registers are written back to the hardware when resuming. + */ + +static int cs4270_i2c_suspend(struct i2c_client *client, pm_message_t mesg) +{ + struct cs4270_private *cs4270 = i2c_get_clientdata(client); + struct snd_soc_codec *codec = &cs4270->codec; + int reg = snd_soc_read(codec, CS4270_PWRCTL); + + if (reg < 0) + return reg; + + reg |= CS4270_PWRCTL_PDN_ALL; + return snd_soc_write(codec, CS4270_PWRCTL, reg); +} + +static int cs4270_i2c_resume(struct i2c_client *client) +{ + struct cs4270_private *cs4270 = i2c_get_clientdata(client); + struct snd_soc_codec *codec = &cs4270->codec; + int reg; + + /* In case the device was put to hard reset during sleep, we need to + * wait 500ns here before any I2C communication. */ + 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(client, reg, val)) { + dev_err(codec->dev, "i2c write failed\n"); + return -EIO; + } + } + + /* ... then disable the power-down bits */ + reg = snd_soc_read(codec, CS4270_PWRCTL); + reg &= ~CS4270_PWRCTL_PDN_ALL; + return snd_soc_write(codec, CS4270_PWRCTL, reg); +} +#else +#define cs4270_i2c_suspend NULL +#define cs4270_i2c_resume NULL +#endif /* CONFIG_PM */ + /* * cs4270_i2c_driver - I2C device identification * @@ -802,6 +859,8 @@ static struct i2c_driver cs4270_i2c_driver = { .id_table = cs4270_id, .probe = cs4270_i2c_probe, .remove = cs4270_i2c_remove, + .suspend = cs4270_i2c_suspend, + .resume = cs4270_i2c_resume, };
/*
Daniel Mack wrote:
+static int cs4270_i2c_suspend(struct i2c_client *client, pm_message_t mesg) +{
- struct cs4270_private *cs4270 = i2c_get_clientdata(client);
- struct snd_soc_codec *codec = &cs4270->codec;
- int reg = snd_soc_read(codec, CS4270_PWRCTL);
- if (reg < 0)
return reg;
You check for an error code from snd_soc_read() here, but ...
- /* ... then disable the power-down bits */
- reg = snd_soc_read(codec, CS4270_PWRCTL);
you don't check for it here.
In cs4270_i2c_suspend(), snd_soc_read() can never return an error, because you're passing a hard-coded register address.
On Tue, May 05, 2009 at 10:30:48AM -0500, Timur Tabi wrote:
+static int cs4270_i2c_suspend(struct i2c_client *client, pm_message_t mesg) +{
- struct cs4270_private *cs4270 = i2c_get_clientdata(client);
- struct snd_soc_codec *codec = &cs4270->codec;
- int reg = snd_soc_read(codec, CS4270_PWRCTL);
- if (reg < 0)
return reg;You check for an error code from snd_soc_read() here, but ...
- /* ... then disable the power-down bits */
- reg = snd_soc_read(codec, CS4270_PWRCTL);
you don't check for it here.
In cs4270_i2c_suspend(), snd_soc_read() can never return an error, because you're passing a hard-coded register address.
Ok - see the patch below.
Thanks, Daniel
Daniel Mack wrote:
From d2303f37695145018e07aad85200f06b24341648 Mon Sep 17 00:00:00 2001 From: Daniel Mack daniel@caiaq.de Date: Thu, 2 Apr 2009 15:43:44 +0200 Subject: [PATCH 2/2] ASoC: cs4270: add power management support
You forgot to add "v2" to the subject line. And you'll need a v3 on the next version of the patch, because ...
+static int cs4270_i2c_resume(struct i2c_client *client) +{
- struct cs4270_private *cs4270 = i2c_get_clientdata(client);
- struct snd_soc_codec *codec = &cs4270->codec;
- int reg;
- /* In case the device was put to hard reset during sleep, we need to
* wait 500ns here before any I2C communication. */- 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(client, reg, val)) {dev_err(codec->dev, "i2c write failed\n");return -EIO;}- }
- /* ... then disable the power-down bits */
- reg = snd_soc_read(codec, CS4270_PWRCTL);
- if (reg < 0)
return reg;
... you forgot this "reg < 0" check. Also, ...
+static int cs4270_i2c_suspend(struct i2c_client *client, pm_message_t mesg) +{
- struct cs4270_private *cs4270 = i2c_get_clientdata(client);
- struct snd_soc_codec *codec = &cs4270->codec;
- int reg = snd_soc_read(codec, CS4270_PWRCTL) | CS4270_PWRCTL_PDN_ALL;
- return snd_soc_write(codec, CS4270_PWRCTL, reg);
+}
... you need a blank line above the "return"
- reg &= ~CS4270_PWRCTL_PDN_ALL;
- return snd_soc_write(codec, CS4270_PWRCTL, reg);
+}
And here.
On Tue, May 05, 2009 at 12:55:21PM -0500, Timur Tabi wrote:
You forgot to add "v2" to the subject line. And you'll need a v3 on the next version of the patch, because ...
Ok, next round ...
Daniel
On Tue, May 5, 2009 at 6:26 PM, Daniel Mack daniel@caiaq.de wrote:
From fff7cf8e74d1529060abe5207ad9b7d39c578026 Mon Sep 17 00:00:00 2001 From: Daniel Mack daniel@caiaq.de Date: Thu, 2 Apr 2009 15:43:44 +0200 Subject: [PATCH v3] ASoC: cs4270: add power management support
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Timur Tabi timur@freescale.com Cc: Mark Brown broonie@sirena.org.uk
Acked-by: Timur Tabi timur@freescale.com
On Wed, May 06, 2009 at 10:27:19AM -0500, Timur Tabi wrote:
Acked-by: Timur Tabi timur@freescale.com
Applied, thanks.
Daniel Mack wrote:
Replace the magic 0x80 value with a suitable macro definition.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Timur Tabi timur@freescale.com Cc: Mark Brown broonie@sirena.org.uk
And I thought *I* was anal-retentive! :-)
Acked-by: Timur Tabi timur@freescale.com
(although personally, I would have called it CS4270_I2C_AUTO_INCR)
On Tue, May 05, 2009 at 09:51:47AM -0500, Timur Tabi wrote:
Daniel Mack wrote:
Replace the magic 0x80 value with a suitable macro definition.
Signed-off-by: Daniel Mack daniel@caiaq.de Cc: Timur Tabi timur@freescale.com Cc: Mark Brown broonie@sirena.org.uk
And I thought *I* was anal-retentive! :-)
Erm, yes - need to explain that :)
I planned to use that macro a second time for the register write-back at resume time. But unfortunately, the i2c stack is so badly broken that it does not allow the write of a whole data block without having the length itself as part of the message (which is wrong for the codec).
But the change was there already by then, and so I left the patch in my queue.
Daniel
On Tue, May 05, 2009 at 05:15:38PM +0200, Daniel Mack wrote:
I planned to use that macro a second time for the register write-back at resume time. But unfortunately, the i2c stack is so badly broken that it does not allow the write of a whole data block without having the length itself as part of the message (which is wrong for the codec).
Could you clarify what you mean when you say that the I2C block requires the length be part of the message? The I2C stack needs to know the amount of data to write but I'm not aware of any restrictions it places on the content of the data.
I've applied the first patch.
On Tue, May 05, 2009 at 07:09:01PM +0100, Mark Brown wrote:
I planned to use that macro a second time for the register write-back at resume time. But unfortunately, the i2c stack is so badly broken that it does not allow the write of a whole data block without having the length itself as part of the message (which is wrong for the codec).
Could you clarify what you mean when you say that the I2C block requires the length be part of the message? The I2C stack needs to know the amount of data to write but I'm not aware of any restrictions it places on the content of the data.
All high-level i2c functions to write block data, namely i2c_smbus_write_block_data() and i2c_smbus_write_i2c_block_data(), put the length of the data being sent inside the block sent on the wire. I couldn't believe it myself, but even my hardware I2C analyzer clearly shows that. The API seems to assume that communication to I2C devices always wants data to be sent with a leading command, followed by the number of data bytes attached and then the data itself. Correct me if I'm wrong on that.
Daniel
On Wed, May 06, 2009 at 01:19:24AM +0200, Daniel Mack wrote:
All high-level i2c functions to write block data, namely i2c_smbus_write_block_data() and i2c_smbus_write_i2c_block_data(), put the length of the data being sent inside the block sent on the wire. I couldn't believe it myself, but even my hardware I2C analyzer clearly shows that. The API seems to assume that communication to I2C devices always wants data to be sent with a leading command, followed by the number of data bytes attached and then the data itself. Correct me if I'm wrong on that.
That's not the I2C API, that's the SMBus API. SMBus is more restricted than I2C - it essentially defines a protocol on top of I2C. If the device doesn't implement SMBus then you should use the I2C API directly.
On Wed, May 06, 2009 at 09:44:34AM +0100, Mark Brown wrote:
All high-level i2c functions to write block data, namely i2c_smbus_write_block_data() and i2c_smbus_write_i2c_block_data(), put the length of the data being sent inside the block sent on the wire. I couldn't believe it myself, but even my hardware I2C analyzer clearly shows that. The API seems to assume that communication to I2C devices always wants data to be sent with a leading command, followed by the number of data bytes attached and then the data itself. Correct me if I'm wrong on that.
That's not the I2C API, that's the SMBus API. SMBus is more restricted than I2C - it essentially defines a protocol on top of I2C. If the device doesn't implement SMBus then you should use the I2C API directly.
Hmm, ok. That makes sense. However, the driver uses the equivalent function to read the whole register set, so I wanted to keep it in sync.
Anyway, the patch I sent for the resume code writes the registeres one by one. There are only eigth of them, so it doesn't really matter.
But thanks for your clarification :)
Daniel
On Wed, May 6, 2009 at 4:39 AM, Daniel Mack daniel@caiaq.de wrote:
Hmm, ok. That makes sense. However, the driver uses the equivalent function to read the whole register set, so I wanted to keep it in sync.
That's because I could never figure out which API I should use for this. I just kept trying variants until something worked.
participants (3)
-
Daniel Mack -
Mark Brown -
Timur Tabi