From: Marek Vasut <ma...@denx.de> Sent: Sunday, December 1, 2024 7:44 PM > On 11/28/24 3:20 PM, Christoph Niedermaier wrote: > > [...] > >> +++ b/board/dhelectronics/common/dh_common.c >> @@ -7,6 +7,7 @@ >> #include <dm.h> >> #include <i2c_eeprom.h> >> #include <net.h> >> +#include <u-boot/crc.h> >> >> #include "dh_common.h" >> >> @@ -30,6 +31,148 @@ int dh_get_mac_is_enabled(const char *alias) >> return 0; >> } >> >> +int dh_read_eeprom_id_page(u8 *eeprom_buffer, const char *alias) >> +{ >> + struct eeprom_id_page *eip = (struct eeprom_id_page *)eeprom_buffer; >> + struct udevice *dev; >> + size_t payload_len; >> + int eeprom_size; >> + u16 crc16_calc; >> + u16 crc16_eip; >> + u8 crc8_calc; >> + ofnode node; >> + int ret; >> + >> + node = ofnode_path(alias); >> + >> + ret = uclass_get_device_by_ofnode(UCLASS_I2C_EEPROM, node, &dev); >> + if (ret) >> + return ret; >> + >> + eeprom_size = i2c_eeprom_size(dev); >> + if (eeprom_size < 0) { >> + printf("%s: Error getting EEPROM ID page size! ret = %d\n", >> __func__, ret); >> + return eeprom_size; >> + } else if (eeprom_size == 0 || eeprom_size > >> DH_EEPROM_ID_PAGE_MAX_SIZE) { > > The else part is not necessary, use plain: > > ... > ... > return eeprom_size; > } > > if (eeprom_size == 0 || eeprom_size > DH_EEPROM_ID_PAGE_MAX_SIZE) { > ...
I will change it in V5. >> + eeprom_size = DH_EEPROM_ID_PAGE_MAX_SIZE; >> + printf("Get invalid EEPROM ID page size %d bytes! Try to read >> %d bytes.\n", >> + eeprom_size, DH_EEPROM_ID_PAGE_MAX_SIZE); >> + } >> + >> + ret = i2c_eeprom_read(dev, 0x0, eeprom_buffer, eeprom_size); >> + if (ret) { >> + printf("%s: Error reading EEPROM ID page! ret = %d\n", >> __func__, ret); >> + return ret; >> + } >> + >> + /* Validate header ID */ >> + if (eip->hdr.id[0] != 'D' || eip->hdr.id[1] != 'H' || eip->hdr.id[2] != >> 'E') { >> + printf("%s: Error validating header ID! (got %c%c%c (0x%02x >> 0x%02x 0x%02x) != expected DHE)\n", >> + __func__, isprint(eip->hdr.id[0]) ? eip->hdr.id[0] : '.', >> + isprint(eip->hdr.id[1]) ? eip->hdr.id[1] : '.', >> + isprint(eip->hdr.id[2]) ? eip->hdr.id[2] : '.', >> + eip->hdr.id[0], eip->hdr.id[1], eip->hdr.id[2]); >> + return -EINVAL; >> + } >> + >> + /* Validate header checksum */ >> + crc8_calc = crc8(0xff, eeprom_buffer, offsetof(struct eeprom_id_page, >> hdr.crc8_hdr)); >> + if (eip->hdr.crc8_hdr != crc8_calc) { >> + printf("%s: Error validating header checksum! (got 0x%02x != >> calc 0x%02x)\n", >> + __func__, eip->hdr.crc8_hdr, crc8_calc); >> + return -EINVAL; >> + } >> + >> + /* >> + * Validate header version >> + * The payload is defined by the version specified in the header. >> + * Currently only version 0x10 is defined, so take the length of >> + * the only defined payload as the payload length. >> + */ >> + if (eip->hdr.version == DH_EEPROM_ID_PAGE_V1_0) { > > Invert this conditional: > > ... > if (eip->hdr.version != DH_EEPROM_ID_PAGE_V1_0) { > printf(...); > return ...; > } > > payload_len = sizeof(eip->pl); > ... I will change it in V5. >> + payload_len = sizeof(eip->pl); >> + } else { >> + printf("%s: Error validating version! (0x%02X is not >> supported)\n", >> + __func__, eip->hdr.version); >> + return -EINVAL; >> + } >> + >> + /* Validate payload checksum */ >> + crc16_eip = (eip->hdr.crc16_pl[1] << 8) | eip->hdr.crc16_pl[0]; >> + crc16_calc = crc16(0xffff, eeprom_buffer + sizeof(eip->hdr), >> payload_len); >> + if (crc16_eip != crc16_calc) { >> + printf("%s: Error validating data checksum! (got 0x%02x != calc >> 0x%02x)\n", >> + __func__, crc16_eip, crc16_calc); >> + return -EINVAL; >> + } >> + >> + return 0; >> +} > > [...] Thanks and regards Christoph