[PATCH v2 0/5] gpio / ACPI: Rework ACPI GPIO events and add support for operation regions

[PATCH v2 0/5] gpio / ACPI: Rework ACPI GPIO events and add support for operation regions

[Mika Westerberg]

This is second revision of the patch series. The first version is available
for example here [1].

This series tries to add support for two new ACPI 5.0 features that were
either missing on incomplete in the current Linux kernel:

 * ACPI GPIO signaled events
 * ACPI GPIO operation regions

The current ACPI GPIO support code already added preliminary support for
GPIO signaled events but at the time we didn't have real hardware with real
GPIO triggered events so it was never properly tested. Now there are
devices like Asus T100TA transformer that uses these events so we were able
to see that the ASL code is being executed when a GPIO interrupt is
triggered.

Changes to the previous version:

 * Moved module refcount manipulation to gpiod_request().
 * Call achip acpi_gpio instead.
 * Fold patches [2,4/6] into one [2/5].
 * Update changelog for [3/5] to state that no functional changes, it is
   rename only.
 * Explain ACPI operations regions a bit better in [5/5].

These patches apply on top of two patches from Alexandre Courbot [2] which
introduce gpiochip_get_desc() function.

[1] http://linux-kernel.2935.n7.nabble.com/PATCH-0-6-gpio-ACPI-Rework-ACPI-GPIO-events-and-add-support-for-operation-regions-td811694.html
[2] https://lkml.org/lkml/2014/2/9/24

Mika Westerberg (5):
  gpiolib: Allow GPIO chips to request their own GPIOs
  gpio / ACPI: Allocate ACPI specific data directly in acpi_gpiochip_add()
  gpio / ACPI: Rename acpi_gpio_evt_pin to acpi_gpio_event
  gpio / ACPI: Rework ACPI GPIO event handling
  gpio / ACPI: Add support for ACPI GPIO operation regions

 drivers/gpio/gpiolib-acpi.c | 465 ++++++++++++++++++++++++++++++++------------
 drivers/gpio/gpiolib.c      | 103 +++++++---
 drivers/gpio/gpiolib.h      |   3 +
 3 files changed, 421 insertions(+), 150 deletions(-)

-- 
1.9.0

[PATCH v2 1/5] gpiolib: Allow GPIO chips to request their own GPIOs

[Mika Westerberg]

Sometimes it is useful to allow GPIO chips themselves to request GPIOs they
own through gpiolib API. One use case is ACPI ASL code that should be able
to toggle GPIOs through GPIO operation regions.

We can't use gpio_request() because it will pin the module to the kernel
forever (it calls try_module_get()). To solve this we move module refcount
manipulation to gpiod_request() and let __gpiod_request() handle the actual
request. This changes the sequence a bit as now try_module_get() is called
outside of gpio_lock (I think this is safe, try_module_get() handles
serialization it needs already).

Then we provide gpiolib internal functions gpiochip_request/free_own_desc()
that do the same as gpio_request() but don't manipulate module refrence
count. This allows the GPIO chip driver to request and free descriptors it
owns without being pinned to the kernel forever.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib.c | 100 ++++++++++++++++++++++++++++++++++++-------------
 drivers/gpio/gpiolib.h |   3 ++
 2 files changed, 76 insertions(+), 27 deletions(-)

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f60d74bc2fce..61de1efe85fd 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1458,26 +1458,14 @@ EXPORT_SYMBOL_GPL(gpiochip_remove_pin_ranges);
  * on each other, and help provide better diagnostics in debugfs.
  * They're called even less than the "set direction" calls.
  */
-static int gpiod_request(struct gpio_desc *desc, const char *label)
+static int __gpiod_request(struct gpio_desc *desc, const char *label)
 {
-	struct gpio_chip	*chip;
-	int			status = -EPROBE_DEFER;
+	struct gpio_chip	*chip = desc->chip;
+	int			status;
 	unsigned long		flags;
 
-	if (!desc) {
-		pr_warn("%s: invalid GPIO\n", __func__);
-		return -EINVAL;
-	}
-
 	spin_lock_irqsave(&gpio_lock, flags);
 
-	chip = desc->chip;
-	if (chip == NULL)
-		goto done;
-
-	if (!try_module_get(chip->owner))
-		goto done;
-
 	/* NOTE:  gpio_request() can be called in early boot,
 	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
@@ -1487,7 +1475,6 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 		status = 0;
 	} else {
 		status = -EBUSY;
-		module_put(chip->owner);
 		goto done;
 	}
 
@@ -1499,7 +1486,6 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 
 		if (status < 0) {
 			desc_set_label(desc, NULL);
-			module_put(chip->owner);
 			clear_bit(FLAG_REQUESTED, &desc->flags);
 			goto done;
 		}
@@ -1511,9 +1497,34 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 		spin_lock_irqsave(&gpio_lock, flags);
 	}
 done:
+	spin_unlock_irqrestore(&gpio_lock, flags);
+	return status;
+}
+
+static int gpiod_request(struct gpio_desc *desc, const char *label)
+{
+	int status = -EPROBE_DEFER;
+	struct gpio_chip *chip;
+
+	if (!desc) {
+		pr_warn("%s: invalid GPIO\n", __func__);
+		return -EINVAL;
+	}
+
+	chip = desc->chip;
+	if (!chip)
+		goto done;
+
+	if (try_module_get(chip->owner)) {
+		status = __gpiod_request(desc, label);
+		if (status < 0)
+			module_put(chip->owner);
+	}
+
+done:
 	if (status)
 		gpiod_dbg(desc, "%s: status %d\n", __func__, status);
-	spin_unlock_irqrestore(&gpio_lock, flags);
+
 	return status;
 }
 
@@ -1523,18 +1534,14 @@ int gpio_request(unsigned gpio, const char *label)
 }
 EXPORT_SYMBOL_GPL(gpio_request);
 
-static void gpiod_free(struct gpio_desc *desc)
+static bool __gpiod_free(struct gpio_desc *desc)
 {
+	bool			ret = false;
 	unsigned long		flags;
 	struct gpio_chip	*chip;
 
 	might_sleep();
 
-	if (!desc) {
-		WARN_ON(extra_checks);
-		return;
-	}
-
 	gpiod_unexport(desc);
 
 	spin_lock_irqsave(&gpio_lock, flags);
@@ -1548,15 +1555,23 @@ static void gpiod_free(struct gpio_desc *desc)
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
 		desc_set_label(desc, NULL);
-		module_put(desc->chip->owner);
 		clear_bit(FLAG_ACTIVE_LOW, &desc->flags);
 		clear_bit(FLAG_REQUESTED, &desc->flags);
 		clear_bit(FLAG_OPEN_DRAIN, &desc->flags);
 		clear_bit(FLAG_OPEN_SOURCE, &desc->flags);
-	} else
-		WARN_ON(extra_checks);
+		ret = true;
+	}
 
 	spin_unlock_irqrestore(&gpio_lock, flags);
+	return ret;
+}
+
+static void gpiod_free(struct gpio_desc *desc)
+{
+	if (desc && __gpiod_free(desc))
+		module_put(desc->chip->owner);
+	else
+		WARN_ON(extra_checks);
 }
 
 void gpio_free(unsigned gpio)
@@ -1678,6 +1693,37 @@ const char *gpiochip_is_requested(struct gpio_chip *chip, unsigned offset)
 }
 EXPORT_SYMBOL_GPL(gpiochip_is_requested);
 
+/**
+ * gpiochip_request_own_desc - Allow GPIO chip to request its own descriptor
+ * @desc: GPIO descriptor to request
+ * @label: label for the GPIO
+ *
+ * Function allows GPIO chip drivers to request and use their own GPIO
+ * descriptors via gpiolib API. Difference to gpiod_request() is that this
+ * function will not increase reference count of the GPIO chip module. This
+ * allows the GPIO chip module to be unloaded as needed (we assume that the
+ * GPIO chip driver handles freeing the GPIOs it has requested).
+ */
+int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label)
+{
+	if (!desc || !desc->chip)
+		return -EINVAL;
+
+	return __gpiod_request(desc, label);
+}
+
+/**
+ * gpiochip_free_own_desc - Free GPIO requested by the chip driver
+ * @desc: GPIO descriptor to free
+ *
+ * Function frees the given GPIO requested previously with
+ * gpiochip_request_own_desc().
+ */
+void gpiochip_free_own_desc(struct gpio_desc *desc)
+{
+	if (desc)
+		__gpiod_free(desc);
+}
 
 /* Drivers MUST set GPIO direction before making get/set calls.  In
  * some cases this is done in early boot, before IRQs are enabled.
diff --git a/drivers/gpio/gpiolib.h b/drivers/gpio/gpiolib.h
index 82be586c1f90..cf092941a9fd 100644
--- a/drivers/gpio/gpiolib.h
+++ b/drivers/gpio/gpiolib.h
@@ -43,4 +43,7 @@ acpi_get_gpiod_by_index(struct device *dev, int index,
 }
 #endif
 
+int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label);
+void gpiochip_free_own_desc(struct gpio_desc *desc);
+
 #endif /* GPIOLIB_H */
-- 
1.9.0

Re: [PATCH v2 1/5] gpiolib: Allow GPIO chips to request their own GPIOs

[Alexandre Courbot]

On Mon, Mar 10, 2014 at 9:54 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> Sometimes it is useful to allow GPIO chips themselves to request GPIOs they
> own through gpiolib API. One use case is ACPI ASL code that should be able
> to toggle GPIOs through GPIO operation regions.
>
> We can't use gpio_request() because it will pin the module to the kernel
> forever (it calls try_module_get()). To solve this we move module refcount
> manipulation to gpiod_request() and let __gpiod_request() handle the actual
> request. This changes the sequence a bit as now try_module_get() is called
> outside of gpio_lock (I think this is safe, try_module_get() handles
> serialization it needs already).
>
> Then we provide gpiolib internal functions gpiochip_request/free_own_desc()
> that do the same as gpio_request() but don't manipulate module refrence
> count. This allows the GPIO chip driver to request and free descriptors it
> owns without being pinned to the kernel forever.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

The change is clear and does not add too much complexity to the code,
so no reason to oppose it.

[PATCH v2 2/5] gpio / ACPI: Allocate ACPI specific data directly in acpi_gpiochip_add()

[Mika Westerberg]

We are going to add more ACPI specific data to accompany GPIO chip so
instead of allocating it per each use-case we allocate it once when
acpi_gpiochip_add() is called and release it when acpi_gpiochip_remove() is
called.

Doing this allows us to add more ACPI specific data by merely adding new
fields to struct acpi_gpio_chip.

In addition we embed evt_pins member directly to the structure instead of
having it as a pointer. This simplifies the code a bit since we don't need
to check against NULL.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 106 ++++++++++++++++++++++++++------------------
 1 file changed, 64 insertions(+), 42 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index b7db098ba060..5c0cf1d76c8b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -26,6 +26,11 @@ struct acpi_gpio_evt_pin {
 	unsigned int irq;
 };
 
+struct acpi_gpio_chip {
+	struct gpio_chip *chip;
+	struct list_head evt_pins;
+};
+
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 {
 	if (!gc->dev)
@@ -81,14 +86,14 @@ static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
+static void acpi_gpio_chip_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
+ * @acpi_gpio:      ACPI GPIO chip
  *
  * ACPI5 platforms can use GPIO signaled ACPI events. These GPIO interrupts are
  * handled by ACPI event methods which need to be called from the GPIO
@@ -96,12 +101,12 @@ static void acpi_gpio_evt_dh(acpi_handle handle, void *data)
  * gpio pins have acpi event methods and assigns interrupt handlers that calls
  * the acpi event methods for those pins.
  */
-static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
+static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *acpi_gpio)
 {
 	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct gpio_chip *chip = acpi_gpio->chip;
 	struct acpi_resource *res;
 	acpi_handle handle, evt_handle;
-	struct list_head *evt_pins = NULL;
 	acpi_status status;
 	unsigned int pin;
 	int irq, ret;
@@ -114,23 +119,7 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 	if (!handle)
 		return;
 
-	status = acpi_get_event_resources(handle, &buf);
-	if (ACPI_FAILURE(status))
-		return;
-
-	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;
-			}
-		}
-	}
+	INIT_LIST_HEAD(&acpi_gpio->evt_pins);
 
 	/*
 	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
@@ -167,14 +156,18 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 				data = ev_handle;
 			}
 		}
-		if (!handler && evt_pins) {
+		if (!handler) {
 			struct acpi_gpio_evt_pin *evt_pin;
 
+			status = acpi_get_handle(handle, "_EVT", &evt_handle);
+			if (ACPI_FAILURE(status))
+				continue
+
 			evt_pin = kzalloc(sizeof(*evt_pin), GFP_KERNEL);
 			if (!evt_pin)
 				continue;
 
-			list_add_tail(&evt_pin->node, evt_pins);
+			list_add_tail(&evt_pin->node, &acpi_gpio->evt_pins);
 			evt_pin->evt_handle = evt_handle;
 			evt_pin->pin = pin;
 			evt_pin->irq = irq;
@@ -197,39 +190,27 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 
 /**
  * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
- * @chip:      gpio chip
+ * @acpi_gpio:      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.
  */
-static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
+static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *acpi_gpio)
 {
-	acpi_handle handle;
-	acpi_status status;
-	struct list_head *evt_pins;
 	struct acpi_gpio_evt_pin *evt_pin, *ep;
+	struct gpio_chip *chip = acpi_gpio->chip;
 
 	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) {
+	list_for_each_entry_safe_reverse(evt_pin, ep, &acpi_gpio->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);
 }
 
 struct acpi_gpio_lookup {
@@ -312,10 +293,51 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
 
 void acpi_gpiochip_add(struct gpio_chip *chip)
 {
-	acpi_gpiochip_request_interrupts(chip);
+	struct acpi_gpio_chip *acpi_gpio;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(chip->dev);
+	if (!handle)
+		return;
+
+	acpi_gpio = kzalloc(sizeof(*acpi_gpio), GFP_KERNEL);
+	if (!acpi_gpio) {
+		dev_err(chip->dev,
+			"Failed to allocate memory for ACPI GPIO chip\n");
+		return;
+	}
+
+	acpi_gpio->chip = chip;
+
+	status = acpi_attach_data(handle, acpi_gpio_chip_dh, acpi_gpio);
+	if (ACPI_FAILURE(status)) {
+		dev_err(chip->dev, "Failed to attach ACPI GPIO chip\n");
+		kfree(acpi_gpio);
+		return;
+	}
+
+	acpi_gpiochip_request_interrupts(acpi_gpio);
 }
 
 void acpi_gpiochip_remove(struct gpio_chip *chip)
 {
-	acpi_gpiochip_free_interrupts(chip);
+	struct acpi_gpio_chip *acpi_gpio;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(chip->dev);
+	if (!handle)
+		return;
+
+	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&acpi_gpio);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(chip->dev, "Failed to retrieve ACPI GPIO chip\n");
+		return;
+	}
+
+	acpi_gpiochip_free_interrupts(acpi_gpio);
+
+	acpi_detach_data(handle, acpi_gpio_chip_dh);
+	kfree(acpi_gpio);
 }
-- 
1.9.0

[PATCH v2 3/5] gpio / ACPI: Rename acpi_gpio_evt_pin to acpi_gpio_event

[Mika Westerberg]

In order to consolidate _Exx, _Lxx and _EVT to use the same structure make
the structure name to reflect that we are dealing with any event, not just
_EVT.

This is just rename, no functional changes.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 39 +++++++++++++++++++--------------------
 1 file changed, 19 insertions(+), 20 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 5c0cf1d76c8b..be09e7526890 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -19,16 +19,16 @@
 
 #include "gpiolib.h"
 
-struct acpi_gpio_evt_pin {
+struct acpi_gpio_event {
 	struct list_head node;
-	acpi_handle *evt_handle;
+	acpi_handle handle;
 	unsigned int pin;
 	unsigned int irq;
 };
 
 struct acpi_gpio_chip {
 	struct gpio_chip *chip;
-	struct list_head evt_pins;
+	struct list_head events;
 };
 
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
@@ -79,9 +79,9 @@ static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
 
 static irqreturn_t acpi_gpio_irq_handler_evt(int irq, void *data)
 {
-	struct acpi_gpio_evt_pin *evt_pin = data;
+	struct acpi_gpio_event *event = data;
 
-	acpi_execute_simple_method(evt_pin->evt_handle, NULL, evt_pin->pin);
+	acpi_execute_simple_method(event->handle, NULL, event->pin);
 
 	return IRQ_HANDLED;
 }
@@ -119,7 +119,7 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *acpi_gpio)
 	if (!handle)
 		return;
 
-	INIT_LIST_HEAD(&acpi_gpio->evt_pins);
+	INIT_LIST_HEAD(&acpi_gpio->events);
 
 	/*
 	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
@@ -157,22 +157,22 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *acpi_gpio)
 			}
 		}
 		if (!handler) {
-			struct acpi_gpio_evt_pin *evt_pin;
+			struct acpi_gpio_event *event;
 
 			status = acpi_get_handle(handle, "_EVT", &evt_handle);
 			if (ACPI_FAILURE(status))
 				continue
 
-			evt_pin = kzalloc(sizeof(*evt_pin), GFP_KERNEL);
-			if (!evt_pin)
+			event = kzalloc(sizeof(*event), GFP_KERNEL);
+			if (!event)
 				continue;
 
-			list_add_tail(&evt_pin->node, &acpi_gpio->evt_pins);
-			evt_pin->evt_handle = evt_handle;
-			evt_pin->pin = pin;
-			evt_pin->irq = irq;
+			list_add_tail(&event->node, &acpi_gpio->events);
+			event->handle = evt_handle;
+			event->pin = pin;
+			event->irq = irq;
 			handler = acpi_gpio_irq_handler_evt;
-			data = evt_pin;
+			data = event;
 		}
 		if (!handler)
 			continue;
@@ -199,17 +199,16 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *acpi_gpio)
  */
 static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *acpi_gpio)
 {
-	struct acpi_gpio_evt_pin *evt_pin, *ep;
+	struct acpi_gpio_event *event, *ep;
 	struct gpio_chip *chip = acpi_gpio->chip;
 
 	if (!chip->dev || !chip->to_irq)
 		return;
 
-	list_for_each_entry_safe_reverse(evt_pin, ep, &acpi_gpio->evt_pins,
-					 node) {
-		devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
-		list_del(&evt_pin->node);
-		kfree(evt_pin);
+	list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->events, node) {
+		devm_free_irq(chip->dev, event->irq, event);
+		list_del(&event->node);
+		kfree(event);
 	}
 }
 
-- 
1.9.0

[PATCH v2 4/5] gpio / ACPI: Rework ACPI GPIO event handling

[Mika Westerberg]

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>
Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 214 ++++++++++++++++++++++++++------------------
 drivers/gpio/gpiolib.c      |   3 +-
 2 files changed, 131 insertions(+), 86 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index be09e7526890..092ea4e5c9a8 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -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->handle, NULL, NULL, NULL);
 
 	return IRQ_HANDLED;
 }
@@ -91,111 +91,148 @@ static void acpi_gpio_chip_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
- * @acpi_gpio:      ACPI GPIO chip
- *
- * ACPI5 platforms can use GPIO signaled ACPI events. These GPIO interrupts are
- * handled by ACPI event methods which need to be called from the GPIO
- * 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.
- */
-static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *acpi_gpio)
+static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
+						   void *context)
 {
-	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct acpi_gpio_chip *acpi_gpio = context;
 	struct gpio_chip *chip = acpi_gpio->chip;
-	struct acpi_resource *res;
+	struct acpi_resource_gpio *agpio;
 	acpi_handle handle, evt_handle;
-	acpi_status status;
-	unsigned int pin;
-	int irq, ret;
-	char ev_name[5];
+	struct acpi_gpio_event *event;
+	irq_handler_t handler = NULL;
+	struct gpio_desc *desc;
+	unsigned long irqflags;
+	int ret, pin, irq;
 
-	if (!chip->dev || !chip->to_irq)
-		return;
+	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);
-	if (!handle)
-		return;
+	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;
 
-	INIT_LIST_HEAD(&acpi_gpio->events);
+	desc = gpiochip_get_desc(chip, pin);
+	if (IS_ERR(desc)) {
+		dev_err(chip->dev, "Failed to get GPIO descriptor\n");
+		return AE_ERROR;
+	}
 
-	/*
-	 * 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;
+	ret = gpiochip_request_own_desc(desc, "ACPI:Event");
+	if (ret) {
+		dev_err(chip->dev, "Failed to request GPIO\n");
+		return AE_ERROR;
+	}
 
-		pin = res->data.gpio.pin_table[0];
-		if (pin > chip->ngpio)
-			continue;
+	gpiod_direction_input(desc);
 
-		irq = chip->to_irq(chip, pin);
-		if (irq < 0)
-			continue;
+	ret = gpiod_lock_as_irq(desc);
+	if (ret) {
+		dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
+		goto fail_free_desc;
+	}
 
-		if (pin <= 255) {
-			acpi_handle ev_handle;
+	irq = gpiod_to_irq(desc);
+	if (irq < 0) {
+		dev_err(chip->dev, "Failed to translate GPIO to IRQ\n");
+		goto fail_unlock_irq;
+	}
 
-			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;
-			}
+	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;
 		}
-		if (!handler) {
-			struct acpi_gpio_event *event;
-
-			status = acpi_get_handle(handle, "_EVT", &evt_handle);
-			if (ACPI_FAILURE(status))
-				continue
+	}
 
-			event = kzalloc(sizeof(*event), GFP_KERNEL);
-			if (!event)
-				continue;
+	event = kzalloc(sizeof(*event), GFP_KERNEL);
+	if (!event)
+		goto fail_unlock_irq;
 
-			list_add_tail(&event->node, &acpi_gpio->events);
-			event->handle = evt_handle;
-			event->pin = pin;
-			event->irq = irq;
-			handler = acpi_gpio_irq_handler_evt;
-			data = event;
-		}
-		if (!handler)
-			continue;
+	event->handle = evt_handle;
+	event->irq = irq;
+	event->pin = pin;
 
-		/* 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);
+	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, &acpi_gpio->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_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
+ * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events
  * @acpi_gpio:      ACPI GPIO chip
  *
- * Free interrupts associated with the _EVT method for the given GPIO chip.
+ * ACPI5 platforms can use GPIO signaled ACPI events. These GPIO interrupts are
+ * handled by ACPI event methods which need to be called from the GPIO
+ * 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.
+ */
+static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *acpi_gpio)
+{
+	struct gpio_chip *chip = acpi_gpio->chip;
+
+	if (!chip->dev || !chip->to_irq)
+		return;
+
+	INIT_LIST_HEAD(&acpi_gpio->events);
+	acpi_walk_resources(ACPI_HANDLE(chip->dev), "_AEI",
+			    acpi_gpiochip_request_interrupt, acpi_gpio);
+}
+
+/**
+ * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
+ * @acpi_gpio:      ACPI 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 *acpi_gpio)
 {
@@ -206,7 +243,14 @@ static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *acpi_gpio)
 		return;
 
 	list_for_each_entry_safe_reverse(event, ep, &acpi_gpio->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 61de1efe85fd..34975fb97bd7 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)) {
-- 
1.9.0

[PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Mika Westerberg]

GPIO operation regions is a new feature introduced in ACPI 5.0
specification. This feature adds a way for platform ASL code to call back
to OS GPIO driver and toggle GPIO pins.

An example ASL code from Lenovo Miix 2 tablet with only relevant part
listed:

 Device (\_SB.GPO0)
 {
     Name (AVBL, Zero)
     Method (_REG, 2, NotSerialized)
     {
         If (LEqual (Arg0, 0x08))
         {
             // Marks the region available
             Store (Arg1, AVBL)
         }
     }

     OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0C)
     Field (GPOP, ByteAcc, NoLock, Preserve)
     {
         Connection (
             GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionOutputOnly,
                     "\\_SB.GPO0", 0x00, ResourceConsumer,,)
             {
                 0x003B
             }
         ),
         SHD3,   1,
     }
 }

 Device (SHUB)
 {
     Method (_PS0, 0, Serialized)
     {
         If (LEqual (\_SB.GPO0.AVBL, One))
         {
             Store (One, \_SB.GPO0.SHD3)
             Sleep (0x32)
         }
     }
     Method (_PS3, 0, Serialized)
     {
         If (LEqual (\_SB.GPO0.AVBL, One))
         {
             Store (Zero, \_SB.GPO0.SHD3)
         }
     }
 }

How this works is that whenever _PS0 or _PS3 method is run (typically when
SHUB device is transitioned to D0 or D3 respectively), ASL code checks if
the GPIO operation region is available (\_SB.GPO0.AVBL). If it is we go and
store either 0 or 1 to \_SB.GPO0.SHD3.

Now, when ACPICA notices ACPI GPIO operation region access (the store
above) it will call acpi_gpio_adr_space_handler() that then toggles the
GPIO accordingly using standard gpiolib interfaces.

Implement the support by registering GPIO operation region handlers for all
GPIO devices that have an ACPI handle. First time the GPIO is used by the
ASL code we make sure that the GPIO stays requested until the GPIO chip
driver itself is unloaded. If we find out that the GPIO is already
requested we just toggle it according to the value got from ASL code.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 156 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 156 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 092ea4e5c9a8..b81272b7c63b 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/interrupt.h>
+#include <linux/mutex.h>
 
 #include "gpiolib.h"
 
@@ -26,7 +27,20 @@ struct acpi_gpio_event {
 	unsigned int irq;
 };
 
+struct acpi_gpio_connection {
+	struct list_head node;
+	struct gpio_desc *desc;
+};
+
 struct acpi_gpio_chip {
+	/*
+	 * ACPICA requires that the first field of the context parameter
+	 * passed to acpi_install_address_space_handler() is large enough
+	 * to hold struct acpi_connection_info.
+	 */
+	struct acpi_connection_info conn_info;
+	struct list_head conns;
+	struct mutex conn_lock;
 	struct gpio_chip *chip;
 	struct list_head events;
 };
@@ -334,6 +348,146 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
 	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
 }
 
+static acpi_status
+acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
+			    u32 bits, u64 *value, void *handler_context,
+			    void *region_context)
+{
+	struct acpi_gpio_chip *achip = region_context;
+	struct gpio_chip *chip = achip->chip;
+	struct acpi_resource_gpio *agpio;
+	struct acpi_resource *ares;
+	acpi_status status;
+	bool pull;
+	int i;
+
+	status = acpi_buffer_to_resource(achip->conn_info.connection,
+					 achip->conn_info.length, &ares);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) {
+		ACPI_FREE(ares);
+		return AE_BAD_PARAMETER;
+	}
+
+	agpio = &ares->data.gpio;
+	pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
+
+	if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
+	    function == ACPI_WRITE)) {
+		ACPI_FREE(ares);
+		return AE_BAD_PARAMETER;
+	}
+
+	for (i = 0; i < agpio->pin_table_length; i++) {
+		unsigned pin = agpio->pin_table[i];
+		struct acpi_gpio_connection *conn;
+		struct gpio_desc *desc;
+		bool found;
+
+		desc = gpiochip_get_desc(chip, pin);
+		if (IS_ERR(desc)) {
+			status = AE_ERROR;
+			goto out;
+		}
+
+		mutex_lock(&achip->conn_lock);
+
+		found = false;
+		list_for_each_entry(conn, &achip->conns, node) {
+			if (conn->desc == desc) {
+				found = true;
+				break;
+			}
+		}
+		if (!found) {
+			int ret;
+
+			ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion");
+			if (ret) {
+				status = AE_ERROR;
+				mutex_unlock(&achip->conn_lock);
+				goto out;
+			}
+
+			switch (agpio->io_restriction) {
+			case ACPI_IO_RESTRICT_INPUT:
+				gpiod_direction_input(desc);
+				break;
+			case ACPI_IO_RESTRICT_OUTPUT:
+				gpiod_direction_output(desc, pull);
+				break;
+			default:
+				/*
+				 * Assume that the BIOS has configured the
+				 * direction and pull accordingly.
+				 */
+				break;
+			}
+
+			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
+			if (!conn) {
+				status = AE_NO_MEMORY;
+				mutex_unlock(&achip->conn_lock);
+				goto out;
+			}
+
+			conn->desc = desc;
+
+			list_add_tail(&conn->node, &achip->conns);
+		}
+
+		mutex_unlock(&achip->conn_lock);
+
+
+		if (function == ACPI_WRITE)
+			gpiod_set_raw_value(desc, !!((1 << i) & *value));
+		else
+			*value |= gpiod_get_raw_value(desc) << i;
+	}
+
+out:
+	ACPI_FREE(ares);
+	return status;
+}
+
+static void acpi_gpiochip_request_regions(struct acpi_gpio_chip *achip)
+{
+	struct gpio_chip *chip = achip->chip;
+	acpi_handle handle = ACPI_HANDLE(chip->dev);
+	acpi_status status;
+
+	INIT_LIST_HEAD(&achip->conns);
+	mutex_init(&achip->conn_lock);
+	status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+						    acpi_gpio_adr_space_handler,
+						    NULL, achip);
+	if (ACPI_FAILURE(status))
+		dev_err(chip->dev, "Failed to install GPIO OpRegion handler\n");
+}
+
+static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip)
+{
+	struct gpio_chip *chip = achip->chip;
+	acpi_handle handle = ACPI_HANDLE(chip->dev);
+	struct acpi_gpio_connection *conn, *tmp;
+	acpi_status status;
+
+	status = acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+						   acpi_gpio_adr_space_handler);
+	if (ACPI_FAILURE(status)) {
+		dev_err(chip->dev, "Failed to remove GPIO OpRegion handler\n");
+		return;
+	}
+
+	list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) {
+		gpiochip_free_own_desc(conn->desc);
+		list_del(&conn->node);
+		kfree(conn);
+	}
+}
+
 void acpi_gpiochip_add(struct gpio_chip *chip)
 {
 	struct acpi_gpio_chip *acpi_gpio;
@@ -361,6 +515,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 	}
 
 	acpi_gpiochip_request_interrupts(acpi_gpio);
+	acpi_gpiochip_request_regions(acpi_gpio);
 }
 
 void acpi_gpiochip_remove(struct gpio_chip *chip)
@@ -379,6 +534,7 @@ void acpi_gpiochip_remove(struct gpio_chip *chip)
 		return;
 	}
 
+	acpi_gpiochip_free_regions(acpi_gpio);
 	acpi_gpiochip_free_interrupts(acpi_gpio);
 
 	acpi_detach_data(handle, acpi_gpio_chip_dh);
-- 
1.9.0

Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Linus Walleij]

On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

Here:

> +static acpi_status
> +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
> +                           u32 bits, u64 *value, void *handler_context,
> +                           void *region_context)
> +{
> +       struct acpi_gpio_chip *achip = region_context;
> +       struct gpio_chip *chip = achip->chip;
> +       struct acpi_resource_gpio *agpio;
> +       struct acpi_resource *ares;
> +       acpi_status status;
> +       bool pull;

Should be named pull_up, right?

> +       int i;
> +
> +       status = acpi_buffer_to_resource(achip->conn_info.connection,
> +                                        achip->conn_info.length, &ares);
> +       if (ACPI_FAILURE(status))
> +               return status;
> +
> +       if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) {
> +               ACPI_FREE(ares);
> +               return AE_BAD_PARAMETER;
> +       }
> +
> +       agpio = &ares->data.gpio;
> +       pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;

So here ACPI tells us that the pin is pulled up.

And I am suspicious since this is strictly speaking pin control business
and not GPIO, and I won't let pin control stuff sneak into the GPIO
subsystem under the radar just because I'm not paying close enough
attention.

I have been told that these things are not dynamic (i.e. we only get
informed that a pin is pulled up/down, we cannot actively change the
config) is this correct?

If any sort of advanced pin control business is going to come into
ACPI a sibling driver in drivers/pinctrl/pinctrl-acpi.c needs to be
created that can select CONFIG_PINCONF properly reflect this
stuff in debugfs etc. (Maybe also give proper names on the pins
since I hear this is coming to ACPI!)

> +       if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
> +           function == ACPI_WRITE)) {
> +               ACPI_FREE(ares);
> +               return AE_BAD_PARAMETER;
> +       }
> +
> +       for (i = 0; i < agpio->pin_table_length; i++) {
> +               unsigned pin = agpio->pin_table[i];
> +               struct acpi_gpio_connection *conn;
> +               struct gpio_desc *desc;
> +               bool found;
> +
> +               desc = gpiochip_get_desc(chip, pin);
> +               if (IS_ERR(desc)) {
> +                       status = AE_ERROR;
> +                       goto out;
> +               }
> +
> +               mutex_lock(&achip->conn_lock);
> +
> +               found = false;
> +               list_for_each_entry(conn, &achip->conns, node) {
> +                       if (conn->desc == desc) {
> +                               found = true;
> +                               break;
> +                       }
> +               }
> +               if (!found) {
> +                       int ret;
> +
> +                       ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion");
> +                       if (ret) {
> +                               status = AE_ERROR;
> +                               mutex_unlock(&achip->conn_lock);
> +                               goto out;
> +                       }
> +
> +                       switch (agpio->io_restriction) {
> +                       case ACPI_IO_RESTRICT_INPUT:
> +                               gpiod_direction_input(desc);
> +                               break;
> +                       case ACPI_IO_RESTRICT_OUTPUT:
> +                               gpiod_direction_output(desc, pull);

Can you explain why the fact that the line is pulled down affects the
default output value like this? I don't get it.

A comment in the code would be needed I think.

This looks more like a typical piece of code for open collector/drain
(which is already handled by gpiolib) not pull up/down.

> +                               break;
> +                       default:
> +                               /*
> +                                * Assume that the BIOS has configured the
> +                                * direction and pull accordingly.
> +                                */
> +                               break;
> +                       }
> +
> +                       conn = kzalloc(sizeof(*conn), GFP_KERNEL);
> +                       if (!conn) {
> +                               status = AE_NO_MEMORY;
> +                               mutex_unlock(&achip->conn_lock);
> +                               goto out;
> +                       }
> +
> +                       conn->desc = desc;
> +
> +                       list_add_tail(&conn->node, &achip->conns);
> +               }
> +
> +               mutex_unlock(&achip->conn_lock);
> +
> +
> +               if (function == ACPI_WRITE)
> +                       gpiod_set_raw_value(desc, !!((1 << i) & *value));

What is this? How can the expression !!((1 << i) possibly evaluate to
anything else than "true"? I don't get it. Just (desc, *value) seem more
apropriate.

Yours,
Linus Walleij

Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Cox, Alan]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 967 bytes --]

> > +                       gpiod_set_raw_value(desc, !!((1 << i) & *value));
> 
> What is this? How can the expression !!((1 << i) possibly evaluate to
> anything else than "true"? I don't get it. Just (desc, *value) seem more
> apropriate.


The expression is !!((1 << i) & *value)

so its the standard C sematic for 'logical and' ? 1 : 0


---------------------------------------------------------------------
Intel Corporation (UK) Limited
Registered No. 1134945 (England)
Registered Office: Pipers Way, Swindon SN3 1RJ
VAT No: 860 2173 47

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Linus Walleij]

On Thu, Mar 13, 2014 at 4:05 PM, Cox, Alan <alan.cox@intel.com> wrote:
>> > +                       gpiod_set_raw_value(desc, !!((1 << i) & *value));
>>
>> What is this? How can the expression !!((1 << i) possibly evaluate to
>> anything else than "true"? I don't get it. Just (desc, *value) seem more
>> apropriate.
>
>
> The expression is !!((1 << i) & *value)
>
> so its the standard C sematic for 'logical and' ? 1 : 0

Hm I missed the first paranthesis when parsing in my head,
all wrong, too bad. Thanks Alan.

Yours,
Linus Walleij

Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Mika Westerberg]

On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote:
> On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> Here:
> 
> > +static acpi_status
> > +acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
> > +                           u32 bits, u64 *value, void *handler_context,
> > +                           void *region_context)
> > +{
> > +       struct acpi_gpio_chip *achip = region_context;
> > +       struct gpio_chip *chip = achip->chip;
> > +       struct acpi_resource_gpio *agpio;
> > +       struct acpi_resource *ares;
> > +       acpi_status status;
> > +       bool pull;
> 
> Should be named pull_up, right?

Yes.

> 
> > +       int i;
> > +
> > +       status = acpi_buffer_to_resource(achip->conn_info.connection,
> > +                                        achip->conn_info.length, &ares);
> > +       if (ACPI_FAILURE(status))
> > +               return status;
> > +
> > +       if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) {
> > +               ACPI_FREE(ares);
> > +               return AE_BAD_PARAMETER;
> > +       }
> > +
> > +       agpio = &ares->data.gpio;
> > +       pull = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
> 
> So here ACPI tells us that the pin is pulled up.
> 
> And I am suspicious since this is strictly speaking pin control business
> and not GPIO, and I won't let pin control stuff sneak into the GPIO
> subsystem under the radar just because I'm not paying close enough
> attention.

This has more to do in finding out what will be the initial value of the
GPIO when we set it to output (given that it is output).

> I have been told that these things are not dynamic (i.e. we only get
> informed that a pin is pulled up/down, we cannot actively change the
> config) is this correct?

As far as I can tell, yes.

> If any sort of advanced pin control business is going to come into
> ACPI a sibling driver in drivers/pinctrl/pinctrl-acpi.c needs to be
> created that can select CONFIG_PINCONF properly reflect this
> stuff in debugfs etc. (Maybe also give proper names on the pins
> since I hear this is coming to ACPI!)

No advanced pin control business, just GPIO stuff :)

> > +       if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
> > +           function == ACPI_WRITE)) {
> > +               ACPI_FREE(ares);
> > +               return AE_BAD_PARAMETER;
> > +       }
> > +
> > +       for (i = 0; i < agpio->pin_table_length; i++) {
> > +               unsigned pin = agpio->pin_table[i];
> > +               struct acpi_gpio_connection *conn;
> > +               struct gpio_desc *desc;
> > +               bool found;
> > +
> > +               desc = gpiochip_get_desc(chip, pin);
> > +               if (IS_ERR(desc)) {
> > +                       status = AE_ERROR;
> > +                       goto out;
> > +               }
> > +
> > +               mutex_lock(&achip->conn_lock);
> > +
> > +               found = false;
> > +               list_for_each_entry(conn, &achip->conns, node) {
> > +                       if (conn->desc == desc) {
> > +                               found = true;
> > +                               break;
> > +                       }
> > +               }
> > +               if (!found) {
> > +                       int ret;
> > +
> > +                       ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion");
> > +                       if (ret) {
> > +                               status = AE_ERROR;
> > +                               mutex_unlock(&achip->conn_lock);
> > +                               goto out;
> > +                       }
> > +
> > +                       switch (agpio->io_restriction) {
> > +                       case ACPI_IO_RESTRICT_INPUT:
> > +                               gpiod_direction_input(desc);
> > +                               break;
> > +                       case ACPI_IO_RESTRICT_OUTPUT:
> > +                               gpiod_direction_output(desc, pull);
> 
> Can you explain why the fact that the line is pulled down affects the
> default output value like this? I don't get it.

That's the thing - ACPI doesn't tell us what is the initial value of the
pin. There is no such field in GpioIo() resource.

So I'm assuming here that if it says that the pin is pulled up, the default
value will be high.

> A comment in the code would be needed I think.
> 
> This looks more like a typical piece of code for open collector/drain
> (which is already handled by gpiolib) not pull up/down.
>
> 
> > +                               break;
> > +                       default:
> > +                               /*
> > +                                * Assume that the BIOS has configured the
> > +                                * direction and pull accordingly.
> > +                                */
> > +                               break;
> > +                       }
> > +
> > +                       conn = kzalloc(sizeof(*conn), GFP_KERNEL);
> > +                       if (!conn) {
> > +                               status = AE_NO_MEMORY;
> > +                               mutex_unlock(&achip->conn_lock);
> > +                               goto out;
> > +                       }
> > +
> > +                       conn->desc = desc;
> > +
> > +                       list_add_tail(&conn->node, &achip->conns);
> > +               }
> > +
> > +               mutex_unlock(&achip->conn_lock);
> > +
> > +
> > +               if (function == ACPI_WRITE)
> > +                       gpiod_set_raw_value(desc, !!((1 << i) & *value));
> 
> What is this? How can the expression !!((1 << i) possibly evaluate to
> anything else than "true"? I don't get it. Just (desc, *value) seem more
> apropriate.

We are dealing with multiple pins here. So for example if
agpio->pin_table_length == 2 and *value == 0x2 we get:

i == 0: !!((1 << 0) & 0x2) --> false
i == 1: !!((1 << 1) & 0x2) --> true

Maybe

	gpiod_set_raw_value(desc, (*value >> i) & 1);

is cleaner?

Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Linus Walleij]

On Thu, Mar 13, 2014 at 4:18 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote:

>> > +                       case ACPI_IO_RESTRICT_OUTPUT:
>> > +                               gpiod_direction_output(desc, pull);
>>
>> Can you explain why the fact that the line is pulled down affects the
>> default output value like this? I don't get it.
>
> That's the thing - ACPI doesn't tell us what is the initial value of the
> pin. There is no such field in GpioIo() resource.
>
> So I'm assuming here that if it says that the pin is pulled up, the default
> value will be high.

OK! So exactly that statement is what I want to see as a comment
in this switch case.

>> > +               if (function == ACPI_WRITE)
>> > +                       gpiod_set_raw_value(desc, !!((1 << i) & *value));
>>
>> What is this? How can the expression !!((1 << i) possibly evaluate to
>> anything else than "true"? I don't get it. Just (desc, *value) seem more
>> apropriate.
>
> We are dealing with multiple pins here. So for example if
> agpio->pin_table_length == 2 and *value == 0x2 we get:
>
> i == 0: !!((1 << 0) & 0x2) --> false
> i == 1: !!((1 << 1) & 0x2) --> true

Yeah, Alan already pointed out my parse error... this is OK.

Yours,
Linus Walleij

Re: [PATCH v2 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Mika Westerberg]

On Fri, Mar 14, 2014 at 11:53:30AM +0100, Linus Walleij wrote:
> On Thu, Mar 13, 2014 at 4:18 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Thu, Mar 13, 2014 at 03:32:01PM +0100, Linus Walleij wrote:
> 
> >> > +                       case ACPI_IO_RESTRICT_OUTPUT:
> >> > +                               gpiod_direction_output(desc, pull);
> >>
> >> Can you explain why the fact that the line is pulled down affects the
> >> default output value like this? I don't get it.
> >
> > That's the thing - ACPI doesn't tell us what is the initial value of the
> > pin. There is no such field in GpioIo() resource.
> >
> > So I'm assuming here that if it says that the pin is pulled up, the default
> > value will be high.
> 
> OK! So exactly that statement is what I want to see as a comment
> in this switch case.

Sure. I'll add that comment to the next version of this patch.

[PATCH v3 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Mika Westerberg]

GPIO operation regions is a new feature introduced in ACPI 5.0
specification. This feature adds a way for platform ASL code to call back
to OS GPIO driver and toggle GPIO pins.

An example ASL code from Lenovo Miix 2 tablet with only relevant part
listed:

 Device (\_SB.GPO0)
 {
     Name (AVBL, Zero)
     Method (_REG, 2, NotSerialized)
     {
         If (LEqual (Arg0, 0x08))
         {
             // Marks the region available
             Store (Arg1, AVBL)
         }
     }

     OperationRegion (GPOP, GeneralPurposeIo, Zero, 0x0C)
     Field (GPOP, ByteAcc, NoLock, Preserve)
     {
         Connection (
             GpioIo (Exclusive, PullDefault, 0, 0, IoRestrictionOutputOnly,
                     "\\_SB.GPO0", 0x00, ResourceConsumer,,)
             {
                 0x003B
             }
         ),
         SHD3,   1,
     }
 }

 Device (SHUB)
 {
     Method (_PS0, 0, Serialized)
     {
         If (LEqual (\_SB.GPO0.AVBL, One))
         {
             Store (One, \_SB.GPO0.SHD3)
             Sleep (0x32)
         }
     }
     Method (_PS3, 0, Serialized)
     {
         If (LEqual (\_SB.GPO0.AVBL, One))
         {
             Store (Zero, \_SB.GPO0.SHD3)
         }
     }
 }

How this works is that whenever _PS0 or _PS3 method is run (typically when
SHUB device is transitioned to D0 or D3 respectively), ASL code checks if
the GPIO operation region is available (\_SB.GPO0.AVBL). If it is we go and
store either 0 or 1 to \_SB.GPO0.SHD3.

Now, when ACPICA notices ACPI GPIO operation region access (the store
above) it will call acpi_gpio_adr_space_handler() that then toggles the
GPIO accordingly using standard gpiolib interfaces.

Implement the support by registering GPIO operation region handlers for all
GPIO devices that have an ACPI handle. First time the GPIO is used by the
ASL code we make sure that the GPIO stays requested until the GPIO chip
driver itself is unloaded. If we find out that the GPIO is already
requested we just toggle it according to the value got from ASL code.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
Changes to v2:
 * pull -> pull_up
 * Add comment explaining why pull_up is used setting initial value
 * Free the GPIO descriptor if connection memory allocation fails.

 drivers/gpio/gpiolib-acpi.c | 163 ++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 163 insertions(+)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 092ea4e5c9a8..bf0f8b476696 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -16,6 +16,7 @@
 #include <linux/export.h>
 #include <linux/acpi.h>
 #include <linux/interrupt.h>
+#include <linux/mutex.h>
 
 #include "gpiolib.h"
 
@@ -26,7 +27,20 @@ struct acpi_gpio_event {
 	unsigned int irq;
 };
 
+struct acpi_gpio_connection {
+	struct list_head node;
+	struct gpio_desc *desc;
+};
+
 struct acpi_gpio_chip {
+	/*
+	 * ACPICA requires that the first field of the context parameter
+	 * passed to acpi_install_address_space_handler() is large enough
+	 * to hold struct acpi_connection_info.
+	 */
+	struct acpi_connection_info conn_info;
+	struct list_head conns;
+	struct mutex conn_lock;
 	struct gpio_chip *chip;
 	struct list_head events;
 };
@@ -334,6 +348,153 @@ struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
 	return lookup.desc ? lookup.desc : ERR_PTR(-ENOENT);
 }
 
+static acpi_status
+acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
+			    u32 bits, u64 *value, void *handler_context,
+			    void *region_context)
+{
+	struct acpi_gpio_chip *achip = region_context;
+	struct gpio_chip *chip = achip->chip;
+	struct acpi_resource_gpio *agpio;
+	struct acpi_resource *ares;
+	acpi_status status;
+	bool pull_up;
+	int i;
+
+	status = acpi_buffer_to_resource(achip->conn_info.connection,
+					 achip->conn_info.length, &ares);
+	if (ACPI_FAILURE(status))
+		return status;
+
+	if (WARN_ON(ares->type != ACPI_RESOURCE_TYPE_GPIO)) {
+		ACPI_FREE(ares);
+		return AE_BAD_PARAMETER;
+	}
+
+	agpio = &ares->data.gpio;
+	pull_up = agpio->pin_config == ACPI_PIN_CONFIG_PULLUP;
+
+	if (WARN_ON(agpio->io_restriction == ACPI_IO_RESTRICT_INPUT &&
+	    function == ACPI_WRITE)) {
+		ACPI_FREE(ares);
+		return AE_BAD_PARAMETER;
+	}
+
+	for (i = 0; i < agpio->pin_table_length; i++) {
+		unsigned pin = agpio->pin_table[i];
+		struct acpi_gpio_connection *conn;
+		struct gpio_desc *desc;
+		bool found;
+
+		desc = gpiochip_get_desc(chip, pin);
+		if (IS_ERR(desc)) {
+			status = AE_ERROR;
+			goto out;
+		}
+
+		mutex_lock(&achip->conn_lock);
+
+		found = false;
+		list_for_each_entry(conn, &achip->conns, node) {
+			if (conn->desc == desc) {
+				found = true;
+				break;
+			}
+		}
+		if (!found) {
+			int ret;
+
+			ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion");
+			if (ret) {
+				status = AE_ERROR;
+				mutex_unlock(&achip->conn_lock);
+				goto out;
+			}
+
+			switch (agpio->io_restriction) {
+			case ACPI_IO_RESTRICT_INPUT:
+				gpiod_direction_input(desc);
+				break;
+			case ACPI_IO_RESTRICT_OUTPUT:
+				/*
+				 * ACPI GPIO resources don't contain an
+				 * initial value for the GPIO. Therefore we
+				 * deduce that value from the pull field
+				 * instead. If the pin is pulled up we
+				 * assume default to be high, otherwise
+				 * low.
+				 */
+				gpiod_direction_output(desc, pull_up);
+				break;
+			default:
+				/*
+				 * Assume that the BIOS has configured the
+				 * direction and pull accordingly.
+				 */
+				break;
+			}
+
+			conn = kzalloc(sizeof(*conn), GFP_KERNEL);
+			if (!conn) {
+				status = AE_NO_MEMORY;
+				gpiochip_free_own_desc(desc);
+				mutex_unlock(&achip->conn_lock);
+				goto out;
+			}
+
+			conn->desc = desc;
+			list_add_tail(&conn->node, &achip->conns);
+		}
+
+		mutex_unlock(&achip->conn_lock);
+
+		if (function == ACPI_WRITE)
+			gpiod_set_raw_value(desc, !!((1 << i) & *value));
+		else
+			*value |= gpiod_get_raw_value(desc) << i;
+	}
+
+out:
+	ACPI_FREE(ares);
+	return status;
+}
+
+static void acpi_gpiochip_request_regions(struct acpi_gpio_chip *achip)
+{
+	struct gpio_chip *chip = achip->chip;
+	acpi_handle handle = ACPI_HANDLE(chip->dev);
+	acpi_status status;
+
+	INIT_LIST_HEAD(&achip->conns);
+	mutex_init(&achip->conn_lock);
+	status = acpi_install_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+						    acpi_gpio_adr_space_handler,
+						    NULL, achip);
+	if (ACPI_FAILURE(status))
+		dev_err(chip->dev, "Failed to install GPIO OpRegion handler\n");
+}
+
+static void acpi_gpiochip_free_regions(struct acpi_gpio_chip *achip)
+{
+	struct gpio_chip *chip = achip->chip;
+	acpi_handle handle = ACPI_HANDLE(chip->dev);
+	struct acpi_gpio_connection *conn, *tmp;
+	acpi_status status;
+
+	status = acpi_remove_address_space_handler(handle, ACPI_ADR_SPACE_GPIO,
+						   acpi_gpio_adr_space_handler);
+	if (ACPI_FAILURE(status)) {
+		dev_err(chip->dev, "Failed to remove GPIO OpRegion handler\n");
+		return;
+	}
+
+	list_for_each_entry_safe_reverse(conn, tmp, &achip->conns, node) {
+		gpiochip_free_own_desc(conn->desc);
+		list_del(&conn->node);
+		kfree(conn);
+	}
+}
+
 void acpi_gpiochip_add(struct gpio_chip *chip)
 {
 	struct acpi_gpio_chip *acpi_gpio;
@@ -361,6 +522,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 	}
 
 	acpi_gpiochip_request_interrupts(acpi_gpio);
+	acpi_gpiochip_request_regions(acpi_gpio);
 }
 
 void acpi_gpiochip_remove(struct gpio_chip *chip)
@@ -379,6 +541,7 @@ void acpi_gpiochip_remove(struct gpio_chip *chip)
 		return;
 	}
 
+	acpi_gpiochip_free_regions(acpi_gpio);
 	acpi_gpiochip_free_interrupts(acpi_gpio);
 
 	acpi_detach_data(handle, acpi_gpio_chip_dh);
-- 
1.9.0

Re: [PATCH v3 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Linus Walleij]

On Fri, Mar 14, 2014 at 4:58 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> GPIO operation regions is a new feature introduced in ACPI 5.0
> specification. This feature adds a way for platform ASL code to call back
> to OS GPIO driver and toggle GPIO pins.

OK this version of the patch applied!

Yours,
Linus Walleij

Re: [PATCH v3 5/5] gpio / ACPI: Add support for ACPI GPIO operation regions

[Mika Westerberg]

On Fri, Mar 14, 2014 at 05:25:39PM +0100, Linus Walleij wrote:
> On Fri, Mar 14, 2014 at 4:58 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > GPIO operation regions is a new feature introduced in ACPI 5.0
> > specification. This feature adds a way for platform ASL code to call back
> > to OS GPIO driver and toggle GPIO pins.
> 
> OK this version of the patch applied!

Thanks!

Re: [PATCH v2 0/5] gpio / ACPI: Rework ACPI GPIO events and add support for operation regions

[Linus Walleij]

On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> This is second revision of the patch series. The first version is available
> for example here [1].

I have applied patche 1 thru 4.

Let's discuss patch 5 a bit before we continue.

Yours,
Linus Walleij

Re: [PATCH v2 0/5] gpio / ACPI: Rework ACPI GPIO events and add support for operation regions

[Mika Westerberg]

On Thu, Mar 13, 2014 at 03:17:29PM +0100, Linus Walleij wrote:
> On Mon, Mar 10, 2014 at 1:54 PM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> 
> > This is second revision of the patch series. The first version is available
> > for example here [1].
> 
> I have applied patche 1 thru 4.

Thanks!