[PATCH 00/11] 'eeprom' command improvements

[PATCH 00/11] 'eeprom' command improvements

[Marek Behún]

Hi Tom,

this series contains improvements for the 'eeprom' command:
- refactors
- fixes
- improvements
- ability to use driver model EEPROMs (uclass UCLASS_I2C_EEPROM)
- more flexible EEPROM layout support

It should not cause any behavior change for any existing board.

This series is a dependency for some DDR issue fixes for Turris Omnia.
I will be sending that one separately.

github PR link (with CI):
  https://github.com/u-boot/u-boot/pull/540
- there is a failure for
    test.py for sandbox sandbox_clang
  but it seems unrelated to these changes

Marek Behún (11):
  common: eeprom_layout: Assign default layout methods and parameters
    before specific ones
  common: eeprom_layout: Split field finding code from the field update
    function
  common: eeprom_field: Fix updating binary field
  common: eeprom_field: Drop unnecessary comparison
  cmd: eeprom: Fix usage help for the eeprom command
  cmd: eeprom: Hide eeprom layout versioning behind a Kconfig option
  cmd: eeprom: Deduplicate parse_i2c_bus_addr() calls
  cmd: eeprom: Refactor eeprom device specifier parsing
  cmd: eeprom: Refactor command execution into function by action
  cmd: eeprom: Don't read/write whole EEPROM if not necessary
  cmd: eeprom: Extend to EEPROMs probed via driver model

 cmd/Kconfig                   |   9 +-
 cmd/eeprom.c                  | 305 +++++++++++++++++++++++++---------
 common/eeprom/eeprom_field.c  |   4 +-
 common/eeprom/eeprom_layout.c |  54 +++---
 configs/cm_fx6_defconfig      |   1 +
 configs/cm_t43_defconfig      |   1 +
 include/eeprom_layout.h       |   4 +
 7 files changed, 273 insertions(+), 105 deletions(-)

-- 
2.44.1

[PATCH 01/11] common: eeprom_layout: Assign default layout methods and parameters before specific ones

[Marek Behún]

Assign the default eeprom layout parameter .data_size and methods
.print() and .update() before calling eeprom_layout_assign() in
eeprom_layout_setup().

This allows eeprom_layout_assign() to overwrite these if needed.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 common/eeprom/eeprom_layout.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/common/eeprom/eeprom_layout.c b/common/eeprom/eeprom_layout.c
index 5a9be1da06..406db3f7d1 100644
--- a/common/eeprom/eeprom_layout.c
+++ b/common/eeprom/eeprom_layout.c
@@ -111,14 +111,14 @@ void eeprom_layout_setup(struct eeprom_layout *layout, unsigned char *buf,
 	else
 		layout->layout_version = layout_version;
 
+	layout->data_size = buf_size;
+	layout->print = eeprom_layout_print;
+	layout->update = eeprom_layout_update_field;
+
 	eeprom_layout_assign(layout, layout_version);
 	layout->data = buf;
 	for (i = 0; i < layout->num_of_fields; i++) {
 		layout->fields[i].buf = buf;
 		buf += layout->fields[i].size;
 	}
-
-	layout->data_size = buf_size;
-	layout->print = eeprom_layout_print;
-	layout->update = eeprom_layout_update_field;
 }
-- 
2.44.1

[PATCH 02/11] common: eeprom_layout: Split field finding code from the field update function

[Marek Behún]

Split the eeprom layout field finding code from the
eeprom_layout_update_field() function in order to make it usable in
alternative implementations of update method.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 common/eeprom/eeprom_layout.c | 46 +++++++++++++++++++++++------------
 include/eeprom_layout.h       |  4 +++
 2 files changed, 35 insertions(+), 15 deletions(-)

diff --git a/common/eeprom/eeprom_layout.c b/common/eeprom/eeprom_layout.c
index 406db3f7d1..801e90d38d 100644
--- a/common/eeprom/eeprom_layout.c
+++ b/common/eeprom/eeprom_layout.c
@@ -56,6 +56,28 @@ static void eeprom_layout_print(const struct eeprom_layout *layout)
 		fields[i].print(&fields[i]);
 }
 
+/*
+ * eeprom_layout_find_field() - finds a layout field by name
+ * @layout:	A pointer to an existing struct layout.
+ * @field_name:	The name of the field to update.
+ * @warn:	Whether to print a warning if the field is not found.
+ *
+ * Returns: a pointer to the found field or NULL on failure.
+ */
+struct eeprom_field *eeprom_layout_find_field(struct eeprom_layout *layout,
+					      char *field_name, bool warn)
+{
+	for (int i = 0; i < layout->num_of_fields; i++)
+		if (layout->fields[i].name != RESERVED_FIELDS &&
+		    !strcmp(layout->fields[i].name, field_name))
+			return &layout->fields[i];
+
+	if (warn)
+		printf("No such field '%s'\n", field_name);
+
+	return NULL;
+}
+
 /*
  * eeprom_layout_update_field() - update a single field in the layout data.
  * @layout:	A pointer to an existing struct layout.
@@ -67,8 +89,8 @@ static void eeprom_layout_print(const struct eeprom_layout *layout)
 static int eeprom_layout_update_field(struct eeprom_layout *layout,
 				      char *field_name, char *new_data)
 {
-	int i, err;
-	struct eeprom_field *fields = layout->fields;
+	struct eeprom_field *field;
+	int err;
 
 	if (new_data == NULL)
 		return 0;
@@ -76,21 +98,15 @@ static int eeprom_layout_update_field(struct eeprom_layout *layout,
 	if (field_name == NULL)
 		return -1;
 
-	for (i = 0; i < layout->num_of_fields; i++) {
-		if (fields[i].name == RESERVED_FIELDS ||
-		    strcmp(fields[i].name, field_name))
-			continue;
-
-		err = fields[i].update(&fields[i], new_data);
-		if (err)
-			printf("Invalid data for field %s\n", field_name);
-
-		return err;
-	}
+	field = eeprom_layout_find_field(layout, field_name, true);
+	if (field == NULL)
+		return -1;
 
-	printf("No such field '%s'\n", field_name);
+	err = field->update(field, new_data);
+	if (err)
+		printf("Invalid data for field %s\n", field_name);
 
-	return -1;
+	return err;
 }
 
 /*
diff --git a/include/eeprom_layout.h b/include/eeprom_layout.h
index 730d963ab9..b1d6220595 100644
--- a/include/eeprom_layout.h
+++ b/include/eeprom_layout.h
@@ -9,6 +9,8 @@
 #ifndef _LAYOUT_
 #define _LAYOUT_
 
+#include <eeprom_field.h>
+
 #define RESERVED_FIELDS			NULL
 #define LAYOUT_VERSION_UNRECOGNIZED	-1
 #define LAYOUT_VERSION_AUTODETECT	-2
@@ -24,6 +26,8 @@ struct eeprom_layout {
 		      char *new_data);
 };
 
+struct eeprom_field *eeprom_layout_find_field(struct eeprom_layout *layout,
+					      char *field_name, bool warn);
 void eeprom_layout_setup(struct eeprom_layout *layout, unsigned char *buf,
 			 unsigned int buf_size, int layout_version);
 __weak void __eeprom_layout_assign(struct eeprom_layout *layout,
-- 
2.44.1

[PATCH 03/11] common: eeprom_field: Fix updating binary field

[Marek Behún]

The __eeprom_field_update_bin() function is expected to parse a hex
string into bytes (potentially in reverse order), but the
simple_strtoul() function is given 0 as base. This does not work since
the string does not contain '0x' prefix. Add explicit base 16.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 common/eeprom/eeprom_field.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/eeprom/eeprom_field.c b/common/eeprom/eeprom_field.c
index f56eebe679..9b831414a4 100644
--- a/common/eeprom/eeprom_field.c
+++ b/common/eeprom/eeprom_field.c
@@ -55,7 +55,7 @@ static int __eeprom_field_update_bin(struct eeprom_field *field,
 			tmp[k] = value[reverse ? i - 1 + k : i + k];
 		}
 
-		byte = simple_strtoul(tmp, &endptr, 0);
+		byte = simple_strtoul(tmp, &endptr, 16);
 		if (*endptr != '\0' || byte < 0)
 			return -1;
 
-- 
2.44.1

[PATCH 04/11] common: eeprom_field: Drop unnecessary comparison

[Marek Behún]

The byte variable is of type unsigned char, it is never less than zero.
The error case is handled by *endptr, so drop the comparison altogether.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 common/eeprom/eeprom_field.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/common/eeprom/eeprom_field.c b/common/eeprom/eeprom_field.c
index 9b831414a4..26f6041e54 100644
--- a/common/eeprom/eeprom_field.c
+++ b/common/eeprom/eeprom_field.c
@@ -56,7 +56,7 @@ static int __eeprom_field_update_bin(struct eeprom_field *field,
 		}
 
 		byte = simple_strtoul(tmp, &endptr, 16);
-		if (*endptr != '\0' || byte < 0)
+		if (*endptr != '\0')
 			return -1;
 
 		field->buf[j] = byte;
-- 
2.44.1

[PATCH 05/11] cmd: eeprom: Fix usage help for the eeprom command

[Marek Behún]

The bus and devaddr arguments of the eeprom command are optional, and if
only one is given, it is assumed to be devaddr. Change the usage help
from
  <bus> <devaddr>
to
  [[bus] [devaddr]

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/eeprom.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index 322765ad02..0d604832e4 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -418,14 +418,14 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 U_BOOT_CMD(
 	eeprom,	8,	1,	do_eeprom,
 	"EEPROM sub-system",
-	"read  <bus> <devaddr> addr off cnt\n"
-	"eeprom write <bus> <devaddr> addr off cnt\n"
+	"read  [[bus] devaddr] addr off cnt\n"
+	"eeprom write [[bus] devaddr] addr off cnt\n"
 	"       - read/write `cnt' bytes from `devaddr` EEPROM at offset `off'"
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
 	"\n"
-	"eeprom print [-l <layout_version>] <bus> <devaddr>\n"
+	"eeprom print [-l <layout_version>] [[bus] devaddr]\n"
 	"       - Print layout fields and their data in human readable format\n"
-	"eeprom update [-l <layout_version>] <bus> <devaddr> field_name field_value\n"
+	"eeprom update [-l <layout_version>] [[bus] devaddr] field_name field_value\n"
 	"       - Update a specific eeprom field with new data.\n"
 	"         The new data must be written in the same human readable format as shown by the print command.\n"
 	"\n"
-- 
2.44.1

[PATCH 06/11] cmd: eeprom: Hide eeprom layout versioning behind a Kconfig option

[Marek Behún]

Add a new Kconfig option EEPROM_LAYOUT_VERSIONS, and hide eeprom
layout versionsing code behind it. Only print the relevant help in
'eeprom' command usage if this option is enabled.

Enable this new option for cm_fx6_defconfig and cm_t43_defconfig.
These are the only boards using EEPROM layout versioning.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/Kconfig              |  9 ++++++++-
 cmd/eeprom.c             | 20 +++++++++++++++-----
 configs/cm_fx6_defconfig |  1 +
 configs/cm_t43_defconfig |  1 +
 4 files changed, 25 insertions(+), 6 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index b026439c77..8c370993f6 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -803,9 +803,16 @@ config CMD_EEPROM_LAYOUT
 			types of eeprom fields. Can be used for defining
 			custom layouts.
 
+config EEPROM_LAYOUT_VERSIONS
+	bool "Support specifying eeprom layout version"
+	depends on CMD_EEPROM_LAYOUT
+	help
+	  Support specifying eeprom layout version in the 'eeprom' command
+	  via the -l option.
+
 config EEPROM_LAYOUT_HELP_STRING
 	  string "Tells user what layout names are supported"
-	  depends on CMD_EEPROM_LAYOUT
+	  depends on EEPROM_LAYOUT_VERSIONS
 	  default "<not defined>"
 	  help
 	    Help printed with the LAYOUT VERSIONS part of the 'eeprom'
diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index 0d604832e4..d610dc9931 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -252,10 +252,12 @@ static int parse_i2c_bus_addr(int *i2c_bus, ulong *i2c_addr, int argc,
 
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
 
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
 __weak int eeprom_parse_layout_version(char *str)
 {
 	return LAYOUT_VERSION_UNRECOGNIZED;
 }
+#endif
 
 static unsigned char eeprom_buf[CONFIG_SYS_EEPROM_SIZE];
 
@@ -359,7 +361,7 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (action == EEPROM_ACTION_INVALID)
 		return CMD_RET_USAGE;
 
-#ifdef CONFIG_CMD_EEPROM_LAYOUT
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
 	if (action == EEPROM_PRINT || action == EEPROM_UPDATE) {
 		if (!strcmp(argv[index], "-l")) {
 			NEXT_PARAM(argc, index);
@@ -415,6 +417,12 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 				      field_name, field_value, addr, off, cnt);
 }
 
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
+#define EEPROM_LAYOUT_SPEC	"[-l <layout_version>] "
+#else
+#define EEPROM_LAYOUT_SPEC	""
+#endif
+
 U_BOOT_CMD(
 	eeprom,	8,	1,	do_eeprom,
 	"EEPROM sub-system",
@@ -423,16 +431,18 @@ U_BOOT_CMD(
 	"       - read/write `cnt' bytes from `devaddr` EEPROM at offset `off'"
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
 	"\n"
-	"eeprom print [-l <layout_version>] [[bus] devaddr]\n"
+	"eeprom print " EEPROM_LAYOUT_SPEC "[[bus] devaddr]\n"
 	"       - Print layout fields and their data in human readable format\n"
-	"eeprom update [-l <layout_version>] [[bus] devaddr] field_name field_value\n"
+	"eeprom update " EEPROM_LAYOUT_SPEC "[[bus] devaddr] field_name field_value\n"
 	"       - Update a specific eeprom field with new data.\n"
-	"         The new data must be written in the same human readable format as shown by the print command.\n"
-	"\n"
+	"         The new data must be written in the same human readable format as shown by the print command."
+#ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
+	"\n\n"
 	"LAYOUT VERSIONS\n"
 	"The -l option can be used to force the command to interpret the EEPROM data using the chosen layout.\n"
 	"If the -l option is omitted, the command will auto detect the layout based on the data in the EEPROM.\n"
 	"The values which can be provided with the -l option are:\n"
 	CONFIG_EEPROM_LAYOUT_HELP_STRING"\n"
 #endif
+#endif
 );
diff --git a/configs/cm_fx6_defconfig b/configs/cm_fx6_defconfig
index a4d5f91b35..386616cc42 100644
--- a/configs/cm_fx6_defconfig
+++ b/configs/cm_fx6_defconfig
@@ -46,6 +46,7 @@ CONFIG_SYS_MAXARGS=32
 CONFIG_CMD_GREPENV=y
 CONFIG_CMD_EEPROM=y
 CONFIG_CMD_EEPROM_LAYOUT=y
+CONFIG_EEPROM_LAYOUT_VERSIONS=y
 CONFIG_EEPROM_LAYOUT_HELP_STRING="v2, v3"
 CONFIG_SYS_I2C_EEPROM_BUS=2
 CONFIG_SYS_EEPROM_PAGE_WRITE_BITS=4
diff --git a/configs/cm_t43_defconfig b/configs/cm_t43_defconfig
index 93e667292c..32f126a517 100644
--- a/configs/cm_t43_defconfig
+++ b/configs/cm_t43_defconfig
@@ -50,6 +50,7 @@ CONFIG_SYS_PROMPT="CM-T43 # "
 CONFIG_CMD_ASKENV=y
 CONFIG_CMD_EEPROM=y
 CONFIG_CMD_EEPROM_LAYOUT=y
+CONFIG_EEPROM_LAYOUT_VERSIONS=y
 CONFIG_EEPROM_LAYOUT_HELP_STRING="v2, v3"
 CONFIG_SYS_EEPROM_PAGE_WRITE_BITS=4
 CONFIG_SYS_EEPROM_PAGE_WRITE_DELAY_MS=5
-- 
2.44.1

[PATCH 07/11] cmd: eeprom: Deduplicate parse_i2c_bus_addr() calls

[Marek Behún]

Deduplicate the calls to parse_i2c_bus_addr().

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/eeprom.c | 36 +++++++++++++++++-------------------
 1 file changed, 17 insertions(+), 19 deletions(-)

diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index d610dc9931..12902e812e 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -339,6 +339,21 @@ static int eeprom_execute_command(enum eeprom_action action, int i2c_bus,
 	return rcode;
 }
 
+static int eeprom_action_expected_argc(enum eeprom_action action)
+{
+	switch (action) {
+	case EEPROM_READ:
+	case EEPROM_WRITE:
+		return 3;
+	case EEPROM_PRINT:
+		return 0;
+	case EEPROM_UPDATE:
+		return 2;
+	default:
+		return CMD_RET_USAGE;
+	}
+}
+
 #define NEXT_PARAM(argc, index)	{ (argc)--; (index)++; }
 int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
@@ -371,25 +386,8 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	switch (action) {
-	case EEPROM_READ:
-	case EEPROM_WRITE:
-		ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc,
-					 argv + index, 3);
-		break;
-	case EEPROM_PRINT:
-		ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc,
-					 argv + index, 0);
-		break;
-	case EEPROM_UPDATE:
-		ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc,
-					 argv + index, 2);
-		break;
-	default:
-		/* Get compiler to stop whining */
-		return CMD_RET_USAGE;
-	}
-
+	ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc, argv + index,
+				 eeprom_action_expected_argc(action));
 	if (ret == CMD_RET_USAGE)
 		return ret;
 
-- 
2.44.1

[PATCH 08/11] cmd: eeprom: Refactor eeprom device specifier parsing

[Marek Behún]

In preparation for allowing to access eeprom by driver-model device
name, refactor the eeprom device specifier parsing. Instead of filling
two parameters (i2c_bus, i2c_addr), the parsing function now fills one
parameter of type struct eeprom_dev_spec.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/eeprom.c | 77 ++++++++++++++++++++++++++++------------------------
 1 file changed, 41 insertions(+), 36 deletions(-)

diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index 12902e812e..e29639780d 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -208,41 +208,42 @@ static long parse_numeric_param(char *str)
 	return (*endptr != '\0') ? -1 : value;
 }
 
+struct eeprom_dev_spec {
+	int i2c_bus;
+	ulong i2c_addr;
+};
+
 /**
- * parse_i2c_bus_addr - parse the i2c bus and i2c devaddr parameters
+ * parse_eeprom_dev_spec - parse the eeprom device specifier
  *
- * @i2c_bus:	address to store the i2c bus
- * @i2c_addr:	address to store the device i2c address
- * @argc:	count of command line arguments left to parse
+ * @dev:	pointer to eeprom device specifier
+ * @argc:	count of command line arguments that can be used to parse
+ *		the device specifier
  * @argv:	command line arguments left to parse
- * @argc_no_bus_addr:	argc value we expect to see when bus & addr aren't given
  *
  * @returns:	number of arguments parsed or CMD_RET_USAGE if error
  */
-static int parse_i2c_bus_addr(int *i2c_bus, ulong *i2c_addr, int argc,
-			      char *const argv[], int argc_no_bus_addr)
+static int parse_eeprom_dev_spec(struct eeprom_dev_spec *dev, int argc,
+				 char *const argv[])
 {
-	int argc_no_bus = argc_no_bus_addr + 1;
-	int argc_bus_addr = argc_no_bus_addr + 2;
-
 #ifdef CONFIG_SYS_I2C_EEPROM_ADDR
-	if (argc == argc_no_bus_addr) {
-		*i2c_bus = -1;
-		*i2c_addr = CONFIG_SYS_I2C_EEPROM_ADDR;
+	if (argc == 0) {
+		dev->i2c_bus = -1;
+		dev->i2c_addr = CONFIG_SYS_I2C_EEPROM_ADDR;
 
 		return 0;
 	}
 #endif
-	if (argc == argc_no_bus) {
-		*i2c_bus = -1;
-		*i2c_addr = parse_numeric_param(argv[0]);
+	if (argc == 1) {
+		dev->i2c_bus = -1;
+		dev->i2c_addr = parse_numeric_param(argv[0]);
 
 		return 1;
 	}
 
-	if (argc == argc_bus_addr) {
-		*i2c_bus = parse_numeric_param(argv[0]);
-		*i2c_addr = parse_numeric_param(argv[1]);
+	if (argc == 2) {
+		dev->i2c_bus = parse_numeric_param(argv[0]);
+		dev->i2c_addr = parse_numeric_param(argv[1]);
 
 		return 2;
 	}
@@ -287,9 +288,10 @@ static enum eeprom_action parse_action(char *cmd)
 	return EEPROM_ACTION_INVALID;
 }
 
-static int eeprom_execute_command(enum eeprom_action action, int i2c_bus,
-				  ulong i2c_addr, int layout_ver, char *key,
-				  char *value, ulong addr, ulong off, ulong cnt)
+static int eeprom_execute_command(enum eeprom_action action,
+				  struct eeprom_dev_spec *dev,
+				  int layout_ver, char *key, char *value,
+				  ulong addr, ulong off, ulong cnt)
 {
 	int rcode = 0;
 	const char *const fmt =
@@ -301,25 +303,26 @@ static int eeprom_execute_command(enum eeprom_action action, int i2c_bus,
 	if (action == EEPROM_ACTION_INVALID)
 		return CMD_RET_USAGE;
 
-	eeprom_init(i2c_bus);
+	eeprom_init(dev->i2c_bus);
 	if (action == EEPROM_READ) {
-		printf(fmt, i2c_addr, "read", addr, off, cnt);
+		printf(fmt, dev->i2c_addr, "read", addr, off, cnt);
 
-		rcode = eeprom_read(i2c_addr, off, (uchar *)addr, cnt);
+		rcode = eeprom_read(dev->i2c_addr, off, (uchar *)addr, cnt);
 
 		puts("done\n");
 		return rcode;
 	} else if (action == EEPROM_WRITE) {
-		printf(fmt, i2c_addr, "write", addr, off, cnt);
+		printf(fmt, dev->i2c_addr, "write", addr, off, cnt);
 
-		rcode = eeprom_write(i2c_addr, off, (uchar *)addr, cnt);
+		rcode = eeprom_write(dev->i2c_addr, off, (uchar *)addr, cnt);
 
 		puts("done\n");
 		return rcode;
 	}
 
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
-	rcode = eeprom_read(i2c_addr, 0, eeprom_buf, CONFIG_SYS_EEPROM_SIZE);
+	rcode = eeprom_read(dev->i2c_addr, 0, eeprom_buf,
+			    CONFIG_SYS_EEPROM_SIZE);
 	if (rcode < 0)
 		return rcode;
 
@@ -333,7 +336,8 @@ static int eeprom_execute_command(enum eeprom_action action, int i2c_bus,
 
 	layout.update(&layout, key, value);
 
-	rcode = eeprom_write(i2c_addr, 0, layout.data, CONFIG_SYS_EEPROM_SIZE);
+	rcode = eeprom_write(dev->i2c_addr, 0, layout.data,
+			     CONFIG_SYS_EEPROM_SIZE);
 #endif
 
 	return rcode;
@@ -359,9 +363,9 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	int layout_ver = LAYOUT_VERSION_AUTODETECT;
 	enum eeprom_action action = EEPROM_ACTION_INVALID;
-	int i2c_bus = -1, index = 0;
-	ulong i2c_addr = -1, addr = 0, cnt = 0, off = 0;
-	int ret;
+	struct eeprom_dev_spec dev;
+	ulong addr = 0, cnt = 0, off = 0;
+	int ret, index = 0;
 	char *field_name = "";
 	char *field_value = "";
 
@@ -386,8 +390,9 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	ret = parse_i2c_bus_addr(&i2c_bus, &i2c_addr, argc, argv + index,
-				 eeprom_action_expected_argc(action));
+	ret = parse_eeprom_dev_spec(&dev,
+				    argc - eeprom_action_expected_argc(action),
+				    argv + index);
 	if (ret == CMD_RET_USAGE)
 		return ret;
 
@@ -411,8 +416,8 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	return eeprom_execute_command(action, i2c_bus, i2c_addr, layout_ver,
-				      field_name, field_value, addr, off, cnt);
+	return eeprom_execute_command(action, &dev, layout_ver, field_name,
+				      field_value, addr, off, cnt);
 }
 
 #ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
-- 
2.44.1

[PATCH 09/11] cmd: eeprom: Refactor command execution into function by action

[Marek Behún]

Refactor the eeprom_execute_command() function into separate functions
do_eeprom_rw(), do_eeprom_print() and do_eeprom_update().

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/eeprom.c | 111 ++++++++++++++++++++++++++++++++-------------------
 1 file changed, 71 insertions(+), 40 deletions(-)

diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index e29639780d..c76cf43157 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -288,61 +288,75 @@ static enum eeprom_action parse_action(char *cmd)
 	return EEPROM_ACTION_INVALID;
 }
 
-static int eeprom_execute_command(enum eeprom_action action,
-				  struct eeprom_dev_spec *dev,
-				  int layout_ver, char *key, char *value,
-				  ulong addr, ulong off, ulong cnt)
+static int do_eeprom_rw(struct eeprom_dev_spec *dev, bool read,
+			ulong addr, ulong off, ulong cnt)
 {
-	int rcode = 0;
 	const char *const fmt =
 		"\nEEPROM @0x%lX %s: addr 0x%08lx  off 0x%04lx  count %ld ... ";
+	uchar *memloc = (uchar *)addr;
+	int ret;
+
+	printf(fmt, dev->i2c_addr, read ? "read" : "write", addr, off, cnt);
+	if (read)
+		ret = eeprom_read(dev->i2c_addr, off, memloc, cnt);
+	else
+		ret = eeprom_write(dev->i2c_addr, off, memloc, cnt);
+	puts("done\n");
+
+	return ret;
+}
+
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
-	struct eeprom_layout layout;
-#endif
 
-	if (action == EEPROM_ACTION_INVALID)
-		return CMD_RET_USAGE;
+static int do_eeprom_layout(struct eeprom_dev_spec *dev, int layout_ver,
+			    struct eeprom_layout *layout)
+{
+	int ret;
 
-	eeprom_init(dev->i2c_bus);
-	if (action == EEPROM_READ) {
-		printf(fmt, dev->i2c_addr, "read", addr, off, cnt);
+	ret = eeprom_read(dev->i2c_addr, 0, eeprom_buf, CONFIG_SYS_EEPROM_SIZE);
+	if (ret)
+		return ret;
 
-		rcode = eeprom_read(dev->i2c_addr, off, (uchar *)addr, cnt);
+	eeprom_layout_setup(layout, eeprom_buf, CONFIG_SYS_EEPROM_SIZE,
+			    layout_ver);
 
-		puts("done\n");
-		return rcode;
-	} else if (action == EEPROM_WRITE) {
-		printf(fmt, dev->i2c_addr, "write", addr, off, cnt);
+	return 0;
+}
 
-		rcode = eeprom_write(dev->i2c_addr, off, (uchar *)addr, cnt);
+static int do_eeprom_print(struct eeprom_dev_spec *dev, int layout_ver)
+{
+	struct eeprom_layout layout;
+	int ret;
 
-		puts("done\n");
-		return rcode;
-	}
+	ret = do_eeprom_layout(dev, layout_ver, &layout);
+	if (ret)
+		return ret;
 
-#ifdef CONFIG_CMD_EEPROM_LAYOUT
-	rcode = eeprom_read(dev->i2c_addr, 0, eeprom_buf,
-			    CONFIG_SYS_EEPROM_SIZE);
-	if (rcode < 0)
-		return rcode;
+	layout.print(&layout);
 
-	eeprom_layout_setup(&layout, eeprom_buf, CONFIG_SYS_EEPROM_SIZE,
-			    layout_ver);
+	return 0;
+}
 
-	if (action == EEPROM_PRINT) {
-		layout.print(&layout);
-		return 0;
-	}
+static int do_eeprom_update(struct eeprom_dev_spec *dev, int layout_ver,
+			    char *key, char *value)
+{
+	struct eeprom_layout layout;
+	int ret;
 
-	layout.update(&layout, key, value);
+	ret = do_eeprom_layout(dev, layout_ver, &layout);
+	if (ret)
+		return ret;
 
-	rcode = eeprom_write(dev->i2c_addr, 0, layout.data,
-			     CONFIG_SYS_EEPROM_SIZE);
-#endif
+	ret = layout.update(&layout, key, value);
+	if (ret)
+		return CMD_RET_FAILURE;
 
-	return rcode;
+	return eeprom_write(dev->i2c_addr, 0, layout.data,
+			    CONFIG_SYS_EEPROM_SIZE);
 }
 
+#endif
+
 static int eeprom_action_expected_argc(enum eeprom_action action)
 {
 	switch (action) {
@@ -361,13 +375,15 @@ static int eeprom_action_expected_argc(enum eeprom_action action)
 #define NEXT_PARAM(argc, index)	{ (argc)--; (index)++; }
 int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
-	int layout_ver = LAYOUT_VERSION_AUTODETECT;
 	enum eeprom_action action = EEPROM_ACTION_INVALID;
 	struct eeprom_dev_spec dev;
 	ulong addr = 0, cnt = 0, off = 0;
 	int ret, index = 0;
+#ifdef CONFIG_CMD_EEPROM_LAYOUT
 	char *field_name = "";
 	char *field_value = "";
+	int layout_ver = LAYOUT_VERSION_AUTODETECT;
+#endif
 
 	if (argc <= 1)
 		return CMD_RET_USAGE;
@@ -416,8 +432,23 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	return eeprom_execute_command(action, &dev, layout_ver, field_name,
-				      field_value, addr, off, cnt);
+	eeprom_init(dev.i2c_bus);
+
+	switch (action) {
+	case EEPROM_READ:
+	case EEPROM_WRITE:
+		return do_eeprom_rw(&dev, action == EEPROM_READ,
+				    addr, off, cnt);
+#ifdef CONFIG_CMD_EEPROM_LAYOUT
+	case EEPROM_PRINT:
+		return do_eeprom_print(&dev, layout_ver);
+	case EEPROM_UPDATE:
+		return do_eeprom_update(&dev, layout_ver,
+					field_name, field_value);
+#endif
+	default:
+		return CMD_RET_USAGE;
+	}
 }
 
 #ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
-- 
2.44.1

[PATCH 10/11] cmd: eeprom: Don't read/write whole EEPROM if not necessary

[Marek Behún]

Don't read/write whole EEPROM if not necessary when printing / updating
EEPROM layout fields. Only read/write layout.data_size bytes.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/eeprom.c | 11 ++---------
 1 file changed, 2 insertions(+), 9 deletions(-)

diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index c76cf43157..9c4af88738 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -311,16 +311,10 @@ static int do_eeprom_rw(struct eeprom_dev_spec *dev, bool read,
 static int do_eeprom_layout(struct eeprom_dev_spec *dev, int layout_ver,
 			    struct eeprom_layout *layout)
 {
-	int ret;
-
-	ret = eeprom_read(dev->i2c_addr, 0, eeprom_buf, CONFIG_SYS_EEPROM_SIZE);
-	if (ret)
-		return ret;
-
 	eeprom_layout_setup(layout, eeprom_buf, CONFIG_SYS_EEPROM_SIZE,
 			    layout_ver);
 
-	return 0;
+	return eeprom_read(dev->i2c_addr, 0, eeprom_buf, layout->data_size);
 }
 
 static int do_eeprom_print(struct eeprom_dev_spec *dev, int layout_ver)
@@ -351,8 +345,7 @@ static int do_eeprom_update(struct eeprom_dev_spec *dev, int layout_ver,
 	if (ret)
 		return CMD_RET_FAILURE;
 
-	return eeprom_write(dev->i2c_addr, 0, layout.data,
-			    CONFIG_SYS_EEPROM_SIZE);
+	return eeprom_write(dev->i2c_addr, 0, layout.data, layout.data_size);
 }
 
 #endif
-- 
2.44.1

[PATCH 11/11] cmd: eeprom: Extend to EEPROMs probed via driver model

[Marek Behún]

Extend the 'eeprom' command to allow accessing EEPROMs probed via
driver model, uclass UCLASS_I2C_EEPROM.

When the CONFIG_I2C_EEPROM config option is enabled (and so the
i2c-eeprom driver is built), the 'eeprom' command now accepts driver
model device name as EEPROM specifier for the 'eeprom' command, in
addition to the legacy [[bus] devaddr] specifier.

Moreover if no device specifier is given, then the first
UCLASS_I2C_EEPROM device is used, if found.

Signed-off-by: Marek Behún <kabel@kernel.org>
---
 cmd/eeprom.c | 122 ++++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 112 insertions(+), 10 deletions(-)

diff --git a/cmd/eeprom.c b/cmd/eeprom.c
index 9c4af88738..a39fc5ffdc 100644
--- a/cmd/eeprom.c
+++ b/cmd/eeprom.c
@@ -22,8 +22,10 @@
 #include <common.h>
 #include <config.h>
 #include <command.h>
+#include <dm.h>
 #include <eeprom.h>
 #include <i2c.h>
+#include <i2c_eeprom.h>
 #include <eeprom_layout.h>
 #include <linux/delay.h>
 
@@ -209,10 +211,41 @@ static long parse_numeric_param(char *str)
 }
 
 struct eeprom_dev_spec {
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	struct udevice *dev;
+#endif
 	int i2c_bus;
 	ulong i2c_addr;
 };
 
+static void eeprom_dev_spec_init(struct eeprom_dev_spec *dev)
+{
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (!dev->dev)
+#endif
+		eeprom_init(dev->i2c_bus);
+}
+
+static int eeprom_dev_spec_read(struct eeprom_dev_spec *dev,
+				unsigned offset, uchar *buffer, unsigned cnt)
+{
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (dev->dev)
+		return i2c_eeprom_read(dev->dev, offset, buffer, cnt);
+#endif
+	return eeprom_read(dev->i2c_addr, offset, buffer, cnt);
+}
+
+static int eeprom_dev_spec_write(struct eeprom_dev_spec *dev,
+				 unsigned offset, uchar *buffer, unsigned cnt)
+{
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (dev->dev)
+		return i2c_eeprom_write(dev->dev, offset, buffer, cnt);
+#endif
+	return eeprom_write(dev->i2c_addr, offset, buffer, cnt);
+}
+
 /**
  * parse_eeprom_dev_spec - parse the eeprom device specifier
  *
@@ -226,6 +259,28 @@ struct eeprom_dev_spec {
 static int parse_eeprom_dev_spec(struct eeprom_dev_spec *dev, int argc,
 				 char *const argv[])
 {
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (argc == 0) {
+		if (!uclass_first_device_err(UCLASS_I2C_EEPROM, &dev->dev))
+			return 0;
+	}
+
+	if (argc == 1) {
+		if (!uclass_get_device_by_name(UCLASS_I2C_EEPROM, argv[0],
+					       &dev->dev))
+			return 1;
+
+		/*
+		 * If we could not find the device by name and the parameter is
+		 * not numeric (and so won't be handled later), fail.
+		 */
+		if (parse_numeric_param(argv[0]) == -1) {
+			printf("Can't get eeprom device: %s\n", argv[0]);
+			return CMD_RET_USAGE;
+		}
+	}
+#endif
+
 #ifdef CONFIG_SYS_I2C_EEPROM_ADDR
 	if (argc == 0) {
 		dev->i2c_bus = -1;
@@ -265,6 +320,7 @@ static unsigned char eeprom_buf[CONFIG_SYS_EEPROM_SIZE];
 #endif
 
 enum eeprom_action {
+	EEPROM_LIST,
 	EEPROM_READ,
 	EEPROM_WRITE,
 	EEPROM_PRINT,
@@ -274,6 +330,10 @@ enum eeprom_action {
 
 static enum eeprom_action parse_action(char *cmd)
 {
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (!strncmp(cmd, "list", 4))
+		return EEPROM_LIST;
+#endif
 	if (!strncmp(cmd, "read", 4))
 		return EEPROM_READ;
 	if (!strncmp(cmd, "write", 5))
@@ -288,6 +348,24 @@ static enum eeprom_action parse_action(char *cmd)
 	return EEPROM_ACTION_INVALID;
 }
 
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+static int do_eeprom_list(void)
+{
+	struct udevice *dev;
+	struct uclass *uc;
+	int err;
+
+	err = uclass_get(UCLASS_I2C_EEPROM, &uc);
+	if (err)
+		return CMD_RET_FAILURE;
+
+	uclass_foreach_dev(dev, uc)
+		printf("%s (%s)\n", dev->name, dev->driver->name);
+
+	return CMD_RET_SUCCESS;
+}
+#endif
+
 static int do_eeprom_rw(struct eeprom_dev_spec *dev, bool read,
 			ulong addr, ulong off, ulong cnt)
 {
@@ -298,9 +376,9 @@ static int do_eeprom_rw(struct eeprom_dev_spec *dev, bool read,
 
 	printf(fmt, dev->i2c_addr, read ? "read" : "write", addr, off, cnt);
 	if (read)
-		ret = eeprom_read(dev->i2c_addr, off, memloc, cnt);
+		ret = eeprom_dev_spec_read(dev, off, memloc, cnt);
 	else
-		ret = eeprom_write(dev->i2c_addr, off, memloc, cnt);
+		ret = eeprom_dev_spec_write(dev, off, memloc, cnt);
 	puts("done\n");
 
 	return ret;
@@ -314,7 +392,7 @@ static int do_eeprom_layout(struct eeprom_dev_spec *dev, int layout_ver,
 	eeprom_layout_setup(layout, eeprom_buf, CONFIG_SYS_EEPROM_SIZE,
 			    layout_ver);
 
-	return eeprom_read(dev->i2c_addr, 0, eeprom_buf, layout->data_size);
+	return eeprom_dev_spec_read(dev, 0, eeprom_buf, layout->data_size);
 }
 
 static int do_eeprom_print(struct eeprom_dev_spec *dev, int layout_ver)
@@ -345,7 +423,7 @@ static int do_eeprom_update(struct eeprom_dev_spec *dev, int layout_ver,
 	if (ret)
 		return CMD_RET_FAILURE;
 
-	return eeprom_write(dev->i2c_addr, 0, layout.data, layout.data_size);
+	return eeprom_dev_spec_write(dev, 0, layout.data, layout.data_size);
 }
 
 #endif
@@ -353,6 +431,8 @@ static int do_eeprom_update(struct eeprom_dev_spec *dev, int layout_ver,
 static int eeprom_action_expected_argc(enum eeprom_action action)
 {
 	switch (action) {
+	case EEPROM_LIST:
+		return 0;
 	case EEPROM_READ:
 	case EEPROM_WRITE:
 		return 3;
@@ -389,6 +469,11 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (action == EEPROM_ACTION_INVALID)
 		return CMD_RET_USAGE;
 
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	if (action == EEPROM_LIST)
+		return do_eeprom_list();
+#endif
+
 #ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
 	if (action == EEPROM_PRINT || action == EEPROM_UPDATE) {
 		if (!strcmp(argv[index], "-l")) {
@@ -425,7 +510,7 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	}
 #endif
 
-	eeprom_init(dev.i2c_bus);
+	eeprom_dev_spec_init(&dev);
 
 	switch (action) {
 	case EEPROM_READ:
@@ -450,19 +535,37 @@ int do_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 #define EEPROM_LAYOUT_SPEC	""
 #endif
 
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+# define EEPROM_DEV_SPEC	"[device_specifier]"
+#else
+# define EEPROM_DEV_SPEC	"[[bus] devaddr]"
+#endif
+
 U_BOOT_CMD(
 	eeprom,	8,	1,	do_eeprom,
 	"EEPROM sub-system",
-	"read  [[bus] devaddr] addr off cnt\n"
-	"eeprom write [[bus] devaddr] addr off cnt\n"
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	"list\n"
+	"eeprom "
+#endif
+	"read  " EEPROM_DEV_SPEC " addr off cnt\n"
+	"eeprom write " EEPROM_DEV_SPEC " addr off cnt\n"
 	"       - read/write `cnt' bytes from `devaddr` EEPROM at offset `off'"
 #ifdef CONFIG_CMD_EEPROM_LAYOUT
 	"\n"
-	"eeprom print " EEPROM_LAYOUT_SPEC "[[bus] devaddr]\n"
+	"eeprom print " EEPROM_LAYOUT_SPEC EEPROM_DEV_SPEC "\n"
 	"       - Print layout fields and their data in human readable format\n"
-	"eeprom update " EEPROM_LAYOUT_SPEC "[[bus] devaddr] field_name field_value\n"
+	"eeprom update " EEPROM_LAYOUT_SPEC EEPROM_DEV_SPEC " field_name field_value\n"
 	"       - Update a specific eeprom field with new data.\n"
 	"         The new data must be written in the same human readable format as shown by the print command."
+#endif
+#if CONFIG_IS_ENABLED(I2C_EEPROM)
+	"\n\n"
+	"DEVICE SPECIFIER - the eeprom device can be specified\n"
+	"  [dev_name] - by device name (devices can listed with the eeprom list command)\n"
+	"  [[bus] devaddr] - or by I2C bus and I2C device address\n"
+	"If no device specifier is given, the first driver-model found device is used."
+#endif
 #ifdef CONFIG_EEPROM_LAYOUT_VERSIONS
 	"\n\n"
 	"LAYOUT VERSIONS\n"
@@ -471,5 +574,4 @@ U_BOOT_CMD(
 	"The values which can be provided with the -l option are:\n"
 	CONFIG_EEPROM_LAYOUT_HELP_STRING"\n"
 #endif
-#endif
 );
-- 
2.44.1

Re: [PATCH 00/11] 'eeprom' command improvements

[Tom Rini]

On Tue, 21 May 2024 09:13:24 +0200, Marek Behún wrote:

> this series contains improvements for the 'eeprom' command:
> - refactors
> - fixes
> - improvements
> - ability to use driver model EEPROMs (uclass UCLASS_I2C_EEPROM)
> - more flexible EEPROM layout support
> 
> [...]

Applied to u-boot/next, thanks!

-- 
Tom