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 3:
(5 comments)
https://review.coreboot.org/c/coreboot/+/40414/3/src/include/spd_bin.h File src/include/spd_bin.h:
https://review.coreboot.org/c/coreboot/+/40414/3/src/include/spd_bin.h@51 PS3, Line 51: void get_spd_sn(u8 addr, u8 dram_type, u8 *sn);
This should be commented as to what it's doing. sn == serial number based off of commit description. […]
There is SPD_SN_LEN, but that is very implicit. How about a "struct spd_serial_number" that just contains SPD_SN_LEN bytes (if that's true for all SPD formats). Then there's no need for a generic byte pointer.
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... File src/soc/intel/common/block/smbus/smbuslib.c:
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 41: dimm_preset dimm_present.
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 43: < 0 So even if the host controller is busy, it will still return < 0. I believe the correct result for "not present" would be SMBUS_WAIT_UNTIL_ACTIVE_TIMEOUT
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 44: addr << 1 If it's passed in as addr, I don't see a reason why would you print it out shifted left 1. SMBus is a little more clear than I2C on using 7-bit addresses.
https://review.coreboot.org/c/coreboot/+/40414/3/src/soc/intel/common/block/... PS3, Line 91: void suggestion: this should return a bool, false for the !dimm_present or unsupported enum spd_memory_type
-
Tim Wawrzynczak (Code Review)