From: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
To: Douglas Anderson <dianders@chromium.org>,
"Rafael J . Wysocki" <rafael.j.wysocki@intel.com>,
Rob Clark <robdclark@gmail.com>,
Jordan Crouse <jcrouse@codeaurora.org>
Cc: Ulf Hansson <ulf.hansson@linaro.org>,
swboyd@chromium.org, linux-arm-msm@vger.kernel.org,
Bjorn Andersson <bjorn.andersson@linaro.org>,
Akhil P Oommen <akhilpo@codeaurora.org>,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
Date: Wed, 10 Mar 2021 10:37:16 +0000 [thread overview]
Message-ID: <8e274cc7-0c9e-b983-7dd9-c501c159e95a@linaro.org> (raw)
In-Reply-To: <20210305162546.1.I323dad4343256b48af2be160b84b1e87985cc9be@changeid>
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);
>
next prev parent reply other threads:[~2021-03-10 10:38 UTC|newest]
Thread overview: 6+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-03-06 0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
2021-03-06 0:26 ` [PATCH 2/3] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin Douglas Anderson
2021-03-06 0:26 ` [PATCH 3/3] PM: AVS: qcom-cpr: Use nvmem_cell_read_variable_le_u32() Douglas Anderson
2021-03-10 10:37 ` Srinivas Kandagatla [this message]
2021-03-10 15:50 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Doug Anderson
2021-03-10 16:19 ` Srinivas Kandagatla
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e274cc7-0c9e-b983-7dd9-c501c159e95a@linaro.org \
--to=srinivas.kandagatla@linaro.org \
--cc=akhilpo@codeaurora.org \
--cc=bjorn.andersson@linaro.org \
--cc=dianders@chromium.org \
--cc=jcrouse@codeaurora.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=rafael.j.wysocki@intel.com \
--cc=robdclark@gmail.com \
--cc=swboyd@chromium.org \
--cc=ulf.hansson@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).