Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
To:     Vadym Kochan <vadym.kochan@plvision.eu>
Cc:     Rob Herring <robh+dt@kernel.org>, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, Robert Marko <robert.marko@sartura.hr>
References: <20210608190327.22071-1-vadym.kochan@plvision.eu>
 <20210608190327.22071-2-vadym.kochan@plvision.eu>
 <43023500-dd6a-5180-057e-cecc1f1b6500@linaro.org>
 <20210616123356.GA9951@plvision.eu>
 <fea907ed-06ce-5c82-667d-d11f3e902616@linaro.org>
 <vrcxh2zgsnl841.fsf@plvision.eu>
 <7e6d75ed-cebc-597f-7062-34261d184968@linaro.org>
 <vrcxh2pmt3bl4h.fsf@plvision.eu>
 <0e471789-fe29-b747-5153-75c9b4616c7f@linaro.org>
 <vrcxh2o88nbias.fsf@plvision.eu>
 <1da03714-8f23-1004-e89a-891e4599e04a@linaro.org>
 <vrcxh2mto7bfcb.fsf@plvision.eu>
 <1e146349-9fef-972b-9084-577f02d5168b@linaro.org>
 <vrcxh2lf3rbcjc.fsf@plvision.eu>
From:   Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Message-ID: <169d3f36-4297-32a3-3d23-824989625b26@linaro.org>
Date:   Mon, 20 Sep 2021 14:40:49 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.11.0
MIME-Version: 1.0
In-Reply-To: <vrcxh2lf3rbcjc.fsf@plvision.eu>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 20/09/2021 14:29, Vadym Kochan wrote:
> 
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
> 
>> On 20/09/2021 13:29, Vadym Kochan wrote:
>>>
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> 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
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>
>