[PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Timur Tabi]

The CS4270 codec driver dynamically initializes its register cache, rather than
using a hard-coded array.  Somwhere along the conversion to multi-compaonent,
this feature broke.

Because the register cache is more deeply ingrained into ASoC itself, the
initialization of the cache has to happen earlier, in the I2C probe function.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 sound/soc/codecs/cs4270.c |   68 ++++++++++++++++----------------------------
 1 files changed, 25 insertions(+), 43 deletions(-)

diff --git a/sound/soc/codecs/cs4270.c b/sound/soc/codecs/cs4270.c
index 3a582ca..4d36c0e 100644
--- a/sound/soc/codecs/cs4270.c
+++ b/sound/soc/codecs/cs4270.c
@@ -114,6 +114,8 @@ static const char *supply_names[] = {
 struct cs4270_private {
 	enum snd_soc_control_type control_type;
 	void *control_data;
+	u8 reg_cache[CS4270_NUMREGS];
+	struct snd_soc_codec_driver codec_drv;
 	unsigned int mclk; /* Input frequency of the MCLK pin */
 	unsigned int mode; /* The mode (I2S or left-justified) */
 	unsigned int slave_mode;
@@ -263,38 +265,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
@@ -554,14 +524,6 @@ 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);
-	if (ret < 0) {
-		dev_err(codec->dev, "failed to fill register cache\n");
-		return ret;
-	}
-
 	/* Disable auto-mute.  This feature appears to be buggy.  In some
 	 * situations, auto-mute will not deactivate when it should, so we want
 	 * this feature disabled by default.  An application (e.g. alsactl) can
@@ -707,7 +669,7 @@ 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 = {
+static const struct snd_soc_codec_driver soc_codec_device_cs4270 = {
 	.probe =	cs4270_probe,
 	.remove =	cs4270_remove,
 	.suspend =	cs4270_soc_suspend,
@@ -757,12 +719,32 @@ static int cs4270_i2c_probe(struct i2c_client *i2c_client,
 		return -ENOMEM;
 	}
 
+	/* Initialize the register cache.  We do this dynamically because we
+	 * don't know what the default values are.
+	 */
+	ret = i2c_smbus_read_i2c_block_data(i2c_client,
+		CS4270_FIRSTREG | CS4270_I2C_INCR, CS4270_NUMREGS,
+		cs4270->reg_cache);
+	if (ret != CS4270_NUMREGS) {
+		dev_err(&i2c_client->dev, "i2c read failure, addr=0x%x\n",
+		       i2c_client->addr);
+		return -EIO;
+	}
+
+	/* We need to create a new copy of the snd_soc_codec_driver structure
+	 * for each CS4270 because the .reg_cache_default field is different
+	 * for each one.
+	 */
+	memcpy(&cs4270->codec_drv, &soc_codec_device_cs4270,
+	       sizeof(soc_codec_device_cs4270));
+	cs4270->codec_drv.reg_cache_default = cs4270->reg_cache;
+
 	i2c_set_clientdata(i2c_client, cs4270);
 	cs4270->control_data = i2c_client;
 	cs4270->control_type = SND_SOC_I2C;
 
-	ret = snd_soc_register_codec(&i2c_client->dev,
-			&soc_codec_device_cs4270, &cs4270_dai, 1);
+	ret = snd_soc_register_codec(&i2c_client->dev, &cs4270->codec_drv,
+				     &cs4270_dai, 1);
 	if (ret < 0)
 		kfree(cs4270);
 	return ret;
-- 
1.7.3.4

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Mark Brown]

On Thu, Jan 06, 2011 at 12:52:48PM -0600, Timur Tabi wrote:

> +	u8 reg_cache[CS4270_NUMREGS];
> +	struct snd_soc_codec_driver codec_drv;

Having a driver per device is pretty icky and off the top of my head I'd
expect it to cause problems if there are two cs4270 in the system.  It
would be much nicer and more maintainable to avoid bodging around the
API like this.

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Timur Tabi]

Mark Brown wrote:
>> > +	u8 reg_cache[CS4270_NUMREGS];
>> > +	struct snd_soc_codec_driver codec_drv;
> Having a driver per device is pretty icky and off the top of my head I'd
> expect it to cause problems if there are two cs4270 in the system.  It
> would be much nicer and more maintainable to avoid bodging around the
> API like this.

The problem is that I need a distinct default register cache for each CS4270 in
the system, so I don't know how to reconcile the idea that there is supposed to
be only one snd_soc_codec_driver for all devices.

What do I do if there are two CS4270s in a system, and they each have different
power-on default values for the registers?  Granted, it's a contrived example,
but this could happen if the first CS4270 is a rev1 chip, and the second is a
rev2 chip.

And even if this example is contrived, it's conceivable that there can be codecs
where the power-on defaults are set by pin configuration.  Perhaps codec #1 is
muted by default and codec #2 isn't.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Timur Tabi]

Mark Brown wrote:
> Having a driver per device is pretty icky and off the top of my head I'd
> expect it to cause problems if there are two cs4270 in the system.  It
> would be much nicer and more maintainable to avoid bodging around the
> API like this.

BTW, why does ASoC even care about the register cache, if I have to have my own
functions to read and write it?  In other words, why does function
cs4270_read_reg_cache() exist at all?  What is the point of having ASoC allocate
the cache if the driver has to supply functions to read and write it?

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Mark Brown]

On Thu, Jan 06, 2011 at 02:50:49PM -0600, Timur Tabi wrote:

> BTW, why does ASoC even care about the register cache, if I have to have my own
> functions to read and write it?  In other words, why does function
> cs4270_read_reg_cache() exist at all?  What is the point of having ASoC allocate
> the cache if the driver has to supply functions to read and write it?

If you're completely ignoring the standard cache and register I/O code
(which isn't ideal) then you should't be telling the core about your
cache.

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Timur Tabi]

Mark Brown wrote:
> If you're completely ignoring the standard cache and register I/O code
> (which isn't ideal) then you should't be telling the core about your
> cache.

Tell that to the person who hacked up my driver and apparently broke it.  I'm
now playing catch-up.  I need to learn all about this (new) register cache
feature and then try to figure out how to make it work in my driver.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Mark Brown]

On Thu, Jan 06, 2011 at 02:24:11PM -0600, Timur Tabi wrote:

> What do I do if there are two CS4270s in a system, and they each have different
> power-on default values for the registers?  Granted, it's a contrived example,
> but this could happen if the first CS4270 is a rev1 chip, and the second is a
> rev2 chip.

So the driver should cope with this, for example by updating the rev1
configuration to match rev2 at probe time (presumably if the change in
rev2 was important enough to introduce the change for then rev1 needs
the change anyway).

> And even if this example is contrived, it's conceivable that there can be codecs
> where the power-on defaults are set by pin configuration.  Perhaps codec #1 is
> muted by default and codec #2 isn't.

Again, do something that seems tasteful.  For example, mark the relevant
register volatile as it might get varied randomly at runtime anyway (I'm
not sure how the mute pin plays with the register value?).  Or write out
a particular value at startup.

If there really are insurmountable obstacles (I don't beleive there are)
or you're really dead set on it then teach the core about having to read
back the state from the device.

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Mark Brown]

On Thu, Jan 06, 2011 at 03:35:56PM -0600, Timur Tabi wrote:
> Mark Brown wrote:

> > If you're completely ignoring the standard cache and register I/O code
> > (which isn't ideal) then you should't be telling the core about your
> > cache.

> Tell that to the person who hacked up my driver and apparently broke it.  I'm
> now playing catch-up.  I need to learn all about this (new) register cache
> feature and then try to figure out how to make it work in my driver.

That'd be Liam in the multi-component conversion, though obviously that
didn't cause any immediate issues.  The shared I/O code has been present
since mid 2009 though, including the cache management.  This kind of
comes back to what I'm saying about making your code as idiomatic as
possible, the more a given piece of code diverges from standard idioms
the more likely it is that it will be unintentially broken by some other
change.

FWIW using the standard stuff should just be a case providing defaults,
calling snd_soc_set_cache_io() and removing your custom I/O functions.

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Timur Tabi]

Mark Brown wrote:
> That'd be Liam in the multi-component conversion, though obviously that
> didn't cause any immediate issues.  The shared I/O code has been present
> since mid 2009 though, including the cache management.  This kind of
> comes back to what I'm saying about making your code as idiomatic as
> possible, the more a given piece of code diverges from standard idioms
> the more likely it is that it will be unintentially broken by some other
> change.

My code *was* idiomatic the last time I touched it, which was early 2009.

> FWIW using the standard stuff should just be a case providing defaults,
> calling snd_soc_set_cache_io() and removing your custom I/O functions.

Ok, I'll take a look at it tomorrow.

You'd think the guys at Crystal Semi would at least give me a free coffee mug or
something.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] ASoC: cs4270: fix dynamic initialization of register cache

[Mark Brown]

On Thu, Jan 06, 2011 at 04:07:23PM -0600, Timur Tabi wrote:

> My code *was* idiomatic the last time I touched it, which was early 2009.

Well, the OF stuff was pretty hairy prior to multi-component...  :)