lkml.org 
[lkml]   [2014]   [Feb]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH 5/6] gpio / ACPI: Rework ACPI GPIO event handling
    Date
    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.westerberg@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?

    >
    > 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.wysocki@intel.com>

    --
    I speak only for myself.
    Rafael J. Wysocki, Intel Open Source Technology Center.


    \
     
     \ /
      Last update: 2014-02-25 18:01    [W:5.316 / U:0.520 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site