On Tue, 16 Mar 2021 22:04:17 +0530
Pratyush Yadav <p.ya...@ti.com> wrote:
> > + switch (map->width) {
> > + case REGMAP_SIZE_8:
> > + *valp = u.v8;
> > + break;
> > + case REGMAP_SIZE_16:
> > + *valp = u.v16;
> > + break;
> > + case REGMAP_SIZE_32:
> > + *valp = u.v32;
> > + break;
> > + case REGMAP_SIZE_64:
> > + *valp = u.v64;
> > + break;
>
> I think this should fix the problem you are trying to solve.
>
> But I see another problem with this code. What if someone wants to read
> 8 bytes? IIUC, since valp points to a uint, *valp = u.v64 will result in
> 4 bytes being truncated from the result.
>
> I see two options:
>
> - Change the uint pointer to u64 pointer and update every driver to use
> a u64 when using the regmap.
>
> - Change the uint pointer to void pointer and expect every driver to
> pass a container with exactly the required size, based on map->width.
> Update the ones that don't follow this.
>
> I prefer the latter option.
If only these two options are possible, then the first option is
better. The regmap_read function ought to be simple, so no void *
pointer.
There is another option: if a value greater than ULONG_MAX is read, the
regmap_read() function will return -ERANGE. This way this function
will remain simple. ulong is 64-bit wide on 64-bit devices, so this
only is a problem when a 64-bit wide regmap is used on a 32-bit device.
I think we should keep regmap_read() simple, and for people who use a
64-bit wide regmap on 32-bit device, there is regmap_raw_read().
But I think this problem should be solved (however we decide) in a
follow-up patch. This patch fixes the pointer casting bug, and commits
should remain atomic (fixing one thing).
Marek
