Sorry for the delay.
On Wednesday, September 4, 2019 8:34:43 AM CEST 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>
Just as a matter of clarity, I would like the changes to be split into the
rename of EC_FLAGS_GPE_MASKED (without functional impact) and the rest.
> ---
> drivers/acpi/ec.c | 81 +++++++++++++++++++++++++++++++----------
> drivers/acpi/internal.h | 1 +
> 2 files changed, 63 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/acpi/ec.c b/drivers/acpi/ec.c
> index c33756ed3304..37bbec89be82 100644
> --- a/drivers/acpi/ec.c
> +++ b/drivers/acpi/ec.c
> @@ -99,7 +99,7 @@ enum {
> EC_FLAGS_EVT_HANDLER_INSTALLED, /* _Qxx handlers installed */
> EC_FLAGS_STARTED, /* Driver is started */
> EC_FLAGS_STOPPED, /* Driver is stopped */
> - EC_FLAGS_GPE_MASKED, /* GPE masked */
> + EC_FLAGS_EVENTS_MASKED, /* GPE or IRQ masked */
> };
>
> #define ACPI_EC_COMMAND_POLL 0x01 /* Available for command byte */
> @@ -414,20 +414,26 @@ static void acpi_ec_complete_request(struct acpi_ec *ec)
> wake_up(&ec->wait);
> }
>
> -static void acpi_ec_mask_gpe(struct acpi_ec *ec)
> +static void acpi_ec_mask_events(struct acpi_ec *ec)
> {
> - if (!test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
> - acpi_ec_disable_gpe(ec, false);
> + if (!test_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags)) {
> + if (ec->irq >= 0)
> + disable_irq_nosync(ec->irq);
> + else
> + acpi_ec_disable_gpe(ec, false);
So IMO it would be better to change the gpe data type to int and initialize it
to -1.
Then, you can do
if (ec->gpe >= 0)
acpi_ec_disable_gpe(ec, false);
else
disable_irq_nosync(ec->irq);
and also check gpe in some other places like acpi_ec_suspend/resume_noirq().
And fail the initialization when you cannot get neither the GPE number nor
the interrupt.
> ec_dbg_drv("Polling enabled");
> - set_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
> + set_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags);
> }
> }
>
> -static void acpi_ec_unmask_gpe(struct acpi_ec *ec)
> +static void acpi_ec_unmask_events(struct acpi_ec *ec)
> {
> - if (test_bit(EC_FLAGS_GPE_MASKED, &ec->flags)) {
> - clear_bit(EC_FLAGS_GPE_MASKED, &ec->flags);
> - acpi_ec_enable_gpe(ec, false);
> + if (test_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags)) {
> + clear_bit(EC_FLAGS_EVENTS_MASKED, &ec->flags);
> + if (ec->irq >= 0)
> + enable_irq(ec->irq);
> + else
> + acpi_ec_enable_gpe(ec, false);
> ec_dbg_drv("Polling disabled");
> }
> }
> @@ -453,7 +459,7 @@ static bool acpi_ec_submit_flushable_request(struct acpi_ec *ec)
>
> static void acpi_ec_submit_query(struct acpi_ec *ec)
> {
> - acpi_ec_mask_gpe(ec);
> + acpi_ec_mask_events(ec);
> if (!acpi_ec_event_enabled(ec))
> return;
> if (!test_and_set_bit(EC_FLAGS_QUERY_PENDING, &ec->flags)) {
> @@ -469,7 +475,7 @@ static void acpi_ec_complete_query(struct acpi_ec *ec)
> if (test_and_clear_bit(EC_FLAGS_QUERY_PENDING, &ec->flags))
> ec_dbg_evt("Command(%s) unblocked",
> acpi_ec_cmd_string(ACPI_EC_COMMAND_QUERY));
> - acpi_ec_unmask_gpe(ec);
> + acpi_ec_unmask_events(ec);
> }
>
> static inline void __acpi_ec_enable_event(struct acpi_ec *ec)
> @@ -647,7 +653,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->irq < 0)
> + acpi_ec_clear_gpe(ec);
> status = acpi_ec_read_status(ec);
> t = ec->curr;
> /*
> @@ -716,7 +723,7 @@ static void advance_transaction(struct acpi_ec *ec)
> ++t->irq_count;
> /* Allow triggering on 0 threshold */
> if (t->irq_count == ec_storm_threshold)
> - acpi_ec_mask_gpe(ec);
> + acpi_ec_mask_events(ec);
> }
> }
> out:
> @@ -814,7 +821,7 @@ static int acpi_ec_transaction_unlocked(struct acpi_ec *ec,
>
> spin_lock_irqsave(&ec->lock, tmp);
> if (t->irq_count == ec_storm_threshold)
> - acpi_ec_unmask_gpe(ec);
> + acpi_ec_unmask_events(ec);
> ec_dbg_req("Command(%s) stopped", acpi_ec_cmd_string(t->command));
> ec->curr = NULL;
> /* Disable GPE for command processing (IBF=0/OBF=1) */
> @@ -1292,18 +1299,32 @@ 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)
> +{
> + struct acpi_ec *ec = data;
> +
> + acpi_ec_handle_interrupt(ec);
You can pass data here without using the extra local var.
> return ACPI_INTERRUPT_HANDLED;
> }
>
> +static irqreturn_t acpi_ec_irq_handler(int irq, void *data)
> +{
> + struct acpi_ec *ec = data;
> +
> + acpi_ec_handle_interrupt(ec);
> + return IRQ_HANDLED;
> +}
> +
> /* --------------------------------------------------------------------------
> * Address Space Management
> * -------------------------------------------------------------------------- */
> @@ -1376,6 +1397,7 @@ static struct acpi_ec *acpi_ec_alloc(void)
> ec->timestamp = jiffies;
> ec->busy_polling = true;
> ec->polling_guard = 0;
> + ec->irq = -1;
> return ec;
> }
>
> @@ -1419,7 +1441,7 @@ ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval)
> * EC.
> */
> ec->gpe = boot_ec->gpe;
> - } else {
> + } else if (!acpi_gbl_reduced_hardware) {
I'm not sure if this check is necessary.
if _GPE is not present, you can assume hardware-reduced and look for the GPIO
IRQ.
> /* Get GPE bit assignment (EC events). */
> /* TODO: Add support for _GPE returning a package */
> status = acpi_evaluate_integer(handle, "_GPE", NULL, &tmp);
> @@ -1480,7 +1502,8 @@ static int ec_install_handlers(struct acpi_ec *ec, bool handle_events)
> NULL, ec, NULL);
> set_bit(EC_FLAGS_EVT_HANDLER_INSTALLED, &ec->flags);
I would rename this to EC_FLAGS_QUERY_METHODS_INSTALLED ->
> }
> - if (!test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
and this to EC_FLAGS_EVTENT_HANDLER_INSTALLED ->>
> + if (!acpi_gbl_reduced_hardware &&
> + !test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
and I would use it to check if there is an "event" (either GPE or IRQ) handler
registered already.
> status = acpi_install_gpe_raw_handler(NULL, ec->gpe,
> ACPI_GPE_EDGE_TRIGGERED,
> &acpi_ec_gpe_handler, ec);
And there's some code below which I believe you kind of need for the IRQ case
too.
> @@ -1493,6 +1516,12 @@ static int ec_install_handlers(struct acpi_ec *ec, bool handle_events)
> acpi_ec_enable_gpe(ec, true);
> }
> }
> + if (ec->irq >= 0) {
> + int ret = request_irq(ec->irq, acpi_ec_irq_handler,
> + IRQF_SHARED, "ACPI EC", ec);
> + if (ret)
> + return ret;
> + }
I would move this to the "_HANDLER_INSTALLED" block above.
> /* EC is fully operational, allow queries */
> acpi_ec_enable_event(ec);
>
> @@ -1521,6 +1550,8 @@ static void ec_remove_handlers(struct acpi_ec *ec)
> */
> acpi_ec_stop(ec, false);
>
> + if (ec->irq >= 0)
> + free_irq(ec->irq, ec);
> if (test_bit(EC_FLAGS_GPE_HANDLER_INSTALLED, &ec->flags)) {
> if (ACPI_FAILURE(acpi_remove_gpe_handler(NULL, ec->gpe,
> &acpi_ec_gpe_handler)))
> @@ -1611,6 +1642,18 @@ static int acpi_ec_add(struct acpi_device *device)
> acpi_ec_free(ec);
> ec = boot_ec;
> }
> +
> + /*
> + * Reduced hartware platforms use a GpioInt specified in
> + * _CRS.
> + */
> + if (acpi_gbl_reduced_hardware) {
> + ret = acpi_dev_gpio_irq_get(device, 0);
> + if (ret == -EPROBE_DEFER)
> + goto err_alloc;
> +
> + ec->irq = ret;
> + }
And this should go to where you request the IRQ. There's no reason for it
to be here AFAICS.
> }
>
> ret = acpi_ec_setup(ec, true);
> diff --git a/drivers/acpi/internal.h b/drivers/acpi/internal.h
> index f4c2fe6be4f2..34c2c85e667e 100644
> --- a/drivers/acpi/internal.h
> +++ b/drivers/acpi/internal.h
> @@ -166,6 +166,7 @@ static inline void acpi_early_processor_osc(void) {}
> struct acpi_ec {
> acpi_handle handle;
> u32 gpe;
> + int irq;
> unsigned long command_addr;
> unsigned long data_addr;
> bool global_lock;
>
