Messages in this thread | | | Subject | Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy | From | Srinivas Kandagatla <> | Date | Wed, 10 Mar 2021 10:37:16 +0000 |
| |
On 06/03/2021 00:26, Douglas Anderson wrote: > Sometimes the clients of nvmem just want to get a number out of > nvmem. They don't want to think about exactly how many bytes the nvmem > cell took up. They just want the number. Let's make it easy. > > In general this concept is useful because nvmem space is precious and > usually the fewest bits are allocated that will hold a given value on > a given system. However, even though small numbers might be fine on > one system that doesn't mean that logically the number couldn't be > bigger. Imagine nvmem containing a max frequency for a component. On > one system perhaps that fits in 16 bits. On another system it might > fit in 32 bits. The code reading this number doesn't care--it just > wants the number. > > We'll provide two functions: nvmem_cell_read_variable_le_u32() and > nvmem_cell_read_variable_le_u64(). > > Comparing these to the existing functions like nvmem_cell_read_u32(): > * These new functions have no problems if the value was stored in > nvmem in fewer bytes. It's OK to use these function as long as the > value stored will fit in 32-bits (or 64-bits). > * These functions avoid problems that the earlier APIs had with bit > offsets. For instance, you can't use nvmem_cell_read_u32() to read a > value has nbits=32 and bit_offset=4 because the nvmem cell must be > at least 5 bytes big to hold this value. The new API accounts for > this and works fine. > * These functions make it very explicit that they assume that the > number was stored in little endian format. The old functions made > this assumption whenever bit_offset was non-zero (see > nvmem_shift_read_buffer_in_place()) but didn't whenever the > bit_offset was zero. > > NOTE: it's assumed that we don't need an 8-bit or 16-bit version of > this function. The 32-bit version of the function can be used to read > 8-bit or 16-bit data. > > At the moment, I'm only adding the "unsigned" versions of these > functions, but if it ends up being useful someone could add a "signed" > version that did 2's complement sign extension. > > At the moment, I'm only adding the "little endian" versions of these > functions. Adding the "big endian" version would require adding "big > endian" support to nvmem_shift_read_buffer_in_place(). > > Signed-off-by: Douglas Anderson <dianders@chromium.org> > --- > This is a logical follow-up to: > https://lore.kernel.org/r/20210227002603.3260599-1-dianders@chromium.org/ > ...but since it doesn't really share any of the same patches I'm not > marking it as a v2. > > drivers/nvmem/core.c | 95 ++++++++++++++++++++++++++++++++++ > include/linux/nvmem-consumer.h | 4 ++ > 2 files changed, 99 insertions(+) >
This patch as it is LGTM.
If you plan to take this via other trees, here is
Reviewed-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
--srini
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index a5ab1e0c74cf..635e3131eb5f 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -1606,6 +1606,101 @@ int nvmem_cell_read_u64(struct device *dev, const char *cell_id, u64 *val) > } > EXPORT_SYMBOL_GPL(nvmem_cell_read_u64); > > +static void *nvmem_cell_read_variable_common(struct device *dev, > + const char *cell_id, > + size_t max_len, size_t *len) > +{ > + struct nvmem_cell *cell; > + int nbits; > + void *buf; > + > + cell = nvmem_cell_get(dev, cell_id); > + if (IS_ERR(cell)) > + return cell; > + > + nbits = cell->nbits; > + buf = nvmem_cell_read(cell, len); > + nvmem_cell_put(cell); > + if (IS_ERR(buf)) > + return buf; > + > + /* > + * If nbits is set then nvmem_cell_read() can significantly exaggerate > + * the length of the real data. Throw away the extra junk. > + */ > + if (nbits) > + *len = DIV_ROUND_UP(nbits, 8); > + > + if (*len > max_len) { > + kfree(buf); > + return ERR_PTR(-ERANGE); > + } > + > + return buf; > +} > + > +/** > + * nvmem_cell_read_variable_le_u32() - Read up to 32-bits of data as a little endian number. > + * > + * @dev: Device that requests the nvmem cell. > + * @cell_id: Name of nvmem cell to read. > + * @val: pointer to output value. > + * > + * Return: 0 on success or negative errno. > + */ > +int nvmem_cell_read_variable_le_u32(struct device *dev, const char *cell_id, > + u32 *val) > +{ > + size_t len; > + u8 *buf; > + int i; > + > + buf = nvmem_cell_read_variable_common(dev, cell_id, sizeof(*val), &len); > + if (IS_ERR(buf)) > + return PTR_ERR(buf); > + > + /* Copy w/ implicit endian conversion */ > + *val = 0; > + for (i = 0; i < len; i++) > + *val |= buf[i] << (8 * i); > + > + kfree(buf); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nvmem_cell_read_variable_le_u32); > + > +/** > + * nvmem_cell_read_variable_le_u64() - Read up to 64-bits of data as a little endian number. > + * > + * @dev: Device that requests the nvmem cell. > + * @cell_id: Name of nvmem cell to read. > + * @val: pointer to output value. > + * > + * Return: 0 on success or negative errno. > + */ > +int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id, > + u64 *val) > +{ > + size_t len; > + u8 *buf; > + int i; > + > + buf = nvmem_cell_read_variable_common(dev, cell_id, sizeof(*val), &len); > + if (IS_ERR(buf)) > + return PTR_ERR(buf); > + > + /* Copy w/ implicit endian conversion */ > + *val = 0; > + for (i = 0; i < len; i++) > + *val |= buf[i] << (8 * i); > + > + kfree(buf); > + > + return 0; > +} > +EXPORT_SYMBOL_GPL(nvmem_cell_read_variable_le_u64); > + > /** > * nvmem_device_cell_read() - Read a given nvmem device and cell > * > diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h > index 052293f4cbdb..923dada24eb4 100644 > --- a/include/linux/nvmem-consumer.h > +++ b/include/linux/nvmem-consumer.h > @@ -65,6 +65,10 @@ int nvmem_cell_read_u8(struct device *dev, const char *cell_id, u8 *val); > int nvmem_cell_read_u16(struct device *dev, const char *cell_id, u16 *val); > int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val); > int nvmem_cell_read_u64(struct device *dev, const char *cell_id, u64 *val); > +int nvmem_cell_read_variable_le_u32(struct device *dev, const char *cell_id, > + u32 *val); > +int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id, > + u64 *val); > > /* direct nvmem device read/write interface */ > struct nvmem_device *nvmem_device_get(struct device *dev, const char *name); >
| |