Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: lib: Add config MAINBOARD_OPTIONS for getting options from mainboard ......................................................................
Patch Set 12:
(2 comments)
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/12/src/include/option.h@25 PS12, Line 25: mainboard_get_option I guess I don't completely understand what the mainboard_get_option is supposed to be. Is there a reason not to make it specific to the method of getting the options? A mainboard specific method seems like it's going to further fragment the option-getting methods when we could be consolidating them.
What I'd personally like to see, and I understand that it's outside of the scope of this patch, a well enumerated list of possible places to get options from: - boot_rom_get_option - gpio_get_option - i2c_get_option ???
https://review.coreboot.org/c/coreboot/+/45134/12/src/lib/option.c File src/lib/option.c:
https://review.coreboot.org/c/coreboot/+/45134/12/src/lib/option.c@8 PS12, Line 8: : if (strcmp(name, "debug_level") == 0) {
Another idea would be to make this a weak function and the mainboard implements the work of checking […]
I agree that it would make sense to let the mainboard choose what options to implement instead of setting "mainboard" options that are really fixed across all of coreboot.
This might make this particular patch a little more difficult since it's replacing a global option with one that is supposed to be mainboard specific, so it would have to be implemented in any platform that wanted to use the existing Kconfig option.