by Alexander Stein

[permalink] [raw]

Subject: Re: [PATCH v2 12/22] nvmem: core: add an index parameter to the cell

Hi,

Am Montag, 6. Februar 2023, 14:43:46 CET schrieb Srinivas Kandagatla:
> From: Michael Walle <[email protected]>
>
> Sometimes a cell can represend multiple values. For example, a base
> ethernet address stored in the NVMEM can be expanded into multiple
> discreet ones by adding an offset.
>
> For this use case, introduce an index parameter which is then used to
> distiguish between values. This parameter will then be passed to the
> post process hook which can then use it to create different values
> during reading.
>
> At the moment, there is only support for the device tree path. You can
> add the index to the phandle, e.g.
>
> &net {
> nvmem-cells = <&base_mac_address 2>;
> nvmem-cell-names = "mac-address";
> };
>
> &nvmem_provider {
> base_mac_address: base-mac-address@0 {
> #nvmem-cell-cells = <1>;
> reg = <0 6>;
> };
> };
>
> Signed-off-by: Michael Walle <[email protected]>
> Signed-off-by: Srinivas Kandagatla <[email protected]>
> ---
> drivers/nvmem/core.c | 37 ++++++++++++++++++++++++----------
> drivers/nvmem/imx-ocotp.c | 4 ++--
> include/linux/nvmem-provider.h | 4 ++--
> 3 files changed, 30 insertions(+), 15 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 233c6c275031..30567dd51fba 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -60,6 +60,7 @@ struct nvmem_cell_entry {
> struct nvmem_cell {
> struct nvmem_cell_entry *entry;
> const char *id;
> + int index;
> };
>
> static DEFINE_MUTEX(nvmem_mutex);
> @@ -1122,7 +1123,8 @@ struct nvmem_device *devm_nvmem_device_get(struct
> device *dev, const char *id) }
> EXPORT_SYMBOL_GPL(devm_nvmem_device_get);
>
> -static struct nvmem_cell *nvmem_create_cell(struct nvmem_cell_entry *entry,
> const char *id) +static struct nvmem_cell *nvmem_create_cell(struct
> nvmem_cell_entry *entry, +
const char *id, int index)
> {
> struct nvmem_cell *cell;
> const char *name = NULL;
> @@ -1141,6 +1143,7 @@ static struct nvmem_cell *nvmem_create_cell(struct
> nvmem_cell_entry *entry, cons
>
> cell->id = name;
> cell->entry = entry;
> + cell->index = index;
>
> return cell;
> }
> @@ -1179,7 +1182,7 @@ nvmem_cell_get_from_lookup(struct device *dev, const
> char *con_id) __nvmem_device_put(nvmem);
> cell = ERR_PTR(-ENOENT);
> } else {
> - cell = nvmem_create_cell(cell_entry,
con_id);
> + cell = nvmem_create_cell(cell_entry,
con_id, 0);
> if (IS_ERR(cell))
> __nvmem_device_put(nvmem);
> }
> @@ -1227,15 +1230,27 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> device_node *np, const char *id) struct nvmem_device *nvmem;
> struct nvmem_cell_entry *cell_entry;
> struct nvmem_cell *cell;
> + struct of_phandle_args cell_spec;
> int index = 0;
> + int cell_index = 0;
> + int ret;
>
> /* if cell name exists, find index to the name */
> if (id)
> index = of_property_match_string(np, "nvmem-cell-names",
id);
>
> - cell_np = of_parse_phandle(np, "nvmem-cells", index);
> - if (!cell_np)
> - return ERR_PTR(-ENOENT);
> + ret = of_parse_phandle_with_optional_args(np, "nvmem-cells",
> + "#nvmem-cell-
cells",
> + index,
&cell_spec);
> + if (ret)
> + return ERR_PTR(ret);

This change introduce a regression (again, see [1] for that).
dp83867_of_init_io_impedance calls of_nvmem_cell_get() for cell
'io_impedance_ctrl'. If this does not exist, 'nvmem-cell-names' does not exist
as well, of_property_match_string returns -22 for index.
__of_parse_phandle_with_args eventually returns -EINVAL for this invalid
index. -ENOENT was returned before.
There was a bugfix [2] for this, but it's not in linux-next.

Best regards,
Alexander

[1] https://lore.kernel.org/lkml/2143916.GUh0CODmnK@steina-w/
[2] https://lore.kernel.org/lkml/[email protected]/T/
> +
> + if (cell_spec.args_count > 1)
> + return ERR_PTR(-EINVAL);
> +
> + cell_np = cell_spec.np;
> + if (cell_spec.args_count)
> + cell_index = cell_spec.args[0];
>
> nvmem_np = of_get_parent(cell_np);
> if (!nvmem_np) {
> @@ -1257,7 +1272,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct
> device_node *np, const char *id) return ERR_PTR(-ENOENT);
> }
>
> - cell = nvmem_create_cell(cell_entry, id);
> + cell = nvmem_create_cell(cell_entry, id, cell_index);
> if (IS_ERR(cell))
> __nvmem_device_put(nvmem);
>
> @@ -1410,8 +1425,8 @@ static void nvmem_shift_read_buffer_in_place(struct
> nvmem_cell_entry *cell, void }
>
> static int __nvmem_cell_read(struct nvmem_device *nvmem,
> - struct nvmem_cell_entry *cell,
> - void *buf, size_t *len, const char *id)
> + struct nvmem_cell_entry *cell,
> + void *buf, size_t *len, const char *id, int
index)
> {
> int rc;
>
> @@ -1425,7 +1440,7 @@ static int __nvmem_cell_read(struct nvmem_device
> *nvmem, nvmem_shift_read_buffer_in_place(cell, buf);
>
> if (nvmem->cell_post_process) {
> - rc = nvmem->cell_post_process(nvmem->priv, id,
> + rc = nvmem->cell_post_process(nvmem->priv, id, index,
> cell->offset, buf,
cell->bytes);
> if (rc)
> return rc;
> @@ -1460,7 +1475,7 @@ void *nvmem_cell_read(struct nvmem_cell *cell, size_t
> *len) if (!buf)
> return ERR_PTR(-ENOMEM);
>
> - rc = __nvmem_cell_read(nvmem, cell->entry, buf, len, cell->id);
> + rc = __nvmem_cell_read(nvmem, cell->entry, buf, len, cell->id,
> cell->index); if (rc) {
> kfree(buf);
> return ERR_PTR(rc);
> @@ -1773,7 +1788,7 @@ ssize_t nvmem_device_cell_read(struct nvmem_device
> *nvmem, if (rc)
> return rc;
>
> - rc = __nvmem_cell_read(nvmem, &cell, buf, &len, NULL);
> + rc = __nvmem_cell_read(nvmem, &cell, buf, &len, NULL, 0);
> if (rc)
> return rc;
>
> diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
> index 14284e866f26..e9b52ecb3f72 100644
> --- a/drivers/nvmem/imx-ocotp.c
> +++ b/drivers/nvmem/imx-ocotp.c
> @@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned int
> offset, return ret;
> }
>
> -static int imx_ocotp_cell_pp(void *context, const char *id, unsigned int
> offset, - void *data, size_t bytes)
> +static int imx_ocotp_cell_pp(void *context, const char *id, int index,
> + unsigned int offset, void *data, size_t
bytes)
> {
> struct ocotp_priv *priv = context;
>
> diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
> index bb15c9234e21..55181d837969 100644
> --- a/include/linux/nvmem-provider.h
> +++ b/include/linux/nvmem-provider.h
> @@ -20,8 +20,8 @@ typedef int (*nvmem_reg_read_t)(void *priv, unsigned int
> offset, typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset,
> void *val, size_t bytes);
> /* used for vendor specific post processing of cell data */
> -typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id,
> unsigned int offset, - void
*buf, size_t bytes);
> +typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, int
> index, + unsigned int offset, void
*buf, size_t bytes);
>
> enum nvmem_type {
> NVMEM_TYPE_UNKNOWN = 0,