[alsa-devel] ASoC cache code (looking toward 2.6.38 merge window)
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
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.
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)
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)
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
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 = ® - - /* 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, ®, 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 = ® - - /* 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, ®, 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 *)® - - /* 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, ®, 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 *)® - - /* 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, ®, 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)
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; }
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
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
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.
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
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
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
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.
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
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; }
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...
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
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) &® < 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.
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) &® < 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
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...
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_CACHEExcept the machine driver can override this...
Hmm, I thought the codec is always selected not depended? Can it be overridden?
thanks,
Takashi
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.
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_CACHEExcept 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
participants (3)
-
Dimitris Papastamos -
Mark Brown -
Takashi Iwai