Re: [PATCH v2 01/29] nvmem: add support for cell lookups

[Srinivas Kandagatla]

On 28/08/18 15:41, Bartosz Golaszewski wrote:
> 2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <srinivas.kandagatla@xxxxxxxxxx>:
> >
...
> > > I would like to support an additional use case here: the provider is
> > > generic and is not aware of its cells at all. Since the only way of
> > > defining nvmem cells is through DT or nvmem_config, we lack a way to
> > > allow machine code to define cells without the provider code being
> > > aware.
> >
> >
> > machine driver should be able to do
> > nvmem_device_get()
> > nvmem_add_cells()
>
> Indeed, I missed the fact that you can retrieve the nvmem device by
> name. Except that we cannot know that the nvmem provider has been
> registered yet when calling nvmem_device_get(). This could potentially
> be solved by my other patch that adds notifiers to nvmem, but it would
> require much more boilerplate code in every board file. I think that
> removing nvmem_cell_info from nvmem_config and having external cell
> definitions would be cleaner.

Yes, notifiers would work!

...
> > > Yes, I would like to rework nvmem a bit. I don't see any non-DT users
> > > defining nvmem-cells using nvmem_config. I think that what we need is
> > > a way of specifying cell config outside of nvmem providers in some
> > > kind of structures. These tables would reference the provider by name
> > > and define the cells. Then we would have an additional lookup
> > > structure which would associate the consumer (by dev_id and con_id,
> > > where dev_id could optionally be NULL and where we would fall back to
> > > using con_id only) and the nvmem provider + cell together. Similarly
> > > to how GPIO consumers are associated with the gpiochip and hwnum. How
> > > does it sound?
> >
> > Yes, sounds good.
> >
> > Correct me if am wrong!
> > You should be able to add the new cells using struct nvmem_cell_info and add
> > them to particular provider using nvmem_add_cells().
> >
> > Sounds like thats exactly what nvmem_add_lookup_table() would look like.
> >
> > We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
> > nvmem cell which is specific to the provider. This cell can be used by the
> > machine driver to read/write.
>
> Except that we could do it lazily - when the nvmem provider actually
> gets registered instead of doing it right away and risking that the
> device isn't even there yet.
Yes, it makes more sense to do it once the provider is actually present!

> > > > > BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
> > > > > instance even if the cell for this node was already added to the nvmem
> > > > > device.
> > > >
> > > >
> > > > I hope you got the reason why of_nvmem_cell_get() always allocates new
> > > > instance for every get!!
> > >
> > >
> > >
> > > I admit I didn't test it, but just from reading the code it seems like
> > > in nvmem_cell_get() for DT-users we'll always get to
> > > of_nvmem_cell_get() and in there we always end up calling line 873:
> > > cell = kzalloc(sizeof(*cell), GFP_KERNEL);
> > That is correct, this cell is created when we do a get and release when we
> > do a put().
>
> Shouldn't we add the cell to the list, and check first if it's there
> and only create it if not?
Yes I agree, duplicate entry checks are missing!

--srini
> Bart