Re: [PATCH v2 1/4] acpi: Move common tables helper functions to common lib

[Dave Jiang <dave.jiang@intel.com>]

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@intel.com>
>> ---
>>   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.

>
> 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

?