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

[PATCH 0/3] 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.

Josua Mayer (3):
  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 | 61 +++++++++++++++++++++++++++++++-----------------
 1 file changed, 39 insertions(+), 22 deletions(-)

-- 
2.35.3

[PATCH 1/3] 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>
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

Re: [PATCH 1/3] cmd: tlv_eeprom: remove use of global variable has_been_read

[Stefan Roese]

On 4/29/23 11:15, Josua Mayer wrote:
> 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>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

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

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

Re: [PATCH 1/3] cmd: tlv_eeprom: remove use of global variable has_been_read

[Stefan Roese]

Hi Josua,

On 4/29/23 11:15, Josua Mayer wrote:
> 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>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

This patchset does not apply to master. Could you please check, rebase
and send again?

Thanks,
Stefan

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

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

Re: [PATCH 1/3] cmd: tlv_eeprom: remove use of global variable has_been_read

[Josua Mayer]

Hi Stefan,

Thanks for reviewing. I just rebased on 
https://source.denx.de/u-boot/u-boot.git / master,
and will send new version soon.

- Josua Mayer

Am 03.05.23 um 09:55 schrieb Stefan Roese:
> Hi Josua,
>
> On 4/29/23 11:15, Josua Mayer wrote:
>> 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>
>> Cc: Stefan Roese <sr@denx.de>
>> Cc: Baruch Siach <baruch@tkos.co.il>
>> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>
>
> This patchset does not apply to master. Could you please check, rebase
> and send again?
>
> Thanks,
> Stefan
>
>> ---
>>   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;
>>       }
>
> Viele Grüße,
> Stefan Roese
>

[PATCH 2/3] 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>
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

Re: [PATCH 2/3] cmd: tlv_eeprom: handle -ENODEV error from read_eeprom function

[Stefan Roese]

On 4/29/23 11:15, Josua Mayer wrote:
> 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>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

> ---
>   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.

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 3/3] 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>
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 3/3] cmd: tlv_eeprom: enable 'dev' subcommand before 'read'

[Stefan Roese]

On 4/29/23 11:15, Josua Mayer 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>
> Cc: Stefan Roese <sr@denx.de>
> Cc: Baruch Siach <baruch@tkos.co.il>
> Cc: Heinrich Schuchardt <xypron.glpk@gmx.de>

Reviewed-by: Stefan Roese <sr@denx.de>

Thanks,
Stefan

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

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