Re: [PATCH v2] gpio: acpi: Call enable_irq_wake for _IAE GpioInts with Wake set

["Rafael J. Wysocki" <rjw@xxxxxxxxxxxxx>]

On Friday, March 24, 2017 10:51:53 AM Hans de Goede wrote:
> Hi,
> 
> On 20-03-17 22:59, Rafael J. Wysocki wrote:
> > On Monday, March 20, 2017 06:32:21 PM Hans de Goede wrote:
> >> On Bay Trail / Cherry Trail systems with a LID switch, the LID switch is
> >> often connect to a gpioint handled by an _IAE event handler.
> >> Before this commit such systems would not wake up when opening the lid,
> >> requiring the powerbutton to be pressed after opening the lid to wakeup.
> >>
> >> Note that Bay Trail / Cherry Trail systems use suspend-to-idle, so
> >> the interrupts are generated anyway on those lines on lid switch changes,
> >> but they are treated by the IRQ subsystem as spurious while suspended if
> >> not marked as wakeup IRQs.
> >>
> >> This commit calls enable_irq_wake() for _IAE GpioInts with a valid
> >> event handler which have their Wake flag set. This fixes such systems
> >> not waking up when opening the lid.
> >>
> >> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> >> Acked-by: Mika Westerberg <mika.westerberg@xxxxxxxxxxxxxxx>
> >> ---
> >> Changes in v2:
> >> -Improve commit msg
> >> -Add Mika's Acked-by
> >> ---
> >>  drivers/gpio/gpiolib-acpi.c | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> >> index 8cd3f66..18207b2 100644
> >> --- a/drivers/gpio/gpiolib-acpi.c
> >> +++ b/drivers/gpio/gpiolib-acpi.c
> >> @@ -28,6 +28,7 @@ struct acpi_gpio_event {
> >>  	acpi_handle handle;
> >>  	unsigned int pin;
> >>  	unsigned int irq;
> >> +	bool irq_wake_enabled;
> >>  	struct gpio_desc *desc;
> >>  };
> >>
> >> @@ -266,6 +267,11 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> >>  		goto fail_free_event;
> >>  	}
> >>
> >> +	if (agpio->wake_capable == ACPI_WAKE_CAPABLE) {
> >> +		enable_irq_wake(irq);
> >> +		event->irq_wake_enabled = true;
> >> +	}
> >> +
> >>  	list_add_tail(&event->node, &acpi_gpio->events);
> >>  	return AE_OK;
> >>
> >> @@ -339,6 +345,9 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
> >>  	list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->events, node) {
> >>  		struct gpio_desc *desc;
> >>
> >> +		if (event->irq_wake_enabled)
> >
> > It has just occurred to me that if the event is in the list, the IRQ will be
> > enabled to wake up as long as agpio->wake_capable == ACPI_WAKE_CAPABLE, so it
> > looks like it should be sufficient to check
> >
> > 	if (agpio->wake_capable == ACPI_WAKE_CAPABLE)
> >
> > here and the new flag is not necessary.  Or is it?
> 
> We don't have (readily available) access to the acpi_resource_gpio struct
> in acpi_gpiochip_free_interrupts, so I'm going to go with Andy's suggestion
> instead and change the if to:
> 
>                  if (irqd_is_wakeup_set(irq_get_irq_data(event->irq)))
>                          disable_irq_wake(event->irq);
> 
> Still need to run some quick tests and then I will send v3 with this
> change.

OK, fair enough.

Thanks,
Rafael

--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html