Attention is currently required from: Cliff Huang, Furquan Shaikh, Tim Wawrzynczak, Michael Niewöhner, Angel Pons, Patrick Rudolph, Karthik Ramasubramanian. Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/56004 )
Change subject: soc/intel/common/block/acpi: Move pep.asl to acpigen ......................................................................
Patch Set 1:
(2 comments)
File src/soc/intel/common/block/acpi/Kconfig:
https://review.coreboot.org/c/coreboot/+/56004/comment/4257bdd9_89866568 PS1, Line 18: config SOC_INTEL_COMMON_BLOCK_ACPI_PEP
When this is set, the only change is to include pep.c in the build. […]
I'm not sure if I follow. Do you mean that the current Kconfig option's name can lead to confusion with the unrelated pep.asl file?
File src/soc/intel/common/block/include/intelblocks/acpi.h:
https://review.coreboot.org/c/coreboot/+/56004/comment/2dccce0e_b3298182 PS1, Line 94: void generate_acpi_power_engine(const struct device *dev);
Please add #if CONFIG(CONFIG_SOC_INTEL_COMMON_BLOCK_ACPI_PEP) for this declaration.
No. Please never guard function declarations with preprocessor. Otherwise, all calls also need to be guarded with preprocessor, which is very ugly:
#if CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_PEP) generate_acpi_power_engine(dev); #endif
Instead, leave the declaration as-is, and guard the calls with a regular C if statement:
if (CONFIG(SOC_INTEL_COMMON_BLOCK_ACPI_PEP)) generate_acpi_power_engine(dev);
This can be compiled regardless of the state of `SOC_INTEL_COMMON_BLOCK_ACPI_PEP`. However, if the condition is true but `generate_acpi_power_engine()` is not defined, the linker will complain about "undefined symbol" (the function), which results in a build failure.
When the condition is false, the compiler optimises out (removes) the call to `generate_acpi_power_engine()`, so the linker never "sees" it, and building succeeds (assuming there aren't any other issues, of course).
-
Angel Pons (Code Review)