Message ID | 3043137.BtEKcMiXSu@vostro.rjw.lan (mailing list archive) |
---|---|
State | Accepted, archived |
Headers | show |
On Tue, Apr 09, 2013 at 03:57:25PM +0200, Rafael J. Wysocki wrote: > +void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > +{ > + acpi_handle handle; > + acpi_status status; > + struct list_head *evt_pins; > + struct acpi_gpio_evt_pin *evt_pin, *ep; > + > + if (!chip->dev || !chip->to_irq) > + return; > + > + handle = ACPI_HANDLE(chip->dev); > + if (!handle) > + return; > + > + status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); > + if (ACPI_FAILURE(status)) > + return; > + > + list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { > + devm_free_irq(chip->dev, evt_pin->irq, evt_pin); How about using normal request/free_irq() functions for both _EVT and non-_EVT events? Since we now need to call acpi_gpiochip_free_interrupts() anyway, I don't see the point using devm_* functions here. > + list_del(&evt_pin->node); > + kfree(evt_pin); > + } > + > + acpi_detach_data(handle, acpi_gpio_evt_dh); > + kfree(evt_pins); > +} > +EXPORT_SYMBOL(acpi_gpiochip_free_interrupts); -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 04/10/2013 10:53 AM, Mika Westerberg wrote: > On Tue, Apr 09, 2013 at 03:57:25PM +0200, Rafael J. Wysocki wrote: >> +void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) >> +{ >> + acpi_handle handle; >> + acpi_status status; >> + struct list_head *evt_pins; >> + struct acpi_gpio_evt_pin *evt_pin, *ep; >> + >> + if (!chip->dev || !chip->to_irq) >> + return; >> + >> + handle = ACPI_HANDLE(chip->dev); >> + if (!handle) >> + return; >> + >> + status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); >> + if (ACPI_FAILURE(status)) >> + return; >> + >> + list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { >> + devm_free_irq(chip->dev, evt_pin->irq, evt_pin); > > How about using normal request/free_irq() functions for both _EVT and > non-_EVT events? Since we now need to call acpi_gpiochip_free_interrupts() > anyway, I don't see the point using devm_* functions here. > Then we need to create a list of non-_EVT events, or add them to the evt_pins list. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 10, 2013 at 11:17:57AM +0300, Mathias Nyman wrote: > On 04/10/2013 10:53 AM, Mika Westerberg wrote: > >On Tue, Apr 09, 2013 at 03:57:25PM +0200, Rafael J. Wysocki wrote: > >>+void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > >>+{ > >>+ acpi_handle handle; > >>+ acpi_status status; > >>+ struct list_head *evt_pins; > >>+ struct acpi_gpio_evt_pin *evt_pin, *ep; > >>+ > >>+ if (!chip->dev || !chip->to_irq) > >>+ return; > >>+ > >>+ handle = ACPI_HANDLE(chip->dev); > >>+ if (!handle) > >>+ return; > >>+ > >>+ status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); > >>+ if (ACPI_FAILURE(status)) > >>+ return; > >>+ > >>+ list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { > >>+ devm_free_irq(chip->dev, evt_pin->irq, evt_pin); > > > >How about using normal request/free_irq() functions for both _EVT and > >non-_EVT events? Since we now need to call acpi_gpiochip_free_interrupts() > >anyway, I don't see the point using devm_* functions here. > > > > Then we need to create a list of non-_EVT events, or add them to the > evt_pins list. Good point. Maybe we can add them to the evt_pins list and handle the same way as _EVT (except that we need to call _Exx and _Lxx methods instead of _EVT)? -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, April 10, 2013 12:17:47 PM Mika Westerberg wrote: > On Wed, Apr 10, 2013 at 11:17:57AM +0300, Mathias Nyman wrote: > > On 04/10/2013 10:53 AM, Mika Westerberg wrote: > > >On Tue, Apr 09, 2013 at 03:57:25PM +0200, Rafael J. Wysocki wrote: > > >>+void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > > >>+{ > > >>+ acpi_handle handle; > > >>+ acpi_status status; > > >>+ struct list_head *evt_pins; > > >>+ struct acpi_gpio_evt_pin *evt_pin, *ep; > > >>+ > > >>+ if (!chip->dev || !chip->to_irq) > > >>+ return; > > >>+ > > >>+ handle = ACPI_HANDLE(chip->dev); > > >>+ if (!handle) > > >>+ return; > > >>+ > > >>+ status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); > > >>+ if (ACPI_FAILURE(status)) > > >>+ return; > > >>+ > > >>+ list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { > > >>+ devm_free_irq(chip->dev, evt_pin->irq, evt_pin); > > > > > >How about using normal request/free_irq() functions for both _EVT and > > >non-_EVT events? Since we now need to call acpi_gpiochip_free_interrupts() > > >anyway, I don't see the point using devm_* functions here. > > > > > > > Then we need to create a list of non-_EVT events, or add them to the > > evt_pins list. > > Good point. Maybe we can add them to the evt_pins list and handle the same > way as _EVT (except that we need to call _Exx and _Lxx methods instead of > _EVT)? The difference is that the evt_pins data is only needed for _EVT execution, because _EVT takes the pin argument. _Lxx/_Exx don't take arguments and there's no need to add extra data structures for them, as devm_ does what's needed. Of course, plain request/free_irq may be used for the _EVT events only at the expense of a little more complexity in acpi_gpiochip_request_interrupts(). Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Apr 10, 2013 at 12:39:21PM +0200, Rafael J. Wysocki wrote: > On Wednesday, April 10, 2013 12:17:47 PM Mika Westerberg wrote: > > On Wed, Apr 10, 2013 at 11:17:57AM +0300, Mathias Nyman wrote: > > > On 04/10/2013 10:53 AM, Mika Westerberg wrote: > > > >On Tue, Apr 09, 2013 at 03:57:25PM +0200, Rafael J. Wysocki wrote: > > > >>+void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > > > >>+{ > > > >>+ acpi_handle handle; > > > >>+ acpi_status status; > > > >>+ struct list_head *evt_pins; > > > >>+ struct acpi_gpio_evt_pin *evt_pin, *ep; > > > >>+ > > > >>+ if (!chip->dev || !chip->to_irq) > > > >>+ return; > > > >>+ > > > >>+ handle = ACPI_HANDLE(chip->dev); > > > >>+ if (!handle) > > > >>+ return; > > > >>+ > > > >>+ status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); > > > >>+ if (ACPI_FAILURE(status)) > > > >>+ return; > > > >>+ > > > >>+ list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { > > > >>+ devm_free_irq(chip->dev, evt_pin->irq, evt_pin); > > > > > > > >How about using normal request/free_irq() functions for both _EVT and > > > >non-_EVT events? Since we now need to call acpi_gpiochip_free_interrupts() > > > >anyway, I don't see the point using devm_* functions here. > > > > > > > > > > Then we need to create a list of non-_EVT events, or add them to the > > > evt_pins list. > > > > Good point. Maybe we can add them to the evt_pins list and handle the same > > way as _EVT (except that we need to call _Exx and _Lxx methods instead of > > _EVT)? > > The difference is that the evt_pins data is only needed for _EVT execution, > because _EVT takes the pin argument. _Lxx/_Exx don't take arguments and > there's no need to add extra data structures for them, as devm_ does what's > needed. OK, thanks for the explanation. > Of course, plain request/free_irq may be used for the _EVT events only > at the expense of a little more complexity in acpi_gpiochip_request_interrupts(). I'm not sure whether it is a good thing to mix devm_ and plain request/free here. And more complexity is always bad so I guess we can stay with this implementation now. Feel free to add my Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, April 10, 2013 01:54:26 PM Mika Westerberg wrote: > On Wed, Apr 10, 2013 at 12:39:21PM +0200, Rafael J. Wysocki wrote: > > On Wednesday, April 10, 2013 12:17:47 PM Mika Westerberg wrote: > > > On Wed, Apr 10, 2013 at 11:17:57AM +0300, Mathias Nyman wrote: > > > > On 04/10/2013 10:53 AM, Mika Westerberg wrote: > > > > >On Tue, Apr 09, 2013 at 03:57:25PM +0200, Rafael J. Wysocki wrote: > > > > >>+void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) > > > > >>+{ > > > > >>+ acpi_handle handle; > > > > >>+ acpi_status status; > > > > >>+ struct list_head *evt_pins; > > > > >>+ struct acpi_gpio_evt_pin *evt_pin, *ep; > > > > >>+ > > > > >>+ if (!chip->dev || !chip->to_irq) > > > > >>+ return; > > > > >>+ > > > > >>+ handle = ACPI_HANDLE(chip->dev); > > > > >>+ if (!handle) > > > > >>+ return; > > > > >>+ > > > > >>+ status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); > > > > >>+ if (ACPI_FAILURE(status)) > > > > >>+ return; > > > > >>+ > > > > >>+ list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { > > > > >>+ devm_free_irq(chip->dev, evt_pin->irq, evt_pin); > > > > > > > > > >How about using normal request/free_irq() functions for both _EVT and > > > > >non-_EVT events? Since we now need to call acpi_gpiochip_free_interrupts() > > > > >anyway, I don't see the point using devm_* functions here. > > > > > > > > > > > > > Then we need to create a list of non-_EVT events, or add them to the > > > > evt_pins list. > > > > > > Good point. Maybe we can add them to the evt_pins list and handle the same > > > way as _EVT (except that we need to call _Exx and _Lxx methods instead of > > > _EVT)? > > > > The difference is that the evt_pins data is only needed for _EVT execution, > > because _EVT takes the pin argument. _Lxx/_Exx don't take arguments and > > there's no need to add extra data structures for them, as devm_ does what's > > needed. > > OK, thanks for the explanation. > > > Of course, plain request/free_irq may be used for the _EVT events only > > at the expense of a little more complexity in acpi_gpiochip_request_interrupts(). > > I'm not sure whether it is a good thing to mix devm_ and plain request/free > here. Yes, that was my motivation for doing it the way I did it. > And more complexity is always bad so I guess we can stay with this > implementation now. > Feel free to add my > > Acked-by: Mika Westerberg <mika.westerberg@linux.intel.com> Cool, thanks! I wonder if Linus or Grant have any objections, though. Guys? The patch has more to do with ACPI than with GPIO and I think it would be good to follow the spec from the outset here, so I'd like to push it for v3.10. Thanks, Rafael
On Tue, Apr 9, 2013 at 3:57 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Commit 0d1c28a (gpiolib-acpi: Add ACPI5 event model support to gpio.) > that added support for ACPI events signalled through GPIO interrupts > covered only GPIO pins whose numbers are less than or equal to 255. > However, there may be GPIO pins with numbers greater than 255 and > the ACPI spec (ACPI 5.0, Section 5.6.5.1) requires the _EVT method > to be used for handling events corresponding to those pins. > > Moreover, according to the spec, _EVT is the default mechanism > for handling all ACPI events signalled through GPIO interrupts, > so if the _Exx/_Lxx method is not present for the given pin, > _EVT should be used instead. If present, though, _Exx/_Lxx take > precedence over _EVT which shouldn't be executed in that case > (ACPI 5.0, Section 5.6.5.3). > > Modify acpi_gpiochip_request_interrupts() to follow the spec as > described above and add acpi_gpiochip_free_interrupts() needed > to free interrupts associated with _EVT. > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> Patch applied with Mika's ACK, thanks! It's not like I fully understand it, but I totally trust you guys. Yours, Linus Walleij -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, April 10, 2013 11:06:48 PM Linus Walleij wrote: > On Tue, Apr 9, 2013 at 3:57 PM, Rafael J. Wysocki <rjw@sisk.pl> wrote: > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > > > Commit 0d1c28a (gpiolib-acpi: Add ACPI5 event model support to gpio.) > > that added support for ACPI events signalled through GPIO interrupts > > covered only GPIO pins whose numbers are less than or equal to 255. > > However, there may be GPIO pins with numbers greater than 255 and > > the ACPI spec (ACPI 5.0, Section 5.6.5.1) requires the _EVT method > > to be used for handling events corresponding to those pins. > > > > Moreover, according to the spec, _EVT is the default mechanism > > for handling all ACPI events signalled through GPIO interrupts, > > so if the _Exx/_Lxx method is not present for the given pin, > > _EVT should be used instead. If present, though, _Exx/_Lxx take > > precedence over _EVT which shouldn't be executed in that case > > (ACPI 5.0, Section 5.6.5.3). > > > > Modify acpi_gpiochip_request_interrupts() to follow the spec as > > described above and add acpi_gpiochip_free_interrupts() needed > > to free interrupts associated with _EVT. > > > > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > > Patch applied with Mika's ACK, thanks! > > It's not like I fully understand it, but I totally trust you guys. Well, thanks! :-)
Index: linux-pm/drivers/gpio/gpiolib-acpi.c =================================================================== --- linux-pm.orig/drivers/gpio/gpiolib-acpi.c +++ linux-pm/drivers/gpio/gpiolib-acpi.c @@ -17,6 +17,13 @@ #include <linux/acpi.h> #include <linux/interrupt.h> +struct acpi_gpio_evt_pin { + struct list_head node; + acpi_handle *evt_handle; + unsigned int pin; + unsigned int irq; +}; + static int acpi_gpiochip_find(struct gpio_chip *gc, void *data) { if (!gc->dev) @@ -54,7 +61,6 @@ int acpi_get_gpio(char *path, int pin) } EXPORT_SYMBOL_GPL(acpi_get_gpio); - static irqreturn_t acpi_gpio_irq_handler(int irq, void *data) { acpi_handle handle = data; @@ -64,6 +70,27 @@ static irqreturn_t acpi_gpio_irq_handler return IRQ_HANDLED; } +static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data) +{ + struct acpi_gpio_evt_pin *evt_pin = data; + struct acpi_object_list args; + union acpi_object arg; + + arg.type = ACPI_TYPE_INTEGER; + arg.integer.value = evt_pin->pin; + args.count = 1; + args.pointer = &arg; + + acpi_evaluate_object(evt_pin->evt_handle, NULL, &args, NULL); + + return IRQ_HANDLED; +} + +static void acpi_gpio_evt_dh(acpi_handle handle, void *data) +{ + /* The address of this function is used as a key. */ +} + /** * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events * @chip: gpio chip @@ -73,15 +100,13 @@ static irqreturn_t acpi_gpio_irq_handler * chip's interrupt handler. acpi_gpiochip_request_interrupts finds out which * gpio pins have acpi event methods and assigns interrupt handlers that calls * the acpi event methods for those pins. - * - * Interrupts are automatically freed on driver detach */ - void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL}; struct acpi_resource *res; - acpi_handle handle, ev_handle; + acpi_handle handle, evt_handle; + struct list_head *evt_pins = NULL; acpi_status status; unsigned int pin; int irq, ret; @@ -98,13 +123,30 @@ void acpi_gpiochip_request_interrupts(st if (ACPI_FAILURE(status)) return; - /* If a gpio interrupt has an acpi event handler method, then - * set up an interrupt handler that calls the acpi event handler - */ + status = acpi_get_handle(handle, "_EVT", &evt_handle); + if (ACPI_SUCCESS(status)) { + evt_pins = kzalloc(sizeof(*evt_pins), GFP_KERNEL); + if (evt_pins) { + INIT_LIST_HEAD(evt_pins); + status = acpi_attach_data(handle, acpi_gpio_evt_dh, + evt_pins); + if (ACPI_FAILURE(status)) { + kfree(evt_pins); + evt_pins = NULL; + } + } + } + /* + * If a GPIO interrupt has an ACPI event handler method, or _EVT is + * present, set up an interrupt handler that calls the ACPI event + * handler. + */ for (res = buf.pointer; res && (res->type != ACPI_RESOURCE_TYPE_END_TAG); res = ACPI_NEXT_RESOURCE(res)) { + irq_handler_t handler = NULL; + void *data; if (res->type != ACPI_RESOURCE_TYPE_GPIO || res->data.gpio.connection_type != @@ -115,23 +157,42 @@ void acpi_gpiochip_request_interrupts(st if (pin > chip->ngpio) continue; - sprintf(ev_name, "_%c%02X", - res->data.gpio.triggering ? 'E' : 'L', pin); - - status = acpi_get_handle(handle, ev_name, &ev_handle); - if (ACPI_FAILURE(status)) - continue; - irq = chip->to_irq(chip, pin); if (irq < 0) continue; + if (pin <= 255) { + acpi_handle ev_handle; + + sprintf(ev_name, "_%c%02X", + res->data.gpio.triggering ? 'E' : 'L', pin); + status = acpi_get_handle(handle, ev_name, &ev_handle); + if (ACPI_SUCCESS(status)) { + handler = acpi_gpio_irq_handler; + data = ev_handle; + } + } + if (!handler && evt_pins) { + struct acpi_gpio_evt_pin *evt_pin; + + evt_pin = kzalloc(sizeof(*evt_pin), GFP_KERNEL); + if (!evt_pin) + continue; + + list_add_tail(&evt_pin->node, evt_pins); + evt_pin->evt_handle = evt_handle; + evt_pin->pin = pin; + evt_pin->irq = irq; + handler = acpi_gpio_irq_handler_evt; + data = evt_pin; + } + if (!handler) + continue; + /* Assume BIOS sets the triggering, so no flags */ - ret = devm_request_threaded_irq(chip->dev, irq, NULL, - acpi_gpio_irq_handler, - 0, - "GPIO-signaled-ACPI-event", - ev_handle); + ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler, + 0, "GPIO-signaled-ACPI-event", + data); if (ret) dev_err(chip->dev, "Failed to request IRQ %d ACPI event handler\n", @@ -139,3 +200,42 @@ void acpi_gpiochip_request_interrupts(st } } EXPORT_SYMBOL(acpi_gpiochip_request_interrupts); + + +/** + * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts. + * @chip: gpio chip + * + * Free interrupts associated with the _EVT method for the given GPIO chip. + * + * The remaining ACPI event interrupts associated with the chip are freed + * automatically. + */ +void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) +{ + acpi_handle handle; + acpi_status status; + struct list_head *evt_pins; + struct acpi_gpio_evt_pin *evt_pin, *ep; + + if (!chip->dev || !chip->to_irq) + return; + + handle = ACPI_HANDLE(chip->dev); + if (!handle) + return; + + status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins); + if (ACPI_FAILURE(status)) + return; + + list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) { + devm_free_irq(chip->dev, evt_pin->irq, evt_pin); + list_del(&evt_pin->node); + kfree(evt_pin); + } + + acpi_detach_data(handle, acpi_gpio_evt_dh); + kfree(evt_pins); +} +EXPORT_SYMBOL(acpi_gpiochip_free_interrupts); Index: linux-pm/include/linux/acpi_gpio.h =================================================================== --- linux-pm.orig/include/linux/acpi_gpio.h +++ linux-pm/include/linux/acpi_gpio.h @@ -8,6 +8,7 @@ int acpi_get_gpio(char *path, int pin); void acpi_gpiochip_request_interrupts(struct gpio_chip *chip); +void acpi_gpiochip_free_interrupts(struct gpio_chip *chip); #else /* CONFIG_GPIO_ACPI */ @@ -17,6 +18,7 @@ static inline int acpi_get_gpio(char *pa } static inline void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { } +static inline void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { } #endif /* CONFIG_GPIO_ACPI */