All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/3] nvmem: core: Add functions to make number reading easy
@ 2021-03-06  0:26 Douglas Anderson
  2021-03-06  0:26   ` Douglas Anderson
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Douglas Anderson @ 2021-03-06  0:26 UTC (permalink / raw)
  To: Rafael J . Wysocki, Rob Clark, Jordan Crouse
  Cc: Ulf Hansson, Niklas Cassel, Jorge Ramirez-Ortiz, swboyd,
	linux-arm-msm, Bjorn Andersson, Akhil P Oommen, Douglas Anderson,
	Srinivas Kandagatla, linux-kernel

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(+)

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);
-- 
2.30.1.766.gb4fecdf3b7-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
  2021-03-06  0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
@ 2021-03-06  0:26   ` 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 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
  2 siblings, 0 replies; 8+ messages in thread
From: Douglas Anderson @ 2021-03-06  0:26 UTC (permalink / raw)
  To: Rafael J . Wysocki, Rob Clark, Jordan Crouse
  Cc: Ulf Hansson, Niklas Cassel, Jorge Ramirez-Ortiz, swboyd,
	linux-arm-msm, Bjorn Andersson, Akhil P Oommen, Douglas Anderson,
	Daniel Vetter, David Airlie, Eric Anholt, Jonathan Marek,
	Sai Prakash Ranjan, Sean Paul, Sharat Masetty, dri-devel,
	freedreno, linux-kernel

Let's use the newly-added nvmem_cell_read_variable_le_u32() to future
proof ourselves a little bit.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This is based on my previous patch ("drm/msm: Fix speed-bin support
not to access outside valid memory") which has already landed in
msm-next. In case it's not obvious, this patch has a strong dependency
on my previous patch and also on patch #1 in this series.

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 0e2024defd79..e34705d17559 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1351,17 +1351,16 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
 {
 	struct opp_table *opp_table;
 	u32 supp_hw = UINT_MAX;
-	u16 speedbin;
+	u32 speedbin;
 	int ret;
 
-	ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
+	ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", &speedbin);
 	if (ret) {
 		DRM_DEV_ERROR(dev,
 			      "failed to read speed-bin (%d). Some OPPs may not be supported by hardware",
 			      ret);
 		goto done;
 	}
-	speedbin = le16_to_cpu(speedbin);
 
 	supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
 
-- 
2.30.1.766.gb4fecdf3b7-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/3] drm/msm: Use nvmem_cell_read_variable_le_u32() to read speed bin
@ 2021-03-06  0:26   ` Douglas Anderson
  0 siblings, 0 replies; 8+ messages in thread
From: Douglas Anderson @ 2021-03-06  0:26 UTC (permalink / raw)
  To: Rafael J . Wysocki, Rob Clark, Jordan Crouse
  Cc: freedreno, Ulf Hansson, Douglas Anderson, Jonathan Marek,
	David Airlie, linux-arm-msm, Sharat Masetty, Akhil P Oommen,
	dri-devel, Bjorn Andersson, Sai Prakash Ranjan, Niklas Cassel,
	Jorge Ramirez-Ortiz, swboyd, Sean Paul, linux-kernel

Let's use the newly-added nvmem_cell_read_variable_le_u32() to future
proof ourselves a little bit.

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
This is based on my previous patch ("drm/msm: Fix speed-bin support
not to access outside valid memory") which has already landed in
msm-next. In case it's not obvious, this patch has a strong dependency
on my previous patch and also on patch #1 in this series.

 drivers/gpu/drm/msm/adreno/a6xx_gpu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
index 0e2024defd79..e34705d17559 100644
--- a/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
+++ b/drivers/gpu/drm/msm/adreno/a6xx_gpu.c
@@ -1351,17 +1351,16 @@ static int a6xx_set_supported_hw(struct device *dev, struct a6xx_gpu *a6xx_gpu,
 {
 	struct opp_table *opp_table;
 	u32 supp_hw = UINT_MAX;
-	u16 speedbin;
+	u32 speedbin;
 	int ret;
 
-	ret = nvmem_cell_read_u16(dev, "speed_bin", &speedbin);
+	ret = nvmem_cell_read_variable_le_u32(dev, "speed_bin", &speedbin);
 	if (ret) {
 		DRM_DEV_ERROR(dev,
 			      "failed to read speed-bin (%d). Some OPPs may not be supported by hardware",
 			      ret);
 		goto done;
 	}
-	speedbin = le16_to_cpu(speedbin);
 
 	supp_hw = fuse_to_supp_hw(dev, revn, speedbin);
 
-- 
2.30.1.766.gb4fecdf3b7-goog

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 3/3] PM: AVS: qcom-cpr: Use nvmem_cell_read_variable_le_u32()
  2021-03-06  0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
  2021-03-06  0:26   ` Douglas Anderson
@ 2021-03-06  0:26 ` Douglas Anderson
  2021-03-10 10:37 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
  2 siblings, 0 replies; 8+ messages in thread
From: Douglas Anderson @ 2021-03-06  0:26 UTC (permalink / raw)
  To: Rafael J . Wysocki, Rob Clark, Jordan Crouse
  Cc: Ulf Hansson, Niklas Cassel, Jorge Ramirez-Ortiz, swboyd,
	linux-arm-msm, Bjorn Andersson, Akhil P Oommen, Douglas Anderson,
	Andy Gross, Niklas Cassel, linux-kernel, linux-pm

Let's delete the private function cpr_read_efuse() since it does the
basically the same thing as the new API call
nvmem_cell_read_variable_le_u32().

Differences between the new API call and the old private function:
* less error printing (I assume this is OK).
* will give an error if the value doesn't fit in 32-bits (the old code
  would have truncated silently).

Signed-off-by: Douglas Anderson <dianders@chromium.org>
---
I haven't done any more than compile-test this. Mostly I'm just
writing this patch because it helped provide inspiration for the
general API function.

 drivers/soc/qcom/cpr.c | 43 +++++-------------------------------------
 1 file changed, 5 insertions(+), 38 deletions(-)

diff --git a/drivers/soc/qcom/cpr.c b/drivers/soc/qcom/cpr.c
index b24cc77d1889..4ce8e816154f 100644
--- a/drivers/soc/qcom/cpr.c
+++ b/drivers/soc/qcom/cpr.c
@@ -801,38 +801,6 @@ static int cpr_set_performance_state(struct generic_pm_domain *domain,
 	return ret;
 }
 
-static int cpr_read_efuse(struct device *dev, const char *cname, u32 *data)
-{
-	struct nvmem_cell *cell;
-	ssize_t len;
-	char *ret;
-	int i;
-
-	*data = 0;
-
-	cell = nvmem_cell_get(dev, cname);
-	if (IS_ERR(cell)) {
-		if (PTR_ERR(cell) != -EPROBE_DEFER)
-			dev_err(dev, "undefined cell %s\n", cname);
-		return PTR_ERR(cell);
-	}
-
-	ret = nvmem_cell_read(cell, &len);
-	nvmem_cell_put(cell);
-	if (IS_ERR(ret)) {
-		dev_err(dev, "can't read cell %s\n", cname);
-		return PTR_ERR(ret);
-	}
-
-	for (i = 0; i < len; i++)
-		*data |= ret[i] << (8 * i);
-
-	kfree(ret);
-	dev_dbg(dev, "efuse read(%s) = %x, bytes %zd\n", cname, *data, len);
-
-	return 0;
-}
-
 static int
 cpr_populate_ring_osc_idx(struct cpr_drv *drv)
 {
@@ -843,8 +811,7 @@ cpr_populate_ring_osc_idx(struct cpr_drv *drv)
 	int ret;
 
 	for (; fuse < end; fuse++, fuses++) {
-		ret = cpr_read_efuse(drv->dev, fuses->ring_osc,
-				     &data);
+		ret = nvmem_cell_read_variable_le_u32(drv->dev, fuses->ring_osc, &data);
 		if (ret)
 			return ret;
 		fuse->ring_osc_idx = data;
@@ -863,7 +830,7 @@ static int cpr_read_fuse_uV(const struct cpr_desc *desc,
 	u32 bits = 0;
 	int ret;
 
-	ret = cpr_read_efuse(drv->dev, init_v_efuse, &bits);
+	ret = nvmem_cell_read_variable_le_u32(drv->dev, init_v_efuse, &bits);
 	if (ret)
 		return ret;
 
@@ -932,7 +899,7 @@ static int cpr_fuse_corner_init(struct cpr_drv *drv)
 		}
 
 		/* Populate target quotient by scaling */
-		ret = cpr_read_efuse(drv->dev, fuses->quotient, &fuse->quot);
+		ret = nvmem_cell_read_variable_le_u32(drv->dev, fuses->quotient, &fuse->quot);
 		if (ret)
 			return ret;
 
@@ -1001,7 +968,7 @@ static int cpr_calculate_scaling(const char *quot_offset,
 	prev_fuse = fuse - 1;
 
 	if (quot_offset) {
-		ret = cpr_read_efuse(drv->dev, quot_offset, &quot_diff);
+		ret = nvmem_cell_read_variable_le_u32(drv->dev, quot_offset, &quot_diff);
 		if (ret)
 			return ret;
 
@@ -1701,7 +1668,7 @@ static int cpr_probe(struct platform_device *pdev)
 	 * initialized after attaching to the power domain,
 	 * since it depends on the CPU's OPP table.
 	 */
-	ret = cpr_read_efuse(dev, "cpr_fuse_revision", &cpr_rev);
+	ret = nvmem_cell_read_variable_le_u32(dev, "cpr_fuse_revision", &cpr_rev);
 	if (ret)
 		return ret;
 
-- 
2.30.1.766.gb4fecdf3b7-goog


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
  2021-03-06  0:26 [PATCH 1/3] nvmem: core: Add functions to make number reading easy Douglas Anderson
  2021-03-06  0:26   ` 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
  2021-03-10 15:50   ` Doug Anderson
  2 siblings, 1 reply; 8+ messages in thread
From: Srinivas Kandagatla @ 2021-03-10 10:37 UTC (permalink / raw)
  To: Douglas Anderson, Rafael J . Wysocki, Rob Clark, Jordan Crouse
  Cc: Ulf Hansson, swboyd, linux-arm-msm, Bjorn Andersson,
	Akhil P Oommen, linux-kernel



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);
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
  2021-03-10 10:37 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
@ 2021-03-10 15:50   ` Doug Anderson
  2021-03-10 16:19     ` Srinivas Kandagatla
  0 siblings, 1 reply; 8+ messages in thread
From: Doug Anderson @ 2021-03-10 15:50 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Rafael J . Wysocki, Rob Clark, Jordan Crouse, Ulf Hansson,
	Stephen Boyd, linux-arm-msm, Bjorn Andersson, Akhil P Oommen,
	LKML

Hi,

On Wed, Mar 10, 2021 at 2:37 AM Srinivas Kandagatla
<srinivas.kandagatla@linaro.org> wrote:
>
>
>
> 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>

Thanks! I think none of this is terribly urgent, though. Unless
someone has a different opinion, my thought would be:

* This patch lands in your tree for 5.13.

* I'll snooze the email for 2 months and poke patch #2 and #3 once
5.13-rc1 is out.

Does that sound OK to you?

-Doug

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
  2021-03-10 15:50   ` Doug Anderson
@ 2021-03-10 16:19     ` Srinivas Kandagatla
  0 siblings, 0 replies; 8+ messages in thread
From: Srinivas Kandagatla @ 2021-03-10 16:19 UTC (permalink / raw)
  To: Doug Anderson
  Cc: Rafael J . Wysocki, Rob Clark, Jordan Crouse, Ulf Hansson,
	Stephen Boyd, linux-arm-msm, Bjorn Andersson, Akhil P Oommen,
	LKML



On 10/03/2021 15:50, Doug Anderson wrote:
> Hi,
> 
> On Wed, Mar 10, 2021 at 2:37 AM Srinivas Kandagatla
> <srinivas.kandagatla@linaro.org> wrote:
>>
>>
>>
>> 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>
> 
> Thanks! I think none of this is terribly urgent, though. Unless
> someone has a different opinion, my thought would be:
> 
> * This patch lands in your tree for 5.13.
> 
> * I'll snooze the email for 2 months and poke patch #2 and #3 once
> 5.13-rc1 is out.
> 
> Does that sound OK to you?
That works for me!

Applied thanks!

--srini
> 
> -Doug
> 

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/3] nvmem: core: Add functions to make number reading easy
@ 2021-03-07  8:02 kernel test robot
  0 siblings, 0 replies; 8+ messages in thread
From: kernel test robot @ 2021-03-07  8:02 UTC (permalink / raw)
  To: kbuild

[-- Attachment #1: Type: text/plain, Size: 4231 bytes --]

CC: kbuild-all(a)01.org
In-Reply-To: <20210305162546.1.I323dad4343256b48af2be160b84b1e87985cc9be@changeid>
References: <20210305162546.1.I323dad4343256b48af2be160b84b1e87985cc9be@changeid>
TO: Douglas Anderson <dianders@chromium.org>
TO: "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
TO: Rob Clark <robdclark@gmail.com>
TO: Jordan Crouse <jcrouse@codeaurora.org>
CC: Ulf Hansson <ulf.hansson@linaro.org>
CC: Niklas Cassel <niklas.cassel@linaro.org>
CC: "Jorge Ramirez-Ortiz" <jorge.ramirez-ortiz@linaro.org>
CC: swboyd(a)chromium.org
CC: linux-arm-msm(a)vger.kernel.org
CC: Bjorn Andersson <bjorn.andersson@linaro.org>
CC: Akhil P Oommen <akhilpo@codeaurora.org>

Hi Douglas,

I love your patch! Perhaps something to improve:

[auto build test WARNING on next-20210305]
[also build test WARNING on v5.12-rc2]
[cannot apply to linus/master v5.12-rc1 v5.11 v5.11-rc7]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Douglas-Anderson/nvmem-core-Add-functions-to-make-number-reading-easy/20210307-095202
base:    4641b32307b3b54397652a71f852bb0bafb0820d
:::::: branch date: 6 hours ago
:::::: commit date: 6 hours ago
config: i386-randconfig-m021-20210307 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>

smatch warnings:
drivers/nvmem/core.c:1696 nvmem_cell_read_variable_le_u64() warn: should 'buf[i] << (8 * i)' be a 64 bit type?

vim +1696 drivers/nvmem/core.c

cb1857bda8160a Douglas Anderson 2021-03-05  1672  
cb1857bda8160a Douglas Anderson 2021-03-05  1673  /**
cb1857bda8160a Douglas Anderson 2021-03-05  1674   * nvmem_cell_read_variable_le_u64() - Read up to 64-bits of data as a little endian number.
cb1857bda8160a Douglas Anderson 2021-03-05  1675   *
cb1857bda8160a Douglas Anderson 2021-03-05  1676   * @dev: Device that requests the nvmem cell.
cb1857bda8160a Douglas Anderson 2021-03-05  1677   * @cell_id: Name of nvmem cell to read.
cb1857bda8160a Douglas Anderson 2021-03-05  1678   * @val: pointer to output value.
cb1857bda8160a Douglas Anderson 2021-03-05  1679   *
cb1857bda8160a Douglas Anderson 2021-03-05  1680   * Return: 0 on success or negative errno.
cb1857bda8160a Douglas Anderson 2021-03-05  1681   */
cb1857bda8160a Douglas Anderson 2021-03-05  1682  int nvmem_cell_read_variable_le_u64(struct device *dev, const char *cell_id,
cb1857bda8160a Douglas Anderson 2021-03-05  1683  				    u64 *val)
cb1857bda8160a Douglas Anderson 2021-03-05  1684  {
cb1857bda8160a Douglas Anderson 2021-03-05  1685  	size_t len;
cb1857bda8160a Douglas Anderson 2021-03-05  1686  	u8 *buf;
cb1857bda8160a Douglas Anderson 2021-03-05  1687  	int i;
cb1857bda8160a Douglas Anderson 2021-03-05  1688  
cb1857bda8160a Douglas Anderson 2021-03-05  1689  	buf = nvmem_cell_read_variable_common(dev, cell_id, sizeof(*val), &len);
cb1857bda8160a Douglas Anderson 2021-03-05  1690  	if (IS_ERR(buf))
cb1857bda8160a Douglas Anderson 2021-03-05  1691  		return PTR_ERR(buf);
cb1857bda8160a Douglas Anderson 2021-03-05  1692  
cb1857bda8160a Douglas Anderson 2021-03-05  1693  	/* Copy w/ implicit endian conversion */
cb1857bda8160a Douglas Anderson 2021-03-05  1694  	*val = 0;
cb1857bda8160a Douglas Anderson 2021-03-05  1695  	for (i = 0; i < len; i++)
cb1857bda8160a Douglas Anderson 2021-03-05 @1696  		*val |= buf[i] << (8 * i);
cb1857bda8160a Douglas Anderson 2021-03-05  1697  
cb1857bda8160a Douglas Anderson 2021-03-05  1698  	kfree(buf);
cb1857bda8160a Douglas Anderson 2021-03-05  1699  
cb1857bda8160a Douglas Anderson 2021-03-05  1700  	return 0;
cb1857bda8160a Douglas Anderson 2021-03-05  1701  }
cb1857bda8160a Douglas Anderson 2021-03-05  1702  EXPORT_SYMBOL_GPL(nvmem_cell_read_variable_le_u64);
cb1857bda8160a Douglas Anderson 2021-03-05  1703  

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 38996 bytes --]

^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2021-03-10 16:20 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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   ` 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 ` [PATCH 1/3] nvmem: core: Add functions to make number reading easy Srinivas Kandagatla
2021-03-10 15:50   ` Doug Anderson
2021-03-10 16:19     ` Srinivas Kandagatla
2021-03-07  8:02 kernel test robot

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.