Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45774 )
Change subject: [RFC] Introduce vpd_get_option() ......................................................................
Patch Set 3:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/45774/3/src/Kconfig@146 PS3, Line 146: option to be queried needs to be added manually to vpd_get_option(). I still maintain that using this to control the console is generally not a good idea. Maybe this help text should warn that once you enable this option, you will re-read the whole VPD from flash in every stage and any kind of FMAP or flash access problem can result in the device hanging before you see anything on the console?
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h File src/drivers/vpd/vpd.h:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.h@10 PS3, Line 10: /* Keep in sync with Kconfig VPD_OPTION_REGION. */ Don't you just want to make a little function with
if (CONFIG(VPD_OPTION_REGION_RO)) return VPD_RO; if (CONFIG(VPD_OPTION_REGION_RW)) return VPD_RW; ...
? I think that would be much cleaner that trying to manually stay in sync with some enum.
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c File src/drivers/vpd/vpd.c:
https://review.coreboot.org/c/coreboot/+/45774/3/src/drivers/vpd/vpd.c@306 PS3, Line 306: char Why is this `char`? init_log_level() is passing a pointer to `int`. (In fact, I think you could pass `dest` straight through to vpd_get_int() if you want...)