[alsa-devel] ASoC cache code (looking toward 2.6.38 merge window)

[Takashi Iwai]

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)