| Message ID | 20170320173221.3397-1-hdegoede@redhat.com |
|---|---|
| State | New |
| Headers | show |
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@redhat.com> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > --- > 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? > + disable_irq_wake(event->irq); > + > free_irq(event->irq, event); > desc = event->desc; > if (WARN_ON(IS_ERR(desc))) > Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2017-03-20 at 18:32 +0100, 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@redhat.com> > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > > + bool irq_wake_enabled; > > + if (event->irq_wake_enabled) > Same (new) comment as in v1.
On Tue, Mar 21, 2017 at 1:30 PM, Andy Shevchenko <andriy.shevchenko@linux.intel.com> wrote: > On Mon, 2017-03-20 at 18:32 +0100, 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@redhat.com> >> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> > >> + bool irq_wake_enabled; >> > >> + if (event->irq_wake_enabled) >> > > Same (new) comment as in v1. Why not just check for agpio->wake_capable == ACPI_WAKE_CAPABLE instead as I said? Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2017-03-21 at 13:36 +0100, Rafael J. Wysocki wrote: > On Tue, Mar 21, 2017 at 1:30 PM, Andy Shevchenko > <andriy.shevchenko@linux.intel.com> wrote: > > On Mon, 2017-03-20 at 18:32 +0100, Hans de Goede wrote: > > > + bool irq_wake_enabled; > > > > > > + if (event->irq_wake_enabled) > > > > > > > Same (new) comment as in v1. > > Why not just check for agpio->wake_capable == ACPI_WAKE_CAPABLE > instead as I said? Ah, yes, it could work as well as far as I can see from the code point of view.
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@redhat.com> >> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> >> --- >> 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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-gpio" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@redhat.com> > >> Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> > >> --- > >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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) + disable_irq_wake(event->irq); + free_irq(event->irq, event); desc = event->desc; if (WARN_ON(IS_ERR(desc)))