All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser
@ 2021-06-08 19:03 Vadym Kochan
  2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
                   ` (2 more replies)
  0 siblings, 3 replies; 36+ messages in thread
From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw)
  To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: Robert Marko, Vadym Kochan

This series adds cells parser for the ONIE TLV attributes which are
stored on NVMEM device. It adds possibility to read the mac address (and
other info) by other drivers.

Because ONIE stores info in TLV format it should be parsed first and
then register the cells. Current NVMEM API allows to register cell
table with known cell's offset which is not guaranteed in case of TLV.

To make it properly handled the NVMEM parser object is introduced. The
parser needs to be registered before target NVMEM device is registered.
During the registration of NVMEM device the parser is called to parse
the device's cells and reister the cell table.

v2:
    1) Series title changed, "provider" is replaced by "parser".

    2) onie-cells.c renamed to onie-tlv-cells.c

    3) Do not register onie cells parser via platform driver,
       but via module_init().

    4) Removed cells_remove callback from nvmem_parser_config because
       now cells table and lookups are registered by core.c

    5) Introduced nvmem_parser_data which is passed to parser
       handler/driver to fill cells table & lookups.

    6) core.c registers cells table & lookups to simplify the parser
       handler logic.

    7) parser's module refcount is incremented/decremented on each parser
       bind/unbind to nvmem device.

    8) core.c relies on parser name which is obtained via
       device_property API, instead of of_node which was used
       in previous version.

    9) nvmem-cell-parser-name DT nvmem property was added to bind
       cells parser to nvmem device during the nvmem registration.

P.S.
I marked it with v2 as relative to series which was sent as:

    "nvmem: add ONIE NVMEM cells provider"
    
Vadym Kochan (3):
  nvmem: core: introduce cells parser
  dt-bindings: nvmem: document nvmem-cells-parser-name property
  nvmem: add ONIE nvmem cells parser

 .../devicetree/bindings/nvmem/nvmem.yaml      |   5 +
 drivers/nvmem/Kconfig                         |   9 +
 drivers/nvmem/Makefile                        |   2 +
 drivers/nvmem/core.c                          | 178 +++++++++++
 drivers/nvmem/onie-tlv-cells.c                | 302 ++++++++++++++++++
 include/linux/nvmem-provider.h                |  31 ++
 6 files changed, 527 insertions(+)
 create mode 100644 drivers/nvmem/onie-tlv-cells.c

-- 
2.17.1


^ permalink raw reply	[flat|nested] 36+ messages in thread

* [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan
@ 2021-06-08 19:03 ` Vadym Kochan
  2021-06-08 22:49     ` kernel test robot
                     ` (2 more replies)
  2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan
  2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan
  2 siblings, 3 replies; 36+ messages in thread
From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw)
  To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: Robert Marko, Vadym Kochan

Current implementation does not allow to register cells for already
registered nvmem device and requires that this should be done before. But
there might a driver which needs to parse the nvmem device and after
register a cells table.

Introduce nvmem_parser API which allows to register cells parser which
is called during nvmem device registration. During this stage the parser
can read the nvmem device and register the cells table.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
v2:
    1) Added nvmem_parser_data so parser implementation
       should fill it with cells table and lookups which
       will be registered by core.c after cells_parse was
       succeed.

    2) Removed cells_remove callback from nvmem_parser_config which
       is not needed because cells table & lookups are
       registered/unregistered automatically by core.

    3) Use new device property to read cells parser name during nvmem
       registration. Removed of_node usage.

    4) parser's module refcount is incremented/decremented on each parser
       bind/unbind to nvmem device.

 drivers/nvmem/core.c           | 178 +++++++++++++++++++++++++++++++++
 include/linux/nvmem-provider.h |  31 ++++++
 2 files changed, 209 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index bca671ff4e54..648373ced6d4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -39,6 +39,7 @@ struct nvmem_device {
 	nvmem_reg_read_t	reg_read;
 	nvmem_reg_write_t	reg_write;
 	struct gpio_desc	*wp_gpio;
+	struct nvmem_parser_data *parser_data;
 	void *priv;
 };
 
@@ -57,6 +58,13 @@ struct nvmem_cell {
 	struct list_head	node;
 };
 
+struct nvmem_parser {
+	struct list_head	head;
+	nvmem_parse_t		cells_parse;
+	const char		*name;
+	struct module		*owner;
+};
+
 static DEFINE_MUTEX(nvmem_mutex);
 static DEFINE_IDA(nvmem_ida);
 
@@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
 static DEFINE_MUTEX(nvmem_lookup_mutex);
 static LIST_HEAD(nvmem_lookup_list);
 
+static DEFINE_MUTEX(nvmem_parser_mutex);
+static LIST_HEAD(nvmem_parser_list);
+
 static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
 
 static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
@@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
 	.name		= "nvmem",
 };
 
+static struct nvmem_parser *__nvmem_parser_get(const char *name)
+{
+	struct nvmem_parser *tmp, *parser = NULL;
+
+	list_for_each_entry(tmp, &nvmem_parser_list, head) {
+		if (strcmp(name, tmp->name) == 0) {
+			parser = tmp;
+			break;
+		}
+	}
+
+	if (!parser)
+		return ERR_PTR(-EPROBE_DEFER);
+
+	if (!try_module_get(parser->owner)) {
+		pr_err("could not increase module refcount for parser %s\n",
+		       parser->name);
+		return ERR_PTR(-EINVAL);
+
+	}
+
+	return parser;
+}
+
+static void nvmem_parser_put(const struct nvmem_parser *parser)
+{
+	module_put(parser->owner);
+}
+
+static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
+{
+	struct nvmem_parser_data *data;
+	struct nvmem_parser *parser;
+	int err;
+
+	mutex_lock(&nvmem_parser_mutex);
+
+	parser = __nvmem_parser_get(name);
+	err = PTR_ERR_OR_ZERO(parser);
+	if (!err) {
+		data = kzalloc(sizeof(*data), GFP_KERNEL);
+		if (data) {
+			data->parser = parser;
+			nvmem->parser_data = data;
+		} else {
+			nvmem_parser_put(parser);
+			err = -ENOMEM;
+		}
+	}
+
+	mutex_unlock(&nvmem_parser_mutex);
+
+	return err;
+}
+
+static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
+{
+	struct nvmem_parser_data *data = nvmem->parser_data;
+
+	if (data->table.cells) {
+		nvmem_del_cell_table(&data->table);
+		kfree(data->table.cells);
+	}
+	if (data->lookup) {
+		nvmem_del_cell_lookups(data->lookup, data->nlookups);
+		kfree(data->lookup);
+	}
+
+	nvmem_parser_put(data->parser);
+}
+
+static void nvmem_parser_data_register(struct nvmem_device *nvmem,
+				       struct nvmem_parser_data *data)
+{
+	if (data->table.cells) {
+		if (!data->table.nvmem_name)
+			data->table.nvmem_name = nvmem_dev_name(nvmem);
+
+		nvmem_add_cell_table(&data->table);
+	}
+
+	if (data->lookup) {
+		struct nvmem_cell_lookup *lookup = &data->lookup[0];
+		int i = 0;
+
+		for (i = 0; i < data->nlookups; i++, lookup++) {
+			if (!lookup->nvmem_name)
+				lookup->nvmem_name = nvmem_dev_name(nvmem);
+
+			if (!lookup->dev_id)
+				lookup->dev_id = data->parser->name;
+		}
+
+		nvmem_add_cell_lookups(data->lookup, data->nlookups);
+	}
+}
+
+static void nvmem_cells_parse(struct nvmem_device *nvmem)
+{
+	struct nvmem_parser_data *data = nvmem->parser_data;
+	struct nvmem_parser *parser = data->parser;
+	int err;
+
+	mutex_lock(&nvmem_parser_mutex);
+
+	err = parser->cells_parse(nvmem, data);
+	if (!err)
+		nvmem_parser_data_register(nvmem, data);
+
+	mutex_unlock(&nvmem_parser_mutex);
+}
+
 static void nvmem_cell_drop(struct nvmem_cell *cell)
 {
 	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_CELL_REMOVE, cell);
@@ -435,6 +558,9 @@ static void nvmem_device_remove_all_cells(const struct nvmem_device *nvmem)
 
 	list_for_each_entry_safe(cell, p, &nvmem->cells, node)
 		nvmem_cell_drop(cell);
+
+	if (nvmem->parser_data)
+		nvmem_parser_unbind(nvmem);
 }
 
 static void nvmem_cell_add(struct nvmem_cell *cell)
@@ -739,6 +865,7 @@ static int nvmem_add_cells_from_of(struct nvmem_device *nvmem)
 struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 {
 	struct nvmem_device *nvmem;
+	const char *parser_name;
 	int rval;
 
 	if (!config->dev)
@@ -809,6 +936,16 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 	nvmem->read_only = device_property_present(config->dev, "read-only") ||
 			   config->read_only || !nvmem->reg_write;
 
+	if (!device_property_read_string(config->dev, "nvmem-cell-parser-name",
+					 &parser_name)) {
+		rval = nvmem_parser_bind(nvmem, parser_name);
+		if (rval) {
+			ida_free(&nvmem_ida, nvmem->id);
+			kfree(nvmem);
+			return ERR_PTR(rval);
+		}
+	}
+
 #ifdef CONFIG_NVMEM_SYSFS
 	nvmem->dev.groups = nvmem_dev_groups;
 #endif
@@ -837,6 +974,9 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			goto err_teardown_compat;
 	}
 
+	if (nvmem->parser_data)
+		nvmem_cells_parse(nvmem);
+
 	rval = nvmem_add_cells_from_table(nvmem);
 	if (rval)
 		goto err_remove_cells;
@@ -857,6 +997,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 err_device_del:
 	device_del(&nvmem->dev);
 err_put_device:
+	if (nvmem->parser_data)
+		nvmem_parser_unbind(nvmem);
 	put_device(&nvmem->dev);
 
 	return ERR_PTR(rval);
@@ -1891,6 +2033,42 @@ const char *nvmem_dev_name(struct nvmem_device *nvmem)
 }
 EXPORT_SYMBOL_GPL(nvmem_dev_name);
 
+struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config)
+{
+	struct nvmem_parser *parser;
+
+	if (!config->cells_parse)
+		return ERR_PTR(-EINVAL);
+
+	if (!config->name)
+		return ERR_PTR(-EINVAL);
+
+	parser = kzalloc(sizeof(*parser), GFP_KERNEL);
+	if (!parser)
+		return ERR_PTR(-ENOMEM);
+
+	parser->cells_parse = config->cells_parse;
+	parser->owner = config->owner;
+	parser->name = config->name;
+
+	mutex_lock(&nvmem_parser_mutex);
+	list_add(&parser->head, &nvmem_parser_list);
+	mutex_unlock(&nvmem_parser_mutex);
+
+	return parser;
+}
+EXPORT_SYMBOL(nvmem_parser_register);
+
+void nvmem_parser_unregister(struct nvmem_parser *parser)
+{
+	mutex_lock(&nvmem_parser_mutex);
+	list_del(&parser->head);
+	kfree(parser);
+	mutex_unlock(&nvmem_parser_mutex);
+}
+EXPORT_SYMBOL(nvmem_parser_unregister);
+
 static int __init nvmem_init(void)
 {
 	return bus_register(&nvmem_bus_type);
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index e162b757b6d5..447241706554 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -15,10 +15,15 @@
 
 struct nvmem_device;
 struct nvmem_cell_info;
+struct nvmem_cell_table;
+struct nvmem_parser_data;
+
 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,
 				 void *val, size_t bytes);
+typedef int (*nvmem_parse_t)(struct nvmem_device *nvmem,
+			     struct nvmem_parser_data *data);
 
 enum nvmem_type {
 	NVMEM_TYPE_UNKNOWN = 0,
@@ -117,6 +122,19 @@ struct nvmem_cell_table {
 	struct list_head	node;
 };
 
+struct nvmem_parser_config {
+	const char	*name;
+	nvmem_parse_t	cells_parse;
+	struct module	*owner;
+};
+
+struct nvmem_parser_data {
+	struct nvmem_cell_table		table;
+	struct nvmem_cell_lookup	*lookup;
+	int				nlookups;
+	struct nvmem_parser		*parser;
+};
+
 #if IS_ENABLED(CONFIG_NVMEM)
 
 struct nvmem_device *nvmem_register(const struct nvmem_config *cfg);
@@ -130,6 +148,11 @@ int devm_nvmem_unregister(struct device *dev, struct nvmem_device *nvmem);
 void nvmem_add_cell_table(struct nvmem_cell_table *table);
 void nvmem_del_cell_table(struct nvmem_cell_table *table);
 
+struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config);
+
+void nvmem_parser_unregister(struct nvmem_parser *parser);
+
 #else
 
 static inline struct nvmem_device *nvmem_register(const struct nvmem_config *c)
@@ -154,5 +177,13 @@ devm_nvmem_unregister(struct device *dev, struct nvmem_device *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 struct nvmem_parser *
+nvmem_parser_register(const struct nvmem_parser_config *config)
+{
+	return -EOPNOTSUPP;
+}
+
+static inline void nvmem_parser_unregister(struct nvmem_parser *parser) {}
+
 #endif /* CONFIG_NVMEM */
 #endif  /* ifndef _LINUX_NVMEM_PROVIDER_H */
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property
  2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan
  2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
@ 2021-06-08 19:03 ` Vadym Kochan
  2021-06-18 20:59   ` Rob Herring
  2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan
  2 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw)
  To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: Robert Marko, Vadym Kochan

Describe new "nvmem-cells-parser-name" which is used to bind registered
parser with nvmem device.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
 Documentation/devicetree/bindings/nvmem/nvmem.yaml | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
index b8dc3d2b6e92..36c6361d0605 100644
--- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
+++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
@@ -39,6 +39,11 @@ properties:
       when it's driven low (logical '0') to allow writing.
     maxItems: 1
 
+  nvmem-cells-parser-name:
+    description:
+      Name of the registered parser to parse and register the cells.
+    maxItems: 1
+
 patternProperties:
   "^.*@[0-9a-f]+$":
     type: object
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser
  2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan
  2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
  2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan
@ 2021-06-08 19:03 ` Vadym Kochan
  2021-08-06 15:39   ` Jan Lübbe
  2 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-06-08 19:03 UTC (permalink / raw)
  To: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: Robert Marko, Vadym Kochan

ONIE is a small operating system, pre-installed on bare metal network
switches, that provides an environment for automated provisioning.

This system requires that NVMEM (EEPROM) device holds various system
information (mac address, platform name, etc) in a special TLV layout.

The driver registers ONIE TLV attributes as NVMEM cells which can be
accessed by other platform driver. Also it allows to use
of_get_mac_address() to retrieve mac address for the netdev.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
v2:
    1) onie-cells.c renamed to onie-tlv-cells.c

    2) Do not register onie cells parser via platform driver,
       but via module_init().

    3) Simplified cells table & lookups registration by just filling
       all this in nvmem_parser_data.

 drivers/nvmem/Kconfig          |   9 +
 drivers/nvmem/Makefile         |   2 +
 drivers/nvmem/onie-tlv-cells.c | 302 +++++++++++++++++++++++++++++++++
 3 files changed, 313 insertions(+)
 create mode 100644 drivers/nvmem/onie-tlv-cells.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index dd2019006838..a08ff087361b 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -288,4 +288,13 @@ config NVMEM_BRCM_NVRAM
 	  This driver provides support for Broadcom's NVRAM that can be accessed
 	  using I/O mapping.
 
+config NVMEM_ONIE_TLV_CELLS
+	tristate "ONIE TLV cells support"
+	help
+	  This is a driver to provide cells from ONIE TLV structure stored
+	  on NVME device.
+
+	  This driver can also be built as a module. If so, the module
+	  will be called nvmem-onie-tlv-cells.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index bbea1410240a..f70d7b817377 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -59,3 +59,5 @@ obj-$(CONFIG_NVMEM_RMEM) 	+= nvmem-rmem.o
 nvmem-rmem-y			:= rmem.o
 obj-$(CONFIG_NVMEM_BRCM_NVRAM)	+= nvmem_brcm_nvram.o
 nvmem_brcm_nvram-y		:= brcm_nvram.o
+obj-$(CONFIG_NVMEM_ONIE_TLV_CELLS)	+= nvmem-onie-tlv-cells.o
+nvmem-onie-tlv-cells-y		:= onie-tlv-cells.o
diff --git a/drivers/nvmem/onie-tlv-cells.c b/drivers/nvmem/onie-tlv-cells.c
new file mode 100644
index 000000000000..85b1c92da0c5
--- /dev/null
+++ b/drivers/nvmem/onie-tlv-cells.c
@@ -0,0 +1,302 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ONIE NVMEM cells provider
+ *
+ * Author: Vadym Kochan <vadym.kochan@plvision.eu>
+ */
+
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/kref.h>
+#include <linux/module.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/slab.h>
+
+#define ONIE_NVMEM_TLV_MAX_LEN	2048
+
+#define ONIE_NVMEM_HDR_ID	"TlvInfo"
+
+struct onie_tlv_hdr {
+	u8 id[8];
+	u8 version;
+	__be16 data_len;
+} __packed;
+
+struct onie_tlv {
+	u8 type;
+	u8 len;
+	u8 val[0];
+} __packed;
+
+struct onie_nvmem_attr {
+	struct list_head head;
+	const char *name;
+	unsigned int offset;
+	unsigned int len;
+};
+
+struct onie_tlv_parser {
+	unsigned int attr_count;
+	struct list_head attrs;
+	struct device *dev;
+
+	struct nvmem_cell_lookup *lookup;
+	int			nlookups;
+};
+
+static struct nvmem_parser *nvmem_parser;
+
+static bool onie_nvmem_hdr_is_valid(struct onie_tlv_hdr *hdr)
+{
+	if (memcmp(hdr->id, ONIE_NVMEM_HDR_ID, sizeof(hdr->id)) != 0)
+		return false;
+	if (hdr->version != 0x1)
+		return false;
+
+	return true;
+}
+
+static void onie_nvmem_attrs_free(struct onie_tlv_parser *parser)
+{
+	struct onie_nvmem_attr *attr, *tmp;
+
+	list_for_each_entry_safe(attr, tmp, &parser->attrs, head) {
+		list_del(&attr->head);
+		kfree(attr);
+	}
+}
+
+static const char *onie_nvmem_attr_name(u8 type)
+{
+	switch (type) {
+	case 0x21: return "product-name";
+	case 0x22: return "part-number";
+	case 0x23: return "serial-number";
+	case 0x24: return "mac-address";
+	case 0x25: return "manufacture-date";
+	case 0x26: return "device-version";
+	case 0x27: return "label-revision";
+	case 0x28: return "platforn-name";
+	case 0x29: return "onie-version";
+	case 0x2A: return "num-macs";
+	case 0x2B: return "manufacturer";
+	case 0x2C: return "country-code";
+	case 0x2D: return "vendor";
+	case 0x2E: return "diag-version";
+	case 0x2F: return "service-tag";
+	case 0xFD: return "vendor-extension";
+	case 0xFE: return "crc32";
+
+	default: return "unknown";
+	}
+}
+
+static int onie_nvmem_tlv_parse(struct onie_tlv_parser *parser, u8 *data, u16 len)
+{
+	unsigned int hlen = sizeof(struct onie_tlv_hdr);
+	unsigned int offset = 0;
+	int err;
+
+	parser->attr_count = 0;
+
+	while (offset < len) {
+		struct onie_nvmem_attr *attr;
+		struct onie_tlv *tlv;
+
+		tlv = (struct onie_tlv *)(data + offset);
+
+		if (offset + tlv->len >= len) {
+			pr_err("TLV len is too big(0x%x) at 0x%x\n",
+				tlv->len, hlen + offset);
+
+			/* return success in case something was parsed */
+			return 0;
+		}
+
+		attr = kmalloc(sizeof(*attr), GFP_KERNEL);
+		if (!attr) {
+			err = -ENOMEM;
+			goto err_attr_alloc;
+		}
+
+		attr->name = onie_nvmem_attr_name(tlv->type);
+		/* skip 'type' and 'len' */
+		attr->offset = hlen + offset + 2;
+		attr->len = tlv->len;
+
+		list_add(&attr->head, &parser->attrs);
+		parser->attr_count++;
+
+		offset += sizeof(*tlv) + tlv->len;
+	}
+
+	if (!parser->attr_count)
+		return -EINVAL;
+
+	return 0;
+
+err_attr_alloc:
+	onie_nvmem_attrs_free(parser);
+	return err;
+}
+
+static int onie_nvmem_decode(struct onie_tlv_parser *parser, struct nvmem_device *nvmem)
+{
+	struct onie_tlv_hdr hdr;
+	u8 *data;
+	u16 len;
+	int ret;
+
+	ret = nvmem_device_read(nvmem, 0, sizeof(hdr), &hdr);
+	if (ret < 0)
+		return ret;
+
+	if (!onie_nvmem_hdr_is_valid(&hdr)) {
+		pr_err("invalid ONIE TLV header\n");
+		return -EINVAL;
+	}
+
+	len = be16_to_cpu(hdr.data_len);
+
+	if (len > ONIE_NVMEM_TLV_MAX_LEN)
+		len = ONIE_NVMEM_TLV_MAX_LEN;
+
+	data = kmalloc(len, GFP_KERNEL);
+	if (!data)
+		return -ENOMEM;
+
+	ret = nvmem_device_read(nvmem, sizeof(hdr), len, data);
+	if (ret < 0)
+		goto err_data_read;
+
+	ret = onie_nvmem_tlv_parse(parser, data, len);
+	if (ret)
+		goto err_info_parse;
+
+	kfree(data);
+
+	return 0;
+
+err_info_parse:
+err_data_read:
+	kfree(data);
+	return ret;
+}
+
+static int onie_nvmem_cells_parse(struct onie_tlv_parser *parser,
+				  struct nvmem_device *nvmem,
+				  struct nvmem_cell_table *table)
+{
+	struct nvmem_cell_info *cells;
+	struct onie_nvmem_attr *attr;
+	unsigned int ncells = 0;
+	int err;
+
+	INIT_LIST_HEAD(&parser->attrs);
+	parser->attr_count = 0;
+
+	err = onie_nvmem_decode(parser, nvmem);
+	if (err)
+		return err;
+
+	cells = kmalloc_array(parser->attr_count, sizeof(*cells), GFP_KERNEL);
+	if (!cells) {
+		err = -ENOMEM;
+		goto err_cells_alloc;
+	}
+
+	parser->lookup = kmalloc_array(parser->attr_count,
+				     sizeof(struct nvmem_cell_lookup),
+				     GFP_KERNEL);
+	if (!parser->lookup) {
+		err = -ENOMEM;
+		goto err_lookup_alloc;
+	}
+
+	list_for_each_entry(attr, &parser->attrs, head) {
+		struct nvmem_cell_lookup *lookup;
+		struct nvmem_cell_info *cell;
+
+		cell = &cells[ncells];
+
+		lookup = &parser->lookup[ncells];
+		lookup->con_id = NULL;
+
+		cell->offset = attr->offset;
+		cell->name = attr->name;
+		cell->bytes = attr->len;
+		cell->bit_offset = 0;
+		cell->nbits = 0;
+
+		lookup->cell_name = cell->name;
+		lookup->con_id = cell->name;
+
+		ncells++;
+	}
+
+	table->ncells = ncells;
+	table->cells = cells;
+
+	parser->nlookups = ncells;
+
+	onie_nvmem_attrs_free(parser);
+
+	return 0;
+
+err_lookup_alloc:
+	kfree(cells);
+err_cells_alloc:
+	onie_nvmem_attrs_free(parser);
+
+	return err;
+}
+
+static int onie_cells_parse(struct nvmem_device *nvmem,
+			    struct nvmem_parser_data *data)
+{
+	struct onie_tlv_parser parser;
+	int err;
+
+	err = onie_nvmem_cells_parse(&parser, nvmem, &data->table);
+	if (err) {
+		pr_err("failed to parse ONIE attributes\n");
+		return err;
+	}
+
+	data->nlookups = parser.nlookups;
+	data->lookup = parser.lookup;
+
+	return 0;
+}
+
+static int __init onie_tlv_init(void)
+{
+	struct nvmem_parser_config parser_config = { };
+
+	parser_config.cells_parse = onie_cells_parse;
+	parser_config.owner = THIS_MODULE;
+	parser_config.name = "onie-tlv-cells";
+
+	nvmem_parser = nvmem_parser_register(&parser_config);
+	if (IS_ERR(nvmem_parser)) {
+		pr_err("failed to register %s parser\n", parser_config.name);
+		return PTR_ERR(nvmem_parser);
+	}
+
+	pr_info("registered %s parser\n", parser_config.name);
+
+	return 0;
+}
+
+static void __exit onie_tlv_exit(void)
+{
+	nvmem_parser_unregister(nvmem_parser);
+}
+
+module_init(onie_tlv_init);
+module_exit(onie_tlv_exit);
+
+MODULE_AUTHOR("Vadym Kochan <vadym.kochan@plvision.eu>");
+MODULE_DESCRIPTION("ONIE TLV NVMEM cells parser");
+MODULE_LICENSE("GPL");
-- 
2.17.1


^ permalink raw reply related	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
@ 2021-06-08 22:49     ` kernel test robot
  2021-06-09  3:05     ` kernel test robot
  2021-06-14 10:44   ` Srinivas Kandagatla
  2 siblings, 0 replies; 36+ messages in thread
From: kernel test robot @ 2021-06-08 22:49 UTC (permalink / raw)
  To: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: kbuild-all, Robert Marko, Vadym Kochan

[-- Attachment #1: Type: text/plain, Size: 2230 bytes --]

Hi Vadym,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linux/master linus/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: powerpc-allnoconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
        git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/rtc.h:18,
                    from arch/powerpc/kernel/time.c:47:
   include/linux/nvmem-provider.h: In function 'nvmem_parser_register':
>> include/linux/nvmem-provider.h:183:9: error: returning 'int' from a function with return type 'struct nvmem_parser *' makes pointer from integer without a cast [-Werror=int-conversion]
     183 |  return -EOPNOTSUPP;
         |         ^
   cc1: all warnings being treated as errors


vim +183 include/linux/nvmem-provider.h

   179	
   180	static inline struct nvmem_parser *
   181	nvmem_parser_register(const struct nvmem_parser_config *config)
   182	{
 > 183		return -EOPNOTSUPP;
   184	}
   185	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 7138 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
@ 2021-06-08 22:49     ` kernel test robot
  0 siblings, 0 replies; 36+ messages in thread
From: kernel test robot @ 2021-06-08 22:49 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 2283 bytes --]

Hi Vadym,

I love your patch! Yet something to improve:

[auto build test ERROR on robh/for-next]
[also build test ERROR on linux/master linus/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: powerpc-allnoconfig (attached as .config)
compiler: powerpc-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
        git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross ARCH=powerpc 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All errors (new ones prefixed by >>):

   In file included from include/linux/rtc.h:18,
                    from arch/powerpc/kernel/time.c:47:
   include/linux/nvmem-provider.h: In function 'nvmem_parser_register':
>> include/linux/nvmem-provider.h:183:9: error: returning 'int' from a function with return type 'struct nvmem_parser *' makes pointer from integer without a cast [-Werror=int-conversion]
     183 |  return -EOPNOTSUPP;
         |         ^
   cc1: all warnings being treated as errors


vim +183 include/linux/nvmem-provider.h

   179	
   180	static inline struct nvmem_parser *
   181	nvmem_parser_register(const struct nvmem_parser_config *config)
   182	{
 > 183		return -EOPNOTSUPP;
   184	}
   185	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 7138 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
@ 2021-06-09  3:05     ` kernel test robot
  2021-06-09  3:05     ` kernel test robot
  2021-06-14 10:44   ` Srinivas Kandagatla
  2 siblings, 0 replies; 36+ messages in thread
From: kernel test robot @ 2021-06-09  3:05 UTC (permalink / raw)
  To: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: kbuild-all, clang-built-linux, Robert Marko, Vadym Kochan

[-- Attachment #1: Type: text/plain, Size: 24230 bytes --]

Hi Vadym,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linux/master linus/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: s390-randconfig-r012-20210608 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d2012d965d60c3258b3a69d024491698f8aec386)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
        git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from lib/vsprintf.c:36:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   In file included from lib/vsprintf.c:40:
   In file included from include/net/addrconf.h:50:
   In file included from include/linux/ipv6.h:88:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from lib/vsprintf.c:40:
   In file included from include/net/addrconf.h:50:
   In file included from include/linux/ipv6.h:88:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from lib/vsprintf.c:40:
   In file included from include/net/addrconf.h:50:
   In file included from include/linux/ipv6.h:88:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   13 warnings generated.
--
   In file included from lib/test_printf.c:13:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   lib/test_printf.c:157:52: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                  ~~~~                       ^
                                  %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:55: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                       ~~~~                     ^
                                       %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:58: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                            ~~~~                   ^~~
                                            %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:63: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                                 ~~~~                   ^~~
                                                 %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:68: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                                      ~~~~                   ^~
                                                      %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:52: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                  ~~~~                       ^
                                  %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:55: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                       ~~~~                     ^
                                       %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:58: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                            ~~~~                   ^~~
                                            %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:63: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                                 ~~~~                   ^~~
                                                 %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:68: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                                      ~~~~                   ^~
                                                      %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:41: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                     ~~~          ^~~~
                                     %o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:47: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                        ~~~             ^~~~
                                        %o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:53: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                           ~~~~               ^~~~~~
                                           %#o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   14 warnings generated.
--
   In file included from block/partitions/msdos.c:31:
   In file included from block/partitions/check.h:3:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from block/partitions/msdos.c:31:
   In file included from block/partitions/check.h:3:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from block/partitions/msdos.c:31:
   In file included from block/partitions/check.h:3:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   In file included from block/partitions/msdos.c:32:
   In file included from block/partitions/efi.h:20:
   In file included from include/linux/efi.h:20:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   13 warnings generated.
--
   In file included from kernel/time/ntp.c:10:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from kernel/time/ntp.c:10:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from kernel/time/ntp.c:10:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   In file included from kernel/time/ntp.c:19:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   kernel/time/ntp.c:245:19: warning: unused function 'ntp_synced' [-Wunused-function]
   static inline int ntp_synced(void)
                     ^
   14 warnings generated.
..


vim +183 include/linux/nvmem-provider.h

   179	
   180	static inline struct nvmem_parser *
   181	nvmem_parser_register(const struct nvmem_parser_config *config)
   182	{
 > 183		return -EOPNOTSUPP;
   184	}
   185	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 15185 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
@ 2021-06-09  3:05     ` kernel test robot
  0 siblings, 0 replies; 36+ messages in thread
From: kernel test robot @ 2021-06-09  3:05 UTC (permalink / raw)
  To: kbuild-all

[-- Attachment #1: Type: text/plain, Size: 24594 bytes --]

Hi Vadym,

I love your patch! Perhaps something to improve:

[auto build test WARNING on robh/for-next]
[also build test WARNING on linux/master linus/master v5.13-rc5 next-20210608]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]

url:    https://github.com/0day-ci/linux/commits/Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
base:   https://git.kernel.org/pub/scm/linux/kernel/git/robh/linux.git for-next
config: s390-randconfig-r012-20210608 (attached as .config)
compiler: clang version 13.0.0 (https://github.com/llvm/llvm-project d2012d965d60c3258b3a69d024491698f8aec386)
reproduce (this is a W=1 build):
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # install s390 cross compiling tool for clang build
        # apt-get install binutils-s390x-linux-gnu
        # https://github.com/0day-ci/linux/commit/e7cd03f23c4980914d9de330fd018cb733f84d8f
        git remote add linux-review https://github.com/0day-ci/linux
        git fetch --no-tags linux-review Vadym-Kochan/nvmem-add-ONIE-NVMEM-cells-parser/20210609-031008
        git checkout e7cd03f23c4980914d9de330fd018cb733f84d8f
        # save the attached .config to linux build tree
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=s390 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>

All warnings (new ones prefixed by >>):

   In file included from lib/vsprintf.c:36:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   In file included from lib/vsprintf.c:40:
   In file included from include/net/addrconf.h:50:
   In file included from include/linux/ipv6.h:88:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from lib/vsprintf.c:40:
   In file included from include/net/addrconf.h:50:
   In file included from include/linux/ipv6.h:88:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from lib/vsprintf.c:40:
   In file included from include/net/addrconf.h:50:
   In file included from include/linux/ipv6.h:88:
   In file included from include/linux/tcp.h:17:
   In file included from include/linux/skbuff.h:31:
   In file included from include/linux/dma-mapping.h:10:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   13 warnings generated.
--
   In file included from lib/test_printf.c:13:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   lib/test_printf.c:157:52: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                  ~~~~                       ^
                                  %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:55: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                       ~~~~                     ^
                                       %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:58: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                            ~~~~                   ^~~
                                            %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:63: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                                 ~~~~                   ^~~
                                                 %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:157:68: warning: format specifies type 'unsigned char' but the argument has type 'int' [-Wformat]
           test("0|1|1|128|255", "%hhu|%hhu|%hhu|%hhu|%hhu", 0, 1, 257, 128, -1);
                                                      ~~~~                   ^~
                                                      %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:52: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                  ~~~~                       ^
                                  %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:55: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                       ~~~~                     ^
                                       %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:58: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                            ~~~~                   ^~~
                                            %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:63: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                                 ~~~~                   ^~~
                                                 %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:158:68: warning: format specifies type 'char' but the argument has type 'int' [-Wformat]
           test("0|1|1|-128|-1", "%hhd|%hhd|%hhd|%hhd|%hhd", 0, 1, 257, 128, -1);
                                                      ~~~~                   ^~
                                                      %d
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:41: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                     ~~~          ^~~~
                                     %o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:47: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                        ~~~             ^~~~
                                        %o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   lib/test_printf.c:159:53: warning: format specifies type 'unsigned short' but the argument has type 'int' [-Wformat]
           test("2015122420151225", "%ho%ho%#ho", 1037, 5282, -11627);
                                           ~~~~               ^~~~~~
                                           %#o
   lib/test_printf.c:137:40: note: expanded from macro 'test'
           __test(expect, strlen(expect), fmt, ##__VA_ARGS__)
                                          ~~~    ^~~~~~~~~~~
   14 warnings generated.
--
   In file included from block/partitions/msdos.c:31:
   In file included from block/partitions/check.h:3:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from block/partitions/msdos.c:31:
   In file included from block/partitions/check.h:3:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from block/partitions/msdos.c:31:
   In file included from block/partitions/check.h:3:
   In file included from include/linux/blkdev.h:25:
   In file included from include/linux/scatterlist.h:9:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   In file included from block/partitions/msdos.c:32:
   In file included from block/partitions/efi.h:20:
   In file included from include/linux/efi.h:20:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   13 warnings generated.
--
   In file included from kernel/time/ntp.c:10:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:464:31: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __raw_readb(PCI_IOBASE + addr);
                             ~~~~~~~~~~ ^
   include/asm-generic/io.h:477:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le16_to_cpu((__le16 __force)__raw_readw(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:36:59: note: expanded from macro '__le16_to_cpu'
   #define __le16_to_cpu(x) __swab16((__force __u16)(__le16)(x))
                                                             ^
   include/uapi/linux/swab.h:102:54: note: expanded from macro '__swab16'
   #define __swab16(x) (__u16)__builtin_bswap16((__u16)(x))
                                                        ^
   In file included from kernel/time/ntp.c:10:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:490:61: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           val = __le32_to_cpu((__le32 __force)__raw_readl(PCI_IOBASE + addr));
                                                           ~~~~~~~~~~ ^
   include/uapi/linux/byteorder/big_endian.h:34:59: note: expanded from macro '__le32_to_cpu'
   #define __le32_to_cpu(x) __swab32((__force __u32)(__le32)(x))
                                                             ^
   include/uapi/linux/swab.h:115:54: note: expanded from macro '__swab32'
   #define __swab32(x) (__u32)__builtin_bswap32((__u32)(x))
                                                        ^
   In file included from kernel/time/ntp.c:10:
   In file included from include/linux/clocksource.h:22:
   In file included from arch/s390/include/asm/io.h:75:
   include/asm-generic/io.h:501:33: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writeb(value, PCI_IOBASE + addr);
                               ~~~~~~~~~~ ^
   include/asm-generic/io.h:511:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writew((u16 __force)cpu_to_le16(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:521:59: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           __raw_writel((u32 __force)cpu_to_le32(value), PCI_IOBASE + addr);
                                                         ~~~~~~~~~~ ^
   include/asm-generic/io.h:609:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsb(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:617:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsw(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:625:20: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           readsl(PCI_IOBASE + addr, buffer, count);
                  ~~~~~~~~~~ ^
   include/asm-generic/io.h:634:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesb(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:643:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesw(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   include/asm-generic/io.h:652:21: warning: performing pointer arithmetic on a null pointer has undefined behavior [-Wnull-pointer-arithmetic]
           writesl(PCI_IOBASE + addr, buffer, count);
                   ~~~~~~~~~~ ^
   In file included from kernel/time/ntp.c:19:
   In file included from include/linux/rtc.h:18:
>> include/linux/nvmem-provider.h:183:9: warning: incompatible integer to pointer conversion returning 'int' from a function with result type 'struct nvmem_parser *' [-Wint-conversion]
           return -EOPNOTSUPP;
                  ^~~~~~~~~~~
   kernel/time/ntp.c:245:19: warning: unused function 'ntp_synced' [-Wunused-function]
   static inline int ntp_synced(void)
                     ^
   14 warnings generated.
..


vim +183 include/linux/nvmem-provider.h

   179	
   180	static inline struct nvmem_parser *
   181	nvmem_parser_register(const struct nvmem_parser_config *config)
   182	{
 > 183		return -EOPNOTSUPP;
   184	}
   185	

---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org

[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 15185 bytes --]

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
  2021-06-08 22:49     ` kernel test robot
  2021-06-09  3:05     ` kernel test robot
@ 2021-06-14 10:44   ` Srinivas Kandagatla
  2021-06-16 12:33     ` Vadym Kochan
  2021-09-08  9:44     ` Vadym Kochan
  2 siblings, 2 replies; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-06-14 10:44 UTC (permalink / raw)
  To: Vadym Kochan, Rob Herring, linux-kernel, devicetree; +Cc: Robert Marko



On 08/06/2021 20:03, Vadym Kochan wrote:
> Current implementation does not allow to register cells for already
> registered nvmem device and requires that this should be done before. But
> there might a driver which needs to parse the nvmem device and after
> register a cells table.
> 
> Introduce nvmem_parser API which allows to register cells parser which
> is called during nvmem device registration. During this stage the parser
> can read the nvmem device and register the cells table.
> 
> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
> v2:
>      1) Added nvmem_parser_data so parser implementation
>         should fill it with cells table and lookups which
>         will be registered by core.c after cells_parse was
>         succeed.
> 
>      2) Removed cells_remove callback from nvmem_parser_config which
>         is not needed because cells table & lookups are
>         registered/unregistered automatically by core.
> 
>      3) Use new device property to read cells parser name during nvmem
>         registration. Removed of_node usage.
> 
>      4) parser's module refcount is incremented/decremented on each parser
>         bind/unbind to nvmem device.
> 
>   drivers/nvmem/core.c           | 178 +++++++++++++++++++++++++++++++++
>   include/linux/nvmem-provider.h |  31 ++++++
>   2 files changed, 209 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index bca671ff4e54..648373ced6d4 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -39,6 +39,7 @@ struct nvmem_device {
>   	nvmem_reg_read_t	reg_read;
>   	nvmem_reg_write_t	reg_write;
>   	struct gpio_desc	*wp_gpio;
> +	struct nvmem_parser_data *parser_data;

This should be renamed to nvmem_cell_info_parser or something on those 
lines to avoid any misunderstanding on what exactly this parser is about.

May be can totally avoid this by using parser name only during register.

>   	void *priv;
>   };
>   
> @@ -57,6 +58,13 @@ struct nvmem_cell {
>   	struct list_head	node;
>   };
>   
> +struct nvmem_parser {
> +	struct list_head	head;
> +	nvmem_parse_t		cells_parse;
> +	const char		*name;
> +	struct module		*owner;
> +};
> +
>   static DEFINE_MUTEX(nvmem_mutex);
>   static DEFINE_IDA(nvmem_ida);
>   
> @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
>   static DEFINE_MUTEX(nvmem_lookup_mutex);
>   static LIST_HEAD(nvmem_lookup_list);
>   
> +static DEFINE_MUTEX(nvmem_parser_mutex);
> +static LIST_HEAD(nvmem_parser_list);
> +
>   static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>   
>   static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
> @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
>   	.name		= "nvmem",
>   };
>   
> +static struct nvmem_parser *__nvmem_parser_get(const char *name)
> +{
> +	struct nvmem_parser *tmp, *parser = NULL;
> +
> +	list_for_each_entry(tmp, &nvmem_parser_list, head) {
> +		if (strcmp(name, tmp->name) == 0) {
> +			parser = tmp;
> +			break;
> +		}
> +	}
> +
> +	if (!parser)
> +		return ERR_PTR(-EPROBE_DEFER);
> +
> +	if (!try_module_get(parser->owner)) {
> +		pr_err("could not increase module refcount for parser %s\n",
> +		       parser->name);
> +		return ERR_PTR(-EINVAL);
> +
> +	}
> +
> +	return parser;
> +}
> +
> +static void nvmem_parser_put(const struct nvmem_parser *parser)
> +{
> +	module_put(parser->owner);
> +}
> +
> +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
> +{
Do we really need parser bind/unbind mechanisms for what we are trying 
to do here.

It's just simply parsing cell info during nvmem register, do we really 
care if parser is there or not after that?

code can be probably made much simpler by just doing this in nvmem_register

parser_get()
parse_get_cell_info()
parser_put()

AFAIU, that is all we need.

> +	struct nvmem_parser_data *data;
> +	struct nvmem_parser *parser;
> +	int err;
> +
> +	mutex_lock(&nvmem_parser_mutex);
> +
> +	parser = __nvmem_parser_get(name);
> +	err = PTR_ERR_OR_ZERO(parser);
> +	if (!err) { > +		data = kzalloc(sizeof(*data), GFP_KERNEL);
> +		if (data) {
> +			data->parser = parser;
> +			nvmem->parser_data = data;
> +		} else {
> +			nvmem_parser_put(parser);
> +			err = -ENOMEM;
> +		}
> +	}
> +
> +	mutex_unlock(&nvmem_parser_mutex);
> +
> +	return err;
> +}
> +
> +static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
> +{
> +	struct nvmem_parser_data *data = nvmem->parser_data;
> +
> +	if (data->table.cells) {
> +		nvmem_del_cell_table(&data->table);
> +		kfree(data->table.cells);
who has allocated memory for this, its confusing for this to be freed in 
core.
> +	}
> +	if (data->lookup) { > +		nvmem_del_cell_lookups(data->lookup, data->nlookups);
> +		kfree(data->lookup);
> +	}
> +
> +	nvmem_parser_put(data->parser);
> +}
> +
> +static void nvmem_parser_data_register(struct nvmem_device *nvmem,
> +				       struct nvmem_parser_data *data)
> +{
> +	if (data->table.cells) {
> +		if (!data->table.nvmem_name)
> +			data->table.nvmem_name = nvmem_dev_name(nvmem);
> +
> +		nvmem_add_cell_table(&data->table);
> +	}
> +
> +	if (data->lookup) {
Why do we need lookups?
the cells are already associated with provider. lookups are normally 
used for associating devices with nvmemcell more for old non-dt machine 
code.

you can remove this.
> +		struct nvmem_cell_lookup *lookup = &data->lookup[0];
> +		int i = 0;
> +
> +		for (i = 0; i < data->nlookups; i++, lookup++) {
> +			if (!lookup->nvmem_name)
> +				lookup->nvmem_name = nvmem_dev_name(nvmem);
> +
> +			if (!lookup->dev_id)
> +				lookup->dev_id = data->parser->name;
> +		}
> +
> +		nvmem_add_cell_lookups(data->lookup, data->nlookups);
> +	}
> +}
> +

--srini

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-14 10:44   ` Srinivas Kandagatla
@ 2021-06-16 12:33     ` Vadym Kochan
  2021-06-21 11:00       ` Srinivas Kandagatla
  2021-09-08  9:44     ` Vadym Kochan
  1 sibling, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-06-16 12:33 UTC (permalink / raw)
  To: Srinivas Kandagatla; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko

Hi Srinivas,

On Mon, Jun 14, 2021 at 11:44:59AM +0100, Srinivas Kandagatla wrote:
> 
> 
> On 08/06/2021 20:03, Vadym Kochan wrote:
> > Current implementation does not allow to register cells for already
> > registered nvmem device and requires that this should be done before. But
> > there might a driver which needs to parse the nvmem device and after
> > register a cells table.
> > 
> > Introduce nvmem_parser API which allows to register cells parser which
> > is called during nvmem device registration. During this stage the parser
> > can read the nvmem device and register the cells table.
> > 
> > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> > ---
> > v2:
> >      1) Added nvmem_parser_data so parser implementation
> >         should fill it with cells table and lookups which
> >         will be registered by core.c after cells_parse was
> >         succeed.
> > 
> >      2) Removed cells_remove callback from nvmem_parser_config which
> >         is not needed because cells table & lookups are
> >         registered/unregistered automatically by core.
> > 
> >      3) Use new device property to read cells parser name during nvmem
> >         registration. Removed of_node usage.
> > 
> >      4) parser's module refcount is incremented/decremented on each parser
> >         bind/unbind to nvmem device.
> > 
> >   drivers/nvmem/core.c           | 178 +++++++++++++++++++++++++++++++++
> >   include/linux/nvmem-provider.h |  31 ++++++
> >   2 files changed, 209 insertions(+)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index bca671ff4e54..648373ced6d4 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -39,6 +39,7 @@ struct nvmem_device {
> >   	nvmem_reg_read_t	reg_read;
> >   	nvmem_reg_write_t	reg_write;
> >   	struct gpio_desc	*wp_gpio;
> > +	struct nvmem_parser_data *parser_data;
> 
> This should be renamed to nvmem_cell_info_parser or something on those lines
> to avoid any misunderstanding on what exactly this parser is about.
> 
> May be can totally avoid this by using parser name only during register.
> 

I added this to keep parsed cells particulary for this nvmem in case
same parser is used for several nvmem's and mostly because of using also
cell lookup info. I will try to also answer your below question why do I need
lookups ?

I use of_get_mac_address() func to fetch mac-address from nvmem cell.
Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
info of platform_device. I use the 2nd option with the following sample
solution:

	## DT ##
	eeprom_at24: at24@56 {
		compatible = "atmel,24c32";
		nvmem-cell-parser-name = "onie-tlv-cells";
		reg = <0x56>;
	};

	onie_tlv_parser: onie-tlv-cells {
		compatible = "nvmem-cell-parser";
		status = "okay";

---> add ability here to map cell con_id to cell_name ?

	};

	some_dev_node {
		compatible = "xxx";
		base-mac-provider = <&onie_tlv_parser>;
		status = "okay";
	};
	########

	== CODE ==
	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
	ret = of_get_mac_address(base_mac_np, base_mac);
	==========


And it works with this implementation because onie-tlv-cells is
registered as platform_device which name is the same as parser's name.
So the really tricky part for me is to make this cells lookup work.

Of course would be great if you can point a way/idea to get rid the need of
lookups.

> >   	void *priv;
> >   };
> > @@ -57,6 +58,13 @@ struct nvmem_cell {
> >   	struct list_head	node;
> >   };
> > +struct nvmem_parser {
> > +	struct list_head	head;
> > +	nvmem_parse_t		cells_parse;
> > +	const char		*name;
> > +	struct module		*owner;
> > +};
> > +
> >   static DEFINE_MUTEX(nvmem_mutex);
> >   static DEFINE_IDA(nvmem_ida);
> > @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
> >   static DEFINE_MUTEX(nvmem_lookup_mutex);
> >   static LIST_HEAD(nvmem_lookup_list);
> > +static DEFINE_MUTEX(nvmem_parser_mutex);
> > +static LIST_HEAD(nvmem_parser_list);
> > +
> >   static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
> >   static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
> > @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
> >   	.name		= "nvmem",
> >   };
> > +static struct nvmem_parser *__nvmem_parser_get(const char *name)
> > +{
> > +	struct nvmem_parser *tmp, *parser = NULL;
> > +
> > +	list_for_each_entry(tmp, &nvmem_parser_list, head) {
> > +		if (strcmp(name, tmp->name) == 0) {
> > +			parser = tmp;
> > +			break;
> > +		}
> > +	}
> > +
> > +	if (!parser)
> > +		return ERR_PTR(-EPROBE_DEFER);
> > +
> > +	if (!try_module_get(parser->owner)) {
> > +		pr_err("could not increase module refcount for parser %s\n",
> > +		       parser->name);
> > +		return ERR_PTR(-EINVAL);
> > +
> > +	}
> > +
> > +	return parser;
> > +}
> > +
> > +static void nvmem_parser_put(const struct nvmem_parser *parser)
> > +{
> > +	module_put(parser->owner);
> > +}
> > +
> > +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
> > +{
> Do we really need parser bind/unbind mechanisms for what we are trying to do
> here.
> 
> It's just simply parsing cell info during nvmem register, do we really care
> if parser is there or not after that?
> 
> code can be probably made much simpler by just doing this in nvmem_register
> 
> parser_get()
> parse_get_cell_info()
> parser_put()
> 
> AFAIU, that is all we need.
> 

because of need of lookup info (just in case if it is needed) the unbind
stage should free the lookups during the nvmem cells removal, but cells table
can be freed even during nvmem_register after cells were added.

> > +	struct nvmem_parser_data *data;
> > +	struct nvmem_parser *parser;
> > +	int err;
> > +
> > +	mutex_lock(&nvmem_parser_mutex);
> > +
> > +	parser = __nvmem_parser_get(name);
> > +	err = PTR_ERR_OR_ZERO(parser);
> > +	if (!err) { > +		data = kzalloc(sizeof(*data), GFP_KERNEL);
> > +		if (data) {
> > +			data->parser = parser;
> > +			nvmem->parser_data = data;
> > +		} else {
> > +			nvmem_parser_put(parser);
> > +			err = -ENOMEM;
> > +		}
> > +	}
> > +
> > +	mutex_unlock(&nvmem_parser_mutex);
> > +
> > +	return err;
> > +}
> > +
> > +static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
> > +{
> > +	struct nvmem_parser_data *data = nvmem->parser_data;
> > +
> > +	if (data->table.cells) {
> > +		nvmem_del_cell_table(&data->table);
> > +		kfree(data->table.cells);
> who has allocated memory for this, its confusing for this to be freed in
> core.

Well, looks like I need to call parser->cells_clear() so the parser driver will clear
things by itself.

> > +	}
> > +	if (data->lookup) { > +		nvmem_del_cell_lookups(data->lookup, data->nlookups);
> > +		kfree(data->lookup);
> > +	}
> > +
> > +	nvmem_parser_put(data->parser);
> > +}
> > +
> > +static void nvmem_parser_data_register(struct nvmem_device *nvmem,
> > +				       struct nvmem_parser_data *data)
> > +{
> > +	if (data->table.cells) {
> > +		if (!data->table.nvmem_name)
> > +			data->table.nvmem_name = nvmem_dev_name(nvmem);
> > +
> > +		nvmem_add_cell_table(&data->table);
> > +	}
> > +
> > +	if (data->lookup) {
> Why do we need lookups?
> the cells are already associated with provider. lookups are normally used
> for associating devices with nvmemcell more for old non-dt machine code.
> 
> you can remove this.

I hope I replied this in 1st reply.

> > +		struct nvmem_cell_lookup *lookup = &data->lookup[0];
> > +		int i = 0;
> > +
> > +		for (i = 0; i < data->nlookups; i++, lookup++) {
> > +			if (!lookup->nvmem_name)
> > +				lookup->nvmem_name = nvmem_dev_name(nvmem);
> > +
> > +			if (!lookup->dev_id)
> > +				lookup->dev_id = data->parser->name;
> > +		}
> > +
> > +		nvmem_add_cell_lookups(data->lookup, data->nlookups);
> > +	}
> > +}
> > +
> 
> --srini

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property
  2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan
@ 2021-06-18 20:59   ` Rob Herring
  0 siblings, 0 replies; 36+ messages in thread
From: Rob Herring @ 2021-06-18 20:59 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Srinivas Kandagatla, linux-kernel, devicetree, Robert Marko

On Tue, Jun 08, 2021 at 10:03:26PM +0300, Vadym Kochan wrote:
> Describe new "nvmem-cells-parser-name" which is used to bind registered
> parser with nvmem device.
> 
> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
>  Documentation/devicetree/bindings/nvmem/nvmem.yaml | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/nvmem/nvmem.yaml b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> index b8dc3d2b6e92..36c6361d0605 100644
> --- a/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> +++ b/Documentation/devicetree/bindings/nvmem/nvmem.yaml
> @@ -39,6 +39,11 @@ properties:
>        when it's driven low (logical '0') to allow writing.
>      maxItems: 1
>  
> +  nvmem-cells-parser-name:
> +    description:
> +      Name of the registered parser to parse and register the cells.
> +    maxItems: 1

I think this should be a 'compatible' string. We already have something 
along that line with:

Documentation/devicetree/bindings/mtd/partitions/nvmem-cells.yaml

Rob

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-16 12:33     ` Vadym Kochan
@ 2021-06-21 11:00       ` Srinivas Kandagatla
  2021-09-08  9:38         ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-06-21 11:00 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 16/06/2021 13:33, Vadym Kochan wrote:
>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>> index bca671ff4e54..648373ced6d4 100644
>>> --- a/drivers/nvmem/core.c
>>> +++ b/drivers/nvmem/core.c
>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>>    	nvmem_reg_read_t	reg_read;
>>>    	nvmem_reg_write_t	reg_write;
>>>    	struct gpio_desc	*wp_gpio;
>>> +	struct nvmem_parser_data *parser_data;
>> This should be renamed to nvmem_cell_info_parser or something on those lines
>> to avoid any misunderstanding on what exactly this parser is about.
>>
>> May be can totally avoid this by using parser name only during register.
>>
> I added this to keep parsed cells particulary for this nvmem in case
> same parser is used for several nvmem's and mostly because of using also
> cell lookup info. I will try to also answer your below question why do I need
> lookups ?
> 
> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
> info of platform_device. I use the 2nd option with the following sample
> solution:
> 
> 	## DT ##
> 	eeprom_at24: at24@56 {
> 		compatible = "atmel,24c32";
> 		nvmem-cell-parser-name = "onie-tlv-cells";
> 		reg = <0x56>;
> 	};
> 
> 	onie_tlv_parser: onie-tlv-cells {
> 		compatible = "nvmem-cell-parser";
> 		status = "okay";
> 
> ---> add ability here to map cell con_id to cell_name ?
> 
> 	};
> 
> 	some_dev_node {
> 		compatible = "xxx";
> 		base-mac-provider = <&onie_tlv_parser>;

Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as 
your mac provider?
If you use eeprom_at24 then of_get_mac_address() should get mac-address 
directly from cell info.


> 		status = "okay";
> 	};
> 	########
> 
> 	== CODE ==
> 	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
> 	ret = of_get_mac_address(base_mac_np, base_mac);
> 	==========
> 
> 
> And it works with this implementation because onie-tlv-cells is
> registered as platform_device which name is the same as parser's name.
> So the really tricky part for me is to make this cells lookup work.

cell lookups are more of intended for board files, adding them in this 
case is really not correct.  The whole purpose of this driver is to 
parse the tlv cell infos into nvmem cell info.


--srini


> 
> Of course would be great if you can point a way/idea to get rid the need of
> lookups.
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser
  2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan
@ 2021-08-06 15:39   ` Jan Lübbe
  2021-09-08  9:56     ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Jan Lübbe @ 2021-08-06 15:39 UTC (permalink / raw)
  To: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree
  Cc: Robert Marko

Hi,

On Tue, 2021-06-08 at 22:03 +0300, Vadym Kochan wrote:

...
> +	case 0x24: return "mac-address";
...
> +	case 0x2A: return "num-macs";

Is suspect these properties define which range of MACs is assigned to the
device. How would you use them to assign MAC addresses to multiple interfaces?
The nvmem-cells property in the network device's node can only refer to one
cell, and not to i.e the cells value + 1.

I think it would be useful to have a way to express this setup for systems with
many interfaces, but am unsure of where this should be described. Maybe a "mac-
address-offset" property in the generic ethernet controller binding?

Regards,
Jan
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-21 11:00       ` Srinivas Kandagatla
@ 2021-09-08  9:38         ` Vadym Kochan
  2021-09-13 14:19           ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-08  9:38 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko


Hi Srini,

Sorry for such delay in replies, I am still confused how to
implement it properly, let me please explain the issues
which I faced with:

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 16/06/2021 13:33, Vadym Kochan wrote:
>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>> index bca671ff4e54..648373ced6d4 100644
>>>> --- a/drivers/nvmem/core.c
>>>> +++ b/drivers/nvmem/core.c
>>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>>>    	nvmem_reg_read_t	reg_read;
>>>>    	nvmem_reg_write_t	reg_write;
>>>>    	struct gpio_desc	*wp_gpio;
>>>> +	struct nvmem_parser_data *parser_data;
>>> This should be renamed to nvmem_cell_info_parser or something on those lines
>>> to avoid any misunderstanding on what exactly this parser is about.
>>>
>>> May be can totally avoid this by using parser name only during register.
>>>
>> I added this to keep parsed cells particulary for this nvmem in case
>> same parser is used for several nvmem's and mostly because of using also
>> cell lookup info. I will try to also answer your below question why do I need
>> lookups ?
>> 
>> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
>> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
>> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
>> info of platform_device. I use the 2nd option with the following sample
>> solution:
>> 
>> 	## DT ##
>> 	eeprom_at24: at24@56 {
>> 		compatible = "atmel,24c32";
>> 		nvmem-cell-parser-name = "onie-tlv-cells";
>> 		reg = <0x56>;
>> 	};
>> 
>> 	onie_tlv_parser: onie-tlv-cells {
>> 		compatible = "nvmem-cell-parser";
>> 		status = "okay";
>> 
>> ---> add ability here to map cell con_id to cell_name ?
>> 
>> 	};
>> 
>> 	some_dev_node {
>> 		compatible = "xxx";
>> 		base-mac-provider = <&onie_tlv_parser>;
>
> Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as 
> your mac provider?
> If you use eeprom_at24 then of_get_mac_address() should get mac-address 
> directly from cell info.

1) This DT node is a trick to register it as a platform_device because of:

static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
{
        struct platform_device *pdev = of_find_device_by_node(np);
        struct nvmem_cell *cell;
        const void *mac;
        size_t len;
        int ret;
 
        /* Try lookup by device first, there might be a nvmem_cell_lookup
         * associated with a given device.
         */
        if (pdev) {
                ret = nvmem_get_mac_address(&pdev->dev, addr);
                put_device(&pdev->dev);
                return ret;
        }
 
        cell = of_nvmem_cell_get(np, "mac-address");
        if (IS_ERR(cell))
                return PTR_ERR(cell);
 
        ...
}

I tried to use at24_eeprom as ref in DTS file, but this device is not a
platform device but a nvmem bus device, so it fails on:

        ...

        struct platform_device *pdev = of_find_device_by_node(np);

        ...

        /* Try lookup by device first, there might be a nvmem_cell_lookup
         * associated with a given device.
         */
        if (pdev) {
                ret = nvmem_get_mac_address(&pdev->dev, addr);
                put_device(&pdev->dev);
                return ret;
        }

        ...

Probably this might be fixed by lookup nvmem device too ?

2) Regarding cell lookups registration, I had to use it because
of_nvmem_cell_get() will not find parser cells via OF.

>
>
>> 		status = "okay";
>> 	};
>> 	########
>> 
>> 	== CODE ==
>> 	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
>> 	ret = of_get_mac_address(base_mac_np, base_mac);
>> 	==========
>> 
>> 
>> And it works with this implementation because onie-tlv-cells is
>> registered as platform_device which name is the same as parser's name.
>> So the really tricky part for me is to make this cells lookup work.
>
> cell lookups are more of intended for board files, adding them in this 
> case is really not correct.  The whole purpose of this driver is to 
> parse the tlv cell infos into nvmem cell info.
>
>
> --srini
>
>
>> 
>> Of course would be great if you can point a way/idea to get rid the need of
>> lookups.
>> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-06-14 10:44   ` Srinivas Kandagatla
  2021-06-16 12:33     ` Vadym Kochan
@ 2021-09-08  9:44     ` Vadym Kochan
  1 sibling, 0 replies; 36+ messages in thread
From: Vadym Kochan @ 2021-09-08  9:44 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko


Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 08/06/2021 20:03, Vadym Kochan wrote:
>> Current implementation does not allow to register cells for already
>> registered nvmem device and requires that this should be done before. But
>> there might a driver which needs to parse the nvmem device and after
>> register a cells table.
>> 
>> Introduce nvmem_parser API which allows to register cells parser which
>> is called during nvmem device registration. During this stage the parser
>> can read the nvmem device and register the cells table.
>> 
>> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
>> ---
>> v2:
>>      1) Added nvmem_parser_data so parser implementation
>>         should fill it with cells table and lookups which
>>         will be registered by core.c after cells_parse was
>>         succeed.
>> 
>>      2) Removed cells_remove callback from nvmem_parser_config which
>>         is not needed because cells table & lookups are
>>         registered/unregistered automatically by core.
>> 
>>      3) Use new device property to read cells parser name during nvmem
>>         registration. Removed of_node usage.
>> 
>>      4) parser's module refcount is incremented/decremented on each parser
>>         bind/unbind to nvmem device.
>> 
>>   drivers/nvmem/core.c           | 178 +++++++++++++++++++++++++++++++++
>>   include/linux/nvmem-provider.h |  31 ++++++
>>   2 files changed, 209 insertions(+)
>> 
>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>> index bca671ff4e54..648373ced6d4 100644
>> --- a/drivers/nvmem/core.c
>> +++ b/drivers/nvmem/core.c
>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>   	nvmem_reg_read_t	reg_read;
>>   	nvmem_reg_write_t	reg_write;
>>   	struct gpio_desc	*wp_gpio;
>> +	struct nvmem_parser_data *parser_data;
>
> This should be renamed to nvmem_cell_info_parser or something on those 
> lines to avoid any misunderstanding on what exactly this parser is about.
>
> May be can totally avoid this by using parser name only during register.
>
>>   	void *priv;
>>   };
>>   
>> @@ -57,6 +58,13 @@ struct nvmem_cell {
>>   	struct list_head	node;
>>   };
>>   
>> +struct nvmem_parser {
>> +	struct list_head	head;
>> +	nvmem_parse_t		cells_parse;
>> +	const char		*name;
>> +	struct module		*owner;
>> +};
>> +
>>   static DEFINE_MUTEX(nvmem_mutex);
>>   static DEFINE_IDA(nvmem_ida);
>>   
>> @@ -66,6 +74,9 @@ static LIST_HEAD(nvmem_cell_tables);
>>   static DEFINE_MUTEX(nvmem_lookup_mutex);
>>   static LIST_HEAD(nvmem_lookup_list);
>>   
>> +static DEFINE_MUTEX(nvmem_parser_mutex);
>> +static LIST_HEAD(nvmem_parser_list);
>> +
>>   static BLOCKING_NOTIFIER_HEAD(nvmem_notifier);
>>   
>>   static int __nvmem_reg_read(struct nvmem_device *nvmem, unsigned int offset,
>> @@ -418,6 +429,118 @@ static struct bus_type nvmem_bus_type = {
>>   	.name		= "nvmem",
>>   };
>>   
>> +static struct nvmem_parser *__nvmem_parser_get(const char *name)
>> +{
>> +	struct nvmem_parser *tmp, *parser = NULL;
>> +
>> +	list_for_each_entry(tmp, &nvmem_parser_list, head) {
>> +		if (strcmp(name, tmp->name) == 0) {
>> +			parser = tmp;
>> +			break;
>> +		}
>> +	}
>> +
>> +	if (!parser)
>> +		return ERR_PTR(-EPROBE_DEFER);
>> +
>> +	if (!try_module_get(parser->owner)) {
>> +		pr_err("could not increase module refcount for parser %s\n",
>> +		       parser->name);
>> +		return ERR_PTR(-EINVAL);
>> +
>> +	}
>> +
>> +	return parser;
>> +}
>> +
>> +static void nvmem_parser_put(const struct nvmem_parser *parser)
>> +{
>> +	module_put(parser->owner);
>> +}
>> +
>> +static int nvmem_parser_bind(struct nvmem_device *nvmem, const char *name)
>> +{
> Do we really need parser bind/unbind mechanisms for what we are trying 
> to do here.
>
> It's just simply parsing cell info during nvmem register, do we really 
> care if parser is there or not after that?
>
> code can be probably made much simpler by just doing this in nvmem_register
>
> parser_get()
> parse_get_cell_info()
> parser_put()
>
> AFAIU, that is all we need.
>

bind/unbind is just for connecting parser instance with a nvmem device,
but the real reason is because of need of these cell lookups info,
and then freeing them.

If it would be possible to lookup parser's cell without these lookups
(I tried to explain the issue with this in other reply in this series)
then yes, just adding cell info would be enough.

>> +	struct nvmem_parser_data *data;
>> +	struct nvmem_parser *parser;
>> +	int err;
>> +
>> +	mutex_lock(&nvmem_parser_mutex);
>> +
>> +	parser = __nvmem_parser_get(name);
>> +	err = PTR_ERR_OR_ZERO(parser);
>> +	if (!err) { > +		data = kzalloc(sizeof(*data), GFP_KERNEL);
>> +		if (data) {
>> +			data->parser = parser;
>> +			nvmem->parser_data = data;
>> +		} else {
>> +			nvmem_parser_put(parser);
>> +			err = -ENOMEM;
>> +		}
>> +	}
>> +
>> +	mutex_unlock(&nvmem_parser_mutex);
>> +
>> +	return err;
>> +}
>> +
>> +static void nvmem_parser_unbind(const struct nvmem_device *nvmem)
>> +{
>> +	struct nvmem_parser_data *data = nvmem->parser_data;
>> +
>> +	if (data->table.cells) {
>> +		nvmem_del_cell_table(&data->table);
>> +		kfree(data->table.cells);
> who has allocated memory for this, its confusing for this to be freed in 
> core.
>> +	}
>> +	if (data->lookup) { > +		nvmem_del_cell_lookups(data->lookup, data->nlookups);
>> +		kfree(data->lookup);
>> +	}
>> +
>> +	nvmem_parser_put(data->parser);
>> +}
>> +
>> +static void nvmem_parser_data_register(struct nvmem_device *nvmem,
>> +				       struct nvmem_parser_data *data)
>> +{
>> +	if (data->table.cells) {
>> +		if (!data->table.nvmem_name)
>> +			data->table.nvmem_name = nvmem_dev_name(nvmem);
>> +
>> +		nvmem_add_cell_table(&data->table);
>> +	}
>> +
>> +	if (data->lookup) {
> Why do we need lookups?
> the cells are already associated with provider. lookups are normally 
> used for associating devices with nvmemcell more for old non-dt machine 
> code.
>
> you can remove this.
>> +		struct nvmem_cell_lookup *lookup = &data->lookup[0];
>> +		int i = 0;
>> +
>> +		for (i = 0; i < data->nlookups; i++, lookup++) {
>> +			if (!lookup->nvmem_name)
>> +				lookup->nvmem_name = nvmem_dev_name(nvmem);
>> +
>> +			if (!lookup->dev_id)
>> +				lookup->dev_id = data->parser->name;
>> +		}
>> +
>> +		nvmem_add_cell_lookups(data->lookup, data->nlookups);
>> +	}
>> +}
>> +
>
> --srini


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser
  2021-08-06 15:39   ` Jan Lübbe
@ 2021-09-08  9:56     ` Vadym Kochan
  2021-09-12 21:06       ` John Thomson
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-08  9:56 UTC (permalink / raw)
  To: Jan Lübbe
  Cc: Vadym Kochan, Srinivas Kandagatla, Rob Herring, linux-kernel,
	devicetree, Robert Marko


Hi Jan,

Jan Lübbe <jlu@pengutronix.de> writes:

> Hi,
>
> On Tue, 2021-06-08 at 22:03 +0300, Vadym Kochan wrote:
>
> ...
>> +	case 0x24: return "mac-address";
> ...

This is a base mac,

>> +	case 0x2A: return "num-macs";
>
> Is suspect these properties define which range of MACs is assigned to the

Yes

> device. How would you use them to assign MAC addresses to multiple interfaces?
> The nvmem-cells property in the network device's node can only refer to one
> cell, and not to i.e the cells value + 1.
>

Currently in net/ethernet/marvell/prestera/prestera_main.c it is
incremented and hard-coded by the driver.

> I think it would be useful to have a way to express this setup for systems with
> many interfaces, but am unsure of where this should be described. Maybe a "mac-
> address-offset" property in the generic ethernet controller binding?
>
> Regards,
> Jan

May be something like eth_address_provider should be introduced in
net/ethernet/ ?

This provider can provide something like eth_provider_address_next() which
will consider "mac-address-num" (or other specific fields).

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser
  2021-09-08  9:56     ` Vadym Kochan
@ 2021-09-12 21:06       ` John Thomson
  2021-09-13 14:20         ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: John Thomson @ 2021-09-12 21:06 UTC (permalink / raw)
  To: Vadym Kochan, Jan Lübbe
  Cc: Srinivas Kandagatla, Rob Herring, linux-kernel, devicetree, Robert Marko

Hi Vadym,

On Wed, 8 Sep 2021, at 09:56, Vadym Kochan wrote:
> 
> Hi Jan,
> 
> Jan Lübbe <jlu@pengutronix.de> writes:
> 
> > I think it would be useful to have a way to express this setup for systems with
> > many interfaces, but am unsure of where this should be described. Maybe a "mac-
> > address-offset" property in the generic ethernet controller binding?
> >
> > Regards,
> > Jan
> 
> May be something like eth_address_provider should be introduced in
> net/ethernet/ ?
> 
> This provider can provide something like eth_provider_address_next() which
> will consider "mac-address-num" (or other specific fields).
> 

A patch series proposed the devicetree property
mac-address-increment, but it did not get support at the time:
of_net: add mac-address-increment support
https://lore.kernel.org/all/20200920095724.8251-4-ansuelsmth@gmail.com/
dt-bindings: net: Document use of mac-address-increment
https://lore.kernel.org/all/20200920095724.8251-5-ansuelsmth@gmail.com/

Cheers,
-- 
  John Thomson

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-08  9:38         ` Vadym Kochan
@ 2021-09-13 14:19           ` Srinivas Kandagatla
  2021-09-20 10:24             ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-13 14:19 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko

Hi Vadym,


On 08/09/2021 10:38, Vadym Kochan wrote:
> 
> Hi Srini,
> 
> Sorry for such delay in replies, I am still confused how to
> implement it properly, let me please explain the issues
> which I faced with:

> 
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
> 
>> On 16/06/2021 13:33, Vadym Kochan wrote:
>>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>>> index bca671ff4e54..648373ced6d4 100644
>>>>> --- a/drivers/nvmem/core.c
>>>>> +++ b/drivers/nvmem/core.c
>>>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>>>>     	nvmem_reg_read_t	reg_read;
>>>>>     	nvmem_reg_write_t	reg_write;
>>>>>     	struct gpio_desc	*wp_gpio;
>>>>> +	struct nvmem_parser_data *parser_data;
>>>> This should be renamed to nvmem_cell_info_parser or something on those lines
>>>> to avoid any misunderstanding on what exactly this parser is about.
>>>>
>>>> May be can totally avoid this by using parser name only during register.
>>>>
>>> I added this to keep parsed cells particulary for this nvmem in case
>>> same parser is used for several nvmem's and mostly because of using also
>>> cell lookup info. I will try to also answer your below question why do I need
>>> lookups ?
>>>
>>> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
>>> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
>>> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
>>> info of platform_device. I use the 2nd option with the following sample
>>> solution:
>>>
>>> 	## DT ##
>>> 	eeprom_at24: at24@56 {
>>> 		compatible = "atmel,24c32";
>>> 		nvmem-cell-parser-name = "onie-tlv-cells";
>>> 		reg = <0x56>;
>>> 	};
>>>
>>> 	onie_tlv_parser: onie-tlv-cells {
>>> 		compatible = "nvmem-cell-parser";
>>> 		status = "okay";
>>>
>>> ---> add ability here to map cell con_id to cell_name ?
>>>
>>> 	};
>>>
>>> 	some_dev_node {
>>> 		compatible = "xxx";
>>> 		base-mac-provider = <&onie_tlv_parser>;
>>
>> Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as
>> your mac provider?
>> If you use eeprom_at24 then of_get_mac_address() should get mac-address
>> directly from cell info.
> 
> 1) This DT node is a trick to register it as a platform_device because of:
> 
> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
> {
>          struct platform_device *pdev = of_find_device_by_node(np);
>          struct nvmem_cell *cell;
>          const void *mac;
>          size_t len;
>          int ret;
>   
>          /* Try lookup by device first, there might be a nvmem_cell_lookup
>           * associated with a given device.
>           */
>          if (pdev) {
>                  ret = nvmem_get_mac_address(&pdev->dev, addr);
>                  put_device(&pdev->dev);
>                  return ret;
>          }
>   
>          cell = of_nvmem_cell_get(np, "mac-address");
>          if (IS_ERR(cell))
>                  return PTR_ERR(cell);
>   
>          ...
> }
> 
> I tried to use at24_eeprom as ref in DTS file, but this device is not a
> platform device but a nvmem bus device, so it fails on:
> 
>          ...
> 
>          struct platform_device *pdev = of_find_device_by_node(np);
> 
>          ...
> 
>          /* Try lookup by device first, there might be a nvmem_cell_lookup
>           * associated with a given device.
>           */
>          if (pdev) {
>                  ret = nvmem_get_mac_address(&pdev->dev, addr);
>                  put_device(&pdev->dev);
>                  return ret;
>          }
> 
>          ...
> 
> Probably this might be fixed by lookup nvmem device too ?

Honestly, this approach is a total hack to get it working.

This is what Am thinking which should look like:

## DT ##
eeprom_at24: at24@56 {
	compatible = "atmel,24c32";
	/* Some way to identify that this is a TLV data */
	nvmem-cell-parser-name = "onie-tlv-cells";
	reg = <0x56>;

	mac_address: mac-address {
		/* THIS REG is updated once TLV is parsed */
		reg = <0 0x6>
	};
};

some_dev_node {
	compatible = "xxx";
	nvmem-cells = <&mac_address>;
	nvmem-cell-names = "mac-address";
};

== CODE ==
ret = of_get_mac_address(dev->of_node, base_mac);
==========


If you notice the mac_address node reg is 0.
This node "reg" property should be updated ( using of_update_property()) 
by nvmem-provider driver while tlv parsing and by matching the node name 
with tlv name.

That way rest of the code will work as usual.

If this work for you then we can ask Rob if he foresee any issues in 
this approach. I already see similar usage in reserved-memory case.


--srini

> 
> 2) Regarding cell lookups registration, I had to use it because
> of_nvmem_cell_get() will not find parser cells via OF.
> 
>>
>>
>>> 		status = "okay";
>>> 	};
>>> 	########
>>>
>>> 	== CODE ==
>>> 	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
>>> 	ret = of_get_mac_address(base_mac_np, base_mac);
>>> 	==========
>>>
>>>
>>> And it works with this implementation because onie-tlv-cells is
>>> registered as platform_device which name is the same as parser's name.
>>> So the really tricky part for me is to make this cells lookup work.
>>
>> cell lookups are more of intended for board files, adding them in this
>> case is really not correct.  The whole purpose of this driver is to
>> parse the tlv cell infos into nvmem cell info.
>>
>>
>> --srini
>>
>>
>>>
>>> Of course would be great if you can point a way/idea to get rid the need of
>>> lookups.
>>>
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser
  2021-09-12 21:06       ` John Thomson
@ 2021-09-13 14:20         ` Srinivas Kandagatla
  0 siblings, 0 replies; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-13 14:20 UTC (permalink / raw)
  To: John Thomson, Vadym Kochan, Jan Lübbe
  Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 12/09/2021 22:06, John Thomson wrote:
> Hi Vadym,
> 
> On Wed, 8 Sep 2021, at 09:56, Vadym Kochan wrote:
>>
>> Hi Jan,
>>
>> Jan Lübbe <jlu@pengutronix.de> writes:
> …
>>
>>> I think it would be useful to have a way to express this setup for systems with
>>> many interfaces, but am unsure of where this should be described. Maybe a "mac-
>>> address-offset" property in the generic ethernet controller binding?
>>>
>>> Regards,
>>> Jan
>>
>> May be something like eth_address_provider should be introduced in
>> net/ethernet/ ?
>>
>> This provider can provide something like eth_provider_address_next() which
>> will consider "mac-address-num" (or other specific fields).
>>
> 
> A patch series proposed the devicetree property
> mac-address-increment, but it did not get support at the time:

Please have a look at some recent nvmem patches 
https://lkml.org/lkml/2021/9/8/270 that adds support for vendor specific 
post-processing of nvmem-cells.

Am hoping that increment usecase (along with other variants) should also 
be dealt in similar way.


--srini
> of_net: add mac-address-increment support
> https://lore.kernel.org/all/20200920095724.8251-4-ansuelsmth@gmail.com/
> dt-bindings: net: Document use of mac-address-increment
> https://lore.kernel.org/all/20200920095724.8251-5-ansuelsmth@gmail.com/
> 



> Cheers,
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-13 14:19           ` Srinivas Kandagatla
@ 2021-09-20 10:24             ` Vadym Kochan
  2021-09-20 10:36               ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-20 10:24 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko

Hi Srini,

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> Hi Vadym,
>
>
> On 08/09/2021 10:38, Vadym Kochan wrote:
>> 
>> Hi Srini,
>> 
>> Sorry for such delay in replies, I am still confused how to
>> implement it properly, let me please explain the issues
>> which I faced with:
>
>> 
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>> 
>>> On 16/06/2021 13:33, Vadym Kochan wrote:
>>>>>> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
>>>>>> index bca671ff4e54..648373ced6d4 100644
>>>>>> --- a/drivers/nvmem/core.c
>>>>>> +++ b/drivers/nvmem/core.c
>>>>>> @@ -39,6 +39,7 @@ struct nvmem_device {
>>>>>>     	nvmem_reg_read_t	reg_read;
>>>>>>     	nvmem_reg_write_t	reg_write;
>>>>>>     	struct gpio_desc	*wp_gpio;
>>>>>> +	struct nvmem_parser_data *parser_data;
>>>>> This should be renamed to nvmem_cell_info_parser or something on those lines
>>>>> to avoid any misunderstanding on what exactly this parser is about.
>>>>>
>>>>> May be can totally avoid this by using parser name only during register.
>>>>>
>>>> I added this to keep parsed cells particulary for this nvmem in case
>>>> same parser is used for several nvmem's and mostly because of using also
>>>> cell lookup info. I will try to also answer your below question why do I need
>>>> lookups ?
>>>>
>>>> I use of_get_mac_address() func to fetch mac-address from nvmem cell.
>>>> Eventually this func calls of_get_mac_addr_nvmem() which (as I understand it
>>>> correctly) can find cells via DT by parsing "nvmem-cell-names" or via cell lookup
>>>> info of platform_device. I use the 2nd option with the following sample
>>>> solution:
>>>>
>>>> 	## DT ##
>>>> 	eeprom_at24: at24@56 {
>>>> 		compatible = "atmel,24c32";
>>>> 		nvmem-cell-parser-name = "onie-tlv-cells";
>>>> 		reg = <0x56>;
>>>> 	};
>>>>
>>>> 	onie_tlv_parser: onie-tlv-cells {
>>>> 		compatible = "nvmem-cell-parser";
>>>> 		status = "okay";
>>>>
>>>> ---> add ability here to map cell con_id to cell_name ?
>>>>
>>>> 	};
>>>>
>>>> 	some_dev_node {
>>>> 		compatible = "xxx";
>>>> 		base-mac-provider = <&onie_tlv_parser>;
>>>
>>> Real nvmem provider is eeprom_at24, why do you use onie_tlv_parse as
>>> your mac provider?
>>> If you use eeprom_at24 then of_get_mac_address() should get mac-address
>>> directly from cell info.
>> 
>> 1) This DT node is a trick to register it as a platform_device because of:
>> 
>> static int of_get_mac_addr_nvmem(struct device_node *np, u8 *addr)
>> {
>>          struct platform_device *pdev = of_find_device_by_node(np);
>>          struct nvmem_cell *cell;
>>          const void *mac;
>>          size_t len;
>>          int ret;
>>   
>>          /* Try lookup by device first, there might be a nvmem_cell_lookup
>>           * associated with a given device.
>>           */
>>          if (pdev) {
>>                  ret = nvmem_get_mac_address(&pdev->dev, addr);
>>                  put_device(&pdev->dev);
>>                  return ret;
>>          }
>>   
>>          cell = of_nvmem_cell_get(np, "mac-address");
>>          if (IS_ERR(cell))
>>                  return PTR_ERR(cell);
>>   
>>          ...
>> }
>> 
>> I tried to use at24_eeprom as ref in DTS file, but this device is not a
>> platform device but a nvmem bus device, so it fails on:
>> 
>>          ...
>> 
>>          struct platform_device *pdev = of_find_device_by_node(np);
>> 
>>          ...
>> 
>>          /* Try lookup by device first, there might be a nvmem_cell_lookup
>>           * associated with a given device.
>>           */
>>          if (pdev) {
>>                  ret = nvmem_get_mac_address(&pdev->dev, addr);
>>                  put_device(&pdev->dev);
>>                  return ret;
>>          }
>> 
>>          ...
>> 
>> Probably this might be fixed by lookup nvmem device too ?
>
> Honestly, this approach is a total hack to get it working.
>
> This is what Am thinking which should look like:
>
> ## DT ##
> eeprom_at24: at24@56 {
> 	compatible = "atmel,24c32";
> 	/* Some way to identify that this is a TLV data */
> 	nvmem-cell-parser-name = "onie-tlv-cells";
> 	reg = <0x56>;
>
> 	mac_address: mac-address {
> 		/* THIS REG is updated once TLV is parsed */
> 		reg = <0 0x6>
> 	};

I assume these cell nodes should be marked with some property that they
should be evaluated later, so the cell will be not read
in case it was not parsed because it may
exist in nvmem device optionally.

Or, treat cells with length "0" in a special way and allow to update
cell info later.

> };
>
> some_dev_node {
> 	compatible = "xxx";
> 	nvmem-cells = <&mac_address>;
> 	nvmem-cell-names = "mac-address";
> };
>
> == CODE ==
> ret = of_get_mac_address(dev->of_node, base_mac);
> ==========
>
>
> If you notice the mac_address node reg is 0.
> This node "reg" property should be updated ( using of_update_property()) 
> by nvmem-provider driver while tlv parsing and by matching the node name 
> with tlv name.
>

I assume parser driver can just invoke add_cell_table (with may be
by adding 'bool update' field to the cell_info struct) and core.c will just
update existing cells parsed from OF.

> That way rest of the code will work as usual.
>
> If this work for you then we can ask Rob if he foresee any issues in 
> this approach. I already see similar usage in reserved-memory case.
>
>
> --srini
>
>> 
>> 2) Regarding cell lookups registration, I had to use it because
>> of_nvmem_cell_get() will not find parser cells via OF.
>> 
>>>
>>>
>>>> 		status = "okay";
>>>> 	};
>>>> 	########
>>>>
>>>> 	== CODE ==
>>>> 	base_mac_np = of_parse_phandle(np, "base-mac-provider", 0);
>>>> 	ret = of_get_mac_address(base_mac_np, base_mac);
>>>> 	==========
>>>>
>>>>
>>>> And it works with this implementation because onie-tlv-cells is
>>>> registered as platform_device which name is the same as parser's name.
>>>> So the really tricky part for me is to make this cells lookup work.
>>>
>>> cell lookups are more of intended for board files, adding them in this
>>> case is really not correct.  The whole purpose of this driver is to
>>> parse the tlv cell infos into nvmem cell info.
>>>
>>>
>>> --srini
>>>
>>>
>>>>
>>>> Of course would be great if you can point a way/idea to get rid the need of
>>>> lookups.
>>>>
>> 


Regards,
Vadym Kochan

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 10:24             ` Vadym Kochan
@ 2021-09-20 10:36               ` Srinivas Kandagatla
  2021-09-20 11:25                 ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-20 10:36 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 20/09/2021 11:24, Vadym Kochan wrote:
>>>
>>> Probably this might be fixed by lookup nvmem device too ?
>> Honestly, this approach is a total hack to get it working.
>>
>> This is what Am thinking which should look like:
>>
>> ## DT ##
>> eeprom_at24: at24@56 {
>> 	compatible = "atmel,24c32";
>> 	/* Some way to identify that this is a TLV data */
>> 	nvmem-cell-parser-name = "onie-tlv-cells";
>> 	reg = <0x56>;
>>
>> 	mac_address: mac-address {
>> 		/* THIS REG is updated once TLV is parsed */
>> 		reg = <0 0x6>
>> 	};
> I assume these cell nodes should be marked with some property that they
> should be evaluated later, so the cell will be not read
> in case it was not parsed because it may
> exist in nvmem device optionally.
No, we should hit that use case to start with.

nvmem cells are parsed in the core during register path.
Parser should parse the cells before this to keep it simple.

> 
> Or, treat cells with length "0" in a special way and allow to update
> cell info later.you can update irrespective of the length, as long as this is done 
before register.


> 
>> };
>>
>> some_dev_node {
>> 	compatible = "xxx";
>> 	nvmem-cells = <&mac_address>;
>> 	nvmem-cell-names = "mac-address";
>> };
>>
>> == CODE ==
>> ret = of_get_mac_address(dev->of_node, base_mac);
>> ==========
>>
>>
>> If you notice the mac_address node reg is 0.
>> This node "reg" property should be updated ( using of_update_property())
>> by nvmem-provider driver while tlv parsing and by matching the node name
>> with tlv name.
>>
> I assume parser driver can just invoke add_cell_table (with may be
> by adding 'bool update' field to the cell_info struct) and core.c will just
> update existing cells parsed from OF.
> 

Lets keep the core code clean for now, I would expect the provider 
driver along with parser function to do update the nodes.

--srini

>> That way rest of the code will work as usual.
>>
>> If this work for you then we can ask Rob if he foresee any issues in
>> this approach. I already see similar usage in reserved-memory case.

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 10:36               ` Srinivas Kandagatla
@ 2021-09-20 11:25                 ` Vadym Kochan
  2021-09-20 11:32                   ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-20 11:25 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko


Just to clarify regarding the interfaces:

Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 20/09/2021 11:24, Vadym Kochan wrote:
>>>>
>>>> Probably this might be fixed by lookup nvmem device too ?
>>> Honestly, this approach is a total hack to get it working.
>>>
>>> This is what Am thinking which should look like:
>>>
>>> ## DT ##
>>> eeprom_at24: at24@56 {
>>> 	compatible = "atmel,24c32";
>>> 	/* Some way to identify that this is a TLV data */
>>> 	nvmem-cell-parser-name = "onie-tlv-cells";
>>> 	reg = <0x56>;
>>>
>>> 	mac_address: mac-address {
>>> 		/* THIS REG is updated once TLV is parsed */
>>> 		reg = <0 0x6>
>>> 	};
>> I assume these cell nodes should be marked with some property that they
>> should be evaluated later, so the cell will be not read
>> in case it was not parsed because it may
>> exist in nvmem device optionally.
> No, we should hit that use case to start with.
>
> nvmem cells are parsed in the core during register path.
> Parser should parse the cells before this to keep it simple.
>
>> 
>> Or, treat cells with length "0" in a special way and allow to update
>> cell info later.you can update irrespective of the length, as long as this is done 
> before register.
>
>
>> 
>>> };
>>>
>>> some_dev_node {
>>> 	compatible = "xxx";
>>> 	nvmem-cells = <&mac_address>;
>>> 	nvmem-cell-names = "mac-address";
>>> };
>>>
>>> == CODE ==
>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>> ==========
>>>
>>>
>>> If you notice the mac_address node reg is 0.
>>> This node "reg" property should be updated ( using of_update_property())
>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>> with tlv name.
>>>
>> I assume parser driver can just invoke add_cell_table (with may be
>> by adding 'bool update' field to the cell_info struct) and core.c will just
>> update existing cells parsed from OF.
>> 
>
> Lets keep the core code clean for now, I would expect the provider 
> driver along with parser function to do update the nodes.
>
> --srini
>

core.c sequence:

1) after cells parsed from OF:

2) lookup the parser

3) parser->cells_parse(ctx, table)

3.a) update existing cells matched by name from table

4) parser->cells_clean(ctx, table)
/* to free cell's_info allocated by the parser driver */

as alternative way, I think it would be more generic to
provide nvmem-provider.h API to update the existing cell info,
however it makes sense only when cells were parsed
by the cell parser, in the other situations the
cell should be well defined.

with this approach the parser driver will be just called
via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
for each parsed cells.

>>> That way rest of the code will work as usual.
>>>
>>> If this work for you then we can ask Rob if he foresee any issues in
>>> this approach. I already see similar usage in reserved-memory case.


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 11:25                 ` Vadym Kochan
@ 2021-09-20 11:32                   ` Srinivas Kandagatla
  2021-09-20 12:29                     ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-20 11:32 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 20/09/2021 12:25, Vadym Kochan wrote:
>>> Or, treat cells with length "0" in a special way and allow to update
>>> cell info later.you can update irrespective of the length, as long as this is done
>> before register.
>>
>>
>>>> };
>>>>
>>>> some_dev_node {
>>>> 	compatible = "xxx";
>>>> 	nvmem-cells = <&mac_address>;
>>>> 	nvmem-cell-names = "mac-address";
>>>> };
>>>>
>>>> == CODE ==
>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>> ==========
>>>>
>>>>
>>>> If you notice the mac_address node reg is 0.
>>>> This node "reg" property should be updated ( using of_update_property())
>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>> with tlv name.
>>>>
>>> I assume parser driver can just invoke add_cell_table (with may be
>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>> update existing cells parsed from OF.
>>>
>> Lets keep the core code clean for now, I would expect the provider
>> driver along with parser function to do update the nodes.
>>
>> --srini
>>
> core.c sequence:
> 
> 1) after cells parsed from OF:
> 
> 2) lookup the parser
> 
> 3) parser->cells_parse(ctx, table)
> 
> 3.a) update existing cells matched by name from table
> 
> 4) parser->cells_clean(ctx, table)
> /* to free cell's_info allocated by the parser driver */
> 
> as alternative way, I think it would be more generic to
> provide nvmem-provider.h API to update the existing cell info,
> however it makes sense only when cells were parsed
> by the cell parser, in the other situations the
> cell should be well defined.
> 
> with this approach the parser driver will be just called
> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
> for each parsed cells.

TBH, This is an over kill.

In nvmem provider driver probe you can parse the tlv data and update the 
dt nodes before nvmem register.

rest of the code should just work as it is.

--srini


> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 11:32                   ` Srinivas Kandagatla
@ 2021-09-20 12:29                     ` Vadym Kochan
  2021-09-20 12:34                       ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-20 12:29 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko


Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>> Or, treat cells with length "0" in a special way and allow to update
>>>> cell info later.you can update irrespective of the length, as long as this is done
>>> before register.
>>>
>>>
>>>>> };
>>>>>
>>>>> some_dev_node {
>>>>> 	compatible = "xxx";
>>>>> 	nvmem-cells = <&mac_address>;
>>>>> 	nvmem-cell-names = "mac-address";
>>>>> };
>>>>>
>>>>> == CODE ==
>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>> ==========
>>>>>
>>>>>
>>>>> If you notice the mac_address node reg is 0.
>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>> with tlv name.
>>>>>
>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>> update existing cells parsed from OF.
>>>>
>>> Lets keep the core code clean for now, I would expect the provider
>>> driver along with parser function to do update the nodes.
>>>
>>> --srini
>>>
>> core.c sequence:
>> 
>> 1) after cells parsed from OF:
>> 
>> 2) lookup the parser
>> 
>> 3) parser->cells_parse(ctx, table)
>> 
>> 3.a) update existing cells matched by name from table
>> 
>> 4) parser->cells_clean(ctx, table)
>> /* to free cell's_info allocated by the parser driver */
>> 
>> as alternative way, I think it would be more generic to
>> provide nvmem-provider.h API to update the existing cell info,
>> however it makes sense only when cells were parsed
>> by the cell parser, in the other situations the
>> cell should be well defined.
>> 
>> with this approach the parser driver will be just called
>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>> for each parsed cells.
>
> TBH, This is an over kill.
>
> In nvmem provider driver probe you can parse the tlv data and update the 
> dt nodes before nvmem register.
>
> rest of the code should just work as it is.
>
> --srini

You mean to parse TLV in the particular nvmem provider driver (for
example in at24 driver) ? If so, then it will not allow to parse
this TLV format from the other kinds of nvmem.

>
>
>> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 12:29                     ` Vadym Kochan
@ 2021-09-20 12:34                       ` Srinivas Kandagatla
  2021-09-20 13:29                         ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-20 12:34 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 20/09/2021 13:29, Vadym Kochan wrote:
> 
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
> 
>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>> before register.
>>>>
>>>>
>>>>>> };
>>>>>>
>>>>>> some_dev_node {
>>>>>> 	compatible = "xxx";
>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>> };
>>>>>>
>>>>>> == CODE ==
>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>> ==========
>>>>>>
>>>>>>
>>>>>> If you notice the mac_address node reg is 0.
>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>> with tlv name.
>>>>>>
>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>> update existing cells parsed from OF.
>>>>>
>>>> Lets keep the core code clean for now, I would expect the provider
>>>> driver along with parser function to do update the nodes.
>>>>
>>>> --srini
>>>>
>>> core.c sequence:
>>>
>>> 1) after cells parsed from OF:
>>>
>>> 2) lookup the parser
>>>
>>> 3) parser->cells_parse(ctx, table)
>>>
>>> 3.a) update existing cells matched by name from table
>>>
>>> 4) parser->cells_clean(ctx, table)
>>> /* to free cell's_info allocated by the parser driver */
>>>
>>> as alternative way, I think it would be more generic to
>>> provide nvmem-provider.h API to update the existing cell info,
>>> however it makes sense only when cells were parsed
>>> by the cell parser, in the other situations the
>>> cell should be well defined.
>>>
>>> with this approach the parser driver will be just called
>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>> for each parsed cells.
>>
>> TBH, This is an over kill.
>>
>> In nvmem provider driver probe you can parse the tlv data and update the
>> dt nodes before nvmem register.
>>
>> rest of the code should just work as it is.
>>
>> --srini
> 
> You mean to parse TLV in the particular nvmem provider driver (for
> example in at24 driver) ? If so, then it will not allow to parse
> this TLV format from the other kinds of nvmem.

Why not?

Can you also tell us which other nvmem providers are you going to test 
this on?

As long as you represent this parsing function as some library function, 
I do not see any issue.
If any exported symbol is available for this then any nvmem provider 
could use it.

--srini


> 
>>
>>
>>>
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 12:34                       ` Srinivas Kandagatla
@ 2021-09-20 13:29                         ` Vadym Kochan
  2021-09-20 13:40                           ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-20 13:29 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, Rob Herring, linux-kernel, devicetree, Robert Marko


Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 20/09/2021 13:29, Vadym Kochan wrote:
>> 
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>> 
>>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>>> before register.
>>>>>
>>>>>
>>>>>>> };
>>>>>>>
>>>>>>> some_dev_node {
>>>>>>> 	compatible = "xxx";
>>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>>> };
>>>>>>>
>>>>>>> == CODE ==
>>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>>> ==========
>>>>>>>
>>>>>>>
>>>>>>> If you notice the mac_address node reg is 0.
>>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>>> with tlv name.
>>>>>>>
>>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>>> update existing cells parsed from OF.
>>>>>>
>>>>> Lets keep the core code clean for now, I would expect the provider
>>>>> driver along with parser function to do update the nodes.
>>>>>
>>>>> --srini
>>>>>
>>>> core.c sequence:
>>>>
>>>> 1) after cells parsed from OF:
>>>>
>>>> 2) lookup the parser
>>>>
>>>> 3) parser->cells_parse(ctx, table)
>>>>
>>>> 3.a) update existing cells matched by name from table
>>>>
>>>> 4) parser->cells_clean(ctx, table)
>>>> /* to free cell's_info allocated by the parser driver */
>>>>
>>>> as alternative way, I think it would be more generic to
>>>> provide nvmem-provider.h API to update the existing cell info,
>>>> however it makes sense only when cells were parsed
>>>> by the cell parser, in the other situations the
>>>> cell should be well defined.
>>>>
>>>> with this approach the parser driver will be just called
>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>>> for each parsed cells.
>>>
>>> TBH, This is an over kill.
>>>
>>> In nvmem provider driver probe you can parse the tlv data and update the
>>> dt nodes before nvmem register.
>>>
>>> rest of the code should just work as it is.
>>>
>>> --srini
>> 
>> You mean to parse TLV in the particular nvmem provider driver (for
>> example in at24 driver) ? If so, then it will not allow to parse
>> this TLV format from the other kinds of nvmem.
>
> Why not?
>

Well, I think that nvmem driver and TLV parsing should somehow be
de-coupled from each other like block devices and FS does. BUT,
in case this TLV data will be used only on at24 devices then
may be it is OK.

> Can you also tell us which other nvmem providers are you going to test 
> this on?
>

Currently I can test only on at24 devices. From the:

https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html

"
Each ONIE system must include non-volatile storage which contains vital
product data assigned by the manufacturer. The non-volatile storage
could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
sector or any other non-volatile storage medium.
"

I am not aware about other nvmem devices which are used for existing
ONIE supported boards.

> As long as you represent this parsing function as some library function, 
> I do not see any issue.
> If any exported symbol is available for this then any nvmem provider 
> could use it.
>
> --srini
>
>
>> 
>>>
>>>
>>>>
>> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 13:29                         ` Vadym Kochan
@ 2021-09-20 13:40                           ` Srinivas Kandagatla
  2021-09-21  5:50                             ` John Thomson
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-20 13:40 UTC (permalink / raw)
  To: Vadym Kochan; +Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 20/09/2021 14:29, Vadym Kochan wrote:
> 
> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
> 
>> On 20/09/2021 13:29, Vadym Kochan wrote:
>>>
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>>>
>>>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>>>> before register.
>>>>>>
>>>>>>
>>>>>>>> };
>>>>>>>>
>>>>>>>> some_dev_node {
>>>>>>>> 	compatible = "xxx";
>>>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>>>> };
>>>>>>>>
>>>>>>>> == CODE ==
>>>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>>>> ==========
>>>>>>>>
>>>>>>>>
>>>>>>>> If you notice the mac_address node reg is 0.
>>>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>>>> with tlv name.
>>>>>>>>
>>>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>>>> update existing cells parsed from OF.
>>>>>>>
>>>>>> Lets keep the core code clean for now, I would expect the provider
>>>>>> driver along with parser function to do update the nodes.
>>>>>>
>>>>>> --srini
>>>>>>
>>>>> core.c sequence:
>>>>>
>>>>> 1) after cells parsed from OF:
>>>>>
>>>>> 2) lookup the parser
>>>>>
>>>>> 3) parser->cells_parse(ctx, table)
>>>>>
>>>>> 3.a) update existing cells matched by name from table
>>>>>
>>>>> 4) parser->cells_clean(ctx, table)
>>>>> /* to free cell's_info allocated by the parser driver */
>>>>>
>>>>> as alternative way, I think it would be more generic to
>>>>> provide nvmem-provider.h API to update the existing cell info,
>>>>> however it makes sense only when cells were parsed
>>>>> by the cell parser, in the other situations the
>>>>> cell should be well defined.
>>>>>
>>>>> with this approach the parser driver will be just called
>>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>>>> for each parsed cells.
>>>>
>>>> TBH, This is an over kill.
>>>>
>>>> In nvmem provider driver probe you can parse the tlv data and update the
>>>> dt nodes before nvmem register.
>>>>
>>>> rest of the code should just work as it is.
>>>>
>>>> --srini
>>>
>>> You mean to parse TLV in the particular nvmem provider driver (for
>>> example in at24 driver) ? If so, then it will not allow to parse
>>> this TLV format from the other kinds of nvmem.
>>
>> Why not?
>>
> 
> Well, I think that nvmem driver and TLV parsing should somehow be
> de-coupled from each other like block devices and FS does. BUT,
> in case this TLV data will be used only on at24 devices then
> may be it is OK.
> 

It has to be decoupled yes, which could be at any level with simple 
library function to a infrastructure level..

In this case with few or single users, doing with an additional 
infrastructure is a bit of over kill IMO.


--srini
>> Can you also tell us which other nvmem providers are you going to test
>> this on?
>>
> 
> Currently I can test only on at24 devices. From the:
> 
> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
> 
> "
> Each ONIE system must include non-volatile storage which contains vital
> product data assigned by the manufacturer. The non-volatile storage
> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
> sector or any other non-volatile storage medium.
> "
> 
> I am not aware about other nvmem devices which are used for existing
> ONIE supported boards.
> 
>> As long as you represent this parsing function as some library function,
>> I do not see any issue.
>> If any exported symbol is available for this then any nvmem provider
>> could use it.
>>
>> --srini
>>
>>
>>>
>>>>
>>>>
>>>>>
>>>
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-20 13:40                           ` Srinivas Kandagatla
@ 2021-09-21  5:50                             ` John Thomson
  2021-09-27  7:50                               ` Vadym Kochan
  2021-09-27 10:12                               ` Srinivas Kandagatla
  0 siblings, 2 replies; 36+ messages in thread
From: John Thomson @ 2021-09-21  5:50 UTC (permalink / raw)
  To: Srinivas Kandagatla, Vadym Kochan
  Cc: Rob Herring, linux-kernel, devicetree, Robert Marko

On Mon, 20 Sep 2021, at 13:40, Srinivas Kandagatla wrote:
> On 20/09/2021 14:29, Vadym Kochan wrote:
>> 
>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>> 
>>> On 20/09/2021 13:29, Vadym Kochan wrote:
>>>>
>>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>>>>
>>>>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>>>>> before register.
>>>>>>>
>>>>>>>
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> some_dev_node {
>>>>>>>>> 	compatible = "xxx";
>>>>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>>>>> };
>>>>>>>>>
>>>>>>>>> == CODE ==
>>>>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>>>>> ==========
>>>>>>>>>
>>>>>>>>>
>>>>>>>>> If you notice the mac_address node reg is 0.
>>>>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>>>>> with tlv name.
>>>>>>>>>
>>>>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>>>>> update existing cells parsed from OF.
>>>>>>>>
>>>>>>> Lets keep the core code clean for now, I would expect the provider
>>>>>>> driver along with parser function to do update the nodes.
>>>>>>>
>>>>>>> --srini
>>>>>>>
>>>>>> core.c sequence:
>>>>>>
>>>>>> 1) after cells parsed from OF:
>>>>>>
>>>>>> 2) lookup the parser
>>>>>>
>>>>>> 3) parser->cells_parse(ctx, table)
>>>>>>
>>>>>> 3.a) update existing cells matched by name from table
>>>>>>
>>>>>> 4) parser->cells_clean(ctx, table)
>>>>>> /* to free cell's_info allocated by the parser driver */
>>>>>>
>>>>>> as alternative way, I think it would be more generic to
>>>>>> provide nvmem-provider.h API to update the existing cell info,
>>>>>> however it makes sense only when cells were parsed
>>>>>> by the cell parser, in the other situations the
>>>>>> cell should be well defined.
>>>>>>
>>>>>> with this approach the parser driver will be just called
>>>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>>>>> for each parsed cells.
>>>>>
>>>>> TBH, This is an over kill.
>>>>>
>>>>> In nvmem provider driver probe you can parse the tlv data and update the
>>>>> dt nodes before nvmem register.
>>>>>
>>>>> rest of the code should just work as it is.
>>>>>
>>>>> --srini
>>>>
>>>> You mean to parse TLV in the particular nvmem provider driver (for
>>>> example in at24 driver) ? If so, then it will not allow to parse
>>>> this TLV format from the other kinds of nvmem.
>>>
>>> Why not?
>>>
>> 
>> Well, I think that nvmem driver and TLV parsing should somehow be
>> de-coupled from each other like block devices and FS does. BUT,
>> in case this TLV data will be used only on at24 devices then
>> may be it is OK.
>> 
>
> It has to be decoupled yes, which could be at any level with simple 
> library function to a infrastructure level..
>
> In this case with few or single users, doing with an additional 
> infrastructure is a bit of over kill IMO.
>
>
> --srini
>>> Can you also tell us which other nvmem providers are you going to test
>>> this on?
>>>
>> 
>> Currently I can test only on at24 devices. From the:
>> 
>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>> 
>> "
>> Each ONIE system must include non-volatile storage which contains vital
>> product data assigned by the manufacturer. The non-volatile storage
>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>> sector or any other non-volatile storage medium.
>> "
>> 
>> I am not aware about other nvmem devices which are used for existing
>> ONIE supported boards.
>> 
>>> As long as you represent this parsing function as some library function,
>>> I do not see any issue.
>>> If any exported symbol is available for this then any nvmem provider
>>> could use it.
>>>
>>> --srini
>>>

Hi Srinivas,

Can I note here that I would like to parse
TLV data from an SPI-NOR device to NVMEM cells.
The same general use case (getting mac-address from OEM data).

Was planning to base my work on this series, as well as
https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
(thanks for pointing that out Srinivas)

Cheers,
-- 
  John Thomson

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-21  5:50                             ` John Thomson
@ 2021-09-27  7:50                               ` Vadym Kochan
  2021-09-27 10:12                                 ` Srinivas Kandagatla
  2021-09-27 10:12                               ` Srinivas Kandagatla
  1 sibling, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-27  7:50 UTC (permalink / raw)
  To: John Thomson
  Cc: Srinivas Kandagatla, Vadym Kochan, Rob Herring, linux-kernel,
	devicetree, Robert Marko

Hi John, Srini,

John Thomson <john@johnthomson.fastmail.com.au> writes:

> On Mon, 20 Sep 2021, at 13:40, Srinivas Kandagatla wrote:
>> On 20/09/2021 14:29, Vadym Kochan wrote:
>>> 
>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>>> 
>>>> On 20/09/2021 13:29, Vadym Kochan wrote:
>>>>>
>>>>> Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:
>>>>>
>>>>>> On 20/09/2021 12:25, Vadym Kochan wrote:
>>>>>>>>> Or, treat cells with length "0" in a special way and allow to update
>>>>>>>>> cell info later.you can update irrespective of the length, as long as this is done
>>>>>>>> before register.
>>>>>>>>
>>>>>>>>
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> some_dev_node {
>>>>>>>>>> 	compatible = "xxx";
>>>>>>>>>> 	nvmem-cells = <&mac_address>;
>>>>>>>>>> 	nvmem-cell-names = "mac-address";
>>>>>>>>>> };
>>>>>>>>>>
>>>>>>>>>> == CODE ==
>>>>>>>>>> ret = of_get_mac_address(dev->of_node, base_mac);
>>>>>>>>>> ==========
>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> If you notice the mac_address node reg is 0.
>>>>>>>>>> This node "reg" property should be updated ( using of_update_property())
>>>>>>>>>> by nvmem-provider driver while tlv parsing and by matching the node name
>>>>>>>>>> with tlv name.
>>>>>>>>>>
>>>>>>>>> I assume parser driver can just invoke add_cell_table (with may be
>>>>>>>>> by adding 'bool update' field to the cell_info struct) and core.c will just
>>>>>>>>> update existing cells parsed from OF.
>>>>>>>>>
>>>>>>>> Lets keep the core code clean for now, I would expect the provider
>>>>>>>> driver along with parser function to do update the nodes.
>>>>>>>>
>>>>>>>> --srini
>>>>>>>>
>>>>>>> core.c sequence:
>>>>>>>
>>>>>>> 1) after cells parsed from OF:
>>>>>>>
>>>>>>> 2) lookup the parser
>>>>>>>
>>>>>>> 3) parser->cells_parse(ctx, table)
>>>>>>>
>>>>>>> 3.a) update existing cells matched by name from table
>>>>>>>
>>>>>>> 4) parser->cells_clean(ctx, table)
>>>>>>> /* to free cell's_info allocated by the parser driver */
>>>>>>>
>>>>>>> as alternative way, I think it would be more generic to
>>>>>>> provide nvmem-provider.h API to update the existing cell info,
>>>>>>> however it makes sense only when cells were parsed
>>>>>>> by the cell parser, in the other situations the
>>>>>>> cell should be well defined.
>>>>>>>
>>>>>>> with this approach the parser driver will be just called
>>>>>>> via parser->cells_parse(ctx) and will call nvmem_cell_info_update()
>>>>>>> for each parsed cells.
>>>>>>
>>>>>> TBH, This is an over kill.
>>>>>>
>>>>>> In nvmem provider driver probe you can parse the tlv data and update the
>>>>>> dt nodes before nvmem register.
>>>>>>
>>>>>> rest of the code should just work as it is.
>>>>>>
>>>>>> --srini
>>>>>
>>>>> You mean to parse TLV in the particular nvmem provider driver (for
>>>>> example in at24 driver) ? If so, then it will not allow to parse
>>>>> this TLV format from the other kinds of nvmem.
>>>>
>>>> Why not?
>>>>
>>> 
>>> Well, I think that nvmem driver and TLV parsing should somehow be
>>> de-coupled from each other like block devices and FS does. BUT,
>>> in case this TLV data will be used only on at24 devices then
>>> may be it is OK.
>>> 
>>
>> It has to be decoupled yes, which could be at any level with simple 
>> library function to a infrastructure level..
>>
>> In this case with few or single users, doing with an additional 
>> infrastructure is a bit of over kill IMO.
>>
>>
>> --srini
>>>> Can you also tell us which other nvmem providers are you going to test
>>>> this on?
>>>>
>>> 
>>> Currently I can test only on at24 devices. From the:
>>> 
>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>>> 
>>> "
>>> Each ONIE system must include non-volatile storage which contains vital
>>> product data assigned by the manufacturer. The non-volatile storage
>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>>> sector or any other non-volatile storage medium.
>>> "
>>> 
>>> I am not aware about other nvmem devices which are used for existing
>>> ONIE supported boards.
>>> 
>>>> As long as you represent this parsing function as some library function,
>>>> I do not see any issue.
>>>> If any exported symbol is available for this then any nvmem provider
>>>> could use it.
>>>>
>>>> --srini
>>>>
>
> Hi Srinivas,
>
> Can I note here that I would like to parse
> TLV data from an SPI-NOR device to NVMEM cells.
> The same general use case (getting mac-address from OEM data).
>
> Was planning to base my work on this series, as well as
> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
> (thanks for pointing that out Srinivas)
>
> Cheers,

What about at least to have just one call in core.c to make it a bit
de-coupled, like:

core.c

struct nvmem_device *nvmem_register(const struct nvmem_config *config)
{
...
         rval = nvmem_add_cells_from_table(nvmem);
         if (rval)
                 goto err_remove_cells;

+        rval = nvmem_parse_cells(nvmem, of);
+        if (rval) {
+        /* err handling */
+        }
+
         rval = nvmem_add_cells_from_of(nvmem);
         if (rval)
                 goto err_remove_cells;

         blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);

         return nvmem;

...

}

somewhere in nvmem-parser.c:

/* retreive parser name from of_node and call appropriate function to parse
   non-fixed cells and update via of_update */
int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node)
{
    ...
}

?

Regards,
Vadym Kochan

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-27  7:50                               ` Vadym Kochan
@ 2021-09-27 10:12                                 ` Srinivas Kandagatla
  2021-09-28 13:31                                   ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-27 10:12 UTC (permalink / raw)
  To: Vadym Kochan, John Thomson
  Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 27/09/2021 08:50, Vadym Kochan wrote:
>>>> Currently I can test only on at24 devices. From the:
>>>>
>>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>>>>
>>>> "
>>>> Each ONIE system must include non-volatile storage which contains vital
>>>> product data assigned by the manufacturer. The non-volatile storage
>>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>>>> sector or any other non-volatile storage medium.
>>>> "
>>>>
>>>> I am not aware about other nvmem devices which are used for existing
>>>> ONIE supported boards.
>>>>
>>>>> As long as you represent this parsing function as some library function,
>>>>> I do not see any issue.
>>>>> If any exported symbol is available for this then any nvmem provider
>>>>> could use it.
>>>>>
>>>>> --srini
>>>>>
>> Hi Srinivas,
>>
>> Can I note here that I would like to parse
>> TLV data from an SPI-NOR device to NVMEM cells.
>> The same general use case (getting mac-address from OEM data).
>>
>> Was planning to base my work on this series, as well as
>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>> (thanks for pointing that out Srinivas)
>>
>> Cheers,
> What about at least to have just one call in core.c to make it a bit
> de-coupled, like:

Why do you want to decouple this? the provider driver should be very 
well aware of the format the data layout.

Its fine to an extent to adding parse_cells() callback in nvmem_config.

> 
> core.c
> 
> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> {
> ...
>           rval = nvmem_add_cells_from_table(nvmem);
>           if (rval)
>                   goto err_remove_cells;
> 
> +        rval = nvmem_parse_cells(nvmem, of);
> +        if (rval) {
> +        /* err handling */
> +        }
> +
>           rval = nvmem_add_cells_from_of(nvmem);
>           if (rval)
>                   goto err_remove_cells;
> 
>           blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
> 
>           return nvmem;
> 
> ...
> 
> }
> 

> somewhere in nvmem-parser.c:

However this is totally over kill.

> 
> /* retreive parser name from of_node and call appropriate function to parse
>     non-fixed cells and update via of_update */

This is completely provider drivers job, nothing nvmem core should worry 
about.

If you have concern of having code duplicated then we could make some of 
the common functions as library functions, But it still is within the 
scope of provider drivers.

--srini

> int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node)
> {
>      ...
> }
> 
> ?


> 
> Regards,

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-21  5:50                             ` John Thomson
  2021-09-27  7:50                               ` Vadym Kochan
@ 2021-09-27 10:12                               ` Srinivas Kandagatla
  2021-09-27 12:38                                 ` John Thomson
  1 sibling, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-27 10:12 UTC (permalink / raw)
  To: John Thomson, Vadym Kochan
  Cc: Rob Herring, linux-kernel, devicetree, Robert Marko



On 21/09/2021 06:50, John Thomson wrote:
>>> Currently I can test only on at24 devices. From the:
>>>
>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>>>
>>> "
>>> Each ONIE system must include non-volatile storage which contains vital
>>> product data assigned by the manufacturer. The non-volatile storage
>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>>> sector or any other non-volatile storage medium.
>>> "
>>>
>>> I am not aware about other nvmem devices which are used for existing
>>> ONIE supported boards.
>>>
>>>> As long as you represent this parsing function as some library function,
>>>> I do not see any issue.
>>>> If any exported symbol is available for this then any nvmem provider
>>>> could use it.
>>>>
>>>> --srini
>>>>
> Hi Srinivas,
> 
> Can I note here that I would like to parse
> TLV data from an SPI-NOR device to NVMEM cells.
> The same general use case (getting mac-address from OEM data).
> 
> Was planning to base my work on this series, as well as
> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/

This series is for post-processing nvmem cell data before it gets to 
consumers.
Are you referring to parsing nvmem cell information (offset, name) in 
your usecase like: 
https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html

Or
Are you referring to post-processing nvmem cell data ?


--srini
> (thanks for pointing that out Srinivas)
> 
> Cheers,
> -- John Thomson

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-27 10:12                               ` Srinivas Kandagatla
@ 2021-09-27 12:38                                 ` John Thomson
  0 siblings, 0 replies; 36+ messages in thread
From: John Thomson @ 2021-09-27 12:38 UTC (permalink / raw)
  To: Srinivas Kandagatla, Vadym Kochan
  Cc: Rob Herring, linux-kernel, devicetree, Robert Marko

Hi Srinivas,

On Mon, 27 Sep 2021, at 10:12, Srinivas Kandagatla wrote:
> On 21/09/2021 06:50, John Thomson wrote:
>> Hi Srinivas,
>> 
>> Can I note here that I would like to parse
>> TLV data from an SPI-NOR device to NVMEM cells.
>> The same general use case (getting mac-address from OEM data).
>> 
>> Was planning to base my work on this series, as well as
>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>
> This series is for post-processing nvmem cell data before it gets to 
> consumers.
> Are you referring to parsing nvmem cell information (offset, name) in 
> your usecase like: 
> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>
> Or
> Are you referring to post-processing nvmem cell data ?

Both.
I have TLV data where I want to parse the tag lengths and tag IDs to map them into offsets and names like a NVMEM cells lookups table.

Then, some of these cell data would need to be post processed to use directly.
I have only a base MAC address available, and would like to specify increments for different network ports.
As an additional example, another TLV tag:value has compressed wifi calibration data. If I could post process this cell data I could then feed it into ath9k with: https://lore.kernel.org/all/f9b732b50a3453fadf3923cc75d365bae3505fe7.1630157099.git.chunkeey@gmail.com/

In my case, this is all currently done in userspace.
I saw this (ONIE TLV NVMEM parser) series and thought it offered something very similar to
(the first part of) what I would like to do.

Cheers,
-- 
  John Thomson

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-27 10:12                                 ` Srinivas Kandagatla
@ 2021-09-28 13:31                                   ` Vadym Kochan
  2021-09-28 13:51                                     ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-28 13:31 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, John Thomson, Rob Herring, linux-kernel,
	devicetree, Robert Marko


Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 27/09/2021 08:50, Vadym Kochan wrote:
>>>>> Currently I can test only on at24 devices. From the:
>>>>>
>>>>> https://opencomputeproject.github.io/onie/design-spec/hw_requirements.html
>>>>>
>>>>> "
>>>>> Each ONIE system must include non-volatile storage which contains vital
>>>>> product data assigned by the manufacturer. The non-volatile storage
>>>>> could take the form of an EEPROM, a NOR-flash sector, a NAND-flash
>>>>> sector or any other non-volatile storage medium.
>>>>> "
>>>>>
>>>>> I am not aware about other nvmem devices which are used for existing
>>>>> ONIE supported boards.
>>>>>
>>>>>> As long as you represent this parsing function as some library function,
>>>>>> I do not see any issue.
>>>>>> If any exported symbol is available for this then any nvmem provider
>>>>>> could use it.
>>>>>>
>>>>>> --srini
>>>>>>
>>> Hi Srinivas,
>>>
>>> Can I note here that I would like to parse
>>> TLV data from an SPI-NOR device to NVMEM cells.
>>> The same general use case (getting mac-address from OEM data).
>>>
>>> Was planning to base my work on this series, as well as
>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>>> (thanks for pointing that out Srinivas)
>>>
>>> Cheers,
>> What about at least to have just one call in core.c to make it a bit
>> de-coupled, like:
>
> Why do you want to decouple this? the provider driver should be very 
> well aware of the format the data layout.
>

In my understanding nvmem device should not aware about the data layout
(in case it does not rely on device's specific characteristics). Same
cells layout (TLV, etc) might exist on other nvmem devices.

> Its fine to an extent to adding parse_cells() callback in nvmem_config.
>

OK, in that case it will require small change in the core.

>> 
>> core.c
>> 
>> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>> {
>> ...
>>           rval = nvmem_add_cells_from_table(nvmem);
>>           if (rval)
>>                   goto err_remove_cells;
>> 
>> +        rval = nvmem_parse_cells(nvmem, of);
>> +        if (rval) {
>> +        /* err handling */
>> +        }
>> +
>>           rval = nvmem_add_cells_from_of(nvmem);
>>           if (rval)
>>                   goto err_remove_cells;
>> 
>>           blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>> 
>>           return nvmem;
>> 
>> ...
>> 
>> }
>> 
>
>> somewhere in nvmem-parser.c:
>
> However this is totally over kill.
>
>> 
>> /* retreive parser name from of_node and call appropriate function to parse
>>     non-fixed cells and update via of_update */
>
> This is completely provider drivers job, nothing nvmem core should worry 
> about.
>
> If you have concern of having code duplicated then we could make some of 
> the common functions as library functions, But it still is within the 
> scope of provider drivers.
>

Do I understand correctly that this parser function should be exported
from at24.c (in case of ONIE) and not from a separate C module ? Or
it just means that if there will be more users of this parsing function
then it might be moved to separate C module ?

> --srini
>

BTW, what if such change will be declined by particular nvmem driver
maintainer ?

>> int nvmem_parse_cells(struct nvmem_device *nvmem, struct device_node *of_node)
>> {
>>      ...
>> }
>> 
>> ?
>
>
>> 
>> Regards,


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-28 13:31                                   ` Vadym Kochan
@ 2021-09-28 13:51                                     ` Srinivas Kandagatla
  2021-09-28 14:11                                       ` Vadym Kochan
  0 siblings, 1 reply; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-28 13:51 UTC (permalink / raw)
  To: Vadym Kochan
  Cc: John Thomson, Rob Herring, linux-kernel, devicetree, Robert Marko



On 28/09/2021 14:31, Vadym Kochan wrote:
>>>> Can I note here that I would like to parse
>>>> TLV data from an SPI-NOR device to NVMEM cells.
>>>> The same general use case (getting mac-address from OEM data).
>>>>
>>>> Was planning to base my work on this series, as well as
>>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>>>> (thanks for pointing that out Srinivas)
>>>>
>>>> Cheers,
>>> What about at least to have just one call in core.c to make it a bit
>>> de-coupled, like:
>> Why do you want to decouple this? the provider driver should be very
>> well aware of the format the data layout.
>>
> In my understanding nvmem device should not aware about the data layout
> (in case it does not rely on device's specific characteristics). Same
> cells layout (TLV, etc) might exist on other nvmem devices.
> 
How would provider driver parse this without even knowing data layout?


>> Its fine to an extent to adding parse_cells() callback in nvmem_config.
>>
> OK, in that case it will require small change in the core.
> 
>>> core.c
>>>
>>> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>> {
>>> ...
>>>            rval = nvmem_add_cells_from_table(nvmem);
>>>            if (rval)
>>>                    goto err_remove_cells;
>>>
>>> +        rval = nvmem_parse_cells(nvmem, of);
>>> +        if (rval) {
>>> +        /* err handling */
>>> +        }
>>> +
>>>            rval = nvmem_add_cells_from_of(nvmem);
>>>            if (rval)
>>>                    goto err_remove_cells;
>>>
>>>            blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>>>
>>>            return nvmem;
>>>
>>> ...
>>>
>>> }
>>>
>>> somewhere in nvmem-parser.c:
>> However this is totally over kill.
>>
>>> /* retreive parser name from of_node and call appropriate function to parse
>>>      non-fixed cells and update via of_update */
>> This is completely provider drivers job, nothing nvmem core should worry
>> about.
>>
>> If you have concern of having code duplicated then we could make some of
>> the common functions as library functions, But it still is within the
>> scope of provider drivers.
>>
> Do I understand correctly that this parser function should be exported
> from at24.c (in case of ONIE) and not from a separate C module ? Or
> it just means that if there will be more users of this parsing function
> then it might be moved to separate C module ?
yes.
For now am not really sure how many users are for such parsing function.

> 
>> --srini
>>
> BTW, what if such change will be declined by particular nvmem driver
> maintainer ?

You would need some changes to provider driver to be able to flag that 
there is some kind of parsing required anyway.

--srini
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-28 13:51                                     ` Srinivas Kandagatla
@ 2021-09-28 14:11                                       ` Vadym Kochan
  2021-09-28 14:39                                         ` Srinivas Kandagatla
  0 siblings, 1 reply; 36+ messages in thread
From: Vadym Kochan @ 2021-09-28 14:11 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Vadym Kochan, John Thomson, Rob Herring, linux-kernel,
	devicetree, Robert Marko


Srinivas Kandagatla <srinivas.kandagatla@linaro.org> writes:

> On 28/09/2021 14:31, Vadym Kochan wrote:
>>>>> Can I note here that I would like to parse
>>>>> TLV data from an SPI-NOR device to NVMEM cells.
>>>>> The same general use case (getting mac-address from OEM data).
>>>>>
>>>>> Was planning to base my work on this series, as well as
>>>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>>>>> (thanks for pointing that out Srinivas)
>>>>>
>>>>> Cheers,
>>>> What about at least to have just one call in core.c to make it a bit
>>>> de-coupled, like:
>>> Why do you want to decouple this? the provider driver should be very
>>> well aware of the format the data layout.
>>>
>> In my understanding nvmem device should not aware about the data layout
>> (in case it does not rely on device's specific characteristics). Same
>> cells layout (TLV, etc) might exist on other nvmem devices.
>> 
> How would provider driver parse this without even knowing data layout?
>
>
>>> Its fine to an extent to adding parse_cells() callback in nvmem_config.
>>>
>> OK, in that case it will require small change in the core.
>> 
>>>> core.c
>>>>
>>>> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>>> {
>>>> ...
>>>>            rval = nvmem_add_cells_from_table(nvmem);
>>>>            if (rval)
>>>>                    goto err_remove_cells;
>>>>
>>>> +        rval = nvmem_parse_cells(nvmem, of);
>>>> +        if (rval) {
>>>> +        /* err handling */
>>>> +        }
>>>> +
>>>>            rval = nvmem_add_cells_from_of(nvmem);
>>>>            if (rval)
>>>>                    goto err_remove_cells;
>>>>
>>>>            blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>>>>
>>>>            return nvmem;
>>>>
>>>> ...
>>>>
>>>> }
>>>>
>>>> somewhere in nvmem-parser.c:
>>> However this is totally over kill.
>>>
>>>> /* retreive parser name from of_node and call appropriate function to parse
>>>>      non-fixed cells and update via of_update */
>>> This is completely provider drivers job, nothing nvmem core should worry
>>> about.
>>>
>>> If you have concern of having code duplicated then we could make some of
>>> the common functions as library functions, But it still is within the
>>> scope of provider drivers.
>>>
>> Do I understand correctly that this parser function should be exported
>> from at24.c (in case of ONIE) and not from a separate C module ? Or
>> it just means that if there will be more users of this parsing function
>> then it might be moved to separate C module ?
> yes.
> For now am not really sure how many users are for such parsing function.
>
>> 
>>> --srini
>>>
>> BTW, what if such change will be declined by particular nvmem driver
>> maintainer ?
>
> You would need some changes to provider driver to be able to flag that 
> there is some kind of parsing required anyway.
>

It might be some new property in device-tree which can be used also
for the other providers.

> --srini
>> 


^ permalink raw reply	[flat|nested] 36+ messages in thread

* Re: [PATCH v2 1/3] nvmem: core: introduce cells parser
  2021-09-28 14:11                                       ` Vadym Kochan
@ 2021-09-28 14:39                                         ` Srinivas Kandagatla
  0 siblings, 0 replies; 36+ messages in thread
From: Srinivas Kandagatla @ 2021-09-28 14:39 UTC (permalink / raw)
  To: Vadym Kochan
  Cc: John Thomson, Rob Herring, linux-kernel, devicetree, Robert Marko



On 28/09/2021 15:11, Vadym Kochan wrote:
> Srinivas Kandagatla<srinivas.kandagatla@linaro.org>  writes:
> 
>> On 28/09/2021 14:31, Vadym Kochan wrote:
>>>>>> Can I note here that I would like to parse
>>>>>> TLV data from an SPI-NOR device to NVMEM cells.
>>>>>> The same general use case (getting mac-address from OEM data).
>>>>>>
>>>>>> Was planning to base my work on this series, as well as
>>>>>> https://lore.kernel.org/all/20210908100257.17833-1-qiangqing.zhang@nxp.com/
>>>>>> (thanks for pointing that out Srinivas)
>>>>>>
>>>>>> Cheers,
>>>>> What about at least to have just one call in core.c to make it a bit
>>>>> de-coupled, like:
>>>> Why do you want to decouple this? the provider driver should be very
>>>> well aware of the format the data layout.
>>>>
>>> In my understanding nvmem device should not aware about the data layout
>>> (in case it does not rely on device's specific characteristics). Same
>>> cells layout (TLV, etc) might exist on other nvmem devices.
>>>
>> How would provider driver parse this without even knowing data layout?
>>
>>
>>>> Its fine to an extent to adding parse_cells() callback in nvmem_config.
>>>>
>>> OK, in that case it will require small change in the core.
>>>
>>>>> core.c
>>>>>
>>>>> struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>>>>> {
>>>>> ...
>>>>>             rval = nvmem_add_cells_from_table(nvmem);
>>>>>             if (rval)
>>>>>                     goto err_remove_cells;
>>>>>
>>>>> +        rval = nvmem_parse_cells(nvmem, of);
>>>>> +        if (rval) {
>>>>> +        /* err handling */
>>>>> +        }
>>>>> +
>>>>>             rval = nvmem_add_cells_from_of(nvmem);
>>>>>             if (rval)
>>>>>                     goto err_remove_cells;
>>>>>
>>>>>             blocking_notifier_call_chain(&nvmem_notifier, NVMEM_ADD, nvmem);
>>>>>
>>>>>             return nvmem;
>>>>>
>>>>> ...
>>>>>
>>>>> }
>>>>>
>>>>> somewhere in nvmem-parser.c:
>>>> However this is totally over kill.
>>>>
>>>>> /* retreive parser name from of_node and call appropriate function to parse
>>>>>       non-fixed cells and update via of_update */
>>>> This is completely provider drivers job, nothing nvmem core should worry
>>>> about.
>>>>
>>>> If you have concern of having code duplicated then we could make some of
>>>> the common functions as library functions, But it still is within the
>>>> scope of provider drivers.
>>>>
>>> Do I understand correctly that this parser function should be exported
>>> from at24.c (in case of ONIE) and not from a separate C module ? Or
>>> it just means that if there will be more users of this parsing function
>>> then it might be moved to separate C module ?
>> yes.
>> For now am not really sure how many users are for such parsing function.
>>
>>>> --srini
>>>>
>>> BTW, what if such change will be declined by particular nvmem driver
>>> maintainer ?
>> You would need some changes to provider driver to be able to flag that
>> there is some kind of parsing required anyway.
>>
> It might be some new property in device-tree which can be used also
> for the other providers.

But this new binding will still belong to the provider driver that you 
want to support parsing.


--srini
> 

^ permalink raw reply	[flat|nested] 36+ messages in thread

end of thread, other threads:[~2021-09-28 14:40 UTC | newest]

Thread overview: 36+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-08 19:03 [PATCH v2 0/3] nvmem: add ONIE NVMEM cells parser Vadym Kochan
2021-06-08 19:03 ` [PATCH v2 1/3] nvmem: core: introduce " Vadym Kochan
2021-06-08 22:49   ` kernel test robot
2021-06-08 22:49     ` kernel test robot
2021-06-09  3:05   ` kernel test robot
2021-06-09  3:05     ` kernel test robot
2021-06-14 10:44   ` Srinivas Kandagatla
2021-06-16 12:33     ` Vadym Kochan
2021-06-21 11:00       ` Srinivas Kandagatla
2021-09-08  9:38         ` Vadym Kochan
2021-09-13 14:19           ` Srinivas Kandagatla
2021-09-20 10:24             ` Vadym Kochan
2021-09-20 10:36               ` Srinivas Kandagatla
2021-09-20 11:25                 ` Vadym Kochan
2021-09-20 11:32                   ` Srinivas Kandagatla
2021-09-20 12:29                     ` Vadym Kochan
2021-09-20 12:34                       ` Srinivas Kandagatla
2021-09-20 13:29                         ` Vadym Kochan
2021-09-20 13:40                           ` Srinivas Kandagatla
2021-09-21  5:50                             ` John Thomson
2021-09-27  7:50                               ` Vadym Kochan
2021-09-27 10:12                                 ` Srinivas Kandagatla
2021-09-28 13:31                                   ` Vadym Kochan
2021-09-28 13:51                                     ` Srinivas Kandagatla
2021-09-28 14:11                                       ` Vadym Kochan
2021-09-28 14:39                                         ` Srinivas Kandagatla
2021-09-27 10:12                               ` Srinivas Kandagatla
2021-09-27 12:38                                 ` John Thomson
2021-09-08  9:44     ` Vadym Kochan
2021-06-08 19:03 ` [PATCH v2 2/3] dt-bindings: nvmem: document nvmem-cells-parser-name property Vadym Kochan
2021-06-18 20:59   ` Rob Herring
2021-06-08 19:03 ` [PATCH v2 3/3] nvmem: add ONIE nvmem cells parser Vadym Kochan
2021-08-06 15:39   ` Jan Lübbe
2021-09-08  9:56     ` Vadym Kochan
2021-09-12 21:06       ` John Thomson
2021-09-13 14:20         ` Srinivas Kandagatla

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.