[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
For codecs whose reg_cache_step is not 1, the original cache io code may fetch a wrong value since it does not check the reg_cache_step and remainly use the reg as index to fetch value from cache.
This patch provides help functions for conversion between cache index and register index and the cache io functions will use them in right place to ensure to fetch a correct register value.
Signed-off-by: Dong Aisheng b29396@freescale.com Cc: Mark Brown broonie@opensource.wolfsonmicro.com Cc: Liam Girdwood lrg@ti.com Cc: Sascha Hauer s.hauer@pengutronix.de Cc: Wolfram Sang w.sang@pengutronix.de --- include/sound/soc.h | 4 ++ sound/soc/soc-cache.c | 117 +++++++++++++++++++++++++++++++++++++------------ sound/soc/soc-core.c | 15 +++--- sound/soc/soc-io.c | 10 +++- 4 files changed, 107 insertions(+), 39 deletions(-)
diff --git a/include/sound/soc.h b/include/sound/soc.h index aa19f5a..b70789d 100644 --- a/include/sound/soc.h +++ b/include/sound/soc.h @@ -306,6 +306,10 @@ 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); +int snd_soc_cache_reg_to_idx(struct snd_soc_codec *codec, + unsigned int reg); +int snd_soc_cache_idx_to_reg(struct snd_soc_codec *codec, + unsigned int idx); int snd_soc_default_volatile_register(struct snd_soc_codec *codec, unsigned int reg); int snd_soc_default_readable_register(struct snd_soc_codec *codec, diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index d9f8ade..1a08bcf 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -66,6 +66,38 @@ static unsigned int snd_soc_get_cache_val(const void *base, unsigned int idx, return -1; }
+int snd_soc_cache_reg_to_idx(struct snd_soc_codec *codec, + unsigned int reg) +{ + const struct snd_soc_codec_driver *codec_drv; + unsigned int idx = 0; + + codec_drv = codec->driver; + + if (codec_drv->reg_cache_step > 0) + idx = reg / codec_drv->reg_cache_step; + else + idx = reg; + + return idx; +} + +int snd_soc_cache_idx_to_reg(struct snd_soc_codec *codec, + unsigned int idx) +{ + const struct snd_soc_codec_driver *codec_drv; + unsigned int reg = 0; + + codec_drv = codec->driver; + + if (codec_drv->reg_cache_step > 0) + reg = idx * codec_drv->reg_cache_step; + else + reg = idx; + + return reg; +} + struct snd_soc_rbtree_node { struct rb_node node; /* the actual rbtree node holding this block */ unsigned int base_reg; /* base register handled by this block */ @@ -262,14 +294,17 @@ static int snd_soc_rbtree_cache_write(struct snd_soc_codec *codec, unsigned int pos; int i; int ret; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
rbtree_ctx = codec->reg_cache; /* look up the required register in the cached rbnode */ rbnode = rbtree_ctx->cached_rbnode; if (rbnode) { snd_soc_rbtree_get_base_top_reg(rbnode, &base_reg, &top_reg); - if (reg >= base_reg && reg <= top_reg) { - reg_tmp = reg - base_reg; + if (idx >= base_reg && idx <= top_reg) { + reg_tmp = idx - base_reg; val = snd_soc_rbtree_get_register(rbnode, reg_tmp); if (val == value) return 0; @@ -280,9 +315,9 @@ static int snd_soc_rbtree_cache_write(struct snd_soc_codec *codec, /* if we can't locate it in the cached rbnode we'll have * to traverse the rbtree looking for it. */ - rbnode = snd_soc_rbtree_lookup(&rbtree_ctx->root, reg); + rbnode = snd_soc_rbtree_lookup(&rbtree_ctx->root, idx); if (rbnode) { - reg_tmp = reg - rbnode->base_reg; + reg_tmp = idx - rbnode->base_reg; val = snd_soc_rbtree_get_register(rbnode, reg_tmp); if (val == value) return 0; @@ -298,15 +333,15 @@ static int snd_soc_rbtree_cache_write(struct snd_soc_codec *codec, rbnode_tmp = rb_entry(node, struct snd_soc_rbtree_node, node); for (i = 0; i < rbnode_tmp->blklen; ++i) { reg_tmp = rbnode_tmp->base_reg + i; - if (abs(reg_tmp - reg) != 1) + if (abs(reg_tmp - idx) != 1) continue; /* decide where in the block to place our register */ - if (reg_tmp + 1 == reg) + if (reg_tmp + 1 == idx) pos = i + 1; else pos = i; ret = snd_soc_rbtree_insert_to_block(rbnode_tmp, pos, - reg, value); + idx, value); if (ret) return ret; rbtree_ctx->cached_rbnode = rbnode_tmp; @@ -322,7 +357,7 @@ static int snd_soc_rbtree_cache_write(struct snd_soc_codec *codec, if (!rbnode) return -ENOMEM; rbnode->blklen = 1; - rbnode->base_reg = reg; + rbnode->base_reg = idx; rbnode->word_size = codec->driver->reg_word_size; rbnode->block = kmalloc(rbnode->blklen * rbnode->word_size, GFP_KERNEL); @@ -345,14 +380,17 @@ static int snd_soc_rbtree_cache_read(struct snd_soc_codec *codec, struct snd_soc_rbtree_node *rbnode; unsigned int base_reg, top_reg; unsigned int reg_tmp; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
rbtree_ctx = codec->reg_cache; /* look up the required register in the cached rbnode */ rbnode = rbtree_ctx->cached_rbnode; if (rbnode) { snd_soc_rbtree_get_base_top_reg(rbnode, &base_reg, &top_reg); - if (reg >= base_reg && reg <= top_reg) { - reg_tmp = reg - base_reg; + if (idx >= base_reg && reg <= top_reg) { + reg_tmp = idx - base_reg; *value = snd_soc_rbtree_get_register(rbnode, reg_tmp); return 0; } @@ -360,9 +398,9 @@ static int snd_soc_rbtree_cache_read(struct snd_soc_codec *codec, /* if we can't locate it in the cached rbnode we'll have * to traverse the rbtree looking for it. */ - rbnode = snd_soc_rbtree_lookup(&rbtree_ctx->root, reg); + rbnode = snd_soc_rbtree_lookup(&rbtree_ctx->root, idx); if (rbnode) { - reg_tmp = reg - rbnode->base_reg; + reg_tmp = idx - rbnode->base_reg; *value = snd_soc_rbtree_get_register(rbnode, reg_tmp); rbtree_ctx->cached_rbnode = rbnode; } else { @@ -408,6 +446,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) unsigned int val; int i; int ret; + unsigned int reg;
codec->reg_cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL); if (!codec->reg_cache) @@ -426,7 +465,8 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) word_size); if (!val) continue; - ret = snd_soc_rbtree_cache_write(codec, i, val); + reg = snd_soc_cache_idx_to_reg(codec, i); + ret = snd_soc_rbtree_cache_write(codec, reg, val); if (ret) goto err; } @@ -560,21 +600,23 @@ static int snd_soc_lzo_cache_sync(struct snd_soc_codec *codec) unsigned int val; int i; int ret; + unsigned int reg;
lzo_blocks = codec->reg_cache; for_each_set_bit(i, lzo_blocks[0]->sync_bmp, lzo_blocks[0]->sync_bmp_nbits) { + reg = snd_soc_cache_idx_to_reg(codec, i); WARN_ON(codec->writable_register && - codec->writable_register(codec, i)); - ret = snd_soc_cache_read(codec, i, &val); + codec->writable_register(codec, reg)); + ret = snd_soc_cache_read(codec, reg, &val); if (ret) return ret; codec->cache_bypass = 1; - ret = snd_soc_write(codec, i, val); + ret = snd_soc_write(codec, reg, val); codec->cache_bypass = 0; if (ret) return ret; dev_dbg(codec->dev, "Synced register %#x, value = %#x\n", - i, val); + reg, val); }
return 0; @@ -587,11 +629,14 @@ static int snd_soc_lzo_cache_write(struct snd_soc_codec *codec, int ret, blkindex, blkpos; size_t blksize, tmp_dst_len; void *tmp_dst; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
/* index of the compressed lzo block */ - blkindex = snd_soc_lzo_get_blkindex(codec, reg); + blkindex = snd_soc_lzo_get_blkindex(codec, idx); /* register index within the decompressed block */ - blkpos = snd_soc_lzo_get_blkpos(codec, reg); + blkpos = snd_soc_lzo_get_blkpos(codec, idx); /* size of the compressed block */ blksize = snd_soc_lzo_get_blksize(codec); lzo_blocks = codec->reg_cache; @@ -632,7 +677,7 @@ static int snd_soc_lzo_cache_write(struct snd_soc_codec *codec, }
/* set the bit so we know we have to sync this register */ - set_bit(reg, lzo_block->sync_bmp); + set_bit(idx, lzo_block->sync_bmp); kfree(tmp_dst); kfree(lzo_block->src); return 0; @@ -649,12 +694,15 @@ static int snd_soc_lzo_cache_read(struct snd_soc_codec *codec, int ret, blkindex, blkpos; size_t blksize, tmp_dst_len; void *tmp_dst; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
*value = 0; /* index of the compressed lzo block */ - blkindex = snd_soc_lzo_get_blkindex(codec, reg); + blkindex = snd_soc_lzo_get_blkindex(codec, idx); /* register index within the decompressed block */ - blkpos = snd_soc_lzo_get_blkpos(codec, reg); + blkpos = snd_soc_lzo_get_blkpos(codec, idx); /* size of the compressed block */ blksize = snd_soc_lzo_get_blksize(codec); lzo_blocks = codec->reg_cache; @@ -820,23 +868,25 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec) int ret; const struct snd_soc_codec_driver *codec_drv; unsigned int val; + unsigned int reg;
codec_drv = codec->driver; for (i = 0; i < codec_drv->reg_cache_size; ++i) { + reg = snd_soc_cache_idx_to_reg(codec, i); WARN_ON(codec->writable_register && - codec->writable_register(codec, i)); - ret = snd_soc_cache_read(codec, i, &val); + codec->writable_register(codec, reg)); + ret = snd_soc_cache_read(codec, reg, &val); if (ret) return ret; if (codec->reg_def_copy) if (snd_soc_get_cache_val(codec->reg_def_copy, - i, codec_drv->reg_word_size) == val) + reg, codec_drv->reg_word_size) == val) continue; - ret = snd_soc_write(codec, i, val); + ret = snd_soc_write(codec, reg, val); if (ret) return ret; dev_dbg(codec->dev, "Synced register %#x, value = %#x\n", - i, val); + reg, val); } return 0; } @@ -844,15 +894,24 @@ 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) { - snd_soc_set_cache_val(codec->reg_cache, reg, value, + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg); + + snd_soc_set_cache_val(codec->reg_cache, idx, 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) { - *value = snd_soc_get_cache_val(codec->reg_cache, reg, + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg); + + *value = snd_soc_get_cache_val(codec->reg_cache, idx, codec->driver->reg_word_size); return 0; } diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 83ad8ca..a23971e 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -125,11 +125,12 @@ static int format_register_str(struct snd_soc_codec *codec, static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, size_t count, loff_t pos) { - int i, step = 1; + int i; int wordsize, regsize; int len; size_t total = 0; loff_t p = 0; + unsigned int reg;
wordsize = min_bytes_needed(codec->driver->reg_cache_size) * 2; regsize = codec->driver->reg_word_size * 2; @@ -139,22 +140,20 @@ static ssize_t soc_codec_reg_show(struct snd_soc_codec *codec, char *buf, if (!codec->driver->reg_cache_size) return 0;
- if (codec->driver->reg_cache_step) - step = codec->driver->reg_cache_step; - - for (i = 0; i < codec->driver->reg_cache_size; i += step) { - if (codec->readable_register && !codec->readable_register(codec, i)) + for (i = 0; i <= codec->driver->reg_cache_size; i++) { + reg = snd_soc_cache_idx_to_reg(codec, i); + if (codec->readable_register && !codec->readable_register(codec, reg)) continue; if (codec->driver->display_register) { count += codec->driver->display_register(codec, buf + count, - PAGE_SIZE - count, i); + PAGE_SIZE - count, reg); } else { /* only support larger than PAGE_SIZE bytes debugfs * entries for the default case */ if (p >= pos) { if (total + len >= count - 1) break; - format_register_str(codec, i, buf + total, len); + format_register_str(codec, reg, buf + total, len); total += len; } p += len; diff --git a/sound/soc/soc-io.c b/sound/soc/soc-io.c index cca490c..e5c15d3 100644 --- a/sound/soc/soc-io.c +++ b/sound/soc/soc-io.c @@ -35,9 +35,12 @@ static int do_hw_write(struct snd_soc_codec *codec, unsigned int reg, unsigned int value, const void *data, int len) { int ret; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
if (!snd_soc_codec_volatile_register(codec, reg) && - reg < codec->driver->reg_cache_size && + idx < codec->driver->reg_cache_size && !codec->cache_bypass) { ret = snd_soc_cache_write(codec, reg, value); if (ret < 0) @@ -62,8 +65,11 @@ static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) { int ret; unsigned int val; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
- if (reg >= codec->driver->reg_cache_size || + if (idx >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg) || codec->cache_bypass) { if (codec->cache_only)
On Mon, Aug 01, 2011 at 07:38:10PM +0800, Dong Aisheng wrote:
You've done this at the wrong abstraction level...
@@ -62,8 +65,11 @@ static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) { int ret; unsigned int val;
- unsigned int idx;
- idx = snd_soc_cache_reg_to_idx(codec, reg);
- if (reg >= codec->driver->reg_cache_size ||
- if (idx >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg) || codec->cache_bypass) { if (codec->cache_only)
...hw_read() shouldn't need to know about this stuff, and there's no way the rbtree cache should be using step sizes (which you did in the text I deleted) as it will naturally not create the cache entries for registers that don't exist. Whatever we do should be hidden in the flat (and possibly LZO, though I'd be tempted not to bother) cache, plus having a defualt readable_register() would be sensible.
This may mean starting off with some factoring out of legacy code which still assumes flat caches, replacing them with a check that the register is cachable.
The purpose of the step size is to save space in the register cache.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Monday, August 01, 2011 7:52 PM To: Dong Aisheng-B29396 Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
On Mon, Aug 01, 2011 at 07:38:10PM +0800, Dong Aisheng wrote:
You've done this at the wrong abstraction level...
@@ -62,8 +65,11 @@ static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) { int ret; unsigned int val;
- unsigned int idx;
- idx = snd_soc_cache_reg_to_idx(codec, reg);
- if (reg >= codec->driver->reg_cache_size ||
- if (idx >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg) || codec->cache_bypass) { if (codec->cache_only)
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register cache array. Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion? Do I need to implement a help function like reg_is_cachable(reg) to be used here?
and there's no way the rbtree cache should be using step sizes (which you did in the text I deleted) as it will naturally not create the cache entries for registers that don't exist. Whatever we do should be hidden in the flat (and possibly LZO, though I'd be tempted not to bother) cache, plus having a defualt readable_register() would be sensible.
The cache block in each rbnode is still using cache index to fetch value which is similar like flat cache. (see snd_soc_rbtree_get_register & snd_soc_rbtree_set_register). Additionally, the snd_soc_rbtree_cache_init using cache index to do init by default. However, the rbtree_cache_read/write still use reg to index.
If not change it, my test will fail (MX28EVK + sgtl5000). (LZO showed the same result.)
The main problem is that the default cache array is indexed by cache index like cache[idx] while all io function using reg to fetch data. Many places in cache core miss used cache index and reg index.
The purpose of this patch is to use both of them in correct places. The simple rule is converted to cache index to fetch data from cache when do Register io.
This may mean starting off with some factoring out of legacy code which still assumes flat caches, replacing them with a check that the register is cachable.
The purpose of the step size is to save space in the register cache.
Yes.
On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register cache array. Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
I see what you're doing, but like I say this is just legacy code that shouldn't be peering at the cache size any more and layering additional stuff in is just going to make the situation worse.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion?
Yes.
Do I need to implement a help function like reg_is_cachable(reg) to be used here?
No, I think we should hide these decisions completely within the cache code.
The cache block in each rbnode is still using cache index to fetch value which is similar like flat cache. (see snd_soc_rbtree_get_register & snd_soc_rbtree_set_register). Additionally, the snd_soc_rbtree_cache_init using cache index to do init by default. However, the rbtree_cache_read/write still use reg to index.
This doesn't mean it's a good idea to add extra complexity here! A register map with step size greater than one should never have more than one register in a block anyway with the current code so the step size should never be perceptible.
The main problem is that the default cache array is indexed by cache index like cache[idx] while all io function using reg to fetch data. Many places in cache core miss used cache index and reg index.
The purpose of this patch is to use both of them in correct places. The simple rule is converted to cache index to fetch data from cache when do Register io.
We need to deal with this sanely, dancing around with step sizes in every place where we access registers is just not going to be maintainable. Essentially no modern hardware uses step sizes other than one so they'll get very little testing, especially with the advanced cache mechanisms which aren't really useful on older CODECs, and the additional complexity really hurts legibility.
It occurs to me that what we want to do here may be to make a new register cache type for step sizes then hide all the complexity for these devices in there. That keeps everything together and ensures that newer devices don't pay a complexity cost.
For the register default tables it's probably sensible to just pad them out with the zero registers; it'll cost a little space but not too much.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, August 02, 2011 4:39 PM To: Dong Aisheng-B29396 Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register
cache array.
Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
I see what you're doing, but like I say this is just legacy code that shouldn't be peering at the cache size any more and layering additional stuff in is just going to make the situation worse.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion?
Yes.
Do I need to implement a help function like reg_is_cachable(reg) to be
used here?
No, I think we should hide these decisions completely within the cache code.
Yes, but the issue is that hw_read will check if reg is in cache array And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong when the step is not 1 that will cause registers with their index are greater than cache index will not be able to fetch data from cache.
If we high this in cache code, do you mean hide it in snd_soc_cache_read? If that, the hw_read may fail and it looks not as we expected.
@@ -62,8 +65,11 @@ static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) { int ret; unsigned int val; + unsigned int idx; + + idx = snd_soc_cache_reg_to_idx(codec, reg);
- if (reg >= codec->driver->reg_cache_size || + if (idx >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg) || codec->cache_bypass) { if (codec->cache_only)
The cache block in each rbnode is still using cache index to fetch value which is similar like flat cache. (see snd_soc_rbtree_get_register & snd_soc_rbtree_set_register). Additionally, the snd_soc_rbtree_cache_init using cache index to do init by default. However, the rbtree_cache_read/write still use reg to
index.
This doesn't mean it's a good idea to add extra complexity here!
I agree.
A register map with step size greater than one should never have more than one register in a block anyway with the current code so the step size should never be perceptible.
The question is that rbtree uses reg index to index as follows: if (rbnode) { snd_soc_rbtree_get_base_top_reg(rbnode, &base_reg, &top_reg); if (reg >= base_reg && reg <= top_reg) { reg_tmp = reg - base_reg; *value = snd_soc_rbtree_get_register(rbnode, reg_tmp); return 0; } } So the block may be reg0, reg2, reg4..... And the block is flat, the value is get/set by rbnode->block[reg_tmp]: I understand right, there may be hole in the block, right?
The main problem is that the default cache array is indexed by cache index like cache[idx] while all io function using reg to fetch data. Many places in cache core miss used cache index and reg index.
The purpose of this patch is to use both of them in correct places. The simple rule is converted to cache index to fetch data from cache when do Register io.
We need to deal with this sanely, dancing around with step sizes in every place where we access registers is just not going to be maintainable. Essentially no modern hardware uses step sizes other than one so they'll get very little testing, especially with the advanced cache mechanisms which aren't really useful on older CODECs, and the additional complexity really hurts legibility.
Yes, I agree that we should not introduce too much additional complexity. The better case may be that only change the rbtree init code to get correct Register value for the registers. Then the rbtree_cache_read/write can index the register correctly. (I also think the rbtree cache should not know step)
Then the only complexity is checking reg index for flat cache read/write But that is really needed for different cache step of flat cache.
It occurs to me that what we want to do here may be to make a new register cache type for step sizes then hide all the complexity for these devices in there. That keeps everything together and ensures that newer devices don't pay a complexity cost.
If we add new cache type, does it have any big difference with FLAT? Maybe if we can fix the cache step issue, the FLAT cache can be reuse.
For the register default tables it's probably sensible to just pad them out with the zero registers; it'll cost a little space but not too much.
Yes, it's the most easy way now.
At Tue, 2 Aug 2011 09:41:22 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, August 02, 2011 4:39 PM To: Dong Aisheng-B29396 Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register
cache array.
Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
I see what you're doing, but like I say this is just legacy code that shouldn't be peering at the cache size any more and layering additional stuff in is just going to make the situation worse.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion?
Yes.
Do I need to implement a help function like reg_is_cachable(reg) to be
used here?
No, I think we should hide these decisions completely within the cache code.
Yes, but the issue is that hw_read will check if reg is in cache array And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong when the step is not 1 that will cause registers with their index are greater than cache index will not be able to fetch data from cache.
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc-io.c (and other ASoC core) is correct.
That is, the codec drivers setting ARRAY_SIZE() to reg_cache_size with reg_cache_step > 1 are buggy and should be fixed.
thanks,
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 6:34 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
At Tue, 2 Aug 2011 09:41:22 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, August 02, 2011 4:39 PM To: Dong Aisheng-B29396 Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register
cache array.
Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
I see what you're doing, but like I say this is just legacy code that shouldn't be peering at the cache size any more and layering additional stuff in is just going to make the situation worse.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion?
Yes.
Do I need to implement a help function like reg_is_cachable(reg) to be
used here?
No, I think we should hide these decisions completely within the cache code.
Yes, but the issue is that hw_read will check if reg is in cache array And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong when the step is not 1 that will cause registers with their index are greater than cache index will not be able to fetch data from
cache.
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
That is, the codec drivers setting ARRAY_SIZE() to reg_cache_size with reg_cache_step > 1 are buggy and should be fixed.
At Tue, 2 Aug 2011 10:55:25 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 6:34 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
At Tue, 2 Aug 2011 09:41:22 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, August 02, 2011 4:39 PM To: Dong Aisheng-B29396 Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register
cache array.
Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
I see what you're doing, but like I say this is just legacy code that shouldn't be peering at the cache size any more and layering additional stuff in is just going to make the situation worse.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion?
Yes.
Do I need to implement a help function like reg_is_cachable(reg) to be
used here?
No, I think we should hide these decisions completely within the cache code.
Yes, but the issue is that hw_read will check if reg is in cache array And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong when the step is not 1 that will cause registers with their index are greater than cache index will not be able to fetch data from
cache.
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
I'm not sure what is referred here as reg, but the argument passed to snd_soc_{read|write}() is the raw register index. reg = 2 is 2 no matter whether reg_cache_step is 1 or 2. This is passed down to hw_read(), thus reg and reg_cache_size do match even when step > 1.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 7:09 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
I'm not sure what is referred here as reg, but the argument passed to snd_soc_{read|write}() is the raw register index. reg = 2 is 2 no matter whether reg_cache_step is 1 or 2. This is passed down to hw_read(), thus reg and reg_cache_size do match even when step > 1.
Yes, it is raw register index here. The case is that cache array is [reg0, reg1, reg2, reg3], So the size is 4. But when step is 2, reg0 is 0x0, reg1 is 0x2, reg3 is 0x6. So check if reg3 > reg_cache_size is not correct. I mean this mismatch. Am I correct?
Regards Dong Aisheng
At Tue, 2 Aug 2011 11:15:28 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 7:09 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
I'm not sure what is referred here as reg, but the argument passed to snd_soc_{read|write}() is the raw register index. reg = 2 is 2 no matter whether reg_cache_step is 1 or 2. This is passed down to hw_read(), thus reg and reg_cache_size do match even when step > 1.
Yes, it is raw register index here. The case is that cache array is [reg0, reg1, reg2, reg3], So the size is 4. But when step is 2, reg0 is 0x0, reg1 is 0x2, reg3 is 0x6. So check if reg3 > reg_cache_size is not correct. I mean this mismatch. Am I correct?
No, the (flat) cache array held in codec instance contains padding, e.g. [reg0, 0, reg1, 0, reg2, 0, reg3, 0] in the case above. cache_reg_step is there just for avoiding the access to invalid registers in the table-lookup code in soc-*.c.
Thus it's a bug of the driver if it passes reg_cache_default with a packed array with reg_cache_step > 1. If the size matters, we may fix it in soc-core.c to accept packed arrays, but I'm not sure whether it's worth alone.
(For the sparse data like wm8995, it's a different question; Obviously it can be better packed in a form of {index,data} pairs instead of the whole sparse array as initial data. Then we'd need to introduce another type of default-data copier in soc-core.c.)
And, in principle, the driver shouldn't access codec->reg_cache contents directly but use helper functions. Then the most of inconsistency issue should go away.
Takashi
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 8:10 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
At Tue, 2 Aug 2011 11:15:28 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 7:09 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
I'm not sure what is referred here as reg, but the argument passed to snd_soc_{read|write}() is the raw register index. reg = 2 is 2 no matter whether reg_cache_step is 1 or 2. This is passed down to hw_read(), thus reg and reg_cache_size do match even when step > 1.
Yes, it is raw register index here. The case is that cache array is [reg0, reg1, reg2, reg3], So the size is 4. But when step is 2, reg0 is 0x0, reg1 is 0x2, reg3 is 0x6. So check if reg3 > reg_cache_size is not correct. I mean this mismatch. Am I correct?
No, the (flat) cache array held in codec instance contains padding, e.g. [reg0, 0, reg1, 0, reg2, 0, reg3, 0] in the case above. cache_reg_step is there just for avoiding the access to invalid registers in the table-lookup code in soc-*.c.
I saw most codec drivers with cache_reg_step of 2 do not have padding such as wm9705, wm9712 driver(you can check the code). So It looks the cache_reg_step here causes some misleading.
Thus it's a bug of the driver if it passes reg_cache_default with a packed array with reg_cache_step > 1. If the size matters, we may fix it in soc-core.c to accept packed arrays, but I'm not sure whether it's worth alone.
(For the sparse data like wm8995, it's a different question; Obviously it can be better packed in a form of {index,data} pairs instead of the whole sparse array as initial data. Then we'd need to introduce another type of default-data copier in soc-core.c.)
And, in principle, the driver shouldn't access codec->reg_cache contents directly but use helper functions. Then the most of inconsistency issue should go away.
Yes. The problem is that if the cache array is not padded, the snd_soc_read/write may work incorrectly.
At Tue, 2 Aug 2011 12:29:48 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 8:10 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
At Tue, 2 Aug 2011 11:15:28 +0000, Dong Aisheng-B29396 wrote:
-----Original Message----- From: Takashi Iwai [mailto:tiwai@suse.de] Sent: Tuesday, August 02, 2011 7:09 PM To: Dong Aisheng-B29396 Cc: Mark Brown; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc- io.c (and other ASoC core) is correct.
But the reg is related to step. So reg and reg_cache_size are un-match when step > 1, right?
I'm not sure what is referred here as reg, but the argument passed to snd_soc_{read|write}() is the raw register index. reg = 2 is 2 no matter whether reg_cache_step is 1 or 2. This is passed down to hw_read(), thus reg and reg_cache_size do match even when step > 1.
Yes, it is raw register index here. The case is that cache array is [reg0, reg1, reg2, reg3], So the size is 4. But when step is 2, reg0 is 0x0, reg1 is 0x2, reg3 is 0x6. So check if reg3 > reg_cache_size is not correct. I mean this mismatch. Am I correct?
No, the (flat) cache array held in codec instance contains padding, e.g. [reg0, 0, reg1, 0, reg2, 0, reg3, 0] in the case above. cache_reg_step is there just for avoiding the access to invalid registers in the table-lookup code in soc-*.c.
I saw most codec drivers with cache_reg_step of 2 do not have padding such as wm9705, wm9712 driver(you can check the code). So It looks the cache_reg_step here causes some misleading.
Yeah, there have been confusion about the usage of these (I vaguely remember I complained years ago :), and I guess something is significantly broken there now.
Thus it's a bug of the driver if it passes reg_cache_default with a packed array with reg_cache_step > 1. If the size matters, we may fix it in soc-core.c to accept packed arrays, but I'm not sure whether it's worth alone.
(For the sparse data like wm8995, it's a different question; Obviously it can be better packed in a form of {index,data} pairs instead of the whole sparse array as initial data. Then we'd need to introduce another type of default-data copier in soc-core.c.)
And, in principle, the driver shouldn't access codec->reg_cache contents directly but use helper functions. Then the most of inconsistency issue should go away.
Yes. The problem is that if the cache array is not padded, the snd_soc_read/write may work incorrectly.
Well, the problem is that there are inconsistencies in the code. Especially soc-cache.c doesn't consider about reg_cache_step at all (e.g. snd_soc_flat_cache_sync()).
Mark, Liam, could you guys take a deeper look and clean these messes up?
thanks,
Takashi
On Tue, Aug 02, 2011 at 02:52:29PM +0200, Takashi Iwai wrote:
Mark, Liam, could you guys take a deeper look and clean these messes up?
Like I've indicated several times now we should just get rid of the code or hide it from the rest of the subsystem, it's being too cute for vanishingly little value. The register maps for these devices are usually at most 255 registers so the memory savings are really not meaningful. I'm hoping the guys working with this device will find time to look at fixing things, but if not I'd imagine we'll get to it at some point in the release cycle.
At Wed, 3 Aug 2011 00:48:16 +0900, Mark Brown wrote:
On Tue, Aug 02, 2011 at 02:52:29PM +0200, Takashi Iwai wrote:
Mark, Liam, could you guys take a deeper look and clean these messes up?
Like I've indicated several times now we should just get rid of the code or hide it from the rest of the subsystem, it's being too cute for vanishingly little value. The register maps for these devices are usually at most 255 registers so the memory savings are really not meaningful. I'm hoping the guys working with this device will find time to look at fixing things, but if not I'd imagine we'll get to it at some point in the release cycle.
Well, there aren't so many drivers suffering from this bug, so a temporary fix would be easy like below (totally untested).
Takashi
--- diff --git a/sound/soc/codecs/ad1980.c b/sound/soc/codecs/ad1980.c index 923b364..6aea8e2 100644 --- a/sound/soc/codecs/ad1980.c +++ b/sound/soc/codecs/ad1980.c @@ -244,7 +244,7 @@ static int ad1980_soc_remove(struct snd_soc_codec *codec) static struct snd_soc_codec_driver soc_codec_dev_ad1980 = { .probe = ad1980_soc_probe, .remove = ad1980_soc_remove, - .reg_cache_size = ARRAY_SIZE(ad1980_reg), + .reg_cache_size = ARRAY_SIZE(ad1980_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_default = ad1980_reg, .reg_cache_step = 2, diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 76258f2..52e5ba3 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -1437,7 +1437,7 @@ static struct snd_soc_codec_driver sgtl5000_driver = { .suspend = sgtl5000_suspend, .resume = sgtl5000_resume, .set_bias_level = sgtl5000_set_bias_level, - .reg_cache_size = ARRAY_SIZE(sgtl5000_regs), + .reg_cache_size = SGTL5000_MAX_REG_OFFSET, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = sgtl5000_regs, diff --git a/sound/soc/codecs/wm9705.c b/sound/soc/codecs/wm9705.c index 646b58d..7088a89 100644 --- a/sound/soc/codecs/wm9705.c +++ b/sound/soc/codecs/wm9705.c @@ -374,7 +374,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm9705 = { .resume = wm9705_soc_resume, .read = ac97_read, .write = ac97_write, - .reg_cache_size = ARRAY_SIZE(wm9705_reg), + .reg_cache_size = ARRAY_SIZE(wm9705_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = wm9705_reg, diff --git a/sound/soc/codecs/wm9712.c b/sound/soc/codecs/wm9712.c index 90117f8..6149f02 100644 --- a/sound/soc/codecs/wm9712.c +++ b/sound/soc/codecs/wm9712.c @@ -662,7 +662,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm9712 = { .read = ac97_read, .write = ac97_write, .set_bias_level = wm9712_set_bias_level, - .reg_cache_size = ARRAY_SIZE(wm9712_reg), + .reg_cache_size = ARRAY_SIZE(wm9712_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = wm9712_reg, diff --git a/sound/soc/codecs/wm9713.c b/sound/soc/codecs/wm9713.c index 7167cb6..1995671 100644 --- a/sound/soc/codecs/wm9713.c +++ b/sound/soc/codecs/wm9713.c @@ -1245,7 +1245,7 @@ static struct snd_soc_codec_driver soc_codec_dev_wm9713 = { .read = ac97_read, .write = ac97_write, .set_bias_level = wm9713_set_bias_level, - .reg_cache_size = ARRAY_SIZE(wm9713_reg), + .reg_cache_size = ARRAY_SIZE(wm9713_reg) << 1, .reg_word_size = sizeof(u16), .reg_cache_step = 2, .reg_cache_default = wm9713_reg, diff --git a/sound/soc/soc-cache.c b/sound/soc/soc-cache.c index d9f8ade..d999bc8 100644 --- a/sound/soc/soc-cache.c +++ b/sound/soc/soc-cache.c @@ -408,6 +408,7 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) unsigned int val; int i; int ret; + int step = 1;
codec->reg_cache = kmalloc(sizeof *rbtree_ctx, GFP_KERNEL); if (!codec->reg_cache) @@ -421,7 +422,9 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) return 0;
word_size = codec->driver->reg_word_size; - for (i = 0; i < codec->driver->reg_cache_size; ++i) { + if (codec->driver->reg_cache_step) + step = codec->driver->reg_cache_step; + for (i = 0; i < codec->driver->reg_cache_size; i += step) { val = snd_soc_get_cache_val(codec->reg_def_copy, i, word_size); if (!val) @@ -820,9 +823,12 @@ static int snd_soc_flat_cache_sync(struct snd_soc_codec *codec) int ret; const struct snd_soc_codec_driver *codec_drv; unsigned int val; + int step = 1;
codec_drv = codec->driver; - for (i = 0; i < codec_drv->reg_cache_size; ++i) { + if (codec_drv->reg_cache_step) + step = codec_drv->reg_cache_step; + for (i = 0; i < codec_drv->reg_cache_size; i += step) { WARN_ON(codec->writable_register && codec->writable_register(codec, i)); ret = snd_soc_cache_read(codec, i, &val); diff --git a/sound/soc/soc-core.c b/sound/soc/soc-core.c index 83ad8ca..596773c 100644 --- a/sound/soc/soc-core.c +++ b/sound/soc/soc-core.c @@ -3269,12 +3269,25 @@ int snd_soc_register_codec(struct device *dev, * the cache. */ if (codec_drv->reg_cache_default) { - codec->reg_def_copy = kmemdup(codec_drv->reg_cache_default, - reg_size, GFP_KERNEL); + codec->reg_def_copy = kzalloc(reg_size, GFP_KERNEL); if (!codec->reg_def_copy) { ret = -ENOMEM; goto fail; } + if (codec_drv->reg_cache_step > 1) { + /* FIXME: reg_cache_default with step > 1 is + * supposed to be a packed array, so here we + * expand into the flat cache array + */ + int step = codec_drv->reg_cache_step; + int wsize = codec_dev->reg_word_size; + for (i = 0; i < reg_size / step; i += wsize) + memcpy(codec->reg_def_copy + i * step, + codec->reg_cache_default + i, + wsize); + } else + memcpy(codec->reg_def_copy, + codec_drv->reg_cache_default, reg_size); } }
On Tue, Aug 02, 2011 at 06:13:11PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
Like I've indicated several times now we should just get rid of the code or hide it from the rest of the subsystem, it's being too cute for vanishingly little value. The register maps for these devices are usually at most 255 registers so the memory savings are really not meaningful. I'm hoping the guys working with this device will find time to look at fixing things, but if not I'd imagine we'll get to it at some point in the release cycle.
Well, there aren't so many drivers suffering from this bug, so a temporary fix would be easy like below (totally untested).
If we're going to do something like this I'd preserve the driver interface that's there rather than fiddling with their reg_cache_sizes - half the trouble here is that the meaning of that has become a bit slippery, the current code used to be correct.
@@ -421,7 +422,9 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) return 0;
word_size = codec->driver->reg_word_size;
- for (i = 0; i < codec->driver->reg_cache_size; ++i) {
- if (codec->driver->reg_cache_step)
step = codec->driver->reg_cache_step;- for (i = 0; i < codec->driver->reg_cache_size; i += step) { val = snd_soc_get_cache_val(codec->reg_def_copy, i, word_size); if (!val)
I'm also really unhappy with handling this in the complex caches, I'd be much more inclined to just disallow their use with devices with step sizes than to add any complexity to them.
At Wed, 3 Aug 2011 01:40:06 +0900, Mark Brown wrote:
On Tue, Aug 02, 2011 at 06:13:11PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
Like I've indicated several times now we should just get rid of the code or hide it from the rest of the subsystem, it's being too cute for vanishingly little value. The register maps for these devices are usually at most 255 registers so the memory savings are really not meaningful. I'm hoping the guys working with this device will find time to look at fixing things, but if not I'd imagine we'll get to it at some point in the release cycle.
Well, there aren't so many drivers suffering from this bug, so a temporary fix would be easy like below (totally untested).
If we're going to do something like this I'd preserve the driver interface that's there rather than fiddling with their reg_cache_sizes - half the trouble here is that the meaning of that has become a bit slippery, the current code used to be correct.
I don't mind either way as long as it gets fixed in way applicable to stable kernel tree.
@@ -421,7 +422,9 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) return 0;
word_size = codec->driver->reg_word_size;
- for (i = 0; i < codec->driver->reg_cache_size; ++i) {
- if (codec->driver->reg_cache_step)
step = codec->driver->reg_cache_step;- for (i = 0; i < codec->driver->reg_cache_size; i += step) { val = snd_soc_get_cache_val(codec->reg_def_copy, i, word_size); if (!val)
I'm also really unhappy with handling this in the complex caches, I'd be much more inclined to just disallow their use with devices with step sizes than to add any complexity to them.
Yeah, I find it's ugly, too. OTOH, reg_cache_step defines the validity of the register access, so we can't drop it completely. Accessing the odd number of register index is invalid when step=2, for example. So, alternatively, we can put the condition in some generic filter function like readable/writeble things.
thanks,
Takashi
At Wed, 3 Aug 2011 14:23:35 +0900, Mark Brown wrote:
On Tue, Aug 02, 2011 at 08:06:23PM +0200, Takashi Iwai wrote:
example. So, alternatively, we can put the condition in some generic filter function like readable/writeble things.
Yes, I suggested that already, these are just unreadable and unwritable registers :/
BTW, looking through snd_soc_get_reg_access_index() for *_readable() & co , I wonder what would be the merit of using rbtree if this kind of indexed array is present. If the actual value is also kept in that array, we can drop the whole complex stuff like rbtree and lzo.
In addition, the default values aren't necessary to be in a flat array form at all. What we need is a copy function to expand the data appropriately to reg_def_copy. Or, we can consider dropping the flat reg_def_copy since it's just wasteful to keep the flat array for all types unconditionally, but let the codec driver provide an iterator and an look-up or such.
thanks,
Takashi
On Wed, Aug 03, 2011 at 08:20:56AM +0200, Takashi Iwai wrote:
BTW, looking through snd_soc_get_reg_access_index() for *_readable() & co , I wonder what would be the merit of using rbtree if this kind of indexed array is present. If the actual value is also kept in that array, we can drop the whole complex stuff like rbtree and lzo.
I agree, you may have seen me mentioning that we should just disallow the use of advanced caches for the register maps with non-zero steps. There's really very little benefit from them.
In addition, the default values aren't necessary to be in a flat array form at all. What we need is a copy function to expand the data appropriately to reg_def_copy.
Yes, we can do better here. We do need something that's reasonably easy to write in source code, though. I was thinking something like an array of:
{ reg, value }
might do the trick for the sparse devices. Ideally all this code is going to get pushed out into regmap too so other subsystems can use it.
Or, we can consider dropping the flat reg_def_copy since it's just wasteful to keep the flat array for all types unconditionally, but let the codec driver provide an iterator and an look-up or such.
The purpose of that is slightly different, the idea is to let us discard initdata while still being able to reference the defaults at runtime for CODECs that didn't get instantiated in the system. In order to allow the discard to happen we need to take a copy of the defaults table when we probe the device.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Wednesday, August 03, 2011 1:24 PM To: Takashi Iwai Cc: Dong Aisheng-B29396; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 08:06:23PM +0200, Takashi Iwai wrote:
example. So, alternatively, we can put the condition in some generic filter function like readable/writeble things.
Yes, I suggested that already, these are just unreadable and unwritable registers :/
For padded cache array, the unreadable & unwritable function is correct as Takashi indicated accessing the odd number of register index is invalid with step = 2. But for packed cache array...
There's a few confuse on the using of reg_cache_size and reg_cache_step in current kernel. (like some drivers use packed cache while some use padded)
Maybe we need to first unify it.
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Wednesday, August 03, 2011 12:40 AM To: Takashi Iwai Cc: Dong Aisheng-B29396; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 06:13:11PM +0200, Takashi Iwai wrote:
Mark Brown wrote:
Like I've indicated several times now we should just get rid of the code or hide it from the rest of the subsystem, it's being too cute for vanishingly little value. The register maps for these devices are usually at most 255 registers so the memory savings are really not meaningful. I'm hoping the guys working with this device will find time to look at fixing things, but if not I'd imagine we'll get to it at some point in the release cycle.
Well, there aren't so many drivers suffering from this bug, so a temporary fix would be easy like below (totally untested).
If we're going to do something like this I'd preserve the driver interface that's there rather than fiddling with their reg_cache_sizes - half the trouble here is that the meaning of that has become a bit slippery, the current code used to be correct.
Can we fix FLAT first since it is most used and let rbtree and LZO as before?
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Wednesday, August 03, 2011 5:32 PM To: Dong Aisheng-B29396 Cc: Takashi Iwai; alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: Re: [alsa-devel] [PATCH 1/1] ASoC: core: cache index fix
On Wed, Aug 03, 2011 at 07:39:27AM +0000, Dong Aisheng-B29396 wrote:
Can we fix FLAT first since it is most used and let rbtree and LZO as
before?
Leaving rbtree and LZO alone is exactly what I've been saying!
Sorry for not catch your point before.
Do you think the my patch that provides a register and cache index conversion helper function behind for FLAT could be a way to try?
If yes, I can clean my patch to only include FLAT fix and send it out for You to take a look.
Using this method, codec driver does not need to pad the default cache array. During the cache read, the register will be automatically converted to cache index to fetch value. This can also fix the soc_codec_reg_show for codecs who are using packed (FLAT) Cache while step = 2. (Here I assume the packed cache only has a different step with FLAT cache. FIXME if wrong)
Hi,
I am writing an SoC driver. I have the need to power up the Playback and Capture parts of the device separately depending on the call to codec structure. How can I identify the stream as playback or capture in set_bias call back. It only gets a codec pointer and a state.
Regards, Preetam
On Sun, Aug 07, 2011 at 12:31:07AM +0530, preetam wrote:
Don't start a new thread by replying to completely unrelated mails, write a new mail to the list.
I am writing an SoC driver. I have the need to power up the Playback and Capture parts of the device separately depending on the call to codec structure. How can I identify the stream as playback or capture in set_bias call back. It only gets a codec pointer and a state.
You can't. You should be using DAPM for this, not set_bias_level().
On Sunday 07 August 2011 08:32 AM, Mark Brown wrote:
On Sun, Aug 07, 2011 at 12:31:07AM +0530, preetam wrote:
Don't start a new thread by replying to completely unrelated mails, write a new mail to the list.
I am writing an SoC driver. I have the need to power up the Playback and Capture parts of the device separately depending on the call to codec structure. How can I identify the stream as playback or capture in set_bias call back. It only gets a codec pointer and a state.
You can't. You should be using DAPM for this, not set_bias_level().
Then what is the ideal use case of set_bias_level ?
On Sunday 07 August 2011 09:42 AM, preetam wrote:
On Sunday 07 August 2011 08:32 AM, Mark Brown wrote:
On Sun, Aug 07, 2011 at 12:31:07AM +0530, preetam wrote:
Don't start a new thread by replying to completely unrelated mails, write a new mail to the list.
I am writing an SoC driver. I have the need to power up the Playback and Capture parts of the device separately depending on the call to codec structure. How can I identify the stream as playback or capture in set_bias call back. It only gets a codec pointer and a state.
You can't. You should be using DAPM for this, not set_bias_level().
Then what is the ideal use case of set_bias_level ?
I am asking this because, I was using set_bias to enable the clocks to the DAC and ADC blocks. This device has two independednt clock sources for DAC and ADC blocks. I would like to power up only one of them depending on which steam the calls are being made for. So do I do this?
On Sun, Aug 07, 2011 at 09:48:08AM +0530, preetam wrote:
On Sunday 07 August 2011 09:42 AM, preetam wrote:
On Sunday 07 August 2011 08:32 AM, Mark Brown wrote:
Always CC people on kernel related mails.
You can't. You should be using DAPM for this, not set_bias_level().
Then what is the ideal use case of set_bias_level ?
Managing global CODEC power state such as master biases.
I am asking this because, I was using set_bias to enable the clocks to the DAC and ADC blocks. This device has two independednt clock sources for DAC and ADC blocks. I would like to power up only one of them depending on which steam the calls are being made for. So do I do this?
_SUPPLY()
On Tue, Aug 02, 2011 at 12:34:23PM +0200, Takashi Iwai wrote:
reg_cache_size is supposed to be the real size of the cache table. This isn't influenced by reg_cache_step value. So, the behavior in soc-io.c (and other ASoC core) is correct.
That is, the codec drivers setting ARRAY_SIZE() to reg_cache_size with reg_cache_step > 1 are buggy and should be fixed.
Yes, that's probably the best spot fix - I think it should work. But of course really all this code is bit rotten and should be refactored to be more comprehensible so nobody has to worry if it's doing the right thing.
-----Original Message----- From: linux-arm-kernel-bounces@lists.infradead.org [mailto:linux-arm- kernel-bounces@lists.infradead.org] On Behalf Of Dong Aisheng-B29396 Sent: Tuesday, August 02, 2011 5:41 PM To: Mark Brown Cc: alsa-devel@alsa-project.org; s.hauer@pengutronix.de; lrg@ti.com; linux-arm-kernel@lists.infradead.org; w.sang@pengutronix.de Subject: RE: [PATCH 1/1] ASoC: core: cache index fix
-----Original Message----- From: Mark Brown [mailto:broonie@opensource.wolfsonmicro.com] Sent: Tuesday, August 02, 2011 4:39 PM To: Dong Aisheng-B29396 Cc: alsa-devel@alsa-project.org; linux-arm-kernel@lists.infradead.org; lrg@ti.com; s.hauer@pengutronix.de; w.sang@pengutronix.de Subject: Re: [PATCH 1/1] ASoC: core: cache index fix
On Tue, Aug 02, 2011 at 08:03:23AM +0000, Dong Aisheng-B29396 wrote:
...hw_read() shouldn't need to know about this stuff
Here the reg_cache_size is the maximum cache index in the register
cache array.
Therefore, the original code using reg to compare with reg_cache_size is not correct when the reg_cache_step is not 1. e.g. the reg to read is 0x30 and reg_cache_size maybe only 0x16. So we use idx to check if it exceeds the cache size.
I see what you're doing, but like I say this is just legacy code that shouldn't be peering at the cache size any more and layering additional stuff in is just going to make the situation worse.
BTW, do you mean the soc_io layer does not need to know the details of idx® Conversion?
Yes.
Do I need to implement a help function like reg_is_cachable(reg) to be
used here?
No, I think we should hide these decisions completely within the cache code.
Yes, but the issue is that hw_read will check if reg is in cache array And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong when the step is not 1 that will cause registers with their index are greater than cache index will not be able to fetch data from cache.
If we high this in cache code, do you mean hide it in snd_soc_cache_read? If that, the hw_read may fail and it looks not as we expected.
@@ -62,8 +65,11 @@ static unsigned int hw_read(struct snd_soc_codec *codec, unsigned int reg) { int ret; unsigned int val;
- unsigned int idx;
- idx = snd_soc_cache_reg_to_idx(codec, reg);
- if (reg >= codec->driver->reg_cache_size ||
- if (idx >= codec->driver->reg_cache_size || snd_soc_codec_volatile_register(codec, reg) || codec->cache_bypass) { if (codec->cache_only)
The cache block in each rbnode is still using cache index to fetch value which is similar like flat cache. (see snd_soc_rbtree_get_register & snd_soc_rbtree_set_register). Additionally, the snd_soc_rbtree_cache_init using cache index to do init by default. However, the rbtree_cache_read/write still use reg to
index.
This doesn't mean it's a good idea to add extra complexity here!
I agree.
A register map with step size greater than one should never have more than one register in a block anyway with the current code so the step size should never be perceptible.
The question is that rbtree uses reg index to index as follows: if (rbnode) { snd_soc_rbtree_get_base_top_reg(rbnode, &base_reg, &top_reg); if (reg >= base_reg && reg <= top_reg) { reg_tmp = reg - base_reg; *value = snd_soc_rbtree_get_register(rbnode, reg_tmp); return 0; } } So the block may be reg0, reg2, reg4..... And the block is flat, the value is get/set by rbnode->block[reg_tmp]: I understand right, there may be hole in the block, right?
The main problem is that the default cache array is indexed by cache index like cache[idx] while all io function using reg to fetch data. Many places in cache core miss used cache index and reg index.
The purpose of this patch is to use both of them in correct places. The simple rule is converted to cache index to fetch data from cache when do Register io.
We need to deal with this sanely, dancing around with step sizes in every place where we access registers is just not going to be
maintainable.
Essentially no modern hardware uses step sizes other than one so they'll get very little testing, especially with the advanced cache mechanisms which aren't really useful on older CODECs, and the additional complexity really hurts legibility.
Yes, I agree that we should not introduce too much additional complexity.
The better case may be that only change the rbtree init code to get correct Register value for the registers. Then the rbtree_cache_read/write can index the register correctly. (I also think the rbtree cache should not know step)
For rbtree, i tried that only changed snd_soc_rbtree_cache_init as follows Could work. Then rbtree_cache_read/write do not need to care about step. This could reduce many code changes and complexity. But the disadvantage is that the rbtree cache may not be able to find a adjacent register in the same block if the reg step is 2. However it works. Do you think this is acceptable?
@@ -426,7 +465,8 @@ static int snd_soc_rbtree_cache_init(struct snd_soc_codec *codec) word_size); if (!val) continue; - ret = snd_soc_rbtree_cache_write(codec, i, val); + reg = snd_soc_cache_idx_to_reg(codec, i); + ret = snd_soc_rbtree_cache_write(codec, reg, val); if (ret) goto err; }
Then the only complexity is checking reg index for flat cache read/write But that is really needed for different cache step of flat cache.
It occurs to me that what we want to do here may be to make a new register cache type for step sizes then hide all the complexity for these devices in there. That keeps everything together and ensures that newer devices don't pay a complexity cost.
If we add new cache type, does it have any big difference with FLAT? Maybe if we can fix the cache step issue, the FLAT cache can be reuse.
For the register default tables it's probably sensible to just pad them out with the zero registers; it'll cost a little space but not too
much. Yes, it's the most easy way now.
linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
On Tue, Aug 02, 2011 at 01:17:12PM +0000, Dong Aisheng-B29396 wrote:
For rbtree, i tried that only changed snd_soc_rbtree_cache_init as follows Could work. Then rbtree_cache_read/write do not need to care about step. This could reduce many code changes and complexity. But the disadvantage is that the rbtree cache may not be able to find a adjacent register in the same block if the reg step is 2. However it works. Do you think this is acceptable?
No, like I've been saying the rbtree should have *no* visibility of step sizes. This is exactly the sort of complexity and fragility that I've been raising as an issue.
On Tue, Aug 02, 2011 at 09:41:22AM +0000, Dong Aisheng-B29396 wrote:
No, I think we should hide these decisions completely within the cache code.
Yes, but the issue is that hw_read will check if reg is in cache array And checking like " if (reg >= codec->driver->reg_cache_size ||" is wrong when the step is not 1 that will cause registers with their index are greater than cache index will not be able to fetch data from cache.
This is in no way inconsistent with what I'm saying above.
A register map with step size greater than one should never have more than one register in a block anyway with the current code so the step size should never be perceptible.
The question is that rbtree uses reg index to index as follows:
The rbtree should only see registers meaning registers.
if (rbnode) { snd_soc_rbtree_get_base_top_reg(rbnode, &base_reg, &top_reg); if (reg >= base_reg && reg <= top_reg) { reg_tmp = reg - base_reg; *value = snd_soc_rbtree_get_register(rbnode, reg_tmp); return 0; } }
So the block may be reg0, reg2, reg4..... And the block is flat, the value is get/set by rbnode->block[reg_tmp]: I understand right, there may be hole in the block, right?
No. The rbtree is dealing with registers only. The rbtree has no idea about steps, and nor should it.
It occurs to me that what we want to do here may be to make a new register cache type for step sizes then hide all the complexity for these devices in there. That keeps everything together and ensures that newer devices don't pay a complexity cost.
If we add new cache type, does it have any big difference with FLAT? Maybe if we can fix the cache step issue, the FLAT cache can be reuse.
Step size and if you want to keep the defaults also compressed then the defaults size.
It occurs to me that what we want to do here may be to make a new register cache type for step sizes then hide all the complexity for these devices in there. That keeps everything together and ensures that newer devices don't pay a complexity cost.
For the register default tables it's probably sensible to just pad them out with the zero registers; it'll cost a little space but not too much.
So, do I get it right, that my initial patch (attached below again) might be interesting after all, because it makes the register layout truly flat (also makes the driver usable for me). Then, based on that, a new cache type which takes step size into account could be added to the soc-core and the codec driver can be later switched to the new cache type?
Regards,
Wolfram
From: Wolfram Sang w.sang@pengutronix.de Subject: [PATCH 3/3] ASoC: sgtl5000: fix cache handling
Cache handling in this driver is seriously broken. The chip has 16-bit registers, yet their register numbering also increases by 2 per register, i.e. there are only even-numbered registers. The default cache in this driver, though, simply increments register numbers, so it does need some mapping as seen in sgtl5000_restore_regs(), note the '>> 1':
snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL, cache[SGTL5000_CHIP_LINREG_CTRL >> 1]);
That, of course, won't work with snd_soc_update_bits(). (Thus, we won't even notice the missing register 0x1c in the default regs which shifted all follwing registers to wrong values.) Noticed on the MX28EVK where enabling the regulators simply locked up the chip.
Refactor the routines and use a properly sized default_regs array which matches the register layout of the underlying chip. Also saves some code which should make up for the bigger array a little.
Signed-off-by: Wolfram Sang w.sang@pengutronix.de Cc: Dong Aisheng b29396@freescale.com Cc: Zeng Zhaoming b32542@freescale.com Cc: Mark Brown broonie@opensource.wolfsonmicro.com --- sound/soc/codecs/sgtl5000.c | 128 ++++++++++++------------------------------- 1 files changed, 35 insertions(+), 93 deletions(-)
diff --git a/sound/soc/codecs/sgtl5000.c b/sound/soc/codecs/sgtl5000.c index 76258f2..7e4066e 100644 --- a/sound/soc/codecs/sgtl5000.c +++ b/sound/soc/codecs/sgtl5000.c @@ -33,73 +33,31 @@ #define SGTL5000_DAP_REG_OFFSET 0x0100 #define SGTL5000_MAX_REG_OFFSET 0x013A
-/* default value of sgtl5000 registers except DAP */ -static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET >> 1] = { - 0xa011, /* 0x0000, CHIP_ID. 11 stand for revison 17 */ - 0x0000, /* 0x0002, CHIP_DIG_POWER. */ - 0x0008, /* 0x0004, CHIP_CKL_CTRL */ - 0x0010, /* 0x0006, CHIP_I2S_CTRL */ - 0x0000, /* 0x0008, reserved */ - 0x0008, /* 0x000A, CHIP_SSS_CTRL */ - 0x0000, /* 0x000C, reserved */ - 0x020c, /* 0x000E, CHIP_ADCDAC_CTRL */ - 0x3c3c, /* 0x0010, CHIP_DAC_VOL */ - 0x0000, /* 0x0012, reserved */ - 0x015f, /* 0x0014, CHIP_PAD_STRENGTH */ - 0x0000, /* 0x0016, reserved */ - 0x0000, /* 0x0018, reserved */ - 0x0000, /* 0x001A, reserved */ - 0x0000, /* 0x001E, reserved */ - 0x0000, /* 0x0020, CHIP_ANA_ADC_CTRL */ - 0x1818, /* 0x0022, CHIP_ANA_HP_CTRL */ - 0x0111, /* 0x0024, CHIP_ANN_CTRL */ - 0x0000, /* 0x0026, CHIP_LINREG_CTRL */ - 0x0000, /* 0x0028, CHIP_REF_CTRL */ - 0x0000, /* 0x002A, CHIP_MIC_CTRL */ - 0x0000, /* 0x002C, CHIP_LINE_OUT_CTRL */ - 0x0404, /* 0x002E, CHIP_LINE_OUT_VOL */ - 0x7060, /* 0x0030, CHIP_ANA_POWER */ - 0x5000, /* 0x0032, CHIP_PLL_CTRL */ - 0x0000, /* 0x0034, CHIP_CLK_TOP_CTRL */ - 0x0000, /* 0x0036, CHIP_ANA_STATUS */ - 0x0000, /* 0x0038, reserved */ - 0x0000, /* 0x003A, CHIP_ANA_TEST2 */ - 0x0000, /* 0x003C, CHIP_SHORT_CTRL */ - 0x0000, /* reserved */ -}; - -/* default value of dap registers */ -static const u16 sgtl5000_dap_regs[] = { - 0x0000, /* 0x0100, DAP_CONTROL */ - 0x0000, /* 0x0102, DAP_PEQ */ - 0x0040, /* 0x0104, DAP_BASS_ENHANCE */ - 0x051f, /* 0x0106, DAP_BASS_ENHANCE_CTRL */ - 0x0000, /* 0x0108, DAP_AUDIO_EQ */ - 0x0040, /* 0x010A, DAP_SGTL_SURROUND */ - 0x0000, /* 0x010C, DAP_FILTER_COEF_ACCESS */ - 0x0000, /* 0x010E, DAP_COEF_WR_B0_MSB */ - 0x0000, /* 0x0110, DAP_COEF_WR_B0_LSB */ - 0x0000, /* 0x0112, reserved */ - 0x0000, /* 0x0114, reserved */ - 0x002f, /* 0x0116, DAP_AUDIO_EQ_BASS_BAND0 */ - 0x002f, /* 0x0118, DAP_AUDIO_EQ_BAND0 */ - 0x002f, /* 0x011A, DAP_AUDIO_EQ_BAND2 */ - 0x002f, /* 0x011C, DAP_AUDIO_EQ_BAND3 */ - 0x002f, /* 0x011E, DAP_AUDIO_EQ_TREBLE_BAND4 */ - 0x8000, /* 0x0120, DAP_MAIN_CHAN */ - 0x0000, /* 0x0122, DAP_MIX_CHAN */ - 0x0510, /* 0x0124, DAP_AVC_CTRL */ - 0x1473, /* 0x0126, DAP_AVC_THRESHOLD */ - 0x0028, /* 0x0128, DAP_AVC_ATTACK */ - 0x0050, /* 0x012A, DAP_AVC_DECAY */ - 0x0000, /* 0x012C, DAP_COEF_WR_B1_MSB */ - 0x0000, /* 0x012E, DAP_COEF_WR_B1_LSB */ - 0x0000, /* 0x0130, DAP_COEF_WR_B2_MSB */ - 0x0000, /* 0x0132, DAP_COEF_WR_B2_LSB */ - 0x0000, /* 0x0134, DAP_COEF_WR_A1_MSB */ - 0x0000, /* 0x0136, DAP_COEF_WR_A1_LSB */ - 0x0000, /* 0x0138, DAP_COEF_WR_A2_MSB */ - 0x0000, /* 0x013A, DAP_COEF_WR_A2_LSB */ +/* default value of sgtl5000 registers */ +static const u16 sgtl5000_regs[SGTL5000_MAX_REG_OFFSET] = { + [SGTL5000_CHIP_CLK_CTRL] = 0x0008, + [SGTL5000_CHIP_I2S_CTRL] = 0x0010, + [SGTL5000_CHIP_SSS_CTRL] = 0x0008, + [SGTL5000_CHIP_DAC_VOL] = 0x3c3c, + [SGTL5000_CHIP_PAD_STRENGTH] = 0x015f, + [SGTL5000_CHIP_ANA_HP_CTRL] = 0x1818, + [SGTL5000_CHIP_ANA_CTRL] = 0x0111, + [SGTL5000_CHIP_LINE_OUT_VOL] = 0x0404, + [SGTL5000_CHIP_ANA_POWER] = 0x7060, + [SGTL5000_CHIP_PLL_CTRL] = 0x5000, + [SGTL5000_DAP_BASS_ENHANCE] = 0x0040, + [SGTL5000_DAP_BASS_ENHANCE_CTRL] = 0x051f, + [SGTL5000_DAP_SURROUND] = 0x0040, + [SGTL5000_DAP_EQ_BASS_BAND0] = 0x002f, + [SGTL5000_DAP_EQ_BASS_BAND1] = 0x002f, + [SGTL5000_DAP_EQ_BASS_BAND2] = 0x002f, + [SGTL5000_DAP_EQ_BASS_BAND3] = 0x002f, + [SGTL5000_DAP_EQ_BASS_BAND4] = 0x002f, + [SGTL5000_DAP_MAIN_CHAN] = 0x8000, + [SGTL5000_DAP_AVC_CTRL] = 0x0510, + [SGTL5000_DAP_AVC_THRESHOLD] = 0x1473, + [SGTL5000_DAP_AVC_ATTACK] = 0x0028, + [SGTL5000_DAP_AVC_DECAY] = 0x0050, };
/* regulator supplies for sgtl5000, VDDD is an optional external supply */ @@ -1023,12 +981,10 @@ static int sgtl5000_suspend(struct snd_soc_codec *codec, pm_message_t state) static int sgtl5000_restore_regs(struct snd_soc_codec *codec) { u16 *cache = codec->reg_cache; - int i; - int regular_regs = SGTL5000_CHIP_SHORT_CTRL >> 1; + u16 reg;
/* restore regular registers */ - for (i = 0; i < regular_regs; i++) { - int reg = i << 1; + for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) {
/* this regs depends on the others */ if (reg == SGTL5000_CHIP_ANA_POWER || @@ -1038,35 +994,31 @@ static int sgtl5000_restore_regs(struct snd_soc_codec *codec) reg == SGTL5000_CHIP_CLK_CTRL) continue;
- snd_soc_write(codec, reg, cache[i]); + snd_soc_write(codec, reg, cache[reg]); }
/* restore dap registers */ - for (i = SGTL5000_DAP_REG_OFFSET >> 1; - i < SGTL5000_MAX_REG_OFFSET >> 1; i++) { - int reg = i << 1; - - snd_soc_write(codec, reg, cache[i]); - } + for (reg = SGTL5000_DAP_REG_OFFSET; reg < SGTL5000_MAX_REG_OFFSET; reg += 2) + snd_soc_write(codec, reg, cache[reg]);
/* * restore power and other regs according * to set_power() and set_clock() */ snd_soc_write(codec, SGTL5000_CHIP_LINREG_CTRL, - cache[SGTL5000_CHIP_LINREG_CTRL >> 1]); + cache[SGTL5000_CHIP_LINREG_CTRL]);
snd_soc_write(codec, SGTL5000_CHIP_ANA_POWER, - cache[SGTL5000_CHIP_ANA_POWER >> 1]); + cache[SGTL5000_CHIP_ANA_POWER]);
snd_soc_write(codec, SGTL5000_CHIP_CLK_CTRL, - cache[SGTL5000_CHIP_CLK_CTRL >> 1]); + cache[SGTL5000_CHIP_CLK_CTRL]);
snd_soc_write(codec, SGTL5000_CHIP_REF_CTRL, - cache[SGTL5000_CHIP_REF_CTRL >> 1]); + cache[SGTL5000_CHIP_REF_CTRL]);
snd_soc_write(codec, SGTL5000_CHIP_LINE_OUT_CTRL, - cache[SGTL5000_CHIP_LINE_OUT_CTRL >> 1]); + cache[SGTL5000_CHIP_LINE_OUT_CTRL]); return 0; }
@@ -1454,16 +1406,6 @@ static __devinit int sgtl5000_i2c_probe(struct i2c_client *client, if (!sgtl5000) return -ENOMEM;
- /* - * copy DAP default values to default value array. - * sgtl5000 register space has a big hole, merge it - * at init phase makes life easy. - * FIXME: should we drop 'const' of sgtl5000_regs? - */ - memcpy((void *)(&sgtl5000_regs[0] + (SGTL5000_DAP_REG_OFFSET >> 1)), - sgtl5000_dap_regs, - SGTL5000_MAX_REG_OFFSET - SGTL5000_DAP_REG_OFFSET); - i2c_set_clientdata(client, sgtl5000);
ret = snd_soc_register_codec(&client->dev,
At Tue, 2 Aug 2011 11:51:41 +0200, Wolfram Sang wrote:
@@ -1023,12 +981,10 @@ static int sgtl5000_suspend(struct snd_soc_codec *codec, pm_message_t state) static int sgtl5000_restore_regs(struct snd_soc_codec *codec) { u16 *cache = codec->reg_cache;
- int i;
- int regular_regs = SGTL5000_CHIP_SHORT_CTRL >> 1;
u16 reg;
/* restore regular registers */
- for (i = 0; i < regular_regs; i++) {
int reg = i << 1;
- for (reg = 0; reg <= SGTL5000_CHIP_SHORT_CTRL; reg += 2) {
Heh, this exposed another bug in the original code :) It should have been for (i = 0; i <= regular_regs; i++) {
thanks,
Takashi
On Tue, Aug 02, 2011 at 11:51:41AM +0200, Wolfram Sang wrote:
So, do I get it right, that my initial patch (attached below again) might be interesting after all, because it makes the register layout truly flat (also makes the driver usable for me). Then, based on that, a new cache type which takes step size into account could be added to the soc-core and the codec driver can be later switched to the new cache type?
Yes, this is entirely sensible. Even if there are issues with this approach they're kept local to the CODEC driver. Please send normally for review.
participants (6)
-
Dong Aisheng -
Dong Aisheng-B29396
-
Mark Brown -
preetam -
Takashi Iwai -
Wolfram Sang