[PATCH] [v3] ASoC: cs4270: use the built-in register cache support

[PATCH] [v3] ASoC: cs4270: use the built-in register cache support

[Timur Tabi]

Update the CS4270 driver to use ASoC's internal codec register cache feature.
This change allows ASoC to perform the low-level I2C operations necessary to
read the register cache.  Support is also added for initializing the register
cache with an array of known power-on default values.

The CS4270 driver was handling the register cache itself, but somwhere along
the conversion to multi-compaonent, this feature broke.

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

Mark, I don't have power-management support working on my hardware, so I can't
test cs4270_soc_resume(), but I have a suspicion that the call to
i2c_smbus_write_byte_data() should be replaced with codec->hw_write().

 sound/soc/codecs/cs4270.c |  161 +++++++++++++-------------------------------
 1 files changed, 48 insertions(+), 113 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 3a582ca..8b51245 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -106,6 +106,21 @@
 #define CS4270_MUTE_DAC_A	0x01
 #define CS4270_MUTE_DAC_B	0x02
 
+/* Power-on default values for the registers
+ *
+ * This array contains the power-on default values of the registers, with the
+ * exception of the "CHIPID" register (01h).  The lower four bits of that
+ * register contain the hardware revision, so it is treated as volatile.
+ *
+ * Also note that on the CS4270, the first readable register is 1, but ASoC
+ * assumes the first register is 0.  Therfore, the array must have an entry for
+ * register 0, but we use cs4270_reg_is_readable() to tell ASoC that it can't
+ * be read.
+ */
+static const u8 cs4270_default_reg_cache[CS4270_LASTREG + 1] = {
+	0x00, 0x00, 0x00, 0x30, 0x00, 0x60, 0x20, 0x00, 0x00
+};
+
 static const char *supply_names[] = {
 	"va", "vd", "vlc"
 };
@@ -178,6 +193,20 @@ static struct cs4270_mode_ratios cs4270_mode_ratios[] = {
 /* The number of MCLK/LRCK ratios supported by the CS4270 */
 #define NUM_MCLK_RATIOS		ARRAY_SIZE(cs4270_mode_ratios)
 
+static int cs4270_reg_is_readable(unsigned int reg)
+{
+	return (reg >= CS4270_FIRSTREG) && (reg <= CS4270_LASTREG);
+}
+
+static int cs4270_reg_is_volatile(unsigned int reg)
+{
+	/* Unreadable registers are considered volatile */
+	if ((reg < CS4270_FIRSTREG) || (reg > CS4270_LASTREG))
+		return 1;
+
+	return reg == CS4270_CHIPID;
+}
+
 /**
  * cs4270_set_dai_sysclk - determine the CS4270 samples rates.
  * @codec_dai: the codec DAI
@@ -263,97 +292,6 @@ static int cs4270_set_dai_fmt(struct snd_soc_dai *codec_dai,
 }
 
 /**
- * cs4270_fill_cache - pre-fill the CS4270 register cache.
- * @codec: the codec for this CS4270
- *
- * This function fills in the CS4270 register cache by reading the register
- * values from the hardware.
- *
- * This CS4270 registers are cached to avoid excessive I2C I/O operations.
- * After the initial read to pre-fill the cache, the CS4270 never updates
- * the register values, so we won't have a cache coherency problem.
- *
- * We use the auto-increment feature of the CS4270 to read all registers in
- * one shot.
- */
-static int cs4270_fill_cache(struct snd_soc_codec *codec)
-{
-	u8 *cache = codec->reg_cache;
-	struct i2c_client *i2c_client = codec->control_data;
-	s32 length;
-
-	length = i2c_smbus_read_i2c_block_data(i2c_client,
-		CS4270_FIRSTREG | CS4270_I2C_INCR, CS4270_NUMREGS, cache);
-
-	if (length != CS4270_NUMREGS) {
-		dev_err(codec->dev, "i2c read failure, addr=0x%x\n",
-		       i2c_client->addr);
-		return -EIO;
-	}
-
-	return 0;
-}
-
-/**
- * cs4270_read_reg_cache - read from the CS4270 register cache.
- * @codec: the codec for this CS4270
- * @reg: the register to read
- *
- * This function returns the value for a given register.  It reads only from
- * the register cache, not the hardware itself.
- *
- * This CS4270 registers are cached to avoid excessive I2C I/O operations.
- * After the initial read to pre-fill the cache, the CS4270 never updates
- * the register values, so we won't have a cache coherency problem.
- */
-static unsigned int cs4270_read_reg_cache(struct snd_soc_codec *codec,
-	unsigned int reg)
-{
-	u8 *cache = codec->reg_cache;
-
-	if ((reg < CS4270_FIRSTREG) || (reg > CS4270_LASTREG))
-		return -EIO;
-
-	return cache[reg - CS4270_FIRSTREG];
-}
-
-/**
- * cs4270_i2c_write - write to a CS4270 register via the I2C bus.
- * @codec: the codec for this CS4270
- * @reg: the register to write
- * @value: the value to write to the register
- *
- * This function writes the given value to the given CS4270 register, and
- * also updates the register cache.
- *
- * Note that we don't use the hw_write function pointer of snd_soc_codec.
- * That's because it's too clunky: the hw_write_t prototype does not match
- * i2c_smbus_write_byte_data(), and it's just another layer of overhead.
- */
-static int cs4270_i2c_write(struct snd_soc_codec *codec, unsigned int reg,
-			    unsigned int value)
-{
-	u8 *cache = codec->reg_cache;
-
-	if ((reg < CS4270_FIRSTREG) || (reg > CS4270_LASTREG))
-		return -EIO;
-
-	/* Only perform an I2C operation if the new value is different */
-	if (cache[reg - CS4270_FIRSTREG] != value) {
-		struct i2c_client *client = codec->control_data;
-		if (i2c_smbus_write_byte_data(client, reg, value)) {
-			dev_err(codec->dev, "i2c write failed\n");
-			return -EIO;
-		}
-
-		/* We've written to the hardware, so update the cache */
-		cache[reg - CS4270_FIRSTREG] = value;
-	}
-
-	return 0;
-}
-
-/**
  * cs4270_hw_params - program the CS4270 with the given hardware parameters.
  * @substream: the audio stream
  * @params: the hardware parameters to set
@@ -550,15 +488,16 @@ static struct snd_soc_dai_driver cs4270_dai = {
 static int cs4270_probe(struct snd_soc_codec *codec)
 {
 	struct cs4270_private *cs4270 = snd_soc_codec_get_drvdata(codec);
-	int i, ret, reg;
+	int i, ret;
 
 	codec->control_data = cs4270->control_data;
 
-	/* The I2C interface is set up, so pre-fill our register cache */
-
-	ret = cs4270_fill_cache(codec);
+	/* Tell ASoC what kind of I/O to use to read the registers.  ASoC will
+	 * then do the I2C transactions itself.
+	 */
+	ret = snd_soc_codec_set_cache_io(codec, 8, 8, cs4270->control_type);
 	if (ret < 0) {
-		dev_err(codec->dev, "failed to fill register cache\n");
+		dev_err(codec->dev, "failed to set cache I/O (ret=%i)\n", ret);
 		return ret;
 	}
 
@@ -567,10 +506,7 @@ static int cs4270_probe(struct snd_soc_codec *codec)
 	 * this feature disabled by default.  An application (e.g. alsactl) can
 	 * re-enabled it by using the controls.
 	 */
-
-	reg = cs4270_read_reg_cache(codec, CS4270_MUTE);
-	reg &= ~CS4270_MUTE_AUTO;
-	ret = cs4270_i2c_write(codec, CS4270_MUTE, reg);
+	ret = snd_soc_update_bits(codec, CS4270_MUTE, CS4270_MUTE_AUTO, 0);
 	if (ret < 0) {
 		dev_err(codec->dev, "i2c write failed\n");
 		return ret;
@@ -581,10 +517,8 @@ static int cs4270_probe(struct snd_soc_codec *codec)
 	 * playback has started.  An application (e.g. alsactl) can
 	 * re-enabled it by using the controls.
 	 */
-
-	reg = cs4270_read_reg_cache(codec, CS4270_TRANS);
-	reg &= ~(CS4270_TRANS_SOFT | CS4270_TRANS_ZERO);
-	ret = cs4270_i2c_write(codec, CS4270_TRANS, reg);
+	ret = snd_soc_update_bits(codec, CS4270_TRANS,
+		CS4270_TRANS_SOFT | CS4270_TRANS_ZERO, 0);
 	if (ret < 0) {
 		dev_err(codec->dev, "i2c write failed\n");
 		return ret;
@@ -707,15 +641,16 @@ static int cs4270_soc_resume(struct snd_soc_codec *codec)
  * Assign this variable to the codec_dev field of the machine driver's
  * snd_soc_device structure.
  */
-static struct snd_soc_codec_driver soc_codec_device_cs4270 = {
-	.probe =	cs4270_probe,
-	.remove =	cs4270_remove,
-	.suspend =	cs4270_soc_suspend,
-	.resume =	cs4270_soc_resume,
-	.read = cs4270_read_reg_cache,
-	.write = cs4270_i2c_write,
-	.reg_cache_size = CS4270_NUMREGS,
-	.reg_word_size = sizeof(u8),
+static const struct snd_soc_codec_driver soc_codec_device_cs4270 = {
+	.probe =		cs4270_probe,
+	.remove =		cs4270_remove,
+	.suspend =		cs4270_soc_suspend,
+	.resume =		cs4270_soc_resume,
+	.volatile_register =	cs4270_reg_is_volatile,
+	.readable_register =	cs4270_reg_is_readable,
+	.reg_cache_size =	CS4270_LASTREG + 1,
+	.reg_word_size =	sizeof(u8),
+	.reg_cache_default =	cs4270_default_reg_cache,
 };
 
 /**
-- 
1.7.3.4

[PATCH] ASoC: let snd_soc_update_bits() return an error code

[Timur Tabi]

Update snd_soc_update_bits() so that it returns a negative error code if the
the read or write operation fails.

Note that currently, a lot of the lower-level read functions have an unsigned
integer return type (and some of them even try to return a negative number),
but this code still appears to work in those cases.

An examination of the code shows that all current callers are compatible with
this change.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/soc-core.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index a233607..ea4a8e6 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -2132,19 +2132,26 @@ EXPORT_SYMBOL_GPL(snd_soc_write);
  *
  * Writes new register value.
  *
- * Returns 1 for change else 0.
+ * Returns 1 for change, 0 for no change, or negative error code.
  */
 int snd_soc_update_bits(struct snd_soc_codec *codec, unsigned short reg,
 				unsigned int mask, unsigned int value)
 {
 	int change;
 	unsigned int old, new;
+	int ret;
+
+	ret = old = snd_soc_read(codec, reg);
+	if (ret < 0)
+		return ret;
 
-	old = snd_soc_read(codec, reg);
 	new = (old & ~mask) | value;
 	change = old != new;
-	if (change)
-		snd_soc_write(codec, reg, new);
+	if (change) {
+		ret = snd_soc_write(codec, reg, new);
+		if (ret < 0)
+			return ret;
+	}
 
 	return change;
 }
-- 
1.7.3.4

Re: [PATCH] ASoC: let snd_soc_update_bits() return an error code

[Mark Brown]

On Mon, Jan 10, 2011 at 01:28:33PM -0600, Timur Tabi wrote:

> +	ret = old = snd_soc_read(codec, reg);
> +	if (ret < 0)
> +		return ret;

Please don't use multiple assignments in a single statement, it does
nothing for legibility.

Re: [PATCH] ASoC: let snd_soc_update_bits() return an error code

[Timur Tabi]

On Mon, Jan 10, 2011 at 1:29 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Mon, Jan 10, 2011 at 01:28:33PM -0600, Timur Tabi wrote:
>
>> +     ret = old = snd_soc_read(codec, reg);
>> +     if (ret < 0)
>> +             return ret;
>
> Please don't use multiple assignments in a single statement, it does
> nothing for legibility.

Bummer.  I really like that trick.  It's just so much more elegant than:

	ret = snd_soc_read(codec, reg);
	if (ret < 0)
		return ret;

	old = ret;

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] [v3] ASoC: cs4270: use the built-in register cache support

[Liam Girdwood]

On Mon, 2011-01-10 at 13:28 -0600, Timur Tabi wrote:
> Update the CS4270 driver to use ASoC's internal codec register cache feature.
> This change allows ASoC to perform the low-level I2C operations necessary to
> read the register cache.  Support is also added for initializing the register
> cache with an array of known power-on default values.
> 
> The CS4270 driver was handling the register cache itself, but somwhere along
> the conversion to multi-compaonent, this feature broke.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Acked-by: Liam Girdwood <lrg@slimlogic.co.uk>

> ---
> 
> Mark, I don't have power-management support working on my hardware, so I can't
> test cs4270_soc_resume(), but I have a suspicion that the call to
> i2c_smbus_write_byte_data() should be replaced with codec->hw_write().
> 

You may want to look at using snd_soc_cache_sync() to flush your cache
at resume().

Thanks

Liam

-- 
Freelance Developer, SlimLogic Ltd
ASoC and Voltage Regulator Maintainer.
http://www.slimlogic.co.uk

Re: [PATCH] [v3] ASoC: cs4270: use the built-in register cache support

[Mark Brown]

On Mon, Jan 10, 2011 at 01:28:32PM -0600, Timur Tabi wrote:
> Update the CS4270 driver to use ASoC's internal codec register cache feature.
> This change allows ASoC to perform the low-level I2C operations necessary to
> read the register cache.  Support is also added for initializing the register
> cache with an array of known power-on default values.
> 
> The CS4270 driver was handling the register cache itself, but somwhere along
> the conversion to multi-compaonent, this feature broke.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Applied, thanks.

> Mark, I don't have power-management support working on my hardware, so I can't
> test cs4270_soc_resume(), but I have a suspicion that the call to
> i2c_smbus_write_byte_data() should be replaced with codec->hw_write().

Like Liam said snd_soc_cache_sync() is probably what you're looking for
here.

Re: [PATCH] [v3] ASoC: cs4270: use the built-in register cache support

[Timur Tabi]

Mark Brown wrote:
>> > Mark, I don't have power-management support working on my hardware, so I can't
>> > test cs4270_soc_resume(), but I have a suspicion that the call to
>> > i2c_smbus_write_byte_data() should be replaced with codec->hw_write().
> Like Liam said snd_soc_cache_sync() is probably what you're looking for
> here.

Unfortunately, since I can't test any power management stuff on the 8610, I
can't test any changes.  I have to rely on the author of power management code
to do that.

-- 
Timur Tabi
Linux kernel developer at Freescale