[PATCH 0/4] ASoC: Implement new caching API

[PATCH 0/4] ASoC: Implement new caching API

[Dimitris Papastamos]

This patch series introduces the new caching API.  The idea behind this
caching interface is that we can provide different means of organizing
and accessing the register cache.  This is useful for large and sparse
register maps, where one can use some kind of compression algorithm to
reduce the memory footprint.  The caching API is designed in such way to
eliminate the need for modifying any existing drivers.

Dimitris Papastamos (4):
  ASoC: soc.h: Add new caching API prototypes and hooks
  ASoC: soc-cache: Add support for standard register caching
  ASoC: soc-core: Adapt soc-core to fit the new caching API
  ASoC: soc-cache: Add support for LZO based register caching

 include/sound/soc.h   |   27 ++
 sound/soc/soc-cache.c |  646 ++++++++++++++++++++++++++++++++++++++++++++++---
 sound/soc/soc-core.c  |   38 ++--
 3 files changed, 657 insertions(+), 54 deletions(-)

-- 
1.7.3.1

[PATCH 1/4] ASoC: soc.h: Add new caching API prototypes and hooks

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 include/sound/soc.h |   27 +++++++++++++++++++++++++++
 1 files changed, 27 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index 5c3bce8..29b4aef 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -238,6 +238,7 @@ struct soc_enum;
 struct snd_soc_ac97_ops;
 struct snd_soc_jack;
 struct snd_soc_jack_pin;
+struct snd_soc_cache_ops;
 
 #ifdef CONFIG_GPIOLIB
 struct snd_soc_jack_gpio;
@@ -253,6 +254,11 @@ enum snd_soc_control_type {
 	SND_SOC_SPI,
 };
 
+enum snd_soc_compress_type {
+	SND_SOC_NO_COMPRESSION,
+	SND_SOC_LZO_COMPRESSION
+};
+
 int snd_soc_register_platform(struct device *dev,
 		struct snd_soc_platform_driver *platform_drv);
 void snd_soc_unregister_platform(struct device *dev);
@@ -264,6 +270,13 @@ int snd_soc_codec_volatile_register(struct snd_soc_codec *codec, int reg);
 int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 			       int addr_bits, int data_bits,
 			       enum snd_soc_control_type control);
+int snd_soc_cache_sync(struct snd_soc_codec *codec);
+int snd_soc_cache_init(struct snd_soc_codec *codec);
+int snd_soc_cache_deinit(struct snd_soc_codec *codec);
+int snd_soc_cache_write(struct snd_soc_codec *codec,
+			unsigned int reg, unsigned int value);
+int snd_soc_cache_read(struct snd_soc_codec *codec,
+		       unsigned int reg, unsigned int *value);
 
 /* Utility functions to get clock rates from various things */
 int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
@@ -420,6 +433,18 @@ struct snd_soc_ops {
 	int (*trigger)(struct snd_pcm_substream *, int);
 };
 
+/* SoC cache ops */
+struct snd_soc_cache_ops {
+	int id; /* corresponds to snd_soc_compress_type */
+	int (*init)(struct snd_soc_codec *codec);
+	int (*deinit)(struct snd_soc_codec *codec);
+	int (*read)(struct snd_soc_codec *codec, unsigned int reg,
+		unsigned int *value);
+	int (*write)(struct snd_soc_codec *codec, unsigned int reg,
+		unsigned int value);
+	int (*sync)(struct snd_soc_codec *codec);
+};
+
 /* SoC Audio Codec device */
 struct snd_soc_codec {
 	const char *name;
@@ -450,6 +475,7 @@ struct snd_soc_codec {
 	hw_write_t hw_write;
 	unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int);
 	void *reg_cache;
+	const struct snd_soc_cache_ops *cache_ops;
 
 	/* dapm */
 	u32 pop_time;
@@ -488,6 +514,7 @@ struct snd_soc_codec_driver {
 	short reg_cache_step;
 	short reg_word_size;
 	const void *reg_cache_default;
+	enum snd_soc_compress_type compress_type;
 
 	/* codec bias level */
 	int (*set_bias_level)(struct snd_soc_codec *,
-- 
1.7.3.1

Re: [PATCH 1/4] ASoC: soc.h: Add new caching API prototypes and hooks

[Mark Brown]

On Fri, Oct 22, 2010 at 03:28:19PM +0100, Dimitris Papastamos wrote:
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

Where possible you should try to split your series up such that each
is useful individually - splitting things up by file is mostly done for
the addition of large new blocks of code.  This means that the patches
can be applied one at a time without leaving the tree in an
"interesting" state.

> +int snd_soc_cache_sync(struct snd_soc_codec *codec);
> +int snd_soc_cache_init(struct snd_soc_codec *codec);
> +int snd_soc_cache_deinit(struct snd_soc_codec *codec);
> +int snd_soc_cache_write(struct snd_soc_codec *codec,
> +			unsigned int reg, unsigned int value);
> +int snd_soc_cache_read(struct snd_soc_codec *codec,
> +		       unsigned int reg, unsigned int *value);

For example, a patch adding the cache operations but supporting only the
current flat cache would introduce the cache API and factor out some
code usefully by itself.

[PATCH 2/4] ASoC: soc-cache: Add support for standard register caching

[Dimitris Papastamos]

This patch adds support for normal caching (no compression).  One
can still access codec->reg_cache directly but this is not advised
as that will not be portable across different caching strategies.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/soc-cache.c |  267 +++++++++++++++++++++++++++++++++++++++++++------
 1 files changed, 236 insertions(+), 31 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index d214f02..a8ec23a 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -18,7 +18,8 @@
 static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
 {
-	u16 *cache = codec->reg_cache;
+	int ret;
+	unsigned int val;
 
 	if (reg >= codec->driver->reg_cache_size ||
 		snd_soc_codec_volatile_register(codec, reg)) {
@@ -28,13 +29,15 @@ static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
 			return codec->hw_read(codec, reg);
 	}
 
-	return cache[reg];
+	ret = snd_soc_cache_read(codec, reg, &val);
+	if (ret < 0)
+		return -1;
+	return val;
 }
 
 static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
-	u16 *cache = codec->reg_cache;
 	u8 data[2];
 	int ret;
 
@@ -42,8 +45,11 @@ static int snd_soc_4_12_write(struct snd_soc_codec *codec, unsigned int reg,
 	data[1] = value & 0x00ff;
 
 	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size)
-			cache[reg] = value;
+		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;
@@ -94,7 +100,8 @@ 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)
 {
-	u16 *cache = codec->reg_cache;
+	int ret;
+	unsigned int val;
 
 	if (reg >= codec->driver->reg_cache_size ||
 		snd_soc_codec_volatile_register(codec, reg)) {
@@ -104,13 +111,15 @@ static unsigned int snd_soc_7_9_read(struct snd_soc_codec *codec,
 			return codec->hw_read(codec, reg);
 	}
 
-	return cache[reg];
+	ret = snd_soc_cache_read(codec, reg, &val);
+	if (ret < 0)
+		return -1;
+	return val;
 }
 
 static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
-	u16 *cache = codec->reg_cache;
 	u8 data[2];
 	int ret;
 
@@ -118,8 +127,11 @@ static int snd_soc_7_9_write(struct snd_soc_codec *codec, unsigned int reg,
 	data[1] = value & 0x00ff;
 
 	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size)
-			cache[reg] = value;
+		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;
@@ -170,16 +182,19 @@ static int snd_soc_7_9_spi_write(void *control_data, const char *data,
 static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
-	u8 *cache = codec->reg_cache;
 	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)
-			cache[reg] = value;
+		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;
@@ -197,7 +212,8 @@ static int snd_soc_8_8_write(struct snd_soc_codec *codec, unsigned int reg,
 static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
 {
-	u8 *cache = codec->reg_cache;
+	int ret;
+	unsigned int val;
 
 	reg &= 0xff;
 	if (reg >= codec->driver->reg_cache_size ||
@@ -208,7 +224,10 @@ static unsigned int snd_soc_8_8_read(struct snd_soc_codec *codec,
 			return codec->hw_read(codec, reg);
 	}
 
-	return cache[reg];
+	ret = snd_soc_cache_read(codec, reg, &val);
+	if (ret < 0)
+		return -1;
+	return val;
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -244,16 +263,19 @@ static int snd_soc_8_8_spi_write(void *control_data, const char *data,
 static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg,
 			      unsigned int value)
 {
-	u16 *reg_cache = codec->reg_cache;
 	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)
-		reg_cache[reg] = value;
+		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;
@@ -271,7 +293,8 @@ static int snd_soc_8_16_write(struct snd_soc_codec *codec, unsigned int reg,
 static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec,
 				      unsigned int reg)
 {
-	u16 *cache = codec->reg_cache;
+	int ret;
+	unsigned int val;
 
 	if (reg >= codec->driver->reg_cache_size ||
 	    snd_soc_codec_volatile_register(codec, reg)) {
@@ -279,9 +302,12 @@ static unsigned int snd_soc_8_16_read(struct snd_soc_codec *codec,
 			return -1;
 
 		return codec->hw_read(codec, reg);
-	} else {
-		return cache[reg];
 	}
+
+	ret = snd_soc_cache_read(codec, reg, &val);
+	if (ret < 0)
+		return -1;
+	return val;
 }
 
 #if defined(CONFIG_SPI_MASTER)
@@ -420,7 +446,8 @@ 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)
 {
-	u8 *cache = codec->reg_cache;
+	int ret;
+	unsigned int val;
 
 	reg &= 0xff;
 	if (reg >= codec->driver->reg_cache_size ||
@@ -431,13 +458,15 @@ static unsigned int snd_soc_16_8_read(struct snd_soc_codec *codec,
 			return codec->hw_read(codec, reg);
 	}
 
-	return cache[reg];
+	ret = snd_soc_cache_read(codec, reg, &val);
+	if (ret < 0)
+		return -1;
+	return val;
 }
 
 static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg,
 			     unsigned int value)
 {
-	u8 *cache = codec->reg_cache;
 	u8 data[3];
 	int ret;
 
@@ -447,8 +476,11 @@ static int snd_soc_16_8_write(struct snd_soc_codec *codec, unsigned int reg,
 
 	reg &= 0xff;
 	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size)
-			cache[reg] = value;
+		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;
@@ -534,7 +566,8 @@ 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)
 {
-	u16 *cache = codec->reg_cache;
+	int ret;
+	unsigned int val;
 
 	if (reg >= codec->driver->reg_cache_size ||
 	    snd_soc_codec_volatile_register(codec, reg)) {
@@ -544,13 +577,16 @@ static unsigned int snd_soc_16_16_read(struct snd_soc_codec *codec,
 		return codec->hw_read(codec, reg);
 	}
 
-	return cache[reg];
+	ret = snd_soc_cache_read(codec, reg, &val);
+	if (ret < 0)
+		return -1;
+
+	return val;
 }
 
 static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg,
 			       unsigned int value)
 {
-	u16 *cache = codec->reg_cache;
 	u8 data[4];
 	int ret;
 
@@ -560,8 +596,11 @@ static int snd_soc_16_16_write(struct snd_soc_codec *codec, unsigned int reg,
 	data[3] = value & 0xff;
 
 	if (!snd_soc_codec_volatile_register(codec, reg) &&
-		reg < codec->driver->reg_cache_size)
-			cache[reg] = value;
+		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;
@@ -724,3 +763,169 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 	return 0;
 }
 EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);
+
+static int snd_soc_cache_default_sync(struct snd_soc_codec *codec)
+{
+	const u8 *cache;
+	struct snd_soc_codec_driver *codec_drv;
+	unsigned int val;
+	int n;
+
+	codec_drv = codec->driver;
+	for (n = 0; n < codec_drv->reg_cache_size; ++n) {
+		snd_soc_cache_read(codec, n, &val);
+		if (codec_drv->reg_cache_default) {
+			cache = codec_drv->reg_cache_default;
+			cache += (n * codec_drv->reg_word_size);
+			if (!memcmp(&val, cache, codec_drv->reg_word_size))
+				continue;
+		}
+		snd_soc_write(codec, n, val);
+	}
+	return 0;
+}
+
+static int snd_soc_cache_default_write(struct snd_soc_codec *codec,
+				       unsigned int reg, unsigned int value)
+{
+	u8 *cache;
+
+	cache = codec->reg_cache;
+	memcpy(&cache[reg * codec->driver->reg_word_size],
+	       &value, codec->driver->reg_word_size);
+	return 0;
+}
+
+static int snd_soc_cache_default_read(struct snd_soc_codec *codec,
+				      unsigned int reg, unsigned int *value)
+{
+	u8 *cache;
+
+	*value = 0;
+	cache = codec->reg_cache;
+	memcpy(value, &cache[reg * codec->driver->reg_word_size],
+	       codec->driver->reg_word_size);
+	return 0;
+}
+
+static int snd_soc_cache_default_deinit(struct snd_soc_codec *codec)
+{
+	kfree(codec->reg_cache);
+	return 0;
+};
+
+static int snd_soc_cache_default_init(struct snd_soc_codec *codec)
+{
+	struct snd_soc_codec_driver *codec_drv;
+	size_t reg_size;
+
+	codec_drv = codec->driver;
+	reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+
+	if (codec_drv->reg_cache_default)
+		codec->reg_cache = kmemdup(codec_drv->reg_cache_default,
+					   reg_size, GFP_KERNEL);
+	else
+		codec->reg_cache = kzalloc(reg_size, GFP_KERNEL);
+	if (!codec->reg_cache)
+		return -EINVAL;
+
+	return 0;
+}
+
+/* an array of all supported compression types */
+static const struct snd_soc_cache_ops cache_types[] = {
+	{
+		.id = SND_SOC_NO_COMPRESSION,
+		.init = snd_soc_cache_default_init,
+		.deinit = snd_soc_cache_default_deinit,
+		.read = snd_soc_cache_default_read,
+		.write = snd_soc_cache_default_write,
+		.sync = snd_soc_cache_default_sync
+	}
+};
+
+int snd_soc_cache_init(struct snd_soc_codec *codec)
+{
+	int i;
+
+	for (i = 0; i < ARRAY_SIZE(cache_types); ++i)
+		if (cache_types[i].id == codec->driver->compress_type)
+			break;
+	if (i == ARRAY_SIZE(cache_types)) {
+		dev_err(codec->dev, "Could not match compress type: %d\n",
+			codec->driver->compress_type);
+		return -EINVAL;
+	}
+
+	codec->cache_ops = &cache_types[i];
+
+	if (codec->cache_ops->init)
+		return codec->cache_ops->init(codec);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(snd_soc_cache_init);
+
+int snd_soc_cache_deinit(struct snd_soc_codec *codec)
+{
+	if (codec->cache_ops && codec->cache_ops->deinit)
+		return codec->cache_ops->deinit(codec);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(snd_soc_cache_deinit);
+
+/**
+ * snd_soc_cache_read: Fetch the value of a given register from the cache.
+ *
+ * @codec: CODEC to configure.
+ * @reg: The register index.
+ * @value: The value to be returned.
+ */
+int snd_soc_cache_read(struct snd_soc_codec *codec,
+		       unsigned int reg, unsigned int *value)
+{
+	if (value && codec->cache_ops && codec->cache_ops->read)
+		return codec->cache_ops->read(codec, reg, value);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(snd_soc_cache_read);
+
+/**
+ * snd_soc_cache_write: Set the value of a given register in the cache.
+ *
+ * @codec: CODEC to configure.
+ * @reg: The register index.
+ * @value: The new register value.
+ */
+int snd_soc_cache_write(struct snd_soc_codec *codec,
+			unsigned int reg, unsigned int value)
+{
+	if (codec->cache_ops && codec->cache_ops->write)
+		return codec->cache_ops->write(codec, reg, value);
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(snd_soc_cache_write);
+
+/**
+ * snd_soc_cache_sync: Sync the register cache with the hardware.
+ *
+ * @codec: CODEC to configure.
+ *
+ * Any registers that should not be synced should be marked as
+ * volatile.
+ */
+int snd_soc_cache_sync(struct snd_soc_codec *codec)
+{
+	int ret;
+
+	if (!codec->cache_sync)
+		return 0;
+	if (codec->cache_ops && codec->cache_ops->sync) {
+		ret = codec->cache_ops->sync(codec);
+		if (!ret)
+			codec->cache_sync = 0;
+		return ret;
+	}
+	return -EINVAL;
+}
+EXPORT_SYMBOL_GPL(snd_soc_cache_sync);
-- 
1.7.3.1

Re: [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching

[Mark Brown]

On Fri, Oct 22, 2010 at 03:28:20PM +0100, Dimitris Papastamos wrote:

> +static int snd_soc_cache_default_sync(struct snd_soc_codec *codec)
> +{
> +       const u8 *cache;
> +       struct snd_soc_codec_driver *codec_drv;
> +       unsigned int val;
> 
> +	codec_drv = codec->driver;
> +	for (n = 0; n < codec_drv->reg_cache_size; ++n) {

Please use i as an array index unless using something meaningful.

> +                       if (!memcmp(&val, cache, codec_drv->reg_word_size))
> +                               continue;

This memcmp() looks very suspicious - we're copying from an unsigned int
into a variable of another type.  That seems to have a bit of an
endianness assumption, doesn't it?  It certainly needs comments
explaining how it works; a similar thing applies to the other memcpy()
and memcmp() operations in the code.

> +static int snd_soc_cache_default_deinit(struct snd_soc_codec *codec)
> +{

_exit or _free would be traditional.

> +	kfree(codec->reg_cache);
> +	return 0;
> +};

Extra ; (I'm surprised nothing warns).

Re: [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching

[Dimitris Papastamos]

On Fri, 2010-10-22 at 09:03 -0700, Mark Brown wrote:
> On Fri, Oct 22, 2010 at 03:28:20PM +0100, Dimitris Papastamos wrote:
> 
> > +static int snd_soc_cache_default_sync(struct snd_soc_codec *codec)
> > +{
> > +       const u8 *cache;
> > +       struct snd_soc_codec_driver *codec_drv;
> > +       unsigned int val;
> > 
> > +	codec_drv = codec->driver;
> > +	for (n = 0; n < codec_drv->reg_cache_size; ++n) {
> 
> Please use i as an array index unless using something meaningful.
> 
> > +                       if (!memcmp(&val, cache, codec_drv->reg_word_size))
> > +                               continue;
> 
> This memcmp() looks very suspicious - we're copying from an unsigned int
> into a variable of another type.  That seems to have a bit of an
> endianness assumption, doesn't it?  It certainly needs comments
> explaining how it works; a similar thing applies to the other memcpy()
> and memcmp() operations in the code.

Consider the following example. (unsigned int is 4 bytes).

unsigned int old = 0xABCD, new = 0;
void *p;

On a little-endian system this will be stored in memory as DCBA with D
being at a lower address.  Now consider the following code.

p = &old;
memcpy(&new, p, sizeof (unsigned int));

Now the value of new will be 0xABCD (stored in memory as DCBA again).
This holds both on a little-endian system as well as a big-endian
system.

The only problem I see with the above code, is when
codec_drv->reg_word_size > sizeof (unsigned int) but that can't really
happen in practice.

Thanks,
Dimitrios

Re: [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching

[Timur Tabi]

On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos
<dp@opensource.wolfsonmicro.com> wrote:
> The only problem I see with the above code, is when
> codec_drv->reg_word_size > sizeof (unsigned int) but that can't really
> happen in practice.

I'm going to have to agree with Mark that this code is suspect.  I
understand everything you said, but it makes me nervous.  Unless this
code is in some kind of fast-path, I would prefer to see it rewritten
to avoid any assumption about the sizes of the types involved.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching

[Mark Brown]

On Sun, Oct 24, 2010 at 08:18:59AM -0500, Timur Tabi wrote:
> On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos

> > The only problem I see with the above code, is when
> > codec_drv->reg_word_size > sizeof (unsigned int) but that can't really
> > happen in practice.

Plus if it did happen the rest of the code would fall over fairly badly
since we've got that assumption embedded in the register read and write
APIs.

> I'm going to have to agree with Mark that this code is suspect.  I
> understand everything you said, but it makes me nervous.  Unless this
> code is in some kind of fast-path, I would prefer to see it rewritten
> to avoid any assumption about the sizes of the types involved.

I think the important thing here is that the code is clear - from a
maintainability so long as it's clear how and why the code works things
should be fine, otherwise we'll have people scratching their heads over
it every time someone looks at the code which is going to be painful.
This could be done with documentation as well as with code changes,
though code changes should definitely be considered.

Re: [PATCH 2/4] ASoC: soc-cache: Add support for standard register caching

[Dimitris Papastamos]

On Sun, 2010-10-24 at 14:35 -0700, Mark Brown wrote:
> On Sun, Oct 24, 2010 at 08:18:59AM -0500, Timur Tabi wrote:
> > On Sat, Oct 23, 2010 at 1:24 PM, Dimitris Papastamos
> 
> > > The only problem I see with the above code, is when
> > > codec_drv->reg_word_size > sizeof (unsigned int) but that can't really
> > > happen in practice.
> 
> Plus if it did happen the rest of the code would fall over fairly badly
> since we've got that assumption embedded in the register read and write
> APIs.
> 
> > I'm going to have to agree with Mark that this code is suspect.  I
> > understand everything you said, but it makes me nervous.  Unless this
> > code is in some kind of fast-path, I would prefer to see it rewritten
> > to avoid any assumption about the sizes of the types involved.
> 
> I think the important thing here is that the code is clear - from a
> maintainability so long as it's clear how and why the code works things
> should be fine, otherwise we'll have people scratching their heads over
> it every time someone looks at the code which is going to be painful.
> This could be done with documentation as well as with code changes,
> though code changes should definitely be considered.

Yes that makes sense.  The reason why I did this in the first place was
to make it work with 8/16-byte reads/writes without using branching.  I
will change this to explicitly dereference a u8/u16 pointer.

Thanks,
Dimitrios

[PATCH 3/4] ASoC: soc-core: Adapt soc-core to fit the new caching API

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/soc-core.c |   38 +++++++++++++++-----------------------
 1 files changed, 15 insertions(+), 23 deletions(-)

diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c
index 70d9a73..c77f2b5 100644
--- a/sound/soc/soc-core.c
+++ b/sound/soc/soc-core.c
@@ -3213,23 +3213,6 @@ int snd_soc_register_codec(struct device *dev,
 		return -ENOMEM;
 	}
 
-	/* allocate CODEC register cache */
-	if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
-
-		if (codec_drv->reg_cache_default)
-			codec->reg_cache = kmemdup(codec_drv->reg_cache_default,
-				codec_drv->reg_cache_size * codec_drv->reg_word_size, GFP_KERNEL);
-		else
-			codec->reg_cache = kzalloc(codec_drv->reg_cache_size *
-				codec_drv->reg_word_size, GFP_KERNEL);
-
-		if (codec->reg_cache == NULL) {
-			kfree(codec->name);
-			kfree(codec);
-			return -ENOMEM;
-		}
-	}
-
 	codec->dev = dev;
 	codec->driver = codec_drv;
 	codec->bias_level = SND_SOC_BIAS_OFF;
@@ -3238,6 +3221,16 @@ int snd_soc_register_codec(struct device *dev,
 	INIT_LIST_HEAD(&codec->dapm_widgets);
 	INIT_LIST_HEAD(&codec->dapm_paths);
 
+	/* allocate CODEC register cache */
+	if (codec_drv->reg_cache_size && codec_drv->reg_word_size) {
+		ret = snd_soc_cache_init(codec);
+		if (ret < 0) {
+			dev_err(codec->dev, "Failed to set cache compression type: %d\n",
+				ret);
+			goto error_cache;
+		}
+	}
+
 	for (i = 0; i < num_dai; i++) {
 		fixup_codec_formats(&dai_drv[i].playback);
 		fixup_codec_formats(&dai_drv[i].capture);
@@ -3247,7 +3240,7 @@ int snd_soc_register_codec(struct device *dev,
 	if (num_dai) {
 		ret = snd_soc_register_dais(dev, dai_drv, num_dai);
 		if (ret < 0)
-			goto error;
+			goto error_dais;
 	}
 
 	mutex_lock(&client_mutex);
@@ -3258,12 +3251,12 @@ int snd_soc_register_codec(struct device *dev,
 	pr_debug("Registered codec '%s'\n", codec->name);
 	return 0;
 
-error:
+error_dais:
 	for (i--; i >= 0; i--)
 		snd_soc_unregister_dai(dev);
 
-	if (codec->reg_cache)
-		kfree(codec->reg_cache);
+	snd_soc_cache_deinit(codec);
+error_cache:
 	kfree(codec->name);
 	kfree(codec);
 	return ret;
@@ -3297,8 +3290,7 @@ found:
 
 	pr_debug("Unregistered codec '%s'\n", codec->name);
 
-	if (codec->reg_cache)
-		kfree(codec->reg_cache);
+	snd_soc_cache_deinit(codec);
 	kfree(codec->name);
 	kfree(codec);
 }
-- 
1.7.3.1

[PATCH 4/4] ASoC: soc-cache: Add support for LZO based register caching

[Dimitris Papastamos]

This patch adds support for LZO compression when storing the register
cache.  The initial register defaults cache is marked as __devinitconst
and the only change required for a driver to use LZO compression is
to set the compress_type member in codec->driver to SND_SOC_LZO_COMPRESSION.

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 sound/soc/soc-cache.c |  379 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 379 insertions(+), 0 deletions(-)

diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c
index a8ec23a..ea9d80f 100644
--- a/sound/soc/soc-cache.c
+++ b/sound/soc/soc-cache.c
@@ -14,6 +14,8 @@
 #include <linux/i2c.h>
 #include <linux/spi/spi.h>
 #include <sound/soc.h>
+#include <linux/lzo.h>
+#include <linux/bitmap.h>
 
 static unsigned int snd_soc_4_12_read(struct snd_soc_codec *codec,
 				     unsigned int reg)
@@ -764,6 +766,375 @@ int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 }
 EXPORT_SYMBOL_GPL(snd_soc_codec_set_cache_io);
 
+struct snd_soc_lzo_ctx {
+	void *wmem;
+	void *dst;
+	const void *src;
+	size_t src_len;
+	size_t dst_len;
+	size_t decompressed_size;
+	unsigned long *sync_bmp;
+	int sync_bmp_nbits;
+};
+
+#define LZO_BLOCK_NUM 8
+static int snd_soc_lzo_block_count(void)
+{
+	return LZO_BLOCK_NUM;
+}
+
+static int snd_soc_lzo_prepare(struct snd_soc_lzo_ctx *lzo_ctx)
+{
+	lzo_ctx->wmem = kmalloc(LZO1X_MEM_COMPRESS, GFP_KERNEL);
+	if (!lzo_ctx->wmem)
+		return -ENOMEM;
+	return 0;
+}
+
+static int snd_soc_lzo_compress(struct snd_soc_lzo_ctx *lzo_ctx)
+{
+	size_t compress_size;
+	int ret;
+
+	ret = lzo1x_1_compress(lzo_ctx->src, lzo_ctx->src_len,
+			       lzo_ctx->dst, &compress_size, lzo_ctx->wmem);
+	if (ret != LZO_E_OK || compress_size > lzo_ctx->dst_len)
+		return -EINVAL;
+	lzo_ctx->dst_len = compress_size;
+	return 0;
+}
+
+static int snd_soc_lzo_decompress(struct snd_soc_lzo_ctx *lzo_ctx)
+{
+	size_t dst_len;
+	int ret;
+
+	dst_len = lzo_ctx->dst_len;
+	ret = lzo1x_decompress_safe(lzo_ctx->src, lzo_ctx->src_len,
+				    lzo_ctx->dst, &dst_len);
+	if (ret != LZO_E_OK || dst_len != lzo_ctx->dst_len)
+		return -EINVAL;
+	return 0;
+}
+
+static int snd_soc_compress_cache_block(struct snd_soc_codec *codec,
+					struct snd_soc_lzo_ctx *lzo_ctx)
+{
+	int ret;
+
+	lzo_ctx->dst_len = lzo1x_worst_compress(PAGE_SIZE);
+	lzo_ctx->dst = kmalloc(lzo_ctx->dst_len, GFP_KERNEL);
+	if (!lzo_ctx->dst) {
+		lzo_ctx->dst_len = 0;
+		return -ENOMEM;
+	}
+
+	ret = snd_soc_lzo_compress(lzo_ctx);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static int snd_soc_decompress_cache_block(struct snd_soc_codec *codec,
+		struct snd_soc_lzo_ctx *lzo_ctx)
+{
+	int ret;
+
+	lzo_ctx->dst_len = lzo_ctx->decompressed_size;
+	lzo_ctx->dst = kmalloc(lzo_ctx->dst_len, GFP_KERNEL);
+	if (!lzo_ctx->dst) {
+		lzo_ctx->dst_len = 0;
+		return -ENOMEM;
+	}
+
+	ret = snd_soc_lzo_decompress(lzo_ctx);
+	if (ret < 0)
+		return ret;
+	return 0;
+}
+
+static inline int snd_soc_get_lzo_blkindex(struct snd_soc_codec *codec,
+		unsigned int reg)
+{
+	struct snd_soc_codec_driver *codec_drv;
+	size_t reg_size;
+
+	codec_drv = codec->driver;
+	reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+	return (reg * codec_drv->reg_word_size) /
+	       DIV_ROUND_UP(reg_size, snd_soc_lzo_block_count());
+}
+
+static inline int snd_soc_get_lzo_blkpos(struct snd_soc_codec *codec,
+		unsigned int reg)
+{
+	struct snd_soc_codec_driver *codec_drv;
+	size_t reg_size;
+
+	codec_drv = codec->driver;
+	reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+	return reg % (DIV_ROUND_UP(reg_size, snd_soc_lzo_block_count()) /
+		      codec_drv->reg_word_size);
+}
+
+static inline int snd_soc_get_lzo_blksize(struct snd_soc_codec *codec)
+{
+	struct snd_soc_codec_driver *codec_drv;
+	size_t reg_size;
+
+	codec_drv = codec->driver;
+	reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+	return DIV_ROUND_UP(reg_size, snd_soc_lzo_block_count());
+}
+
+static int snd_soc_cache_lzo_sync(struct snd_soc_codec *codec)
+{
+	struct snd_soc_lzo_ctx **lzo_blocks;
+	unsigned int val;
+	int n;
+
+	lzo_blocks = codec->reg_cache;
+	for_each_set_bit(n, lzo_blocks[0]->sync_bmp, lzo_blocks[0]->sync_bmp_nbits) {
+		snd_soc_cache_read(codec, n, &val);
+		snd_soc_write(codec, n, val);
+	}
+
+	return 0;
+}
+
+static int snd_soc_cache_lzo_write(struct snd_soc_codec *codec,
+				   unsigned int reg, unsigned int value)
+{
+	struct snd_soc_lzo_ctx *lzo_block, **lzo_blocks;
+	u8 *cache;
+	int ret, blkindex, blkpos;
+	size_t blksize, tmp_dst_len;
+	void *tmp_dst;
+
+	/* index of the compressed lzo block */
+	blkindex = snd_soc_get_lzo_blkindex(codec, reg);
+	/* register index within the decompressed block */
+	blkpos = snd_soc_get_lzo_blkpos(codec, reg);
+	blkpos *= codec->driver->reg_word_size;
+	/* size of the compressed block */
+	blksize = snd_soc_get_lzo_blksize(codec);
+	lzo_blocks = codec->reg_cache;
+	lzo_block = lzo_blocks[blkindex];
+
+	/* save the pointer and length of the compressed block */
+	tmp_dst = lzo_block->dst;
+	tmp_dst_len = lzo_block->dst_len;
+
+	/* prepare the source to be the compressed block */
+	lzo_block->src = lzo_block->dst;
+	lzo_block->src_len = lzo_block->dst_len;
+
+	/* decompress the block */
+	ret = snd_soc_decompress_cache_block(codec, lzo_block);
+	if (ret < 0) {
+		kfree(lzo_block->dst);
+		goto out;
+	}
+
+	cache = lzo_block->dst;
+	/* bail out early, no change in value */
+	if (!memcmp(&cache[blkpos], &value,
+		    codec->driver->reg_word_size)) {
+		kfree(lzo_block->dst);
+		goto out;
+	}
+	/* set the new register value */
+	memcpy(&cache[blkpos], &value, codec->driver->reg_word_size);
+
+	/* prepare the source to be the decompressed block */
+	lzo_block->src = lzo_block->dst;
+	lzo_block->src_len = lzo_block->dst_len;
+
+	/* compress the block */
+	ret = snd_soc_compress_cache_block(codec, lzo_block);
+	if (ret < 0) {
+		kfree(lzo_block->dst);
+		kfree(lzo_block->src);
+		goto out;
+	}
+
+	/* set the bit so we know we have to sync this register */
+	set_bit(reg, lzo_block->sync_bmp);
+	kfree(tmp_dst);
+	kfree(lzo_block->src);
+	return 0;
+out:
+	lzo_block->dst = tmp_dst;
+	lzo_block->dst_len = tmp_dst_len;
+	return ret;
+}
+
+static int snd_soc_cache_lzo_read(struct snd_soc_codec *codec,
+				  unsigned int reg, unsigned int *value)
+{
+	struct snd_soc_lzo_ctx *lzo_block, **lzo_blocks;
+	u8 *cache;
+	int ret, blkindex, blkpos;
+	size_t blksize, tmp_dst_len;
+	void *tmp_dst;
+
+	*value = 0;
+	/* index of the compressed lzo block */
+	blkindex = snd_soc_get_lzo_blkindex(codec, reg);
+	/* register index within the decompressed block */
+	blkpos = snd_soc_get_lzo_blkpos(codec, reg);
+	blkpos *= codec->driver->reg_word_size;
+	/* size of the compressed block */
+	blksize = snd_soc_get_lzo_blksize(codec);
+	lzo_blocks = codec->reg_cache;
+	lzo_block = lzo_blocks[blkindex];
+
+	/* save the pointer and length of the compressed block */
+	tmp_dst = lzo_block->dst;
+	tmp_dst_len = lzo_block->dst_len;
+
+	/* prepare the source to be the compressed block */
+	lzo_block->src = lzo_block->dst;
+	lzo_block->src_len = lzo_block->dst_len;
+
+	/* decompress the block */
+	ret = snd_soc_decompress_cache_block(codec, lzo_block);
+	if (ret >= 0) {
+		cache = lzo_block->dst;
+		memcpy(value, &cache[blkpos],
+		       codec->driver->reg_word_size);
+	}
+
+	kfree(lzo_block->dst);
+	/* restore the pointer and length of the compressed block */
+	lzo_block->dst = tmp_dst;
+	lzo_block->dst_len = tmp_dst_len;
+	return 0;
+}
+
+static int snd_soc_cache_lzo_deinit(struct snd_soc_codec *codec)
+{
+	struct snd_soc_lzo_ctx **lzo_blocks;
+	int i, blkcount;
+
+	lzo_blocks = codec->reg_cache;
+	if (!lzo_blocks)
+		return -EINVAL;
+
+	blkcount = snd_soc_lzo_block_count();
+	if (lzo_blocks[0])
+		kfree(lzo_blocks[0]->sync_bmp);
+	for (i = 0; i < blkcount; ++i) {
+		if (lzo_blocks[i]) {
+			kfree(lzo_blocks[i]->wmem);
+			kfree(lzo_blocks[i]->dst);
+		}
+		kfree(lzo_blocks[i]);
+		lzo_blocks[i] = NULL;
+	}
+	kfree(lzo_blocks);
+	codec->reg_cache = NULL;
+	return 0;
+}
+
+static int snd_soc_cache_lzo_init(struct snd_soc_codec *codec)
+{
+	struct snd_soc_lzo_ctx **lzo_blocks;
+	size_t reg_size;
+	struct snd_soc_codec_driver *codec_drv;
+	int ret, tofree, i, blksize, blkcount;
+	const char *p, *end;
+	unsigned long *sync_bmp;
+
+	ret = 0;
+	codec_drv = codec->driver;
+	reg_size = codec_drv->reg_cache_size * codec_drv->reg_word_size;
+
+	/*
+	 * If we have not been given a default register cache
+	 * then allocate a dummy zero-ed out region, compress it
+	 * and remember to free it afterwards.
+	 */
+	tofree = 0;
+	if (!codec_drv->reg_cache_default)
+		tofree = 1;
+
+	if (!codec_drv->reg_cache_default) {
+		codec_drv->reg_cache_default = kzalloc(reg_size,
+						       GFP_KERNEL);
+		if (!codec_drv->reg_cache_default)
+			return -ENOMEM;
+	}
+
+	blkcount = snd_soc_lzo_block_count();
+	codec->reg_cache = kzalloc(blkcount * sizeof(*lzo_blocks),
+				   GFP_KERNEL);
+	if (!codec->reg_cache) {
+		ret = -ENOMEM;
+		goto err_tofree;
+	}
+	lzo_blocks = codec->reg_cache;
+
+	/*
+	 * allocate a bitmap to be used when syncing the cache with
+	 * the hardware.  Each time a register is modified, the corresponding
+	 * bit is set in the bitmap, so we know that we have to sync
+	 * that register.
+	 */
+	sync_bmp = kmalloc(BITS_TO_LONGS(reg_size) * sizeof(unsigned long),
+			   GFP_KERNEL);
+	if (!sync_bmp) {
+		ret = -ENOMEM;
+		goto err;
+	}
+	bitmap_zero(sync_bmp, reg_size);
+
+	/* allocate the lzo blocks and initialize them */
+	for (i = 0; i < blkcount; ++i) {
+		lzo_blocks[i] = kzalloc(sizeof **lzo_blocks,
+					GFP_KERNEL);
+		if (!lzo_blocks[i]) {
+			kfree(sync_bmp);
+			ret = -ENOMEM;
+			goto err;
+		}
+		lzo_blocks[i]->sync_bmp = sync_bmp;
+		lzo_blocks[i]->sync_bmp_nbits = reg_size;
+		/* alloc the working space for the compressed block */
+		ret = snd_soc_lzo_prepare(lzo_blocks[i]);
+		if (ret < 0)
+			goto err;
+	}
+
+	blksize = snd_soc_get_lzo_blksize(codec);
+	p = codec_drv->reg_cache_default;
+	end = codec_drv->reg_cache_default + reg_size;
+	/* compress the register map and fill the lzo blocks */
+	for (i = 0; i < blkcount; ++i, p += blksize) {
+		lzo_blocks[i]->src = p;
+		if (p + blksize > end)
+			lzo_blocks[i]->src_len = end - p;
+		else
+			lzo_blocks[i]->src_len = blksize;
+		ret = snd_soc_compress_cache_block(codec,
+						   lzo_blocks[i]);
+		if (ret < 0)
+			goto err;
+		lzo_blocks[i]->decompressed_size =
+			lzo_blocks[i]->src_len;
+	}
+
+	if (tofree)
+		kfree(codec_drv->reg_cache_default);
+	return 0;
+err:
+	snd_soc_cache_deinit(codec);
+err_tofree:
+	if (tofree)
+		kfree(codec_drv->reg_cache_default);
+	return ret;
+}
+
 static int snd_soc_cache_default_sync(struct snd_soc_codec *codec)
 {
 	const u8 *cache;
@@ -842,6 +1213,14 @@ static const struct snd_soc_cache_ops cache_types[] = {
 		.read = snd_soc_cache_default_read,
 		.write = snd_soc_cache_default_write,
 		.sync = snd_soc_cache_default_sync
+	},
+	{
+		.id = SND_SOC_LZO_COMPRESSION,
+		.init = snd_soc_cache_lzo_init,
+		.deinit = snd_soc_cache_lzo_deinit,
+		.read = snd_soc_cache_lzo_read,
+		.write = snd_soc_cache_lzo_write,
+		.sync = snd_soc_cache_lzo_sync
 	}
 };
 
-- 
1.7.3.1

Re: [PATCH 4/4] ASoC: soc-cache: Add support for LZO based register caching

[Mark Brown]

On Fri, Oct 22, 2010 at 03:28:22PM +0100, Dimitris Papastamos wrote:

> This patch adds support for LZO compression when storing the register
> cache.  The initial register defaults cache is marked as __devinitconst
> and the only change required for a driver to use LZO compression is

This looks good, though the changelog could use a bit more discussion as
to the tradeoffs involved - clearly we're trading CPU consumption for
memory consumption here, but things like numbers about the sorts of
compression you can get and the amount of CPU time burned relative to
the actual I/O operations would help people understand when and how to
use this.

Actually, now that I think about it debug data either via debugfs or via
dev_dbg() showing the before and after memory sizes would be very useful
for people trying to decide if their register map compresses down well
enough to really benefit from compression.

It looks like this patch also needs to add selects for LZO_COMPRESS and
LZO_DECOMPRESS to Kconfig, otherwise we'll fail to build if nothing else
has enabled them.

> to set the compress_type member in codec->driver to
> SND_SOC_LZO_COMPRESSION.

It would be good if machine drivers were able to override this, 

> +static int snd_soc_compress_cache_block(struct snd_soc_codec *codec,
> +					struct snd_soc_lzo_ctx *lzo_ctx)
> +{

This is all rather assuming that LZO is the only compression method we
can use?  It doesn't really matter, though, as this is all internal to
the cache code so we can deal with adding new compression types as and
when we want them.

Re: [PATCH 4/4] ASoC: soc-cache: Add support for LZO based register caching

[Dimitris Papastamos]

On Fri, 2010-10-22 at 09:18 -0700, Mark Brown wrote:
> On Fri, Oct 22, 2010 at 03:28:22PM +0100, Dimitris Papastamos wrote:
> 
> > This patch adds support for LZO compression when storing the register
> > cache.  The initial register defaults cache is marked as __devinitconst
> > and the only change required for a driver to use LZO compression is
> 
> This looks good, though the changelog could use a bit more discussion as
> to the tradeoffs involved - clearly we're trading CPU consumption for
> memory consumption here, but things like numbers about the sorts of
> compression you can get and the amount of CPU time burned relative to
> the actual I/O operations would help people understand when and how to
> use this.

Yes, that makes sense.

> Actually, now that I think about it debug data either via debugfs or via
> dev_dbg() showing the before and after memory sizes would be very useful
> for people trying to decide if their register map compresses down well
> enough to really benefit from compression.

I'll send an incremental patch for that.

> It looks like this patch also needs to add selects for LZO_COMPRESS and
> LZO_DECOMPRESS to Kconfig, otherwise we'll fail to build if nothing else
> has enabled them.
> 
> > to set the compress_type member in codec->driver to
> > SND_SOC_LZO_COMPRESSION.
> 
> It would be good if machine drivers were able to override this, 

I'll think about this to see how it can be overriden.

> > +static int snd_soc_compress_cache_block(struct snd_soc_codec *codec,
> > +					struct snd_soc_lzo_ctx *lzo_ctx)
> > +{
> 
> This is all rather assuming that LZO is the only compression method we
> can use?  It doesn't really matter, though, as this is all internal to
> the cache code so we can deal with adding new compression types as and
> when we want them.

The function naming is wrong.  This function is a helper function for
the LZO compression type.  I've prefixed this function with LZO to avoid
misunderstanding.  Ideally this would live in a separate file.

Thanks,
Dimitrios

Re: [PATCH 4/4] ASoC: soc-cache: Add support for LZO based register caching

[Mark Brown]

On Sat, Oct 23, 2010 at 07:28:01PM +0100, Dimitris Papastamos wrote:

> The function naming is wrong.  This function is a helper function for
> the LZO compression type.  I've prefixed this function with LZO to avoid
> misunderstanding.  Ideally this would live in a separate file.

Go ahead and split out if it seems useful.

[PATCH 0/4] ASoC: Introduce the new caching API

[Dimitris Papastamos]

This patch series introduces the new caching API.  The idea behind this
caching interface is that we can provide different means of organizing
and accessing the register cache.  This is useful for large and sparse
register maps, where one can use some kind of compression algorithm to
reduce the memory footprint.  The caching API is designed in such way to
eliminate the need for modifying any existing drivers.

Things that still need to be done.

 - Memory usage statistics, to make it easier to select the proper caching
 technique.
 - Support for bulk reads/writes.

Dimitris Papastamos (4):
  ASoC: soc.h: Add new caching API prototypes and hooks
  ASoC: soc-cache: Add support for flat register caching
  ASoC: soc-cache: Add support for LZO register caching
  ASoC: soc-cache: Add support for rbtree based register caching

 include/sound/soc.h   |   28 ++
 sound/soc/Kconfig     |    2 +
 sound/soc/soc-cache.c |  988 +++++++++++++++++++++++++++++++++++++++++++++++--
 sound/soc/soc-core.c  |   38 +--
 4 files changed, 1002 insertions(+), 54 deletions(-)

-- 
1.7.3.2

[PATCH 1/4] ASoC: soc.h: Add new caching API prototypes and hooks

[Dimitris Papastamos]

Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>
---
 include/sound/soc.h |   26 ++++++++++++++++++++++++++
 sound/soc/Kconfig   |    2 ++
 2 files changed, 28 insertions(+), 0 deletions(-)

diff --git a/include/sound/soc.h b/include/sound/soc.h
index aaf34d7..90a9b60 100644
--- a/include/sound/soc.h
+++ b/include/sound/soc.h
@@ -238,6 +238,7 @@ struct soc_enum;
 struct snd_soc_ac97_ops;
 struct snd_soc_jack;
 struct snd_soc_jack_pin;
+struct snd_soc_cache_ops;
 
 #ifdef CONFIG_GPIOLIB
 struct snd_soc_jack_gpio;
@@ -253,6 +254,10 @@ enum snd_soc_control_type {
 	SND_SOC_SPI,
 };
 
+enum snd_soc_compress_type {
+	SND_SOC_NO_COMPRESSION
+};
+
 int snd_soc_register_platform(struct device *dev,
 		struct snd_soc_platform_driver *platform_drv);
 void snd_soc_unregister_platform(struct device *dev);
@@ -264,6 +269,13 @@ int snd_soc_codec_volatile_register(struct snd_soc_codec *codec, int reg);
 int snd_soc_codec_set_cache_io(struct snd_soc_codec *codec,
 			       int addr_bits, int data_bits,
 			       enum snd_soc_control_type control);
+int snd_soc_cache_sync(struct snd_soc_codec *codec);
+int snd_soc_cache_init(struct snd_soc_codec *codec);
+int snd_soc_cache_exit(struct snd_soc_codec *codec);
+int snd_soc_cache_write(struct snd_soc_codec *codec,
+			unsigned int reg, unsigned int value);
+int snd_soc_cache_read(struct snd_soc_codec *codec,
+		       unsigned int reg, unsigned int *value);
 
 /* Utility functions to get clock rates from various things */
 int snd_soc_calc_frame_size(int sample_size, int channels, int tdm_slots);
@@ -420,6 +432,18 @@ struct snd_soc_ops {
 	int (*trigger)(struct snd_pcm_substream *, int);
 };
 
+/* SoC cache ops */
+struct snd_soc_cache_ops {
+	int id; /* corresponds to snd_soc_compress_type */
+	int (*init)(struct snd_soc_codec *codec);
+	int (*exit)(struct snd_soc_codec *codec);
+	int (*read)(struct snd_soc_codec *codec, unsigned int reg,
+		unsigned int *value);
+	int (*write)(struct snd_soc_codec *codec, unsigned int reg,
+		unsigned int value);
+	int (*sync)(struct snd_soc_codec *codec);
+};
+
 /* SoC Audio Codec device */
 struct snd_soc_codec {
 	const char *name;
@@ -450,6 +474,7 @@ struct snd_soc_codec {
 	hw_write_t hw_write;
 	unsigned int (*hw_read)(struct snd_soc_codec *, unsigned int);
 	void *reg_cache;
+	const struct snd_soc_cache_ops *cache_ops;
 
 	/* dapm */
 	u32 pop_time;
@@ -488,6 +513,7 @@ struct snd_soc_codec_driver {
 	short reg_cache_step;
 	short reg_word_size;
 	const void *reg_cache_default;
+	enum snd_soc_compress_type compress_type;
 
 	/* codec bias level */
 	int (*set_bias_level)(struct snd_soc_codec *,
diff --git a/sound/soc/Kconfig b/sound/soc/Kconfig
index 3e598e7..4562c89 100644
--- a/sound/soc/Kconfig
+++ b/sound/soc/Kconfig
@@ -4,6 +4,8 @@
 
 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
-- 
1.7.3.2

Re: [PATCH 1/4] ASoC: soc.h: Add new caching API prototypes and hooks

[Mark Brown]

On Thu, Nov 04, 2010 at 02:22:41PM +0000, Dimitris Papastamos wrote:

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

As previously mentioned please split this series up into patches which
individually add some useful functiionality 

> +/* SoC cache ops */
> +struct snd_soc_cache_ops {
> +	int id; /* corresponds to snd_soc_compress_type */

Shouldn't this be using the enum then?

> --- a/sound/soc/Kconfig
> +++ b/sound/soc/Kconfig
> @@ -4,6 +4,8 @@
>  
>  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

This definitely looks like it should be in a separate patch.