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

[Srinivas Kandagatla]

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
>