Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41209 )
Change subject: fw_config: Add firmware configuration interface ......................................................................
Patch Set 16:
(3 comments)
https://review.coreboot.org/c/coreboot/+/41209/15/Documentation/lib/fw_confi... File Documentation/lib/fw_config.md:
https://review.coreboot.org/c/coreboot/+/41209/15/Documentation/lib/fw_confi... PS15, Line 8: to be a variant of the same baseboard nit: to use the same coreboot build target.
variants in coreboot are associated with separate build targets.
https://review.coreboot.org/c/coreboot/+/41209/15/src/include/device/device.... File src/include/device/device.h:
https://review.coreboot.org/c/coreboot/+/41209/15/src/include/device/device.... PS15, Line 153: probe_list Should this be guarded by CONFIG(FW_CONFIG) to ensure that it doesn't get added unless mainboard wants to use this?
https://review.coreboot.org/c/coreboot/+/41209/15/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/41209/15/src/lib/fw_config.c@33 PS15, Line 33: } I know it's not likely that fw_config_value would be partially initialized if cbfs_boot_load_file() failed, but should we set it to 0 here in case FW_CONFIG_SOURCE_CBFS is the only option selected by mainboard.