At Mon, 20 Dec 2010 15:27:09 +0000, Mark Brown wrote:
On Mon, Dec 20, 2010 at 03:50:54PM +0100, Takashi Iwai wrote:
It's pretty easy to do some cleanups. After 5 minutes rewrite, over 300 LOC reduced. But I remember you wanted to work on it for portability, e.g. endianness. The same problem seems still remaining,
It wasn't portability, it was specifically the fact that some drivers want to do block operations which means they depend very strongly on the exact memory layout of the cache (obviously this only works for uncompressed caches).
But this is exactly what hinders the portability :) Which part of soc_4_12_spi_read/write specifies that this must be aligned to be BE while spi_7_9 to be LE? Why soc_16_16_i2c converts to BE internally while soc_16_8_i2c doesn't? I mean, such a difference isn't visible in the API level but hard-coded in the implementation although the API appears as if it's portable.
for example, snd_socc_16_16_read_i2c() has cpu_to_be16() while no others have such. Any progress on this? Or should I post you patchesf first?
I don't think anyone's working on it, I'm not aware of it actually bothering anyone - the code is repetitive obviously but it's also very simple. I've you've got patches please go ahead and post them but bear in mind the thing with the memory layout.
OK, I'm going to feed a few.
Also, for now, the new nice cache compression support is always built-in although the additional code size isn't so negligible and its user is just only one. Wouldn't it be better to give new Kconfig for them? I'm also no big fan for small Kconfigs, but in this case, I see no better resolution.
I don't see this as a particularly urgent issue, especially in the context of the overall ASoC code size and memory usage. There's more potential users than currently have it enabled, it's just that the WM8994 driver is the only driver that turns it on by default right now (several other CODEC drivers should but need testing just to confirm that it's OK). It's only a few kilobytes and Kconfig really isn't good at supporting this sort of selection, I'd worry about the usability issues.
Well, it drags LZO COMPRESS and DECOMPRESS unconditionally, which I feel uneasy. Usually the consumer of such lib stuff has a Kconfig, especially the feature can be optional (and in this case it is).
The necessary change is simple like below. The code that requires a compressed cache just needs to select SND_SOC_LZO_CACHE or whatever.
It's no urgent issue, yeah. It's just what I feel uneasy to see such a thing...
thanks,
Takashi
--- diff --git a/include/sound/soc.h b/include/sound/soc.h index 7e65b01..f3a5272 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -254,8 +254,12 @@ enum snd_soc_control_type {
enum snd_soc_compress_type { SND_SOC_FLAT_COMPRESSION = 1, - SND_SOC_LZO_COMPRESSION, - SND_SOC_RBTREE_COMPRESSION +#ifdef CONFIG_SND_SOC_LZO_CACHE + SND_SOC_LZO_COMPRESSION = 2, +#endif +#ifdef CONFIG_SND_SOC_RBTREE_CACHE + SND_SOC_RBTREE_COMPRESSION = 3, +#endif };
int snd_soc_register_platform(struct device *dev, diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig index 21a5465..1082c57 100644 --- a/sound/soc/Kconfig +++ b/sound/soc/Kconfig @@ -4,8 +4,6 @@
menuconfig SND_SOC tristate "ALSA for SoC audio support" - select LZO_COMPRESS - select LZO_DECOMPRESS select SND_PCM select AC97_BUS if SND_SOC_AC97_BUS select SND_JACK if INPUT=y || INPUT=SND @@ -25,6 +23,14 @@ if SND_SOC config SND_SOC_AC97_BUS bool
+config SND_SOC_LZO_CACHE + bool + select LZO_COMPRESS + select LZO_DECOMPRESS + +config SND_SOC_RBTREE_CACHE + bool + # All the supported SoCs source "sound/soc/atmel/Kconfig" source "sound/soc/au1x/Kconfig" diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig index 0f33db2..ef9fbd3 100644 --- a/sound/soc/codecs/Kconfig +++ b/sound/soc/codecs/Kconfig @@ -305,6 +305,7 @@ config SND_SOC_WM8993
config SND_SOC_WM8994 tristate + select SND_SOC_RBTREE_CACHE
config SND_SOC_WM9081 tristate diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index 0e17b40..3c471b7 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -761,6 +761,11 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec, } EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);
+/* + * RB-tree cache compression + */ +#ifdef CONFIG_SND_SOC_LZO_CACHE + struct snd_soc_rbtree_node { struct rb_node node; unsigned int reg; @@ -988,6 +993,13 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) return 0; }
+#endif /* RBTREE cache compression */ + +/* + * LZO cache compression + */ +#ifdef CONFIG_SND_SOC_LZO_CACHE + struct snd_soc_lzo_ctx { void *wmem; void *dst; @@ -1400,6 +1412,8 @@ err_tofree: return ret; }
+#endif /* CONFIG_SND_SOC_LZO_CACHE */ + static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec) { int i; @@ -1540,6 +1554,7 @@ static const struct snd_soc_cache_ops cache_types[] = { .write = snd_soc_flat_cache_write, .sync = snd_soc_flat_cache_sync }, +#ifdef CONFIG_SND_SOC_LZO_CACHE { .id = SND_SOC_LZO_COMPRESSION, .name = "LZO", @@ -1549,6 +1564,8 @@ static const struct snd_soc_cache_ops cache_types[] = { .write = snd_soc_lzo_cache_write, .sync = snd_soc_lzo_cache_sync }, +#endif +#ifdef CONFIG_SND_SOC_RBTREE_CACHE { .id = SND_SOC_RBTREE_COMPRESSION, .name = "rbtree", @@ -1558,6 +1575,7 @@ static const struct snd_soc_cache_ops cache_types[] = { .write = snd_soc_rbtree_cache_write, .sync = snd_soc_rbtree_cache_sync } +#endif };
int snd_soc_cache_init(struct snd_soc_codec *codec)