Re: [PATCH v2 1/3] nvmem: core: introduce cells parser

[Vadym Kochan]

Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> writes:

> On 27/09/2021 08:50, Vadym Kochan wrote:
>>>>> Currently I can test only on at24 devices. From the:
>>>>>
>>>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>>>>>
>>>>> "
>>>>> Each ONIE system must include non-volatile storage which contains vital
>>>>> product data assigned by the manufacturer. The non-volatile storage
>>>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>>>>> sector or any other non-volatile storage medium.
>>>>> "
>>>>>
>>>>> I am not aware about other nvmem devices which are used for existing
>>>>> ONIE supported boards.
>>>>>
>>>>>> As long as you represent this parsing function as some library function,
>>>>>> I do not see any issue.
>>>>>> If any exported symbol is available for this then any nvmem provider
>>>>>> could use it.
>>>>>>
>>>>>> --srini
>>>>>>
>>> Hi Srinivas,
>>>
>>> Can I note here that I would like to parse
>>> TLV data from an SPI-NOR device to NVMEM cells.
>>> The same general use case (getting mac-address from OEM data).
>>>
>>> Was planning to base my work on this series, as well as
>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@xxxxxxx/
>>> (thanks for pointing that out Srinivas)
>>>
>>> Cheers,
>> What about at least to have just one call in core.c to make it a bit
>> de-coupled, like:
>
> Why do you want to decouple this? the provider driver should be very 
> well aware of the format the data layout.
>

In my understanding nvmem device should not aware about the data layout
(in case it does not rely on device's specific characteristics). Same
cells layout (TLV, etc) might exist on other nvmem devices.

> Its fine to an extent to adding parse_cells() callback in nvmem_config.
>

OK, in that case it will require small change in the core.

>> 
>> core.c
>> 
>> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>> {
>> ...
>>           rval = nvmem_add_cells_from_table(nvmem);
>>           if (rval)
>>                   goto err_remove_cells;
>> 
>> +        rval = nvmem_parse_cells(nvmem, of);
>> +        if (rval) {
>> +        /* err handling */
>> +        }
>> +
>>           rval = nvmem_add_cells_from_of(nvmem);
>>           if (rval)
>>                   goto err_remove_cells;
>> 
>>           blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>> 
>>           return nvmem;
>> 
>> ...
>> 
>> }
>> 
>
>> somewhere in nvmem-parser.c:
>
> However this is totally over kill.
>
>> 
>> /* retreive parser name from of_node and call appropriate function to parse
>>     non-fixed cells and update via of_update */
>
> This is completely provider drivers job, nothing nvmem core should worry 
> about.
>
> If you have concern of having code duplicated then we could make some of 
> the common functions as library functions, But it still is within the 
> scope of provider drivers.
>

Do I understand correctly that this parser function should be exported
from at24.c (in case of ONIE) and not from a separate C module ? Or
it just means that if there will be more users of this parsing function
then it might be moved to separate C module ?

> --srini
>

BTW, what if such change will be declined by particular nvmem driver
maintainer ?

>> int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node)
>> {
>>      ...
>> }
>> 
>> ?
>
>
>> 
>> Regards,