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

[Marek Vasut]

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.

[...]

>>> +     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

>>> +                            __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 ?

>> 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 ?

[...]

>> 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));
?