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

From: Srinivas Kandagatla
Date: Tue Sep 28 2021 - 10:40:03 EST




On 28/09/2021 15:11, Vadym Kochan wrote:
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.

But this new binding will still belong to the provider driver that you want to support parsing.


--srini