On Monday, October 14, 2019 10:56:02 AM CET Daniel Drake wrote:
> As defined in the ACPI spec section 12.11, ACPI hardware-reduced
> platforms define the EC SCI interrupt as a GpioInt in the _CRS object.
>
> This replaces the previous way of using a GPE for this interrupt;
> GPE blocks are not available on reduced hardware platforms.
>
> Add support for handling this interrupt as an EC event source, and
> avoid GPE usage on reduced hardware platforms.
>
> This enables the use of several media keys (e.g. screen brightness
> up/down) on Asus UX434DA.
>
> Signed-off-by: Daniel Drake <drake@xxxxxxxxxxxx>
> ---
>
> Notes:
> v2:
> - Split renames into a preceding patch
> - Tried to address feedback. It wasn't super easy, further comments
> are very welcome.
>
> drivers/acpi/ec.c | 146 ++++++++++++++++++++++++++++++----------
> drivers/acpi/internal.h | 3 +-
> 2 files changed, 114 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index 97e08d0e3192..4b1a285c3c78 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -398,7 +398,7 @@ static void acpi_ec_submit_request(struct acpi_ec *ec)
> {
> ec->reference_count++;
> if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags) &&
> - ec->reference_count == 1)
> + ec->gpe >= 0 && ec->reference_count == 1)
> acpi_ec_enable_gpe(ec, true);
> }
>
> @@ -408,7 +408,7 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
>
> ec->reference_count--;
> if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags) &&
> - ec->reference_count == 0)
> + ec->gpe >= 0 && ec->reference_count == 0)
> acpi_ec_disable_gpe(ec, true);
> flushed = acpi_ec_flushed(ec);
> if (flushed)
> @@ -418,7 +418,10 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
> static void acpi_ec_mask_events(struct acpi_ec *ec)
> {
> if (!test_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags)) {
> - acpi_ec_disable_gpe(ec, false);
> + if (ec->gpe >= 0)
> + acpi_ec_disable_gpe(ec, false);
> + else
> + disable_irq_nosync(ec->irq);
> ec_dbg_drv("Polling enabled");
> set_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags);
> }
> @@ -428,7 +431,10 @@ static void acpi_ec_unmask_events(struct acpi_ec *ec)
> {
> if (test_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags)) {
> clear_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags);
> - acpi_ec_enable_gpe(ec, false);
> + if (ec->gpe >= 0)
> + acpi_ec_enable_gpe(ec, false);
> + else
> + enable_irq(ec->irq);
> ec_dbg_drv("Polling disabled");
> }
> }
> @@ -648,7 +654,8 @@ static void advance_transaction(struct acpi_ec *ec)
> * ensure a hardware STS 0->1 change after this clearing can always
> * trigger a GPE interrupt.
> */
> - acpi_ec_clear_gpe(ec);
> + if (ec->gpe >= 0)
> + acpi_ec_clear_gpe(ec);
> status = acpi_ec_read_status(ec);
> t = ec->curr;
> /*
> @@ -1275,18 +1282,28 @@ static void acpi_ec_event_handler(struct work_struct *work)
> acpi_ec_check_event(ec);
> }
>
> -static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
> - u32 gpe_number, void *data)
> +static void acpi_ec_handle_interrupt(struct acpi_ec *ec)
> {
> unsigned long flags;
> - struct acpi_ec *ec = data;
>
> spin_lock_irqsave(&ec->lock, flags);
> advance_transaction(ec);
> spin_unlock_irqrestore(&ec->lock, flags);
> +}
> +
> +static u32 acpi_ec_gpe_handler(acpi_handle gpe_device,
> + u32 gpe_number, void *data)
> +{
> + acpi_ec_handle_interrupt(data);
> return ACPI_INTERRUPT_HANDLED;
> }
>
> +static irqreturn_t acpi_ec_irq_handler(int irq, void *data)
> +{
> + acpi_ec_handle_interrupt(data);
> + return IRQ_HANDLED;
> +}
> +
> /* --------------------------------------------------------------------------
> * Address Space Management
> * -------------------------------------------------------------------------- */
> @@ -1359,6 +1376,8 @@ static struct acpi_ec *acpi_ec_alloc(void)
> ec->timestamp = jiffies;
> ec->busy_polling = true;
> ec->polling_guard = 0;
> + ec->gpe = -1;
> + ec->irq = -1;
> return ec;
> }
>
> @@ -1406,9 +1425,12 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> /* Get GPE bit assignment (EC events). */
> /* TODO: Add support for _GPE returning a package */
> status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
> - if (ACPI_FAILURE(status))
> - return status;
> - ec->gpe = tmp;
> + if (ACPI_SUCCESS(status))
> + ec->gpe = tmp;
> + /*
> + * Errors are non-fatal, allowing for ACPI Reduced Hardware
> + * platforms which use GpioInt instead of GPE.
> + */
> }
> /* Use the global lock for all EC transactions? */
> tmp = 0;
> @@ -1418,12 +1440,57 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> return AE_CTRL_TERMINATE;
> }
>
> +static void install_gpe_event_handler(struct acpi_ec *ec)
> +{
> + acpi_status status =
> + acpi_install_gpe_raw_handler(NULL, ec->gpe,
> + ACPI_GPE_EDGE_TRIGGERED,
> + &acpi_ec_gpe_handler,
> + ec);
> + if (ACPI_SUCCESS(status)) {
> + /* This is not fatal as we can poll EC events */
> + set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
> + acpi_ec_leave_noirq(ec);
> + if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
> + ec->reference_count >= 1)
> + acpi_ec_enable_gpe(ec, true);
> + }
> +}
> +
> +/* ACPI reduced hardware platforms use a GpioInt specified in _CRS. */
> +static int install_gpio_irq_event_handler(struct acpi_ec *ec,
> + struct acpi_device *device)
> +{
> + int irq = acpi_dev_gpio_irq_get(device, 0);
> + int ret;
> +
> + if (irq < 0)
> + return irq;
> +
> + ret = request_irq(irq, acpi_ec_irq_handler, IRQF_SHARED,
> + "ACPI EC", ec);
> +
> + /*
> + * Unlike the GPE case, we treat errors here as fatal, we'll only
> + * implement GPIO polling if we find a case that needs it.
> + */
> + if (ret < 0)
> + return ret;
> +
> + ec->irq = irq;
> + set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
> + acpi_ec_leave_noirq(ec);
> +
> + return 0;
> +}
> +
> /*
> * Note: This function returns an error code only when the address space
> * handler is not installed, which means "not able to handle
> * transactions".
> */
> -static int ec_install_handlers(struct acpi_ec *ec, bool handle_events)
> +static int ec_install_handlers(struct acpi_ec *ec, struct acpi_device *device,
> + bool handle_events)
> {
> acpi_status status;
>
> @@ -1464,16 +1531,15 @@ static int ec_install_handlers(struct acpi_ec *ec, bool handle_events)
> set_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags);
> }
> if (!test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
> - status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
> - ACPI_GPE_EDGE_TRIGGERED,
> - &acpi_ec_gpe_handler, ec);
> - /* This is not fatal as we can poll EC events */
> - if (ACPI_SUCCESS(status)) {
> - set_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
> - acpi_ec_leave_noirq(ec);
> - if (test_bit(EC_FLAGS_STARTED, &ec->flags) &&
> - ec->reference_count >= 1)
> - acpi_ec_enable_gpe(ec, true);
> + if (ec->gpe >= 0) {
> + install_gpe_event_handler(ec);
> + } else if (device) {
> + int ret = install_gpio_irq_event_handler(ec, device);
> +
> + if (ret)
> + return ret;
> + } else { /* No GPE and no GpioInt? */
> + return -ENODEV;
> }
> }
> /* EC is fully operational, allow queries */
> @@ -1505,9 +1571,13 @@ static void ec_remove_handlers(struct acpi_ec *ec)
> acpi_ec_stop(ec, false);
>
> if (test_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags)) {
> - if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
> - &acpi_ec_gpe_handler)))
> + if (ec->gpe >= 0 &&
> + ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
> + &acpi_ec_gpe_handler)))
> pr_err("failed to remove gpe handler\n");
> + if (ec->irq >= 0)
> + free_irq(ec->irq, ec);
> +
> clear_bit(EC_FLAGS_EVENT_HANDLER_INSTALLED, &ec->flags);
> }
> if (test_bit(EC_FLAGS_QUERY_METHODS_INSTALLED, &ec->flags)) {
> @@ -1516,11 +1586,12 @@ static void ec_remove_handlers(struct acpi_ec *ec)
> }
> }
>
> -static int acpi_ec_setup(struct acpi_ec *ec, bool handle_events)
> +static int acpi_ec_setup(struct acpi_ec *ec, struct acpi_device *device,
> + bool handle_events)
> {
> int ret;
>
> - ret = ec_install_handlers(ec, handle_events);
> + ret = ec_install_handlers(ec, device, handle_events);
> if (ret)
> return ret;
>
> @@ -1531,8 +1602,8 @@ static int acpi_ec_setup(struct acpi_ec *ec, bool handle_events)
> }
>
> acpi_handle_info(ec->handle,
> - "GPE=0x%x, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
> - ec->gpe, ec->command_addr, ec->data_addr);
> + "GPE=0x%x, IRQ=%d, EC_CMD/EC_SC=0x%lx, EC_DATA=0x%lx\n",
> + ec->gpe, ec->irq, ec->command_addr, ec->data_addr);
> return ret;
> }
>
> @@ -1596,7 +1667,7 @@ static int acpi_ec_add(struct acpi_device *device)
> }
> }
>
> - ret = acpi_ec_setup(ec, true);
> + ret = acpi_ec_setup(ec, device, true);
> if (ret)
> goto err_query;
>
> @@ -1716,7 +1787,7 @@ void __init acpi_ec_dsdt_probe(void)
> * At this point, the GPE is not fully initialized, so do not to
> * handle the events.
> */
> - ret = acpi_ec_setup(ec, false);
> + ret = acpi_ec_setup(ec, NULL, false);
> if (ret) {
> acpi_ec_free(ec);
> return;
> @@ -1889,14 +1960,21 @@ void __init acpi_ec_ecdt_probe(void)
> ec->command_addr = ecdt_ptr->control.address;
> ec->data_addr = ecdt_ptr->data.address;
> }
> - ec->gpe = ecdt_ptr->gpe;
> +
> + /*
> + * Ignore the GPE value on Reduced Hardware platforms.
> + * Some products have this set to an erroneous value.
> + */
> + if (!acpi_gbl_reduced_hardware)
> + ec->gpe = ecdt_ptr->gpe;
> +
> ec->handle = ACPI_ROOT_OBJECT;
>
> /*
> * At this point, the namespace is not initialized, so do not find
> * the namespace objects, or handle the events.
> */
> - ret = acpi_ec_setup(ec, false);
> + ret = acpi_ec_setup(ec, NULL, false);
> if (ret) {
> acpi_ec_free(ec);
> return;
> @@ -1928,7 +2006,7 @@ static int acpi_ec_suspend_noirq(struct device *dev)
> * masked at the low level without side effects.
> */
> if (ec_no_wakeup && test_bit(EC_FLAGS_STARTED, &ec->flags) &&
> - ec->reference_count >= 1)
> + ec->gpe >= 0 && ec->reference_count >= 1)
> acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_DISABLE);
>
> acpi_ec_enter_noirq(ec);
> @@ -1943,7 +2021,7 @@ static int acpi_ec_resume_noirq(struct device *dev)
> acpi_ec_leave_noirq(ec);
>
> if (ec_no_wakeup && test_bit(EC_FLAGS_STARTED, &ec->flags) &&
> - ec->reference_count >= 1)
> + ec->gpe >= 0 && ec->reference_count >= 1)
> acpi_set_gpe(NULL, ec->gpe, ACPI_GPE_ENABLE);
>
> return 0;
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index afe6636f9ad3..3616daec650b 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -165,7 +165,8 @@ static inline void acpi_early_processor_osc(void) {}
> -------------------------------------------------------------------------- */
> struct acpi_ec {
> acpi_handle handle;
> - u32 gpe;
> + int gpe;
> + int irq;
> unsigned long command_addr;
> unsigned long data_addr;
> bool global_lock;
>
Applying the series as 5.5 material, thanks!
