On Tue, Feb 25, 2014 at 03:44:52PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:10 PM Mika Westerberg wrote:
> > The current ACPI GPIO event handling code was never tested against real
> > hardware with functioning GPIO triggered events (at the time such hardware
> > wasn't available). Thus it misses certain things like requesting the GPIOs
> > properly, passing correct flags to the interrupt handler and so on.
> >
> > This patch reworks ACPI GPIO event handling so that we:
> >
> > 1) Use struct acpi_gpio_event for all GPIO signaled events.
> > 2) Switch to use GPIO descriptor API and request GPIOs by calling
> > gpiochip_request_own_desc() that we added in a previous patch.
> > 3) Pass proper flags from ACPI GPIO resource to request_threaded_irq().
> >
> > Also instead of open-coding the _AEI iteration loop we can use
> > acpi_walk_resources(). This simplifies the code a bit and fixes memory leak
> > that was caused by missing kfree() for buffer returned by
> > acpi_get_event_resources().
> >
> > Since the remove path now calls gpiochip_free_own_desc() which takes GPIO
> > spinlock we need to call acpi_gpiochip_remove() outside of that lock
> > (analogous to acpi_gpiochip_add() path where the lock is released before
> > those funtions are called).
> >
> > Signed-off-by: Mika Westerberg <mika.westerb...@linux.intel.com>
> > ---
> > drivers/gpio/gpiolib-acpi.c | 221
> > ++++++++++++++++++++++++++------------------
> > drivers/gpio/gpiolib.c | 3 +-
> > 2 files changed, 132 insertions(+), 92 deletions(-)
> >
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index c60db4ddc166..275735f390af 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -21,7 +21,7 @@
> >
> > struct acpi_gpio_event {
> > struct list_head node;
> > - acpi_handle *evt_handle;
> > + acpi_handle evt_handle;
> > unsigned int pin;
> > unsigned int irq;
> > };
> > @@ -70,9 +70,9 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int
> > pin)
> >
> > static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> > {
> > - acpi_handle handle = data;
> > + struct acpi_gpio_event *event = data;
> >
> > - acpi_evaluate_object(handle, NULL, NULL, NULL);
> > + acpi_evaluate_object(event->evt_handle, NULL, NULL, NULL);
>
> This is a threaded irq handler, isn't it?
Yes.
> >
> > return IRQ_HANDLED;
> > }
> > @@ -91,6 +91,120 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void
> > *data)
> > /* The address of this function is used as a key. */
> > }
> >
> > +static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource
> > *ares,
> > + void *context)
> > +{
> > + struct acpi_gpio_chip *achip = context;
> > + struct gpio_chip *chip = achip->chip;
> > + struct acpi_resource_gpio *agpio;
> > + acpi_handle handle, evt_handle;
> > + struct acpi_gpio_event *event;
> > + irq_handler_t handler = NULL;
> > + struct gpio_desc *desc;
> > + unsigned long irqflags;
> > + int ret, pin, irq;
> > +
> > + if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
> > + return AE_OK;
> > +
> > + agpio = &ares->data.gpio;
> > + if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
> > + return AE_OK;
> > +
> > + handle = ACPI_HANDLE(chip->dev);
> > + pin = agpio->pin_table[0];
> > +
> > + if (pin <= 255) {
> > + char ev_name[5];
> > + sprintf(ev_name, "_%c%02X",
> > + agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
> > + pin);
> > + if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
> > + handler = acpi_gpio_irq_handler;
> > + }
> > + if (!handler) {
> > + if (ACPI_SUCCESS(acpi_get_handle(handle, "_EVT", &evt_handle)))
> > + handler = acpi_gpio_irq_handler_evt;
> > + }
> > + if (!handler)
> > + return AE_BAD_PARAMETER;
> > +
> > + desc = gpiochip_get_desc(chip, pin);
> > + if (IS_ERR(desc)) {
> > + dev_err(chip->dev, "Failed to get GPIO descriptor\n");
> > + return AE_ERROR;
> > + }
> > +
> > + ret = gpiochip_request_own_desc(desc, "ACPI:Event");
> > + if (ret) {
> > + dev_err(chip->dev, "Failed to request GPIO\n");
> > + return AE_ERROR;
> > + }
> > +
> > + gpiod_direction_input(desc);
> > +
> > + ret = gpiod_lock_as_irq(desc);
> > + if (ret) {
> > + dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
> > + goto fail_free_desc;
> > + }
> > +
> > + irq = gpiod_to_irq(desc);
> > + if (irq < 0) {
> > + dev_err(chip->dev, "Failed to translate GPIO to IRQ\n");
> > + goto fail_unlock_irq;
> > + }
> > +
> > + irqflags = IRQF_ONESHOT;
> > + if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
> > + if (agpio->polarity == ACPI_ACTIVE_HIGH)
> > + irqflags |= IRQF_TRIGGER_HIGH;
> > + else
> > + irqflags |= IRQF_TRIGGER_LOW;
> > + } else {
> > + switch (agpio->polarity) {
> > + case ACPI_ACTIVE_HIGH:
> > + irqflags |= IRQF_TRIGGER_RISING;
> > + break;
> > + case ACPI_ACTIVE_LOW:
> > + irqflags |= IRQF_TRIGGER_FALLING;
> > + break;
> > + default:
> > + irqflags |= IRQF_TRIGGER_RISING |
> > + IRQF_TRIGGER_FALLING;
> > + break;
> > + }
> > + }
> > +
> > + event = kzalloc(sizeof(*event), GFP_KERNEL);
> > + if (!event)
> > + goto fail_unlock_irq;
> > +
> > + event->evt_handle = evt_handle;
> > + event->irq = irq;
> > + event->pin = pin;
> > +
> > + ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
> > + "ACPI:Event", event);
> > + if (ret) {
> > + dev_err(chip->dev, "Failed to setup interrupt handler for %d\n",
> > + event->irq);
> > + goto fail_free_event;
> > + }
> > +
> > + list_add_tail(&event->node, &achip->events);
> > + return AE_OK;
> > +
> > +fail_free_event:
> > + kfree(event);
> > +fail_unlock_irq:
> > + gpiod_unlock_as_irq(desc);
> > +fail_free_desc:
> > + gpiochip_free_own_desc(desc);
> > +
> > + return AE_ERROR;
> > +}
> > +
> > /**
> > * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI
> > events
> > * @achip: ACPI GPIO chip
> > @@ -103,104 +217,22 @@ static void acpi_gpio_chip_dh(acpi_handle handle,
> > void *data)
> > */
> > static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
> > {
> > - struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> > struct gpio_chip *chip = achip->chip;
> > - struct acpi_resource *res;
> > - acpi_handle handle;
> > - acpi_status status;
> > - unsigned int pin;
> > - int irq, ret;
> > - char ev_name[5];
> >
> > if (!chip->dev || !chip->to_irq)
> > return;
> >
> > - handle = ACPI_HANDLE(chip->dev);
> > - if (!handle)
> > - return;
> > -
> > INIT_LIST_HEAD(&achip->events);
> > -
> > - status = acpi_get_event_resources(handle, &buf);
> > - if (ACPI_FAILURE(status))
> > - return;
> > -
> > - /*
> > - * 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 !=
> > - ACPI_RESOURCE_GPIO_TYPE_INT)
> > - continue;
> > -
> > - pin = res->data.gpio.pin_table[0];
> > - if (pin > chip->ngpio)
> > - 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) {
> > - struct acpi_gpio_event *event;
> > - acpi_handle evt_handle;
> > -
> > - status = acpi_get_handle(handle, "_EVT", &evt_handle);
> > - if (ACPI_FAILURE(status))
> > - continue;
> > -
> > - event = kzalloc(sizeof(*event), GFP_KERNEL);
> > - if (!event)
> > - continue;
> > -
> > - list_add_tail(&event->node, &achip->events);
> > - event->evt_handle = evt_handle;
> > - event->pin = pin;
> > - event->irq = irq;
> > - handler = acpi_gpio_irq_handler_evt;
> > - data = event;
> > - }
> > - if (!handler)
> > - continue;
> > -
> > - /* Assume BIOS sets the triggering, so no flags */
> > - 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",
> > - irq);
> > - }
> > + acpi_walk_resources(ACPI_HANDLE(chip->dev), "_AEI",
> > + acpi_gpiochip_request_interrupt, achip);
> > }
> >
> > /**
> > - * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> > + * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
> > * @achip: ACPI 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.
> > + * Free interrupts associated with GPIO ACPI event method for the given
> > + * GPIO chip.
> > */
> > static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> > {
> > @@ -211,7 +243,14 @@ static void acpi_gpiochip_free_interrupts(struct
> > acpi_gpio_chip *achip)
> > return;
> >
> > list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
> > - devm_free_irq(chip->dev, event->irq, event);
> > + struct gpio_desc *desc;
> > +
> > + free_irq(event->irq, event);
> > + desc = gpiochip_get_desc(chip, event->pin);
> > + if (WARN_ON(IS_ERR(desc)))
> > + continue;
> > + gpiod_unlock_as_irq(desc);
> > + gpiochip_free_own_desc(desc);
> > list_del(&event->node);
> > kfree(event);
> > }
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 489a63524eb6..474f7d1eb7d7 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1266,11 +1266,12 @@ int gpiochip_remove(struct gpio_chip *chip)
> > int status = 0;
> > unsigned id;
> >
> > + acpi_gpiochip_remove(chip);
> > +
> > spin_lock_irqsave(&gpio_lock, flags);
> >
> > gpiochip_remove_pin_ranges(chip);
> > of_gpiochip_remove(chip);
> > - acpi_gpiochip_remove(chip);
> >
> > for (id = 0; id < chip->ngpio; id++) {
> > if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
> >
>
> Looks good to me.
>
> Reviewed-by: Rafael J. Wysocki <rafael.j.wyso...@intel.com>
Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
