[PATCH 1/1] ASoC: core: cache index fix

[PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng]

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

[PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: Monday, August 01, 2011 7:52 PM
> To: Dong Aisheng-B29396
> Cc: alsa-devel at alsa-project.org; linux-arm-kernel at lists.infradead.org;
> lrg at ti.com; s.hauer at pengutronix.de; w.sang at 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&reg
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.
	

[PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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

[PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: Tuesday, August 02, 2011 4:39 PM
> To: Dong Aisheng-B29396
> Cc: alsa-devel at alsa-project.org; linux-arm-kernel at lists.infradead.org;
> lrg at ti.com; s.hauer at pengutronix.de; w.sang at 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&reg 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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

At Tue, 2 Aug 2011 09:41:22 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> > Sent: Tuesday, August 02, 2011 4:39 PM
> > To: Dong Aisheng-B29396
> > Cc: alsa-devel at alsa-project.org; linux-arm-kernel at lists.infradead.org;
> > lrg at ti.com; s.hauer at pengutronix.de; w.sang at 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&reg 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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, August 02, 2011 6:34 PM
> To: Dong Aisheng-B29396
> Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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 at opensource.wolfsonmicro.com]
> > > Sent: Tuesday, August 02, 2011 4:39 PM
> > > To: Dong Aisheng-B29396
> > > Cc: alsa-devel at alsa-project.org;
> > > linux-arm-kernel at lists.infradead.org;
> > > lrg at ti.com; s.hauer at pengutronix.de; w.sang at 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&reg 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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

At Tue, 2 Aug 2011 10:55:25 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, August 02, 2011 6:34 PM
> > To: Dong Aisheng-B29396
> > Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> > lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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 at opensource.wolfsonmicro.com]
> > > > Sent: Tuesday, August 02, 2011 4:39 PM
> > > > To: Dong Aisheng-B29396
> > > > Cc: alsa-devel at alsa-project.org;
> > > > linux-arm-kernel at lists.infradead.org;
> > > > lrg at ti.com; s.hauer at pengutronix.de; w.sang at 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&reg 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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, August 02, 2011 7:09 PM
> To: Dong Aisheng-B29396
> Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

At Tue, 2 Aug 2011 11:15:28 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, August 02, 2011 7:09 PM
> > To: Dong Aisheng-B29396
> > Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> > lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Takashi Iwai [mailto:tiwai at suse.de]
> Sent: Tuesday, August 02, 2011 8:10 PM
> To: Dong Aisheng-B29396
> Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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 at suse.de]
> > > Sent: Tuesday, August 02, 2011 7:09 PM
> > > To: Dong Aisheng-B29396
> > > Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> > > lrg at ti.com; linux-arm-kernel at lists.infradead.org;
> > > w.sang at 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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

At Tue, 2 Aug 2011 12:29:48 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:tiwai at suse.de]
> > Sent: Tuesday, August 02, 2011 8:10 PM
> > To: Dong Aisheng-B29396
> > Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> > lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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 at suse.de]
> > > > Sent: Tuesday, August 02, 2011 7:09 PM
> > > > To: Dong Aisheng-B29396
> > > > Cc: Mark Brown; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> > > > lrg at ti.com; linux-arm-kernel at lists.infradead.org;
> > > > w.sang at 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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

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);
 		}
 	}
 

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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 :/

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

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

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: Wednesday, August 03, 2011 1:24 PM
> To: Takashi Iwai
> Cc: Dong Aisheng-B29396; alsa-devel at alsa-project.org;
> s.hauer at pengutronix.de; lrg at ti.com; linux-arm-kernel at lists.infradead.org;
> w.sang at 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.

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: Wednesday, August 03, 2011 12:40 AM
> To: Takashi Iwai
> Cc: Dong Aisheng-B29396; alsa-devel at alsa-project.org;
> s.hauer at pengutronix.de; lrg at ti.com; linux-arm-kernel at lists.infradead.org;
> w.sang at 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?

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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!

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> Sent: Wednesday, August 03, 2011 5:32 PM
> To: Dong Aisheng-B29396
> Cc: Takashi Iwai; alsa-devel at alsa-project.org; s.hauer at pengutronix.de;
> lrg at ti.com; linux-arm-kernel at lists.infradead.org; w.sang at 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)

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[PATCH 1/1] ASoC: core: cache index fix

[Dong Aisheng-B29396]

> -----Original Message-----
> From: linux-arm-kernel-bounces at lists.infradead.org [mailto:linux-arm-
> kernel-bounces at lists.infradead.org] On Behalf Of Dong Aisheng-B29396
> Sent: Tuesday, August 02, 2011 5:41 PM
> To: Mark Brown
> Cc: alsa-devel at alsa-project.org; s.hauer at pengutronix.de; lrg at ti.com;
> linux-arm-kernel at lists.infradead.org; w.sang at pengutronix.de
> Subject: RE: [PATCH 1/1] ASoC: core: cache index fix
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:broonie at opensource.wolfsonmicro.com]
> > Sent: Tuesday, August 02, 2011 4:39 PM
> > To: Dong Aisheng-B29396
> > Cc: alsa-devel at alsa-project.org; linux-arm-kernel at lists.infradead.org;
> > lrg at ti.com; s.hauer at pengutronix.de; w.sang at 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&reg 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 at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.

[PATCH 1/1] ASoC: core: cache index fix

[Wolfram Sang]

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

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20110802/2e5dc5cd/attachment.sig>

[alsa-devel] [PATCH 1/1] ASoC: core: cache index fix

[Takashi Iwai]

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

[PATCH 1/1] ASoC: core: cache index fix

[Mark Brown]

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.