Re: [PATCH] acpi: allow building without CONFIG_HAS_IOPORT

[Arnd Bergmann]

On Wed, Oct 9, 2024, at 19:40, Rafael J. Wysocki wrote:
> On Mon, Oct 7, 2024 at 9:23 PM Arnd Bergmann <arnd@xxxxxxxx> wrote:
>> diff --git a/drivers/acpi/Makefile b/drivers/acpi/Makefile
>> index 61ca4afe83dc..132357815324 100644
>> --- a/drivers/acpi/Makefile
>> +++ b/drivers/acpi/Makefile
>> @@ -41,7 +41,7 @@ acpi-y                                += resource.o
>>  acpi-y                         += acpi_processor.o
>>  acpi-y                         += processor_core.o
>>  acpi-$(CONFIG_ARCH_MIGHT_HAVE_ACPI_PDC) += processor_pdc.o
>> -acpi-y                         += ec.o
>> +acpi-$(CONFIG_HAS_IOPORT)      += ec.o
>>  acpi-$(CONFIG_ACPI_DOCK)       += dock.o
>>  acpi-$(CONFIG_PCI)             += pci_root.o pci_link.o pci_irq.o
>>  obj-$(CONFIG_ACPI_MCFG)                += pci_mcfg.o
>> diff --git a/drivers/acpi/bus.c b/drivers/acpi/bus.c
>> index 16917dc3ad60..535d6a72ce1b 100644
>> --- a/drivers/acpi/bus.c
>> +++ b/drivers/acpi/bus.c
>> @@ -1356,7 +1356,8 @@ static int __init acpi_bus_init(void)
>>          * Do that before calling acpi_initialize_objects() which may trigger EC
>>          * address space accesses.
>>          */
>> -       acpi_ec_ecdt_probe();
>> +       if (IS_ENABLED(CONFIG_HAS_IOPORT))
>> +               acpi_ec_ecdt_probe();
>>
>>         status = acpi_enable_subsystem(ACPI_NO_ACPI_ENABLE);
>>         if (ACPI_FAILURE(status)) {
>> @@ -1391,7 +1392,8 @@ static int __init acpi_bus_init(void)
>>          * Maybe EC region is required at bus_scan/acpi_get_devices. So it
>>          * is necessary to enable it as early as possible.
>>          */
>> -       acpi_ec_dsdt_probe();
>> +       if (IS_ENABLED(CONFIG_HAS_IOPORT))
>> +               acpi_ec_dsdt_probe();
>
> The above two changes mean that it is not necessary to compile either
> acpi_ec_ecdt_probe() or acpi_ec_dsdt_probe() at all if
> CONFIG_HAS_IOPORT is not enabled.

Correct, this is a result of removing ec.o from the list of objects.

>>         pr_info("Interpreter enabled\n");
>>
>> @@ -1464,7 +1466,8 @@ static int __init acpi_init(void)
>>         acpi_arm_init();
>>         acpi_riscv_init();
>>         acpi_scan_init();
>> -       acpi_ec_init();
>> +       if (IS_ENABLED(CONFIG_HAS_IOPORT))
>> +               acpi_ec_init();
>
> And this means that the whole EC driver is not going to work at all then.

Correct. The way I read ec.c it makes no sense if acpi_ec_write_cmd()
and acpi_ec_write_data() don't actually access the registers. Is
there anything in ec.c that still makes sense without port I/O?

>> diff --git a/drivers/acpi/processor_perflib.c b/drivers/acpi/processor_perflib.c
>> index 4265814c74f8..8be453d89ef8 100644
>> --- a/drivers/acpi/processor_perflib.c
>> +++ b/drivers/acpi/processor_perflib.c
>> @@ -455,7 +455,8 @@ int acpi_processor_pstate_control(void)
>>  {
>>         acpi_status status;
>>
>> -       if (!acpi_gbl_FADT.smi_command || !acpi_gbl_FADT.pstate_control)
>> +       if (!IS_ENABLED(CONFIG_HAS_IOPORT) ||
>> +           !acpi_gbl_FADT.smi_command || !acpi_gbl_FADT.pstate_control)
>>                 return 0;
>
> All of the existing callers of acpi_processor_pstate_control() are x86
> which has CONFIG_HAS_IOPORT AFAICS.
>
> And if you care about the code size, acpi_processor_notify_smm() can
> go away for !CONFIG_HAS_IOPORT too.
>
>>         pr_debug("Writing pstate_control [0x%x] to smi_command [0x%x]\n",
>> diff --git a/drivers/acpi/scan.c b/drivers/acpi/scan.c
>> index 7ecc401fb97f..9d5e6dd542bf 100644
>> --- a/drivers/acpi/scan.c
>> +++ b/drivers/acpi/scan.c
>> @@ -2293,7 +2293,8 @@ static int acpi_bus_attach(struct acpi_device *device, void *first_pass)
>>         if (device->handler)
>>                 goto ok;
>>
>> -       acpi_ec_register_opregions(device);
>> +       if (IS_ENABLED(CONFIG_HAS_IOPORT))
>> +               acpi_ec_register_opregions(device);
>
> I'd rather have an empty stub of acpi_ec_register_opregions() for
> !CONFIG_HAS_IOPORT instead.

Fair enough. Should I add stubs for all the global EC functions then?
I also wonder if there should be a separate CONFIG_ACPI_EC symbol
that allows the EC logic to be turned off on non-x86 architectures
even when HAS_IOPORT is otherwise enabled. 

      Arnd