[PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Daniel Mack]

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",
-- 
1.6.2.1

[PATCH 2/2] ASoC: cs4270: add power management support

[Daniel Mack]

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,
 };
 
 /*
-- 
1.6.2.1

Re: [PATCH 2/2] ASoC: cs4270: add power management support

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ASoC: cs4270: add power management support

[Daniel Mack]

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


>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

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 |   59 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 58 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index bc338df..65c68a0 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,58 @@ 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) | 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);
+	if (reg < 0)
+		return reg;
+
+	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 +857,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,
 };
 
 /*
-- 
1.6.2.1

Re: [PATCH 2/2] ASoC: cs4270: add power management support

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ASoC: cs4270: add power management support

[Daniel Mack]

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

>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>
---
 sound/soc/codecs/cs4270.c |   58 ++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 57 insertions(+), 1 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index bc338df..4d4a45e 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,57 @@ 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) | 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 +856,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,
 };
 
 /*
-- 
1.6.2.1

Re: [PATCH 2/2] ASoC: cs4270: add power management support

[Timur Tabi]

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>

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/2] ASoC: cs4270: add power management support

[Mark Brown]

On Wed, May 06, 2009 at 10:27:19AM -0500, Timur Tabi wrote:

> Acked-by: Timur Tabi <timur@freescale.com>

Applied, thanks.

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Timur Tabi]

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)

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Daniel Mack]

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

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Mark Brown]

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.

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Daniel Mack]

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

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Mark Brown]

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.

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Daniel Mack]

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

Re: [PATCH 1/2] ASoC: cs4270: introduce CS4270_I2C_INCR

[Timur Tabi]

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.

-- 
Timur Tabi
Linux kernel developer at Freescale