Re: [RFC] regmap: clairify max_register meaning

[Mark Brown <broonie@kernel.org>]

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

On Sun, Jan 17, 2016 at 10:52:56PM -0800, Stefan Agner wrote:

Please submit one change per patch.  You've mixed a change to the
documentation with a code here and it's really unclear what either of
them are supposed to do.

> Maybe I see something completely wrong here, but to me it seems
> that the regcache-flat.c is the only code interpreting max_register
> as register indexes rather then the maximum relative address...

I don't think I understand what the above means, sorry.  What is a
"relative address" in the context of a register map?

> At least regmap.c compares with range_max, which seems to use
> addresses.

That's a fairly large source file, can you be more specific please?

> -	map->cache = kcalloc(map->max_register + 1, sizeof(unsigned int),
> -			     GFP_KERNEL);
> +	map->cache = kzalloc(map->max_register + map->reg_stride, GFP_KERNEL);

This isn't very obvious and appears broken for a lot of devices.  It
does two things, it converts from allocating an array of unsigned
integers to an array of bytes and it replaces the handling of address 0
by adding an extra element to the array with adding stride bytes on the
end of the array.  This means that for a regmap with 16 bit values and a
stride of 1 we'll end up allocating nowhere the space we need as we'll
only allocate one byte per register plus an extra byte at the end but
the implementation of the flat cache is treating the cache as an array
of unsigned ints so needs at least four bytes per register.  It should
wash out as the same thing if the stride is equal to the size of an
unsigned int but most other cases will be broken.

Your changelog doesn't actually say this but I think what this is trying
to do is make the cache more memory efficient by not allocating space
for the registers that don't exist due to striding (ie, if we have a 4
byte stride then currently we'll allocate space for 4 times as many
registers as actually exist).  That's a reasonable goal but I don't
immediately have a good idea for doing it bearing in mind that we are
very performance sensitive here.

> - * @max_register: Optional, specifies the maximum valid register index.
> + * @max_register: Optional, specifies the maximum valid register address.

This is reasonable (index and address are synonyms here).

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]