[RFC 04/10] eeprom: starfive: Correct get_pcb_revision_from_eeprom()

[Hal Feng]

> On 03.09.25 06:32, Heinrich Schuchardt wrote:
> On 9/3/25 00:12, E Shattow wrote:
> >
> > On 9/2/25 00:44, Hal Feng wrote:
> >>> On 29.08.25 15:44, Heinrich Schuchardt wrote:
> >>> On 29.08.25 08:09, Hal Feng wrote:
> >>>> pcb_revision is stored in the pcb_revision field of ATOM4. Correct it.
> >>>> Move the function description to the header file.
> >>>> Remove the function calls in board/starfive/visionfive2/.
> >>>>
> >>>> Fixes: aea1bd95b61e ("eeprom: starfive: Enable ID EEPROM
> >>>> configuration")
> >>>> Signed-off-by: Hal Feng <hal.feng at starfivetech.com>
> >>>> ---
> >>>>    arch/riscv/include/asm/arch-jh7110/eeprom.h   |  5 +++++
> >>>>    board/starfive/visionfive2/spl.c              | 18 ++++++-----------
> >>>>    .../visionfive2/starfive_visionfive2.c        | 20 ++++++-------------
> >>>>    .../visionfive2/visionfive2-i2c-eeprom.c      | 11 ++--------
> >>>>    4 files changed, 19 insertions(+), 35 deletions(-)
> >>>>
> >>>> diff --git a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>> b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>>> index 6d0a0ba0c4a..025f1d32c49 100644
> >>>> --- a/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>>> +++ b/arch/riscv/include/asm/arch-jh7110/eeprom.h
> >>>> @@ -9,6 +9,11 @@
> >>>>
> >>>>    #include <linux/types.h>
> >>>>
> >>>> +/**
> >>>> + * get_pcb_revision_from_eeprom() - get the PCB revision
> >>>> + *
> >>>> + * @return: the PCB revision or 0xFF on error.
> >>>> + */
> >
> > No. Inverted logic. See below comment about body of function.
> >
> >>>>    u8 get_pcb_revision_from_eeprom(void);
> >>>>
> >>>>    /**
> >>>> diff --git a/board/starfive/visionfive2/spl.c
> >>> b/board/starfive/visionfive2/spl.c
> >>>> index 3dfa931b655..901e7b58f36 100644
> >>>> --- a/board/starfive/visionfive2/spl.c
> >>>> +++ b/board/starfive/visionfive2/spl.c
> >>>> @@ -126,19 +126,13 @@ int board_fit_config_name_match(const char
> >>> *name)
> >>>>    		    !strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> >>>>    		return 0;
> >>>>    	} else if (!strcmp(name,
> >>>> "starfive/jh7110-starfive-visionfive-2-v1.2a")
> >>> &&
> >>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> >>>> -		switch (get_pcb_revision_from_eeprom()) {
> >>>> -		case 'a':
> >>>> -		case 'A':
> >>>> -			return 0;
> >>>> -		}
> >>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7)
> >>> ||
> >>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)))
> >>> {
> >>>> +		return 0;
> >>>>    	} else if (!strcmp(name,
> >>>> "starfive/jh7110-starfive-visionfive-2-v1.3b")
> >>> &&
> >>>> -		    !strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> >>>> -		switch (get_pcb_revision_from_eeprom()) {
> >>>> -		case 'b':
> >>>> -		case 'B':
> >>>> -			return 0;
> >>>> -		}
> >>>> +		   (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7)
> >>> ||
> >>>> +		    !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)))
> >>> {
> >>>> +		return 0;
> >>>>    	}
> >>>>
> >>>>    	return -EINVAL;
> >
> > I agree it is good to delete the case statement and drop lowercase
> > compare as suggested, also...
> >
> >>>> diff --git a/board/starfive/visionfive2/starfive_visionfive2.c
> >>> b/board/starfive/visionfive2/starfive_visionfive2.c
> >>>> index bfbb11a2ee7..f38433e94ac 100644
> >>>> --- a/board/starfive/visionfive2/starfive_visionfive2.c
> >>>> +++ b/board/starfive/visionfive2/starfive_visionfive2.c
> >>>> @@ -59,20 +59,12 @@ static void set_fdtfile(void)
> >>>>    		fdtfile = "starfive/jh7110-milkv-mars.dtb";
> >>>>    	} else if (!strncmp(get_product_id_from_eeprom(), "STAR64", 6)) {
> >>>>    		fdtfile = "starfive/jh7110-pine64-star64.dtb";
> >>>> -	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110", 6)) {
> >>>> -		switch (get_pcb_revision_from_eeprom()) {
> >>>> -		case 'a':
> >>>> -		case 'A':
> >>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> >>> v1.2a.dtb";
> >>>> -			break;
> >>>> -		case 'b':
> >>>> -		case 'B':
> >>>> -			fdtfile = "starfive/jh7110-starfive-visionfive-2-
> >>> v1.3b.dtb";
> >>>> -			break;
> >>>> -		default:
> >>>> -			log_err("Unknown revision\n");
> >>>> -			return;
> >>>> -		}
> >>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110A", 7) ||
> >>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110a", 7)) {
> >>>
> >>> Where boards both with 'a' and 'b' ever shipped?
> >>> I have only seen 'A' and 'B' in pbuf.eeprom.atom1.data.pstr[6].
> >>
> >> You're right. There are no boards marked "VF7110a" or "VF7110b".
> >> Let's remove the comparison of "VF7110a" and "VF7110b".
> >>
> >> Best regards,
> >> Hal
> >
> > ... again here too delete the case statement and drop lowercase compare.
> > Thanks.
> >
> >>
> >>>
> >>> The change looks technically correct.
> >>>
> >>> Reviewed-by: Heinrich Schuchardt <heinrich.schuchardt at canonical.com>
> >>>
> >>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.2a.dtb";
> >>>> +	} else if (!strncmp(get_product_id_from_eeprom(), "VF7110B", 7) ||
> >>>> +		   !strncmp(get_product_id_from_eeprom(), "VF7110b", 7)) {
> >>>> +		fdtfile = "starfive/jh7110-starfive-visionfive-2-v1.3b.dtb";
> >>>>    	} else {
> >>>>    		log_err("Unknown product\n");
> >>>>    		return;
> >
> >>>> diff --git a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>> b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>>> index 3866d07f9d4..43b8af4fc59 100644
> >>>> --- a/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>>> +++ b/board/starfive/visionfive2/visionfive2-i2c-eeprom.c
> >>>> @@ -535,19 +535,12 @@ int mac_read_from_eeprom(void)
> >>>>    	return 0;
> >>>>    }
> >>>>
> >>>> -/**
> >>>> - * get_pcb_revision_from_eeprom - get the PCB revision
> >>>> - *
> >>>> - * 1.2A return 'A'/'a', 1.3B return 'B'/'b',other values are
> >>>> illegal
> >>>> - */
> >>>>    u8 get_pcb_revision_from_eeprom(void)
> >>>>    {
> >>>> -	u8 pv = 0xFF;
> >>>> -
> >>>>    	if (read_eeprom())
> >>>> -		return pv;
> >>>> +		return 0xFF;
> >>>>
> >>>> -	return pbuf.eeprom.atom1.data.pstr[6];
> >>>> +	return pbuf.eeprom.atom4.data.pcb_revision;
> >>>>    }
> >>>>
> >>>>    u8 get_ddr_size_from_eeprom(void)
> >>
> >
> > No. I do not want to have to guess what error does "0xFF" have the
> > meaning of, it makes the code annoying to read every function header
> > documentation to learn this information.
> >
> > Since we remove all users of mac_read_from_eeprom() it is certainly
> > not a problem we care about now anymore. Default PCB revision in case
> > of error may be zero. It is not important to make distinction of
> > additional failure detection, this is already resolved by
> > read_eeprom() where any error message may be presented to the user.
> >
> > I appreciate the effort to be more technically correct and have a
> > unique error value but with u8 return value 0xFF it is an additional
> > layer of complication not needed. If you insist on this approach then
> > the return value should be arbitrarily defined as a compiler macro,
> > say CMD_EEPROM_FAIL (255) and consistently used. In fact I would not
> > complain if it improved code readability.
> 
> There is no necessity to use u8 as the return value. We could also use int and
> return a negative error code (-EIO) on failure.

Yeah, will return a negative error code (-EIO) on failure.
I think the reason why we set the return value to 0xFF on failure was that
0xFF is the default value after EEPROM is erased.

Best regards,
Hal