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
