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

Heinrich Schuchardt
2025-09-02T22:32:28Z

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

Best regards

Heinrich

>
> -E