ASoC cache code (looking toward 2.6.38 merge window)

ASoC cache code (looking toward 2.6.38 merge window)

[Takashi Iwai]

Hi,

while I took a lazy glance over ASoC code now (well in a sort of
escapism mode), I found again soc-cache.c has many redundant codes.

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,
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?

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.

Since 2.6.38 merge window gets closer, we need to decide it quickly.


thanks,

Takashi

Re: ASoC cache code (looking toward 2.6.38 merge window)

[Mark Brown]

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).

> 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.

> 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.

Re: 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)

[PATCH 1/4] ASoC: clean up cache read/write functions

[Takashi Iwai]

Use common helper functions for standard read/write functions of each
data type.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-cache.c |  251 ++++++++++---------------------------------------
 1 files changed, 49 insertions(+), 202 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index 0e17b40..374b06a 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -18,19 +18,44 @@
 #include <linux/bitmap.h>
 #include <linux/rbtree.h>
 
-static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
-				     unsigned int reg)
+static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg,
+		       unsigned int value, void *data, unsigned int size)
+{
+	int ret;
+
+	if (!snd_soc_codec_volatile_register(codec, reg) &&
+	    reg < codec->driver->reg_cache_size) {
+		ret = snd_soc_cache_write(codec, reg, value);
+		if (ret < 0)
+			return -1;
+	}
+
+	if (codec->cache_only) {
+		codec->cache_sync = 1;
+		return 0;
+	}
+
+	ret = codec->hw_write(codec->control_data, data, size);
+	if (ret == size)
+		return 0;
+	if (ret < 0)
+		return ret;
+	else
+		return -EIO;
+}
+
+static unsigned int do_hw_read(struct snd_soc_codec *codec, unsigned int reg)
 {
 	int ret;
 	unsigned int val;
 
 	if (reg >= codec->driver->reg_cache_size ||
-		snd_soc_codec_volatile_register(codec, reg)) {
-			if (codec->cache_only)
-				return -1;
+	    snd_soc_codec_volatile_register(codec, reg)) {
+		if (codec->cache_only)
+			return -1;
 
-			BUG_ON(!codec->hw_read);
-			return codec->hw_read(codec, reg);
+		BUG_ON(!codec->hw_read);
+		return codec->hw_read(codec, reg);
 	}
 
 	ret = snd_soc_cache_read(codec, reg, &val);
@@ -39,34 +64,21 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
 	return val;
 }
 
+static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
+				      unsigned int reg)
+{
+	return do_hw_read(codec, reg);
+}
+
 static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
 	u8 data[2];
-	int ret;
 
 	data[0] = (reg << 4) | ((value >> 8) & 0x000f);
 	data[1] = value & 0x00ff;
 
-	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size) {
-		ret = snd_soc_cache_write(codec, reg, value);
-		if (ret < 0)
-			return -1;
-	}
-
-	if (codec->cache_only) {
-		codec->cache_sync = 1;
-		return 0;
-	}
-
-	ret = codec->hw_write(codec->control_data, data, 2);
-	if (ret == 2)
-		return 0;
-	if (ret < 0)
-		return ret;
-	else
-		return -EIO;
+	return do_hw_write(codec, reg, value, data, 2);
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -102,52 +114,18 @@ static int snd_soc_4_12_spi_write(void *control_data, const char *data,
 static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
 {
-	int ret;
-	unsigned int val;
-
-	if (reg >= codec->driver->reg_cache_size ||
-		snd_soc_codec_volatile_register(codec, reg)) {
-			if (codec->cache_only)
-				return -1;
-
-			BUG_ON(!codec->hw_read);
-			return codec->hw_read(codec, reg);
-	}
-
-	ret = snd_soc_cache_read(codec, reg, &val);
-	if (ret < 0)
-		return -1;
-	return val;
+	return do_hw_read(codec, reg);
 }
 
 static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
 	u8 data[2];
-	int ret;
 
 	data[0] = (reg << 1) | ((value >> 8) & 0x0001);
 	data[1] = value & 0x00ff;
 
-	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size) {
-		ret = snd_soc_cache_write(codec, reg, value);
-		if (ret < 0)
-			return -1;
-	}
-
-	if (codec->cache_only) {
-		codec->cache_sync = 1;
-		return 0;
-	}
-
-	ret = codec->hw_write(codec->control_data, data, 2);
-	if (ret == 2)
-		return 0;
-	if (ret < 0)
-		return ret;
-	else
-		return -EIO;
+	return do_hw_write(codec, reg, value, data, 2);
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -184,50 +162,19 @@ static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
 	u8 data[2];
-	int ret;
 
 	reg &= 0xff;
 	data[0] = reg;
 	data[1] = value & 0xff;
 
-	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size) {
-		ret = snd_soc_cache_write(codec, reg, value);
-		if (ret < 0)
-			return -1;
-	}
-
-	if (codec->cache_only) {
-		codec->cache_sync = 1;
-		return 0;
-	}
-
-	if (codec->hw_write(codec->control_data, data, 2) == 2)
-		return 0;
-	else
-		return -EIO;
+	return do_hw_write(codec, reg, value, data, 2);
 }
 
 static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
 {
-	int ret;
-	unsigned int val;
-
 	reg &= 0xff;
-	if (reg >= codec->driver->reg_cache_size ||
-		snd_soc_codec_volatile_register(codec, reg)) {
-			if (codec->cache_only)
-				return -1;
-
-			BUG_ON(!codec->hw_read);
-			return codec->hw_read(codec, reg);
-	}
-
-	ret = snd_soc_cache_read(codec, reg, &val);
-	if (ret < 0)
-		return -1;
-	return val;
+	return do_hw_read(codec, reg);
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -264,49 +211,18 @@ static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg,
 			      unsigned int value)
 {
 	u8 data[3];
-	int ret;
 
 	data[0] = reg;
 	data[1] = (value >> 8) & 0xff;
 	data[2] = value & 0xff;
 
-	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size) {
-		ret = snd_soc_cache_write(codec, reg, value);
-		if (ret < 0)
-			return -1;
-	}
-
-	if (codec->cache_only) {
-		codec->cache_sync = 1;
-		return 0;
-	}
-
-	if (codec->hw_write(codec->control_data, data, 3) == 3)
-		return 0;
-	else
-		return -EIO;
+	return do_hw_write(codec, reg, value, data, 3);
 }
 
 static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec,
 				      unsigned int reg)
 {
-	int ret;
-	unsigned int val;
-
-	if (reg >= codec->driver->reg_cache_size ||
-	    snd_soc_codec_volatile_register(codec, reg)) {
-		if (codec->cache_only)
-			return -1;
-
-		BUG_ON(!codec->hw_read);
-		return codec->hw_read(codec, reg);
-	}
-
-	ret = snd_soc_cache_read(codec, reg, &val);
-	if (ret < 0)
-		return -1;
-	return val;
+	return do_hw_read(codec, reg);
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -445,55 +361,21 @@ static unsigned int snd_soc_16_8_read_i2c(struct snd_soc_codec *codec,
 static unsigned int snd_soc_16_8_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
 {
-	int ret;
-	unsigned int val;
-
 	reg &= 0xff;
-	if (reg >= codec->driver->reg_cache_size ||
-		snd_soc_codec_volatile_register(codec, reg)) {
-			if (codec->cache_only)
-				return -1;
-
-			BUG_ON(!codec->hw_read);
-			return codec->hw_read(codec, reg);
-	}
-
-	ret = snd_soc_cache_read(codec, reg, &val);
-	if (ret < 0)
-		return -1;
-	return val;
+	return do_hw_read(codec, reg);
 }
 
 static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
 	u8 data[3];
-	int ret;
 
 	data[0] = (reg >> 8) & 0xff;
 	data[1] = reg & 0xff;
 	data[2] = value;
 
 	reg &= 0xff;
-	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size) {
-		ret = snd_soc_cache_write(codec, reg, value);
-		if (ret < 0)
-			return -1;
-	}
-
-	if (codec->cache_only) {
-		codec->cache_sync = 1;
-		return 0;
-	}
-
-	ret = codec->hw_write(codec->control_data, data, 3);
-	if (ret == 3)
-		return 0;
-	if (ret < 0)
-		return ret;
-	else
-		return -EIO;
+	return do_hw_write(codec, reg, value, data, 3);
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -564,55 +446,20 @@ static unsigned int snd_soc_16_16_read_i2c(struct snd_soc_codec *codec,
 static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec,
 				       unsigned int reg)
 {
-	int ret;
-	unsigned int val;
-
-	if (reg >= codec->driver->reg_cache_size ||
-	    snd_soc_codec_volatile_register(codec, reg)) {
-		if (codec->cache_only)
-			return -1;
-
-		BUG_ON(!codec->hw_read);
-		return codec->hw_read(codec, reg);
-	}
-
-	ret = snd_soc_cache_read(codec, reg, &val);
-	if (ret < 0)
-		return -1;
-
-	return val;
+	return do_hw_read(codec, reg);
 }
 
 static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg,
 			       unsigned int value)
 {
 	u8 data[4];
-	int ret;
 
 	data[0] = (reg >> 8) & 0xff;
 	data[1] = reg & 0xff;
 	data[2] = (value >> 8) & 0xff;
 	data[3] = value & 0xff;
 
-	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size) {
-		ret = snd_soc_cache_write(codec, reg, value);
-		if (ret < 0)
-			return -1;
-	}
-
-	if (codec->cache_only) {
-		codec->cache_sync = 1;
-		return 0;
-	}
-
-	ret = codec->hw_write(codec->control_data, data, 4);
-	if (ret == 4)
-		return 0;
-	if (ret < 0)
-		return ret;
-	else
-		return -EIO;
+	return do_hw_write(codec, reg, value, data, 4);
 }
 
 #if defined(CONFIG_SPI_MASTER)
-- 
1.7.3.4

[PATCH 2/4] ASoC: clean up spi cache read/write functions

[Takashi Iwai]

Use a common helper function for spi write
Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-cache.c |  106 +++++++++++++------------------------------------
 1 files changed, 28 insertions(+), 78 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index 374b06a..5faad25 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -64,6 +64,28 @@ static unsigned int do_hw_read(struct snd_soc_codec *codec, unsigned int reg)
 	return val;
 }
 
+#ifdef CONFIG_SPI_MASTER
+static int do_spi_write(struct spi_device *spi, u8 *msg, int len)
+{
+	struct spi_transfer t;
+	struct spi_message m;
+
+	if (len <= 0)
+		return 0;
+
+	spi_message_init(&m);
+	memset(&t, 0, sizeof t);
+
+	t.tx_buf = &msg[0];
+	t.len = len;
+
+	spi_message_add_tail(&t, &m);
+	spi_sync(spi, &m);
+
+	return len;
+}
+#endif
+
 static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
 				      unsigned int reg)
 {
@@ -85,9 +107,6 @@ static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg,
 static int snd_soc_4_12_spi_write(void *control_data, const char *data,
 				 int len)
 {
-	struct spi_device *spi = control_data;
-	struct spi_transfer t;
-	struct spi_message m;
 	u8 msg[2];
 
 	if (len <= 0)
@@ -96,16 +115,7 @@ static int snd_soc_4_12_spi_write(void *control_data, const char *data,
 	msg[0] = data[1];
 	msg[1] = data[0];
 
-	spi_message_init(&m);
-	memset(&t, 0, sizeof t);
-
-	t.tx_buf = &msg[0];
-	t.len = len;
-
-	spi_message_add_tail(&t, &m);
-	spi_sync(spi, &m);
-
-	return len;
+	return do_spi_write(control_data, msg, len);
 }
 #else
 #define snd_soc_4_12_spi_write NULL
@@ -132,9 +142,6 @@ static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg,
 static int snd_soc_7_9_spi_write(void *control_data, const char *data,
 				 int len)
 {
-	struct spi_device *spi = control_data;
-	struct spi_transfer t;
-	struct spi_message m;
 	u8 msg[2];
 
 	if (len <= 0)
@@ -143,16 +150,7 @@ static int snd_soc_7_9_spi_write(void *control_data, const char *data,
 	msg[0] = data[0];
 	msg[1] = data[1];
 
-	spi_message_init(&m);
-	memset(&t, 0, sizeof t);
-
-	t.tx_buf = &msg[0];
-	t.len = len;
-
-	spi_message_add_tail(&t, &m);
-	spi_sync(spi, &m);
-
-	return len;
+	return do_spi_write(control_data, msg, len);
 }
 #else
 #define snd_soc_7_9_spi_write NULL
@@ -181,9 +179,6 @@ static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec,
 static int snd_soc_8_8_spi_write(void *control_data, const char *data,
 				 int len)
 {
-	struct spi_device *spi = control_data;
-	struct spi_transfer t;
-	struct spi_message m;
 	u8 msg[2];
 
 	if (len <= 0)
@@ -192,16 +187,7 @@ static int snd_soc_8_8_spi_write(void *control_data, const char *data,
 	msg[0] = data[0];
 	msg[1] = data[1];
 
-	spi_message_init(&m);
-	memset(&t, 0, sizeof t);
-
-	t.tx_buf = &msg[0];
-	t.len = len;
-
-	spi_message_add_tail(&t, &m);
-	spi_sync(spi, &m);
-
-	return len;
+	return do_spi_write(control_data, msg, len);
 }
 #else
 #define snd_soc_8_8_spi_write NULL
@@ -229,9 +215,6 @@ static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec,
 static int snd_soc_8_16_spi_write(void *control_data, const char *data,
 				 int len)
 {
-	struct spi_device *spi = control_data;
-	struct spi_transfer t;
-	struct spi_message m;
 	u8 msg[3];
 
 	if (len <= 0)
@@ -241,16 +224,7 @@ static int snd_soc_8_16_spi_write(void *control_data, const char *data,
 	msg[1] = data[1];
 	msg[2] = data[2];
 
-	spi_message_init(&m);
-	memset(&t, 0, sizeof t);
-
-	t.tx_buf = &msg[0];
-	t.len = len;
-
-	spi_message_add_tail(&t, &m);
-	spi_sync(spi, &m);
-
-	return len;
+	return do_spi_write(control_data, msg, len);
 }
 #else
 #define snd_soc_8_16_spi_write NULL
@@ -382,9 +356,6 @@ static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg,
 static int snd_soc_16_8_spi_write(void *control_data, const char *data,
 				 int len)
 {
-	struct spi_device *spi = control_data;
-	struct spi_transfer t;
-	struct spi_message m;
 	u8 msg[3];
 
 	if (len <= 0)
@@ -394,16 +365,7 @@ static int snd_soc_16_8_spi_write(void *control_data, const char *data,
 	msg[1] = data[1];
 	msg[2] = data[2];
 
-	spi_message_init(&m);
-	memset(&t, 0, sizeof t);
-
-	t.tx_buf = &msg[0];
-	t.len = len;
-
-	spi_message_add_tail(&t, &m);
-	spi_sync(spi, &m);
-
-	return len;
+	return do_spi_write(control_data, msg, len);
 }
 #else
 #define snd_soc_16_8_spi_write NULL
@@ -466,9 +428,6 @@ static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg,
 static int snd_soc_16_16_spi_write(void *control_data, const char *data,
 				 int len)
 {
-	struct spi_device *spi = control_data;
-	struct spi_transfer t;
-	struct spi_message m;
 	u8 msg[4];
 
 	if (len <= 0)
@@ -479,16 +438,7 @@ static int snd_soc_16_16_spi_write(void *control_data, const char *data,
 	msg[2] = data[2];
 	msg[3] = data[3];
 
-	spi_message_init(&m);
-	memset(&t, 0, sizeof t);
-
-	t.tx_buf = &msg[0];
-	t.len = len;
-
-	spi_message_add_tail(&t, &m);
-	spi_sync(spi, &m);
-
-	return len;
+	return do_spi_write(control_data, msg, len);
 }
 #else
 #define snd_soc_16_16_spi_write NULL
-- 
1.7.3.4

[PATCH 3/4] ASoC: clean up i2c cache read / write functions

[Takashi Iwai]

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
 sound/soc/soc-cache.c |  130 +++++++++++++++++--------------------------------
 1 files changed, 44 insertions(+), 86 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index 5faad25..a3d448d 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -18,6 +18,10 @@
 #include <linux/bitmap.h>
 #include <linux/rbtree.h>
 
+#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+#define HAVE_I2C_OPS
+#endif
+
 static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg,
 		       unsigned int value, void *data, unsigned int size)
 {
@@ -86,6 +90,36 @@ static int do_spi_write(struct spi_device *spi, u8 *msg, int len)
 }
 #endif
 
+#ifdef HAVE_I2C_OPS
+static unsigned int do_i2c_read(struct snd_soc_codec *codec,
+				int reglen, void *regp,
+				int datalen, void *datap)
+{
+	struct i2c_msg xfer[2];
+	int ret;
+	struct i2c_client *client = codec->control_data;
+
+	/* Write register */
+	xfer[0].addr = client->addr;
+	xfer[0].flags = 0;
+	xfer[0].len = reglen;
+	xfer[0].buf = regp;
+
+	/* Read data */
+	xfer[1].addr = client->addr;
+	xfer[1].flags = I2C_M_RD;
+	xfer[1].len = datalen;
+	xfer[1].buf = datap;
+
+	ret = i2c_transfer(client->adapter, xfer, 2);
+	if (ret != 2) {
+		dev_err(&client->dev, "i2c_transfer() returned %d\n", ret);
+		return -EIO;
+	}
+	return 0;
+}
+#endif
+
 static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
 				      unsigned int reg)
 {
@@ -230,103 +264,46 @@ static int snd_soc_8_16_spi_write(void *control_data, const char *data,
 #define snd_soc_8_16_spi_write NULL
 #endif
 
-#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+#ifdef HAVE_I2C_OPS
 static unsigned int snd_soc_8_8_read_i2c(struct snd_soc_codec *codec,
 					  unsigned int r)
 {
-	struct i2c_msg xfer[2];
 	u8 reg = r;
 	u8 data;
-	int ret;
-	struct i2c_client *client = codec->control_data;
-
-	/* Write register */
-	xfer[0].addr = client->addr;
-	xfer[0].flags = 0;
-	xfer[0].len = 1;
-	xfer[0].buf = &reg;
-
-	/* Read data */
-	xfer[1].addr = client->addr;
-	xfer[1].flags = I2C_M_RD;
-	xfer[1].len = 1;
-	xfer[1].buf = &data;
 
-	ret = i2c_transfer(client->adapter, xfer, 2);
-	if (ret != 2) {
-		dev_err(&client->dev, "i2c_transfer() returned %d\n", ret);
+	if (do_i2c_read(codec, 1, &reg, 1, &data))
 		return 0;
-	}
-
 	return data;
 }
 #else
 #define snd_soc_8_8_read_i2c NULL
 #endif
 
-#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+#ifdef HAVE_I2C_OPS
 static unsigned int snd_soc_8_16_read_i2c(struct snd_soc_codec *codec,
 					  unsigned int r)
 {
-	struct i2c_msg xfer[2];
 	u8 reg = r;
 	u16 data;
-	int ret;
-	struct i2c_client *client = codec->control_data;
-
-	/* Write register */
-	xfer[0].addr = client->addr;
-	xfer[0].flags = 0;
-	xfer[0].len = 1;
-	xfer[0].buf = &reg;
-
-	/* Read data */
-	xfer[1].addr = client->addr;
-	xfer[1].flags = I2C_M_RD;
-	xfer[1].len = 2;
-	xfer[1].buf = (u8 *)&data;
 
-	ret = i2c_transfer(client->adapter, xfer, 2);
-	if (ret != 2) {
-		dev_err(&client->dev, "i2c_transfer() returned %d\n", ret);
+	if (do_i2c_read(codec, 1, &reg, 2, &data))
 		return 0;
-	}
-
 	return (data >> 8) | ((data & 0xff) << 8);
 }
 #else
 #define snd_soc_8_16_read_i2c NULL
 #endif
 
-#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+#ifdef HAVE_I2C_OPS
 static unsigned int snd_soc_16_8_read_i2c(struct snd_soc_codec *codec,
 					  unsigned int r)
 {
-	struct i2c_msg xfer[2];
 	u16 reg = r;
 	u8 data;
-	int ret;
-	struct i2c_client *client = codec->control_data;
-
-	/* Write register */
-	xfer[0].addr = client->addr;
-	xfer[0].flags = 0;
-	xfer[0].len = 2;
-	xfer[0].buf = (u8 *)&reg;
-
-	/* Read data */
-	xfer[1].addr = client->addr;
-	xfer[1].flags = I2C_M_RD;
-	xfer[1].len = 1;
-	xfer[1].buf = &data;
 
-	ret = i2c_transfer(client->adapter, xfer, 2);
-	if (ret != 2) {
-		dev_err(&client->dev, "i2c_transfer() returned %d\n", ret);
+	if (do_i2c_read(codec, 2, &reg, 1, &data))
 		return 0;
-	}
-
-	return data;
+	return (data >> 8) | ((data & 0xff) << 8);
 }
 #else
 #define snd_soc_16_8_read_i2c NULL
@@ -371,34 +348,15 @@ static int snd_soc_16_8_spi_write(void *control_data, const char *data,
 #define snd_soc_16_8_spi_write NULL
 #endif
 
-#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+#ifdef HAVE_I2C_OPS
 static unsigned int snd_soc_16_16_read_i2c(struct snd_soc_codec *codec,
 					   unsigned int r)
 {
-	struct i2c_msg xfer[2];
 	u16 reg = cpu_to_be16(r);
 	u16 data;
-	int ret;
-	struct i2c_client *client = codec->control_data;
-
-	/* Write register */
-	xfer[0].addr = client->addr;
-	xfer[0].flags = 0;
-	xfer[0].len = 2;
-	xfer[0].buf = (u8 *)&reg;
-
-	/* Read data */
-	xfer[1].addr = client->addr;
-	xfer[1].flags = I2C_M_RD;
-	xfer[1].len = 2;
-	xfer[1].buf = (u8 *)&data;
 
-	ret = i2c_transfer(client->adapter, xfer, 2);
-	if (ret != 2) {
-		dev_err(&client->dev, "i2c_transfer() returned %d\n", ret);
+	if (do_i2c_read(codec, 2, &reg, 2, &data))
 		return 0;
-	}
-
 	return be16_to_cpu(data);
 }
 #else
@@ -533,7 +491,7 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 		break;
 
 	case SND_SOC_I2C:
-#if defined(CONFIG_I2C) || (defined(CONFIG_I2C_MODULE) && defined(MODULE))
+#ifdef HAVE_I2C_OPS
 		codec->hw_write = (hw_write_t)i2c_master_send;
 #endif
 		if (io_types[i].i2c_read)
-- 
1.7.3.4

[PATCH 4/4] ASoC: clean up cache accesser

[Takashi Iwai]

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---

Erm, subject might be wrong, but I'm too dizzy now.

And this kind of change isn't always optimal indeed.  But, it improves
the readability, and often readability wins over a few byte
performance win.  Moreover, it makes easier to change word_size
extension to 3 or 4 (if any in future).


 sound/soc/soc-cache.c |  213 +++++++++++++++----------------------------------
 1 files changed, 65 insertions(+), 148 deletions(-)
f
diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index a3d448d..1e721d5 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -516,6 +516,35 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 }
 EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);
 
+static inline bool set_cache_val(void *base, unsigned int idx,
+				unsigned int val, unsigned int word_size)
+{
+	if (word_size == 1) {
+		u8 *cache = base;
+		if (cache[idx] == val)
+			return true;
+		cache[idx] = val;
+	} else {
+		u16 *cache = base;
+		if (cache[idx] == val)
+			return true;
+		cache[idx] = val;
+	}
+	return false;
+}
+
+static inline unsigned int get_cache_val(const void *base, unsigned int idx,
+					 unsigned int word_size)
+{
+	if (word_size == 1) {
+		const u8 *cache = base;
+		return cache[idx];
+	} else {
+		const u16 *cache = base;
+		return cache[idx];
+	}
+}
+
 struct snd_soc_rbtree_node {
 	struct rb_node node;
 	unsigned int reg;
@@ -680,7 +709,9 @@ static int snd_soc_rbtree_cache_exit(struct snd_soc_codec *codec)
 static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec)
 {
 	struct snd_soc_rbtree_ctx *rbtree_ctx;
-
+	int i, word_size;
+	struct snd_soc_rbtree_node *rbtree_node;
+									\
 	codec->reg_cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
 	if (!codec->reg_cache)
 		return -ENOMEM;
@@ -691,55 +722,27 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec)
 	if (!codec->reg_def_copy)
 		return 0;
 
-/*
- * populate the rbtree with the initialized registers.  All other
- * registers will be inserted into the tree when they are first written.
- *
- * The reasoning behind this, is that we need to step through and
- * dereference the cache in u8/u16 increments without sacrificing
- * portability.  This could also be done using memcpy() but that would
- * be slightly more cryptic.
- */
-#define snd_soc_rbtree_populate(cache)					\
-({									\
-	int ret, i;							\
-	struct snd_soc_rbtree_node *rbtree_node;			\
-									\
-	ret = 0;							\
-	cache = codec->reg_def_copy;					\
-	for (i = 0; i < codec->driver->reg_cache_size; ++i) {		\
-		if (!cache[i])						\
-			continue;					\
-		rbtree_node = kzalloc(sizeof *rbtree_node, GFP_KERNEL);	\
-		if (!rbtree_node) {					\
-			ret = -ENOMEM;					\
-			snd_soc_cache_exit(codec);			\
-			break;						\
-		}							\
-		rbtree_node->reg = i;					\
-		rbtree_node->value = cache[i];				\
-		rbtree_node->defval = cache[i];				\
-		snd_soc_rbtree_insert(&rbtree_ctx->root,		\
-				      rbtree_node);			\
-	}								\
-	ret;								\
-})
-
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		const u8 *cache;
-
-		return snd_soc_rbtree_populate(cache);
-	}
-	case 2: {
-		const u16 *cache;
-
-		return snd_soc_rbtree_populate(cache);
-	}
-	default:
-		BUG();
+	/*
+	 * populate the rbtree with the initialized registers.  All other
+	 * registers will be inserted into the tree when they are first written.
+	 */
+	word_size = codec->driver->reg_word_size;
+	for (i = 0; i < codec->driver->reg_cache_size; ++i) {
+		unsigned int val;
+		val = get_cache_val(codec->reg_def_copy, i, word_size);
+		if (!val)
+			continue;
+		rbtree_node = kzalloc(sizeof(*rbtree_node), GFP_KERNEL);
+		if (!rbtree_node) {
+			snd_soc_cache_exit(codec);
+			return -ENOMEM;
+			break;
+		}
+		rbtree_node->reg = i;
+		rbtree_node->value = val;
+		rbtree_node->defval = val;
+		snd_soc_rbtree_insert(&rbtree_ctx->root, rbtree_node);
 	}
-
 	return 0;
 }
 
@@ -919,29 +922,10 @@ static int snd_soc_lzo_cache_write(struct snd_soc_codec *codec,
 	}
 
 	/* write the new value to the cache */
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		u8 *cache;
-		cache = lzo_block->dst;
-		if (cache[blkpos] == value) {
-			kfree(lzo_block->dst);
-			goto out;
-		}
-		cache[blkpos] = value;
-	}
-	break;
-	case 2: {
-		u16 *cache;
-		cache = lzo_block->dst;
-		if (cache[blkpos] == value) {
-			kfree(lzo_block->dst);
-			goto out;
-		}
-		cache[blkpos] = value;
-	}
-	break;
-	default:
-		BUG();
+	if (set_cache_val(lzo_block->dst, blkpos, value,
+			  codec->driver->reg_word_size)) {
+		kfree(lzo_block->dst);
+		goto out;
 	}
 
 	/* prepare the source to be the decompressed block */
@@ -995,25 +979,9 @@ static int snd_soc_lzo_cache_read(struct snd_soc_codec *codec,
 
 	/* decompress the block */
 	ret = snd_soc_lzo_decompress_cache_block(codec, lzo_block);
-	if (ret >= 0) {
-		/* fetch the value from the cache */
-		switch (codec->driver->reg_word_size) {
-		case 1: {
-			u8 *cache;
-			cache = lzo_block->dst;
-			*value = cache[blkpos];
-		}
-		break;
-		case 2: {
-			u16 *cache;
-			cache = lzo_block->dst;
-			*value = cache[blkpos];
-		}
-		break;
-		default:
-			BUG();
-		}
-	}
+	if (ret >= 0)
+		*value = get_cache_val(lzo_block->dst, blkpos,
+				       codec->driver->reg_word_size);
 
 	kfree(lzo_block->dst);
 	/* restore the pointer and length of the compressed block */
@@ -1168,26 +1136,9 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec)
 		if (ret)
 			return ret;
 		if (codec_drv->reg_cache_default) {
-			switch (codec_drv->reg_word_size) {
-			case 1: {
-				const u8 *cache;
-
-				cache = codec_drv->reg_cache_default;
-				if (cache[i] == val)
-					continue;
-			}
-			break;
-			case 2: {
-				const u16 *cache;
-
-				cache = codec_drv->reg_cache_default;
-				if (cache[i] == val)
+			if (get_cache_val(codec_drv->reg_cache_default, i,
+					  codec_drv->reg_word_size) == val)
 					continue;
-			}
-			break;
-			default:
-				BUG();
-			}
 		}
 		ret = snd_soc_write(codec, i, val);
 		if (ret)
@@ -1201,50 +1152,16 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec)
 static int snd_soc_flat_cache_write(struct snd_soc_codec *codec,
 				    unsigned int reg, unsigned int value)
 {
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		u8 *cache;
-
-		cache = codec->reg_cache;
-		cache[reg] = value;
-	}
-	break;
-	case 2: {
-		u16 *cache;
-
-		cache = codec->reg_cache;
-		cache[reg] = value;
-	}
-	break;
-	default:
-		BUG();
-	}
-
+	set_cache_val(codec->reg_cache, reg, value,
+		      codec->driver->reg_word_size);
 	return 0;
 }
 
 static int snd_soc_flat_cache_read(struct snd_soc_codec *codec,
 				   unsigned int reg, unsigned int *value)
 {
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		u8 *cache;
-
-		cache = codec->reg_cache;
-		*value = cache[reg];
-	}
-	break;
-	case 2: {
-		u16 *cache;
-
-		cache = codec->reg_cache;
-		*value = cache[reg];
-	}
-	break;
-	default:
-		BUG();
-	}
-
+	*value = get_cache_val(codec->reg_cache, reg,
+			       codec->driver->reg_word_size);
 	return 0;
 }
 
-- 
1.7.3.4

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Dimitris Papastamos]

On Mon, 2010-12-20 at 17:05 +0100, Takashi Iwai wrote:
> +static inline bool set_cache_val(void *base, unsigned int idx,
> +				unsigned int val, unsigned int word_size)
> +{
> +	if (word_size == 1) {
> +		u8 *cache = base;
> +		if (cache[idx] == val)
> +			return true;
> +		cache[idx] = val;
> +	} else {
> +		u16 *cache = base;
> +		if (cache[idx] == val)
> +			return true;
> +		cache[idx] = val;
> +	}
> +	return false;
> +}

If word_size is anything other than 1 byte, the above else will try to
handle it and assume it is 16 bits.  I'd expect for an explicit check
for word_size == 2.  A switch statement would perhaps be preferred for
legibility.  It'd perhaps be wise to simply die via BUG() or similar if
an unsupported word size was passed in.

> +static inline unsigned int get_cache_val(const void *base, unsigned int idx,
> +					 unsigned int word_size)
> +{
> +	if (word_size == 1) {
> +		const u8 *cache = base;
> +		return cache[idx];
> +	} else {
> +		const u16 *cache = base;
> +		return cache[idx];
> +	}
> +}

Same here.

The rest looks good.

Thanks,
Dimitrios

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Takashi Iwai]

At Mon, 20 Dec 2010 16:27:49 +0000,
Dimitris Papastamos wrote:
> 
> On Mon, 2010-12-20 at 17:05 +0100, Takashi Iwai wrote:
> > +static inline bool set_cache_val(void *base, unsigned int idx,
> > +				unsigned int val, unsigned int word_size)
> > +{
> > +	if (word_size == 1) {
> > +		u8 *cache = base;
> > +		if (cache[idx] == val)
> > +			return true;
> > +		cache[idx] = val;
> > +	} else {
> > +		u16 *cache = base;
> > +		if (cache[idx] == val)
> > +			return true;
> > +		cache[idx] = val;
> > +	}
> > +	return false;
> > +}
> 
> If word_size is anything other than 1 byte, the above else will try to
> handle it and assume it is 16 bits.  I'd expect for an explicit check
> for word_size == 2.  A switch statement would perhaps be preferred for
> legibility.  It'd perhaps be wise to simply die via BUG() or similar if
> an unsupported word size was passed in.

Yes, this would be safer.  I didn't put it since I wasn't sure whether
BUG() content is also expanded at each caller.  If yes, it would
bloat.   (Alternatively we may use snd_BUG_ON() -- it's built in only
when CONFIG_SND_DEBUG is set.)

Anyway, such a check should have been done rather at initialization.


thanks,

Takashi

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Mark Brown]

On Mon, Dec 20, 2010 at 05:47:13PM +0100, Takashi Iwai wrote:

> Yes, this would be safer.  I didn't put it since I wasn't sure whether
> BUG() content is also expanded at each caller.  If yes, it would
> bloat.   (Alternatively we may use snd_BUG_ON() -- it's built in only
> when CONFIG_SND_DEBUG is set.)

That's entirely up to the compiler - inline is purely advice and may
well be completely ignored by the compile (and of course functions that 
aren't marked inline may be inlined if it decides that's useful).

I'd be inclined to just not mark the functions inline.

> Anyway, such a check should have been done rather at initialization.

One reason for making it a BUG() is that if we get far enough for the
check to go off when doing actual register lookups we've hit a definite
bug in the code.

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Dimitris Papastamos]

On Mon, 2010-12-20 at 16:54 +0000, Mark Brown wrote:
> On Mon, Dec 20, 2010 at 05:47:13PM +0100, Takashi Iwai wrote:
> 
> > Yes, this would be safer.  I didn't put it since I wasn't sure whether
> > BUG() content is also expanded at each caller.  If yes, it would
> > bloat.   (Alternatively we may use snd_BUG_ON() -- it's built in only
> > when CONFIG_SND_DEBUG is set.)
> 
> That's entirely up to the compiler - inline is purely advice and may
> well be completely ignored by the compile (and of course functions that 
> aren't marked inline may be inlined if it decides that's useful).

One way to enforce the expansion of inline functions on gcc is to use
__attribute__ ((always_inline)).  Generally it is best left up to the
compiler to perform the inling if it so deems necessary.

> One reason for making it a BUG() is that if we get far enough for the
> check to go off when doing actual register lookups we've hit a definite
> bug in the code.

By performing the check at *_init, it will be impossible for the code to
ever reach any of the read()/write() functions if an unsupported word
size has been passed in.  The downside is that we depend on the
implementers of the *_init functions to perform this check.

Thanks,
Dimitrios

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Takashi Iwai]

At Mon, 20 Dec 2010 17:09:00 +0000,
Dimitris Papastamos wrote:
> 
> On Mon, 2010-12-20 at 16:54 +0000, Mark Brown wrote:
> > On Mon, Dec 20, 2010 at 05:47:13PM +0100, Takashi Iwai wrote:
> > 
> > > Yes, this would be safer.  I didn't put it since I wasn't sure whether
> > > BUG() content is also expanded at each caller.  If yes, it would
> > > bloat.   (Alternatively we may use snd_BUG_ON() -- it's built in only
> > > when CONFIG_SND_DEBUG is set.)
> > 
> > That's entirely up to the compiler - inline is purely advice and may
> > well be completely ignored by the compile (and of course functions that 
> > aren't marked inline may be inlined if it decides that's useful).
> 
> One way to enforce the expansion of inline functions on gcc is to use
> __attribute__ ((always_inline)).  Generally it is best left up to the
> compiler to perform the inling if it so deems necessary.

Right.  I removed inline in the revised patch.


Takashi

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Dimitris Papastamos]

On Mon, 2010-12-20 at 17:47 +0100, Takashi Iwai wrote:
> At Mon, 20 Dec 2010 16:27:49 +0000,
> Dimitris Papastamos wrote:
> > 
> > On Mon, 2010-12-20 at 17:05 +0100, Takashi Iwai wrote:
> > > +static inline bool set_cache_val(void *base, unsigned int idx,
> > > +				unsigned int val, unsigned int word_size)
> > > +{
> > > +	if (word_size == 1) {
> > > +		u8 *cache = base;
> > > +		if (cache[idx] == val)
> > > +			return true;
> > > +		cache[idx] = val;
> > > +	} else {
> > > +		u16 *cache = base;
> > > +		if (cache[idx] == val)
> > > +			return true;
> > > +		cache[idx] = val;
> > > +	}
> > > +	return false;
> > > +}
> > 
> > If word_size is anything other than 1 byte, the above else will try to
> > handle it and assume it is 16 bits.  I'd expect for an explicit check
> > for word_size == 2.  A switch statement would perhaps be preferred for
> > legibility.  It'd perhaps be wise to simply die via BUG() or similar if
> > an unsupported word size was passed in.
> 
> Yes, this would be safer.  I didn't put it since I wasn't sure whether
> BUG() content is also expanded at each caller.  If yes, it would
> bloat.   (Alternatively we may use snd_BUG_ON() -- it's built in only
> when CONFIG_SND_DEBUG is set.)
> 
> Anyway, such a check should have been done rather at initialization.

I'd expect macro expansion to happen before inline function expansion.

I'll send in a patch to perform this check at init time once yours has
been merged.

Thanks,
Dimitrios

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Mark Brown]

On Mon, Dec 20, 2010 at 05:05:51PM +0100, Takashi Iwai wrote:

> Erm, subject might be wrong, but I'm too dizzy now.

I think you just mean "Factor out cache accesses" or something?

> +static inline bool set_cache_val(void *base, unsigned int idx,
> +				unsigned int val, unsigned int word_size)
> +{
> +	if (word_size == 1) {
> +		u8 *cache = base;

Like Dimitris says these really do need to be switch statements.
Otherwise it looks OK.

Re: [PATCH 4/4] ASoC: clean up cache accesser

[Takashi Iwai]

At Mon, 20 Dec 2010 16:42:02 +0000,
Mark Brown wrote:
> 
> On Mon, Dec 20, 2010 at 05:05:51PM +0100, Takashi Iwai wrote:
> 
> > Erm, subject might be wrong, but I'm too dizzy now.
> 
> I think you just mean "Factor out cache accesses" or something?

Yes, that sounds better.  Also, no proper wording came to my mind
about the stuff this patch touched.  I should have wrote it as
cache_ops or so.

> > +static inline bool set_cache_val(void *base, unsigned int idx,
> > +				unsigned int val, unsigned int word_size)
> > +{
> > +	if (word_size == 1) {
> > +		u8 *cache = base;
> 
> Like Dimitris says these really do need to be switch statements.
> Otherwise it looks OK.

OK.  Will resend shortly later.


thanks,

Takashi

[PATCH 4/4] ASoC: Factor out cache_ops implementations

[Takashi Iwai]

Use common helper functions to access cache contents for clean up
the open codes here and there.

Signed-off-by: Takashi Iwai <tiwai@suse.de>
---
v1->v2: change subject & changelog; add BUG() to invalid word_size cases

 sound/soc/soc-cache.c |  226 +++++++++++++++++--------------------------------
 1 files changed, 78 insertions(+), 148 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index a3d448d..aa45161 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -516,6 +516,48 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 }
 EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);
 
+static bool set_cache_val(void *base, unsigned int idx, unsigned int val,
+			  unsigned int word_size)
+{
+	switch (word_size) {
+	case 1: {
+		u8 *cache = base;
+		if (cache[idx] == val)
+			return true;
+		cache[idx] = val;
+		break;
+	}
+	case 2: {
+		u16 *cache = base;
+		if (cache[idx] == val)
+			return true;
+		cache[idx] = val;
+		break;
+	}
+	default:
+		BUG();
+	}
+	return false;
+}
+
+static unsigned int get_cache_val(const void *base, unsigned int idx,
+				  unsigned int word_size)
+{
+	switch (word_size) {
+	case 1: {
+		const u8 *cache = base;
+		return cache[idx];
+	}
+	case 2: {
+		const u16 *cache = base;
+		return cache[idx];
+	}
+	default:
+		BUG();
+		return -1;
+	}
+}
+
 struct snd_soc_rbtree_node {
 	struct rb_node node;
 	unsigned int reg;
@@ -680,7 +722,9 @@ static int snd_soc_rbtree_cache_exit(struct snd_soc_codec *codec)
 static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec)
 {
 	struct snd_soc_rbtree_ctx *rbtree_ctx;
-
+	int i, word_size;
+	struct snd_soc_rbtree_node *rbtree_node;
+									\
 	codec->reg_cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL);
 	if (!codec->reg_cache)
 		return -ENOMEM;
@@ -691,55 +735,27 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec)
 	if (!codec->reg_def_copy)
 		return 0;
 
-/*
- * populate the rbtree with the initialized registers.  All other
- * registers will be inserted into the tree when they are first written.
- *
- * The reasoning behind this, is that we need to step through and
- * dereference the cache in u8/u16 increments without sacrificing
- * portability.  This could also be done using memcpy() but that would
- * be slightly more cryptic.
- */
-#define snd_soc_rbtree_populate(cache)					\
-({									\
-	int ret, i;							\
-	struct snd_soc_rbtree_node *rbtree_node;			\
-									\
-	ret = 0;							\
-	cache = codec->reg_def_copy;					\
-	for (i = 0; i < codec->driver->reg_cache_size; ++i) {		\
-		if (!cache[i])						\
-			continue;					\
-		rbtree_node = kzalloc(sizeof *rbtree_node, GFP_KERNEL);	\
-		if (!rbtree_node) {					\
-			ret = -ENOMEM;					\
-			snd_soc_cache_exit(codec);			\
-			break;						\
-		}							\
-		rbtree_node->reg = i;					\
-		rbtree_node->value = cache[i];				\
-		rbtree_node->defval = cache[i];				\
-		snd_soc_rbtree_insert(&rbtree_ctx->root,		\
-				      rbtree_node);			\
-	}								\
-	ret;								\
-})
-
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		const u8 *cache;
-
-		return snd_soc_rbtree_populate(cache);
-	}
-	case 2: {
-		const u16 *cache;
-
-		return snd_soc_rbtree_populate(cache);
-	}
-	default:
-		BUG();
+	/*
+	 * populate the rbtree with the initialized registers.  All other
+	 * registers will be inserted into the tree when they are first written.
+	 */
+	word_size = codec->driver->reg_word_size;
+	for (i = 0; i < codec->driver->reg_cache_size; ++i) {
+		unsigned int val;
+		val = get_cache_val(codec->reg_def_copy, i, word_size);
+		if (!val)
+			continue;
+		rbtree_node = kzalloc(sizeof(*rbtree_node), GFP_KERNEL);
+		if (!rbtree_node) {
+			snd_soc_cache_exit(codec);
+			return -ENOMEM;
+			break;
+		}
+		rbtree_node->reg = i;
+		rbtree_node->value = val;
+		rbtree_node->defval = val;
+		snd_soc_rbtree_insert(&rbtree_ctx->root, rbtree_node);
 	}
-
 	return 0;
 }
 
@@ -919,29 +935,10 @@ static int snd_soc_lzo_cache_write(struct snd_soc_codec *codec,
 	}
 
 	/* write the new value to the cache */
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		u8 *cache;
-		cache = lzo_block->dst;
-		if (cache[blkpos] == value) {
-			kfree(lzo_block->dst);
-			goto out;
-		}
-		cache[blkpos] = value;
-	}
-	break;
-	case 2: {
-		u16 *cache;
-		cache = lzo_block->dst;
-		if (cache[blkpos] == value) {
-			kfree(lzo_block->dst);
-			goto out;
-		}
-		cache[blkpos] = value;
-	}
-	break;
-	default:
-		BUG();
+	if (set_cache_val(lzo_block->dst, blkpos, value,
+			  codec->driver->reg_word_size)) {
+		kfree(lzo_block->dst);
+		goto out;
 	}
 
 	/* prepare the source to be the decompressed block */
@@ -995,25 +992,9 @@ static int snd_soc_lzo_cache_read(struct snd_soc_codec *codec,
 
 	/* decompress the block */
 	ret = snd_soc_lzo_decompress_cache_block(codec, lzo_block);
-	if (ret >= 0) {
-		/* fetch the value from the cache */
-		switch (codec->driver->reg_word_size) {
-		case 1: {
-			u8 *cache;
-			cache = lzo_block->dst;
-			*value = cache[blkpos];
-		}
-		break;
-		case 2: {
-			u16 *cache;
-			cache = lzo_block->dst;
-			*value = cache[blkpos];
-		}
-		break;
-		default:
-			BUG();
-		}
-	}
+	if (ret >= 0)
+		*value = get_cache_val(lzo_block->dst, blkpos,
+				       codec->driver->reg_word_size);
 
 	kfree(lzo_block->dst);
 	/* restore the pointer and length of the compressed block */
@@ -1168,26 +1149,9 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec)
 		if (ret)
 			return ret;
 		if (codec_drv->reg_cache_default) {
-			switch (codec_drv->reg_word_size) {
-			case 1: {
-				const u8 *cache;
-
-				cache = codec_drv->reg_cache_default;
-				if (cache[i] == val)
-					continue;
-			}
-			break;
-			case 2: {
-				const u16 *cache;
-
-				cache = codec_drv->reg_cache_default;
-				if (cache[i] == val)
+			if (get_cache_val(codec_drv->reg_cache_default, i,
+					  codec_drv->reg_word_size) == val)
 					continue;
-			}
-			break;
-			default:
-				BUG();
-			}
 		}
 		ret = snd_soc_write(codec, i, val);
 		if (ret)
@@ -1201,50 +1165,16 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec)
 static int snd_soc_flat_cache_write(struct snd_soc_codec *codec,
 				    unsigned int reg, unsigned int value)
 {
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		u8 *cache;
-
-		cache = codec->reg_cache;
-		cache[reg] = value;
-	}
-	break;
-	case 2: {
-		u16 *cache;
-
-		cache = codec->reg_cache;
-		cache[reg] = value;
-	}
-	break;
-	default:
-		BUG();
-	}
-
+	set_cache_val(codec->reg_cache, reg, value,
+		      codec->driver->reg_word_size);
 	return 0;
 }
 
 static int snd_soc_flat_cache_read(struct snd_soc_codec *codec,
 				   unsigned int reg, unsigned int *value)
 {
-	switch (codec->driver->reg_word_size) {
-	case 1: {
-		u8 *cache;
-
-		cache = codec->reg_cache;
-		*value = cache[reg];
-	}
-	break;
-	case 2: {
-		u16 *cache;
-
-		cache = codec->reg_cache;
-		*value = cache[reg];
-	}
-	break;
-	default:
-		BUG();
-	}
-
+	*value = get_cache_val(codec->reg_cache, reg,
+			       codec->driver->reg_word_size);
 	return 0;
 }
 
-- 
1.7.3.4

Re: [PATCH 2/4] ASoC: clean up spi cache read/write functions

[Mark Brown]

On Mon, Dec 20, 2010 at 05:01:53PM +0100, Takashi Iwai wrote:

> +#ifdef CONFIG_SPI_MASTER
> +static int do_spi_write(struct spi_device *spi, u8 *msg, int len)
> +{

This looks a lot like the standard spi_write() function now.  Which we
probably should've been using anyway but hey ho...

Re: [PATCH 1/4] ASoC: clean up cache read/write functions

[Dimitris Papastamos]

On Mon, 2010-12-20 at 17:01 +0100, Takashi Iwai wrote:
> Use common helper functions for standard read/write functions of each
> data type.
> 
> Signed-off-by: Takashi Iwai <tiwai@suse.de>
> ---
>  sound/soc/soc-cache.c |  251 ++++++++++---------------------------------------
>  1 files changed, 49 insertions(+), 202 deletions(-)
> 
> diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
> index 0e17b40..374b06a 100644
> --- a/sound/soc/soc-cache.c
> +++ b/sound/soc/soc-cache.c
> @@ -18,19 +18,44 @@
>  #include <linux/bitmap.h>
>  #include <linux/rbtree.h>
>  
> -static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
> -				     unsigned int reg)
> +static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg,
> +		       unsigned int value, void *data, unsigned int size)
> +{
> +	int ret;
> +
> +	if (!snd_soc_codec_volatile_register(codec, reg) &&
> +	    reg < codec->driver->reg_cache_size) {
> +		ret = snd_soc_cache_write(codec, reg, value);
> +		if (ret < 0)
> +			return -1;
> +	}
> +
> +	if (codec->cache_only) {
> +		codec->cache_sync = 1;
> +		return 0;
> +	}
> +
> +	ret = codec->hw_write(codec->control_data, data, size);
> +	if (ret == size)
> +		return 0;
> +	if (ret < 0)
> +		return ret;
> +	else
> +		return -EIO;
> +}
> +
> +static unsigned int do_hw_read(struct snd_soc_codec *codec, unsigned int reg)
>  {
>  	int ret;
>  	unsigned int val;
>  
>  	if (reg >= codec->driver->reg_cache_size ||
> -		snd_soc_codec_volatile_register(codec, reg)) {
> -			if (codec->cache_only)
> -				return -1;
> +	    snd_soc_codec_volatile_register(codec, reg)) {
> +		if (codec->cache_only)
> +			return -1;
>  
> -			BUG_ON(!codec->hw_read);
> -			return codec->hw_read(codec, reg);
> +		BUG_ON(!codec->hw_read);
> +		return codec->hw_read(codec, reg);
>  	}
>  
>  	ret = snd_soc_cache_read(codec, reg, &val);
> @@ -39,34 +64,21 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
>  	return val;
>  }
>  
> +static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
> +				      unsigned int reg)
> +{
> +	return do_hw_read(codec, reg);
> +}
> +
>  static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg,
>  			     unsigned int value)
>  {
>  	u8 data[2];
> -	int ret;
>  
>  	data[0] = (reg << 4) | ((value >> 8) & 0x000f);
>  	data[1] = value & 0x00ff;
>  
> -	if (!snd_soc_codec_volatile_register(codec, reg) &&
> -		reg < codec->driver->reg_cache_size) {
> -		ret = snd_soc_cache_write(codec, reg, value);
> -		if (ret < 0)
> -			return -1;
> -	}
> -
> -	if (codec->cache_only) {
> -		codec->cache_sync = 1;
> -		return 0;
> -	}
> -
> -	ret = codec->hw_write(codec->control_data, data, 2);
> -	if (ret == 2)
> -		return 0;
> -	if (ret < 0)
> -		return ret;
> -	else
> -		return -EIO;
> +	return do_hw_write(codec, reg, value, data, 2);
>  }
>  
>  #if defined(CONFIG_SPI_MASTER)
> @@ -102,52 +114,18 @@ static int snd_soc_4_12_spi_write(void *control_data, const char *data,
>  static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec,
>  				     unsigned int reg)
>  {
> -	int ret;
> -	unsigned int val;
> -
> -	if (reg >= codec->driver->reg_cache_size ||
> -		snd_soc_codec_volatile_register(codec, reg)) {
> -			if (codec->cache_only)
> -				return -1;
> -
> -			BUG_ON(!codec->hw_read);
> -			return codec->hw_read(codec, reg);
> -	}
> -
> -	ret = snd_soc_cache_read(codec, reg, &val);
> -	if (ret < 0)
> -		return -1;
> -	return val;
> +	return do_hw_read(codec, reg);
>  }
>  
>  static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg,
>  			     unsigned int value)
>  {
>  	u8 data[2];
> -	int ret;
>  
>  	data[0] = (reg << 1) | ((value >> 8) & 0x0001);
>  	data[1] = value & 0x00ff;
>  
> -	if (!snd_soc_codec_volatile_register(codec, reg) &&
> -		reg < codec->driver->reg_cache_size) {
> -		ret = snd_soc_cache_write(codec, reg, value);
> -		if (ret < 0)
> -			return -1;
> -	}
> -
> -	if (codec->cache_only) {
> -		codec->cache_sync = 1;
> -		return 0;
> -	}
> -
> -	ret = codec->hw_write(codec->control_data, data, 2);
> -	if (ret == 2)
> -		return 0;
> -	if (ret < 0)
> -		return ret;
> -	else
> -		return -EIO;
> +	return do_hw_write(codec, reg, value, data, 2);
>  }
>  
>  #if defined(CONFIG_SPI_MASTER)
> @@ -184,50 +162,19 @@ static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg,
>  			     unsigned int value)
>  {
>  	u8 data[2];
> -	int ret;
>  
>  	reg &= 0xff;
>  	data[0] = reg;
>  	data[1] = value & 0xff;
>  
> -	if (!snd_soc_codec_volatile_register(codec, reg) &&
> -		reg < codec->driver->reg_cache_size) {
> -		ret = snd_soc_cache_write(codec, reg, value);
> -		if (ret < 0)
> -			return -1;
> -	}
> -
> -	if (codec->cache_only) {
> -		codec->cache_sync = 1;
> -		return 0;
> -	}
> -
> -	if (codec->hw_write(codec->control_data, data, 2) == 2)
> -		return 0;
> -	else
> -		return -EIO;
> +	return do_hw_write(codec, reg, value, data, 2);
>  }
>  
>  static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec,
>  				     unsigned int reg)
>  {
> -	int ret;
> -	unsigned int val;
> -
>  	reg &= 0xff;
> -	if (reg >= codec->driver->reg_cache_size ||
> -		snd_soc_codec_volatile_register(codec, reg)) {
> -			if (codec->cache_only)
> -				return -1;
> -
> -			BUG_ON(!codec->hw_read);
> -			return codec->hw_read(codec, reg);
> -	}
> -
> -	ret = snd_soc_cache_read(codec, reg, &val);
> -	if (ret < 0)
> -		return -1;
> -	return val;
> +	return do_hw_read(codec, reg);
>  }
>  
>  #if defined(CONFIG_SPI_MASTER)
> @@ -264,49 +211,18 @@ static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg,
>  			      unsigned int value)
>  {
>  	u8 data[3];
> -	int ret;
>  
>  	data[0] = reg;
>  	data[1] = (value >> 8) & 0xff;
>  	data[2] = value & 0xff;
>  
> -	if (!snd_soc_codec_volatile_register(codec, reg) &&
> -		reg < codec->driver->reg_cache_size) {
> -		ret = snd_soc_cache_write(codec, reg, value);
> -		if (ret < 0)
> -			return -1;
> -	}
> -
> -	if (codec->cache_only) {
> -		codec->cache_sync = 1;
> -		return 0;
> -	}
> -
> -	if (codec->hw_write(codec->control_data, data, 3) == 3)
> -		return 0;
> -	else
> -		return -EIO;
> +	return do_hw_write(codec, reg, value, data, 3);
>  }
>  
>  static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec,
>  				      unsigned int reg)
>  {
> -	int ret;
> -	unsigned int val;
> -
> -	if (reg >= codec->driver->reg_cache_size ||
> -	    snd_soc_codec_volatile_register(codec, reg)) {
> -		if (codec->cache_only)
> -			return -1;
> -
> -		BUG_ON(!codec->hw_read);
> -		return codec->hw_read(codec, reg);
> -	}
> -
> -	ret = snd_soc_cache_read(codec, reg, &val);
> -	if (ret < 0)
> -		return -1;
> -	return val;
> +	return do_hw_read(codec, reg);
>  }
>  
>  #if defined(CONFIG_SPI_MASTER)
> @@ -445,55 +361,21 @@ static unsigned int snd_soc_16_8_read_i2c(struct snd_soc_codec *codec,
>  static unsigned int snd_soc_16_8_read(struct snd_soc_codec *codec,
>  				     unsigned int reg)
>  {
> -	int ret;
> -	unsigned int val;
> -
>  	reg &= 0xff;
> -	if (reg >= codec->driver->reg_cache_size ||
> -		snd_soc_codec_volatile_register(codec, reg)) {
> -			if (codec->cache_only)
> -				return -1;
> -
> -			BUG_ON(!codec->hw_read);
> -			return codec->hw_read(codec, reg);
> -	}
> -
> -	ret = snd_soc_cache_read(codec, reg, &val);
> -	if (ret < 0)
> -		return -1;
> -	return val;
> +	return do_hw_read(codec, reg);
>  }
>  
>  static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg,
>  			     unsigned int value)
>  {
>  	u8 data[3];
> -	int ret;
>  
>  	data[0] = (reg >> 8) & 0xff;
>  	data[1] = reg & 0xff;
>  	data[2] = value;
>  
>  	reg &= 0xff;
> -	if (!snd_soc_codec_volatile_register(codec, reg) &&
> -		reg < codec->driver->reg_cache_size) {
> -		ret = snd_soc_cache_write(codec, reg, value);
> -		if (ret < 0)
> -			return -1;
> -	}
> -
> -	if (codec->cache_only) {
> -		codec->cache_sync = 1;
> -		return 0;
> -	}
> -
> -	ret = codec->hw_write(codec->control_data, data, 3);
> -	if (ret == 3)
> -		return 0;
> -	if (ret < 0)
> -		return ret;
> -	else
> -		return -EIO;
> +	return do_hw_write(codec, reg, value, data, 3);
>  }
>  
>  #if defined(CONFIG_SPI_MASTER)
> @@ -564,55 +446,20 @@ static unsigned int snd_soc_16_16_read_i2c(struct snd_soc_codec *codec,
>  static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec,
>  				       unsigned int reg)
>  {
> -	int ret;
> -	unsigned int val;
> -
> -	if (reg >= codec->driver->reg_cache_size ||
> -	    snd_soc_codec_volatile_register(codec, reg)) {
> -		if (codec->cache_only)
> -			return -1;
> -
> -		BUG_ON(!codec->hw_read);
> -		return codec->hw_read(codec, reg);
> -	}
> -
> -	ret = snd_soc_cache_read(codec, reg, &val);
> -	if (ret < 0)
> -		return -1;
> -
> -	return val;
> +	return do_hw_read(codec, reg);
>  }
>  
>  static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg,
>  			       unsigned int value)
>  {
>  	u8 data[4];
> -	int ret;
>  
>  	data[0] = (reg >> 8) & 0xff;
>  	data[1] = reg & 0xff;
>  	data[2] = (value >> 8) & 0xff;
>  	data[3] = value & 0xff;
>  
> -	if (!snd_soc_codec_volatile_register(codec, reg) &&
> -		reg < codec->driver->reg_cache_size) {
> -		ret = snd_soc_cache_write(codec, reg, value);
> -		if (ret < 0)
> -			return -1;
> -	}
> -
> -	if (codec->cache_only) {
> -		codec->cache_sync = 1;
> -		return 0;
> -	}
> -
> -	ret = codec->hw_write(codec->control_data, data, 4);
> -	if (ret == 4)
> -		return 0;
> -	if (ret < 0)
> -		return ret;
> -	else
> -		return -EIO;
> +	return do_hw_write(codec, reg, value, data, 4);
>  }
>  
>  #if defined(CONFIG_SPI_MASTER)

All

Acked-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

Re: [PATCH 1/4] ASoC: clean up cache read/write functions

[Mark Brown]

On Mon, Dec 20, 2010 at 05:01:09PM +0100, Takashi Iwai wrote:

> +static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg,
> +                      unsigned int value, void *data, unsigned int size)
> +{
> +       int ret;
> +
> +       if (!snd_soc_codec_volatile_register(codec, reg) &&
> +           reg < codec->driver->reg_cache_size) {
> +               ret = snd_soc_cache_write(codec, reg, value);
> +               if (ret < 0)
> +                       return -1;
> +       }

This isn't actually doing a hardware write, though - it's the entire
write path which may or may not end up at the hardware.  The whole
passing of both the mangled and unmangled versions also feels a bit odd
here.

I think it'd be clearer to do this by making this a plain function and
adding a mangle operation set by the cache types which gets called out
to at the appropriate moment, that'd probably make the code flow more
naturally.

> +static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
> +				      unsigned int reg)
> +{
> +	return do_hw_read(codec, reg);
> +}
> +

>  static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec,
>  				     unsigned int reg)
>  {
> -	int ret;
> -	unsigned int val;
> -
> -	if (reg >= codec->driver->reg_cache_size ||
> -		snd_soc_codec_volatile_register(codec, reg)) {
> -			if (codec->cache_only)
> -				return -1;
> -
> -			BUG_ON(!codec->hw_read);
> -			return codec->hw_read(codec, reg);
> -	}
> -
> -	ret = snd_soc_cache_read(codec, reg, &val);
> -	if (ret < 0)
> -		return -1;
> -	return val;
> +	return do_hw_read(codec, reg);

If this is OK to do we should just be making do_hw_read() the operation
directly.

Re: [PATCH 1/4] ASoC: clean up cache read/write functions

[Takashi Iwai]

At Mon, 20 Dec 2010 17:24:05 +0000,
Mark Brown wrote:
> 
> On Mon, Dec 20, 2010 at 05:01:09PM +0100, Takashi Iwai wrote:
> 
> > +static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg,
> > +                      unsigned int value, void *data, unsigned int size)
> > +{
> > +       int ret;
> > +
> > +       if (!snd_soc_codec_volatile_register(codec, reg) &&
> > +           reg < codec->driver->reg_cache_size) {
> > +               ret = snd_soc_cache_write(codec, reg, value);
> > +               if (ret < 0)
> > +                       return -1;
> > +       }
> 
> This isn't actually doing a hardware write, though - it's the entire
> write path which may or may not end up at the hardware.  The whole
> passing of both the mangled and unmangled versions also feels a bit odd
> here.

Yes, this could be done better in the common place before write op is
called.

> I think it'd be clearer to do this by making this a plain function and
> adding a mangle operation set by the cache types which gets called out
> to at the appropriate moment, that'd probably make the code flow more
> naturally.

Sounds reasonable.

> > +static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
> > +				      unsigned int reg)
> > +{
> > +	return do_hw_read(codec, reg);
> > +}
> > +
> 
> >  static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec,
> >  				     unsigned int reg)
> >  {
> > -	int ret;
> > -	unsigned int val;
> > -
> > -	if (reg >= codec->driver->reg_cache_size ||
> > -		snd_soc_codec_volatile_register(codec, reg)) {
> > -			if (codec->cache_only)
> > -				return -1;
> > -
> > -			BUG_ON(!codec->hw_read);
> > -			return codec->hw_read(codec, reg);
> > -	}
> > -
> > -	ret = snd_soc_cache_read(codec, reg, &val);
> > -	if (ret < 0)
> > -		return -1;
> > -	return val;
> > +	return do_hw_read(codec, reg);
> 
> If this is OK to do we should just be making do_hw_read() the operation
> directly.

My patch is just a clean up, and I kept the code "reg &= 0xff" in some
places.  Without these, all can be the same function.


Takashi

Re: ASoC cache code (looking toward 2.6.38 merge window)

[Mark Brown]

On Mon, Dec 20, 2010 at 04:56:03PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > 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.

They should all be specifying something, quite what they should be
specifying varies - it just comes down to whatever the standard wire
format that the chip vendors have come up with.  Mostly if it's not
specified it should be little endian.

It's not really been an issue as pretty much all the CPUs have the same
endianness.

> > 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.

That's one way of doing it, but it does hurt usability.

> It's no urgent issue, yeah.  It's just what I feel uneasy to see such
> a thing...

If someone's worried about memory usage in ASoC they'd be much better
going after the strings than this TBH.

> +#ifdef CONFIG_SND_SOC_LZO_CACHE
> +	SND_SOC_LZO_COMPRESSION = 2,
> +#endif
> +#ifdef CONFIG_SND_SOC_RBTREE_CACHE
> +	SND_SOC_RBTREE_COMPRESSION = 3,
> +#endif

If we were going to make this user selectable we should keep the defines
and then fall back to a flat cache if they're turned off otherwise you
end up having too much faff with Kconfig interdependencies.

>  config SND_SOC_WM8994
>         tristate
> +       select SND_SOC_RBTREE_CACHE

Except the machine driver can override this...

Re: ASoC cache code (looking toward 2.6.38 merge window)

[Takashi Iwai]

At Mon, 20 Dec 2010 16:34:36 +0000,
Mark Brown wrote:
> 
> On Mon, Dec 20, 2010 at 04:56:03PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > 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.
> 
> They should all be specifying something, quite what they should be
> specifying varies - it just comes down to whatever the standard wire
> format that the chip vendors have come up with.  Mostly if it's not
> specified it should be little endian.
> 
> It's not really been an issue as pretty much all the CPUs have the same
> endianness.

Yeah, I know it's working now.  But it's hidden inside and merely
called portable ;)


> > > 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.
> 
> That's one way of doing it, but it does hurt usability.

That's a drawback, sure.

> > It's no urgent issue, yeah.  It's just what I feel uneasy to see such
> > a thing...
> 
> If someone's worried about memory usage in ASoC they'd be much better
> going after the strings than this TBH.
> 
> > +#ifdef CONFIG_SND_SOC_LZO_CACHE
> > +	SND_SOC_LZO_COMPRESSION = 2,
> > +#endif
> > +#ifdef CONFIG_SND_SOC_RBTREE_CACHE
> > +	SND_SOC_RBTREE_COMPRESSION = 3,
> > +#endif
> 
> If we were going to make this user selectable we should keep the defines
> and then fall back to a flat cache if they're turned off otherwise you
> end up having too much faff with Kconfig interdependencies.

My intention of the chunk above was to give a build error for
unsupported cache compressions.  But fallback sounds also reasonable,
too.

> >  config SND_SOC_WM8994
> >         tristate
> > +       select SND_SOC_RBTREE_CACHE
> 
> Except the machine driver can override this...

Hmm, I thought the codec is always selected not depended?
Can it be overridden?


thanks,

Takashi

Re: ASoC cache code (looking toward 2.6.38 merge window)

[Mark Brown]

On Mon, Dec 20, 2010 at 05:51:53PM +0100, Takashi Iwai wrote:
> Mark Brown wrote:

> > >  config SND_SOC_WM8994
> > >         tristate
> > > +       select SND_SOC_RBTREE_CACHE

> > Except the machine driver can override this...

> Hmm, I thought the codec is always selected not depended?
> Can it be overridden?

The CODEC support is selected (it's a physical property of the machine)
but the machine driver can override the cache type - the CODEC driver is
only providing a defualt.

Re: ASoC cache code (looking toward 2.6.38 merge window)

[Takashi Iwai]

At Mon, 20 Dec 2010 16:58:18 +0000,
Mark Brown wrote:
> 
> On Mon, Dec 20, 2010 at 05:51:53PM +0100, Takashi Iwai wrote:
> > Mark Brown wrote:
> 
> > > >  config SND_SOC_WM8994
> > > >         tristate
> > > > +       select SND_SOC_RBTREE_CACHE
> 
> > > Except the machine driver can override this...
> 
> > Hmm, I thought the codec is always selected not depended?
> > Can it be overridden?
> 
> The CODEC support is selected (it's a physical property of the machine)
> but the machine driver can override the cache type - the CODEC driver is
> only providing a defualt.

OK, it makes situation complicated, indeed.
Let's disregard this, then.


thanks,

Takashi