| Message ID | s5hipqfn65k.wl%tiwai@suse.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Aug 02, 2011 at 06:13:11PM +0200, Takashi Iwai wrote: > Mark Brown wrote: > > Like I've indicated several times now we should just get rid of the code > > or hide it from the rest of the subsystem, it's being too cute for > > vanishingly little value. The register maps for these devices are > > usually at most 255 registers so the memory savings are really not > > meaningful. I'm hoping the guys working with this device will find time > > to look at fixing things, but if not I'd imagine we'll get to it at some > > point in the release cycle. > Well, there aren't so many drivers suffering from this bug, so a > temporary fix would be easy like below (totally untested). If we're going to do something like this I'd preserve the driver interface that's there rather than fiddling with their reg_cache_sizes - half the trouble here is that the meaning of that has become a bit slippery, the current code used to be correct. > @@ -421,7 +422,9 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) > return 0; > > word_size = codec->driver->reg_word_size; > - for (i = 0; i < codec->driver->reg_cache_size; ++i) { > + if (codec->driver->reg_cache_step) > + step = codec->driver->reg_cache_step; > + for (i = 0; i < codec->driver->reg_cache_size; i += step) { > val = snd_soc_get_cache_val(codec->reg_def_copy, i, > word_size); > if (!val) I'm also really unhappy with handling this in the complex caches, I'd be much more inclined to just disallow their use with devices with step sizes than to add any complexity to them.
At Wed, 3 Aug 2011 01:40:06 +0900, Mark Brown wrote: > > On Tue, Aug 02, 2011 at 06:13:11PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > Like I've indicated several times now we should just get rid of the code > > > or hide it from the rest of the subsystem, it's being too cute for > > > vanishingly little value. The register maps for these devices are > > > usually at most 255 registers so the memory savings are really not > > > meaningful. I'm hoping the guys working with this device will find time > > > to look at fixing things, but if not I'd imagine we'll get to it at some > > > point in the release cycle. > > > Well, there aren't so many drivers suffering from this bug, so a > > temporary fix would be easy like below (totally untested). > > If we're going to do something like this I'd preserve the driver > interface that's there rather than fiddling with their reg_cache_sizes - > half the trouble here is that the meaning of that has become a bit > slippery, the current code used to be correct. I don't mind either way as long as it gets fixed in way applicable to stable kernel tree. > > @@ -421,7 +422,9 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) > > return 0; > > > > word_size = codec->driver->reg_word_size; > > - for (i = 0; i < codec->driver->reg_cache_size; ++i) { > > + if (codec->driver->reg_cache_step) > > + step = codec->driver->reg_cache_step; > > + for (i = 0; i < codec->driver->reg_cache_size; i += step) { > > val = snd_soc_get_cache_val(codec->reg_def_copy, i, > > word_size); > > if (!val) > > I'm also really unhappy with handling this in the complex caches, I'd be > much more inclined to just disallow their use with devices with step > sizes than to add any complexity to them. Yeah, I find it's ugly, too. OTOH, reg_cache_step defines the validity of the register access, so we can't drop it completely. Accessing the odd number of register index is invalid when step=2, for example. So, alternatively, we can put the condition in some generic filter function like readable/writeble things. thanks, Takashi
On Tue, Aug 02, 2011 at 08:06:23PM +0200, Takashi Iwai wrote: > example. So, alternatively, we can put the condition in some generic > filter function like readable/writeble things. Yes, I suggested that already, these are just unreadable and unwritable registers :/
At Wed, 3 Aug 2011 14:23:35 +0900, Mark Brown wrote: > > On Tue, Aug 02, 2011 at 08:06:23PM +0200, Takashi Iwai wrote: > > > example. So, alternatively, we can put the condition in some generic > > filter function like readable/writeble things. > > Yes, I suggested that already, these are just unreadable and unwritable > registers :/ BTW, looking through snd_soc_get_reg_access_index() for *_readable() & co , I wonder what would be the merit of using rbtree if this kind of indexed array is present. If the actual value is also kept in that array, we can drop the whole complex stuff like rbtree and lzo. In addition, the default values aren't necessary to be in a flat array form at all. What we need is a copy function to expand the data appropriately to reg_def_copy. Or, we can consider dropping the flat reg_def_copy since it's just wasteful to keep the flat array for all types unconditionally, but let the codec driver provide an iterator and an look-up or such. thanks, Takashi
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: Wednesday, August 03, 2011 1:24 PM > To: Takashi Iwai > Cc: Dong Aisheng-B29396; alsa-devel@alsa-project.org; > s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; > w.sang@pengutronix.de > Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix > > On Tue, Aug 02, 2011 at 08:06:23PM +0200, Takashi Iwai wrote: > > > example. So, alternatively, we can put the condition in some generic > > filter function like readable/writeble things. > > Yes, I suggested that already, these are just unreadable and unwritable > registers :/ For padded cache array, the unreadable & unwritable function is correct as Takashi indicated accessing the odd number of register index is invalid with step = 2. But for packed cache array... There's a few confuse on the using of reg_cache_size and reg_cache_step in current kernel. (like some drivers use packed cache while some use padded) Maybe we need to first unify it.
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: Wednesday, August 03, 2011 12:40 AM > To: Takashi Iwai > Cc: Dong Aisheng-B29396; alsa-devel@alsa-project.org; > s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; > w.sang@pengutronix.de > Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix > > On Tue, Aug 02, 2011 at 06:13:11PM +0200, Takashi Iwai wrote: > > Mark Brown wrote: > > > > Like I've indicated several times now we should just get rid of the > > > code or hide it from the rest of the subsystem, it's being too cute > > > for vanishingly little value. The register maps for these devices > > > are usually at most 255 registers so the memory savings are really > > > not meaningful. I'm hoping the guys working with this device will > > > find time to look at fixing things, but if not I'd imagine we'll get > > > to it at some point in the release cycle. > > > Well, there aren't so many drivers suffering from this bug, so a > > temporary fix would be easy like below (totally untested). > > If we're going to do something like this I'd preserve the driver > interface that's there rather than fiddling with their reg_cache_sizes - > half the trouble here is that the meaning of that has become a bit > slippery, the current code used to be correct. > Can we fix FLAT first since it is most used and let rbtree and LZO as before?
On Wed, Aug 03, 2011 at 08:20:56AM +0200, Takashi Iwai wrote: > BTW, looking through snd_soc_get_reg_access_index() for *_readable() & > co , I wonder what would be the merit of using rbtree if this kind of > indexed array is present. If the actual value is also kept in that > array, we can drop the whole complex stuff like rbtree and lzo. I agree, you may have seen me mentioning that we should just disallow the use of advanced caches for the register maps with non-zero steps. There's really very little benefit from them. > In addition, the default values aren't necessary to be in a flat array > form at all. What we need is a copy function to expand the data > appropriately to reg_def_copy. Yes, we can do better here. We do need something that's reasonably easy to write in source code, though. I was thinking something like an array of: { reg, value } might do the trick for the sparse devices. Ideally all this code is going to get pushed out into regmap too so other subsystems can use it. > Or, we can consider dropping the flat reg_def_copy since it's just > wasteful to keep the flat array for all types unconditionally, but let > the codec driver provide an iterator and an look-up or such. The purpose of that is slightly different, the idea is to let us discard initdata while still being able to reference the defaults at runtime for CODECs that didn't get instantiated in the system. In order to allow the discard to happen we need to take a copy of the defaults table when we probe the device.
On Wed, Aug 03, 2011 at 07:39:27AM +0000, Dong Aisheng-B29396 wrote:
> Can we fix FLAT first since it is most used and let rbtree and LZO as before?
Leaving rbtree and LZO alone is exactly what I've been saying!
> -----Original Message----- > From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] > Sent: Wednesday, August 03, 2011 5:32 PM > To: Dong Aisheng-B29396 > Cc: Takashi Iwai; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; > lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de > Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix > > On Wed, Aug 03, 2011 at 07:39:27AM +0000, Dong Aisheng-B29396 wrote: > > > Can we fix FLAT first since it is most used and let rbtree and LZO as > before? > > Leaving rbtree and LZO alone is exactly what I've been saying! Sorry for not catch your point before. Do you think the my patch that provides a register and cache index conversion helper function behind for FLAT could be a way to try? If yes, I can clean my patch to only include FLAT fix and send it out for You to take a look. Using this method, codec driver does not need to pad the default cache array. During the cache read, the register will be automatically converted to cache index to fetch value. This can also fix the soc_codec_reg_show for codecs who are using packed (FLAT) Cache while step = 2. (Here I assume the packed cache only has a different step with FLAT cache. FIXME if wrong)
diff --git a/sound/soc/codecs/ad1980.c b/sound/soc/codecs/ad1980.c index 923b364..6aea8e2 100644 --- a/sound/soc/codecs/ad1980.c +++ b/sound/soc/codecs/ad1980.c @@ -244,7 +244,7 @@ static int ad1980_soc_remove(struct snd_soc_codec *codec) static struct snd_soc_codec_driver soc_codec_dev_ad1980 = { .probe = ad1980_soc_probe, .remove = ad1980_soc_remove, - .reg_cache_size = ARRAY_SIZE(ad1980_reg), + .reg_cache_size = ARRAY_SIZE(ad1980_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_default = ad1980_reg, .reg_cache_step = 2, diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 76258f2..52e5ba3 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1437,7 +1437,7 @@ static struct snd_soc_codec_driver sgtl5000_driver = { .suspend = sgtl5000_suspend, .resume = sgtl5000_resume, .set_bias_level = sgtl5000_set_bias_level, - .reg_cache_size = ARRAY_SIZE(sgtl5000_regs), + .reg_cache_size = SGTL5000_MAX_REG_OFFSET, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = sgtl5000_regs, diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c index 646b58d..7088a89 100644 --- a/sound/soc/codecs/wm9705.c +++ b/sound/soc/codecs/wm9705.c @@ -374,7 +374,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm9705 = { .resume = wm9705_soc_resume, .read = ac97_read, .write = ac97_write, - .reg_cache_size = ARRAY_SIZE(wm9705_reg), + .reg_cache_size = ARRAY_SIZE(wm9705_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = wm9705_reg, diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c index 90117f8..6149f02 100644 --- a/sound/soc/codecs/wm9712.c +++ b/sound/soc/codecs/wm9712.c @@ -662,7 +662,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm9712 = { .read = ac97_read, .write = ac97_write, .set_bias_level = wm9712_set_bias_level, - .reg_cache_size = ARRAY_SIZE(wm9712_reg), + .reg_cache_size = ARRAY_SIZE(wm9712_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = wm9712_reg, diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c index 7167cb6..1995671 100644 --- a/sound/soc/codecs/wm9713.c +++ b/sound/soc/codecs/wm9713.c @@ -1245,7 +1245,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm9713 = { .read = ac97_read, .write = ac97_write, .set_bias_level = wm9713_set_bias_level, - .reg_cache_size = ARRAY_SIZE(wm9713_reg), + .reg_cache_size = ARRAY_SIZE(wm9713_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = wm9713_reg, diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index d9f8ade..d999bc8 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -408,6 +408,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) unsigned int val; int i; int ret; + int step = 1; codec->reg_cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL); if (!codec->reg_cache) @@ -421,7 +422,9 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) return 0; word_size = codec->driver->reg_word_size; - for (i = 0; i < codec->driver->reg_cache_size; ++i) { + if (codec->driver->reg_cache_step) + step = codec->driver->reg_cache_step; + for (i = 0; i < codec->driver->reg_cache_size; i += step) { val = snd_soc_get_cache_val(codec->reg_def_copy, i, word_size); if (!val) @@ -820,9 +823,12 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec) int ret; const struct snd_soc_codec_driver *codec_drv; unsigned int val; + int step = 1; codec_drv = codec->driver; - for (i = 0; i < codec_drv->reg_cache_size; ++i) { + if (codec_drv->reg_cache_step) + step = codec_drv->reg_cache_step; + for (i = 0; i < codec_drv->reg_cache_size; i += step) { WARN_ON(codec->writable_register && codec->writable_register(codec, i)); ret = snd_soc_cache_read(codec, i, &val); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 83ad8ca..596773c 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3269,12 +3269,25 @@ int snd_soc_register_codec(struct device *dev, * the cache. */ if (codec_drv->reg_cache_default) { - codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default, - reg_size, GFP_KERNEL); + codec->reg_def_copy = kzalloc(reg_size, GFP_KERNEL); if (!codec->reg_def_copy) { ret = -ENOMEM; goto fail; } + if (codec_drv->reg_cache_step > 1) { + /* FIXME: reg_cache_default with step > 1 is + * supposed to be a packed array, so here we + * expand into the flat cache array + */ + int step = codec_drv->reg_cache_step; + int wsize = codec_dev->reg_word_size; + for (i = 0; i < reg_size / step; i += wsize) + memcpy(codec->reg_def_copy + i * step, + codec->reg_cache_default + i, + wsize); + } else + memcpy(codec->reg_def_copy, + codec_drv->reg_cache_default, reg_size); } }