Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/31133 )
Change subject: soc/intel/cannonlake: Add SOC_INTEL_WHISKEYLAKE kconfig ......................................................................
Patch Set 6:
(3 comments)
https://review.coreboot.org/#/c/31133/6/src/mainboard/intel/coffeelake_rvp/K... File src/mainboard/intel/coffeelake_rvp/Kconfig:
https://review.coreboot.org/#/c/31133/6/src/mainboard/intel/coffeelake_rvp/K... PS6, Line 15: select SOC_INTEL_COFFEELAKE It might be easier to just select this in Kconfig.name under appropriate BOARD_INTEL_*. That way you can avoid the long chain of || conditions.
https://review.coreboot.org/#/c/31133/6/src/soc/intel/cannonlake/Kconfig File src/soc/intel/cannonlake/Kconfig:
https://review.coreboot.org/#/c/31133/6/src/soc/intel/cannonlake/Kconfig@16 PS6, Line 16: select SOC_INTEL_CANNONLAKE Just thinking out loud: Should this do a "select SOC_INTEL_COFFEELAKE" or maybe add another config option "SOC_INTEL_CFL_WHL" and select that? As you mentioned on the other CL, there will be CML patches coming to CNL as well and that would mean 3 SoC config checks in multiple places (and probably in all the same places?). Thoughts?
https://review.coreboot.org/#/c/31133/6/src/soc/intel/cannonlake/Kconfig@258 PS6, Line 258: default "src/vendorcode/intel/fsp/fsp2_0/cannonlake/" if !SOC_INTEL_COFFEELAKE && !SOC_INTEL_WHISKEYLAKE : default "3rdparty/fsp/CoffeeLakeFspBinPkg/Include/" if SOC_INTEL_COFFEELAKE || SOC_INTEL_WHISKEYLAKE Can we just swap the order so that the we can have the second case as default "src/vendorcode/intel/fsp/fsp2_0/cannonlake/" and avoid the if there?
-
Furquan Shaikh (Code Review)