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