From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     gregkh@linuxfoundation.org,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>,
        Michael Walle <michael@walle.cc>
Cc:     linux-kernel@vger.kernel.org,
        Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Subject: Re: [PATCH v2 12/22] nvmem: core: add an index parameter to the cell
Date:   Fri, 10 Feb 2023 12:01:59 +0100
Message-ID: <6214078.lOV4Wx5bFT@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <20230206134356.839737-13-srinivas.kandagatla@linaro.org>
References: <20230206134356.839737-1-srinivas.kandagatla@linaro.org> <20230206134356.839737-13-srinivas.kandagatla@linaro.org>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Hi,

Am Montag, 6. Februar 2023, 14:43:46 CET schrieb Srinivas Kandagatla:
> From: Michael Walle <michael@walle.cc>
> 
> 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 <michael@walle.cc>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> ---
>  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/20230105135931.2743351-1-michael@walle.cc/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,