[PATCH 01/12] cmd: tlv_eeprom: remove use of global variable current_dev

Stefan Roese sr at denx.de
Tue May 3 08:09:20 CEST 2022


Hi Josua,

a few general comments about this series:

- A cover letter for this patchset would be very helpful, so that
   reviewers have a summary of the changes.
- Please Cc the original author of this code Baruch Siach in the
   next version as well (if there is next version that is).

On 02.05.22 16:18, Josua Mayer wrote:
> Make tlv_eeprom command device selection an explicit parameter of all
> function calls.
> 
> Signed-off-by: Josua Mayer <josua at solid-run.com>

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

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 bf8d453dc5..f91c11b304 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:
>   			cmd_usage(cmdtp);
> @@ -886,7 +885,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;
>   
> @@ -956,14 +955,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,
> @@ -1018,10 +1017,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;
>   	}
> @@ -1086,7 +1086,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;
> @@ -1095,7 +1095,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 a2c333e744..fd45e5f6eb 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: Wolfgang Denk
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-51 Fax: (+49)-8142-66989-80 Email: sr at denx.de


More information about the U-Boot mailing list