insomniac has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
Patch Set 2:
(5 comments)
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@15 PS2, Line 15: CONFIG_VERSION_VPD This should be checked to be a non-empty string
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@16 PS2, Line 16: printk(BIOS_DEBUG, "Get version from VPD: %s\n", version); can you add the name of the VPD variable in the debug log?
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@19 PS2, Line 19: printk(BIOS_ERR, "Get version from VPD failed\n"); the name of the VPD variable would be useful here too
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@20 PS2, Line 20: if (strlen(CONFIG_VERSION_VPD_DEFAULT_IF_MISSING)) nit: for readability I'd consider the more explicit "strlen(..) != 0". I'm not sure if there's a preferred style about this though
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@21 PS2, Line 21: return CONFIG_VERSION_VPD_DEFAULT_IF_MISSING; another debug printk here and below would be helpful, something like:
printk("BIOS version set to CONFIG_VERSION_VPD_DEFAULT_IF_MISSING: '%s'", CONFIG_VERSION_VPD_DEFAULT_IF_MISSING)
and
printk("BIOS version set to coreboot version: '%s'", coreboot_version)