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

[Stefan Agner]

On 2016-07-08 10:23, Srinivas Kandagatla wrote:
> On 08/07/16 17:42, Stefan Agner wrote:
>> On 2016-07-08 08:41, Srinivas Kandagatla wrote:
>>> On 07/07/16 14:48, maitysanchayan@xxxxxxxxx wrote:
>>>> Hello Srinivas,
>>>>
>>>> On 16-07-07 1
> 
> ...
> 
>>>>>>
>>>>>> 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?
>>>>
>>>> Sorry for not providing the background directly. The patches before this
>>>> series used that approach. In the previous discussions it has been pointed
>>>> out that it is not acceptable to have additional device tree bindings for
>>>> providing data that the driver wants at the SoC node level or to have bindings
>>>> just for the SoC bus driver alone since we aren't really describing the
>>>> hardware.
>>>>
>>> SOC driver seems to search for an arbitrary node by its name, which is
>>> not a binding and can break anytime in cases If the scope of nvmem
>>> provider is out of soc node or if the nvmem cells are not named as
>>> expected. That looks very fragile.
>>
>> In that case, that just "won't happen" because the soc driver is a very
>> soc specific driver only used for this device tree. We it will always
>> bind to that high level soc node.
>>
>>>
>>> If the soc node is actual consumer of nvmem cells, I see no reason why
>>> we should not use proper nvmem bindings?
>>
>> There is a reason: We don't describe the hardware with it...
>>
>> The cfg0/cfg1 register which Sanchayan needs to read in the soc bus
>> driver are just two register with a unique ID of the SoC. In whatever
> 
> "Unique ID of the SOC" doesn't this mean that its a part of soc hw
> description/configuration/setup?
> 
> Am still not clear why this setup any different to other use cases
> like mac address/calibration data?
> 
> I still feel that this should be described in the DT.
> 
> Rob,
>  What do you think?

[added Rob to "To"]

Rob, can you comment?

--
Stefan

> 
> 
>> driver throughout the system we use that ID (e.g. in a random generator
>> for initialization) we never describe an actual hardware relation... Its
>> just software and how we use that unique ID. The device tree is ment to
>> describe hardware. Hence the NVMEM consumer binding is not suited for
>> such NVMEM cells...
>>
>> By describing the NVMEM cells location in device tree (producer API, the
>> NVMEM cells are in hardware at that location, so using the device tree
>> for that part is fine) and hard coding the NVMEM cell we need in the
>> driver code we don't violate the device tree matra "describe the
>> hardware"...
> 
> IMHO, We should indeed describe the SOC hardware and its relationship
> w.r.t to nvmem provider in device tree. Reasoning being these both are
> some form of IP blocks/hw which depend on each other.
> 
>>
>> Looking-up the nodes direcly is what Rob suggested here:
>> https://lkml.org/lkml/2016/5/23/573
> 
> I did read this, I was not convinced that we should do a direct lookup
> for nvmem cells.
> 
> thanks,
> srini
>>
>>>
>>> Given the fact that the patch is potentially bypassing the nvmem
>>> bindings, am not happy to take it!
>>
>> If you can provide a solution acceptable by the device tree folks and
>> works without this patch, I am happy to do it...
> 
> 
>>
>> Btw, I am not entirely happy with the API name, but did not had a better
>> idea... And we we should probably add a note that the device tree
>> consumer binding is the preferred way to do it.
>>
>> --
>> Stefan
>>
>>
>>>
>>> thanks,
>>> srini
>>>
>>>> For the discussion,
>>>> https://lkml.org/lkml/2016/5/23/573
>>>> https://lkml.org/lkml/2016/5/2/71
>>>>
>>>> Regards,
>>>> Sanchayan.
>>>>
>>>>
>>>>>
>>>>>> 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@xxxxxxxxx>
>>>>>>>> ---
>>>>>>>>      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,
>>>>>>>>