Johnny Lin has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a function that reads BIOS version from a VPD variable ......................................................................
Patch Set 6:
(16 comments)
Resolve some comments, if it's better to move the function and Kconfig to x86/smbios.c will do. Updated the commit message to reflect the special usage for updating BIOS version w/o rebuilding from source, it's for internal build and test processes. Thanks for your suggestion for different approaches, wait for others opinions and decision.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@9 PS2, Line 9: VPD_COREBOOT_VERSION
I would call this VPD_SMBIOS_VERSION since it's really more related to SMBIOS than to coreboot.
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@12 PS2, Line 12: VPD
&& GENERATE_SMBIOS_TABLES
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@15 PS2, Line 15: VPD_RO and override SMBIOS type 0 version.
Selecting this option will read …
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@19 PS2, Line 19: default "version"
Does this really need to be user configurable? Can't you just pick a slightly more expressive name ( […]
Changed to firmware_version, I think using a config can provide more flexibility for internal build & test process.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@23 PS2, Line 23: is "version". The value of the variable is limited to 20 characters for now.
Re-flow so both lines length are closer to each other.
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@27 PS2, Line 27: default ""
We already have CONFIG_LOCALVERSION, isn't that the same thing?
Yes, use CONFIG_LOCALVERSION.
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/Kconfig@19 PS4, Line 19: version
do you want to change the default name to coreboot_version or firmware_version or smbios_version? […]
Changed to firmware_version.
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@5 PS2, Line 5: #include <smbios.h>
Sort the entries?
Do you mean sort the include files alphabetically? Just updated.
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
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@16 PS2, Line 16: Get version from VPD: %s\n
Just using present tense *Get* with the version from VPD printed is a little confusing to me. […]
Done
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?
Done
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
Done
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". […]
Done
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: […]
Done
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@22 PS2, Line 22: else
The else is not needed, but it’s subjective.
Done
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/4/src/drivers/vpd/smbios.c@32 PS4, Line 32: BIOS
SMBIOS ?
Done