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

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(-)

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

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);
return 0; }show_eeprom(current_dev, eeprom);
@@ -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))
return 0; }if (!read_eeprom(current_dev, eeprom)) printf("EEPROM data loaded from device to memory.\n");
@@ -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);
case 'e': /* erase */ strcpy(eeprom_hdr->signature, TLV_INFO_ID_STRING);prog_eeprom(current_dev, eeprom); break;
@@ -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();
default: return CMD_RET_USAGE;show_tlv_devices(current_dev); break;
@@ -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

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; }

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.

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; }

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

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
participants (3)
-
Josua Mayer
-
Simon Glass
-
Stefan Roese