[PATCH 0/5] gpiolib: convert ACPI GPIO helpers to gpiod_ interfaces

[PATCH 0/5] gpiolib: convert ACPI GPIO helpers to gpiod_ interfaces

[Mika Westerberg]

Hi all,

This series converts the ACPI GPIO helpers in gpiolib-acpi.c to use the new
and preferred GPIO descriptor based interface as suggested by the GPIO
subsystem maintainer.

The first patch is just a cosmetic fix to move acpi_gpiochip_free_interrupts()
closer to acpi_gpiochip_request_interrupts(). Patches [2-5/5] do the actual
conversion.

The series is based on the latest patches from Alexandre Courbot and I've
tested them on Intel Haswell based machine.

Please review.

Mika Westerberg (5):
  gpiolib / ACPI: move acpi_gpiochip_free_interrupts next to the request function
  gpiolib / ACPI: convert to gpiod interfaces
  gpiolib / ACPI: add ACPI support for gpiod_get_index()
  gpiolib / ACPI: allow passing GPIOF_ACTIVE_LOW for GpioInt resources
  gpiolib / ACPI: document the GPIO descriptor based interface

 Documentation/acpi/enumeration.txt |  22 +++++++
 drivers/gpio/gpiolib-acpi.c        | 129 ++++++++++++++++++-------------------
 drivers/gpio/gpiolib.c             |  20 ++++++
 include/linux/acpi_gpio.h          |  40 +++++++++---
 4 files changed, 138 insertions(+), 73 deletions(-)

-- 
1.8.4.rc3

[PATCH 1/5] gpiolib / ACPI: move acpi_gpiochip_free_interrupts next to the request function

[Mika Westerberg]

It makes more sense to have these functions close to each other. No
functional changes.

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index f2beb72..1745ce5 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -194,6 +194,44 @@ void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 }
 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);
+
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
@@ -271,41 +309,3 @@ int acpi_get_gpio_by_index(struct device *dev, int index,
 	return lookup.gpio;
 }
 EXPORT_SYMBOL_GPL(acpi_get_gpio_by_index);
-
-/**
- * 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);
-- 
1.8.4.rc3

Re: [PATCH 1/5] gpiolib / ACPI: move acpi_gpiochip_free_interrupts next to the request function

[Linus Walleij]

On Tue, Oct 1, 2013 at 4:58 PM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:

> It makes more sense to have these functions close to each other. No
> functional changes.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Patch applied.

Yours,
Linus Walleij

[PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Mika Westerberg]

The new GPIO descriptor based interface is now preferred over the old
integer based one. This patch converts the ACPI GPIO helpers to use this
new interface internally. In addition to that provide compatibility
functions acpi_get_gpio() and acpi_get_gpio_by_index() that convert the
returned GPIO descriptors to integers.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c | 51 +++++++++++++++++++++------------------------
 include/linux/acpi_gpio.h   | 38 ++++++++++++++++++++++++++-------
 2 files changed, 54 insertions(+), 35 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 1745ce5..333297c 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -11,7 +11,7 @@
  */
 
 #include <linux/errno.h>
-#include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 #include <linux/export.h>
 #include <linux/acpi_gpio.h>
 #include <linux/acpi.h>
@@ -33,14 +33,15 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
 }
 
 /**
- * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API
+ * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
  * @path:	ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
  * @pin:	ACPI GPIO pin number (0-based, controller-relative)
  *
- * Returns GPIO number to use with Linux generic GPIO API, or errno error value
+ * Returns GPIO descriptor to use with Linux generic GPIO API, or ERR_PTR
+ * error value
  */
 
-int acpi_get_gpio(char *path, int pin)
+struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 {
 	struct gpio_chip *chip;
 	acpi_handle handle;
@@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
 
 	status = acpi_get_handle(NULL, path, &handle);
 	if (ACPI_FAILURE(status))
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 
 	chip = gpiochip_find(handle, acpi_gpiochip_find);
 	if (!chip)
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 
-	if (!gpio_is_valid(chip->base + pin))
-		return -EINVAL;
-
-	return chip->base + pin;
+	return gpio_to_desc(chip->base + pin);
 }
-EXPORT_SYMBOL_GPL(acpi_get_gpio);
+EXPORT_SYMBOL_GPL(acpi_get_gpiod);
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
 {
@@ -235,7 +233,7 @@ EXPORT_SYMBOL(acpi_gpiochip_free_interrupts);
 struct acpi_gpio_lookup {
 	struct acpi_gpio_info info;
 	int index;
-	int gpio;
+	struct gpio_desc *desc;
 	int n;
 };
 
@@ -246,11 +244,11 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
 	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
 		return 1;
 
-	if (lookup->n++ == lookup->index && lookup->gpio < 0) {
+	if (lookup->n++ == lookup->index && !lookup->desc) {
 		const struct acpi_resource_gpio *agpio = &ares->data.gpio;
 
-		lookup->gpio = acpi_get_gpio(agpio->resource_source.string_ptr,
-					     agpio->pin_table[0]);
+		lookup->desc = acpi_get_gpiod(agpio->resource_source.string_ptr,
+					      agpio->pin_table[0]);
 		lookup->info.gpioint =
 			agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
 	}
@@ -259,24 +257,24 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
 }
 
 /**
- * acpi_get_gpio_by_index() - get a GPIO number from device resources
+ * acpi_get_gpiod_by_index() - get a GPIO descriptor from device resources
  * @dev: pointer to a device to get GPIO from
  * @index: index of GpioIo/GpioInt resource (starting from %0)
  * @info: info pointer to fill in (optional)
  *
  * Function goes through ACPI resources for @dev and based on @index looks
- * up a GpioIo/GpioInt resource, translates it to the Linux GPIO number,
+ * up a GpioIo/GpioInt resource, translates it to the Linux GPIO descriptor,
  * and returns it. @index matches GpioIo/GpioInt resources only so if there
  * are total %3 GPIO resources, the index goes from %0 to %2.
  *
- * If the GPIO cannot be translated or there is an error, negative errno is
+ * If the GPIO cannot be translated or there is an error an ERR_PTR is
  * returned.
  *
  * Note: if the GPIO resource has multiple entries in the pin list, this
  * function only returns the first.
  */
-int acpi_get_gpio_by_index(struct device *dev, int index,
-			   struct acpi_gpio_info *info)
+struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
+					  struct acpi_gpio_info *info)
 {
 	struct acpi_gpio_lookup lookup;
 	struct list_head resource_list;
@@ -285,27 +283,26 @@ int acpi_get_gpio_by_index(struct device *dev, int index,
 	int ret;
 
 	if (!dev)
-		return -EINVAL;
+		return ERR_PTR(-EINVAL);
 
 	handle = ACPI_HANDLE(dev);
 	if (!handle || acpi_bus_get_device(handle, &adev))
-		return -ENODEV;
+		return ERR_PTR(-ENODEV);
 
 	memset(&lookup, 0, sizeof(lookup));
 	lookup.index = index;
-	lookup.gpio = -ENODEV;
 
 	INIT_LIST_HEAD(&resource_list);
 	ret = acpi_dev_get_resources(adev, &resource_list, acpi_find_gpio,
 				     &lookup);
 	if (ret < 0)
-		return ret;
+		return ERR_PTR(ret);
 
 	acpi_dev_free_resource_list(&resource_list);
 
-	if (lookup.gpio >= 0 && info)
+	if (lookup.desc && info)
 		*info = lookup.info;
 
-	return lookup.gpio;
+	return lookup.desc ? lookup.desc : ERR_PTR(-ENODEV);
 }
-EXPORT_SYMBOL_GPL(acpi_get_gpio_by_index);
+EXPORT_SYMBOL_GPL(acpi_get_gpiod_by_index);
diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h
index 4c120a1..bbfa83a 100644
--- a/include/linux/acpi_gpio.h
+++ b/include/linux/acpi_gpio.h
@@ -2,8 +2,10 @@
 #define _LINUX_ACPI_GPIO_H_
 
 #include <linux/device.h>
+#include <linux/err.h>
 #include <linux/errno.h>
 #include <linux/gpio.h>
+#include <linux/gpio/consumer.h>
 
 /**
  * struct acpi_gpio_info - ACPI GPIO specific information
@@ -15,23 +17,24 @@ struct acpi_gpio_info {
 
 #ifdef CONFIG_GPIO_ACPI
 
-int acpi_get_gpio(char *path, int pin);
-int acpi_get_gpio_by_index(struct device *dev, int index,
-			   struct acpi_gpio_info *info);
+struct gpio_desc *acpi_get_gpiod(char *path, int pin);
+struct gpio_desc *acpi_get_gpiod_by_index(struct device *dev, int index,
+					  struct acpi_gpio_info *info);
 void acpi_gpiochip_request_interrupts(struct gpio_chip *chip);
 void acpi_gpiochip_free_interrupts(struct gpio_chip *chip);
 
 #else /* CONFIG_GPIO_ACPI */
 
-static inline int acpi_get_gpio(char *path, int pin)
+static inline struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 {
-	return -ENODEV;
+	return ERR_PTR(-ENOSYS);
 }
 
-static inline int acpi_get_gpio_by_index(struct device *dev, int index,
-					 struct acpi_gpio_info *info)
+static inline struct gpio_desc *
+acpi_get_gpiod_by_index(struct device *dev, int index,
+			struct acpi_gpio_info *info)
 {
-	return -ENODEV;
+	return ERR_PTR(-ENOSYS);
 }
 
 static inline void acpi_gpiochip_request_interrupts(struct gpio_chip *chip) { }
@@ -39,4 +42,23 @@ static inline void acpi_gpiochip_free_interrupts(struct gpio_chip *chip) { }
 
 #endif /* CONFIG_GPIO_ACPI */
 
+static inline int acpi_get_gpio(char *path, int pin)
+{
+	struct gpio_desc *desc = acpi_get_gpiod(path, pin);
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+	return desc_to_gpio(desc);
+}
+
+static inline int acpi_get_gpio_by_index(struct device *dev, int index,
+					 struct acpi_gpio_info *info)
+{
+	struct gpio_desc *desc = acpi_get_gpiod_by_index(dev, index, info);
+
+	if (IS_ERR(desc))
+		return PTR_ERR(desc);
+	return desc_to_gpio(desc);
+}
+
 #endif /* _LINUX_ACPI_GPIO_H_ */
-- 
1.8.4.rc3

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Alexandre Courbot]

On Tue, Oct 1, 2013 at 7:58 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> The new GPIO descriptor based interface is now preferred over the old
> integer based one. This patch converts the ACPI GPIO helpers to use this
> new interface internally. In addition to that provide compatibility
> functions acpi_get_gpio() and acpi_get_gpio_by_index() that convert the
> returned GPIO descriptors to integers.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 51 +++++++++++++++++++++------------------------
>  include/linux/acpi_gpio.h   | 38 ++++++++++++++++++++++++++-------
>  2 files changed, 54 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 1745ce5..333297c 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -11,7 +11,7 @@
>   */
>
>  #include <linux/errno.h>
> -#include <linux/gpio.h>
> +#include <linux/gpio/consumer.h>
>  #include <linux/export.h>
>  #include <linux/acpi_gpio.h>
>  #include <linux/acpi.h>
> @@ -33,14 +33,15 @@ static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
>  }
>
>  /**
> - * acpi_get_gpio() - Translate ACPI GPIO pin to GPIO number usable with GPIO API
> + * acpi_get_gpiod() - Translate ACPI GPIO pin to GPIO descriptor usable with GPIO API
>   * @path:      ACPI GPIO controller full path name, (e.g. "\\_SB.GPO1")
>   * @pin:       ACPI GPIO pin number (0-based, controller-relative)
>   *
> - * Returns GPIO number to use with Linux generic GPIO API, or errno error value
> + * Returns GPIO descriptor to use with Linux generic GPIO API, or ERR_PTR
> + * error value
>   */
>
> -int acpi_get_gpio(char *path, int pin)
> +struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  {
>         struct gpio_chip *chip;
>         acpi_handle handle;
> @@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
>
>         status = acpi_get_handle(NULL, path, &handle);
>         if (ACPI_FAILURE(status))
> -               return -ENODEV;
> +               return ERR_PTR(-ENODEV);
>
>         chip = gpiochip_find(handle, acpi_gpiochip_find);
>         if (!chip)
> -               return -ENODEV;
> +               return ERR_PTR(-ENODEV);
>
> -       if (!gpio_is_valid(chip->base + pin))
> -               return -EINVAL;
> -
> -       return chip->base + pin;
> +       return gpio_to_desc(chip->base + pin);

I think you want to avoid using gpio_to_desc(). It is just a
convenience function provided to ease the transition for consumers
that still need to rely on GPIO numbers for some reason. The same
applies to desc_to_gpio(), although the usage you are making of it
(implemented fallbacks for the integer space) is the one that is
intended.

The last line could be changed to "return &chip->desc[pin];" after
checking that 0 <= pin <= chip->ngpio.

This minor issue apart, I really like the fact you are moving this
under the gpiolib APIs (also appreciate the testing of the new API
this patchset provides!).

I also wonder whether you could also avoid exporting acpi_get_gpiod*
(which allow GPIO consumers to shortcut gpiolib) by implementing
acpi_get_gpio* into the C file (maybe even using gpiod_get()). I see a
"char *path" parameter though, so maybe that would not be possible at
the moment.

Once the gpio_to_desc() issue mentioned above is fixed I think I can give my

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

on patches 2-5.

Thanks,
Alex.

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Mika Westerberg]

On Mon, Oct 07, 2013 at 09:54:13PM -0700, Alexandre Courbot wrote:
> > +struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> >  {
> >         struct gpio_chip *chip;
> >         acpi_handle handle;
> > @@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
> >
> >         status = acpi_get_handle(NULL, path, &handle);
> >         if (ACPI_FAILURE(status))
> > -               return -ENODEV;
> > +               return ERR_PTR(-ENODEV);
> >
> >         chip = gpiochip_find(handle, acpi_gpiochip_find);
> >         if (!chip)
> > -               return -ENODEV;
> > +               return ERR_PTR(-ENODEV);
> >
> > -       if (!gpio_is_valid(chip->base + pin))
> > -               return -EINVAL;
> > -
> > -       return chip->base + pin;
> > +       return gpio_to_desc(chip->base + pin);
> 
> I think you want to avoid using gpio_to_desc(). It is just a
> convenience function provided to ease the transition for consumers
> that still need to rely on GPIO numbers for some reason. The same
> applies to desc_to_gpio(), although the usage you are making of it
> (implemented fallbacks for the integer space) is the one that is
> intended.
> 
> The last line could be changed to "return &chip->desc[pin];" after
> checking that 0 <= pin <= chip->ngpio.

I tried that but it doesn't compile anymore :-(

drivers/gpio/gpiolib-acpi.c: In function ‘acpi_get_gpiod’:
drivers/gpio/gpiolib-acpi.c:61:2: error: invalid use of undefined type ‘struct gpio_desc’
drivers/gpio/gpiolib-acpi.c:61:20: error: dereferencing pointer to incomplete type

Since struct gpio_desc is internal to gpiolib.c we can't dereference it
outside.

The DT version also uses gpio_to_desc().

> This minor issue apart, I really like the fact you are moving this
> under the gpiolib APIs (also appreciate the testing of the new API
> this patchset provides!).
> 
> I also wonder whether you could also avoid exporting acpi_get_gpiod*
> (which allow GPIO consumers to shortcut gpiolib) by implementing
> acpi_get_gpio* into the C file (maybe even using gpiod_get()). I see a
> "char *path" parameter though, so maybe that would not be possible at
> the moment.

Good point, I'll check if we can do that.

> Once the gpio_to_desc() issue mentioned above is fixed I think I can give my
> 
> Reviewed-by: Alexandre Courbot <acourbot@nvidia.com>

Thanks!

> 
> on patches 2-5.
> 
> Thanks,
> Alex.
--
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

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Mika Westerberg]

On Tue, Oct 08, 2013 at 11:45:08AM +0300, Mika Westerberg wrote:
> > I also wonder whether you could also avoid exporting acpi_get_gpiod*
> > (which allow GPIO consumers to shortcut gpiolib) by implementing
> > acpi_get_gpio* into the C file (maybe even using gpiod_get()). I see a
> > "char *path" parameter though, so maybe that would not be possible at
> > the moment.
> 
> Good point, I'll check if we can do that.

Looks like we can get rid of acpi_get_gpio() and acpi_get_gpiod() as there
are no users for those outside gpiolib-acpi. I'm going to do that in the
next revision.

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Alexandre Courbot]

On Tue, Oct 8, 2013 at 3:36 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Oct 08, 2013 at 11:45:08AM +0300, Mika Westerberg wrote:
>> > I also wonder whether you could also avoid exporting acpi_get_gpiod*
>> > (which allow GPIO consumers to shortcut gpiolib) by implementing
>> > acpi_get_gpio* into the C file (maybe even using gpiod_get()). I see a
>> > "char *path" parameter though, so maybe that would not be possible at
>> > the moment.
>>
>> Good point, I'll check if we can do that.
>
> Looks like we can get rid of acpi_get_gpio() and acpi_get_gpiod() as there
> are no users for those outside gpiolib-acpi. I'm going to do that in the
> next revision.

That's good news. If you can make it such that ACPI GPIO consumers
don't need to include acpi_gpio.h anymore, then this would make my
day. :)

Alex.

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Mika Westerberg]

On Tue, Oct 08, 2013 at 09:26:14AM -0700, Alexandre Courbot wrote:
> On Tue, Oct 8, 2013 at 3:36 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Tue, Oct 08, 2013 at 11:45:08AM +0300, Mika Westerberg wrote:
> >> > I also wonder whether you could also avoid exporting acpi_get_gpiod*
> >> > (which allow GPIO consumers to shortcut gpiolib) by implementing
> >> > acpi_get_gpio* into the C file (maybe even using gpiod_get()). I see a
> >> > "char *path" parameter though, so maybe that would not be possible at
> >> > the moment.
> >>
> >> Good point, I'll check if we can do that.
> >
> > Looks like we can get rid of acpi_get_gpio() and acpi_get_gpiod() as there
> > are no users for those outside gpiolib-acpi. I'm going to do that in the
> > next revision.
> 
> That's good news. If you can make it such that ACPI GPIO consumers
> don't need to include acpi_gpio.h anymore, then this would make my
> day. :)

We can get rid of the whole apci_gpio.h eventually. But before doing that I
would like to have something that supports both the ACPI way and gpiod way
in v3.13 if possible.

Once we have gpiod_get_index() in mainline, we can convert the existing
user to it and drop acpi_gpio.h completely.

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Alexandre Courbot]

On Tue, Oct 8, 2013 at 1:45 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Oct 07, 2013 at 09:54:13PM -0700, Alexandre Courbot wrote:
>> > +struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>> >  {
>> >         struct gpio_chip *chip;
>> >         acpi_handle handle;
>> > @@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
>> >
>> >         status = acpi_get_handle(NULL, path, &handle);
>> >         if (ACPI_FAILURE(status))
>> > -               return -ENODEV;
>> > +               return ERR_PTR(-ENODEV);
>> >
>> >         chip = gpiochip_find(handle, acpi_gpiochip_find);
>> >         if (!chip)
>> > -               return -ENODEV;
>> > +               return ERR_PTR(-ENODEV);
>> >
>> > -       if (!gpio_is_valid(chip->base + pin))
>> > -               return -EINVAL;
>> > -
>> > -       return chip->base + pin;
>> > +       return gpio_to_desc(chip->base + pin);
>>
>> I think you want to avoid using gpio_to_desc(). It is just a
>> convenience function provided to ease the transition for consumers
>> that still need to rely on GPIO numbers for some reason. The same
>> applies to desc_to_gpio(), although the usage you are making of it
>> (implemented fallbacks for the integer space) is the one that is
>> intended.
>>
>> The last line could be changed to "return &chip->desc[pin];" after
>> checking that 0 <= pin <= chip->ngpio.
>
> I tried that but it doesn't compile anymore :-(
>
> drivers/gpio/gpiolib-acpi.c: In function ‘acpi_get_gpiod’:
> drivers/gpio/gpiolib-acpi.c:61:2: error: invalid use of undefined type ‘struct gpio_desc’
> drivers/gpio/gpiolib-acpi.c:61:20: error: dereferencing pointer to incomplete type

Ah right, there is no way to know the size of a gpio_desc from here...
Maybe I should make a gpiochip_get_desc(index) function accessible to
drivers only that takes care of this. Another possibility would be to
move this function into gpiolib.c but there are probably too many
dependencies with ACPI for that.

In the long term I would like to see gpio_to_desc()/desc_to_gpio()
disappear from the consumer interface as they allow unsafe use of GPIO
descriptors.

> The DT version also uses gpio_to_desc().

That seems to speak in favor of that gpiochip_get_desc() function I
talked about earlier.

Thanks,
Alex.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Mika Westerberg]

On Tue, Oct 08, 2013 at 09:24:50AM -0700, Alexandre Courbot wrote:
> On Tue, Oct 8, 2013 at 1:45 AM, Mika Westerberg
> <mika.westerberg@linux.intel.com> wrote:
> > On Mon, Oct 07, 2013 at 09:54:13PM -0700, Alexandre Courbot wrote:
> >> > +struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> >> >  {
> >> >         struct gpio_chip *chip;
> >> >         acpi_handle handle;
> >> > @@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
> >> >
> >> >         status = acpi_get_handle(NULL, path, &handle);
> >> >         if (ACPI_FAILURE(status))
> >> > -               return -ENODEV;
> >> > +               return ERR_PTR(-ENODEV);
> >> >
> >> >         chip = gpiochip_find(handle, acpi_gpiochip_find);
> >> >         if (!chip)
> >> > -               return -ENODEV;
> >> > +               return ERR_PTR(-ENODEV);
> >> >
> >> > -       if (!gpio_is_valid(chip->base + pin))
> >> > -               return -EINVAL;
> >> > -
> >> > -       return chip->base + pin;
> >> > +       return gpio_to_desc(chip->base + pin);
> >>
> >> I think you want to avoid using gpio_to_desc(). It is just a
> >> convenience function provided to ease the transition for consumers
> >> that still need to rely on GPIO numbers for some reason. The same
> >> applies to desc_to_gpio(), although the usage you are making of it
> >> (implemented fallbacks for the integer space) is the one that is
> >> intended.
> >>
> >> The last line could be changed to "return &chip->desc[pin];" after
> >> checking that 0 <= pin <= chip->ngpio.
> >
> > I tried that but it doesn't compile anymore :-(
> >
> > drivers/gpio/gpiolib-acpi.c: In function ‘acpi_get_gpiod’:
> > drivers/gpio/gpiolib-acpi.c:61:2: error: invalid use of undefined type ‘struct gpio_desc’
> > drivers/gpio/gpiolib-acpi.c:61:20: error: dereferencing pointer to incomplete type
> 
> Ah right, there is no way to know the size of a gpio_desc from here...
> Maybe I should make a gpiochip_get_desc(index) function accessible to
> drivers only that takes care of this. Another possibility would be to
> move this function into gpiolib.c but there are probably too many
> dependencies with ACPI for that.
> 
> In the long term I would like to see gpio_to_desc()/desc_to_gpio()
> disappear from the consumer interface as they allow unsafe use of GPIO
> descriptors.
> 
> > The DT version also uses gpio_to_desc().
> 
> That seems to speak in favor of that gpiochip_get_desc() function I
> talked about earlier.

OK, I can convert the ACPI code to use that once such function exists. I
the meanwhile, can I use gpio_to_desc() in the next revision of the
patches?
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/5] gpiolib / ACPI: convert to gpiod interfaces

[Alexandre Courbot]

On Wed, Oct 9, 2013 at 12:58 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Tue, Oct 08, 2013 at 09:24:50AM -0700, Alexandre Courbot wrote:
>> On Tue, Oct 8, 2013 at 1:45 AM, Mika Westerberg
>> <mika.westerberg@linux.intel.com> wrote:
>> > On Mon, Oct 07, 2013 at 09:54:13PM -0700, Alexandre Courbot wrote:
>> >> > +struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>> >> >  {
>> >> >         struct gpio_chip *chip;
>> >> >         acpi_handle handle;
>> >> > @@ -48,18 +49,15 @@ int acpi_get_gpio(char *path, int pin)
>> >> >
>> >> >         status = acpi_get_handle(NULL, path, &handle);
>> >> >         if (ACPI_FAILURE(status))
>> >> > -               return -ENODEV;
>> >> > +               return ERR_PTR(-ENODEV);
>> >> >
>> >> >         chip = gpiochip_find(handle, acpi_gpiochip_find);
>> >> >         if (!chip)
>> >> > -               return -ENODEV;
>> >> > +               return ERR_PTR(-ENODEV);
>> >> >
>> >> > -       if (!gpio_is_valid(chip->base + pin))
>> >> > -               return -EINVAL;
>> >> > -
>> >> > -       return chip->base + pin;
>> >> > +       return gpio_to_desc(chip->base + pin);
>> >>
>> >> I think you want to avoid using gpio_to_desc(). It is just a
>> >> convenience function provided to ease the transition for consumers
>> >> that still need to rely on GPIO numbers for some reason. The same
>> >> applies to desc_to_gpio(), although the usage you are making of it
>> >> (implemented fallbacks for the integer space) is the one that is
>> >> intended.
>> >>
>> >> The last line could be changed to "return &chip->desc[pin];" after
>> >> checking that 0 <= pin <= chip->ngpio.
>> >
>> > I tried that but it doesn't compile anymore :-(
>> >
>> > drivers/gpio/gpiolib-acpi.c: In function ‘acpi_get_gpiod’:
>> > drivers/gpio/gpiolib-acpi.c:61:2: error: invalid use of undefined type ‘struct gpio_desc’
>> > drivers/gpio/gpiolib-acpi.c:61:20: error: dereferencing pointer to incomplete type
>>
>> Ah right, there is no way to know the size of a gpio_desc from here...
>> Maybe I should make a gpiochip_get_desc(index) function accessible to
>> drivers only that takes care of this. Another possibility would be to
>> move this function into gpiolib.c but there are probably too many
>> dependencies with ACPI for that.
>>
>> In the long term I would like to see gpio_to_desc()/desc_to_gpio()
>> disappear from the consumer interface as they allow unsafe use of GPIO
>> descriptors.
>>
>> > The DT version also uses gpio_to_desc().
>>
>> That seems to speak in favor of that gpiochip_get_desc() function I
>> talked about earlier.
>
> OK, I can convert the ACPI code to use that once such function exists. I
> the meanwhile, can I use gpio_to_desc() in the next revision of the
> patches?

Sure, please do so - anyway that's the only way you have at the
moment. I will add gpiochip_get_desc() to v2 of gpiod and we'll
convert your code either before or after it is merged, depending on
our timing.
--
To unsubscribe from this list: send the line "unsubscribe linux-gpio" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/5] gpiolib / ACPI: add ACPI support for gpiod_get_index()

[Mika Westerberg]

gpiod_get_index() and gpiod_get() are now the new preferred way to request
GPIOs. Add support for finding the corresponding GPIO descriptor from ACPI
namespace.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index e2a2cdd..1e099de 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -10,6 +10,7 @@
 #include <linux/seq_file.h>
 #include <linux/gpio.h>
 #include <linux/of_gpio.h>
+#include <linux/acpi_gpio.h>
 #include <linux/idr.h>
 #include <linux/slab.h>
 
@@ -2111,6 +2112,12 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *id,
 }
 #endif
 
+static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
+					unsigned int idx, unsigned long *flags)
+{
+	return acpi_get_gpiod_by_index(dev, idx, NULL);
+}
+
 static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
 				    unsigned int idx, unsigned long *flags)
 {
@@ -2192,6 +2199,9 @@ struct gpio_desc *__must_check gpiod_get_index(struct device *dev,
 	if (IS_ENABLED(CONFIG_OF) && dev && dev->of_node) {
 		dev_dbg(dev, "using device tree for GPIO lookup\n");
 		desc = of_find_gpio(dev, con_id, idx, &flags);
+	} else if (IS_ENABLED(CONFIG_ACPI) && dev && ACPI_HANDLE(dev)) {
+		dev_dbg(dev, "using ACPI for GPIO lookup\n");
+		desc = acpi_find_gpio(dev, con_id, idx, &flags);
 	} else {
 		dev_dbg(dev, "using lookup tables for GPIO lookup");
 		desc = gpiod_find(dev, con_id, idx, &flags);
-- 
1.8.4.rc3

[PATCH 4/5] gpiolib / ACPI: allow passing GPIOF_ACTIVE_LOW for GpioInt resources

[Mika Westerberg]

The ACPI GpioInt resources contain polarity field that is used to specify
whether the interrupt is active high or low. Since gpiolib supports
GPIOF_ACTIVE_LOW we can pass this information in the flags field in
acpi_find_gpio(), analogous to the DeviceTree version.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/gpio/gpiolib-acpi.c |  2 ++
 drivers/gpio/gpiolib.c      | 12 +++++++++++-
 include/linux/acpi_gpio.h   |  2 ++
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 333297c..4c6df41 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -251,6 +251,8 @@ static int acpi_find_gpio(struct acpi_resource *ares, void *data)
 					      agpio->pin_table[0]);
 		lookup->info.gpioint =
 			agpio->connection_type == ACPI_RESOURCE_GPIO_TYPE_INT;
+		lookup->info.active_low =
+			agpio->polarity == ACPI_ACTIVE_LOW;
 	}
 
 	return 1;
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 1e099de..b01a231 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -2115,7 +2115,17 @@ static struct gpio_desc *of_find_gpio(struct device *dev, const char *id,
 static struct gpio_desc *acpi_find_gpio(struct device *dev, const char *con_id,
 					unsigned int idx, unsigned long *flags)
 {
-	return acpi_get_gpiod_by_index(dev, idx, NULL);
+	struct acpi_gpio_info info;
+	struct gpio_desc *desc;
+
+	desc = acpi_get_gpiod_by_index(dev, idx, &info);
+	if (IS_ERR(desc))
+		return desc;
+
+	if (info.gpioint && info.active_low)
+		*flags |= GPIOF_ACTIVE_LOW;
+
+	return desc;
 }
 
 static struct gpio_desc *gpiod_find(struct device *dev, const char *con_id,
diff --git a/include/linux/acpi_gpio.h b/include/linux/acpi_gpio.h
index bbfa83a..af599ec 100644
--- a/include/linux/acpi_gpio.h
+++ b/include/linux/acpi_gpio.h
@@ -10,9 +10,11 @@
 /**
  * struct acpi_gpio_info - ACPI GPIO specific information
  * @gpioint: if %true this GPIO is of type GpioInt otherwise type is GpioIo
+ * @active_low: in case of @gpioint, the pin is active low
  */
 struct acpi_gpio_info {
 	bool gpioint;
+	bool active_low;
 };
 
 #ifdef CONFIG_GPIO_ACPI
-- 
1.8.4.rc3

[PATCH 5/5] gpiolib / ACPI: document the GPIO descriptor based interface

[Mika Westerberg]

In addition to the existing ACPI specific GPIO interface, document the new
descriptor based GPIO interface in Documentation/acpi/enumeration.txt, so
it is clear that this new interface is preferred over the ACPI specific
version.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 Documentation/acpi/enumeration.txt | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/Documentation/acpi/enumeration.txt b/Documentation/acpi/enumeration.txt
index aca4e69..48bb9ab 100644
--- a/Documentation/acpi/enumeration.txt
+++ b/Documentation/acpi/enumeration.txt
@@ -322,3 +322,25 @@ suitable to the gpiolib before passing them.
 
 In case of GpioInt resource an additional call to gpio_to_irq() must be
 done before calling request_irq().
+
+Note that the above API is ACPI specific and not recommended for drivers
+that need to support non-ACPI systems. The recommended way is to use
+the descriptor based GPIO interfaces. The above example looks like this
+when converted to the GPIO desc:
+
+	#include <linux/gpio/consumer.h>
+	...
+
+	struct gpio_desc *irq_desc, *power_desc;
+
+	irq_desc = gpiod_get_index(dev, NULL, 1);
+	if (IS_ERR(irq_desc))
+		/* handle error */
+
+	power_desc = gpiod_get_index(dev, NULL, 0);
+	if (IS_ERR(power_desc))
+		/* handle error */
+
+	/* Now we can use the GPIO descriptors */
+
+See also Documentation/gpio.txt.
-- 
1.8.4.rc3

Re: [PATCH 0/5] gpiolib: convert ACPI GPIO helpers to gpiod_ interfaces

[Rafael J. Wysocki]

On Tuesday, October 01, 2013 05:58:52 PM Mika Westerberg wrote:
> Hi all,
> 
> This series converts the ACPI GPIO helpers in gpiolib-acpi.c to use the new
> and preferred GPIO descriptor based interface as suggested by the GPIO
> subsystem maintainer.
> 
> The first patch is just a cosmetic fix to move acpi_gpiochip_free_interrupts()
> closer to acpi_gpiochip_request_interrupts(). Patches [2-5/5] do the actual
> conversion.
> 
> The series is based on the latest patches from Alexandre Courbot and I've
> tested them on Intel Haswell based machine.
> 
> Please review.
> 
> Mika Westerberg (5):
>   gpiolib / ACPI: move acpi_gpiochip_free_interrupts next to the request function
>   gpiolib / ACPI: convert to gpiod interfaces
>   gpiolib / ACPI: add ACPI support for gpiod_get_index()
>   gpiolib / ACPI: allow passing GPIOF_ACTIVE_LOW for GpioInt resources
>   gpiolib / ACPI: document the GPIO descriptor based interface

The patches look good to me so

Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

for the whole series.

Thanks!


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