[PATCH 1/1] starfive: fix return code of `mac write_eeprom`

[Heinrich Schuchardt]

On 11.08.25 05:16, E Shattow wrote:
> 
> 
> On 8/9/25 01:21, Heinrich Schuchardt wrote:
>> When writing the EEPROM fails, the command usage help text is displayed
>> after the error message. We should only display the error message instead.
>>
>> If writing the EEPROM fails, return CMD_RET_FAILURE (1) instead of
>> CMD_RET_USAGE (-1).
>>
>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM configuration")
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
>> ---
>>   board/starfive/visionfive2/visionfive2-i2c-eeprom.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>> index 010e386e64d..17a44020bcf 100644
>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
>> @@ -275,7 +275,7 @@ static int prog_eeprom(unsigned int size)
>>   
>>   	if (is_match_magic()) {
>>   		printf("MAGIC ERROR, Please check the data@%p.\n", pbuf.buf);
>> -		return -1;
>> +		return CMD_RET_FAILURE;
>>   	}
>>   
>>   	ret = i2c_get_chip_for_busnum(CONFIG_SYS_EEPROM_BUS_NUM,
>> @@ -285,7 +285,7 @@ static int prog_eeprom(unsigned int size)
>>   	if (ret) {
>>   		printf("Get i2c bus:%d addr:%d fail.\n", CONFIG_SYS_EEPROM_BUS_NUM,
>>   		       CONFIG_SYS_I2C_EEPROM_ADDR);
>> -		return ret;
>> +		return CMD_RET_FAILURE;
>>   	}
>>   
>>   	for (i = 0, p = (u8 *)pbuf.buf; i < size; ) {
>> @@ -314,11 +314,11 @@ static int prog_eeprom(unsigned int size)
>>   	if (ret) {
>>   		has_been_read = -1;
>>   		printf("Programming failed.\n");
>> -		return -1;
>> +		return CMD_RET_FAILURE;
>>   	}
>>   
>>   	printf("Programming passed.\n");
>> -	return 0;
>> +	return CMD_RET_SUCCESS;
>>   }
>>   
>>   /**
> 
> This patch is a good start, but I think maybe more is needed? Quoting
> the notes I wrote for myself last year:
> 
> "
> mac command:
> 
> if data did not change then "Programming passed." is printed regardless
> of write protect state but no action was taken and no usage help is
> printed. 'mac' still outputs in-memory info.
> 
> when data was changed the command shows "Programming failed." or
> "Programming passed." followed by usage help and 'mac' command shows no
> output until 'mac read_eeprom'.
> 
> Suggested:
> 
> If data did not change at time of 'mac eeprom_write' then it should
> still try to write so the message pass/fail will be correct.
> The usage info following 'mac write_eeprom' is a bug, should not be printed.

Thank you for reviewing.

The EEPROM driver does the right thing. If data is not changed, do not 
write. This avoids wear.

We would have to keep a second copy of the EEPROM contents when reading 
if we wanted to observe that no change was done.

This is beyond the scope of the current patch.

> 
> prog_eeprom return values inconsistent with command.h
> 
> prog_eeprom returns -1 which overlaps with CMD_RET_USAGE
> should be 1 for failure

This is covered by the current patch.

> 
> show_eeprom()
> 
> Print a message "No valid data. Please do mac read_eeprom".
> Maybe "Read nnn bytes from EEPROM" and "Displaying local copy of EEPROM
> from memory" to be more clear.

show_eeprom() already checks the value of variable has_been_read and 
doesn't display the EEPROM content if it is not set. A message could be 
added in this case.

But the EEPROM is read before reaching the coammand line if 
CONFIG_ID_EEPROM=y:

common/board_r.c:732:   INITCALL(mac_read_from_eeprom);

Therefore we typically don't observe has_been_read=0 when executing the 
mac command.

Best regards

Heinrich

> "
> 
> Acked-by: E Shattow <e at freeshell.de>