ACPI: EC: add support for hardware-reduced systems
PATCH 1/1 ACPI: EC: add support for hardware-reduced systems
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.
--- drivers/acpi/ec.c | 81 +++++++++++++++++++++++++++++++---------- drivers/acpi/internal.h | 1 + 2 files changed, 63 insertions(+), 19 deletions(-)
drivers/acpi/ec.c +62 -19
@@ -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);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);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) {/* 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); } -if (!test_bit (EC_FLAGS_GPE_HANDLER_INSTALLED , &ec->flags)) { +if (!acpi_gbl_reduced_hardware && + !test_bit (EC_FLAGS_GPE_HANDLER_INSTALLED , &ec->flags)) { status =acpi_install_gpe_raw_handler (NULL , ec->gpe,ACPI_GPE_EDGE_TRIGGERED , &acpi_ec_gpe_handler, ec);
@@ -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; + }/* 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; + } } ret =acpi_ec_setup (ec, true);
drivers/acpi/internal.h +1 -0
@@ -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;
Signature · 1 line
2.20.1
Sorry for the delay.
Quoted 6 lines · CEST Daniel Drake
As defined in the ACPI spec section 12.11, ACPI hardware-reduced platforms define the EC SCI interrupt as a GpioInt in the
_CRSobject.This replaces the previous way of using a GPE for this interrupt; GPE blocks are not available on reduced hardware platforms.
Quoted 7 lines
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 <[email protected]>
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.
Quoted 22 lines
--- drivers/acpi/ec.c | 81 +++++++++++++++++++++++++++++++---------- drivers/acpi/internal.h | 1 + 2 files changed, 63 insertions(+), 19 deletions(-)drivers/acpi/ec.c +1
-1@@ -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); }
drivers/acpi/ec.c static void acpi_ec_complete_request(struct acpi_ec *ec) · line 423
Quoted 10 lines
drivers/acpi/ec.c +6
-3-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.
Quoted 79 lines
drivers/acpi/ec.c +15
-13ec_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);
drivers/acpi/ec.c static void acpi_ec_event_handler(struct work_struct *work) · line 1316
Quoted 10 lines
drivers/acpi/ec.c +8
-0advance_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.
Quoted 23 lines
drivers/acpi/ec.c +9
-0return 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; }
drivers/acpi/ec.c ec_parse_device(acpi_handle handle, u32 Level, void *context, void **retval) · line 1444
Quoted 5 lines
drivers/acpi/ec.c +1
-1@@ -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.
drivers/acpi/ec.c static int ec_install_handlers(struct acpi_ec *ec, bool handle_events) · line 1503
Quoted 6 lines
drivers/acpi/ec.c +0
-0/* 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 ->
drivers/acpi/ec.c static int ec_install_handlers(struct acpi_ec *ec, bool handle_events) · line 1483
Quoted 2 lines
drivers/acpi/ec.c +0
-1} -if (!test_bit (EC_FLAGS_GPE_HANDLER_INSTALLED , &ec->flags)) {
and this to EC_FLAGS_EVTENT_HANDLER_INSTALLED ->>
drivers/acpi/ec.c static int ec_install_handlers(struct acpi_ec *ec, bool handle_events) · line 1506
Quoted 2 lines
drivers/acpi/ec.c +2
-0+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.
drivers/acpi/ec.c static int ec_install_handlers(struct acpi_ec *ec, bool handle_events) · line 1509
Quoted 3 lines
drivers/acpi/ec.c +0
-0status =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.
drivers/acpi/ec.c static int ec_install_handlers(struct acpi_ec *ec, bool handle_events) · line 1524
Quoted 10 lines
drivers/acpi/ec.c +6
-0@@ -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.
Quoted 18 lines
drivers/acpi/ec.c +4
-0/* 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; } + +/*
drivers/acpi/ec.c static int acpi_ec_add(struct acpi_device *device) · line 1656
Quoted 10 lines
drivers/acpi/ec.c +10
-0+* 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.
Quoted 16 lines
drivers/acpi/ec.c +0
-0} ret =acpi_ec_setup (ec, true);drivers/acpi/internal.h +1
-0@@ -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;