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

Reply via email to