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

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

On 6/2/23 05:35, Jonathan Cameron wrote:
> On Thu, 01 Jun 2023 14:31:52 -0700
> Dave Jiang <dave.jiang@intel.com> wrote:
>
>> Some of the routines in ACPI driver/acpi/tables.c can be shared with
>> parsing CDAT. CDAT is a device-provided data structure that is formatted
>> similar to a platform provided ACPI table. CDAT is used by CXL and can
>> exist on platforms that do not use ACPI. Split out the common routine
>> from ACPI to accommodate platforms that do not support ACPI and move that
>> to /lib. The common routines can be built outside of ACPI if
>> FIRMWARE_TABLES is selected.
>>
>> Link: https://lore.kernel.org/linux-cxl/CAJZ5v0jipbtTNnsA0-o5ozOk8ZgWnOg34m34a9pPenTyRLj=6A@mail.gmail.com/
>> Suggested-by: Rafael J. Wysocki <rafael@kernel.org>
>> Signed-off-by: Dave Jiang <dave.jiang@intel.com>
> Minor comment to fix inline. With that tidied up
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>
>
>> diff --git a/include/linux/fw_table.h b/include/linux/fw_table.h
>> new file mode 100644
>> index 000000000000..ff8fa58d5818
>> --- /dev/null
>> +++ b/include/linux/fw_table.h
>> @@ -0,0 +1,43 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + *  fw_tables.h - Parsing support for ACPI and ACPI-like tables provided by
>> + *                platform or device firmware
>> + *
>> + *  Copyright (C) 2001 Paul Diefenbaugh <paul.s.diefenbaugh@intel.com>
>> + *  Copyright (C) 2023 Intel Corp.
>> + */
>> +#ifndef _FW_TABLE_H_
>> +#define _FW_TABLE_H_
>> +
>> +union acpi_subtable_headers;
>> +
>> +typedef int (*acpi_tbl_entry_handler)(union acpi_subtable_headers *header,
>> +				      const unsigned long end);
>> +
>> +typedef int (*acpi_tbl_entry_handler_arg)(union acpi_subtable_headers *header,
>> +					  void *arg, const unsigned long end);
>> +
>> +struct acpi_subtable_proc {
>> +	int id;
>> +	acpi_tbl_entry_handler handler;
>> +	acpi_tbl_entry_handler_arg handler_arg;
>> +	void *arg;
>> +	int count;
>> +};
>> +
>> +#include <linux/acpi.h>
>> +#include <acpi/acpi.h>
> Includes mid way down the files is not a common pattern and I can't see why
> it's particularly useful to do so here.
>
> + linux/acpi.h includes acpi/acpi.h and I can't see that changing any time
> soon...

Unfortunately because linux/acpi.h needs defines from this header file, 
and this header file needs ACPI table definitions from acpi/acpi.h, and 
acpi/acpi.h can't be included independently without linux/acpi.h, this 
seems to be the only way to get everything to build w/o complaints from 
the compiler. This header really just needs some ACPI table struct 
definitions but there's no easy way to just include those.  Also having 
the headers in the beginning causes compiler errors. It's ugly but seems 
to be the way things will build cleanly.


>> +
>> +union acpi_subtable_headers {
>> +	struct acpi_subtable_header common;
>> +	struct acpi_hmat_structure hmat;
>> +	struct acpi_prmt_module_header prmt;
>> +	struct acpi_cedt_header cedt;
>> +};
>> +
>> +int acpi_parse_entries_array(char *id, unsigned long table_size,
>> +			     struct acpi_table_header *table_header,
>> +			     struct acpi_subtable_proc *proc,
>> +			     int proc_num, unsigned int max_entries);
>> +
>> +#endif
>