Re: [PATCH v4 3/5] nvmem: core: Add consumer API to get nvmem cell from node

[Srinivas Kandagatla <srinivas.kandagatla@linaro.org>]

On 07/07/16 13:33, maitysanchayan@gmail.com wrote:
> Hello Srinivas,
>
> On 16-07-07 10:18:49, Srinivas Kandagatla wrote:
>>
>>
>> On 07/07/16 07:39, Sanchayan Maity wrote:
>>> From: Stefan Agner <stefan@agner.ch>
>>>
>>> The existing NVMEM consumer API's do not allow to get a
>>> NVMEM cell directly given a device tree node. This patch
>>> adds a function to provide this functionality.
>>>
>>> Assuming the nvmem cell id name is known, this can be used
>>> as follows
>>>
>>> struct device_node *cell_np;
>>> struct nvmem_cell *foo_cell;
>>>
>>> cell_np = of_find_node_by_name(parent, "foo");
>>> foo_cell = of_nvmem_cell_get_direct(cell_np);
>>
>> I don't see a real gain in adding this new api,
>> This will encourage people to use non-standard nvmem bindings.
>>
>> why not just use standard nvmem bindings.. and use
>>
>> of_nvmem_cell_get(np, "foo");
>>
>> which should work in your case.
>
> It will not work in our case. I believe what you are referring to will
> work if I were to pass the device node pointer which was a NVMEM consumer
> containing the nvmem-cells property. In our case, we pass the device node
> pointer pointing to /soc which is not a nvmem consumer. In this case it
> will not have nvmem-cells property causing of_nvmem_cell_get to return
> EINVAL when it calls of_parse_phandle with "nvmem-cells".

I could not see any bindings/ dt patches or dt examples for this this 
series.. so Am guessing your node would look like:

soc {
	cfg0: cfg0 {
		...
	};
	cfg1: cfg1 {
		...
	};
};

If this is not how it looks, can you share the details.

What Am saying is that why not have:

soc {
	nvmem-cells = <&cfg0>, <&cfg1>;
	nvmem-cell-names = "cfg0", "cfg1";

	cfg0: cfg0 {
		...
	};

	cfg1: cfg1 {
		...
	};
};

>
> Our requirement is to be able to pass the soc node pointer and then
> be able to get a nvmem cell by specifying it's name. So for our case

Why?

> ocotp node has cfg0 and cfg1 which we want but we cannot use existing
> nvmem consumer API since that requires having the nvmem consumer properties
> in the node we are binding to viz. is a direct nvmem consumer.
>
> Regards,
> Sanchayan.
>
>>
>> thanks,
>> srini
>>>
>>> Parent node can also be the of_node of the main SoC device
>>> node.
>>>
>>> Signed-off-by: Sanchayan Maity <maitysanchayan@gmail.com>
>>> ---
>>>    drivers/nvmem/core.c           | 44 +++++++++++++++++++++++++++++-------------
>>>    include/linux/nvmem-consumer.h |  1 +
>>>    2 files changed, 32 insertions(+), 13 deletions(-)
>>>
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index 965911d..470abee 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -743,29 +743,21 @@ static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
>>>
>>>    #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
>>>    /**
>>> - * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>>> + * of_nvmem_cell_get_direct() - Get a nvmem cell from given device node
>>>     *
>>> - * @dev node: Device tree node that uses the nvmem cell
>>> - * @id: nvmem cell name from nvmem-cell-names property.
>>> + * @dev node: Device tree node that uses nvmem cell
>>>     *
>>>     * Return: Will be an ERR_PTR() on error or a valid pointer
>>>     * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>>>     * nvmem_cell_put().
>>>     */
>>> -struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>> -					    const char *name)
>>> +struct nvmem_cell *of_nvmem_cell_get_direct(struct device_node *cell_np)
>>>    {
>>> -	struct device_node *cell_np, *nvmem_np;
>>> +	struct device_node *nvmem_np;
>>>    	struct nvmem_cell *cell;
>>>    	struct nvmem_device *nvmem;
>>>    	const __be32 *addr;
>>> -	int rval, len, index;
>>> -
>>> -	index = of_property_match_string(np, "nvmem-cell-names", name);
>>> -
>>> -	cell_np = of_parse_phandle(np, "nvmem-cells", index);
>>> -	if (!cell_np)
>>> -		return ERR_PTR(-EINVAL);
>>> +	int rval, len;
>>>
>>>    	nvmem_np = of_get_next_parent(cell_np);
>>>    	if (!nvmem_np)
>>> @@ -824,6 +816,32 @@ err_mem:
>>>
>>>    	return ERR_PTR(rval);
>>>    }
>>> +EXPORT_SYMBOL_GPL(of_nvmem_cell_get_direct);
>>> +
>>> +/**
>>> + * of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
>>> + *
>>> + * @dev node: Device tree node that uses the nvmem cell
>>> + * @id: nvmem cell name from nvmem-cell-names property.
>>> + *
>>> + * Return: Will be an ERR_PTR() on error or a valid pointer
>>> + * to a struct nvmem_cell.  The nvmem_cell will be freed by the
>>> + * nvmem_cell_put().
>>> + */
>>> +struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>> +					    const char *name)
>>> +{
>>> +	struct device_node *cell_np;
>>> +	int index;
>>> +
>>> +	index = of_property_match_string(np, "nvmem-cell-names", name);
>>> +
>>> +	cell_np = of_parse_phandle(np, "nvmem-cells", index);
>>> +	if (!cell_np)
>>> +		return ERR_PTR(-EINVAL);
>>> +
>>> +	return of_nvmem_cell_get_direct(cell_np);
>>> +}
>>>    EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
>>>    #endif
>>>
>>> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
>>> index 9bb77d3..bf879fc 100644
>>> --- a/include/linux/nvmem-consumer.h
>>> +++ b/include/linux/nvmem-consumer.h
>>> @@ -136,6 +136,7 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
>>>    #endif /* CONFIG_NVMEM */
>>>
>>>    #if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
>>> +struct nvmem_cell *of_nvmem_cell_get_direct(struct device_node *cell_np);
>>>    struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
>>>    				     const char *name);
>>>    struct nvmem_device *of_nvmem_device_get(struct device_node *np,
>>>