From: Bartosz Golaszewski <[email protected]>
This series contains nvmem framework changes prerequisite for further
development of my previous series[1] that aims at removal of the
platform data struct from at24 EEPROM driver.
First we remove unused APIs and the global cell list. We then switch
to using kref instead of manual user counting. Next we simplify the
provider unregistration by removing the return value from
nvmem_unregister().
Next three patches improve the framework by adding a notifier chain
for future use and fixing the issue with nvmem device names.
Finally we add support for cell definitions, cell lookups and make
DT systems resolve the nvmem cells during provider's registration.
Last patches just use the SPDX license identifiers and make the
naming convention for some arguments more consistent.
Tested both DT and non-DT use cases.
[1] https://lkml.org/lkml/2018/8/10/149
v1 -> v2:
- extended the lookup structure with a proper con_id independent from the
cell name defined in the cell definition table
- added a patch that makes the naming convention for the cell name argument
in the nvmem_cell_get() family of functions consistent
- there were two users of nvmem_unregister() that still checked the return
value, now switched to devm_nvmem_register()
- fixed build failures reported by kbuild test robot
Bartosz Golaszewski (16):
nvmem: remove unused APIs
nvmem: remove the global cell list
nvmem: use kref
nvmem: lpc18xx_eeprom: use devm_nvmem_register()
nvmem: sunxi_sid: use devm_nvmem_register()
nvmem: mxs-ocotp: use devm_nvmem_register()
nvmem: change the signature of nvmem_unregister()
nvmem: provide nvmem_dev_name()
nvmem: remove the name field from struct nvmem_device
nvmem: add a notifier chain
nvmem: add support for cell info
nvmem: resolve cells from DT at registration time
nvmem: add support for cell lookups from machine code
Documentation: nvmem: document cell tables and lookup entries
nvmem: use SPDX license identifiers
nvmem: make the naming of arguments in nvmem_cell_get() consistent
Documentation/nvmem/nvmem.txt | 31 ++
MAINTAINERS | 1 +
drivers/nvmem/core.c | 676 ++++++++++++++++++---------------
drivers/nvmem/lpc18xx_eeprom.c | 6 +-
drivers/nvmem/mxs-ocotp.c | 4 +-
drivers/nvmem/sunxi_sid.c | 20 +-
include/linux/nvmem-consumer.h | 62 ++-
include/linux/nvmem-machine.h | 57 +++
include/linux/nvmem-provider.h | 27 +-
9 files changed, 493 insertions(+), 391 deletions(-)
create mode 100644 include/linux/nvmem-machine.h
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
The argument representing the cell name in the nvmem_cell_get() family
of functions is not consistend between function prototypes and
definitions. Name it 'id' in all those routines. This is in line with
other frameworks and can represent both the DT cell name from the
nvmem-cell-names property as well as the con_id field from cell
lookup entries.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 27 ++++++++++++++-------------
include/linux/nvmem-consumer.h | 12 ++++++------
2 files changed, 20 insertions(+), 19 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 320b3cc975a0..efcc41ba487d 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -873,16 +873,15 @@ EXPORT_SYMBOL_GPL(devm_nvmem_device_get);
* of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
*
* @np: Device tree node that uses the nvmem cell.
- * @name: nvmem cell name from nvmem-cell-names property, or NULL
- * for the cell at index 0 (the lone cell with no accompanying
- * nvmem-cell-names property).
+ * @id: nvmem cell name from nvmem-cell-names property, or NULL
+ * for the cell at index 0 (the lone cell with no accompanying
+ * nvmem-cell-names property).
*
* Return: Will be an ERR_PTR() on error or a valid pointer
* to a struct nvmem_cell. The nvmem_cell will be freed by the
* nvmem_cell_put().
*/
-struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
- const char *name)
+struct nvmem_cell *of_nvmem_cell_get(struct device_node *np, const char *id)
{
struct device_node *cell_np, *nvmem_np;
struct nvmem_device *nvmem;
@@ -890,8 +889,8 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
int index = 0;
/* if cell name exists, find index to the name */
- if (name)
- index = of_property_match_string(np, "nvmem-cell-names", name);
+ if (id)
+ index = of_property_match_string(np, "nvmem-cell-names", id);
cell_np = of_parse_phandle(np, "nvmem-cells", index);
if (!cell_np)
@@ -959,22 +958,24 @@ nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
* nvmem_cell_get() - Get nvmem cell of device form a given cell name
*
* @dev: Device that requests the nvmem cell.
- * @cell_id: nvmem cell name to get.
+ * @id: nvmem cell name to get (this corresponds with the name from the
+ * nvmem-cell-names property for DT systems and with the con_id from
+ * the lookup entry for non-DT systems).
*
* Return: Will be an ERR_PTR() on error or a valid pointer
* to a struct nvmem_cell. The nvmem_cell will be freed by the
* nvmem_cell_put().
*/
-struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *cell_id)
+struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id)
{
if (dev->of_node)
- return of_nvmem_cell_get(dev->of_node, cell_id);
+ return of_nvmem_cell_get(dev->of_node, id);
- /* Only allow empty cell_id for DT systems. */
- if (!cell_id)
+ /* Only allow empty cell id for DT systems. */
+ if (!id)
return ERR_PTR(-EINVAL);
- return nvmem_cell_get_from_lookup(dev, cell_id);
+ return nvmem_cell_get_from_lookup(dev, id);
}
EXPORT_SYMBOL_GPL(nvmem_cell_get);
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 4c146379b9ba..31ff907c7fe7 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -27,8 +27,8 @@ enum {
#if IS_ENABLED(CONFIG_NVMEM)
/* Cell based interface */
-struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *name);
-struct nvmem_cell *devm_nvmem_cell_get(struct device *dev, const char *name);
+struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *id);
+struct nvmem_cell *devm_nvmem_cell_get(struct device *dev, const char *id);
void nvmem_cell_put(struct nvmem_cell *cell);
void devm_nvmem_cell_put(struct device *dev, struct nvmem_cell *cell);
void *nvmem_cell_read(struct nvmem_cell *cell, size_t *len);
@@ -54,13 +54,13 @@ int nvmem_unregister_notifier(struct notifier_block *nb);
#else
static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
- const char *name)
+ const char *id)
{
return ERR_PTR(-ENOSYS);
}
static inline struct nvmem_cell *devm_nvmem_cell_get(struct device *dev,
- const char *name)
+ const char *id)
{
return ERR_PTR(-ENOSYS);
}
@@ -145,12 +145,12 @@ static inline int nvmem_unregister_notifier(struct notifier_block *nb)
#if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
- const char *name);
+ const char *id);
struct nvmem_device *of_nvmem_device_get(struct device_node *np,
const char *name);
#else
static inline struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
- const char *name)
+ const char *id)
{
return ERR_PTR(-ENOSYS);
}
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Document the new nvmem kernel APIs.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
Documentation/nvmem/nvmem.txt | 31 +++++++++++++++++++++++++++++++
1 file changed, 31 insertions(+)
diff --git a/Documentation/nvmem/nvmem.txt b/Documentation/nvmem/nvmem.txt
index 8d8d8f58f96f..fc2fe4b18655 100644
--- a/Documentation/nvmem/nvmem.txt
+++ b/Documentation/nvmem/nvmem.txt
@@ -58,6 +58,37 @@ static int qfprom_probe(struct platform_device *pdev)
It is mandatory that the NVMEM provider has a regmap associated with its
struct device. Failure to do would return error code from nvmem_register().
+Users of board files can define and register nvmem cells using the
+nvmem_cell_table struct:
+
+static struct nvmem_cell_info foo_nvmem_cells[] = {
+ {
+ .name = "macaddr",
+ .offset = 0x7f00,
+ .bytes = ETH_ALEN,
+ }
+};
+
+static struct nvmem_cell_table foo_nvmem_cell_table = {
+ .nvmem_name = "i2c-eeprom",
+ .cells = foo_nvmem_cells,
+ .ncells = ARRAY_SIZE(foo_nvmem_cells),
+};
+
+nvmem_add_cell_table(&foo_nvmem_cell_table);
+
+Additionally it is possible to create nvmem cell lookup entries and register
+them with the nvmem framework from machine code as shown in the example below:
+
+static struct nvmem_cell_lookup foo_nvmem_lookup = {
+ .nvmem_name = "i2c-eeprom",
+ .cell_name = "macaddr",
+ .dev_id = "foo_mac.0",
+ .con_id = "mac-address",
+};
+
+nvmem_add_cell_lookups(&foo_nvmem_lookup, 1);
+
NVMEM Consumers
+++++++++++++++
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Currently we're creating a new cell structure everytime a DT users
calls nvmem_cell_get().
Change this behavior by resolving the cells during nvmem provider
registration and adding all cells to the provider's list. Make
of_nvmem_cell_get() just parse the phandle and look the cell up
in the relevant provider's list.
Don't drop the cell in nvmem_cell_put().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 122 ++++++++++++++++++++++++++-----------------
1 file changed, 74 insertions(+), 48 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 854baa0559a1..da7a9d5beb33 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -405,6 +405,73 @@ static int nvmem_add_cells_from_list(struct nvmem_device *nvmem)
return rval;
}
+static struct nvmem_cell *
+nvmem_find_cell_by_index(struct nvmem_device *nvmem, int index)
+{
+ struct nvmem_cell *cell = NULL;
+ int i = 0;
+
+ mutex_lock(&nvmem_mutex);
+ list_for_each_entry(cell, &nvmem->cells, node) {
+ if (index == i++)
+ break;
+ }
+ mutex_unlock(&nvmem_mutex);
+
+ return cell;
+}
+
+static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
+{
+ struct device_node *parent, *child;
+ struct device *dev = &nvmem->dev;
+ struct nvmem_cell *cell;
+ const __be32 *addr;
+ int len;
+
+ parent = dev->of_node;
+
+ for_each_child_of_node(parent, child) {
+ addr = of_get_property(child, "reg", &len);
+ if (!addr || (len < 2 * sizeof(u32))) {
+ dev_err(dev, "nvmem: invalid reg on %pOF\n", child);
+ return -EINVAL;
+ }
+
+ cell = kzalloc(sizeof(*cell), GFP_KERNEL);
+ if (!cell)
+ return -ENOMEM;
+
+ cell->nvmem = nvmem;
+ cell->offset = be32_to_cpup(addr++);
+ cell->bytes = be32_to_cpup(addr);
+ cell->name = child->name;
+
+ addr = of_get_property(child, "bits", &len);
+ if (addr && len == (2 * sizeof(u32))) {
+ cell->bit_offset = be32_to_cpup(addr++);
+ cell->nbits = be32_to_cpup(addr);
+ }
+
+ if (cell->nbits)
+ cell->bytes = DIV_ROUND_UP(
+ cell->nbits + cell->bit_offset,
+ BITS_PER_BYTE);
+
+ if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
+ dev_err(dev, "cell %s unaligned to nvmem stride %d\n",
+ cell->name, nvmem->stride);
+ /* Cells already added will be freed later. */
+ kfree(cell);
+ return -EINVAL;
+ }
+
+ nvmem_cell_add(cell);
+ }
+
+ return 0;
+}
+
/**
* nvmem_register_notifier() - Register a notifier block for nvmem events.
*
@@ -514,6 +581,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
rval = nvmem_add_cells_from_list(nvmem);
if (rval)
goto err_teardown_compat;
+ rval = nvmem_add_cells_from_of(nvmem);
+ if (rval)
+ goto err_remove_cells;
rval = blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
if (rval)
@@ -811,10 +881,8 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
const char *name)
{
struct device_node *cell_np, *nvmem_np;
- struct nvmem_cell *cell;
struct nvmem_device *nvmem;
- const __be32 *addr;
- int rval, len;
+ struct nvmem_cell *cell;
int index = 0;
/* if cell name exists, find index to the name */
@@ -834,54 +902,13 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
if (IS_ERR(nvmem))
return ERR_CAST(nvmem);
- addr = of_get_property(cell_np, "reg", &len);
- if (!addr || (len < 2 * sizeof(u32))) {
- dev_err(&nvmem->dev, "nvmem: invalid reg on %pOF\n",
- cell_np);
- rval = -EINVAL;
- goto err_mem;
- }
-
- cell = kzalloc(sizeof(*cell), GFP_KERNEL);
+ cell = nvmem_find_cell_by_index(nvmem, index);
if (!cell) {
- rval = -ENOMEM;
- goto err_mem;
- }
-
- cell->nvmem = nvmem;
- cell->offset = be32_to_cpup(addr++);
- cell->bytes = be32_to_cpup(addr);
- cell->name = cell_np->name;
-
- addr = of_get_property(cell_np, "bits", &len);
- if (addr && len == (2 * sizeof(u32))) {
- cell->bit_offset = be32_to_cpup(addr++);
- cell->nbits = be32_to_cpup(addr);
- }
-
- if (cell->nbits)
- cell->bytes = DIV_ROUND_UP(cell->nbits + cell->bit_offset,
- BITS_PER_BYTE);
-
- if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
- dev_err(&nvmem->dev,
- "cell %s unaligned to nvmem stride %d\n",
- cell->name, nvmem->stride);
- rval = -EINVAL;
- goto err_sanity;
+ __nvmem_device_put(nvmem);
+ return ERR_PTR(-ENOENT);
}
- nvmem_cell_add(cell);
-
return cell;
-
-err_sanity:
- kfree(cell);
-
-err_mem:
- __nvmem_device_put(nvmem);
-
- return ERR_PTR(rval);
}
EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
#endif
@@ -978,7 +1005,6 @@ void nvmem_cell_put(struct nvmem_cell *cell)
struct nvmem_device *nvmem = cell->nvmem;
__nvmem_device_put(nvmem);
- nvmem_cell_drop(cell);
}
EXPORT_SYMBOL_GPL(nvmem_cell_put);
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Add new structs and routines allowing users to define nvmem cells from
machine code. This global list of entries is parsed when a provider
is registered and cells are associated with the relevant nvmem_device
struct.
A possible improvement for the future is to allow users to register
cell tables after the nvmem provider has been registered by updating
the cell list at each call to nvmem_(add|del)_cell_table().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
MAINTAINERS | 1 +
drivers/nvmem/core.c | 97 ++++++++++++++++++++++++++++++++++-
include/linux/nvmem-machine.h | 41 +++++++++++++++
3 files changed, 138 insertions(+), 1 deletion(-)
create mode 100644 include/linux/nvmem-machine.h
diff --git a/MAINTAINERS b/MAINTAINERS
index 9ad052aeac39..a520924bf0a9 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -10391,6 +10391,7 @@ F: drivers/nvmem/
F: Documentation/devicetree/bindings/nvmem/
F: Documentation/ABI/stable/sysfs-bus-nvmem
F: include/linux/nvmem-consumer.h
+F: include/linux/nvmem-machine.h
F: include/linux/nvmem-provider.h
NXP SGTL5000 DRIVER
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 17307015905a..854baa0559a1 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -21,6 +21,7 @@
#include <linux/init.h>
#include <linux/module.h>
#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-machine.h>
#include <linux/nvmem-provider.h>
#include <linux/of.h>
#include <linux/slab.h>
@@ -58,6 +59,9 @@ struct nvmem_cell {
static DEFINE_MUTEX(nvmem_mutex);
static DEFINE_IDA(nvmem_ida);
+static DEFINE_MUTEX(nvmem_cell_mutex);
+static LIST_HEAD(nvmem_cell_tables);
+
static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -341,6 +345,66 @@ static int nvmem_setup_compat(struct nvmem_device *nvmem,
return 0;
}
+static struct nvmem_cell *
+nvmem_cell_from_cell_info(struct nvmem_device *nvmem,
+ struct nvmem_cell_info *info)
+{
+ struct nvmem_cell *cell;
+
+ cell = kzalloc(sizeof(*cell), GFP_KERNEL);
+ if (!cell)
+ return ERR_PTR(-ENOMEM);
+
+ cell->nvmem = nvmem;
+ cell->offset = info->offset;
+ cell->bytes = info->bytes;
+ cell->name = info->name;
+ cell->bit_offset = info->bit_offset;
+ cell->nbits = info->nbits;
+
+ if (cell->nbits)
+ cell->bytes = DIV_ROUND_UP(cell->nbits + cell->bit_offset,
+ BITS_PER_BYTE);
+
+ if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
+ dev_err(&nvmem->dev,
+ "cell %s unaligned to nvmem stride %d\n",
+ cell->name, nvmem->stride);
+ kfree(cell);
+ return ERR_PTR(-EINVAL);
+ }
+
+ return cell;
+}
+
+static int nvmem_add_cells_from_list(struct nvmem_device *nvmem)
+{
+ struct nvmem_cell_table *table;
+ struct nvmem_cell_info *info;
+ struct nvmem_cell *cell;
+ int rval = 0, i;
+
+ mutex_lock(&nvmem_cell_mutex);
+ list_for_each_entry(table, &nvmem_cell_tables, node) {
+ if (strcmp(nvmem_dev_name(nvmem), table->nvmem_name) == 0) {
+ for (i = 0; i < table->ncells; i++) {
+ info = &table->cells[i];
+ cell = nvmem_cell_from_cell_info(nvmem, info);
+ if (IS_ERR(cell)) {
+ rval = PTR_ERR(cell);
+ goto out;
+ }
+
+ nvmem_cell_add(cell);
+ }
+ }
+ }
+
+out:
+ mutex_unlock(&nvmem_cell_mutex);
+ return rval;
+}
+
/**
* nvmem_register_notifier() - Register a notifier block for nvmem events.
*
@@ -447,13 +511,18 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
}
INIT_LIST_HEAD(&nvmem->cells);
+ rval = nvmem_add_cells_from_list(nvmem);
+ if (rval)
+ goto err_teardown_compat;
rval = blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
if (rval)
- goto err_teardown_compat;
+ goto err_remove_cells;
return nvmem;
+err_remove_cells:
+ nvmem_device_remove_all_cells(nvmem);
err_teardown_compat:
if (config->compat)
device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
@@ -1179,6 +1248,32 @@ int nvmem_device_write(struct nvmem_device *nvmem,
}
EXPORT_SYMBOL_GPL(nvmem_device_write);
+/**
+ * nvmem_add_cell_table() - register a table of cell info entries
+ *
+ * @table: table of cell info entries
+ */
+void nvmem_add_cell_table(struct nvmem_cell_table *table)
+{
+ mutex_lock(&nvmem_cell_mutex);
+ list_add_tail(&table->node, &nvmem_cell_tables);
+ mutex_unlock(&nvmem_cell_mutex);
+}
+EXPORT_SYMBOL_GPL(nvmem_add_cell_table);
+
+/**
+ * nvmem_del_cell_table() - remove a previously registered cell info table
+ *
+ * @table: table of cell info entries
+ */
+void nvmem_del_cell_table(struct nvmem_cell_table *table)
+{
+ mutex_lock(&nvmem_cell_mutex);
+ list_del(&table->node);
+ mutex_unlock(&nvmem_cell_mutex);
+}
+EXPORT_SYMBOL_GPL(nvmem_del_cell_table);
+
/**
* nvmem_dev_name() - Get the name of a given nvmem device.
*
diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
new file mode 100644
index 000000000000..1e199dfaacab
--- /dev/null
+++ b/include/linux/nvmem-machine.h
@@ -0,0 +1,41 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+/*
+ * nvmem framework machine code bindings
+ *
+ * Copyright (C) 2018 Bartosz Golaszewski <[email protected]>
+ */
+
+#ifndef _LINUX_NVMEM_MACHINE_H
+#define _LINUX_NVMEM_MACHINE_H
+
+#include <linux/nvmem-provider.h>
+#include <linux/list.h>
+
+struct nvmem_cell_info {
+ const char *name;
+ unsigned int offset;
+ unsigned int bytes;
+ unsigned int bit_offset;
+ unsigned int nbits;
+};
+
+struct nvmem_cell_table {
+ const char *nvmem_name;
+ struct nvmem_cell_info *cells;
+ size_t ncells;
+ struct list_head node;
+};
+
+#if IS_ENABLED(CONFIG_NVMEM)
+
+void nvmem_add_cell_table(struct nvmem_cell_table *table);
+void nvmem_del_cell_table(struct nvmem_cell_table *table);
+
+#else /* CONFIG_NVMEM */
+
+static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
+static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
+
+#endif /* CONFIG_NVMEM */
+
+#endif /* ifndef _LINUX_NVMEM_MACHINE_H */
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Add a way for machine code users to associate devices with nvmem cells.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 143 +++++++++++++++++++++++++++-------
include/linux/nvmem-machine.h | 16 ++++
2 files changed, 132 insertions(+), 27 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index da7a9d5beb33..9e2f9c993a07 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida);
static DEFINE_MUTEX(nvmem_cell_mutex);
static LIST_HEAD(nvmem_cell_tables);
+static DEFINE_MUTEX(nvmem_lookup_mutex);
+static LIST_HEAD(nvmem_lookup_list);
+
static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
#ifdef CONFIG_DEBUG_LOCK_ALLOC
@@ -285,6 +288,18 @@ static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
return to_nvmem_device(d);
}
+static struct nvmem_device *nvmem_find(const char *name)
+{
+ struct device *d;
+
+ d = bus_find_device_by_name(&nvmem_bus_type, NULL, name);
+
+ if (!d)
+ return NULL;
+
+ return to_nvmem_device(d);
+}
+
static void nvmem_cell_drop(struct nvmem_cell *cell)
{
mutex_lock(&nvmem_mutex);
@@ -421,6 +436,21 @@ nvmem_find_cell_by_index(struct nvmem_device *nvmem, int index)
return cell;
}
+static struct nvmem_cell *
+nvmem_find_cell_by_name(struct nvmem_device *nvmem, const char *cell_id)
+{
+ struct nvmem_cell *cell = NULL;
+
+ mutex_lock(&nvmem_mutex);
+ list_for_each_entry(cell, &nvmem->cells, node) {
+ if (strcmp(cell_id, cell->name) == 0)
+ break;
+ }
+ mutex_unlock(&nvmem_mutex);
+
+ return cell;
+}
+
static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
{
struct device_node *parent, *child;
@@ -691,22 +721,16 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
}
EXPORT_SYMBOL(devm_nvmem_unregister);
-static struct nvmem_device *__nvmem_device_get(struct device_node *np)
+static struct nvmem_device *
+__nvmem_device_get(struct device_node *np, const char *name)
{
struct nvmem_device *nvmem = NULL;
- if (!np)
- return ERR_PTR(-EINVAL);
-
mutex_lock(&nvmem_mutex);
-
- nvmem = of_nvmem_find(np);
- if (!nvmem) {
- mutex_unlock(&nvmem_mutex);
- return ERR_PTR(-EPROBE_DEFER);
- }
-
+ nvmem = np ? of_nvmem_find(np) : nvmem_find(name);
mutex_unlock(&nvmem_mutex);
+ if (!nvmem)
+ return ERR_PTR(-EPROBE_DEFER);
if (!try_module_get(nvmem->owner)) {
dev_err(&nvmem->dev,
@@ -726,18 +750,6 @@ static void __nvmem_device_put(struct nvmem_device *nvmem)
kref_put(&nvmem->refcnt, nvmem_device_release);
}
-static struct nvmem_device *nvmem_find(const char *name)
-{
- struct device *d;
-
- d = bus_find_device_by_name(&nvmem_bus_type, NULL, name);
-
- if (!d)
- return NULL;
-
- return to_nvmem_device(d);
-}
-
#if IS_ENABLED(CONFIG_OF)
/**
* of_nvmem_device_get() - Get nvmem device from a given id
@@ -760,7 +772,7 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
if (!nvmem_np)
return ERR_PTR(-EINVAL);
- return __nvmem_device_get(nvmem_np);
+ return __nvmem_device_get(nvmem_np, NULL);
}
EXPORT_SYMBOL_GPL(of_nvmem_device_get);
#endif
@@ -897,7 +909,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
if (!nvmem_np)
return ERR_PTR(-EINVAL);
- nvmem = __nvmem_device_get(nvmem_np);
+ nvmem = __nvmem_device_get(nvmem_np, NULL);
of_node_put(nvmem_np);
if (IS_ERR(nvmem))
return ERR_CAST(nvmem);
@@ -913,6 +925,44 @@ 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_get_from_lookup(struct device *dev, const char *con_id)
+{
+ struct nvmem_cell *cell = ERR_PTR(-ENOENT);
+ struct nvmem_cell_lookup *lookup;
+ struct nvmem_device *nvmem;
+ const char *dev_id;
+
+ if (!dev)
+ return ERR_PTR(-EINVAL);
+
+ dev_id = dev_name(dev);
+
+ mutex_lock(&nvmem_lookup_mutex);
+
+ list_for_each_entry(lookup, &nvmem_lookup_list, node) {
+ if ((strcmp(lookup->dev_id, dev_id) == 0) &&
+ (strcmp(lookup->con_id, con_id) == 0)) {
+ /* This is the right entry. */
+ nvmem = __nvmem_device_get(NULL, lookup->nvmem_name);
+ if (!nvmem) {
+ /* Provider may not be registered yet. */
+ cell = ERR_PTR(-EPROBE_DEFER);
+ goto out;
+ }
+
+ cell = nvmem_find_cell_by_name(nvmem,
+ lookup->cell_name);
+ if (!cell)
+ goto out;
+ }
+ }
+
+out:
+ mutex_unlock(&nvmem_lookup_mutex);
+ return cell;
+}
+
/**
* nvmem_cell_get() - Get nvmem cell of device form a given cell name
*
@@ -925,10 +975,14 @@ EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
*/
struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *cell_id)
{
- if (!dev->of_node)
+ if (dev->of_node)
+ return of_nvmem_cell_get(dev->of_node, cell_id);
+
+ /* Only allow empty cell_id for DT systems. */
+ if (!cell_id)
return ERR_PTR(-EINVAL);
- return of_nvmem_cell_get(dev->of_node, cell_id);
+ return nvmem_cell_get_from_lookup(dev, cell_id);
}
EXPORT_SYMBOL_GPL(nvmem_cell_get);
@@ -1300,6 +1354,41 @@ void nvmem_del_cell_table(struct nvmem_cell_table *table)
}
EXPORT_SYMBOL_GPL(nvmem_del_cell_table);
+/**
+ * nvmem_add_cell_lookups() - register a list of cell lookup entries
+ *
+ * @entries: array of cell lookup entries
+ * @nentries: number of cell lookup entries in the array
+ */
+void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries)
+{
+ int i;
+
+ mutex_lock(&nvmem_lookup_mutex);
+ for (i = 0; i < nentries; i++)
+ list_add_tail(&entries[i].node, &nvmem_lookup_list);
+ mutex_unlock(&nvmem_lookup_mutex);
+}
+EXPORT_SYMBOL_GPL(nvmem_add_cell_lookups);
+
+/**
+ * nvmem_del_cell_lookups() - remove a list of previously added cell lookup
+ * entries
+ *
+ * @entries: array of cell lookup entries
+ * @nentries: number of cell lookup entries in the array
+ */
+void nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries)
+{
+ int i;
+
+ mutex_lock(&nvmem_lookup_mutex);
+ for (i = 0; i < nentries; i++)
+ list_del(&entries[i].node);
+ mutex_unlock(&nvmem_lookup_mutex);
+}
+EXPORT_SYMBOL_GPL(nvmem_del_cell_lookups);
+
/**
* nvmem_dev_name() - Get the name of a given nvmem device.
*
diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
index 1e199dfaacab..7859c08934d5 100644
--- a/include/linux/nvmem-machine.h
+++ b/include/linux/nvmem-machine.h
@@ -26,16 +26,32 @@ struct nvmem_cell_table {
struct list_head node;
};
+struct nvmem_cell_lookup {
+ const char *nvmem_name;
+ const char *cell_name;
+ const char *dev_id;
+ const char *con_id;
+ struct list_head node;
+};
+
#if IS_ENABLED(CONFIG_NVMEM)
void nvmem_add_cell_table(struct nvmem_cell_table *table);
void nvmem_del_cell_table(struct nvmem_cell_table *table);
+void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries);
+void nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries);
+
#else /* CONFIG_NVMEM */
static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
+static inline void
+nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries) {}
+static inline void
+nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries) {}
+
#endif /* CONFIG_NVMEM */
#endif /* ifndef _LINUX_NVMEM_MACHINE_H */
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
This field is never set and is only used in a single error message.
Remove the field and use nvmem_dev_name() instead.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 364ee16e7d6a..e456aaa6184a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -26,7 +26,6 @@
#include <linux/slab.h>
struct nvmem_device {
- const char *name;
struct module *owner;
struct device dev;
int stride;
@@ -537,7 +536,7 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np)
if (!try_module_get(nvmem->owner)) {
dev_err(&nvmem->dev,
"could not increase module refcount for cell %s\n",
- nvmem->name);
+ nvmem_dev_name(nvmem));
return ERR_PTR(-EINVAL);
}
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Kernel users don't have any means of checking the names of nvmem
devices. Add a routine that returns the name of the nvmem provider.
This will be useful for nvmem notifier subscribers - otherwise they
can't check what device is being added/removed.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 13 +++++++++++++
include/linux/nvmem-consumer.h | 8 ++++++++
2 files changed, 21 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 89b91f73d08a..364ee16e7d6a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1144,6 +1144,19 @@ int nvmem_device_write(struct nvmem_device *nvmem,
}
EXPORT_SYMBOL_GPL(nvmem_device_write);
+/**
+ * nvmem_dev_name() - Get the name of a given nvmem device.
+ *
+ * @nvmem: nvmem device.
+ *
+ * Return: name of the nvmem device.
+ */
+const char *nvmem_dev_name(struct nvmem_device *nvmem)
+{
+ return dev_name(&nvmem->dev);
+}
+EXPORT_SYMBOL_GPL(nvmem_dev_name);
+
static int __init nvmem_init(void)
{
return bus_register(&nvmem_bus_type);
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 7e9fb5a19d91..1313da6731ff 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -43,6 +43,8 @@ int nvmem_device_read(struct nvmem_device *nvmem, unsigned int offset,
int nvmem_device_write(struct nvmem_device *nvmem, unsigned int offset,
size_t bytes, void *buf);
+const char *nvmem_dev_name(struct nvmem_device *nvmem);
+
#else
static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
@@ -117,6 +119,12 @@ static inline int nvmem_device_write(struct nvmem_device *nvmem,
{
return -ENOSYS;
}
+
+static inline const char *nvmem_dev_name(struct nvmem_device *nvmem)
+{
+ return NULL;
+}
+
#endif /* CONFIG_NVMEM */
#if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
This function can no longer fail and there are no more users that check
it return value. Change it to return void.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 8 ++------
include/linux/nvmem-provider.h | 9 +++------
2 files changed, 5 insertions(+), 12 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 2335b72d4b3e..89b91f73d08a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -450,20 +450,16 @@ static void nvmem_device_release(struct kref *kref)
* nvmem_unregister() - Unregister previously registered nvmem device
*
* @nvmem: Pointer to previously registered nvmem device.
- *
- * Return: Will be an negative on error or a zero on success.
*/
-int nvmem_unregister(struct nvmem_device *nvmem)
+void nvmem_unregister(struct nvmem_device *nvmem)
{
kref_put(&nvmem->refcnt, nvmem_device_release);
-
- return 0;
}
EXPORT_SYMBOL_GPL(nvmem_unregister);
static void devm_nvmem_release(struct device *dev, void *res)
{
- WARN_ON(nvmem_unregister(*(struct nvmem_device **)res));
+ nvmem_unregister(*(struct nvmem_device **)res);
}
/**
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index cc8556e3c825..d14a577a002d 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -67,7 +67,7 @@ struct nvmem_config {
#if IS_ENABLED(CONFIG_NVMEM)
struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
-int nvmem_unregister(struct nvmem_device *nvmem);
+void nvmem_unregister(struct nvmem_device *nvmem);
struct nvmem_device *devm_nvmem_register(struct device *dev,
const struct nvmem_config *cfg);
@@ -81,10 +81,7 @@ static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
return ERR_PTR(-ENOSYS);
}
-static inline int nvmem_unregister(struct nvmem_device *nvmem)
-{
- return -ENOSYS;
-}
+static inline void nvmem_unregister(struct nvmem_device *nvmem) {}
static inline struct nvmem_device *
devm_nvmem_register(struct device *dev, const struct nvmem_config *c)
@@ -95,7 +92,7 @@ devm_nvmem_register(struct device *dev, const struct nvmem_config *c)
static inline int
devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
{
- return nvmem_unregister(nvmem);
+ return ERR_PTR(-ENOSYS);
}
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Add a blocking notifier chain with two events (add and remove) so that
users can get notified about the addition of nvmem devices they're
waiting for.
We'll use this instead of the at24 setup callback in the mityomapl138
board file.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 36 ++++++++++++++++++++++++++++++++++
include/linux/nvmem-consumer.h | 19 ++++++++++++++++++
2 files changed, 55 insertions(+)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index e456aaa6184a..17307015905a 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -58,6 +58,8 @@ struct nvmem_cell {
static DEFINE_MUTEX(nvmem_mutex);
static DEFINE_IDA(nvmem_ida);
+static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
+
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key eeprom_lock_key;
#endif
@@ -339,6 +341,32 @@ static int nvmem_setup_compat(struct nvmem_device *nvmem,
return 0;
}
+/**
+ * nvmem_register_notifier() - Register a notifier block for nvmem events.
+ *
+ * @nb: notifier block to be called on nvmem events.
+ *
+ * Return: 0 on success, negative error number on failure.
+ */
+int nvmem_register_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_register(&nvmem_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(nvmem_register_notifier);
+
+/**
+ * nvmem_unregister_notifier() - Unregister a notifier block for nvmem events.
+ *
+ * @nb: notifier block to be unregistered.
+ *
+ * Return: 0 on success, negative error number on failure.
+ */
+int nvmem_unregister_notifier(struct notifier_block *nb)
+{
+ return blocking_notifier_chain_unregister(&nvmem_notifier, nb);
+}
+EXPORT_SYMBOL_GPL(nvmem_unregister_notifier);
+
/**
* nvmem_register() - Register a nvmem device for given nvmem_config.
* Also creates an binary entry in /sys/bus/nvmem/devices/dev-name/nvmem
@@ -420,8 +448,15 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
INIT_LIST_HEAD(&nvmem->cells);
+ rval = blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
+ if (rval)
+ goto err_teardown_compat;
+
return nvmem;
+err_teardown_compat:
+ if (config->compat)
+ device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
err_device_del:
device_del(&nvmem->dev);
err_put_device:
@@ -436,6 +471,7 @@ static void nvmem_device_release(struct kref *kref)
struct nvmem_device *nvmem;
nvmem = container_of(kref, struct nvmem_device, refcnt);
+ blocking_notifier_call_chain(&nvmem_notifier, NVMEM_REMOVE, nvmem);
if (nvmem->flags & FLAG_COMPAT)
device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 1313da6731ff..be583c4eb2d0 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -14,6 +14,7 @@
#include <linux/err.h>
#include <linux/errno.h>
+#include <linux/notifier.h>
struct device;
struct device_node;
@@ -21,6 +22,11 @@ struct device_node;
struct nvmem_cell;
struct nvmem_device;
+enum {
+ NVMEM_ADD = 1,
+ NVMEM_REMOVE,
+};
+
#if IS_ENABLED(CONFIG_NVMEM)
/* Cell based interface */
@@ -45,6 +51,9 @@ int nvmem_device_write(struct nvmem_device *nvmem, unsigned int offset,
const char *nvmem_dev_name(struct nvmem_device *nvmem);
+int nvmem_register_notifier(struct notifier_block *nb);
+int nvmem_unregister_notifier(struct notifier_block *nb);
+
#else
static inline struct nvmem_cell *nvmem_cell_get(struct device *dev,
@@ -125,6 +134,16 @@ static inline const char *nvmem_dev_name(struct nvmem_device *nvmem)
return NULL;
}
+static inline int nvmem_register_notifier(struct notifier_block *nb)
+{
+ return -ENOSYS;
+}
+
+static inline int nvmem_unregister_notifier(struct notifier_block *nb)
+{
+ return -ENOSYS;
+}
+
#endif /* CONFIG_NVMEM */
#if IS_ENABLED(CONFIG_NVMEM) && IS_ENABLED(CONFIG_OF)
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Use the resource managed variant of nvmem_register();
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/mxs-ocotp.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/nvmem/mxs-ocotp.c b/drivers/nvmem/mxs-ocotp.c
index 7018e2ef5714..53122f59c4b2 100644
--- a/drivers/nvmem/mxs-ocotp.c
+++ b/drivers/nvmem/mxs-ocotp.c
@@ -177,7 +177,7 @@ static int mxs_ocotp_probe(struct platform_device *pdev)
ocotp_config.size = data->size;
ocotp_config.priv = otp;
ocotp_config.dev = dev;
- otp->nvmem = nvmem_register(&ocotp_config);
+ otp->nvmem = devm_nvmem_register(dev, &ocotp_config);
if (IS_ERR(otp->nvmem)) {
ret = PTR_ERR(otp->nvmem);
goto err_clk;
@@ -199,7 +199,7 @@ static int mxs_ocotp_remove(struct platform_device *pdev)
clk_unprepare(otp->clk);
- return nvmem_unregister(otp->nvmem);
+ return 0;
}
static struct platform_driver mxs_ocotp_driver = {
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Use kref for reference counting. Use an approach similar to the one
seen in the common clock subsystem: don't actually destroy the nvmem
device until the last user puts it. This way we can get rid of the
users check from nvmem_unregister().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 45 +++++++++++++++++++++-----------------------
1 file changed, 21 insertions(+), 24 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 86af62e2df47..2335b72d4b3e 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -32,7 +32,7 @@ struct nvmem_device {
int stride;
int word_size;
int id;
- int users;
+ struct kref refcnt;
size_t size;
bool read_only;
int flags;
@@ -368,6 +368,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
return ERR_PTR(rval);
}
+ kref_init(&nvmem->refcnt);
+
nvmem->id = rval;
nvmem->owner = config->owner;
if (!nvmem->owner && config->dev->driver)
@@ -430,6 +432,20 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
}
EXPORT_SYMBOL_GPL(nvmem_register);
+static void nvmem_device_release(struct kref *kref)
+{
+ struct nvmem_device *nvmem;
+
+ nvmem = container_of(kref, struct nvmem_device, refcnt);
+
+ if (nvmem->flags & FLAG_COMPAT)
+ device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
+
+ nvmem_device_remove_all_cells(nvmem);
+ device_del(&nvmem->dev);
+ put_device(&nvmem->dev);
+}
+
/**
* nvmem_unregister() - Unregister previously registered nvmem device
*
@@ -439,19 +455,7 @@ EXPORT_SYMBOL_GPL(nvmem_register);
*/
int nvmem_unregister(struct nvmem_device *nvmem)
{
- mutex_lock(&nvmem_mutex);
- if (nvmem->users) {
- mutex_unlock(&nvmem_mutex);
- return -EBUSY;
- }
- mutex_unlock(&nvmem_mutex);
-
- if (nvmem->flags & FLAG_COMPAT)
- device_remove_bin_file(nvmem->base_dev, &nvmem->eeprom);
-
- nvmem_device_remove_all_cells(nvmem);
- device_del(&nvmem->dev);
- put_device(&nvmem->dev);
+ kref_put(&nvmem->refcnt, nvmem_device_release);
return 0;
}
@@ -517,7 +521,6 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
}
EXPORT_SYMBOL(devm_nvmem_unregister);
-
static struct nvmem_device *__nvmem_device_get(struct device_node *np)
{
struct nvmem_device *nvmem = NULL;
@@ -533,30 +536,24 @@ static struct nvmem_device *__nvmem_device_get(struct device_node *np)
return ERR_PTR(-EPROBE_DEFER);
}
- nvmem->users++;
mutex_unlock(&nvmem_mutex);
if (!try_module_get(nvmem->owner)) {
dev_err(&nvmem->dev,
"could not increase module refcount for cell %s\n",
nvmem->name);
-
- mutex_lock(&nvmem_mutex);
- nvmem->users--;
- mutex_unlock(&nvmem_mutex);
-
return ERR_PTR(-EINVAL);
}
+ kref_get(&nvmem->refcnt);
+
return nvmem;
}
static void __nvmem_device_put(struct nvmem_device *nvmem)
{
module_put(nvmem->owner);
- mutex_lock(&nvmem_mutex);
- nvmem->users--;
- mutex_unlock(&nvmem_mutex);
+ kref_put(&nvmem->refcnt, nvmem_device_release);
}
static struct nvmem_device *nvmem_find(const char *name)
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Make each nvmem device the owner of its nvmem cells.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 28 ++++++++++++----------------
1 file changed, 12 insertions(+), 16 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index bb475c2688f9..86af62e2df47 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -38,6 +38,7 @@ struct nvmem_device {
int flags;
struct bin_attribute eeprom;
struct device *base_dev;
+ struct list_head cells;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
void *priv;
@@ -58,9 +59,6 @@ struct nvmem_cell {
static DEFINE_MUTEX(nvmem_mutex);
static DEFINE_IDA(nvmem_ida);
-static LIST_HEAD(nvmem_cells);
-static DEFINE_MUTEX(nvmem_cells_mutex);
-
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key eeprom_lock_key;
#endif
@@ -284,29 +282,25 @@ static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
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);
}
-static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
+static void nvmem_device_remove_all_cells(struct nvmem_device *nvmem)
{
- struct nvmem_cell *cell;
- struct list_head *p, *n;
+ struct nvmem_cell *cell, *p;
- list_for_each_safe(p, n, &nvmem_cells) {
- cell = list_entry(p, struct nvmem_cell, node);
- if (cell->nvmem == nvmem)
- nvmem_cell_drop(cell);
- }
+ list_for_each_entry_safe(cell, p, &nvmem->cells, node)
+ nvmem_cell_drop(cell);
}
static void nvmem_cell_add(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, &cell->nvmem->cells);
+ mutex_unlock(&nvmem_mutex);
}
/*
@@ -423,6 +417,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
goto err_device_del;
}
+ INIT_LIST_HEAD(&nvmem->cells);
+
return nvmem;
err_device_del:
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Remove all APIs dealing with nvmem_cell_info. There are no users and
this part of the subsystem will be reworked.
This patch temprarily disables support for non-DT users.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 212 ++-------------------------------
include/linux/nvmem-consumer.h | 26 ----
include/linux/nvmem-provider.h | 13 --
3 files changed, 12 insertions(+), 239 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index aa1657831b70..bb475c2688f9 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -282,23 +282,6 @@ static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
return to_nvmem_device(d);
}
-static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
-{
- struct nvmem_cell *p;
-
- 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_unlock(&nvmem_cells_mutex);
-
- return NULL;
-}
-
static void nvmem_cell_drop(struct nvmem_cell *cell)
{
mutex_lock(&nvmem_cells_mutex);
@@ -326,82 +309,6 @@ static void nvmem_cell_add(struct nvmem_cell *cell)
mutex_unlock(&nvmem_cells_mutex);
}
-static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
- const struct nvmem_cell_info *info,
- struct nvmem_cell *cell)
-{
- cell->nvmem = nvmem;
- cell->offset = info->offset;
- cell->bytes = info->bytes;
- cell->name = info->name;
-
- cell->bit_offset = info->bit_offset;
- cell->nbits = info->nbits;
-
- if (cell->nbits)
- cell->bytes = DIV_ROUND_UP(cell->nbits + cell->bit_offset,
- BITS_PER_BYTE);
-
- if (!IS_ALIGNED(cell->offset, nvmem->stride)) {
- dev_err(&nvmem->dev,
- "cell %s unaligned to nvmem stride %d\n",
- cell->name, nvmem->stride);
- return -EINVAL;
- }
-
- return 0;
-}
-
-/**
- * nvmem_add_cells() - Add cell information to an nvmem device
- *
- * @nvmem: nvmem device to add cells to.
- * @info: nvmem cell info to add to the device
- * @ncells: number of cells in info
- *
- * Return: 0 or negative error code on failure.
- */
-int nvmem_add_cells(struct nvmem_device *nvmem,
- const struct nvmem_cell_info *info,
- int ncells)
-{
- struct nvmem_cell **cells;
- int i, rval;
-
- cells = kcalloc(ncells, sizeof(*cells), GFP_KERNEL);
- if (!cells)
- return -ENOMEM;
-
- for (i = 0; i < ncells; i++) {
- cells[i] = kzalloc(sizeof(**cells), GFP_KERNEL);
- if (!cells[i]) {
- rval = -ENOMEM;
- goto err;
- }
-
- rval = nvmem_cell_info_to_nvmem_cell(nvmem, &info[i], cells[i]);
- if (rval) {
- kfree(cells[i]);
- goto err;
- }
-
- nvmem_cell_add(cells[i]);
- }
-
- /* remove tmp array */
- kfree(cells);
-
- return 0;
-err:
- while (i--)
- nvmem_cell_drop(cells[i]);
-
- kfree(cells);
-
- return rval;
-}
-EXPORT_SYMBOL_GPL(nvmem_add_cells);
-
/*
* nvmem_setup_compat() - Create an additional binary entry in
* drivers sys directory, to be backwards compatible with the older
@@ -516,9 +423,6 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
goto err_device_del;
}
- if (config->cells)
- nvmem_add_cells(nvmem, config->cells, config->ncells);
-
return nvmem;
err_device_del:
@@ -618,32 +522,19 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
EXPORT_SYMBOL(devm_nvmem_unregister);
-static struct nvmem_device *__nvmem_device_get(struct device_node *np,
- struct nvmem_cell **cellp,
- const char *cell_id)
+static struct nvmem_device *__nvmem_device_get(struct device_node *np)
{
struct nvmem_device *nvmem = NULL;
- mutex_lock(&nvmem_mutex);
-
- if (np) {
- nvmem = of_nvmem_find(np);
- if (!nvmem) {
- mutex_unlock(&nvmem_mutex);
- return ERR_PTR(-EPROBE_DEFER);
- }
- } else {
- struct nvmem_cell *cell = nvmem_find_cell(cell_id);
+ if (!np)
+ return ERR_PTR(-EINVAL);
- if (cell) {
- nvmem = cell->nvmem;
- *cellp = cell;
- }
+ mutex_lock(&nvmem_mutex);
- if (!nvmem) {
- mutex_unlock(&nvmem_mutex);
- return ERR_PTR(-ENOENT);
- }
+ nvmem = of_nvmem_find(np);
+ if (!nvmem) {
+ mutex_unlock(&nvmem_mutex);
+ return ERR_PTR(-EPROBE_DEFER);
}
nvmem->users++;
@@ -706,7 +597,7 @@ struct nvmem_device *of_nvmem_device_get(struct device_node *np, const char *id)
if (!nvmem_np)
return ERR_PTR(-EINVAL);
- return __nvmem_device_get(nvmem_np, NULL, NULL);
+ return __nvmem_device_get(nvmem_np);
}
EXPORT_SYMBOL_GPL(of_nvmem_device_get);
#endif
@@ -810,18 +701,6 @@ struct nvmem_device *devm_nvmem_device_get(struct device *dev, const char *id)
}
EXPORT_SYMBOL_GPL(devm_nvmem_device_get);
-static struct nvmem_cell *nvmem_cell_get_from_list(const char *cell_id)
-{
- struct nvmem_cell *cell = NULL;
- struct nvmem_device *nvmem;
-
- nvmem = __nvmem_device_get(NULL, &cell, cell_id);
- if (IS_ERR(nvmem))
- return ERR_CAST(nvmem);
-
- return cell;
-}
-
#if IS_ENABLED(CONFIG_OF)
/**
* of_nvmem_cell_get() - Get a nvmem cell from given device node and cell id
@@ -857,7 +736,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node *np,
if (!nvmem_np)
return ERR_PTR(-EINVAL);
- nvmem = __nvmem_device_get(nvmem_np, NULL, NULL);
+ nvmem = __nvmem_device_get(nvmem_np);
of_node_put(nvmem_np);
if (IS_ERR(nvmem))
return ERR_CAST(nvmem);
@@ -926,19 +805,10 @@ EXPORT_SYMBOL_GPL(of_nvmem_cell_get);
*/
struct nvmem_cell *nvmem_cell_get(struct device *dev, const char *cell_id)
{
- struct nvmem_cell *cell;
-
- if (dev->of_node) { /* try dt first */
- cell = of_nvmem_cell_get(dev->of_node, cell_id);
- if (!IS_ERR(cell) || PTR_ERR(cell) == -EPROBE_DEFER)
- return cell;
- }
-
- /* NULL cell_id only allowed for device tree; invalid otherwise */
- if (!cell_id)
+ if (!dev->of_node)
return ERR_PTR(-EINVAL);
- return nvmem_cell_get_from_list(cell_id);
+ return of_nvmem_cell_get(dev->of_node, cell_id);
}
EXPORT_SYMBOL_GPL(nvmem_cell_get);
@@ -1227,64 +1097,6 @@ int nvmem_cell_read_u32(struct device *dev, const char *cell_id, u32 *val)
}
EXPORT_SYMBOL_GPL(nvmem_cell_read_u32);
-/**
- * nvmem_device_cell_read() - Read a given nvmem device and cell
- *
- * @nvmem: nvmem device to read from.
- * @info: nvmem cell info to be read.
- * @buf: buffer pointer which will be populated on successful read.
- *
- * Return: length of successful bytes read on success and negative
- * error code on error.
- */
-ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
- struct nvmem_cell_info *info, void *buf)
-{
- struct nvmem_cell cell;
- int rc;
- ssize_t len;
-
- if (!nvmem)
- return -EINVAL;
-
- rc = nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
- if (rc)
- return rc;
-
- rc = __nvmem_cell_read(nvmem, &cell, buf, &len);
- if (rc)
- return rc;
-
- return len;
-}
-EXPORT_SYMBOL_GPL(nvmem_device_cell_read);
-
-/**
- * nvmem_device_cell_write() - Write cell to a given nvmem device
- *
- * @nvmem: nvmem device to be written to.
- * @info: nvmem cell info to be written.
- * @buf: buffer to be written to cell.
- *
- * Return: length of bytes written or negative error code on failure.
- * */
-int nvmem_device_cell_write(struct nvmem_device *nvmem,
- struct nvmem_cell_info *info, void *buf)
-{
- struct nvmem_cell cell;
- int rc;
-
- if (!nvmem)
- return -EINVAL;
-
- rc = nvmem_cell_info_to_nvmem_cell(nvmem, info, &cell);
- if (rc)
- return rc;
-
- return nvmem_cell_write(&cell, buf, cell.bytes);
-}
-EXPORT_SYMBOL_GPL(nvmem_device_cell_write);
-
/**
* nvmem_device_read() - Read from a given nvmem device
*
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 4e85447f7860..7e9fb5a19d91 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -21,14 +21,6 @@ struct device_node;
struct nvmem_cell;
struct nvmem_device;
-struct nvmem_cell_info {
- const char *name;
- unsigned int offset;
- unsigned int bytes;
- unsigned int bit_offset;
- unsigned int nbits;
-};
-
#if IS_ENABLED(CONFIG_NVMEM)
/* Cell based interface */
@@ -50,10 +42,6 @@ int nvmem_device_read(struct nvmem_device *nvmem, unsigned int offset,
size_t bytes, void *buf);
int nvmem_device_write(struct nvmem_device *nvmem, unsigned int offset,
size_t bytes, void *buf);
-ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
- struct nvmem_cell_info *info, void *buf);
-int nvmem_device_cell_write(struct nvmem_device *nvmem,
- struct nvmem_cell_info *info, void *buf);
#else
@@ -116,20 +104,6 @@ static inline void devm_nvmem_device_put(struct device *dev,
{
}
-static inline ssize_t nvmem_device_cell_read(struct nvmem_device *nvmem,
- struct nvmem_cell_info *info,
- void *buf)
-{
- return -ENOSYS;
-}
-
-static inline int nvmem_device_cell_write(struct nvmem_device *nvmem,
- struct nvmem_cell_info *info,
- void *buf)
-{
- return -ENOSYS;
-}
-
static inline int nvmem_device_read(struct nvmem_device *nvmem,
unsigned int offset, size_t bytes,
void *buf)
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 24def6ad09bb..cc8556e3c825 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -16,7 +16,6 @@
#include <linux/errno.h>
struct nvmem_device;
-struct nvmem_cell_info;
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,
@@ -52,8 +51,6 @@ struct nvmem_config {
const char *name;
int id;
struct module *owner;
- const struct nvmem_cell_info *cells;
- int ncells;
bool read_only;
bool root_only;
nvmem_reg_read_t reg_read;
@@ -77,9 +74,6 @@ struct nvmem_device *devm_nvmem_register(struct device *dev,
int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
-int nvmem_add_cells(struct nvmem_device *nvmem,
- const struct nvmem_cell_info *info,
- int ncells);
#else
static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
@@ -105,12 +99,5 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem)
}
-static inline int nvmem_add_cells(struct nvmem_device *nvmem,
- const struct nvmem_cell_info *info,
- int ncells)
-{
- return -ENOSYS;
-}
-
#endif /* CONFIG_NVMEM */
#endif /* ifndef _LINUX_NVMEM_PROVIDER_H */
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Use the resource managed variant of nvmem_register().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/sunxi_sid.c | 20 +++-----------------
1 file changed, 3 insertions(+), 17 deletions(-)
diff --git a/drivers/nvmem/sunxi_sid.c b/drivers/nvmem/sunxi_sid.c
index d020f89248fd..0296c23db322 100644
--- a/drivers/nvmem/sunxi_sid.c
+++ b/drivers/nvmem/sunxi_sid.c
@@ -181,15 +181,13 @@ static int sunxi_sid_probe(struct platform_device *pdev)
else
econfig.reg_read = sunxi_sid_read;
econfig.priv = sid;
- nvmem = nvmem_register(&econfig);
+ nvmem = devm_nvmem_register(dev, &econfig);
if (IS_ERR(nvmem))
return PTR_ERR(nvmem);
randomness = kzalloc(size, GFP_KERNEL);
- if (!randomness) {
- ret = -EINVAL;
- goto err_unreg_nvmem;
- }
+ if (!randomness)
+ return PTR_ERR(-EINVAL);
for (i = 0; i < size; i++)
econfig.reg_read(sid, i, &randomness[i], 1);
@@ -200,17 +198,6 @@ static int sunxi_sid_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, nvmem);
return 0;
-
-err_unreg_nvmem:
- nvmem_unregister(nvmem);
- return ret;
-}
-
-static int sunxi_sid_remove(struct platform_device *pdev)
-{
- struct nvmem_device *nvmem = platform_get_drvdata(pdev);
-
- return nvmem_unregister(nvmem);
}
static const struct sunxi_sid_cfg sun4i_a10_cfg = {
@@ -243,7 +230,6 @@ MODULE_DEVICE_TABLE(of, sunxi_sid_of_match);
static struct platform_driver sunxi_sid_driver = {
.probe = sunxi_sid_probe,
- .remove = sunxi_sid_remove,
.driver = {
.name = "eeprom-sunxi-sid",
.of_match_table = sunxi_sid_of_match,
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Use SPDX license identiefiers to core nvmem files and remove GPL 2.0
license boilerplate.
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/core.c | 10 +---------
include/linux/nvmem-consumer.h | 5 +----
include/linux/nvmem-provider.h | 5 +----
3 files changed, 3 insertions(+), 17 deletions(-)
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 9e2f9c993a07..320b3cc975a0 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -1,17 +1,9 @@
+// SPDX-License-Identifier: GPL-2.0
/*
* nvmem framework core.
*
* Copyright (C) 2015 Srinivas Kandagatla <[email protected]>
* Copyright (C) 2013 Maxime Ripard <[email protected]>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2 and
- * only version 2 as published by the Free Software Foundation.
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
- * GNU General Public License for more details.
*/
#include <linux/device.h>
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index be583c4eb2d0..4c146379b9ba 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -1,12 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
/*
* nvmem framework consumer.
*
* Copyright (C) 2015 Srinivas Kandagatla <[email protected]>
* Copyright (C) 2013 Maxime Ripard <[email protected]>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
*/
#ifndef _LINUX_NVMEM_CONSUMER_H
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index d14a577a002d..171fb6caa578 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -1,12 +1,9 @@
+/* SPDX-License-Identifier: GPL-2.0 */
/*
* nvmem framework provider.
*
* Copyright (C) 2015 Srinivas Kandagatla <[email protected]>
* Copyright (C) 2013 Maxime Ripard <[email protected]>
- *
- * This file is licensed under the terms of the GNU General Public
- * License version 2. This program is licensed "as is" without any
- * warranty of any kind, whether express or implied.
*/
#ifndef _LINUX_NVMEM_PROVIDER_H
--
2.18.0
From: Bartosz Golaszewski <[email protected]>
Use the managed version of nvmem_register().
Signed-off-by: Bartosz Golaszewski <[email protected]>
---
drivers/nvmem/lpc18xx_eeprom.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/nvmem/lpc18xx_eeprom.c b/drivers/nvmem/lpc18xx_eeprom.c
index a9534a6e8636..b42dbaddb419 100644
--- a/drivers/nvmem/lpc18xx_eeprom.c
+++ b/drivers/nvmem/lpc18xx_eeprom.c
@@ -236,7 +236,7 @@ static int lpc18xx_eeprom_probe(struct platform_device *pdev)
lpc18xx_nvmem_config.dev = dev;
lpc18xx_nvmem_config.priv = eeprom;
- eeprom->nvmem = nvmem_register(&lpc18xx_nvmem_config);
+ eeprom->nvmem = devm_nvmem_register(dev, &lpc18xx_nvmem_config);
if (IS_ERR(eeprom->nvmem)) {
ret = PTR_ERR(eeprom->nvmem);
goto err_clk;
@@ -257,10 +257,6 @@ static int lpc18xx_eeprom_remove(struct platform_device *pdev)
struct lpc18xx_eeprom_dev *eeprom = platform_get_drvdata(pdev);
int ret;
- ret = nvmem_unregister(eeprom->nvmem);
- if (ret < 0)
- return ret;
-
clk_disable_unprepare(eeprom->clk);
return 0;
--
2.18.0
On 07/09/18 11:07, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Remove all APIs dealing with nvmem_cell_info. There are no users and
> this part of the subsystem will be reworked.
>
> This patch temprarily disables support for non-DT users.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/nvmem/core.c | 212 ++-------------------------------
> include/linux/nvmem-consumer.h | 26 ----
> include/linux/nvmem-provider.h | 13 --
> 3 files changed, 12 insertions(+), 239 deletions(-)
This looks totally un-necessary, Other patches in this series adds them
back.. Consider updating these in those respective patches.!
-srini
On 07/09/18 11:07, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add new structs and routines allowing users to define nvmem cells from
> machine code. This global list of entries is parsed when a provider
> is registered and cells are associated with the relevant nvmem_device
> struct.
>
> A possible improvement for the future is to allow users to register
> cell tables after the nvmem provider has been registered by updating
> the cell list at each call to nvmem_(add|del)_cell_table().
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> MAINTAINERS | 1 +
> drivers/nvmem/core.c | 97 ++++++++++++++++++++++++++++++++++-
I see some of this code is removed in first patch and added back here, I
dont really see a value in doing this in a single series of patchset.
I would recommend "[PATCH v2 01/16] nvmem: remove unused APIs" and this
patch to be merged.
> include/linux/nvmem-machine.h | 41 +++++++++++++++
> 3 files changed, 138 insertions(+), 1 deletion(-)
> create mode 100644 include/linux/nvmem-machine.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index 9ad052aeac39..a520924bf0a9 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -10391,6 +10391,7 @@ F: drivers/nvmem/
> F: Documentation/devicetree/bindings/nvmem/
> F: Documentation/ABI/stable/sysfs-bus-nvmem
> F: include/linux/nvmem-consumer.h
> +F: include/linux/nvmem-machine.h
> F: include/linux/nvmem-provider.h
>
> NXP SGTL5000 DRIVER
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 17307015905a..854baa0559a1 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
...
> *
> diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
> new file mode 100644
> index 000000000000..1e199dfaacab
> --- /dev/null
> +++ b/include/linux/nvmem-machine.h
This should go in nvmem-consumer.h, I don't think we should add header
files for each usecase. These are nvmem consumers so lets put them in
correct header file.
--srini
On 07/09/18 11:07, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> Add a way for machine code users to associate devices with nvmem cells.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/nvmem/core.c | 143 +++++++++++++++++++++++++++-------
> include/linux/nvmem-machine.h | 16 ++++
> 2 files changed, 132 insertions(+), 27 deletions(-)
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index da7a9d5beb33..9e2f9c993a07 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida);
> static DEFINE_MUTEX(nvmem_cell_mutex);
> static LIST_HEAD(nvmem_cell_tables);
>
> +static DEFINE_MUTEX(nvmem_lookup_mutex);
> +static LIST_HEAD(nvmem_lookup_list);
> +
> static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> @@ -285,6 +288,18 @@ static struct nvmem_device *of_nvmem_find(struct device_node *nvmem_np)
> return to_nvmem_device(d);
> }
>
> +static struct nvmem_device *nvmem_find(const char *name)
> +{
> + struct device *d;
> +
> + d = bus_find_device_by_name(&nvmem_bus_type, NULL, name);
> +
> + if (!d)
> + return NULL;
> +
> + return to_nvmem_device(d);
> +}
> +
This is removed and added back in same patch, you should consider
positioning the caller if possible to avoid any un-necessary changes.
> static void nvmem_cell_drop(struct nvmem_cell *cell)
> {
> mutex_lock(&nvmem_mutex);
> @@ -421,6 +436,21 @@ nvmem_find_cell_by_index(struct nvmem_device *nvmem, int index)
> return cell;
> }
>
> +static struct nvmem_cell *
> +nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
> +{
> + struct nvmem_cell *cell = ERR_PTR(-ENOENT);
> + struct nvmem_cell_lookup *lookup;
> + struct nvmem_device *nvmem;
> + const char *dev_id;
> +
> + if (!dev)
> + return ERR_PTR(-EINVAL);
> +
> + dev_id = dev_name(dev);
> +
> + mutex_lock(&nvmem_lookup_mutex);
> +
> + list_for_each_entry(lookup, &nvmem_lookup_list, node) {
> + if ((strcmp(lookup->dev_id, dev_id) == 0) &&
> + (strcmp(lookup->con_id, con_id) == 0)) {
> + /* This is the right entry. */
> + nvmem = __nvmem_device_get(NULL, lookup->nvmem_name);
> + if (!nvmem) {
> + /* Provider may not be registered yet. */
> + cell = ERR_PTR(-EPROBE_DEFER);
> + goto out;
> + }
> +
> + cell = nvmem_find_cell_by_name(nvmem,
> + lookup->cell_name);
> + if (!cell)
> + goto out;
Here nvmem refcount has already increased, you should probably fix this!
> + }
> + }
> +
> +out:
> + mutex_unlock(&nvmem_lookup_mutex);
> + return cell;
> +}
...
> diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
Should be part of nvmem-consumer.h.
> index 1e199dfaacab..7859c08934d5 100644
> --- a/include/linux/nvmem-machine.h
> +++ b/include/linux/nvmem-machine.h
> @@ -26,16 +26,32 @@ struct nvmem_cell_table {
> struct list_head node;
> };
>
> +struct nvmem_cell_lookup {
> + const char *nvmem_name;
> + const char *cell_name;
> + const char *dev_id;
> + const char *con_id;
> + struct list_head node;
> +};
Consider adding kerneldoc to this structure.
> +
> #if IS_ENABLED(CONFIG_NVMEM)
>
> void nvmem_add_cell_table(struct nvmem_cell_table *table);
> void nvmem_del_cell_table(struct nvmem_cell_table *table);
>
> +void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries);
> +void nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries);
> +
> #else /* CONFIG_NVMEM */
>
> static inline void nvmem_add_cell_table(struct nvmem_cell_table *table) {}
> static inline void nvmem_del_cell_table(struct nvmem_cell_table *table) {}
>
> +static inline void
> +nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries) {}
> +static inline void
> +nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t nentries) {}
> +
> #endif /* CONFIG_NVMEM */
>
> #endif /* ifndef _LINUX_NVMEM_MACHINE_H */
>
On 07/09/18 11:07, Bartosz Golaszewski wrote:
> From: Bartosz Golaszewski <[email protected]>
>
> This function can no longer fail and there are no more users that check
> it return value. Change it to return void.
This should be either part of Kref patch or it should state that
refcount handling is impoved in Kref patch so there is no need of
returning error code from unregister.
>
> Signed-off-by: Bartosz Golaszewski <[email protected]>
> ---
> drivers/nvmem/core.c | 8 ++------
> include/linux/nvmem-provider.h | 9 +++------
> 2 files changed, 5 insertions(+), 12 deletions(-)
Hi Srinivas,
On Mon, 10 Sep 2018 08:32:21 +0100
Srinivas Kandagatla <[email protected]> wrote:
> On 07/09/18 11:07, Bartosz Golaszewski wrote:
> > From: Bartosz Golaszewski <[email protected]>
> >
> > Add new structs and routines allowing users to define nvmem cells from
> > machine code. This global list of entries is parsed when a provider
> > is registered and cells are associated with the relevant nvmem_device
> > struct.
> >
> > A possible improvement for the future is to allow users to register
> > cell tables after the nvmem provider has been registered by updating
> > the cell list at each call to nvmem_(add|del)_cell_table().
> >
> > Signed-off-by: Bartosz Golaszewski <[email protected]>
> > ---
> > MAINTAINERS | 1 +
> > drivers/nvmem/core.c | 97 ++++++++++++++++++++++++++++++++++-
>
> I see some of this code is removed in first patch and added back here, I
> dont really see a value in doing this in a single series of patchset.
>
> I would recommend "[PATCH v2 01/16] nvmem: remove unused APIs" and this
> patch to be merged.
>
> > include/linux/nvmem-machine.h | 41 +++++++++++++++
> > 3 files changed, 138 insertions(+), 1 deletion(-)
> > create mode 100644 include/linux/nvmem-machine.h
> >
> > diff --git a/MAINTAINERS b/MAINTAINERS
> > index 9ad052aeac39..a520924bf0a9 100644
> > --- a/MAINTAINERS
> > +++ b/MAINTAINERS
> > @@ -10391,6 +10391,7 @@ F: drivers/nvmem/
> > F: Documentation/devicetree/bindings/nvmem/
> > F: Documentation/ABI/stable/sysfs-bus-nvmem
> > F: include/linux/nvmem-consumer.h
> > +F: include/linux/nvmem-machine.h
> > F: include/linux/nvmem-provider.h
> >
> > NXP SGTL5000 DRIVER
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 17307015905a..854baa0559a1 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> ...
> > *
> > diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
> > new file mode 100644
> > index 000000000000..1e199dfaacab
> > --- /dev/null
> > +++ b/include/linux/nvmem-machine.h
>
> This should go in nvmem-consumer.h, I don't think we should add header
> files for each usecase. These are nvmem consumers so lets put them in
> correct header file.
Actually no, it should go in nvmem-provider.h. Consumer should not be
allowed to define cells, only reference existing ones.
Regards,
Boris
On 07/09/18 11:07, Bartosz Golaszewski wrote:
> Next three patches improve the framework by adding a notifier chain
> for future use and fixing the issue with nvmem device names.
>
> Finally we add support for cell definitions, cell lookups and make
> DT systems resolve the nvmem cells during provider's registration.
>
> Last patches just use the SPDX license identifiers and make the
> naming convention for some arguments more consistent.
Please send separate series for fixes and things that do not depend on
this new apis or Order/place them at the end of the series.
This can help reviewer when there is large number of patches in series
with unrelated subject.
-s-rini
2018-09-10 9:32 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 07/09/18 11:07, Bartosz Golaszewski wrote:
>>
>> From: Bartosz Golaszewski <[email protected]>
>>
>> Remove all APIs dealing with nvmem_cell_info. There are no users and
>> this part of the subsystem will be reworked.
>>
>> This patch temprarily disables support for non-DT users.
>>
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>> ---
>> drivers/nvmem/core.c | 212 ++-------------------------------
>> include/linux/nvmem-consumer.h | 26 ----
>> include/linux/nvmem-provider.h | 13 --
>> 3 files changed, 12 insertions(+), 239 deletions(-)
>
>
> This looks totally un-necessary, Other patches in this series adds them
> back.. Consider updating these in those respective patches.!
>
While this may not be entirely necessary, it's very useful. There's
almost nothing common between this API and the new one added later in
this series. Later patches are MUCH cleaner thanks to this removal and
I believe it makes them easier for review.
Best regards,
Bart
On 10/09/18 08:58, Bartosz Golaszewski wrote:
> 2018-09-10 9:32 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>>
>>
>> On 07/09/18 11:07, Bartosz Golaszewski wrote:
>>>
>>> From: Bartosz Golaszewski <[email protected]>
>>>
>>> Remove all APIs dealing with nvmem_cell_info. There are no users and
>>> this part of the subsystem will be reworked.
>>>
>>> This patch temprarily disables support for non-DT users.
>>>
>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>> ---
>>> drivers/nvmem/core.c | 212 ++-------------------------------
>>> include/linux/nvmem-consumer.h | 26 ----
>>> include/linux/nvmem-provider.h | 13 --
>>> 3 files changed, 12 insertions(+), 239 deletions(-)
>>
>>
>> This looks totally un-necessary, Other patches in this series adds them
>> back.. Consider updating these in those respective patches.!
>>
>
> While this may not be entirely necessary, it's very useful. There's
Yes, thats exactly what am saying if it not necessary please do not do it.
> almost nothing common between this API and the new one added later in
There is more than 60%-70% of the code reused in new patches, which is
unnecessary churn to me!
> this series. Later patches are MUCH cleaner thanks to this removal and
> I believe it makes them easier for review.
Am not sure about that, it definitely did not help me!
--srini
>
> Best regards,
> Bart
>
2018-09-10 9:32 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 07/09/18 11:07, Bartosz Golaszewski wrote:
>>
>> From: Bartosz Golaszewski <[email protected]>
>>
>> Add a way for machine code users to associate devices with nvmem cells.
>>
>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>> ---
>> drivers/nvmem/core.c | 143 +++++++++++++++++++++++++++-------
>> include/linux/nvmem-machine.h | 16 ++++
>> 2 files changed, 132 insertions(+), 27 deletions(-)
>>
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index da7a9d5beb33..9e2f9c993a07 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida);
>> static DEFINE_MUTEX(nvmem_cell_mutex);
>> static LIST_HEAD(nvmem_cell_tables);
>> +static DEFINE_MUTEX(nvmem_lookup_mutex);
>> +static LIST_HEAD(nvmem_lookup_list);
>> +
>> static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>> #ifdef CONFIG_DEBUG_LOCK_ALLOC
>> @@ -285,6 +288,18 @@ static struct nvmem_device *of_nvmem_find(struct
>> device_node *nvmem_np)
>> return to_nvmem_device(d);
>> }
>> +static struct nvmem_device *nvmem_find(const char *name)
>> +{
>> + struct device *d;
>> +
>> + d = bus_find_device_by_name(&nvmem_bus_type, NULL, name);
>> +
>> + if (!d)
>> + return NULL;
>> +
>> + return to_nvmem_device(d);
>> +}
>> +
>
> This is removed and added back in same patch, you should consider
> positioning the caller if possible to avoid any un-necessary changes.
>
>> static void nvmem_cell_drop(struct nvmem_cell *cell)
>> {
>> mutex_lock(&nvmem_mutex);
>> @@ -421,6 +436,21 @@ nvmem_find_cell_by_index(struct nvmem_device *nvmem,
>> int index)
>> return cell;
>> }
>> +static struct nvmem_cell *
>> +nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
>> +{
>> + struct nvmem_cell *cell = ERR_PTR(-ENOENT);
>> + struct nvmem_cell_lookup *lookup;
>> + struct nvmem_device *nvmem;
>> + const char *dev_id;
>> +
>> + if (!dev)
>> + return ERR_PTR(-EINVAL);
>> +
>> + dev_id = dev_name(dev);
>> +
>> + mutex_lock(&nvmem_lookup_mutex);
>> +
>> + list_for_each_entry(lookup, &nvmem_lookup_list, node) {
>> + if ((strcmp(lookup->dev_id, dev_id) == 0) &&
>> + (strcmp(lookup->con_id, con_id) == 0)) {
>> + /* This is the right entry. */
>> + nvmem = __nvmem_device_get(NULL,
>> lookup->nvmem_name);
>> + if (!nvmem) {
>> + /* Provider may not be registered yet. */
>> + cell = ERR_PTR(-EPROBE_DEFER);
>> + goto out;
>> + }
>> +
>> + cell = nvmem_find_cell_by_name(nvmem,
>> + lookup->cell_name);
>> + if (!cell)
>> + goto out;
>
> Here nvmem refcount has already increased, you should probably fix this!
Indeed.
>>
>> + }
>> + }
>> +
>> +out:
>> + mutex_unlock(&nvmem_lookup_mutex);
>> + return cell;
>> +}
>
>
> ...
>
>> diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
>
>
> Should be part of nvmem-consumer.h.
>
If anything, this should probably go to nvmem-provider.h. But I like
the gpiolib way of putting machine-specific code into a separate
header. Most systems are not interested in these definitions anyway.
IMO this is a valid use case where creating a new header makes sense.
Bart
>> index 1e199dfaacab..7859c08934d5 100644
>> --- a/include/linux/nvmem-machine.h
>> +++ b/include/linux/nvmem-machine.h
>> @@ -26,16 +26,32 @@ struct nvmem_cell_table {
>> struct list_head node;
>> };
>> +struct nvmem_cell_lookup {
>> + const char *nvmem_name;
>> + const char *cell_name;
>> + const char *dev_id;
>> + const char *con_id;
>> + struct list_head node;
>> +};
>
>
> Consider adding kerneldoc to this structure.
>
>
>> +
>> #if IS_ENABLED(CONFIG_NVMEM)
>> void nvmem_add_cell_table(struct nvmem_cell_table *table);
>> void nvmem_del_cell_table(struct nvmem_cell_table *table);
>> +void nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t
>> nentries);
>> +void nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t
>> nentries);
>> +
>> #else /* CONFIG_NVMEM */
>> static inline void nvmem_add_cell_table(struct nvmem_cell_table
>> *table) {}
>> static inline void nvmem_del_cell_table(struct nvmem_cell_table *table)
>> {}
>> +static inline void
>> +nvmem_add_cell_lookups(struct nvmem_cell_lookup *entries, size_t
>> nentries) {}
>> +static inline void
>> +nvmem_del_cell_lookups(struct nvmem_cell_lookup *entries, size_t
>> nentries) {}
>> +
>> #endif /* CONFIG_NVMEM */
>> #endif /* ifndef _LINUX_NVMEM_MACHINE_H */
>>
>
On Mon, 10 Sep 2018 10:17:48 +0200
Bartosz Golaszewski <[email protected]> wrote:
> 2018-09-10 9:32 GMT+02:00 Srinivas Kandagatla <[email protected]>:
> >
> >
> > On 07/09/18 11:07, Bartosz Golaszewski wrote:
> >>
> >> From: Bartosz Golaszewski <[email protected]>
> >>
> >> Add a way for machine code users to associate devices with nvmem cells.
> >>
> >> Signed-off-by: Bartosz Golaszewski <[email protected]>
> >> ---
> >> drivers/nvmem/core.c | 143 +++++++++++++++++++++++++++-------
> >> include/linux/nvmem-machine.h | 16 ++++
> >> 2 files changed, 132 insertions(+), 27 deletions(-)
> >>
> >> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> >> index da7a9d5beb33..9e2f9c993a07 100644
> >> --- a/drivers/nvmem/core.c
> >> +++ b/drivers/nvmem/core.c
> >> @@ -62,6 +62,9 @@ static DEFINE_IDA(nvmem_ida);
> >> static DEFINE_MUTEX(nvmem_cell_mutex);
> >> static LIST_HEAD(nvmem_cell_tables);
> >> +static DEFINE_MUTEX(nvmem_lookup_mutex);
> >> +static LIST_HEAD(nvmem_lookup_list);
> >> +
> >> static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
> >> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> >> @@ -285,6 +288,18 @@ static struct nvmem_device *of_nvmem_find(struct
> >> device_node *nvmem_np)
> >> return to_nvmem_device(d);
> >> }
> >> +static struct nvmem_device *nvmem_find(const char *name)
> >> +{
> >> + struct device *d;
> >> +
> >> + d = bus_find_device_by_name(&nvmem_bus_type, NULL, name);
> >> +
> >> + if (!d)
> >> + return NULL;
> >> +
> >> + return to_nvmem_device(d);
> >> +}
> >> +
> >
> > This is removed and added back in same patch, you should consider
> > positioning the caller if possible to avoid any un-necessary changes.
> >
> >> static void nvmem_cell_drop(struct nvmem_cell *cell)
> >> {
> >> mutex_lock(&nvmem_mutex);
> >> @@ -421,6 +436,21 @@ nvmem_find_cell_by_index(struct nvmem_device *nvmem,
> >> int index)
> >> return cell;
> >> }
> >> +static struct nvmem_cell *
> >> +nvmem_cell_get_from_lookup(struct device *dev, const char *con_id)
> >> +{
> >> + struct nvmem_cell *cell = ERR_PTR(-ENOENT);
> >> + struct nvmem_cell_lookup *lookup;
> >> + struct nvmem_device *nvmem;
> >> + const char *dev_id;
> >> +
> >> + if (!dev)
> >> + return ERR_PTR(-EINVAL);
> >> +
> >> + dev_id = dev_name(dev);
> >> +
> >> + mutex_lock(&nvmem_lookup_mutex);
> >> +
> >> + list_for_each_entry(lookup, &nvmem_lookup_list, node) {
> >> + if ((strcmp(lookup->dev_id, dev_id) == 0) &&
> >> + (strcmp(lookup->con_id, con_id) == 0)) {
> >> + /* This is the right entry. */
> >> + nvmem = __nvmem_device_get(NULL,
> >> lookup->nvmem_name);
> >> + if (!nvmem) {
> >> + /* Provider may not be registered yet. */
> >> + cell = ERR_PTR(-EPROBE_DEFER);
> >> + goto out;
> >> + }
> >> +
> >> + cell = nvmem_find_cell_by_name(nvmem,
> >> + lookup->cell_name);
> >> + if (!cell)
> >> + goto out;
> >
> > Here nvmem refcount has already increased, you should probably fix this!
>
> Indeed.
>
> >>
> >> + }
> >> + }
> >> +
> >> +out:
> >> + mutex_unlock(&nvmem_lookup_mutex);
> >> + return cell;
> >> +}
> >
> >
> > ...
> >
> >> diff --git a/include/linux/nvmem-machine.h b/include/linux/nvmem-machine.h
> >
> >
> > Should be part of nvmem-consumer.h.
> >
>
> If anything, this should probably go to nvmem-provider.h.
Well, if we get rid of nvmem-machine.h, the cell-lookup stuff
should go in nvmem-consumer.h not nvmem-provider.h. On the other hand,
everything that is related to cell creation should be placed in
nvmem-provider.h.
> But I like
> the gpiolib way of putting machine-specific code into a separate
> header. Most systems are not interested in these definitions anyway.
> IMO this is a valid use case where creating a new header makes sense.
2018-09-10 9:54 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 07/09/18 11:07, Bartosz Golaszewski wrote:
>>
>> Next three patches improve the framework by adding a notifier chain
>> for future use and fixing the issue with nvmem device names.
>>
>> Finally we add support for cell definitions, cell lookups and make
>> DT systems resolve the nvmem cells during provider's registration.
>>
>> Last patches just use the SPDX license identifiers and make the
>> naming convention for some arguments more consistent.
>
> Please send separate series for fixes and things that do not depend on this
> new apis or Order/place them at the end of the series.
>
The API changes change so many things, that these series would be
incompatible. I can send the series separately but they would be
co-dependent anyway. Does it sound good?
Bart
> This can help reviewer when there is large number of patches in series with
> unrelated subject.
>
> -s-rini
>
>
2018-09-10 10:09 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 10/09/18 08:58, Bartosz Golaszewski wrote:
>>
>> 2018-09-10 9:32 GMT+02:00 Srinivas Kandagatla
>> <[email protected]>:
>>>
>>>
>>>
>>> On 07/09/18 11:07, Bartosz Golaszewski wrote:
>>>>
>>>>
>>>> From: Bartosz Golaszewski <[email protected]>
>>>>
>>>> Remove all APIs dealing with nvmem_cell_info. There are no users and
>>>> this part of the subsystem will be reworked.
>>>>
>>>> This patch temprarily disables support for non-DT users.
>>>>
>>>> Signed-off-by: Bartosz Golaszewski <[email protected]>
>>>> ---
>>>> drivers/nvmem/core.c | 212
>>>> ++-------------------------------
>>>> include/linux/nvmem-consumer.h | 26 ----
>>>> include/linux/nvmem-provider.h | 13 --
>>>> 3 files changed, 12 insertions(+), 239 deletions(-)
>>>
>>>
>>>
>>> This looks totally un-necessary, Other patches in this series adds them
>>> back.. Consider updating these in those respective patches.!
>>>
>>
>> While this may not be entirely necessary, it's very useful. There's
>
> Yes, thats exactly what am saying if it not necessary please do not do it.
>
>> almost nothing common between this API and the new one added later in
>
> There is more than 60%-70% of the code reused in new patches, which is
> unnecessary churn to me!
>
Pretty much the only thing that is reused are the contents of
nvmem_cell_info_to_nvmem_cell() and even they are placed in a
different function. Also: this change allows us to add new APIs
without having to update users at each step only to change them
afterwards with one of subsequent patches. I would really prefer that
this stay this way. If there are no users, then there's no breakage
that would make your point valid.
>> this series. Later patches are MUCH cleaner thanks to this removal and
>> I believe it makes them easier for review.
>
> Am not sure about that, it definitely did not help me!
>
Why exactly? Aren't clean patches resulting from this removal easier
to read then if they'd also have to take into account the burden of
existing code that will be changes anyway later? I really don't see
why you insist on removing this.
Best regards,
Bart
> --srini
>>
>>
>> Best regards,
>> Bart
>>
>
On 10/09/18 08:36, Boris Brezillon wrote:
>>> --- /dev/null
>>> +++ b/include/linux/nvmem-machine.h
>> This should go in nvmem-consumer.h, I don't think we should add header
>> files for each usecase. These are nvmem consumers so lets put them in
>> correct header file.
> Actually no, it should go in nvmem-provider.h. Consumer should not be
> allowed to define cells, only reference existing ones.The usecase can be provider and consumer.
In arm machine use case it is a consumer and it other use cases this can
be a provider.
I agree with you, It makes more sense to be in nvmem-provider.h
thanks,
srini
>
> Regards,
>
> Boris
On 10/09/18 09:23, Boris Brezillon wrote:
> Well, if we get rid of nvmem-machine.h, the cell-lookup stuff
> should go in nvmem-consumer.h not nvmem-provider.h. On the other hand,
> everything that is related to cell creation should be placed in
> nvmem-provider.h.
Yes, this is how it should be!
--srini
2018-09-10 10:55 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 10/09/18 09:23, Boris Brezillon wrote:
>>
>> Well, if we get rid of nvmem-machine.h, the cell-lookup stuff
>> should go in nvmem-consumer.h not nvmem-provider.h. On the other hand,
>> everything that is related to cell creation should be placed in
>> nvmem-provider.h.
>
> Yes, this is how it should be!
>
Any actual reason for not putting these definitions into a separate
'machine' header? This approach is currently used by gpio, pinctrl,
iio and regulator framework because most systems use either DT or ACPI
and don't need to pull in any stuff aimed at board files.
Bart
On Mon, 10 Sep 2018 11:45:48 +0200
Bartosz Golaszewski <[email protected]> wrote:
> 2018-09-10 10:55 GMT+02:00 Srinivas Kandagatla <[email protected]>:
> >
> >
> > On 10/09/18 09:23, Boris Brezillon wrote:
> >>
> >> Well, if we get rid of nvmem-machine.h, the cell-lookup stuff
> >> should go in nvmem-consumer.h not nvmem-provider.h. On the other hand,
> >> everything that is related to cell creation should be placed in
> >> nvmem-provider.h.
> >
> > Yes, this is how it should be!
> >
>
> Any actual reason for not putting these definitions into a separate
> 'machine' header? This approach is currently used by gpio, pinctrl,
> iio and regulator framework because most systems use either DT or ACPI
> and don't need to pull in any stuff aimed at board files.
I'm perfectly fine with the separate header file, all I'm saying is, if
Srinivas does not want nvmem-machine.h, definitions should be placed in
nvmem-provider.h or nvmem-consumer.h depending on who they're meant to
be used by (providers or consumers).
On 10/09/18 10:45, Bartosz Golaszewski wrote:
>> Yes, this is how it should be!
>>
> Any actual reason for not putting these definitions into a separate
> 'machine' header? This approach is currently used by gpio, pinctrl,
> iio and regulator framework because most systems use either DT or ACPI
> and don't need to pull in any stuff aimed at board files.
I don't want to create header files specific to usecase!
Lets keep it simple!
--srini
On 10/09/18 09:43, Bartosz Golaszewski wrote:
>>> this series. Later patches are MUCH cleaner thanks to this removal and
>>> I believe it makes them easier for review.
>> Am not sure about that, it definitely did not help me!
>>
> Why exactly? Aren't clean patches resulting from this removal easier
> to read then if they'd also have to take into account the burden of
> existing code that will be changes anyway later? I really don't see
> why you insist on removing this.
Here are few reasons:
1> TBH, This cleanup patch removes more than one feature from nvmem
subsystem.
Ex: it remove ability to add static cell from nvmem config, secondly it
removed ability to read/write cells directly using nvmem_cell_info.
2> If you do changes as part of each patch, it will be much traceable on
what is changed exactly and will allow much better review, and
understand the reasoning.
Having a clean start and writing code on top of it is alway nice, but we
will definitely miss things in review in this particular instance.
thanks,
srini
>
> Best regards,
> Bart
>
On 10/09/18 09:24, Bartosz Golaszewski wrote:
> The API changes change so many things, that these series would be
> incompatible. I can send the series separately but they would be
> co-dependent anyway. Does it sound good?
What am asking is to reorder the patches in a such a way that its easy
to review.
ex: Order can be :
1> kref and update nvmem_unregister followed by the provider changes.
2> Add support to cell tables, cell lookup, notifier
3> general cleanup patches followed by fixes
Current set seems to jump from one thing to other which makes it hard to
follow and time consuming while review!
--srini
>
> Bart
2018-09-10 11:50 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 10/09/18 10:45, Bartosz Golaszewski wrote:
>>>
>>> Yes, this is how it should be!
>>>
>> Any actual reason for not putting these definitions into a separate
>> 'machine' header? This approach is currently used by gpio, pinctrl,
>> iio and regulator framework because most systems use either DT or ACPI
>> and don't need to pull in any stuff aimed at board files.
>
>
> I don't want to create header files specific to usecase!
> Lets keep it simple!
>
I won't argue this point anymore, but I disagree. This is not specific
to a usecase but to a whole family of users that need to a) define
nvmem cells without knowing the provider and b) associate them with
consumers. Something normal providers and consumers are not bothered
by, thus a new header file would be in order.
But as you wish...
Bart
2018-09-10 11:55 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 10/09/18 09:43, Bartosz Golaszewski wrote:
>>>>
>>>> this series. Later patches are MUCH cleaner thanks to this removal and
>>>> I believe it makes them easier for review.
>>>
>>> Am not sure about that, it definitely did not help me!
>>>
>> Why exactly? Aren't clean patches resulting from this removal easier
>> to read then if they'd also have to take into account the burden of
>> existing code that will be changes anyway later? I really don't see
>> why you insist on removing this.
>
>
> Here are few reasons:
>
> 1> TBH, This cleanup patch removes more than one feature from nvmem
> subsystem.
> Ex: it remove ability to add static cell from nvmem config, secondly it
> removed ability to read/write cells directly using nvmem_cell_info.
>
About that: there are no users of nvmem_device_cell_read/write()
currently and with the new API I'm not sure this is still needed. Are
you alright with removing those two?
Bart
> 2> If you do changes as part of each patch, it will be much traceable on
> what is changed exactly and will allow much better review, and understand
> the reasoning.
>
> Having a clean start and writing code on top of it is alway nice, but we
> will definitely miss things in review in this particular instance.
>
>
> thanks,
> srini
>
>>
>> Best regards,
>> Bart
>>
>
On 10/09/18 12:31, Bartosz Golaszewski wrote:
> About that: there are no users of nvmem_device_cell_read/write()
> currently and with the new API I'm not sure this is still needed. Are
> you alright with removing those two?
Why do you want to remove them? Other than reason of no users.
All it boils down to if we support device based apis using cell info or
not?
IMO, I see no harm in leaving these apis as it is, unless there is a
strong reason to do so.
thanks,
srini
Hi Srinivas,
On Mon, 10 Sep 2018 12:47:19 +0100
Srinivas Kandagatla <[email protected]> wrote:
> On 10/09/18 12:31, Bartosz Golaszewski wrote:
> > About that: there are no users of nvmem_device_cell_read/write()
> > currently and with the new API I'm not sure this is still needed. Are
> > you alright with removing those two?
>
> Why do you want to remove them? Other than reason of no users.
I'm just sharing my (and probably other maintainers/developers) PoV
here, so please don't take this as an attempt to force you to change
your mind, but rather an attempt at explaining why APIs usually stay
private when they have no users.
Kernel maintainers tend to reject APIs that have no users because
adding something that nobody needs yet is hard to get right. I mean,
you can design an API on what you think will be needed/appropriate, and
then, when people actually start needing this feature, they realize it
does not quite match what they need, and they have to adjust/rework
this API.
>
> All it boils down to if we support device based apis using cell info or
> not?
But it looks like nobody is actually using it, and the first potential
user (Bart) proposed a different approach with the nvmem cell table
declaration.
> IMO, I see no harm in leaving these apis as it is, unless there is a
> strong reason to do so.
It's harmless, but it's also unused. If people start using it and you
realize the API is not working for some use cases, then you'll have to
patch all existing users.
All this being said, it's your call to make, and if you think it makes
sense to keep these functions around for any reason, then be it.
Regards,
Boris
2018-09-10 14:18 GMT+02:00 Boris Brezillon <[email protected]>:
> Hi Srinivas,
>
> On Mon, 10 Sep 2018 12:47:19 +0100
> Srinivas Kandagatla <[email protected]> wrote:
>
>> On 10/09/18 12:31, Bartosz Golaszewski wrote:
>> > About that: there are no users of nvmem_device_cell_read/write()
>> > currently and with the new API I'm not sure this is still needed. Are
>> > you alright with removing those two?
>>
>> Why do you want to remove them? Other than reason of no users.
>
> I'm just sharing my (and probably other maintainers/developers) PoV
> here, so please don't take this as an attempt to force you to change
> your mind, but rather an attempt at explaining why APIs usually stay
> private when they have no users.
>
> Kernel maintainers tend to reject APIs that have no users because
> adding something that nobody needs yet is hard to get right. I mean,
> you can design an API on what you think will be needed/appropriate, and
> then, when people actually start needing this feature, they realize it
> does not quite match what they need, and they have to adjust/rework
> this API.
>
>>
>> All it boils down to if we support device based apis using cell info or
>> not?
>
> But it looks like nobody is actually using it, and the first potential
> user (Bart) proposed a different approach with the nvmem cell table
> declaration.
>
>> IMO, I see no harm in leaving these apis as it is, unless there is a
>> strong reason to do so.
>
> It's harmless, but it's also unused. If people start using it and you
> realize the API is not working for some use cases, then you'll have to
> patch all existing users.
>
> All this being said, it's your call to make, and if you think it makes
> sense to keep these functions around for any reason, then be it.
>
Also: the general notion in most subsystems is that drivers should be
generalized enough to not have to care about the provider's internals,
which is the case if we allow users to specify cell info. They should
just query the subsystem for a given resource, use it, then put it.
Fortunately this is what all current users do in this case - we don't
even have any users of nvmem_device_get() which is good since it
should be reserved for very special cases.
While one can argue that nvmem_device_cell_read/write() may be needed
in some strange corner case now, once we add the cell lookup API, we
can safely remove it and force users to do the right thing.
Best regards,
Bartosz Golaszewski
On 10/09/18 13:22, Bartosz Golaszewski wrote:
> While one can argue that nvmem_device_cell_read/write() may be needed
> in some strange corner case now, once we add the cell lookup API, we
> can safely remove it and force users to do the right thing.
I agree!
I don't have very strong reason to stop removing
nvmem_device_cell_read/write().
Lets see how it turns out!
I would still want nvmem config to have ability to setup static cell list.
--srini
2018-09-10 12:02 GMT+02:00 Srinivas Kandagatla <[email protected]>:
>
>
> On 10/09/18 09:24, Bartosz Golaszewski wrote:
>>
>> The API changes change so many things, that these series would be
>> incompatible. I can send the series separately but they would be
>> co-dependent anyway. Does it sound good?
>
>
> What am asking is to reorder the patches in a such a way that its easy to
> review.
> ex: Order can be :
> 1> kref and update nvmem_unregister followed by the provider changes.
> 2> Add support to cell tables, cell lookup, notifier
> 3> general cleanup patches followed by fixes
>
> Current set seems to jump from one thing to other which makes it hard to
> follow and time consuming while review!
>
I would just change that ordering a bit: I think we should fix stuff
that's broken first, so fix the name field of struct nvmem_device,
then the rest as you listed it.
Bart