Jett Rink has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41209 )
Change subject: fw_config: Add firmware configuration interface ......................................................................
Patch Set 5:
(3 comments)
Overall LGTM
https://review.coreboot.org/c/coreboot/+/41209/5/Documentation/lib/fw_config... File Documentation/lib/fw_config.md:
https://review.coreboot.org/c/coreboot/+/41209/5/Documentation/lib/fw_config... PS5, Line 10: The initial implementation is designed to take advantage of a bitmask returned by the Embedded : Controller on Google Chrome OS devices which allows the manufacturer to use the same firmware : image across multiple devices by selecting various options at runtime. Link to https://chromium.googlesource.com/chromiumos/docs/+/master/design_docs/firmw... ?
https://review.coreboot.org/c/coreboot/+/41209/5/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/41209/5/src/include/fw_config.h@17 PS5, Line 17: const char *field_name; : const char *option_name; if we don't change matching logic, then we should document that these fields are required to be non-null for a match to take place regardless of mask and value
(I do think we shouldn't make that a requirement though)
https://review.coreboot.org/c/coreboot/+/41209/5/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/41209/5/src/lib/fw_config.c@67 PS5, Line 67: match->field_name && match->option_name If the debug string aren't present, shouldn't we still match (but not print out as much in the probe_one function)?