Messages in this thread | | | Subject | Re: [PATCH 2/3] nvmem: core: Allow nvmem_cell_read_u16/32/64 to read smaller cells | From | Srinivas Kandagatla <> | Date | Fri, 5 Mar 2021 10:27:54 +0000 |
| |
On 27/02/2021 00:26, Douglas Anderson wrote: > The current way that cell "length" is specified for nvmem cells is a > little fuzzy. For instance, let's look at the gpu speed bin currently > in sc7180.dtsi: > > gpu_speed_bin: gpu_speed_bin@1d2 { > reg = <0x1d2 0x2>; > bits = <5 8>; > }; > > This is an 8-bit value (as specified by the "bits" field). However, > it has a "length" of 2 (bytes), presumably because the value spans > across two bytes. > > When querying this value right now, it's hard for a client to know if > they should be calling nvmem_cell_read_u16() or nvmem_cell_read_u8(). > Today they must call nvmem_cell_read_u16() because the "length" of the > cell was 2 (bytes). However, if a later SoC ever came around and > didn't span across 2 bytes it would be unclear. If a later Soc > specified, for instance: > > gpu_speed_bin: gpu_speed_bin@100 { > reg = <0x100 0x1>; > bits = <0 8>; > }; > > ...then the caller would need to change to try calling > nvmem_cell_read_u8() because the u16 version would fail. >
If the consumer driver is expecting the sizes to span around byte to many bytes, then, Why not just call nvmem_cell_read() which should also return you how many bytes it has read!
> Let's solve this by allowing clients to read a "larger" value. We'll > just fill it in with 0.
That is misleading the consumer! If the consumer is expecting a u16 or u32, cell size should be of that size!!
--srini
If a client truly wants to know exactly how > big the cell was they can fall back to calling nvmem_cell_read() > directly. > > NOTE: current implementation assumes that cells are little endian when > up-casting the size, but that's already pretty implicit in the way > nvmem works now anyway. See nvmem_shift_read_buffer_in_place(). Let's > document this but not do any auto-conversions just in case there was > an instance where someone was using this API to read big endian data > on a big endian machine and it happened to be working because there > was no bit offset. > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > I will freely admit that I always end up thinking in circles and > getting dizzy when I think too much about endian considerations. If > anyone has better intuition than I do and see that I've goofed this up > then please yell. > > drivers/nvmem/core.c | 21 +++++++++++++++++++-- > 1 file changed, 19 insertions(+), 2 deletions(-) > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index a5ab1e0c74cf..8602390bb124 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -1534,12 +1534,14 @@ static int nvmem_cell_read_common(struct device *dev, const char *cell_id, > nvmem_cell_put(cell); > return PTR_ERR(buf); > } > - if (len != count) { > + if (len > count) { > kfree(buf); > nvmem_cell_put(cell); > return -EINVAL; > + } else if (len != count) { > + memset(val + len, 0, count - len); > } > - memcpy(val, buf, count); > + memcpy(val, buf, len); > kfree(buf); > nvmem_cell_put(cell); > > @@ -1564,6 +1566,11 @@ EXPORT_SYMBOL_GPL(nvmem_cell_read_u8); > /** > * nvmem_cell_read_u16() - Read a cell value as a u16 > * > + * This function can be used to read any cell value 16-bits or less. If > + * this function needs to upcast (or if the cell was stored in nvmem with > + * a bit offset) it will assume that the cell is little endian. Clients > + * should generally call le16_to_cpu() on the returned value. > + * > * @dev: Device that requests the nvmem cell. > * @cell_id: Name of nvmem cell to read. > * @val: pointer to output value. > @@ -1579,6 +1586,11 @@ EXPORT_SYMBOL_GPL(nvmem_cell_read_u16); > /** > * nvmem_cell_read_u32() - Read a cell value as a u32 > * > + * This function can be used to read any cell value 32-bits or less. If > + * this function needs to upcast (or if the cell was stored in nvmem with > + * a bit offset) it will assume that the cell is little endian. Clients > + * should generally call le32_to_cpu() on the returned value. > + * > * @dev: Device that requests the nvmem cell. > * @cell_id: Name of nvmem cell to read. > * @val: pointer to output value. > @@ -1594,6 +1606,11 @@ EXPORT_SYMBOL_GPL(nvmem_cell_read_u32); > /** > * nvmem_cell_read_u64() - Read a cell value as a u64 > * > + * This function can be used to read any cell value 64-bits or less. If > + * this function needs to upcast (or if the cell was stored in nvmem with > + * a bit offset) it will assume that the cell is little endian. Clients > + * should generally call le64_to_cpu() on the returned value. > + * > * @dev: Device that requests the nvmem cell. > * @cell_id: Name of nvmem cell to read. > * @val: pointer to output value. >
| |