linux-mtd.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments
@ 2022-11-28  6:59 Rafał Miłecki
  2022-11-28  6:59 ` [PATCH V2 2/2] nvmem: u-boot-env: reformat MAC in "ethaddr" cell when reading Rafał Miłecki
  2022-11-28  7:35 ` [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Michael Walle
  0 siblings, 2 replies; 4+ messages in thread
From: Rafał Miłecki @ 2022-11-28  6:59 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Michael Walle, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	linux-mtd, devicetree, linux-arm-kernel, linux-kernel, u-boot,
	Rafał Miłecki

From: Rafał Miłecki <rafal@milecki.pl>

Pass whole NVMEM cell struct and length pointer as arguments to callback
functions.

This allows:

1. Cells content to be modified based on more info
   Some cells (identified by their names) contain specific data that
   needs further processing. This can be e.g. MAC address stored in an
   ASCII format. NVMEM consumers expect MAC to be read in a binary form.
   More complex cells may be additionally described in DT. This change
   allows also accessing relevant DT nodes and reading extra info.

2. Adjusting data length
   If cell processing results in reformatting it, it's required to
   adjust length. This again applies e.g. to the MAC format change from
   ASCII to the byte-based.

Later on we may consider more cleanups & features like:
1. Dropping "const char *id" and just using NVMEM cell name
2. Adding extra argument for cells providing multiple values

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
This solution conflicts with 1 part of Michael's work:
[PATCH v2 00/20] nvmem: core: introduce NVMEM layouts
https://lore.kernel.org/linux-arm-kernel/20220901221857.2600340-1-michael@walle.cc/

Instead of:
1. Adding NVMEM cell-level post_process callback
2. Adding callback (.fixup_cell_info()) for setting callbacks
3. Dropping NVMEM device-level post_process callback
I decided to refactor existing callback.

Michael's work on adding #nvmem-cell-cells should be possible to easily
rebase on top of those changes.

This doen't add support for 1 cell providing multiple values. That needs
to be added later once we sort out #nvmem-cell-cells bindings. This
fixes the basic case with reformatting cells data.
---
 drivers/nvmem/core.c           | 19 +++----------------
 drivers/nvmem/imx-ocotp.c      |  8 ++++----
 include/linux/nvmem-provider.h | 17 ++++++++++++++---
 3 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index 321d7d63e068..0bc3e26e4ca8 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -46,16 +46,6 @@ struct nvmem_device {
 #define to_nvmem_device(d) container_of(d, struct nvmem_device, dev)
 
 #define FLAG_COMPAT		BIT(0)
-struct nvmem_cell_entry {
-	const char		*name;
-	int			offset;
-	int			bytes;
-	int			bit_offset;
-	int			nbits;
-	struct device_node	*np;
-	struct nvmem_device	*nvmem;
-	struct list_head	node;
-};
 
 struct nvmem_cell {
 	struct nvmem_cell_entry *entry;
@@ -1416,24 +1406,21 @@ static int __nvmem_cell_read(struct nvmem_device *nvmem,
 	int rc;
 
 	rc = nvmem_reg_read(nvmem, cell->offset, buf, cell->bytes);
-
 	if (rc)
 		return rc;
+	if (len)
+		*len = cell->bytes;
 
 	/* shift bits in-place */
 	if (cell->bit_offset || cell->nbits)
 		nvmem_shift_read_buffer_in_place(cell, buf);
 
 	if (nvmem->cell_post_process) {
-		rc = nvmem->cell_post_process(nvmem->priv, id,
-					      cell->offset, buf, cell->bytes);
+		rc = nvmem->cell_post_process(nvmem->priv, cell, id, buf, len);
 		if (rc)
 			return rc;
 	}
 
-	if (len)
-		*len = cell->bytes;
-
 	return 0;
 }
 
diff --git a/drivers/nvmem/imx-ocotp.c b/drivers/nvmem/imx-ocotp.c
index 14284e866f26..d383989d48bf 100644
--- a/drivers/nvmem/imx-ocotp.c
+++ b/drivers/nvmem/imx-ocotp.c
@@ -222,8 +222,8 @@ static int imx_ocotp_read(void *context, unsigned int offset,
 	return ret;
 }
 
-static int imx_ocotp_cell_pp(void *context, const char *id, unsigned int offset,
-			     void *data, size_t bytes)
+static int imx_ocotp_cell_pp(void *context, struct nvmem_cell_entry *cell,
+			     const char *id, void *data, size_t *len)
 {
 	struct ocotp_priv *priv = context;
 
@@ -233,8 +233,8 @@ static int imx_ocotp_cell_pp(void *context, const char *id, unsigned int offset,
 			u8 *buf = data;
 			int i;
 
-			for (i = 0; i < bytes/2; i++)
-				swap(buf[i], buf[bytes - i - 1]);
+			for (i = 0; i < cell->bytes / 2; i++)
+				swap(buf[i], buf[cell->bytes - i - 1]);
 		}
 	}
 
diff --git a/include/linux/nvmem-provider.h b/include/linux/nvmem-provider.h
index 50caa117cb62..b0d2b6af9f37 100644
--- a/include/linux/nvmem-provider.h
+++ b/include/linux/nvmem-provider.h
@@ -14,14 +14,25 @@
 #include <linux/gpio/consumer.h>
 
 struct nvmem_device;
-struct nvmem_cell_info;
+
+struct nvmem_cell_entry {
+	const char		*name;
+	int			offset;
+	int			bytes;
+	int			bit_offset;
+	int			nbits;
+	struct device_node	*np;
+	struct nvmem_device	*nvmem;
+	struct list_head	node;
+};
+
 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);
 /* used for vendor specific post processing of cell data */
-typedef int (*nvmem_cell_post_process_t)(void *priv, const char *id, unsigned int offset,
-					  void *buf, size_t bytes);
+typedef int (*nvmem_cell_post_process_t)(void *priv, struct nvmem_cell_entry *cell, const char *id,
+					 void *buf, size_t *len);
 
 enum nvmem_type {
 	NVMEM_TYPE_UNKNOWN = 0,
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* [PATCH V2 2/2] nvmem: u-boot-env: reformat MAC in "ethaddr" cell when reading
  2022-11-28  6:59 [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Rafał Miłecki
@ 2022-11-28  6:59 ` Rafał Miłecki
  2022-11-28  7:35 ` [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Michael Walle
  1 sibling, 0 replies; 4+ messages in thread
From: Rafał Miłecki @ 2022-11-28  6:59 UTC (permalink / raw)
  To: Srinivas Kandagatla
  Cc: Michael Walle, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	linux-mtd, devicetree, linux-arm-kernel, linux-kernel, u-boot,
	Rafał Miłecki

From: Rafał Miłecki <rafal@milecki.pl>

NVMEM consumers expect MAC in a byte-based format (see e.g.
nvmem_get_mac_address()). U-Boot environment data stores all values in
ASCII form.

Add post processing callback detecting "ethaddr" reads and reformat data
as expected. This fixes Ethernet drivers reading MAC from NVMEM devices.

Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
---
V2: select GENERIC_NET_UTILS & drop unused "priv" variable
---
 drivers/nvmem/Kconfig      |  1 +
 drivers/nvmem/u-boot-env.c | 19 +++++++++++++++++++
 2 files changed, 20 insertions(+)

diff --git a/drivers/nvmem/Kconfig b/drivers/nvmem/Kconfig
index 755f551426b5..34fb0ba36b80 100644
--- a/drivers/nvmem/Kconfig
+++ b/drivers/nvmem/Kconfig
@@ -326,6 +326,7 @@ config NVMEM_U_BOOT_ENV
 	tristate "U-Boot environment variables support"
 	depends on OF && MTD
 	select CRC32
+	select GENERIC_NET_UTILS
 	help
 	  U-Boot stores its setup as environment variables. This driver adds
 	  support for verifying & exporting such data. It also exposes variables
diff --git a/drivers/nvmem/u-boot-env.c b/drivers/nvmem/u-boot-env.c
index 2a87dda45188..e30ce4f7ea20 100644
--- a/drivers/nvmem/u-boot-env.c
+++ b/drivers/nvmem/u-boot-env.c
@@ -4,6 +4,8 @@
  */
 
 #include <linux/crc32.h>
+#include <linux/etherdevice.h>
+#include <linux/if_ether.h>
 #include <linux/mod_devicetable.h>
 #include <linux/module.h>
 #include <linux/mtd/mtd.h>
@@ -70,6 +72,22 @@ static int u_boot_env_read(void *context, unsigned int offset, void *val,
 	return 0;
 }
 
+static int u_boot_env_cell_post_process(void *context, struct nvmem_cell_entry *cell,
+					const char *id, void *buf, size_t *len)
+{
+	if (!strcmp(cell->name, "ethaddr")) {
+		u8 mac[ETH_ALEN];
+
+		if (mac_pton(buf, mac)) {
+			ether_addr_copy(buf, mac);
+			if (len)
+				*len = ETH_ALEN;
+		}
+	}
+
+	return 0;
+}
+
 static int u_boot_env_add_cells(struct u_boot_env *priv, uint8_t *buf,
 				size_t data_offset, size_t data_len)
 {
@@ -179,6 +197,7 @@ static int u_boot_env_probe(struct platform_device *pdev)
 	struct nvmem_config config = {
 		.name = "u-boot-env",
 		.reg_read = u_boot_env_read,
+		.cell_post_process = u_boot_env_cell_post_process,
 	};
 	struct device *dev = &pdev->dev;
 	struct device_node *np = dev->of_node;
-- 
2.34.1


______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* Re: [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments
  2022-11-28  6:59 [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Rafał Miłecki
  2022-11-28  6:59 ` [PATCH V2 2/2] nvmem: u-boot-env: reformat MAC in "ethaddr" cell when reading Rafał Miłecki
@ 2022-11-28  7:35 ` Michael Walle
  2022-11-28  8:30   ` Miquel Raynal
  1 sibling, 1 reply; 4+ messages in thread
From: Michael Walle @ 2022-11-28  7:35 UTC (permalink / raw)
  To: Rafał Miłecki
  Cc: Srinivas Kandagatla, Miquel Raynal, Richard Weinberger,
	Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	linux-mtd, devicetree, linux-arm-kernel, linux-kernel, u-boot,
	Rafał Miłecki

Am 2022-11-28 07:59, schrieb Rafał Miłecki:
> From: Rafał Miłecki <rafal@milecki.pl>
> 
> Pass whole NVMEM cell struct and length pointer as arguments to 
> callback
> functions.
> 
> This allows:
> 
> 1. Cells content to be modified based on more info
>    Some cells (identified by their names) contain specific data that
>    needs further processing. This can be e.g. MAC address stored in an
>    ASCII format. NVMEM consumers expect MAC to be read in a binary 
> form.
>    More complex cells may be additionally described in DT. This change
>    allows also accessing relevant DT nodes and reading extra info.
> 
> 2. Adjusting data length
>    If cell processing results in reformatting it, it's required to
>    adjust length. This again applies e.g. to the MAC format change from
>    ASCII to the byte-based.
> 
> Later on we may consider more cleanups & features like:
> 1. Dropping "const char *id" and just using NVMEM cell name
> 2. Adding extra argument for cells providing multiple values
> 
> Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> ---
> This solution conflicts with 1 part of Michael's work:
> [PATCH v2 00/20] nvmem: core: introduce NVMEM layouts
> https://lore.kernel.org/linux-arm-kernel/20220901221857.2600340-1-michael@walle.cc/
> 
> Instead of:
> 1. Adding NVMEM cell-level post_process callback
> 2. Adding callback (.fixup_cell_info()) for setting callbacks
> 3. Dropping NVMEM device-level post_process callback
> I decided to refactor existing callback.
> 
> Michael's work on adding #nvmem-cell-cells should be possible to easily
> rebase on top of those changes.

As yours should be easily added on top of my series. I've showed that
providing a global post process hook is bad because that way you need
to have *all* cells of your device read-only.

-michael

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

* Re: [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments
  2022-11-28  7:35 ` [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Michael Walle
@ 2022-11-28  8:30   ` Miquel Raynal
  0 siblings, 0 replies; 4+ messages in thread
From: Miquel Raynal @ 2022-11-28  8:30 UTC (permalink / raw)
  To: Michael Walle
  Cc: Rafał Miłecki, Srinivas Kandagatla, Richard Weinberger,
	Vignesh Raghavendra, Rob Herring, Krzysztof Kozlowski, Shawn Guo,
	linux-mtd, devicetree, linux-arm-kernel, linux-kernel, u-boot,
	Rafał Miłecki

Hi Rafal,

michael@walle.cc wrote on Mon, 28 Nov 2022 08:35:24 +0100:

> Am 2022-11-28 07:59, schrieb Rafał Miłecki:
> > From: Rafał Miłecki <rafal@milecki.pl>
> > 
> > Pass whole NVMEM cell struct and length pointer as arguments to > callback
> > functions.
> > 
> > This allows:
> > 
> > 1. Cells content to be modified based on more info
> >    Some cells (identified by their names) contain specific data that
> >    needs further processing. This can be e.g. MAC address stored in an
> >    ASCII format. NVMEM consumers expect MAC to be read in a binary > form.
> >    More complex cells may be additionally described in DT. This change
> >    allows also accessing relevant DT nodes and reading extra info.
> > 
> > 2. Adjusting data length
> >    If cell processing results in reformatting it, it's required to
> >    adjust length. This again applies e.g. to the MAC format change from
> >    ASCII to the byte-based.

Michael's series brings read_post_process, isn't what you need here?

> > 
> > Later on we may consider more cleanups & features like:
> > 1. Dropping "const char *id" and just using NVMEM cell name
> > 2. Adding extra argument for cells providing multiple values
> > 
> > Signed-off-by: Rafał Miłecki <rafal@milecki.pl>
> > ---
> > This solution conflicts with 1 part of Michael's work:
> > [PATCH v2 00/20] nvmem: core: introduce NVMEM layouts
> > https://lore.kernel.org/linux-arm-kernel/20220901221857.2600340-1-michael@walle.cc/
> > 
> > Instead of:
> > 1. Adding NVMEM cell-level post_process callback
> > 2. Adding callback (.fixup_cell_info()) for setting callbacks
> > 3. Dropping NVMEM device-level post_process callback
> > I decided to refactor existing callback.
> > 
> > Michael's work on adding #nvmem-cell-cells should be possible to easily
> > rebase on top of those changes.  

Yeah, I guess since Michael's series has been out for 2 years and we
finally agreed on the bindings plus some implementation points, I would
expect it to be merged very soon (I don't know if Srinivas still plans
to take it for this release or for the next?) unless someone speaks up
against it.

> As yours should be easily added on top of my series. I've showed that
> providing a global post process hook is bad because that way you need
> to have *all* cells of your device read-only.
> 
> -michael

Thanks,
Miquèl

______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/

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

end of thread, other threads:[~2022-11-28  8:31 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-11-28  6:59 [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Rafał Miłecki
2022-11-28  6:59 ` [PATCH V2 2/2] nvmem: u-boot-env: reformat MAC in "ethaddr" cell when reading Rafał Miłecki
2022-11-28  7:35 ` [PATCH V2 1/2] nvmem: core: refactor .cell_post_process() CB arguments Michael Walle
2022-11-28  8:30   ` Miquel Raynal

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).