Tim Wawrzynczak has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/40414 )
Change subject: lib/spd_bin: add get_spd_sn function ......................................................................
Patch Set 4:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 85: return sn == 0x00000000, if addr is 0x0. : return sn == 0xffffffff, if dimm is not present. */ How about just return values from the function?
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 87: u8 *sn This still looks weird. I understand there is the implicit length and you mentioned it above, but if it's four bytes, why not pass a u32, or a custom struct that has a u8[4] field in it, or even a length passed here too, and you could return an error on that length < SPD_SN_LEN
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 92: /* smbus will return 0xff if addr is zero */ : if (addr == 0x00) { : memset(sn, 0, SPD_SN_LEN); : return; : } If the do_smbus_read_byte() function already handles checking for an invalid address, I don't see a good reason to do it here too. If it's an invalid address, the SMBus library should return SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT (i.e. CB_I2C_NO_DEVICE).
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 99: dram_type This is a u8, but do_dmbus_read_byte returns an int (where negative numbers are the error values), so this would be the place to check for an error.
https://review.coreboot.org/c/coreboot/+/40414/4/src/soc/intel/common/block/... PS4, Line 120: Unsupport Unsupported