currently this adds to the global cell list which is exactly like doing it
via nvmem_config.

quoted

quoted

quoted

with each cell referencing its owner nvmem device. I'm wondering if
this isn't some kind of inversion of ownership. Shouldn't each nvmem
device have a separate list of nvmem cells owned by it? What happens

This is mainly done for use case where consumer does not have idea of
provider name or any details.

It doesn't need to know the provider details, but in most subsystems
the core code associates such resources by dev_id and optional con_id
as Boris already said.

If dev_id here is referring to provider dev_id, then we already do that
using nvmem device apis, except in global cell list which makes dev_id
optional.

quoted

quoted

First thing non dt user should do is use "NVMEM device based consumer
APIs"

ex: First get handle to nvmem device using its nvmem provider name by
calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.

Also am not 100% sure how would maintaining cells list per nvmem provider
would help for the intended purpose of global list?

It would fix the use case where the consumer wants to use
nvmem_cell_get(dev, name) and two nvmem providers would have a cell
with the same name.

There is no code to enforce duplicate checks, so this would just decrease
the chances rather than fixing the problem totally.
I guess this is same problem

Finding cell by name without dev_id would still be an issue, am not too
concerned about this ATM.

However, the idea of having cells per provider does sound good to me.
We should also maintain list of providers in core as a lookup in cases where
dev_id is null.

I did hack up a patch, incase you might want to try:
I did only compile test.
---------------------------------->cut<-------------------------------
Author: Srinivas Kandagatla [off-list ref]
Date:   Tue Aug 28 13:46:21 2018 +0100

    nvmem: core: maintain per provider cell list

    Having a global cell list could be a issue in cases where the cell-id is
same across multiple providers. Making the cell list specific to provider
could avoid such issue by adding additional checks while addding cells.

    Signed-off-by: Srinivas Kandagatla [off-list ref]

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index aa1657831b70..29da603f2fa4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c

@@ -40,6 +40,8 @@ struct nvmem_device {
        struct device           *base_dev;
        nvmem_reg_read_t        reg_read;
        nvmem_reg_write_t       reg_write;
+       struct list_head        node;
+       struct list_head        cells;
        void *priv;
 };

@@ -57,9 +59,7 @@ struct nvmem_cell {

 static DEFINE_MUTEX(nvmem_mutex);
 static DEFINE_IDA(nvmem_ida);
-
-static LIST_HEAD(nvmem_cells);
-static DEFINE_MUTEX(nvmem_cells_mutex);
+static LIST_HEAD(nvmem_devices);

 #ifdef CONFIG_DEBUG_LOCK_ALLOC
 static struct lock_class_key eeprom_lock_key;

@@ -284,26 +284,28 @@ static struct nvmem_device *of_nvmem_find(struct

device_node *nvmem_np)

 static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
 {
-       struct nvmem_cell *p;
+       struct nvmem_device *d;

-       mutex_lock(&nvmem_cells_mutex);
-
-       list_for_each_entry(p, &nvmem_cells, node)
-               if (!strcmp(p->name, cell_id)) {
-                       mutex_unlock(&nvmem_cells_mutex);
-                       return p;
-               }
+       mutex_lock(&nvmem_mutex);
+       list_for_each_entry(d, &nvmem_devices, node) {
+               struct nvmem_cell *p;
+               list_for_each_entry(p, &d->cells, node)
+                       if (!strcmp(p->name, cell_id)) {
+                               mutex_unlock(&nvmem_mutex);
+                               return p;
+                       }
+       }

-       mutex_unlock(&nvmem_cells_mutex);
+       mutex_unlock(&nvmem_mutex);

        return NULL;
 }

 static void nvmem_cell_drop(struct nvmem_cell *cell)
 {
-       mutex_lock(&nvmem_cells_mutex);
+       mutex_lock(&nvmem_mutex);
        list_del(&cell->node);
-       mutex_unlock(&nvmem_cells_mutex);
+       mutex_unlock(&nvmem_mutex);
        kfree(cell);
 }

@@ -312,18 +314,18 @@ static void nvmem_device_remove_all_cells(const struct

nvmem_device *nvmem)
        struct nvmem_cell *cell;
        struct list_head *p, *n;

-       list_for_each_safe(p, n, &nvmem_cells) {
+       list_for_each_safe(p, n, &nvmem->cells) {
                cell = list_entry(p, struct nvmem_cell, node);
                if (cell->nvmem == nvmem)
                        nvmem_cell_drop(cell);
        }
 }

-static void nvmem_cell_add(struct nvmem_cell *cell)
+static void nvmem_cell_add(struct nvmem_device *nvmem, struct nvmem_cell
*cell)
 {
-       mutex_lock(&nvmem_cells_mutex);
-       list_add_tail(&cell->node, &nvmem_cells);
-       mutex_unlock(&nvmem_cells_mutex);
+       mutex_lock(&nvmem_mutex);
+       list_add_tail(&cell->node, &nvmem->cells);
+       mutex_unlock(&nvmem_mutex);
 }

 static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,

@@ -385,7 +387,7 @@ int nvmem_add_cells(struct nvmem_device *nvmem,
                        goto err;
                }

-               nvmem_cell_add(cells[i]);
+               nvmem_cell_add(nvmem, cells[i]);
        }

        /* remove tmp array */

@@ -519,6 +521,10 @@ struct nvmem_device *nvmem_register(const struct

nvmem_config *config)
        if (config->cells)
                nvmem_add_cells(nvmem, config->cells, config->ncells);

+       mutex_lock(&nvmem_mutex);
+       list_add_tail(&nvmem->node, &nvmem_devices);
+       mutex_unlock(&nvmem_mutex);
+
        return nvmem;

 err_device_del:

@@ -544,6 +550,8 @@ int nvmem_unregister(struct nvmem_device *nvmem)
                mutex_unlock(&nvmem_mutex);
                return -EBUSY;
        }
+
+       list_del(&nvmem->node);
        mutex_unlock(&nvmem_mutex);

        if (nvmem->flags & FLAG_COMPAT)

@@ -899,7 +907,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node

*np,
                goto err_sanity;
        }

-       nvmem_cell_add(cell);
+       nvmem_cell_add(nvmem, cell);

        return cell;

---------------------------------->cut<-------------------------------

quoted

Next we could add a way to associate dev_ids with nvmem cells.

quoted

quoted

if we have two nvmem providers with the same names for cells? I'm

Yes, it would return the first instance.. which is a known issue.
Am not really sure this is a big problem as of today! but am open for any
better suggestions!

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.