| Message ID | 20180626102245.30711-2-brgl@bgdev.pl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Thanks for the patch, On 26/06/18 11:22, Bartosz Golaszewski wrote: > From: Bartosz Golaszewski <bgolaszewski@baylibre.com> > > We can currently only register nvmem cells from device tree or by > manually calling nvmem_add_cells(). The latter options however forces > users to make sure that the nvmem provider with which the cells are > associated is registered before the call. > > This patch proposes a new solution inspired by other frameworks that > offer resource lookups (GPIO, PWM etc.). It adds a function that allows > machine code to register nvmem lookup which are later lazily used to > add corresponding nvmem cells. > Overall the idea look fine to me. This needs to be documented in ./Documentation/nvmem/nvmem.txt > Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> > --- > drivers/nvmem/core.c | 57 +++++++++++++++++++++++++++++++++- > include/linux/nvmem-consumer.h | 6 ++++ > include/linux/nvmem-provider.h | 6 ++++ > 3 files changed, 68 insertions(+), 1 deletion(-) > > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c > index b5b0cdc21d01..a2e87b464319 100644 > --- a/drivers/nvmem/core.c > +++ b/drivers/nvmem/core.c > @@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida); > static LIST_HEAD(nvmem_cells); > static DEFINE_MUTEX(nvmem_cells_mutex); > > +static LIST_HEAD(nvmem_cell_lookups); > +static DEFINE_MUTEX(nvmem_lookup_mutex); > + > #ifdef CONFIG_DEBUG_LOCK_ALLOC > static struct lock_class_key eeprom_lock_key; > #endif > @@ -247,6 +250,23 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = { > NULL, > }; > > +/** > + * nvmem_register_lookup() - register a number of nvmem cell lookup entries > + * Can we rename this to nvmem_add_lookup_table()? register sound bit heavy here. We should also have something like nvmem_remove_lookup_table() for consistency, and it should ensure that it clears the cells entry too. > + * @lookup: array of nvmem cell lookup entries > + * @nentries: number of lookup entries in the array > + */ > +void nvmem_register_lookup(struct nvmem_cell_lookup *lookup, size_t nentries) > +{ > + int i; > + > + mutex_lock(&nvmem_lookup_mutex); > + for (i = 0; i < nentries; i++) > + list_add_tail(&lookup[i].list, &nvmem_cell_lookups); > + mutex_unlock(&nvmem_lookup_mutex); > +} > +EXPORT_SYMBOL_GPL(nvmem_register_lookup); > + > static void nvmem_release(struct device *dev) > { > struct nvmem_device *nvmem = to_nvmem_device(dev); > @@ -916,6 +936,37 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, > EXPORT_SYMBOL_GPL(of_nvmem_cell_get); > #endif > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
2018-06-26 13:06 GMT+02:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>: > Thanks for the patch, > > On 26/06/18 11:22, Bartosz Golaszewski wrote: >> >> From: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> >> We can currently only register nvmem cells from device tree or by >> manually calling nvmem_add_cells(). The latter options however forces >> users to make sure that the nvmem provider with which the cells are >> associated is registered before the call. >> >> This patch proposes a new solution inspired by other frameworks that >> offer resource lookups (GPIO, PWM etc.). It adds a function that allows >> machine code to register nvmem lookup which are later lazily used to >> add corresponding nvmem cells. >> > Overall the idea look fine to me. > > This needs to be documented in ./Documentation/nvmem/nvmem.txt > >> Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com> >> --- >> drivers/nvmem/core.c | 57 +++++++++++++++++++++++++++++++++- >> include/linux/nvmem-consumer.h | 6 ++++ >> include/linux/nvmem-provider.h | 6 ++++ >> 3 files changed, 68 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c >> index b5b0cdc21d01..a2e87b464319 100644 >> --- a/drivers/nvmem/core.c >> +++ b/drivers/nvmem/core.c >> @@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida); >> static LIST_HEAD(nvmem_cells); >> static DEFINE_MUTEX(nvmem_cells_mutex); >> +static LIST_HEAD(nvmem_cell_lookups); >> +static DEFINE_MUTEX(nvmem_lookup_mutex); >> + >> #ifdef CONFIG_DEBUG_LOCK_ALLOC >> static struct lock_class_key eeprom_lock_key; >> #endif >> @@ -247,6 +250,23 @@ static const struct attribute_group >> *nvmem_ro_root_dev_groups[] = { >> NULL, >> }; >> +/** >> + * nvmem_register_lookup() - register a number of nvmem cell lookup >> entries >> + * > > > Can we rename this to nvmem_add_lookup_table()? > register sound bit heavy here. > > We should also have something like nvmem_remove_lookup_table() for > consistency, and it should ensure that it clears the cells entry too. > What do you mean by clearing the cells entry exactly? Bart > >> + * @lookup: array of nvmem cell lookup entries >> + * @nentries: number of lookup entries in the array >> + */ >> +void nvmem_register_lookup(struct nvmem_cell_lookup *lookup, size_t >> nentries) >> +{ >> + int i; >> + > + mutex_lock(&nvmem_lookup_mutex); >> + for (i = 0; i < nentries; i++) >> + list_add_tail(&lookup[i].list, &nvmem_cell_lookups); >> + mutex_unlock(&nvmem_lookup_mutex); >> +} >> +EXPORT_SYMBOL_GPL(nvmem_register_lookup); >> + >> static void nvmem_release(struct device *dev) >> { >> struct nvmem_device *nvmem = to_nvmem_device(dev); >> @@ -916,6 +936,37 @@ struct nvmem_cell *of_nvmem_cell_get(struct >> device_node *np, >> EXPORT_SYMBOL_GPL(of_nvmem_cell_get); >> #endif >> -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 28/06/18 08:56, Bartosz Golaszewski wrote: >> We should also have something like nvmem_remove_lookup_table() for >> consistency, and it should ensure that it clears the cells entry too. >> > What do you mean by clearing the cells entry exactly? I meant, Removing entry from the nvmem_cells list. Initially I though we should remove the entry form nvmem_cells list, but It would complicate this path if some consumer has reference to it. So, just removing the lookup list should be good! Lets not worry about removing from nvmem_cells list. --srini > > Bart > -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c index b5b0cdc21d01..a2e87b464319 100644 --- a/drivers/nvmem/core.c +++ b/drivers/nvmem/core.c @@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida); static LIST_HEAD(nvmem_cells); static DEFINE_MUTEX(nvmem_cells_mutex); +static LIST_HEAD(nvmem_cell_lookups); +static DEFINE_MUTEX(nvmem_lookup_mutex); + #ifdef CONFIG_DEBUG_LOCK_ALLOC static struct lock_class_key eeprom_lock_key; #endif @@ -247,6 +250,23 @@ static const struct attribute_group *nvmem_ro_root_dev_groups[] = { NULL, }; +/** + * nvmem_register_lookup() - register a number of nvmem cell lookup entries + * + * @lookup: array of nvmem cell lookup entries + * @nentries: number of lookup entries in the array + */ +void nvmem_register_lookup(struct nvmem_cell_lookup *lookup, size_t nentries) +{ + int i; + + mutex_lock(&nvmem_lookup_mutex); + for (i = 0; i < nentries; i++) + list_add_tail(&lookup[i].list, &nvmem_cell_lookups); + mutex_unlock(&nvmem_lookup_mutex); +} +EXPORT_SYMBOL_GPL(nvmem_register_lookup); + static void nvmem_release(struct device *dev) { struct nvmem_device *nvmem = to_nvmem_device(dev); @@ -916,6 +936,37 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, EXPORT_SYMBOL_GPL(of_nvmem_cell_get); #endif +static struct nvmem_cell *nvmem_cell_from_lookup(const char *cell_id) +{ + struct nvmem_cell *cell = ERR_PTR(-EPROBE_DEFER); + struct nvmem_cell_lookup *lookup; + struct nvmem_device *nvmem; + int rc; + + mutex_lock(&nvmem_lookup_mutex); + + list_for_each_entry(lookup, &nvmem_cell_lookups, list) { + if (strcmp(cell_id, lookup->info.name) == 0) { + nvmem = nvmem_find(lookup->nvmem_name); + if (!nvmem) + goto out; + + rc = nvmem_add_cells(nvmem, &lookup->info, 1); + if (rc) { + cell = ERR_PTR(rc); + goto out; + } + + cell = nvmem_cell_get_from_list(cell_id); + break; + } + } + +out: + mutex_unlock(&nvmem_lookup_mutex); + return cell; +} + /** * nvmem_cell_get() - Get nvmem cell of device form a given cell name * @@ -936,7 +987,11 @@ struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *cell_id) return cell; } - return nvmem_cell_get_from_list(cell_id); + cell = nvmem_cell_get_from_list(cell_id); + if (!IS_ERR(cell) || PTR_ERR(cell) == -EPROBE_DEFER) + return cell; + + return nvmem_cell_from_lookup(cell_id); } EXPORT_SYMBOL_GPL(nvmem_cell_get); diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h index 4e85447f7860..f4b5d3186e94 100644 --- a/include/linux/nvmem-consumer.h +++ b/include/linux/nvmem-consumer.h @@ -29,6 +29,12 @@ struct nvmem_cell_info { unsigned int nbits; }; +struct nvmem_cell_lookup { + struct nvmem_cell_info info; + struct list_head list; + const char *nvmem_name; +}; + #if IS_ENABLED(CONFIG_NVMEM) /* Cell based interface */ diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h index 24def6ad09bb..766c0a96c113 100644 --- a/include/linux/nvmem-provider.h +++ b/include/linux/nvmem-provider.h @@ -17,6 +17,7 @@ struct nvmem_device; struct nvmem_cell_info; +struct nvmem_cell_lookup; typedef int (*nvmem_reg_read_t)(void *priv, unsigned int offset, void *val, size_t bytes); typedef int (*nvmem_reg_write_t)(void *priv, unsigned int offset, @@ -72,6 +73,8 @@ struct nvmem_config { struct nvmem_device *nvmem_register(const struct nvmem_config *cfg); int nvmem_unregister(struct nvmem_device *nvmem); +void nvmem_register_lookup(struct nvmem_cell_lookup *lookup, size_t nentries); + struct nvmem_device *devm_nvmem_register(struct device *dev, const struct nvmem_config *cfg); @@ -92,6 +95,9 @@ static inline int nvmem_unregister(struct nvmem_device *nvmem) return -ENOSYS; } +static inline void +nvmem_register_lookup(struct nvmem_cell_lookup *lookup, size_t nentries) {} + static inline struct nvmem_device * devm_nvmem_register(struct device *dev, const struct nvmem_config *c) {