David Hendricks has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/34634 )
Change subject: drivers/vpd: add framework to search VPD in romstage ......................................................................
Patch Set 10:
(4 comments)
Just a few drive-by comments...
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd.h@7 PS10, Line 7: #include "vpd_lib.h" This should go below the header guard
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c File src/drivers/vpd/vpd_lib.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 8: * : * SPDX-License-Identifier:.*GPL-2.0-or-later We're not using SPDX identifiers for coreboot.
Actually, there was a recent poll where we decides to do away with copyright lines all together: https://civs.cs.cornell.edu/cgi-bin/results.pl?id=E_9e4f5ea789b9ceb9
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_lib.c@... PS10, Line 67: printk(BIOS_CRIT, "binary blob size:%d\n", size); Seems like a debug print that got left in by accident?
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... File src/drivers/vpd/vpd_premem.c:
https://review.coreboot.org/c/coreboot/+/34634/10/src/drivers/vpd/vpd_premem... PS10, Line 42: blob->ro_base = NULL; : blob->ro_size = -1; : blob->rw_base = NULL; : blob->rw_size = -1; Since these aren't actually used (only assigned), you can remove these lines. At the very least, the blob->ro_size and blob->rw_size assignments should probably go - Even though it's valid, I suspect it will throw compiler warnings which could lead to a failure depending on how pedantic the build rules are set up.
I noticed in vpd_cbmem.c, the equivalent ro_size and rw_size members of the cbmem struct are set to 0, so it might be best to just do that here as well to be consistent.