Re: [PATCH 2/8] regmap: Add the indexed cache support

[Lars-Peter Clausen <lars@metafoo.de>]

On 09/02/2011 05:46 PM, Dimitris Papastamos wrote:
> This is the simplest form of a cache available in regcache.  Any
> registers whose default value is 0 are ignored.  If any of those
> registers are modified in the future, they will be placed in the
> cache on demand.  The cache layout is essentially using the provided
> register defaults by the regcache core directly and does not re-map
> it to another representation.
> 
> Signed-off-by: Dimitris Papastamos <dp@opensource.wolfsonmicro.com>

I wonder if we really still need indexed caches, because ...


> ---
>  drivers/base/regmap/Makefile           |    2 +-
>  drivers/base/regmap/internal.h         |    1 +
>  drivers/base/regmap/regcache-indexed.c |   65 ++++++++++++++++++++++++++++++++
>  drivers/base/regmap/regcache.c         |    3 +
>  include/linux/regmap.h                 |    1 +
>  5 files changed, 71 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/base/regmap/regcache-indexed.c
> 
> [...]
> +static int regcache_indexed_read(struct regmap *map, unsigned int reg,
> +				 unsigned int *value)
> +{
> +	int ret;
> +
> +	ret = regcache_lookup_reg(map, reg);

... this lookup will be slower than an rbtree lookup. And given that we use two
integers per register doesn't give it much size advantage either.

I have some patches for the rbtree register cache code, which reduce some of
it's complexity and also implements the adjacent node merging and allows
smaller holes in the rbnodes register block if the block is smaller then an
rbnode. Currently these patches are based on the ASoC register cache code, but
they should be easy to rework onto of the regmap register cache code.

I did some small statistics on the number of rbnodes without and with the
patches applied:

Without patches:
Nodes:
  0: 4000-4000   1
  1: 4009-400a   2
  2: 400b-400c   2
  3: 400d-4010   4
  4: 4015-4015   1
  5: 4017-4017   1
  6: 4019-401b   3
  7: 401c-401d   2
  8: 401e-4020   3
  9: 4021-4022   2
 10: 4023-4026   4
 11: 4029-402a   2
 12: 4031-4031   1
 13: 4036-4036   1
 14: 40eb-40eb   1
 15: 40f2-40f3   2
 16: 40f8-40f9   2
 17: 40fa-40fa   1

Number of nodes: 18 (504 bytes)
Number of registers: 35 (35 bytes)
Total size: 539 bytes

With patches:
Nodes:
  0: 4000-4036  55
  1: 40eb-40fa  16

Number of nodes: 2 (56 bytes)
Number of registers: 71 (71 bytes)
Total size: 127 bytes

In comparison a ASoC flat cache for this driver would have used 250 bytes and
and regmap indexed cache would use 280 bytes. (Not that it really matters at
those sizes anyway)


> +	if (ret < 0)
> +		*value = 0;
> +	else
> +		*value = map->cache_defaults[ret].def;
> +	return 0;
> +}
> +
> [...]
> diff --git a/drivers/base/regmap/regcache.c b/drivers/base/regmap/regcache.c
> index 90b7e1f..dfefe40 100644
> --- a/drivers/base/regmap/regcache.c
> +++ b/drivers/base/regmap/regcache.c
> @@ -16,6 +16,9 @@
>  #include "internal.h"
>  
>  static const struct regcache_ops *cache_types[] = {
> +#ifdef CONFIG_REGCACHE_INDEXED

This symbol doesn't seem to be defined anywhere.

> +	&regcache_indexed_ops,
> +#endif
>  };
>  
>  int regcache_init(struct regmap *map)
> diff --git a/include/linux/regmap.h b/include/linux/regmap.h
> index b81e86a..4d1ad09 100644
> --- a/include/linux/regmap.h
> +++ b/include/linux/regmap.h
> @@ -23,6 +23,7 @@ struct spi_device;
>  /* An enum of all the supported cache types */
>  enum regcache_type {
>  	REGCACHE_NONE,
> +	REGCACHE_INDEXED,
>  };
>  
>  /**