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

Commit Message

Aisheng Dong Aug. 1, 2011, 11:38 a.m. UTC

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 <[e-mail]>
Cc: Mark Brown <[e-mail]>
Cc: Liam Girdwood <[e-mail]>
Cc: Sascha Hauer <[e-mail]>
Cc: Wolfram Sang <[e-mail]>
---
 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(-)

Comments

Mark Brown Aug. 1, 2011, 11:51 a.m. UTC | #1

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.

Aisheng Dong Aug. 2, 2011, 8:03 a.m. UTC | #2

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Monday, August 01, 2011 7:52 PM
> To: Dong Aisheng-B29396
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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.

Mark Brown Aug. 2, 2011, 8:38 a.m. UTC | #3

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.

Aisheng Dong Aug. 2, 2011, 9:41 a.m. UTC | #4

> -----Original Message-----
> From: Mark Brown [mailto:[e-mail]]
> Sent: Tuesday, August 02, 2011 4:39 PM
> To: Dong Aisheng-B29396
> Cc: [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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.

Takashi Iwai Aug. 2, 2011, 10:34 a.m. UTC | #5

At Tue, 2 Aug 2011 09:41:22 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:[e-mail]]
> > Sent: Tuesday, August 02, 2011 4:39 PM
> > To: Dong Aisheng-B29396
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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

Aisheng Dong Aug. 2, 2011, 10:55 a.m. UTC | #6

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Tuesday, August 02, 2011 6:34 PM
> To: Dong Aisheng-B29396
> Cc: Mark Brown; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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:[e-mail]]
> > > Sent: Tuesday, August 02, 2011 4:39 PM
> > > To: Dong Aisheng-B29396
> > > Cc: [e-mail];
> > > [e-mail];
> > > [e-mail]; [e-mail]; [e-mail]
> > > 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.

Takashi Iwai Aug. 2, 2011, 11:09 a.m. UTC | #7

At Tue, 2 Aug 2011 10:55:25 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Tuesday, August 02, 2011 6:34 PM
> > To: Dong Aisheng-B29396
> > Cc: Mark Brown; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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:[e-mail]]
> > > > Sent: Tuesday, August 02, 2011 4:39 PM
> > > > To: Dong Aisheng-B29396
> > > > Cc: [e-mail];
> > > > [e-mail];
> > > > [e-mail]; [e-mail]; [e-mail]
> > > > 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

Aisheng Dong Aug. 2, 2011, 11:15 a.m. UTC | #8

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Tuesday, August 02, 2011 7:09 PM
> To: Dong Aisheng-B29396
> Cc: Mark Brown; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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

Takashi Iwai Aug. 2, 2011, 12:10 p.m. UTC | #9

At Tue, 2 Aug 2011 11:15:28 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Tuesday, August 02, 2011 7:09 PM
> > To: Dong Aisheng-B29396
> > Cc: Mark Brown; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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

Aisheng Dong Aug. 2, 2011, 12:29 p.m. UTC | #10

> -----Original Message-----
> From: Takashi Iwai [mailto:[e-mail]]
> Sent: Tuesday, August 02, 2011 8:10 PM
> To: Dong Aisheng-B29396
> Cc: Mark Brown; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; [e-mail]
> 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:[e-mail]]
> > > Sent: Tuesday, August 02, 2011 7:09 PM
> > > To: Dong Aisheng-B29396
> > > Cc: Mark Brown; [e-mail]; [e-mail];
> > > [e-mail]; [e-mail];
> > > [e-mail]
> > > 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.

Takashi Iwai Aug. 2, 2011, 12:52 p.m. UTC | #11

At Tue, 2 Aug 2011 12:29:48 +0000,
Dong Aisheng-B29396 wrote:
> 
> > -----Original Message-----
> > From: Takashi Iwai [mailto:[e-mail]]
> > Sent: Tuesday, August 02, 2011 8:10 PM
> > To: Dong Aisheng-B29396
> > Cc: Mark Brown; [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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:[e-mail]]
> > > > Sent: Tuesday, August 02, 2011 7:09 PM
> > > > To: Dong Aisheng-B29396
> > > > Cc: Mark Brown; [e-mail]; [e-mail];
> > > > [e-mail]; [e-mail];
> > > > [e-mail]
> > > > 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

Mark Brown Aug. 2, 2011, 12:58 p.m. UTC | #12

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.

Aisheng Dong Aug. 2, 2011, 1:17 p.m. UTC | #13

> -----Original Message-----
> From: [e-mail] [mailto:linux-arm-
> [e-mail]] On Behalf Of Dong Aisheng-B29396
> Sent: Tuesday, August 02, 2011 5:41 PM
> To: Mark Brown
> Cc: [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]
> Subject: RE: [PATCH 1/1] ASoC: core: cache index fix
> 
> > -----Original Message-----
> > From: Mark Brown [mailto:[e-mail]]
> > Sent: Tuesday, August 02, 2011 4:39 PM
> > To: Dong Aisheng-B29396
> > Cc: [e-mail]; [e-mail];
> > [e-mail]; [e-mail]; [e-mail]
> > 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
> [e-mail]
> http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Mark Brown Aug. 2, 2011, 3:27 p.m. UTC | #14

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.

Mark Brown Aug. 2, 2011, 3:48 p.m. UTC | #15

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.

Mark Brown Aug. 2, 2011, 4:05 p.m. UTC | #16

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

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)