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

[Stefan Roese]

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 <jo...@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 <s...@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

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