Furquan Shaikh has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/48251 )
Change subject: soc/intel/common/block/lpc: Include correct acpi.h ......................................................................
Patch Set 3:
(1 comment)
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... File src/soc/intel/common/block/lpc/lpc.c:
https://review.coreboot.org/c/coreboot/+/48251/3/src/soc/intel/common/block/... PS3, Line 14: <soc/acpi.h>
I don't think that using common block defines if the common block isn't use is correct. […]
Humm.. I think long term we should do one of the following: 1. Clean up SKL to enable SOC_INTEL_COMMON_BLOCK_ACPI 2. Move the declaration of functions that are required by this driver into its corresponding header file i.e. southbridge_write_acpi_tables()is expected to be implemented if using this driver. In that case, it should be declared by intel/common/block/lpc.h. What actually implements these functions doesn't matter then - it could be SoC specific implementation or common/block/acpi implementation.
I think #2 might not be too difficult to do right now too. Also, I think we need to be careful about using other common blocks in any given common block since we want to provide flexibility to each SoC to decide what common drivers it wants to use. Here the use of common/block/acpi breaks this flexibility for skl/xeon_sp as you pointed out. Given that #2 sounds like a better approach to me. Thoughts?