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

[Vadym Kochan]

Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> writes:

> On 20/09/2021 13:29, Vadym Kochan wrote:
>> 
>> Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx> writes:
>> 
>>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>>> before register.
>>>>>
>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> some_dev_node {
>>>>>>> 	compatible = "xxx";
>>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>>> };
>>>>>>>
>>>>>>> == CODE ==
>>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>>> ==========
>>>>>>>
>>>>>>>
>>>>>>> If you notice the mac_address node reg is 0.
>>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>>> with tlv name.
>>>>>>>
>>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>>> update existing cells parsed from OF.
>>>>>>
>>>>> Lets keep the core code clean for now, I would expect the provider
>>>>> driver along with parser function to do update the nodes.
>>>>>
>>>>> --srini
>>>>>
>>>> core.c sequence:
>>>>
>>>> 1) after cells parsed from OF:
>>>>
>>>> 2) lookup the parser
>>>>
>>>> 3) parser->cells_parse(ctx, table)
>>>>
>>>> 3.a) update existing cells matched by name from table
>>>>
>>>> 4) parser->cells_clean(ctx, table)
>>>> /* to free cell's_info allocated by the parser driver */
>>>>
>>>> as alternative way, I think it would be more generic to
>>>> provide nvmem-provider.h API to update the existing cell info,
>>>> however it makes sense only when cells were parsed
>>>> by the cell parser, in the other situations the
>>>> cell should be well defined.
>>>>
>>>> with this approach the parser driver will be just called
>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>>> for each parsed cells.
>>>
>>> TBH, This is an over kill.
>>>
>>> In nvmem provider driver probe you can parse the tlv data and update the
>>> dt nodes before nvmem register.
>>>
>>> rest of the code should just work as it is.
>>>
>>> --srini
>> 
>> You mean to parse TLV in the particular nvmem provider driver (for
>> example in at24 driver) ? If so, then it will not allow to parse
>> this TLV format from the other kinds of nvmem.
>
> Why not?
>

Well, I think that nvmem driver and TLV parsing should somehow be
de-coupled from each other like block devices and FS does. BUT,
in case this TLV data will be used only on at24 devices then
may be it is OK.

> Can you also tell us which other nvmem providers are you going to test 
> this on?
>

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
>
>
>> 
>>>
>>>
>>>>
>>