Dave Jiang wrote: > > On 5/22/23 14:31, Dan Williams wrote: > > Dave Jiang wrote: > >> Some of the routines in ACPI tables.c can be shared with parsing CDAT. > > s,ACPI tables.c,driver/acpi/tables.c, > > > >> However, CDAT is used by CXL and can exist on platforms that do not use > >> ACPI. > > Clarify that CDAT is not an ACPI table: > > > > CDAT is a device-provided data structure that is formatted similar to a > > platform-provided ACPI table. > > > >> Split out the common routine from ACPI to accomodate platforms that > >> do not support ACPI. The common routines can be built outside of ACPI if > >> ACPI_TABLES_LIB is selected. > > Might be just me but I get confused where this is indicating "ACPI" the > > platform vs "CONFIG_ACPI" the code. How about just: > > > > Refactor the table parsing routines in driver/acpi/tables.c into helpers > > that can be shared with the CXL driver even in the CONFIG_ACPI=n case. > > > >> Signed-off-by: Dave Jiang <dave.jiang@xxxxxxxxx> > >> --- > >> drivers/Makefile | 2 > >> drivers/acpi/Kconfig | 4 + > >> drivers/acpi/Makefile | 3 + > >> drivers/acpi/tables.c | 173 ---------------------------------------- > >> drivers/acpi/tables_lib.c | 194 +++++++++++++++++++++++++++++++++++++++++++++ > >> include/linux/acpi.h | 63 +++++++++------ > >> 6 files changed, 241 insertions(+), 198 deletions(-) > >> create mode 100644 drivers/acpi/tables_lib.c > > Conversion looks ok to me. Even though the cover letter said "Hi Rafael, > > Please consider these for 6.5 merge window" my expectation is to take > > these through CXL with ACPI acks. > > I thought you wanted Rafael to take the ACPI patches. But going to the > CXL tree works. Ultimately up to Rafael. Either need a stable ACPI tree baseline to base the CDAT work upon, or take this all through CXL. > > > > > One question below: > > > >> diff --git a/drivers/Makefile b/drivers/Makefile > >> index 20b118dca999..1824797f7dfe 100644 > >> --- a/drivers/Makefile > >> +++ b/drivers/Makefile > >> @@ -31,7 +31,7 @@ obj-y += idle/ > >> # IPMI must come before ACPI in order to provide IPMI opregion support > >> obj-y += char/ipmi/ > >> > >> -obj-$(CONFIG_ACPI) += acpi/ > >> +obj-y += acpi/ > >> > >> # PnP must come after ACPI since it will eventually need to check if acpi > >> # was used and do nothing if so > >> diff --git a/drivers/acpi/Kconfig b/drivers/acpi/Kconfig > >> index ccbeab9500ec..ce74a20dc42f 100644 > >> --- a/drivers/acpi/Kconfig > >> +++ b/drivers/acpi/Kconfig > >> @@ -6,12 +6,16 @@ > >> config ARCH_SUPPORTS_ACPI > >> bool > >> > >> +config ACPI_TABLES_LIB > >> + bool > >> + > >> menuconfig ACPI > >> bool "ACPI (Advanced Configuration and Power Interface) Support" > >> depends on ARCH_SUPPORTS_ACPI > >> select PNP > >> select NLS > >> select CRC32 > >> + select ACPI_TABLES_LIB > >> default y if X86 > >> help > >> Advanced Configuration and Power Interface (ACPI) support for > >> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile > >> index feb36c0b9446..4558e2876823 100644 > >> --- a/drivers/acpi/Makefile > >> +++ b/drivers/acpi/Makefile > >> @@ -13,6 +13,9 @@ tables.o: $(src)/../../include/$(CONFIG_ACPI_CUSTOM_DSDT_FILE) ; > >> > >> endif > >> > >> +obj-$(CONFIG_ACPI_TABLES_LIB) += acpi_tables_lib.o > >> +acpi_tables_lib-y := tables_lib.o > > Why is a separate object name needed? > > Not all code in tables.c will be shared. There are ACPI table parsing > specific code in tables.c that CXL does not care about. Or do you mean > just do > > obj-$(CONFIG_ACPI_TABLES_LIB) += tables_lib.o Yes, this.
