[PATCH 1/2] regmap: Return a sensible error code if we fail to read the cache

[PATCH 1/2] regmap: Return a sensible error code if we fail to read the cache

[Mark Brown]

If a register isn't cached then let callers know that so they can fall
back or error handle appropriately.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache-indexed.c |    7 +++----
 drivers/base/regmap/regcache-lzo.c     |    4 ++--
 drivers/base/regmap/regcache-rbtree.c  |    3 +--
 drivers/base/regmap/regcache.c         |    2 +-
 4 files changed, 7 insertions(+), 9 deletions(-)

diff --git a/drivers/base/regmap/regcache-indexed.c b/drivers/base/regmap/regcache-indexed.c
index 2e10bb1..507731a 100644
--- a/drivers/base/regmap/regcache-indexed.c
+++ b/drivers/base/regmap/regcache-indexed.c
@@ -20,11 +20,10 @@ static int regcache_indexed_read(struct regmap *map, unsigned int reg,
 	int ret;
 
 	ret = regcache_lookup_reg(map, reg);
-	if (ret < 0)
-		*value = 0;
-	else
+	if (ret >= 0)
 		*value = map->reg_defaults[ret].def;
-	return 0;
+
+	return ret;
 }
 
 static int regcache_indexed_write(struct regmap *map, unsigned int reg,
diff --git a/drivers/base/regmap/regcache-lzo.c b/drivers/base/regmap/regcache-lzo.c
index ad6af92..066aeec 100644
--- a/drivers/base/regmap/regcache-lzo.c
+++ b/drivers/base/regmap/regcache-lzo.c
@@ -232,7 +232,6 @@ static int regcache_lzo_read(struct regmap *map,
 	size_t blksize, tmp_dst_len;
 	void *tmp_dst;
 
-	*value = 0;
 	/* index of the compressed lzo block */
 	blkindex = regcache_lzo_get_blkindex(map, reg);
 	/* register index within the decompressed block */
@@ -261,7 +260,8 @@ static int regcache_lzo_read(struct regmap *map,
 	/* restore the pointer and length of the compressed block */
 	lzo_block->dst = tmp_dst;
 	lzo_block->dst_len = tmp_dst_len;
-	return 0;
+
+	return ret;
 }
 
 static int regcache_lzo_write(struct regmap *map,
diff --git a/drivers/base/regmap/regcache-rbtree.c b/drivers/base/regmap/regcache-rbtree.c
index f80eebc..e314984 100644
--- a/drivers/base/regmap/regcache-rbtree.c
+++ b/drivers/base/regmap/regcache-rbtree.c
@@ -193,8 +193,7 @@ static int regcache_rbtree_read(struct regmap *map,
 		*value = regcache_rbtree_get_register(rbnode, reg_tmp,
 						      map->cache_word_size);
 	} else {
-		/* uninitialized registers default to 0 */
-		*value = 0;
+		return -ENOENT;
 	}
 
 	return 0;
diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index bf82783..67c7703 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -379,7 +379,7 @@ int regcache_lookup_reg(struct regmap *map, unsigned int reg)
 	if (r)
 		return r - map->reg_defaults;
 	else
-		return -1;
+		return -ENOENT;
 }
 
 int regcache_insert_reg(struct regmap *map, unsigned int reg,
-- 
1.7.6.3

[PATCH 2/2] regmap: Allow caches for devices with no defaults

[Mark Brown]

We only really need the defaults in order to cut down the number of
registers we sync and to satisfy reads while the device is powered off
but not all devices are going to need to do that (always on devices like
PMICs being the prime example) so don't require those devices to supply
a default.  Instead only try to fall back to hardware defaults if the
driver told us to.

Devices using LZO won't be able to instantiate with this, that will require
some updates in the LZO code to handle this case.

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---
 drivers/base/regmap/regcache.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
index 67c7703..666f6f5 100644
--- a/drivers/base/regmap/regcache.c
+++ b/drivers/base/regmap/regcache.c
@@ -120,7 +120,7 @@ int regcache_init(struct regmap *map)
 		if (!tmp_buf)
 			return -ENOMEM;
 		map->reg_defaults = tmp_buf;
-	} else {
+	} else if (map->num_reg_defaults_raw) {
 		/* Some devices such as PMICs don't have cache defaults,
 		 * we cope with this by reading back the HW registers and
 		 * crafting the cache defaults by hand.
-- 
1.7.6.3

Re: [PATCH 2/2] regmap: Allow caches for devices with no defaults

[Valdis.Kletnieks]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

On Sun, 09 Oct 2011 14:44:36 BST, Mark Brown said:
> We only really need the defaults in order to cut down the number of
> registers we sync and to satisfy reads while the device is powered off

So let me get this straight - we cache the values so that if there's a read,
we can return our best guess as to what the device would return if it was
in fact powered on, instead of notifying the caller that the device is off?

I think I just sprained my brain trying to parse the logic there :)

[-- Attachment #2: Type: application/pgp-signature, Size: 227 bytes --]

Re: [PATCH 2/2] regmap: Allow caches for devices with no defaults

[Mark Brown]

On Sun, Oct 09, 2011 at 01:43:42PM -0400, Valdis.Kletnieks@vt.edu wrote:
> On Sun, 09 Oct 2011 14:44:36 BST, Mark Brown said:

> > We only really need the defaults in order to cut down the number of
> > registers we sync and to satisfy reads while the device is powered off

> So let me get this straight - we cache the values so that if there's a read,
> we can return our best guess as to what the device would return if it was
> in fact powered on, instead of notifying the caller that the device is off?

> I think I just sprained my brain trying to parse the logic there :)

It's not a guess, it's very solid information which comes from a
combination of knowing the default register values, knowing that the
registers we cache won't be changed except by writes and caching writes.
We're not really doing the caching for this particular feature, having a
cache is really useful anyway and once you're doing register caching
it's a small step to be able to skip the actual device interaction.

Having a register cache is useful where you don't need much abstraction
between the register and the upper layers that talk to the driver - you
can end up with large chunks of driver which are just data tables saying
"this bitfield in this register has these semantics" which is obviously
a substantial win.

However, not all devices have readback support (on older analogue
processes digital was very expensive and there were a few other issues
for some buses) and this can get very read heavy (both from
read/modify/write cycles and from upper layers querying state) so when
your device is on a slow bus it's really useful to be able to cache the
register values rather than actually read from the device.

Once you've got a cache it's a simple matter to skip the step where we
actually interact with the device for non-volatile registers.  In
situations where the device is idle and we've got the sort of thin
mapping described above we don't really want to have to keep the device
powered on just for the mapping and the register values are about all
the abstraction we need so we don't want another layer. If there a lot
of updates being made this can give a further performance boost as upper
layers can update things in any order they see fit as often as they like
and then we can coalesce the actual writes into a minimal subset when
powering the device up for actual use.

For a concrete example think of an audio CODEC - there's lots of
controls like volume settings which can be mapped very directly to what
users interact with and which have no practical impact unless the device
is actually doing something other than to store the setting which will
be used then.

Re: [PATCH 2/2] regmap: Allow caches for devices with no defaults

[Dimitris Papastamos]

On Sun, Oct 09, 2011 at 02:44:36PM +0100, Mark Brown wrote:
> We only really need the defaults in order to cut down the number of
> registers we sync and to satisfy reads while the device is powered off
> but not all devices are going to need to do that (always on devices like
> PMICs being the prime example) so don't require those devices to supply
> a default.  Instead only try to fall back to hardware defaults if the
> driver told us to.
> 
> Devices using LZO won't be able to instantiate with this, that will require
> some updates in the LZO code to handle this case.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/base/regmap/regcache.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
> index 67c7703..666f6f5 100644
> --- a/drivers/base/regmap/regcache.c
> +++ b/drivers/base/regmap/regcache.c
> @@ -120,7 +120,7 @@ int regcache_init(struct regmap *map)
>  		if (!tmp_buf)
>  			return -ENOMEM;
>  		map->reg_defaults = tmp_buf;
> -	} else {
> +	} else if (map->num_reg_defaults_raw) {
>  		/* Some devices such as PMICs don't have cache defaults,
>  		 * we cope with this by reading back the HW registers and
>  		 * crafting the cache defaults by hand.
> -- 
> 1.7.6.3

We could probably then also remove the first check in
regcache_hw_init() I guess.

The one:

if (!map->num_reg_defaults_raw)
	return -EINVAL;

Thanks,
Dimitris

Re: [PATCH 1/2] regmap: Return a sensible error code if we fail to read the cache

[Dimitris Papastamos]

On Sun, Oct 09, 2011 at 02:44:35PM +0100, Mark Brown wrote:
> If a register isn't cached then let callers know that so they can fall
> back or error handle appropriately.
> 
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>  drivers/base/regmap/regcache-indexed.c |    7 +++----
>  drivers/base/regmap/regcache-lzo.c     |    4 ++--
>  drivers/base/regmap/regcache-rbtree.c  |    3 +--
>  drivers/base/regmap/regcache.c         |    2 +-
>  4 files changed, 7 insertions(+), 9 deletions(-)

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

Re: [PATCH 2/2] regmap: Allow caches for devices with no defaults

[Mark Brown]

On Mon, Oct 10, 2011 at 09:41:26AM +0100, Dimitris Papastamos wrote:
> On Sun, Oct 09, 2011 at 02:44:36PM +0100, Mark Brown wrote:

> > +	} else if (map->num_reg_defaults_raw) {
> >  		/* Some devices such as PMICs don't have cache defaults,
> >  		 * we cope with this by reading back the HW registers and
> >  		 * crafting the cache defaults by hand.

> We could probably then also remove the first check in
> regcache_hw_init() I guess.

> The one:

> if (!map->num_reg_defaults_raw)
> 	return -EINVAL;

I felt it was clearer to have the defaults init fail if it doesn't work
so that the callers know what happened.  Otherwise the callers have to
figure out if it succeeded because it did what it was supposed to.

Re: [PATCH 2/2] regmap: Allow caches for devices with no defaults

[Dimitris Papastamos]

On Mon, Oct 10, 2011 at 10:23:46AM +0100, Mark Brown wrote:
> On Mon, Oct 10, 2011 at 09:41:26AM +0100, Dimitris Papastamos wrote:
> > On Sun, Oct 09, 2011 at 02:44:36PM +0100, Mark Brown wrote:
> 
> > > +	} else if (map->num_reg_defaults_raw) {
> > >  		/* Some devices such as PMICs don't have cache defaults,
> > >  		 * we cope with this by reading back the HW registers and
> > >  		 * crafting the cache defaults by hand.
> 
> > We could probably then also remove the first check in
> > regcache_hw_init() I guess.
> 
> > The one:
> 
> > if (!map->num_reg_defaults_raw)
> > 	return -EINVAL;
> 
> I felt it was clearer to have the defaults init fail if it doesn't work
> so that the callers know what happened.  Otherwise the callers have to
> figure out if it succeeded because it did what it was supposed to.

Aw right yes.

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