Attention is currently required from: Felix Singer, Jeff Daly, Paul Menzel, Patrick Rudolph. Martin Roth has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/60877 )
Change subject: sb/intel/common/firmware: Hook up adding 10GbE LAN firmware ......................................................................
Patch Set 13:
(3 comments)
File src/southbridge/intel/common/firmware/Kconfig:
https://review.coreboot.org/c/coreboot/+/60877/comment/502b2a71_884f0365 PS13, Line 152: HAVE_ Usually the 'HAVE' options are more like what you use above - it usually signifies that a chipset supports an option, then can be enabled or disabled by another config option that uses a 'depends on' for the have option.
Basically, I'm suggesting you swap the name of the two options. Not critical or a blocker, just the normal coreboot convention. (Yeah, that isn't documented anywhere - I guess I should do that.)
https://review.coreboot.org/c/coreboot/+/60877/comment/3c196585_b3cd7f7c PS13, Line 170: HAVE_10GBE_1_BIN Obviously, same as above.
File src/southbridge/intel/common/firmware/Makefile.inc:
https://review.coreboot.org/c/coreboot/+/60877/comment/b7718392_25bd6684 PS13, Line 89: Could you remove the newlines inside the target? We use newlines as breaks between targets, not internal to a target.
-
Martin Roth (Code Review)