[[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup

[[PATCH v2] 0/4] cmd: tlv_eeprom: global variables and error cleanup

[Josua Mayer]

This patch-set removes some uses of global variables, and improves error
reporting for the "read" command.
It is intended to help switching to a split tlv library eventually,
but general enough to apply independently.

Changes since v1:
- rebased on master 0160d58
- added first patch!!! I had forgotten to send in v1
- added reviewed-tags by Stefan for patches 2-4

Josua Mayer (4):
  cmd: tlv_eeprom: remove use of global variable current_dev
  cmd: tlv_eeprom: remove use of global variable has_been_read
  cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function
  cmd: tlv_eeprom: enable 'dev' subcommand before 'read'

 cmd/tlv_eeprom.c     | 107 +++++++++++++++++++++++++------------------
 include/tlv_eeprom.h |   3 +-
 2 files changed, 64 insertions(+), 46 deletions(-)

-- 
2.35.3

[[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev

[Josua Mayer]

Make tlv_eeprom command device selection an explicit parameter of all
function calls.

Signed-off-by: Josua Mayer <josua@solid-run.com>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/tlv_eeprom.c     | 50 ++++++++++++++++++++++----------------------
 include/tlv_eeprom.h |  3 ++-
 2 files changed, 27 insertions(+), 26 deletions(-)

diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 4591ff336bb..8049bf9843c 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -29,18 +29,18 @@ DECLARE_GLOBAL_DATA_PTR;
 
 /* File scope function prototypes */
 static bool is_checksum_valid(u8 *eeprom);
-static int read_eeprom(u8 *eeprom);
-static void show_eeprom(u8 *eeprom);
+static int read_eeprom(int devnum, u8 *eeprom);
+static void show_eeprom(int devnum, u8 *eeprom);
 static void decode_tlv(struct tlvinfo_tlv *tlv);
 static void update_crc(u8 *eeprom);
-static int prog_eeprom(u8 *eeprom);
+static int prog_eeprom(int devnum, u8 *eeprom);
 static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
 static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
 static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
 static int set_mac(char *buf, const char *string);
 static int set_date(char *buf, const char *string);
 static int set_bytes(char *buf, const char *string, int *converted_accum);
-static void show_tlv_devices(void);
+static void show_tlv_devices(int current_dev);
 
 /* Set to 1 if we've read EEPROM into memory */
 static int has_been_read;
@@ -48,7 +48,6 @@ static int has_been_read;
 static u8 eeprom[TLV_INFO_MAX_LEN];
 
 static struct udevice *tlv_devices[MAX_TLV_DEVICES];
-static unsigned int current_dev;
 
 #define to_header(p) ((struct tlvinfo_header *)p)
 #define to_entry(p) ((struct tlvinfo_tlv *)p)
@@ -125,7 +124,7 @@ static bool is_checksum_valid(u8 *eeprom)
  *
  *  Read the EEPROM into memory, if it hasn't already been read.
  */
-static int read_eeprom(u8 *eeprom)
+static int read_eeprom(int devnum, u8 *eeprom)
 {
 	int ret;
 	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
@@ -135,12 +134,11 @@ static int read_eeprom(u8 *eeprom)
 		return 0;
 
 	/* Read the header */
-	ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, current_dev);
+	ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum);
 	/* If the header was successfully read, read the TLVs */
 	if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr))
 		ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE,
-				      be16_to_cpu(eeprom_hdr->totallen),
-				      current_dev);
+				      be16_to_cpu(eeprom_hdr->totallen), devnum);
 
 	// If the contents are invalid, start over with default contents
 	if (!is_valid_tlvinfo_header(eeprom_hdr) ||
@@ -165,7 +163,7 @@ static int read_eeprom(u8 *eeprom)
  *
  *  Display the contents of the EEPROM
  */
-static void show_eeprom(u8 *eeprom)
+static void show_eeprom(int devnum, u8 *eeprom)
 {
 	int tlv_end;
 	int curr_tlv;
@@ -180,7 +178,7 @@ static void show_eeprom(u8 *eeprom)
 		return;
 	}
 
-	printf("TLV: %u\n", current_dev);
+	printf("TLV: %u\n", devnum);
 	printf("TlvInfo Header:\n");
 	printf("   Id String:    %s\n", eeprom_hdr->signature);
 	printf("   Version:      %d\n", eeprom_hdr->version);
@@ -389,7 +387,7 @@ static void update_crc(u8 *eeprom)
  *
  *  Write the EEPROM data from CPU memory to the hardware.
  */
-static int prog_eeprom(u8 *eeprom)
+static int prog_eeprom(int devnum, u8 *eeprom)
 {
 	int ret = 0;
 	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
@@ -398,7 +396,7 @@ static int prog_eeprom(u8 *eeprom)
 	update_crc(eeprom);
 
 	eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
-	ret = write_tlv_eeprom(eeprom, eeprom_len);
+	ret = write_tlv_eeprom(eeprom, eeprom_len, devnum);
 	if (ret) {
 		printf("Programming failed.\n");
 		return -1;
@@ -433,11 +431,12 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	char cmd;
 	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
+	static unsigned int current_dev;
 
 	// If no arguments, read the EERPOM and display its contents
 	if (argc == 1) {
-		read_eeprom(eeprom);
-		show_eeprom(eeprom);
+		read_eeprom(current_dev, eeprom);
+		show_eeprom(current_dev, eeprom);
 		return 0;
 	}
 
@@ -448,7 +447,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	// Read the EEPROM contents
 	if (cmd == 'r') {
 		has_been_read = 0;
-		if (!read_eeprom(eeprom))
+		if (!read_eeprom(current_dev, eeprom))
 			printf("EEPROM data loaded from device to memory.\n");
 		return 0;
 	}
@@ -463,7 +462,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (argc == 2) {
 		switch (cmd) {
 		case 'w':   /* write */
-			prog_eeprom(eeprom);
+			prog_eeprom(current_dev, eeprom);
 			break;
 		case 'e':   /* erase */
 			strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
@@ -476,7 +475,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 			show_tlv_code_list();
 			break;
 		case 'd':   /* dev */
-			show_tlv_devices();
+			show_tlv_devices(current_dev);
 			break;
 		default:
 			return CMD_RET_USAGE;
@@ -883,7 +882,7 @@ static int set_bytes(char *buf, const char *string, int *converted_accum)
 	return 0;
 }
 
-static void show_tlv_devices(void)
+static void show_tlv_devices(int current_dev)
 {
 	unsigned int dev;
 
@@ -953,14 +952,14 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num)
 /**
  * write_tlv_eeprom - write the hwinfo to i2c EEPROM
  */
-int write_tlv_eeprom(void *eeprom, int len)
+int write_tlv_eeprom(void *eeprom, int len, int dev)
 {
 	if (!(gd->flags & GD_FLG_RELOC))
 		return -ENODEV;
-	if (!tlv_devices[current_dev])
+	if (!tlv_devices[dev])
 		return -ENODEV;
 
-	return i2c_eeprom_write(tlv_devices[current_dev], 0, eeprom, len);
+	return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len);
 }
 
 int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr,
@@ -1015,10 +1014,11 @@ int mac_read_from_eeprom(void)
 	int maccount;
 	u8 macbase[6];
 	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
+	int devnum = 0; // TODO: support multiple EEPROMs
 
 	puts("EEPROM: ");
 
-	if (read_eeprom(eeprom)) {
+	if (read_eeprom(devnum, eeprom)) {
 		printf("Read failed.\n");
 		return -1;
 	}
@@ -1083,7 +1083,7 @@ int mac_read_from_eeprom(void)
  *
  *  This function must be called after relocation.
  */
-int populate_serial_number(void)
+int populate_serial_number(int devnum)
 {
 	char serialstr[257];
 	int eeprom_index;
@@ -1092,7 +1092,7 @@ int populate_serial_number(void)
 	if (env_get("serial#"))
 		return 0;
 
-	if (read_eeprom(eeprom)) {
+	if (read_eeprom(devnum, eeprom)) {
 		printf("Read failed.\n");
 		return -1;
 	}
diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h
index a2c333e7446..fd45e5f6ebb 100644
--- a/include/tlv_eeprom.h
+++ b/include/tlv_eeprom.h
@@ -84,11 +84,12 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev);
  * write_tlv_eeprom - Write the entire EEPROM binary data to the hardware
  * @eeprom: Pointer to buffer to hold the binary data
  * @len   : Maximum size of buffer
+ * @dev   : EEPROM device to write
  *
  * Note: this routine does not validate the EEPROM data.
  *
  */
-int write_tlv_eeprom(void *eeprom, int len);
+int write_tlv_eeprom(void *eeprom, int len, int dev);
 
 /**
  * read_tlvinfo_tlv_eeprom - Read the TLV from EEPROM, and validate
-- 
2.35.3

Re: [[PATCH v2] 1/4] cmd: tlv_eeprom: remove use of global variable current_dev

[Stefan Roese]

On 5/5/23 10:20, Josua Mayer wrote:
> Make tlv_eeprom command device selection an explicit parameter of all
> function calls.
> 
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

All patches:

Applied to u-boot-marvell/master

Thanks,
Stefan

> ---
>   cmd/tlv_eeprom.c     | 50 ++++++++++++++++++++++----------------------
>   include/tlv_eeprom.h |  3 ++-
>   2 files changed, 27 insertions(+), 26 deletions(-)
> 
> diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
> index 4591ff336bb..8049bf9843c 100644
> --- a/cmd/tlv_eeprom.c
> +++ b/cmd/tlv_eeprom.c
> @@ -29,18 +29,18 @@ DECLARE_GLOBAL_DATA_PTR;
>   
>   /* File scope function prototypes */
>   static bool is_checksum_valid(u8 *eeprom);
> -static int read_eeprom(u8 *eeprom);
> -static void show_eeprom(u8 *eeprom);
> +static int read_eeprom(int devnum, u8 *eeprom);
> +static void show_eeprom(int devnum, u8 *eeprom);
>   static void decode_tlv(struct tlvinfo_tlv *tlv);
>   static void update_crc(u8 *eeprom);
> -static int prog_eeprom(u8 *eeprom);
> +static int prog_eeprom(int devnum, u8 *eeprom);
>   static bool tlvinfo_find_tlv(u8 *eeprom, u8 tcode, int *eeprom_index);
>   static bool tlvinfo_delete_tlv(u8 *eeprom, u8 code);
>   static bool tlvinfo_add_tlv(u8 *eeprom, int tcode, char *strval);
>   static int set_mac(char *buf, const char *string);
>   static int set_date(char *buf, const char *string);
>   static int set_bytes(char *buf, const char *string, int *converted_accum);
> -static void show_tlv_devices(void);
> +static void show_tlv_devices(int current_dev);
>   
>   /* Set to 1 if we've read EEPROM into memory */
>   static int has_been_read;
> @@ -48,7 +48,6 @@ static int has_been_read;
>   static u8 eeprom[TLV_INFO_MAX_LEN];
>   
>   static struct udevice *tlv_devices[MAX_TLV_DEVICES];
> -static unsigned int current_dev;
>   
>   #define to_header(p) ((struct tlvinfo_header *)p)
>   #define to_entry(p) ((struct tlvinfo_tlv *)p)
> @@ -125,7 +124,7 @@ static bool is_checksum_valid(u8 *eeprom)
>    *
>    *  Read the EEPROM into memory, if it hasn't already been read.
>    */
> -static int read_eeprom(u8 *eeprom)
> +static int read_eeprom(int devnum, u8 *eeprom)
>   {
>   	int ret;
>   	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> @@ -135,12 +134,11 @@ static int read_eeprom(u8 *eeprom)
>   		return 0;
>   
>   	/* Read the header */
> -	ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, current_dev);
> +	ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum);
>   	/* If the header was successfully read, read the TLVs */
>   	if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr))
>   		ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE,
> -				      be16_to_cpu(eeprom_hdr->totallen),
> -				      current_dev);
> +				      be16_to_cpu(eeprom_hdr->totallen), devnum);
>   
>   	// If the contents are invalid, start over with default contents
>   	if (!is_valid_tlvinfo_header(eeprom_hdr) ||
> @@ -165,7 +163,7 @@ static int read_eeprom(u8 *eeprom)
>    *
>    *  Display the contents of the EEPROM
>    */
> -static void show_eeprom(u8 *eeprom)
> +static void show_eeprom(int devnum, u8 *eeprom)
>   {
>   	int tlv_end;
>   	int curr_tlv;
> @@ -180,7 +178,7 @@ static void show_eeprom(u8 *eeprom)
>   		return;
>   	}
>   
> -	printf("TLV: %u\n", current_dev);
> +	printf("TLV: %u\n", devnum);
>   	printf("TlvInfo Header:\n");
>   	printf("   Id String:    %s\n", eeprom_hdr->signature);
>   	printf("   Version:      %d\n", eeprom_hdr->version);
> @@ -389,7 +387,7 @@ static void update_crc(u8 *eeprom)
>    *
>    *  Write the EEPROM data from CPU memory to the hardware.
>    */
> -static int prog_eeprom(u8 *eeprom)
> +static int prog_eeprom(int devnum, u8 *eeprom)
>   {
>   	int ret = 0;
>   	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> @@ -398,7 +396,7 @@ static int prog_eeprom(u8 *eeprom)
>   	update_crc(eeprom);
>   
>   	eeprom_len = HDR_SIZE + be16_to_cpu(eeprom_hdr->totallen);
> -	ret = write_tlv_eeprom(eeprom, eeprom_len);
> +	ret = write_tlv_eeprom(eeprom, eeprom_len, devnum);
>   	if (ret) {
>   		printf("Programming failed.\n");
>   		return -1;
> @@ -433,11 +431,12 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   {
>   	char cmd;
>   	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> +	static unsigned int current_dev;
>   
>   	// If no arguments, read the EERPOM and display its contents
>   	if (argc == 1) {
> -		read_eeprom(eeprom);
> -		show_eeprom(eeprom);
> +		read_eeprom(current_dev, eeprom);
> +		show_eeprom(current_dev, eeprom);
>   		return 0;
>   	}
>   
> @@ -448,7 +447,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	// Read the EEPROM contents
>   	if (cmd == 'r') {
>   		has_been_read = 0;
> -		if (!read_eeprom(eeprom))
> +		if (!read_eeprom(current_dev, eeprom))
>   			printf("EEPROM data loaded from device to memory.\n");
>   		return 0;
>   	}
> @@ -463,7 +462,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   	if (argc == 2) {
>   		switch (cmd) {
>   		case 'w':   /* write */
> -			prog_eeprom(eeprom);
> +			prog_eeprom(current_dev, eeprom);
>   			break;
>   		case 'e':   /* erase */
>   			strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);
> @@ -476,7 +475,7 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>   			show_tlv_code_list();
>   			break;
>   		case 'd':   /* dev */
> -			show_tlv_devices();
> +			show_tlv_devices(current_dev);
>   			break;
>   		default:
>   			return CMD_RET_USAGE;
> @@ -883,7 +882,7 @@ static int set_bytes(char *buf, const char *string, int *converted_accum)
>   	return 0;
>   }
>   
> -static void show_tlv_devices(void)
> +static void show_tlv_devices(int current_dev)
>   {
>   	unsigned int dev;
>   
> @@ -953,14 +952,14 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev_num)
>   /**
>    * write_tlv_eeprom - write the hwinfo to i2c EEPROM
>    */
> -int write_tlv_eeprom(void *eeprom, int len)
> +int write_tlv_eeprom(void *eeprom, int len, int dev)
>   {
>   	if (!(gd->flags & GD_FLG_RELOC))
>   		return -ENODEV;
> -	if (!tlv_devices[current_dev])
> +	if (!tlv_devices[dev])
>   		return -ENODEV;
>   
> -	return i2c_eeprom_write(tlv_devices[current_dev], 0, eeprom, len);
> +	return i2c_eeprom_write(tlv_devices[dev], 0, eeprom, len);
>   }
>   
>   int read_tlvinfo_tlv_eeprom(void *eeprom, struct tlvinfo_header **hdr,
> @@ -1015,10 +1014,11 @@ int mac_read_from_eeprom(void)
>   	int maccount;
>   	u8 macbase[6];
>   	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
> +	int devnum = 0; // TODO: support multiple EEPROMs
>   
>   	puts("EEPROM: ");
>   
> -	if (read_eeprom(eeprom)) {
> +	if (read_eeprom(devnum, eeprom)) {
>   		printf("Read failed.\n");
>   		return -1;
>   	}
> @@ -1083,7 +1083,7 @@ int mac_read_from_eeprom(void)
>    *
>    *  This function must be called after relocation.
>    */
> -int populate_serial_number(void)
> +int populate_serial_number(int devnum)
>   {
>   	char serialstr[257];
>   	int eeprom_index;
> @@ -1092,7 +1092,7 @@ int populate_serial_number(void)
>   	if (env_get("serial#"))
>   		return 0;
>   
> -	if (read_eeprom(eeprom)) {
> +	if (read_eeprom(devnum, eeprom)) {
>   		printf("Read failed.\n");
>   		return -1;
>   	}
> diff --git a/include/tlv_eeprom.h b/include/tlv_eeprom.h
> index a2c333e7446..fd45e5f6ebb 100644
> --- a/include/tlv_eeprom.h
> +++ b/include/tlv_eeprom.h
> @@ -84,11 +84,12 @@ int read_tlv_eeprom(void *eeprom, int offset, int len, int dev);
>    * write_tlv_eeprom - Write the entire EEPROM binary data to the hardware
>    * @eeprom: Pointer to buffer to hold the binary data
>    * @len   : Maximum size of buffer
> + * @dev   : EEPROM device to write
>    *
>    * Note: this routine does not validate the EEPROM data.
>    *
>    */
> -int write_tlv_eeprom(void *eeprom, int len);
> +int write_tlv_eeprom(void *eeprom, int len, int dev);
>   
>   /**
>    * read_tlvinfo_tlv_eeprom - Read the TLV from EEPROM, and validate

Viele Grüße,
Stefan Roese

-- 
DENX Software Engineering GmbH,      Managing Director: Erika Unter
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr@denx.de

[[PATCH v2] 2/4] cmd: tlv_eeprom: remove use of global variable has_been_read

[Josua Mayer]

has_been_read is only used as an optimization for do_tlv_eeprom.
Explicitly use and set inside this function, thus making read_eeprom
stateless.

Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/tlv_eeprom.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 8049bf9843c..d36401e9138 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -42,8 +42,6 @@ static int set_date(char *buf, const char *string);
 static int set_bytes(char *buf, const char *string, int *converted_accum);
 static void show_tlv_devices(int current_dev);
 
-/* Set to 1 if we've read EEPROM into memory */
-static int has_been_read;
 /* The EERPOM contents after being read into memory */
 static u8 eeprom[TLV_INFO_MAX_LEN];
 
@@ -130,9 +128,6 @@ static int read_eeprom(int devnum, u8 *eeprom)
 	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
 	struct tlvinfo_tlv *eeprom_tlv = to_entry(&eeprom[HDR_SIZE]);
 
-	if (has_been_read)
-		return 0;
-
 	/* Read the header */
 	ret = read_tlv_eeprom((void *)eeprom_hdr, 0, HDR_SIZE, devnum);
 	/* If the header was successfully read, read the TLVs */
@@ -149,10 +144,8 @@ static int read_eeprom(int devnum, u8 *eeprom)
 		update_crc(eeprom);
 	}
 
-	has_been_read = 1;
-
 #ifdef DEBUG
-	show_eeprom(eeprom);
+	show_eeprom(devnum, eeprom);
 #endif
 
 	return ret;
@@ -432,10 +425,16 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	char cmd;
 	struct tlvinfo_header *eeprom_hdr = to_header(eeprom);
 	static unsigned int current_dev;
+	/* Set to 1 if we've read EEPROM into memory */
+	static int has_been_read;
+	int ret;
 
 	// If no arguments, read the EERPOM and display its contents
 	if (argc == 1) {
-		read_eeprom(current_dev, eeprom);
+		if (!has_been_read) {
+			if (read_eeprom(current_dev, eeprom) == 0)
+				has_been_read = 1;
+		}
 		show_eeprom(current_dev, eeprom);
 		return 0;
 	}
@@ -447,8 +446,10 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	// Read the EEPROM contents
 	if (cmd == 'r') {
 		has_been_read = 0;
-		if (!read_eeprom(current_dev, eeprom))
+		if (read_eeprom(current_dev, eeprom) == 0) {
 			printf("EEPROM data loaded from device to memory.\n");
+			has_been_read = 1;
+		}
 		return 0;
 	}
 
-- 
2.35.3

[[PATCH v2] 3/4] cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function

[Josua Mayer]

When tlv eeprom does not exist, return error code instead of quietly
making up tlv structure in memory.

Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/tlv_eeprom.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index d36401e9138..636c1fe32ef 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -134,6 +134,8 @@ static int read_eeprom(int devnum, u8 *eeprom)
 	if (ret == 0 && is_valid_tlvinfo_header(eeprom_hdr))
 		ret = read_tlv_eeprom((void *)eeprom_tlv, HDR_SIZE,
 				      be16_to_cpu(eeprom_hdr->totallen), devnum);
+	else if (ret == -ENODEV)
+		return ret;
 
 	// If the contents are invalid, start over with default contents
 	if (!is_valid_tlvinfo_header(eeprom_hdr) ||
@@ -432,8 +434,13 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	// If no arguments, read the EERPOM and display its contents
 	if (argc == 1) {
 		if (!has_been_read) {
-			if (read_eeprom(current_dev, eeprom) == 0)
-				has_been_read = 1;
+			ret = read_eeprom(current_dev, eeprom);
+			if (ret) {
+				printf("Failed to read EEPROM data from device.\n");
+				return 0;
+			}
+
+			has_been_read = 1;
 		}
 		show_eeprom(current_dev, eeprom);
 		return 0;
@@ -446,11 +453,14 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	// Read the EEPROM contents
 	if (cmd == 'r') {
 		has_been_read = 0;
-		if (read_eeprom(current_dev, eeprom) == 0) {
-			printf("EEPROM data loaded from device to memory.\n");
-			has_been_read = 1;
+		ret = read_eeprom(current_dev, eeprom);
+		if (ret) {
+			printf("Failed to read EEPROM data from device.\n");
+			return 0;
 		}
-		return 0;
+
+		printf("EEPROM data loaded from device to memory.\n");
+		has_been_read = 1;
 	}
 
 	// Subsequent commands require that the EEPROM has already been read.
-- 
2.35.3

[[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'

[Josua Mayer]

Move the handler for "tlv_eeprom dev X" command to the beginning of
do_tlv_eeprom, to allow using it before issuing a "read" command for
currently selected eeprom.

Also remove the check if eeprom exists, since that can only work after
the first execution of read_eeprom triggered device lookup.
Instead accept values up to the defined array size (MAX_TLV_DEVICES).

Signed-off-by: Josua Mayer <josua@solid-run.com>
Reviewed-by: Stefan Roese <sr@denx.de>
Cc: Stefan Roese <sr@denx.de>
Cc: Baruch Siach <baruch@tkos.co.il>
Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 cmd/tlv_eeprom.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c
index 636c1fe32ef..79796394c5c 100644
--- a/cmd/tlv_eeprom.c
+++ b/cmd/tlv_eeprom.c
@@ -450,6 +450,22 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	// "reset" will both be treated as "read".
 	cmd = argv[1][0];
 
+	// select device
+	if (cmd == 'd') {
+		 /* 'dev' command */
+		unsigned int devnum;
+
+		devnum = simple_strtoul(argv[2], NULL, 0);
+		if (devnum >= MAX_TLV_DEVICES) {
+			printf("Invalid device number\n");
+			return 0;
+		}
+		current_dev = devnum;
+		has_been_read = 0;
+
+		return 0;
+	}
+
 	// Read the EEPROM contents
 	if (cmd == 'r') {
 		has_been_read = 0;
@@ -508,16 +524,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 		tlvinfo_delete_tlv(eeprom, tcode);
 		if (argc == 4)
 			tlvinfo_add_tlv(eeprom, tcode, argv[3]);
-	} else if (cmd == 'd') { /* 'dev' command */
-		unsigned int devnum;
-
-		devnum = simple_strtoul(argv[2], NULL, 0);
-		if (devnum > MAX_TLV_DEVICES || !tlv_devices[devnum]) {
-			printf("Invalid device number\n");
-			return 0;
-		}
-		current_dev = devnum;
-		has_been_read = 0;
 	} else {
 		return CMD_RET_USAGE;
 	}
-- 
2.35.3

Re: [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'

[Simon Glass]

Hi,

On Fri, 5 May 2023 at 02:22, Josua Mayer <josua@solid-run.com> wrote:
>
> Move the handler for "tlv_eeprom dev X" command to the beginning of
> do_tlv_eeprom, to allow using it before issuing a "read" command for
> currently selected eeprom.
>
> Also remove the check if eeprom exists, since that can only work after
> the first execution of read_eeprom triggered device lookup.
> Instead accept values up to the defined array size (MAX_TLV_DEVICES).
>
> Signed-off-by: Josua Mayer <josua@solid-run.com>
> Reviewed-by: Stefan Roese <sr@denx.de>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
> ---
>  cmd/tlv_eeprom.c | 26 ++++++++++++++++----------
>  1 file changed, 16 insertions(+), 10 deletions(-)

Can someone take a look at fixing up the driver model code?

For example:
- it maintains its own array of eproms, when it should use driver
model to find them.
- the has_been_read flag seems to track only one EEPROM, but the code
indicates there might be two (should be in the device's private data)
- current_dev should be a pointer to a device
- ideally all state should be either in the device or the uclass, so
it can be used before relocation, etc.

Regards,
Simon

Re: [[PATCH v2] 4/4] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'

[Josua Mayer]

Hi Simon,

I have a private rewrite of the tlv functionality as a seperate library 
outside of the command implementation.
Originally I had hoped to finish upstreaming that about a year ago but 
got sidetracked.

Hopefully within the coming weeks I can submit another version as RFC 
that can address your concerns more easily than the current eeprom_tlv 
implementation.

Sincerely
Josua Mayer

Am 08.05.23 um 17:42 schrieb Simon Glass:
> Hi,
>
> On Fri, 5 May 2023 at 02:22, Josua Mayer <josua@solid-run.com> wrote:
>> Move the handler for "tlv_eeprom dev X" command to the beginning of
>> do_tlv_eeprom, to allow using it before issuing a "read" command for
>> currently selected eeprom.
>>
>> Also remove the check if eeprom exists, since that can only work after
>> the first execution of read_eeprom triggered device lookup.
>> Instead accept values up to the defined array size (MAX_TLV_DEVICES).
>>
>> Signed-off-by: Josua Mayer <josua@solid-run.com>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Baruch Siach <baruch@tkos.co.il>
>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>> ---
>>   cmd/tlv_eeprom.c | 26 ++++++++++++++++----------
>>   1 file changed, 16 insertions(+), 10 deletions(-)
> Can someone take a look at fixing up the driver model code?
>
> For example:
> - it maintains its own array of eproms, when it should use driver
> model to find them.
> - the has_been_read flag seems to track only one EEPROM, but the code
> indicates there might be two (should be in the device's private data)
> - current_dev should be a pointer to a device
> - ideally all state should be either in the device or the uclass, so
> it can be used before relocation, etc.
>
> Regards,
> Simon