RE: [PATCH 2/2] arm64: imx8mp: Read item and serial number from EEPROM ID page on DH i.MX8MP DHCOM

[Christoph Niedermaier <cniedermaier@dh-electronics.com>]

From: Marek Vasut <marex@denx.de>
Sent: Thursday, October 17, 2024 2:22 AM
> On 10/16/24 2:31 PM, Christoph Niedermaier wrote:
> 
> [...]
> 
>>>> diff --git a/board/dhelectronics/common/dh_common.h b/board/dhelectronics/common/dh_common.h
>>>> index 4c22ece435..1baa45e340 100644
>>>> --- a/board/dhelectronics/common/dh_common.h
>>>> +++ b/board/dhelectronics/common/dh_common.h
>>>> @@ -6,6 +6,8 @@
>>>>    enum eip_request_values {
>>>>        MAC0,
>>>>        MAC1,
>>>> +     ITEM_NUMBER,
>>>> +     SN,
>>>
>>> Why is this patch not squashed into 1/2 ? It seems to be changing the
>>> same code.
>>
>> The first patch add the reading for MAC address from the EEPROM ID
>> page and add the use of that addresses. The second extends the reading
>> to the serial number and the item number. So that the patch doesn't
>> get too big I found it useful to split it into two. Do you want me to
>> make one patch out of it?
> 
> Yes please. Once you cache the EEPROM content, the patch would likely
> get simpler anyway.

Will be done in v2.

> 
> [...]
> 
>>>> +     ret = dh_get_value_from_eeprom_id_page(ITEM_NUMBER, item_number, sizeof(item_number),
>>>> +                                            "eeprom0");
>>>> +     if (ret) {
>>>> +             /*
>>>> +              * The function only returns the value -ENOENT for SoM rev.100, because
>>>> +              * the EEPROM ID page isn't available there. Therefore the output makes
>>>> +              * no sense and will be suppressed here.
>>>> +              */
>>>> +             if (ret != -ENOENT)
>>>> +                     printf("%s: Unable to get item number form EEPROM ID page! ret = %d\n",
> 
> 'form' typo

Will be fixed in v2.

>>>> +                            __func__, ret);
>>>
>>> This will be printed on every device, even the ones without ID EEPROM,
>>> correct ? This should not be printed on devices without ID EEPROM. Also,
>>
>> This is suppressed by the -ENOENT check.
> 
> Does i2c_eeprom_read() in dh_get_value_from_eeprom_id_page() return
> -ENOENT in case the EEPROM is described in DT, but not populated on the
> board ? I suspect it returns some other error code, -ETIMEDOUT or
> -EINVAL maybe ?

It could only be possible if the DTO for hardware rev. 100 (which has no
EEPROM ID page) is not loaded. The return value then is -ENODEV.
I will included this in v2.

>>> if (ret && ret != -ENOENT) {}
>>>
>>> works equally well without the extra indent.
>>
>> I have an else to (ret) here not to (ret && ret != -ENOENT).
>> This would change the logic.
> 
> } else if (!ret) { or similar should fix that, right ?
> 
> Basically, the question is, can we avoid this two level deep indent ? I
> think yes ?

I want to try it in v2.

> [...]
> 
>>> Also, this shouldn't be repeatedly reading the EEPROM, the EEPROM read
>>> operation is the slow part, the EEPROM is 32 bytes, so the EEPROM should
>>> be read once and cached, and once the cache is populated all read
>>> accesses to the EEPROM should use the cache.
>>
>> This is already covered in function dh_get_value_from_eeprom_id_page().
> It seems that function always calls
> ret = i2c_eeprom_read(dev, 0x0, eipa, sizeof(eipa));
> ?

The 32 bytes are read into a static variable. If the ID (DHE) already exists
in it, the i2c_eeprom_read() function is no longer called.


Regards
Christoph