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

[Vadym Kochan]

Hi John, Srini,

John Thomson <john@xxxxxxxxxxxxxxxxxxxxxxxxxxx> writes:

> On Mon, 20 Sep 2021, at 13:40, Srinivas Kandagatla wrote:
>> On 20/09/2021 14:29, Vadym Kochan wrote:
>>> 
>>> 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.
>>> 
>>
>> It has to be decoupled yes, which could be at any level with simple 
>> library function to a infrastructure level..
>>
>> In this case with few or single users, doing with an additional 
>> infrastructure is a bit of over kill IMO.
>>
>>
>> --srini
>>>> 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
>>>>
>
> 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:

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:

/* retreive parser name from of_node and call appropriate function to parse
   non-fixed cells and update via of_update */
int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node)
{
    ...
}

?

Regards,
Vadym Kochan