ASoC: about the array to cache registers

ASoC: about the array to cache registers

[Joonyoung Shim]

Hi,

Many ASoC codec drivers have an array to cache registers.
The array is not used any longer after memcpy to codec->reg_cache.
I think that memcpy is unnecessary, and it a waste of the memory.

How about continue using the array instead of memcpy?

For example, i modified such following patch in wm8731.c


diff --git a/sound/soc/codecs/wm8731.c b/sound/soc/codecs/wm8731.c
index 1560028..43f8de3 100644
--- a/sound/soc/codecs/wm8731.c
+++ b/sound/soc/codecs/wm8731.c
@@ -36,7 +36,6 @@ struct snd_soc_codec_device soc_codec_dev_wm8731;
 /* codec private data */
 struct wm8731_priv {
 	struct snd_soc_codec codec;
-	u16 reg_cache[WM8731_CACHEREGNUM];
 	unsigned int sysclk;
 };
 
@@ -50,7 +49,7 @@ static int wm8731_spi_write(struct spi_device *spi, const char *data, int len);
  * using 2 wire for device control, so we cache them instead.
  * There is no point in caching the reset register
  */
-static const u16 wm8731_reg[WM8731_CACHEREGNUM] = {
+static u16 wm8731_reg[WM8731_CACHEREGNUM] = {
     0x0097, 0x0097, 0x0079, 0x0079,
     0x000a, 0x0008, 0x009f, 0x000a,
     0x0000, 0x0000
@@ -586,9 +585,7 @@ static int wm8731_register(struct wm8731_priv *wm8731)
 	codec->dai = &wm8731_dai;
 	codec->num_dai = 1;
 	codec->reg_cache_size = WM8731_CACHEREGNUM;
-	codec->reg_cache = &wm8731->reg_cache;
-
-	memcpy(codec->reg_cache, wm8731_reg, sizeof(wm8731_reg));
+	codec->reg_cache = &wm8731_reg;
 
 	ret = wm8731_reset(codec);
 	if (ret < 0) {

Re: ASoC: about the array to cache registers

[Mark Brown]

On Fri, Jul 24, 2009 at 04:06:43PM +0900, Joonyoung Shim wrote:

> Many ASoC codec drivers have an array to cache registers.
> The array is not used any longer after memcpy to codec->reg_cache.
> I think that memcpy is unnecessary, and it a waste of the memory.

> How about continue using the array instead of memcpy?

There's two reasons we keep the data around at the minute.  One is that
in future we'd like to be able to have more than one CODEC, including
more tha one of the same type.  Obviously there's some devices are more
likely to be used in such a configuration than others but you do see
some unusual hardware designs.

The other is that the default values can be used to reduce the amount of
data that needs to be written back to the CODEC to restore the registers
at resume time by only writing non-default values.  This can speed up
resume, especially on larger devices and where the control bus is
contended, but it's not been widely implemented.

For the devices that don't do the resume operation we should at least
mark the arrays as __devinitdata so they can be discarded.

Re: ASoC: about the array to cache registers

[Takashi Iwai]

At Fri, 24 Jul 2009 10:18:13 +0100,
Mark Brown wrote:
> 
> On Fri, Jul 24, 2009 at 04:06:43PM +0900, Joonyoung Shim wrote:
> 
> > Many ASoC codec drivers have an array to cache registers.
> > The array is not used any longer after memcpy to codec->reg_cache.
> > I think that memcpy is unnecessary, and it a waste of the memory.
> 
> > How about continue using the array instead of memcpy?
> 
> There's two reasons we keep the data around at the minute.  One is that
> in future we'd like to be able to have more than one CODEC, including
> more tha one of the same type.  Obviously there's some devices are more
> likely to be used in such a configuration than others but you do see
> some unusual hardware designs.
> 
> The other is that the default values can be used to reduce the amount of
> data that needs to be written back to the CODEC to restore the registers
> at resume time by only writing non-default values.  This can speed up
> resume, especially on larger devices and where the control bus is
> contended, but it's not been widely implemented.

One more thing.  In theory, the driver can be bound/unbound without
reloading.  When unbound, the static array remains as changed.
Now when you binding again, this changed array is used as the initial
state, thus it puts the driver into a wrong start up.

> For the devices that don't do the resume operation we should at least
> mark the arrays as __devinitdata so they can be discarded.

For const data, __devinitconst should be used instead in general.
At least, there were problems regarding with const and __devinitdata
on some archs like ia64.  I guess most architectures for ASoC are
likely OK with __devinitdata, though :)


Takashi