[alsa-devel] [PATCH] ASoC: cs4270: fix dynamic initialization of register cache
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;
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.
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.
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.
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?
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.
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.
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.
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.
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... :)
participants (2)
-
Mark Brown -
Timur Tabi