Martin Roth has posted comments on this change. ( https://review.coreboot.org/19603 )
Change subject: soc/intel/apollolake: Bring in delta for GLK SOC ......................................................................
Patch Set 50:
(12 comments)
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/Kconfig File src/soc/intel/apollolake/Kconfig:
PS50, Line 9: select SOC_INTEL_APOLLOLAKE Would it be better to not select SOC_INTEL_APOLLOLAKE and use "if SOC_INTEL_APOLLOLAKE || SOC_INTEL_GLK" where needed?
PS50, Line 294: default CAR_CQOS if !SOC_INTEL_GLK : default CAR_NEM if SOC_INTEL_GLK Maybe get rid of the second if? default CAR_NEM if SOC_INTEL_GLK default CAR_CQOS
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/acpi/gpio_... File src/soc/intel/apollolake/acpi/gpio_apl.asl:
Line 203: extra line
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/acpi/gpio_... File src/soc/intel/apollolake/acpi/gpio_glk.asl:
Line 22: extra line
Line 208: extra line
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/acpi/lpc_g... File src/soc/intel/apollolake/acpi/lpc_glk.asl:
PS50, Line 25: Device (HPET) : { indent this file with tabs instead of spaces?
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/acpi/lpss_... File src/soc/intel/apollolake/acpi/lpss_glk.asl:
Line 159: extra line
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/include/so... File src/soc/intel/apollolake/include/soc/gpio_glk.h:
PS50, Line 367: GLk use caps?
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/include/so... File src/soc/intel/apollolake/include/soc/pci_ids.h:
PS50, Line 22: _SOC_APOLLOLAKE_PCI_IDS_H_ change to match the define?
Line 23: extra line
https://review.coreboot.org/#/c/19603/50/src/soc/intel/apollolake/romstage.c File src/soc/intel/apollolake/romstage.c:
Line 139: Get rid of the preprocessor #if and just return false here if it's GLC?
if (IS_ENABLED(CONFIG_SOC_INTEL_GLK)) return false;
PS50, Line 347: #if !IS_ENABLED(CONFIG_SOC_INTEL_GLK) /* TODO */ Please use a C if statement unless the preprocessor if is specifically needed.
-
Martin Roth (Code Review)