[PATCH v3 0/3] nvmem: add ONIE NVMEM cells provider

[PATCH v3 0/3] nvmem: add ONIE NVMEM cells provider

[Vadym Kochan]

This series adds cells provider 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.

There is a special case for this driver that before registering cells
the nvmem device should be first parsed to read the TLV layout which
holds the cells data.

v3:
    1) Update core.c changes by extending notification mechanism
       by adding new NVMEM_PRE_ADD event id which is called before lookup
       table cells binding, this allows for notification handler to
       register cells which require nvmem parsing.

    2) Update onie-cells.c to use nvmem notification to parse and
       register cells before device and cells are finally registered.

       Use subsys_init() macro which allows to probe before nvmem drivers.

    3) at24 driver enables regulator and pm state machine after nvmem
       registration which does not allow to use it on handing NVMEM_PRE_ADD event.

v2:
    1) Fixed wrong memcmp comparison

Vadym Kochan (3):
  nvmem: core: allow to register cells during nvmem registration
  nvmem: add ONIE NVMEM cells support
  misc: eeprom: at24: register nvmem only after eeprom is ready to use

 drivers/misc/eeprom/at24.c     |  11 +-
 drivers/nvmem/Kconfig          |   9 +
 drivers/nvmem/Makefile         |   3 +
 drivers/nvmem/core.c           |   2 +
 drivers/nvmem/onie-cells.c     | 410 +++++++++++++++++++++++++++++++++
 include/linux/nvmem-consumer.h |   1 +
 6 files changed, 432 insertions(+), 4 deletions(-)
 create mode 100644 drivers/nvmem/onie-cells.c

-- 
2.17.1

[PATCH v3 1/3] nvmem: core: allow to register cells during nvmem registration

[Vadym Kochan]

Add NVMEM_PRE_ADD notification step which is called before any cells
binding - from lookup table or config, this allows to register cells
in some specific layout (tlv) which should be parsed first and then
registered. So there might be a cell parser driver which can register
lookup table during this notification step.

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
v3:
    1) Update core.c changes by extending notification mechanism
       by adding new NVMEM_PRE_ADD event id which is called before lookup
       table cells binding, this allows for notification handler to
       register cells which require nvmem parsing.

 drivers/nvmem/core.c           | 2 ++
 include/linux/nvmem-consumer.h | 1 +
 2 files changed, 3 insertions(+)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 6cd3edb2eaf6..c48a69e0ebbe 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -668,6 +668,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
 			goto err_device_del;
 	}
 
+	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_PRE_ADD, nvmem);
+
 	if (config->cells) {
 		rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
 		if (rval)
diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
index 052293f4cbdb..0f7107276756 100644
--- a/include/linux/nvmem-consumer.h
+++ b/include/linux/nvmem-consumer.h
@@ -50,6 +50,7 @@ enum {
 	NVMEM_REMOVE,
 	NVMEM_CELL_ADD,
 	NVMEM_CELL_REMOVE,
+	NVMEM_PRE_ADD,
 };
 
 #if IS_ENABLED(CONFIG_NVMEM)
-- 
2.17.1

[PATCH v3 2/3] nvmem: add ONIE NVMEM cells support

[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>
---
v3:
    1) Update onie-cells.c by using nvmem notification to parse and
       register cells before device and cells are finally registered.

    2) Use subsys_init() macro which allows to probe before nvmem drivers.

 drivers/nvmem/Kconfig      |   9 +
 drivers/nvmem/Makefile     |   3 +
 drivers/nvmem/onie-cells.c | 410 +++++++++++++++++++++++++++++++++++++
 3 files changed, 422 insertions(+)
 create mode 100644 drivers/nvmem/onie-cells.c

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 954d3b4a52ab..922d753519f7 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -270,4 +270,13 @@ config SPRD_EFUSE
 	  This driver can also be built as a module. If so, the module
 	  will be called nvmem-sprd-efuse.
 
+config NVMEM_ONIE_CELLS
+	bool "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-cells.
+
 endif
diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
index a7c377218341..2199784a489f 100644
--- a/drivers/nvmem/Makefile
+++ b/drivers/nvmem/Makefile
@@ -55,3 +55,6 @@ obj-$(CONFIG_NVMEM_ZYNQMP)	+= nvmem_zynqmp_nvmem.o
 nvmem_zynqmp_nvmem-y		:= zynqmp_nvmem.o
 obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
 nvmem_sprd_efuse-y		:= sprd-efuse.o
+
+obj-$(CONFIG_NVMEM_ONIE_CELLS)	+= nvmem-onie-cells.o
+nvmem-onie-cells-y		:= onie-cells.o
diff --git a/drivers/nvmem/onie-cells.c b/drivers/nvmem/onie-cells.c
new file mode 100644
index 000000000000..bbb1e421f67c
--- /dev/null
+++ b/drivers/nvmem/onie-cells.c
@@ -0,0 +1,410 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * ONIE NVMEM cells provider
+ *
+ * Author: Vadym Kochan <vadym.kochan@plvision.eu>
+ */
+
+#define ONIE_NVMEM_DRVNAME	"onie-nvmem-cells"
+
+#define pr_fmt(fmt) ONIE_NVMEM_DRVNAME ": " fmt
+
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/init.h>
+#include <linux/kref.h>
+#include <linux/nvmem-consumer.h>
+#include <linux/nvmem-provider.h>
+#include <linux/of.h>
+#include <linux/of_device.h>
+#include <linux/platform_device.h>
+#include <linux/slab.h>
+
+#define ONIE_NVMEM_TLV_MAX_LEN	2048
+
+#define ONIE_NVMEM_HDR_ID	"TlvInfo"
+
+struct onie_nvmem_hdr {
+	u8 id[8];
+	u8 version;
+	__be16 data_len;
+} __packed;
+
+struct onie_nvmem_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_nvmem {
+	struct platform_device *pdev;
+	struct notifier_block nvmem_nb;
+	unsigned int attr_count;
+	struct list_head attrs;
+	struct kref refcnt;
+	const char *nvmem_match;
+
+	struct nvmem_cell_lookup *cell_lookup;
+	struct nvmem_cell_table cell_tbl;
+	struct nvmem_device *nvmem;
+};
+
+static bool onie_nvmem_hdr_is_valid(struct onie_nvmem_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_nvmem *onie)
+{
+	struct onie_nvmem_attr *attr, *tmp;
+
+	list_for_each_entry_safe(attr, tmp, &onie->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_nvmem *onie, u8 *data, u16 len)
+{
+	unsigned int hlen = sizeof(struct onie_nvmem_hdr);
+	unsigned int offset = 0;
+	int err;
+
+	while (offset < len) {
+		struct onie_nvmem_attr *attr;
+		struct onie_nvmem_tlv *tlv;
+
+		tlv = (struct onie_nvmem_tlv *)(data + offset);
+
+		if (offset + tlv->len >= len) {
+			struct nvmem_device *nvmem = onie->nvmem;
+
+			pr_err("%s: TLV len is too big(0x%x) at 0x%x\n",
+			       nvmem_dev_name(nvmem), 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, &onie->attrs);
+		onie->attr_count++;
+
+		offset += sizeof(*tlv) + tlv->len;
+	}
+
+	return 0;
+
+err_attr_alloc:
+	onie_nvmem_attrs_free(onie);
+
+	return err;
+}
+
+static int onie_nvmem_decode(struct onie_nvmem *onie)
+{
+	struct nvmem_device *nvmem = onie->nvmem;
+	struct onie_nvmem_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("%s: invalid ONIE TLV header\n", nvmem_dev_name(nvmem));
+		ret = -EINVAL;
+		goto err_invalid;
+	}
+
+	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) {
+		ret = -ENOMEM;
+		goto err_kmalloc;
+	}
+
+	ret = nvmem_device_read(nvmem, sizeof(hdr), len, data);
+	if (ret < 0)
+		goto err_data_read;
+
+	ret = onie_nvmem_tlv_parse(onie, data, len);
+	if (ret)
+		goto err_info_parse;
+
+	kfree(data);
+
+	return 0;
+
+err_info_parse:
+err_data_read:
+	kfree(data);
+err_kmalloc:
+err_invalid:
+	return ret;
+}
+
+static int onie_nvmem_cells_parse(struct onie_nvmem *onie)
+{
+	struct platform_device *pdev = onie->pdev;
+	struct nvmem_device *nvmem = onie->nvmem;
+	struct device *dev = &pdev->dev;
+	struct nvmem_cell_info *cells;
+	struct onie_nvmem_attr *attr;
+	unsigned int ncells = 0;
+	int err;
+
+	INIT_LIST_HEAD(&onie->attrs);
+	onie->attr_count = 0;
+
+	err = onie_nvmem_decode(onie);
+	if (err)
+		return err;
+
+	if (!onie->attr_count) {
+		pr_err("%s: has no ONIE attributes\n", nvmem_dev_name(nvmem));
+		return -EINVAL;
+	}
+
+	cells = kmalloc_array(onie->attr_count, sizeof(*cells), GFP_KERNEL);
+	if (!cells) {
+		err = -ENOMEM;
+		goto err_cells_alloc;
+	}
+
+	onie->cell_lookup = kmalloc_array(onie->attr_count,
+					  sizeof(struct nvmem_cell_lookup),
+					  GFP_KERNEL);
+	if (!onie->cell_lookup) {
+		err = -ENOMEM;
+		goto err_lookup_alloc;
+	}
+
+	list_for_each_entry(attr, &onie->attrs, head) {
+		struct nvmem_cell_lookup *lookup;
+		struct nvmem_cell_info *cell;
+
+		cell = &cells[ncells];
+
+		lookup = &onie->cell_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->nvmem_name = nvmem_dev_name(onie->nvmem);
+		lookup->dev_id = dev_name(dev);
+		lookup->cell_name = cell->name;
+		lookup->con_id = cell->name;
+
+		ncells++;
+	}
+
+	onie->cell_tbl.nvmem_name = nvmem_dev_name(onie->nvmem);
+	onie->cell_tbl.ncells = ncells;
+	onie->cell_tbl.cells = cells;
+
+	nvmem_add_cell_table(&onie->cell_tbl);
+	nvmem_add_cell_lookups(onie->cell_lookup, ncells);
+
+	onie_nvmem_attrs_free(onie);
+
+	return 0;
+
+err_lookup_alloc:
+	kfree(onie->cell_tbl.cells);
+err_cells_alloc:
+	onie_nvmem_attrs_free(onie);
+
+	return err;
+}
+
+static void onie_nvmem_release(struct kref *kref)
+{
+	kfree(container_of(kref, struct onie_nvmem, refcnt));
+}
+
+static void onie_nvmem_get(struct onie_nvmem *onie)
+{
+	kref_get(&onie->refcnt);
+}
+
+static void onie_nvmem_put(struct onie_nvmem *onie)
+{
+	kref_put(&onie->refcnt, onie_nvmem_release);
+}
+
+static int onie_nvmem_notify(struct notifier_block *nb,
+			     unsigned long val, void *data)
+{
+	struct nvmem_device *nvmem = data;
+	struct onie_nvmem *onie;
+	int err;
+
+	if (val != NVMEM_PRE_ADD && val != NVMEM_REMOVE)
+		return NOTIFY_DONE;
+
+	onie = container_of(nb, struct onie_nvmem, nvmem_nb);
+
+	if (strcmp(onie->nvmem_match, nvmem_dev_name(nvmem)) != 0)
+		return NOTIFY_DONE;
+
+	switch (val) {
+	case NVMEM_PRE_ADD:
+		onie->nvmem = nvmem;
+
+		err = onie_nvmem_cells_parse(onie);
+		if (err)
+			return NOTIFY_BAD;
+
+		onie_nvmem_get(onie);
+		break;
+
+	case NVMEM_REMOVE:
+		nvmem_del_cell_lookups(onie->cell_lookup, onie->attr_count);
+		nvmem_del_cell_table(&onie->cell_tbl);
+
+		kfree(onie->cell_tbl.cells);
+		kfree(onie->cell_lookup);
+
+		onie_nvmem_put(onie);
+		break;
+
+	default:
+		return NOTIFY_DONE;
+	}
+
+	return NOTIFY_OK;
+}
+
+static int onie_nvmem_probe(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct device_node *np = dev->of_node;
+	struct onie_nvmem *onie;
+	const char *nvmem_match;
+	int err;
+
+	err = of_property_read_string(np, "nvmem-name", &nvmem_match);
+	if (err) {
+		dev_err(dev, "error while parsing 'nvmem-name' property\n");
+		return err;
+	}
+
+	onie = kmalloc(sizeof(*onie), GFP_KERNEL);
+	if (!onie)
+		return -ENOMEM;
+
+	kref_init(&onie->refcnt);
+
+	onie->nvmem_nb.notifier_call = onie_nvmem_notify;
+	onie->nvmem_match = nvmem_match;
+	onie->pdev = pdev;
+
+	dev_set_drvdata(dev, onie);
+
+	return nvmem_register_notifier(&onie->nvmem_nb);
+}
+
+static int onie_nvmem_remove(struct platform_device *pdev)
+{
+	struct device *dev = &pdev->dev;
+	struct onie_nvmem *onie;
+
+	onie = dev_get_drvdata(dev);
+
+	nvmem_unregister_notifier(&onie->nvmem_nb);
+	onie_nvmem_put(onie);
+
+	return 0;
+}
+
+static const struct of_device_id onie_nvmem_match[] = {
+	{
+		.compatible = "onie,nvmem-cells",
+	},
+	{ }
+};
+MODULE_DEVICE_TABLE(of, onie_nvmem_match);
+
+static struct platform_driver onie_nvmem_driver = {
+	.probe = onie_nvmem_probe,
+	.remove = onie_nvmem_remove,
+	.driver = {
+		.name = ONIE_NVMEM_DRVNAME,
+		.of_match_table = onie_nvmem_match,
+	},
+};
+
+static int __init onie_nvmem_init(void)
+{
+	return platform_driver_register(&onie_nvmem_driver);
+}
+
+static void __exit onie_nvmem_exit(void)
+{
+	platform_driver_unregister(&onie_nvmem_driver);
+}
+
+subsys_initcall(onie_nvmem_init);
+module_exit(onie_nvmem_exit);
+
+MODULE_AUTHOR("Vadym Kochan <vadym.kochan@plvision.eu>");
+MODULE_DESCRIPTION("ONIE NVMEM cells driver");
+MODULE_LICENSE("GPL");
-- 
2.17.1

[PATCH v3 3/3] misc: eeprom: at24: register nvmem only after eeprom is ready to use

[Vadym Kochan]

During nvmem_register() the nvmem core sends notifications when:

    - cell added
    - nvmem added

and during these notifications some callback func may access the nvmem
device, which will fail in case of at24 eeprom because regulator and pm
are enabled after nvmem_register().

Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
---
v3:
    1) at24 driver enables regulator and pm state machine after nvmem
       registration which does not allow to use it on handing NVMEM_PRE_ADD event.

 drivers/misc/eeprom/at24.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
index 2591c21b2b5d..26a23abc053d 100644
--- a/drivers/misc/eeprom/at24.c
+++ b/drivers/misc/eeprom/at24.c
@@ -692,10 +692,6 @@ static int at24_probe(struct i2c_client *client)
 	nvmem_config.word_size = 1;
 	nvmem_config.size = byte_len;
 
-	at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
-	if (IS_ERR(at24->nvmem))
-		return PTR_ERR(at24->nvmem);
-
 	i2c_set_clientdata(client, at24);
 
 	err = regulator_enable(at24->vcc_reg);
@@ -708,6 +704,13 @@ static int at24_probe(struct i2c_client *client)
 	pm_runtime_set_active(dev);
 	pm_runtime_enable(dev);
 
+	at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
+	if (IS_ERR(at24->nvmem)) {
+		pm_runtime_disable(dev);
+		regulator_disable(at24->vcc_reg);
+		return PTR_ERR(at24->nvmem);
+	}
+
 	/*
 	 * Perform a one-byte test read to verify that the
 	 * chip is functional.
-- 
2.17.1

Re: [PATCH v3 3/3] misc: eeprom: at24: register nvmem only after eeprom is ready to use

[Bartosz Golaszewski]

On Mon, Aug 31, 2020 at 3:56 AM Vadym Kochan <vadym.kochan@plvision.eu> wrote:
>
> During nvmem_register() the nvmem core sends notifications when:
>
>     - cell added
>     - nvmem added
>
> and during these notifications some callback func may access the nvmem
> device, which will fail in case of at24 eeprom because regulator and pm
> are enabled after nvmem_register().
>
> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
> v3:
>     1) at24 driver enables regulator and pm state machine after nvmem
>        registration which does not allow to use it on handing NVMEM_PRE_ADD event.
>
>  drivers/misc/eeprom/at24.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2591c21b2b5d..26a23abc053d 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -692,10 +692,6 @@ static int at24_probe(struct i2c_client *client)
>         nvmem_config.word_size = 1;
>         nvmem_config.size = byte_len;
>
> -       at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
> -       if (IS_ERR(at24->nvmem))
> -               return PTR_ERR(at24->nvmem);
> -
>         i2c_set_clientdata(client, at24);
>
>         err = regulator_enable(at24->vcc_reg);
> @@ -708,6 +704,13 @@ static int at24_probe(struct i2c_client *client)
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>
> +       at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
> +       if (IS_ERR(at24->nvmem)) {
> +               pm_runtime_disable(dev);
> +               regulator_disable(at24->vcc_reg);
> +               return PTR_ERR(at24->nvmem);
> +       }
> +
>         /*
>          * Perform a one-byte test read to verify that the
>          * chip is functional.
> --
> 2.17.1
>

Good catch! Thanks for the patch. Can I queue it for fixes
independently from the earlier patches in this series?

Bart

Re: [PATCH v3 3/3] misc: eeprom: at24: register nvmem only after eeprom is ready to use

[Vadym Kochan]

Hi Bartosz,

On Mon, Aug 31, 2020 at 07:21:39PM +0200, Bartosz Golaszewski wrote:
> On Mon, Aug 31, 2020 at 3:56 AM Vadym Kochan <vadym.kochan@plvision.eu> wrote:
> >
> > During nvmem_register() the nvmem core sends notifications when:
> >
> >     - cell added
> >     - nvmem added
> >
> > and during these notifications some callback func may access the nvmem
> > device, which will fail in case of at24 eeprom because regulator and pm
> > are enabled after nvmem_register().
> >
> > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> > ---
> > v3:
> >     1) at24 driver enables regulator and pm state machine after nvmem
> >        registration which does not allow to use it on handing NVMEM_PRE_ADD event.
> >
> >  drivers/misc/eeprom/at24.c | 11 +++++++----
> >  1 file changed, 7 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> > index 2591c21b2b5d..26a23abc053d 100644
> > --- a/drivers/misc/eeprom/at24.c
> > +++ b/drivers/misc/eeprom/at24.c
> > @@ -692,10 +692,6 @@ static int at24_probe(struct i2c_client *client)
> >         nvmem_config.word_size = 1;
> >         nvmem_config.size = byte_len;
> >
> > -       at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
> > -       if (IS_ERR(at24->nvmem))
> > -               return PTR_ERR(at24->nvmem);
> > -
> >         i2c_set_clientdata(client, at24);
> >
> >         err = regulator_enable(at24->vcc_reg);
> > @@ -708,6 +704,13 @@ static int at24_probe(struct i2c_client *client)
> >         pm_runtime_set_active(dev);
> >         pm_runtime_enable(dev);
> >
> > +       at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
> > +       if (IS_ERR(at24->nvmem)) {
> > +               pm_runtime_disable(dev);
> > +               regulator_disable(at24->vcc_reg);
> > +               return PTR_ERR(at24->nvmem);
> > +       }
> > +
> >         /*
> >          * Perform a one-byte test read to verify that the
> >          * chip is functional.
> > --
> > 2.17.1
> >
> 
> Good catch! Thanks for the patch. Can I queue it for fixes
> independently from the earlier patches in this series?
> 
> Bart

Sure.

Regards,
Vadym Kochan

Re: [PATCH v3 3/3] misc: eeprom: at24: register nvmem only after eeprom is ready to use

[Bartosz Golaszewski]

On Mon, Aug 31, 2020 at 3:56 AM Vadym Kochan <vadym.kochan@plvision.eu> wrote:
>
> During nvmem_register() the nvmem core sends notifications when:
>
>     - cell added
>     - nvmem added
>
> and during these notifications some callback func may access the nvmem
> device, which will fail in case of at24 eeprom because regulator and pm
> are enabled after nvmem_register().
>
> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
> v3:
>     1) at24 driver enables regulator and pm state machine after nvmem
>        registration which does not allow to use it on handing NVMEM_PRE_ADD event.
>
>  drivers/misc/eeprom/at24.c | 11 +++++++----
>  1 file changed, 7 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/misc/eeprom/at24.c b/drivers/misc/eeprom/at24.c
> index 2591c21b2b5d..26a23abc053d 100644
> --- a/drivers/misc/eeprom/at24.c
> +++ b/drivers/misc/eeprom/at24.c
> @@ -692,10 +692,6 @@ static int at24_probe(struct i2c_client *client)
>         nvmem_config.word_size = 1;
>         nvmem_config.size = byte_len;
>
> -       at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
> -       if (IS_ERR(at24->nvmem))
> -               return PTR_ERR(at24->nvmem);
> -
>         i2c_set_clientdata(client, at24);
>
>         err = regulator_enable(at24->vcc_reg);
> @@ -708,6 +704,13 @@ static int at24_probe(struct i2c_client *client)
>         pm_runtime_set_active(dev);
>         pm_runtime_enable(dev);
>
> +       at24->nvmem = devm_nvmem_register(dev, &nvmem_config);
> +       if (IS_ERR(at24->nvmem)) {
> +               pm_runtime_disable(dev);
> +               regulator_disable(at24->vcc_reg);
> +               return PTR_ERR(at24->nvmem);
> +       }
> +
>         /*
>          * Perform a one-byte test read to verify that the
>          * chip is functional.
> --
> 2.17.1
>

Queued for fixes, Cc'ed stable and added Fixes: tag.

Thanks!
Bart

Re: [PATCH v3 1/3] nvmem: core: allow to register cells during nvmem registration

[Srinivas Kandagatla]

Hi Vadym,

Thanks for the patch,
On 31/08/2020 02:55, Vadym Kochan wrote:
> Add NVMEM_PRE_ADD notification step which is called before any cells
> binding - from lookup table or config, this allows to register cells
> in some specific layout (tlv) which should be parsed first and then
> registered. So there might be a cell parser driver which can register
> lookup table during this notification step.
> 
This is going in right direction but totally not correct way to do it.

1> this is not scalable as any consumer that will register for this even 
will have no idea of which what kind of parsing that provider needs.
It can work in your case but not really useful.

2> this is a consumer API, not the provider api.

How about adding a "parse_cells" callback in struct nvmem_config along 
with encoding type.


thanks,
srini

> Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> ---
> v3:
>      1) Update core.c changes by extending notification mechanism
>         by adding new NVMEM_PRE_ADD event id which is called before lookup
>         table cells binding, this allows for notification handler to
>         register cells which require nvmem parsing.
> 
>   drivers/nvmem/core.c           | 2 ++
>   include/linux/nvmem-consumer.h | 1 +
>   2 files changed, 3 insertions(+)
> 
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index 6cd3edb2eaf6..c48a69e0ebbe 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -668,6 +668,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
>   			goto err_device_del;
>   	}
>   
> +	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_PRE_ADD, nvmem);
> +
>   	if (config->cells) {
>   		rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
>   		if (rval)
> diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> index 052293f4cbdb..0f7107276756 100644
> --- a/include/linux/nvmem-consumer.h
> +++ b/include/linux/nvmem-consumer.h
> @@ -50,6 +50,7 @@ enum {
>   	NVMEM_REMOVE,
>   	NVMEM_CELL_ADD,
>   	NVMEM_CELL_REMOVE,
> +	NVMEM_PRE_ADD,
>   };
>   
>   #if IS_ENABLED(CONFIG_NVMEM)
> 

Re: [PATCH v3 2/3] nvmem: add ONIE NVMEM cells support

[Srinivas Kandagatla]

On 31/08/2020 02:55, Vadym Kochan wrote:
> 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>
> ---
> v3:
>      1) Update onie-cells.c by using nvmem notification to parse and
>         register cells before device and cells are finally registered.
> 
>      2) Use subsys_init() macro which allows to probe before nvmem drivers.
> 
>   drivers/nvmem/Kconfig      |   9 +
>   drivers/nvmem/Makefile     |   3 +
>   drivers/nvmem/onie-cells.c | 410 +++++++++++++++++++++++++++++++++++++

where is the device tree bindings for this driver?

And the nvmem provider driver for this?

--srini


>   3 files changed, 422 insertions(+)
>   create mode 100644 drivers/nvmem/onie-cells.c
> 
> diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
> index 954d3b4a52ab..922d753519f7 100644
> --- a/drivers/nvmem/Kconfig
> +++ b/drivers/nvmem/Kconfig
> @@ -270,4 +270,13 @@ config SPRD_EFUSE
>   	  This driver can also be built as a module. If so, the module
>   	  will be called nvmem-sprd-efuse.
>   
> +config NVMEM_ONIE_CELLS
> +	bool "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-cells.
> +
>   endif
> diff --git a/drivers/nvmem/Makefile b/drivers/nvmem/Makefile
> index a7c377218341..2199784a489f 100644
> --- a/drivers/nvmem/Makefile
> +++ b/drivers/nvmem/Makefile
> @@ -55,3 +55,6 @@ obj-$(CONFIG_NVMEM_ZYNQMP)	+= nvmem_zynqmp_nvmem.o
>   nvmem_zynqmp_nvmem-y		:= zynqmp_nvmem.o
>   obj-$(CONFIG_SPRD_EFUSE)	+= nvmem_sprd_efuse.o
>   nvmem_sprd_efuse-y		:= sprd-efuse.o
> +
> +obj-$(CONFIG_NVMEM_ONIE_CELLS)	+= nvmem-onie-cells.o
> +nvmem-onie-cells-y		:= onie-cells.o
> diff --git a/drivers/nvmem/onie-cells.c b/drivers/nvmem/onie-cells.c
> new file mode 100644
> index 000000000000..bbb1e421f67c
> --- /dev/null
> +++ b/drivers/nvmem/onie-cells.c
> @@ -0,0 +1,410 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + * ONIE NVMEM cells provider
> + *
> + * Author: Vadym Kochan <vadym.kochan@plvision.eu>
> + */
> +
> +#define ONIE_NVMEM_DRVNAME	"onie-nvmem-cells"
> +
> +#define pr_fmt(fmt) ONIE_NVMEM_DRVNAME ": " fmt
> +
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/init.h>
> +#include <linux/kref.h>
> +#include <linux/nvmem-consumer.h>
> +#include <linux/nvmem-provider.h>
> +#include <linux/of.h>
> +#include <linux/of_device.h>
> +#include <linux/platform_device.h>
> +#include <linux/slab.h>
> +
> +#define ONIE_NVMEM_TLV_MAX_LEN	2048
> +
> +#define ONIE_NVMEM_HDR_ID	"TlvInfo"
> +
> +struct onie_nvmem_hdr {
> +	u8 id[8];
> +	u8 version;
> +	__be16 data_len;
> +} __packed;
> +
> +struct onie_nvmem_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_nvmem {
> +	struct platform_device *pdev;
> +	struct notifier_block nvmem_nb;
> +	unsigned int attr_count;
> +	struct list_head attrs;
> +	struct kref refcnt;
> +	const char *nvmem_match;
> +
> +	struct nvmem_cell_lookup *cell_lookup;
> +	struct nvmem_cell_table cell_tbl;
> +	struct nvmem_device *nvmem;
> +};
> +
> +static bool onie_nvmem_hdr_is_valid(struct onie_nvmem_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_nvmem *onie)
> +{
> +	struct onie_nvmem_attr *attr, *tmp;
> +
> +	list_for_each_entry_safe(attr, tmp, &onie->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_nvmem *onie, u8 *data, u16 len)
> +{
> +	unsigned int hlen = sizeof(struct onie_nvmem_hdr);
> +	unsigned int offset = 0;
> +	int err;
> +
> +	while (offset < len) {
> +		struct onie_nvmem_attr *attr;
> +		struct onie_nvmem_tlv *tlv;
> +
> +		tlv = (struct onie_nvmem_tlv *)(data + offset);
> +
> +		if (offset + tlv->len >= len) {
> +			struct nvmem_device *nvmem = onie->nvmem;
> +
> +			pr_err("%s: TLV len is too big(0x%x) at 0x%x\n",
> +			       nvmem_dev_name(nvmem), 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, &onie->attrs);
> +		onie->attr_count++;
> +
> +		offset += sizeof(*tlv) + tlv->len;
> +	}
> +
> +	return 0;
> +
> +err_attr_alloc:
> +	onie_nvmem_attrs_free(onie);
> +
> +	return err;
> +}
> +
> +static int onie_nvmem_decode(struct onie_nvmem *onie)
> +{
> +	struct nvmem_device *nvmem = onie->nvmem;
> +	struct onie_nvmem_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("%s: invalid ONIE TLV header\n", nvmem_dev_name(nvmem));
> +		ret = -EINVAL;
> +		goto err_invalid;
> +	}
> +
> +	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) {
> +		ret = -ENOMEM;
> +		goto err_kmalloc;
> +	}
> +
> +	ret = nvmem_device_read(nvmem, sizeof(hdr), len, data);
> +	if (ret < 0)
> +		goto err_data_read;
> +
> +	ret = onie_nvmem_tlv_parse(onie, data, len);
> +	if (ret)
> +		goto err_info_parse;
> +
> +	kfree(data);
> +
> +	return 0;
> +
> +err_info_parse:
> +err_data_read:
> +	kfree(data);
> +err_kmalloc:
> +err_invalid:
> +	return ret;
> +}
> +
> +static int onie_nvmem_cells_parse(struct onie_nvmem *onie)
> +{
> +	struct platform_device *pdev = onie->pdev;
> +	struct nvmem_device *nvmem = onie->nvmem;
> +	struct device *dev = &pdev->dev;
> +	struct nvmem_cell_info *cells;
> +	struct onie_nvmem_attr *attr;
> +	unsigned int ncells = 0;
> +	int err;
> +
> +	INIT_LIST_HEAD(&onie->attrs);
> +	onie->attr_count = 0;
> +
> +	err = onie_nvmem_decode(onie);
> +	if (err)
> +		return err;
> +
> +	if (!onie->attr_count) {
> +		pr_err("%s: has no ONIE attributes\n", nvmem_dev_name(nvmem));
> +		return -EINVAL;
> +	}
> +
> +	cells = kmalloc_array(onie->attr_count, sizeof(*cells), GFP_KERNEL);
> +	if (!cells) {
> +		err = -ENOMEM;
> +		goto err_cells_alloc;
> +	}
> +
> +	onie->cell_lookup = kmalloc_array(onie->attr_count,
> +					  sizeof(struct nvmem_cell_lookup),
> +					  GFP_KERNEL);
> +	if (!onie->cell_lookup) {
> +		err = -ENOMEM;
> +		goto err_lookup_alloc;
> +	}
> +
> +	list_for_each_entry(attr, &onie->attrs, head) {
> +		struct nvmem_cell_lookup *lookup;
> +		struct nvmem_cell_info *cell;
> +
> +		cell = &cells[ncells];
> +
> +		lookup = &onie->cell_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->nvmem_name = nvmem_dev_name(onie->nvmem);
> +		lookup->dev_id = dev_name(dev);
> +		lookup->cell_name = cell->name;
> +		lookup->con_id = cell->name;
> +
> +		ncells++;
> +	}
> +
> +	onie->cell_tbl.nvmem_name = nvmem_dev_name(onie->nvmem);
> +	onie->cell_tbl.ncells = ncells;
> +	onie->cell_tbl.cells = cells;
> +
> +	nvmem_add_cell_table(&onie->cell_tbl);
> +	nvmem_add_cell_lookups(onie->cell_lookup, ncells);
> +
> +	onie_nvmem_attrs_free(onie);
> +
> +	return 0;
> +
> +err_lookup_alloc:
> +	kfree(onie->cell_tbl.cells);
> +err_cells_alloc:
> +	onie_nvmem_attrs_free(onie);
> +
> +	return err;
> +}
> +
> +static void onie_nvmem_release(struct kref *kref)
> +{
> +	kfree(container_of(kref, struct onie_nvmem, refcnt));
> +}
> +
> +static void onie_nvmem_get(struct onie_nvmem *onie)
> +{
> +	kref_get(&onie->refcnt);
> +}
> +
> +static void onie_nvmem_put(struct onie_nvmem *onie)
> +{
> +	kref_put(&onie->refcnt, onie_nvmem_release);
> +}
> +
> +static int onie_nvmem_notify(struct notifier_block *nb,
> +			     unsigned long val, void *data)
> +{
> +	struct nvmem_device *nvmem = data;
> +	struct onie_nvmem *onie;
> +	int err;
> +
> +	if (val != NVMEM_PRE_ADD && val != NVMEM_REMOVE)
> +		return NOTIFY_DONE;
> +
> +	onie = container_of(nb, struct onie_nvmem, nvmem_nb);
> +
> +	if (strcmp(onie->nvmem_match, nvmem_dev_name(nvmem)) != 0)
> +		return NOTIFY_DONE;
> +
> +	switch (val) {
> +	case NVMEM_PRE_ADD:
> +		onie->nvmem = nvmem;
> +
> +		err = onie_nvmem_cells_parse(onie);
> +		if (err)
> +			return NOTIFY_BAD;
> +
> +		onie_nvmem_get(onie);
> +		break;
> +
> +	case NVMEM_REMOVE:
> +		nvmem_del_cell_lookups(onie->cell_lookup, onie->attr_count);
> +		nvmem_del_cell_table(&onie->cell_tbl);
> +
> +		kfree(onie->cell_tbl.cells);
> +		kfree(onie->cell_lookup);
> +
> +		onie_nvmem_put(onie);
> +		break;
> +
> +	default:
> +		return NOTIFY_DONE;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static int onie_nvmem_probe(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct device_node *np = dev->of_node;
> +	struct onie_nvmem *onie;
> +	const char *nvmem_match;
> +	int err;
> +
> +	err = of_property_read_string(np, "nvmem-name", &nvmem_match);
> +	if (err) {
> +		dev_err(dev, "error while parsing 'nvmem-name' property\n");
> +		return err;
> +	}
> +
> +	onie = kmalloc(sizeof(*onie), GFP_KERNEL);
> +	if (!onie)
> +		return -ENOMEM;
> +
> +	kref_init(&onie->refcnt);
> +
> +	onie->nvmem_nb.notifier_call = onie_nvmem_notify;
> +	onie->nvmem_match = nvmem_match;
> +	onie->pdev = pdev;
> +
> +	dev_set_drvdata(dev, onie);
> +
> +	return nvmem_register_notifier(&onie->nvmem_nb);
> +}
> +
> +static int onie_nvmem_remove(struct platform_device *pdev)
> +{
> +	struct device *dev = &pdev->dev;
> +	struct onie_nvmem *onie;
> +
> +	onie = dev_get_drvdata(dev);
> +
> +	nvmem_unregister_notifier(&onie->nvmem_nb);
> +	onie_nvmem_put(onie);
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id onie_nvmem_match[] = {
> +	{
> +		.compatible = "onie,nvmem-cells",
> +	},
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(of, onie_nvmem_match);
> +
> +static struct platform_driver onie_nvmem_driver = {
> +	.probe = onie_nvmem_probe,
> +	.remove = onie_nvmem_remove,
> +	.driver = {
> +		.name = ONIE_NVMEM_DRVNAME,
> +		.of_match_table = onie_nvmem_match,
> +	},
> +};
> +
> +static int __init onie_nvmem_init(void)
> +{
> +	return platform_driver_register(&onie_nvmem_driver);
> +}
> +
> +static void __exit onie_nvmem_exit(void)
> +{
> +	platform_driver_unregister(&onie_nvmem_driver);
> +}
> +
> +subsys_initcall(onie_nvmem_init);
> +module_exit(onie_nvmem_exit);
> +
> +MODULE_AUTHOR("Vadym Kochan <vadym.kochan@plvision.eu>");
> +MODULE_DESCRIPTION("ONIE NVMEM cells driver");
> +MODULE_LICENSE("GPL");
> 

Re: [PATCH v3 1/3] nvmem: core: allow to register cells during nvmem registration

[Vadym Kochan]

Hi Srinivas,

On Fri, Sep 04, 2020 at 12:02:40PM +0100, Srinivas Kandagatla wrote:
> Hi Vadym,
> 
> Thanks for the patch,
> On 31/08/2020 02:55, Vadym Kochan wrote:
> > Add NVMEM_PRE_ADD notification step which is called before any cells
> > binding - from lookup table or config, this allows to register cells
> > in some specific layout (tlv) which should be parsed first and then
> > registered. So there might be a cell parser driver which can register
> > lookup table during this notification step.
> > 
> This is going in right direction but totally not correct way to do it.
> 
> 1> this is not scalable as any consumer that will register for this even
> will have no idea of which what kind of parsing that provider needs.
> It can work in your case but not really useful.
> 
> 2> this is a consumer API, not the provider api.
> 
> How about adding a "parse_cells" callback in struct nvmem_config along with
> encoding type.
> 
> 
> thanks,
> srini
> 

Looks like I missed main point here that this cells parser should be
registered as nvmem provider. I will think on it.

Thanks,

> > Signed-off-by: Vadym Kochan <vadym.kochan@plvision.eu>
> > ---
> > v3:
> >      1) Update core.c changes by extending notification mechanism
> >         by adding new NVMEM_PRE_ADD event id which is called before lookup
> >         table cells binding, this allows for notification handler to
> >         register cells which require nvmem parsing.
> > 
> >   drivers/nvmem/core.c           | 2 ++
> >   include/linux/nvmem-consumer.h | 1 +
> >   2 files changed, 3 insertions(+)
> > 
> > diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> > index 6cd3edb2eaf6..c48a69e0ebbe 100644
> > --- a/drivers/nvmem/core.c
> > +++ b/drivers/nvmem/core.c
> > @@ -668,6 +668,8 @@ struct nvmem_device *nvmem_register(const struct nvmem_config *config)
> >   			goto err_device_del;
> >   	}
> > +	blocking_notifier_call_chain(&nvmem_notifier, NVMEM_PRE_ADD, nvmem);
> > +
> >   	if (config->cells) {
> >   		rval = nvmem_add_cells(nvmem, config->cells, config->ncells);
> >   		if (rval)
> > diff --git a/include/linux/nvmem-consumer.h b/include/linux/nvmem-consumer.h
> > index 052293f4cbdb..0f7107276756 100644
> > --- a/include/linux/nvmem-consumer.h
> > +++ b/include/linux/nvmem-consumer.h
> > @@ -50,6 +50,7 @@ enum {
> >   	NVMEM_REMOVE,
> >   	NVMEM_CELL_ADD,
> >   	NVMEM_CELL_REMOVE,
> > +	NVMEM_PRE_ADD,
> >   };
> >   #if IS_ENABLED(CONFIG_NVMEM)
> > 

Re: [PATCH v3 1/3] nvmem: core: allow to register cells during nvmem registration

[Vadym Kochan]

Hi Srinivas,

On Fri, Sep 04, 2020 at 02:23:10PM +0300, Vadym Kochan wrote:
> Hi Srinivas,
> 
> On Fri, Sep 04, 2020 at 12:02:40PM +0100, Srinivas Kandagatla wrote:
> > Hi Vadym,
> > 
> > Thanks for the patch,
> > On 31/08/2020 02:55, Vadym Kochan wrote:
> > > Add NVMEM_PRE_ADD notification step which is called before any cells
> > > binding - from lookup table or config, this allows to register cells
> > > in some specific layout (tlv) which should be parsed first and then
> > > registered. So there might be a cell parser driver which can register
> > > lookup table during this notification step.
> > > 
> > This is going in right direction but totally not correct way to do it.
> > 
> > 1> this is not scalable as any consumer that will register for this even
> > will have no idea of which what kind of parsing that provider needs.
> > It can work in your case but not really useful.
> > 
> > 2> this is a consumer API, not the provider api.
> > 
> > How about adding a "parse_cells" callback in struct nvmem_config along with
> > encoding type.
> > 
> > 
> > thanks,
> > srini
> > 
> 
> Looks like I missed main point here that this cells parser should be
> registered as nvmem provider. I will think on it.
> 
> Thanks,
> 

I am trying to re-work this approach, but still I need to clarify
something.

It looks strange that this cells parser should be a nvmem provider (or I
missed something) but I remember that you suggested about introducing
something like nvmem parser. And adding nvmem parser looks more clear
for me, because what it should do is just access the nvmem device during
its registration and provide list of cells, that's all:

struct nvmem_device *nvmem_register(const struct nvmem_config *config)
{
    struct nvmem_cell_table table = { };
...
    parser = find_nvmem_parser();
    /* I think that cell lookups may be added on the parser's probe
       statically */
    parser->parse_cells(parser->priv, nvmem, &table);
...
}

/* here I used struct nvmem_config, not sure it is a right way to
   mix nvmem's and parser's struct fields, so may be something like
   struct nvmem_parser_config might be introduced or fill the struct
   nvmem_parser directly by the driver and pass it to the registration func */
struct nvmem_parser *nvmem_parser_register(const struct nvmem_config *config)
{
...
}

void nvmem_parser_unregister(struct nvmem_parser *parser)
{
...
}

Regards,
Vadym Kochan