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

[Vadym Kochan]

Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> writes:

> On 28/09/2021 14:31, Vadym Kochan wrote:
>>>>> 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.
>> 
> How would provider driver parse this without even knowing data layout?
>
>
>>> 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 ?
> yes.
> For now am not really sure how many users are for such parsing function.
>
>> 
>>> --srini
>>>
>> BTW, what if such change will be declined by particular nvmem driver
>> maintainer ?
>
> You would need some changes to provider driver to be able to flag that 
> there is some kind of parsing required anyway.
>

It might be some new property in device-tree which can be used also
for the other providers.

> --srini
>>