Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 10:
(7 comments)
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version"
Does this need to be configurable? Can't we just hardcode this name and have everyone use the same?
Hi Andrea (insomniac), is this config needed for you? Thanks.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@22 PS8, Line 22: #if CONFIG(VPD_SMBIOS_VERSION)
Header includes should not be guarded, they don't hurt anyone.
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@374 PS8, Line 374: #if CONFIG(VPD_SMBIOS_VERSION)
You shouldn't need to guard this (if you also don't guard the header include above). […]
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@378 PS8, Line 378: static char buffer[VER_LEN] = {0};
Since this is ramstage-only (right?) maybe just use vpd_find() and malloc()? (We could also add a vp […]
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@394 PS8, Line 394: return CONFIG_LOCALVERSION;
Don't do this, just have this function return NULL and then have the caller proceed to the next opti […]
Done
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@404 PS8, Line 404: const char *__weak smbios_mainboard_bios_version(void)
I still think we should rewrite this so the weak smbios_mainboard_bios_version() just returns "" and […]
Rewritten, as for the precedence order, although only drivers/i2c/at24rf08c/lenovo_serials.c overrides this function, if I change the order they would be affected so I still keep the same order. Maybe that can be done by another change.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@13 PS2, Line 13: smbios_mainboard_bios_version
Moved to […]
Done