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