Re: [PATCH 02/12] cmd: tlv_eeprom: remove use of global variable has_been_read

On 02.05.22 16:18, 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

cmd/tlv_eeprom.c | 25 ++++++++++++------------- 1 file changed, 12 insertions(+), 13 deletions(-)

diff --git a/cmd/tlv_eeprom.c b/cmd/tlv_eeprom.c index f91c11b304..bfd4882e0d 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,15 @@ 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 devnum if we've read EEPROM into memory */
• static int has_been_read = -1;

You are not introducing this variable but still it would be "better" to change it to bool instead IMHO.

Other than this:

Reviewed-by: Stefan Roese sr@denx.de

Thanks, Stefan

// If no arguments, read the EERPOM and display its contents if (argc == 1) {

• read_eeprom(current_dev, eeprom);
  

• if (has_been_read != current_dev) {
  

• 	if (read_eeprom(current_dev, eeprom) == 0)
  

• 		has_been_read = current_dev;
  

• }
  

  show_eeprom(current_dev, eeprom); return 0; }

@@ -446,14 +444,16 @@ 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))
  

• has_been_read = -1;
  

• if (read_eeprom(current_dev, eeprom) == 0) {
  printf("EEPROM data loaded from device to memory.\n");
  

• 	has_been_read = current_dev;
  

• }
  

  return 0; }

  // Subsequent commands require that the EEPROM has already been read.

• if (!has_been_read) {

• if (has_been_read != current_dev) { printf("Please read the EEPROM data first, using the 'tlv_eeprom read' command.\n"); return 0; }

@@ -509,7 +509,6 @@ int do_tlv_eeprom(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[]) return 0; } current_dev = devnum;

• has_been_read = 0;
  

  } else { cmd_usage(cmdtp); }

Viele Grüße, Stefan Roese