From: Marek Vasut <ma...@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
