Change in coreboot[master]: lib/spd_bin: add get_spd_sn function
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 -- To view, visit https://review.coreboot.org/c/coreboot/+/40414 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: coreboot Gerrit-Branch: master Gerrit-Change-Id: I406bba7cc56debbd9851d430f069e4fb96ec937c Gerrit-Change-Number: 40414 Gerrit-PatchSet: 4 Gerrit-Owner: Jamie Chen <jamie.chen@intel.com> Gerrit-Reviewer: Aaron Durbin <adurbin@chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus@gmail.com> Gerrit-Reviewer: Arthur Heymans <arthur@aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec@chromium.org> Gerrit-Reviewer: Furquan Shaikh <furquan@google.com> Gerrit-Reviewer: Kane Chen <kane.chen@intel.com> Gerrit-Reviewer: Patrick Rudolph <siro@das-labor.org> Gerrit-Reviewer: Tim Wawrzynczak <twawrzynczak@chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply@coreboot.org> Gerrit-CC: EricR Lai <ericr_lai@compal.corp-partner.google.com> Gerrit-CC: Paul Menzel <paulepanter@users.sourceforge.net> Gerrit-Comment-Date: Mon, 04 May 2020 18:41:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment
participants (1)
-
Tim Wawrzynczak (Code Review)