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

[PATCH] [v2] 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 |  156 ++++++++++++++-------------------------------
 1 files changed, 48 insertions(+), 108 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 3a582ca..1897d41 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
@@ -554,11 +492,12 @@ static int cs4270_probe(struct snd_soc_codec *codec)
 
 	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;
 	}
 
@@ -568,9 +507,9 @@ static int cs4270_probe(struct snd_soc_codec *codec)
 	 * re-enabled it by using the controls.
 	 */
 
-	reg = cs4270_read_reg_cache(codec, CS4270_MUTE);
+	reg = snd_soc_read(codec, CS4270_MUTE);
 	reg &= ~CS4270_MUTE_AUTO;
-	ret = cs4270_i2c_write(codec, CS4270_MUTE, reg);
+	ret = snd_soc_write(codec, CS4270_MUTE, reg);
 	if (ret < 0) {
 		dev_err(codec->dev, "i2c write failed\n");
 		return ret;
@@ -582,9 +521,9 @@ static int cs4270_probe(struct snd_soc_codec *codec)
 	 * re-enabled it by using the controls.
 	 */
 
-	reg = cs4270_read_reg_cache(codec, CS4270_TRANS);
+	reg = snd_soc_read(codec, CS4270_TRANS);
 	reg &= ~(CS4270_TRANS_SOFT | CS4270_TRANS_ZERO);
-	ret = cs4270_i2c_write(codec, CS4270_TRANS, reg);
+	ret = snd_soc_write(codec, CS4270_TRANS, reg);
 	if (ret < 0) {
 		dev_err(codec->dev, "i2c write failed\n");
 		return ret;
@@ -707,15 +646,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,
+	.reg_word_size =	sizeof(u8),
+	.reg_cache_default =	cs4270_default_reg_cache,
 };
 
 /**
-- 
1.7.3.4

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

[Dimitris Papastamos]

On Mon, 2011-01-10 at 10:01 -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>

Have you tried using the patch I've submitted today?  Using a defaults
register cache is indeed idiomatic, however, ASoC should be able to
handle NULL defaults register maps with custom read()/write() callbacks.

Thanks,
Dimitris

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

[Dimitris Papastamos]

On Mon, 2011-01-10 at 10:01 -0600, Timur Tabi wrote:
> -	reg = cs4270_read_reg_cache(codec, CS4270_MUTE);
> +	reg = snd_soc_read(codec, CS4270_MUTE);
>  	reg &= ~CS4270_MUTE_AUTO;
> -	ret = cs4270_i2c_write(codec, CS4270_MUTE, reg);
> +	ret = snd_soc_write(codec, CS4270_MUTE, reg);
>  	if (ret < 0) {
>  		dev_err(codec->dev, "i2c write failed\n");
>  		return ret;
> @@ -582,9 +521,9 @@ static int cs4270_probe(struct snd_soc_codec *codec)
>  	 * re-enabled it by using the controls.
>  	 */
>  
> -	reg = cs4270_read_reg_cache(codec, CS4270_TRANS);
> +	reg = snd_soc_read(codec, CS4270_TRANS);
>  	reg &= ~(CS4270_TRANS_SOFT | CS4270_TRANS_ZERO);
> -	ret = cs4270_i2c_write(codec, CS4270_TRANS, reg);
> +	ret = snd_soc_write(codec, CS4270_TRANS, reg);

It'd be better to use snd_soc_update_bits() for all these.

Thanks,
Dimitris

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

[Timur Tabi]

Dimitris Papastamos wrote:
> It'd be better to use snd_soc_update_bits() for all these.

Ok, but I'm not enthusiastic about the fact that snd_soc_update_bits() ignores
any errors from snd_soc_write().  Can that be fixed?

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Mark Brown]

On Mon, Jan 10, 2011 at 10:33:05AM -0600, Timur Tabi wrote:
> Dimitris Papastamos wrote:

> > It'd be better to use snd_soc_update_bits() for all these.

> Ok, but I'm not enthusiastic about the fact that snd_soc_update_bits() ignores
> any errors from snd_soc_write().  Can that be fixed?

It's possible to submit patches to core code, so yes.

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

[Dimitris Papastamos]

On Mon, 2011-01-10 at 10:33 -0600, Timur Tabi wrote:
> Dimitris Papastamos wrote:
> > It'd be better to use snd_soc_update_bits() for all these.
> 
> Ok, but I'm not enthusiastic about the fact that snd_soc_update_bits() ignores
> any errors from snd_soc_write().  Can that be fixed?

There's not much that can be done for error recovery at that point.
Usually a failing snd_soc_read()/snd_soc_write() will complain wildly
with errors from the underlying bus.  The only place that
snd_soc_read()/snd_soc_write() are checked for errors is usually in
_probe() functions.

Thanks,
Dimitris

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

[Timur Tabi]

Dimitris Papastamos wrote:
> There's not much that can be done for error recovery at that point.
> Usually a failing snd_soc_read()/snd_soc_write() will complain wildly
> with errors from the underlying bus.  The only place that
> snd_soc_read()/snd_soc_write() are checked for errors is usually in
> _probe() functions.

And that's when I call it, so that's when I'd like it to report errors.

I'll submit a patch.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Timur Tabi]

Dimitris Papastamos wrote:
> There's not much that can be done for error recovery at that point.
> Usually a failing snd_soc_read()/snd_soc_write() will complain wildly
> with errors from the underlying bus.  The only place that
> snd_soc_read()/snd_soc_write() are checked for errors is usually in
> _probe() functions.

I did a little digging, and I found something of concern, but I'm not sure if
there's a real problem.

On the surface, it appears that all current callers of snd_soc_update_bits() can
handle a negative return code.  In fact, a lot of existing code already assumes
that it does return error codes.

My concern is that there may be some higher-level function that is *not*
expecting a return value of 1 from snd_soc_update_bits().  For example, take
wm8904_put_deemph().  This function calls wm8904_set_deemph(), which ends with this:

	return snd_soc_update_bits(codec, WM8904_DAC_DIGITAL_1,
				   WM8904_DEEMPH_MASK, val);


This doesn't seem right to me.  Here, the .put function returns 0 or 1,
depending on whether or not the bits were actually updated.  Is that what's
intended?  I can't find any documentation that tells me what the return values
of snd_kcontrol_put_t are supposed to be.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Mark Brown]

On Mon, Jan 10, 2011 at 11:35:50AM -0600, Timur Tabi wrote:

> This doesn't seem right to me.  Here, the .put function returns 0 or 1,
> depending on whether or not the bits were actually updated.  Is that what's
> intended?  I can't find any documentation that tells me what the return values
> of snd_kcontrol_put_t are supposed to be.

It's supposed to be 1 for change, 0 for no change or an error - if you
look at the core functions you'll see that they generally all follow
this idiom of using the return value from snd_soc_update_bits() directly.

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

[Timur Tabi]

Mark Brown wrote:
> It's supposed to be 1 for change, 0 for no change or an error - if you
> look at the core functions you'll see that they generally all follow
> this idiom of using the return value from snd_soc_update_bits() directly.

Ok, so that all works then.

I found another issue.  snd_soc_update_bits() breaks if snd_soc_read() returns a
negative number, so I'll fix that.  But why do the I/O functions in soc-cache.c
do this:

static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec,
				       unsigned int reg)
{
...
	ret = snd_soc_cache_read(codec, reg, &val);
	if (ret < 0)
		return -1;


What's wrong with:

	if (ret < 0)
		return ret;

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Mark Brown]

On Mon, Jan 10, 2011 at 12:33:30PM -0600, Timur Tabi wrote:

> static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec,
> 				       unsigned int reg)
> {
> ...
> 	ret = snd_soc_cache_read(codec, reg, &val);
> 	if (ret < 0)
> 		return -1;

> What's wrong with:

> 	if (ret < 0)
> 		return ret;

Note that the return value of the read functions is an unsigned int
rather than an int.

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

[Timur Tabi]

Mark Brown wrote:
> Note that the return value of the read functions is an unsigned int
> rather than an int.

Oh yeah, I didn't see that.  So returning any negative number is broken.

So I don't understand the philosophy here.  Are we going to propagate errors
during read operations, or not?  It seems that sometimes ASoC does, and
sometimes it doesn't, and sometimes it doesn't even know what it wants (e.g. the
snd_soc_X_X_read functions).

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Mark Brown]

On Mon, Jan 10, 2011 at 12:41:30PM -0600, Timur Tabi wrote:

> So I don't understand the philosophy here.  Are we going to propagate errors
> during read operations, or not?  It seems that sometimes ASoC does, and
> sometimes it doesn't, and sometimes it doesn't even know what it wants (e.g. the
> snd_soc_X_X_read functions).

This is all code that's evolved over time, and originally there was no
readback facility at all (everything came from cache) so there were no
errors to report in the first place.  Besides, in general there's
nothing constructive we can do about errors anyway except log them.

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

[Timur Tabi]

Mark Brown wrote:
> This is all code that's evolved over time, and originally there was no
> readback facility at all (everything came from cache) so there were no
> errors to report in the first place.  Besides, in general there's
> nothing constructive we can do about errors anyway except log them.

Well, I'm not sure I agree.  For one thing, the current code doesn't just fail
to report I/O errors to the callers, it re-interprets an error code as valid
date and plows right on ahead.

In addition, there are places where getting a return code would be useful.  I
call snd_soc_update_bits() in my probe function, so that's a great opportunity
to tell ASoC that this device is broken and it shouldn't instantiate a sound card.

-- 
Timur Tabi
Linux kernel developer at Freescale

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

[Mark Brown]

On Mon, Jan 10, 2011 at 01:03:26PM -0600, Timur Tabi wrote:

> Well, I'm not sure I agree.  For one thing, the current code doesn't just fail
> to report I/O errors to the callers, it re-interprets an error code as valid
> date and plows right on ahead.

> In addition, there are places where getting a return code would be useful.  I
> call snd_soc_update_bits() in my probe function, so that's a great opportunity
> to tell ASoC that this device is broken and it shouldn't instantiate a sound card.

If you've got the time and enthusiasm to refactor the code to handle
this better feel free to do so; practically speaking it's not a big deal
- I/O rarely fails in production, and when it does fail there's
generally something terribly wrong with the hardware anyway.