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

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

[Mika Westerberg]

Hi,

This series tries to add what is missing in current Linux ACPI GPIO
support. There are two new features that were introduced with ACPI 5.0: 

 * 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.

Patches [3,4/6] Rework the ACPI GPIO signaled event support.

Support for GPIO operation regions is a new thing (well, kind of, it was
tried already once [1] but never got further from that). The idea here is
that the ASL code can toggle GPIOs via help of ACPI enabled Linux GPIO
driver. Implementation is in Patch [6/6].

These patches are based on two patches from Alexandre Courbot [2] which
introduce gpiochip_get_desc() function.

[1] http://www.spinics.net/lists/linux-acpi/msg46230.html
[2] https://lkml.org/lkml/2014/2/9/24

Mika Westerberg (6):
  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: Embed events list directly into struct acpi_gpio_chip
  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      |  60 +++++-
 drivers/gpio/gpiolib.h      |   3 +
 3 files changed, 398 insertions(+), 130 deletions(-)

-- 
1.9.0.rc3

[PATCH 1/6] 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 usecase is ACPI ASL code that should be able
to toggle GPIOs through GPIO operation regions.

We can't really use gpio_request() and its counterparts because it will pin
the module to the kernel forever (as it calls module_get()). Instead we
provide a gpiolib internal functions gpiochip_request/free_own_desc() that
work the same as gpio_request() but don't manipulate module refrence count.

Since it's the GPIO chip driver who requests the GPIOs in the first place
we can be sure that it cannot be unloaded without the driver knowing about
that. Furthermore we only limit this functionality to be available only
inside gpiolib.

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

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index f60d74bc2fce..489a63524eb6 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1458,7 +1458,8 @@ 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,
+			   bool module_refcount)
 {
 	struct gpio_chip	*chip;
 	int			status = -EPROBE_DEFER;
@@ -1475,8 +1476,10 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 	if (chip == NULL)
 		goto done;
 
-	if (!try_module_get(chip->owner))
-		goto done;
+	if (module_refcount) {
+		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 +1490,8 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 		status = 0;
 	} else {
 		status = -EBUSY;
-		module_put(chip->owner);
+		if (module_refcount)
+			module_put(chip->owner);
 		goto done;
 	}
 
@@ -1499,7 +1503,8 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
 
 		if (status < 0) {
 			desc_set_label(desc, NULL);
-			module_put(chip->owner);
+			if (module_refcount)
+				module_put(chip->owner);
 			clear_bit(FLAG_REQUESTED, &desc->flags);
 			goto done;
 		}
@@ -1517,13 +1522,18 @@ done:
 	return status;
 }
 
+static int gpiod_request(struct gpio_desc *desc, const char *label)
+{
+	return __gpiod_request(desc, label, true);
+}
+
 int gpio_request(unsigned gpio, const char *label)
 {
 	return gpiod_request(gpio_to_desc(gpio), label);
 }
 EXPORT_SYMBOL_GPL(gpio_request);
 
-static void gpiod_free(struct gpio_desc *desc)
+static void __gpiod_free(struct gpio_desc *desc, bool module_refcount)
 {
 	unsigned long		flags;
 	struct gpio_chip	*chip;
@@ -1548,7 +1558,8 @@ static void gpiod_free(struct gpio_desc *desc)
 			spin_lock_irqsave(&gpio_lock, flags);
 		}
 		desc_set_label(desc, NULL);
-		module_put(desc->chip->owner);
+		if (module_refcount)
+			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);
@@ -1559,6 +1570,11 @@ static void gpiod_free(struct gpio_desc *desc)
 	spin_unlock_irqrestore(&gpio_lock, flags);
 }
 
+static void gpiod_free(struct gpio_desc *desc)
+{
+	__gpiod_free(desc, true);
+}
+
 void gpio_free(unsigned gpio)
 {
 	gpiod_free(gpio_to_desc(gpio));
@@ -1678,6 +1694,33 @@ 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)
+{
+	return __gpiod_request(desc, label, false);
+}
+
+/**
+ * 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)
+{
+	__gpiod_free(desc, false);
+}
 
 /* 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.rc3

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

[Rafael J. Wysocki]

On Monday, February 24, 2014 06:00:06 PM Mika Westerberg wrote:
> Sometimes it is useful to allow GPIO chips themselves to request GPIOs they
> own through gpiolib API. One usecase is ACPI ASL code that should be able
> to toggle GPIOs through GPIO operation regions.
> 
> We can't really use gpio_request() and its counterparts because it will pin
> the module to the kernel forever (as it calls module_get()). Instead we
> provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> work the same as gpio_request() but don't manipulate module refrence count.
> 
> Since it's the GPIO chip driver who requests the GPIOs in the first place
> we can be sure that it cannot be unloaded without the driver knowing about
> that. Furthermore we only limit this functionality to be available only
> inside gpiolib.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib.c | 57 +++++++++++++++++++++++++++++++++++++++++++-------
>  drivers/gpio/gpiolib.h |  3 +++
>  2 files changed, 53 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index f60d74bc2fce..489a63524eb6 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1458,7 +1458,8 @@ 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,
> +			   bool module_refcount)
>  {
>  	struct gpio_chip	*chip;
>  	int			status = -EPROBE_DEFER;
> @@ -1475,8 +1476,10 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>  	if (chip == NULL)
>  		goto done;
>  
> -	if (!try_module_get(chip->owner))
> -		goto done;
> +	if (module_refcount) {
> +		if (!try_module_get(chip->owner))
> +			goto done;
> +	}

I'm wondering why you're not moving the module refcount manipulation entirely
to gpiod_request()?

I guess that's because of the locking, but I suppose that desc->chip will never
be NULL in gpiochip_request_own_desc(), so you don't even need to check it there?

So it looks like gpiochip_request_own_desc() can do something like

	lock
	__gpiod_request(stuff)
	unlock

where __gpiod_request() is just the internal part starting at the "NOTE" comment.

>  
>  	/* NOTE:  gpio_request() can be called in early boot,
>  	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
> @@ -1487,7 +1490,8 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>  		status = 0;
>  	} else {
>  		status = -EBUSY;
> -		module_put(chip->owner);
> +		if (module_refcount)
> +			module_put(chip->owner);
>  		goto done;
>  	}
>  
> @@ -1499,7 +1503,8 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
>  
>  		if (status < 0) {
>  			desc_set_label(desc, NULL);
> -			module_put(chip->owner);
> +			if (module_refcount)
> +				module_put(chip->owner);
>  			clear_bit(FLAG_REQUESTED, &desc->flags);
>  			goto done;
>  		}
> @@ -1517,13 +1522,18 @@ done:
>  	return status;
>  }
>  
> +static int gpiod_request(struct gpio_desc *desc, const char *label)
> +{
> +	return __gpiod_request(desc, label, true);
> +}
> +
>  int gpio_request(unsigned gpio, const char *label)
>  {
>  	return gpiod_request(gpio_to_desc(gpio), label);
>  }
>  EXPORT_SYMBOL_GPL(gpio_request);
>  
> -static void gpiod_free(struct gpio_desc *desc)
> +static void __gpiod_free(struct gpio_desc *desc, bool module_refcount)
>  {
>  	unsigned long		flags;
>  	struct gpio_chip	*chip;
> @@ -1548,7 +1558,8 @@ static void gpiod_free(struct gpio_desc *desc)
>  			spin_lock_irqsave(&gpio_lock, flags);
>  		}
>  		desc_set_label(desc, NULL);
> -		module_put(desc->chip->owner);
> +		if (module_refcount)
> +			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);
> @@ -1559,6 +1570,11 @@ static void gpiod_free(struct gpio_desc *desc)
>  	spin_unlock_irqrestore(&gpio_lock, flags);
>  }
>  
> +static void gpiod_free(struct gpio_desc *desc)
> +{
> +	__gpiod_free(desc, true);
> +}
> +
>  void gpio_free(unsigned gpio)
>  {
>  	gpiod_free(gpio_to_desc(gpio));
> @@ -1678,6 +1694,33 @@ 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)
> +{
> +	return __gpiod_request(desc, label, false);
> +}
> +
> +/**
> + * 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)
> +{
> +	__gpiod_free(desc, false);
> +}
>  
>  /* 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 */
> 

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

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

[Mika Westerberg]

On Tue, Feb 25, 2014 at 03:10:24PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:06 PM Mika Westerberg wrote:
> > Sometimes it is useful to allow GPIO chips themselves to request GPIOs they
> > own through gpiolib API. One usecase is ACPI ASL code that should be able
> > to toggle GPIOs through GPIO operation regions.
> > 
> > We can't really use gpio_request() and its counterparts because it will pin
> > the module to the kernel forever (as it calls module_get()). Instead we
> > provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> > work the same as gpio_request() but don't manipulate module refrence count.
> > 
> > Since it's the GPIO chip driver who requests the GPIOs in the first place
> > we can be sure that it cannot be unloaded without the driver knowing about
> > that. Furthermore we only limit this functionality to be available only
> > inside gpiolib.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/gpio/gpiolib.c | 57 +++++++++++++++++++++++++++++++++++++++++++-------
> >  drivers/gpio/gpiolib.h |  3 +++
> >  2 files changed, 53 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index f60d74bc2fce..489a63524eb6 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1458,7 +1458,8 @@ 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,
> > +			   bool module_refcount)
> >  {
> >  	struct gpio_chip	*chip;
> >  	int			status = -EPROBE_DEFER;
> > @@ -1475,8 +1476,10 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
> >  	if (chip == NULL)
> >  		goto done;
> >  
> > -	if (!try_module_get(chip->owner))
> > -		goto done;
> > +	if (module_refcount) {
> > +		if (!try_module_get(chip->owner))
> > +			goto done;
> > +	}
> 
> I'm wondering why you're not moving the module refcount manipulation entirely
> to gpiod_request()?
> 
> I guess that's because of the locking, but I suppose that desc->chip will never
> be NULL in gpiochip_request_own_desc(), so you don't even need to check it there?
> 
> So it looks like gpiochip_request_own_desc() can do something like
> 
> 	lock
> 	__gpiod_request(stuff)
> 	unlock
> 
> where __gpiod_request() is just the internal part starting at the "NOTE" comment.

Sounds good. Only thing I'm not sure about is the fact that
__gpiod_request() releases the lock when it calls chip driver callbacks
(and takes it back of course). Is that acceptable practice to take the lock
outside of a function and release it inside for a while?

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

[Rafael J. Wysocki]

On Wednesday, February 26, 2014 11:05:42 AM Mika Westerberg wrote:
> On Tue, Feb 25, 2014 at 03:10:24PM +0100, Rafael J. Wysocki wrote:
> > On Monday, February 24, 2014 06:00:06 PM Mika Westerberg wrote:
> > > Sometimes it is useful to allow GPIO chips themselves to request GPIOs they
> > > own through gpiolib API. One usecase is ACPI ASL code that should be able
> > > to toggle GPIOs through GPIO operation regions.
> > > 
> > > We can't really use gpio_request() and its counterparts because it will pin
> > > the module to the kernel forever (as it calls module_get()). Instead we
> > > provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> > > work the same as gpio_request() but don't manipulate module refrence count.
> > > 
> > > Since it's the GPIO chip driver who requests the GPIOs in the first place
> > > we can be sure that it cannot be unloaded without the driver knowing about
> > > that. Furthermore we only limit this functionality to be available only
> > > inside gpiolib.
> > > 
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > > ---
> > >  drivers/gpio/gpiolib.c | 57 +++++++++++++++++++++++++++++++++++++++++++-------
> > >  drivers/gpio/gpiolib.h |  3 +++
> > >  2 files changed, 53 insertions(+), 7 deletions(-)
> > > 
> > > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > > index f60d74bc2fce..489a63524eb6 100644
> > > --- a/drivers/gpio/gpiolib.c
> > > +++ b/drivers/gpio/gpiolib.c
> > > @@ -1458,7 +1458,8 @@ 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,
> > > +			   bool module_refcount)
> > >  {
> > >  	struct gpio_chip	*chip;
> > >  	int			status = -EPROBE_DEFER;
> > > @@ -1475,8 +1476,10 @@ static int gpiod_request(struct gpio_desc *desc, const char *label)
> > >  	if (chip == NULL)
> > >  		goto done;
> > >  
> > > -	if (!try_module_get(chip->owner))
> > > -		goto done;
> > > +	if (module_refcount) {
> > > +		if (!try_module_get(chip->owner))
> > > +			goto done;
> > > +	}
> > 
> > I'm wondering why you're not moving the module refcount manipulation entirely
> > to gpiod_request()?
> > 
> > I guess that's because of the locking, but I suppose that desc->chip will never
> > be NULL in gpiochip_request_own_desc(), so you don't even need to check it there?
> > 
> > So it looks like gpiochip_request_own_desc() can do something like
> > 
> > 	lock
> > 	__gpiod_request(stuff)
> > 	unlock
> > 
> > where __gpiod_request() is just the internal part starting at the "NOTE" comment.
> 
> Sounds good. Only thing I'm not sure about is the fact that
> __gpiod_request() releases the lock when it calls chip driver callbacks
> (and takes it back of course). Is that acceptable practice to take the lock
> outside of a function and release it inside for a while?

Yes, you can do that.

There even are sparse annotations for that: __releases() and __acquires()
(__rpm_callback() in drivers/base/power/runtime.c uses them among other things).

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

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

[Mika Westerberg]

On Wed, Feb 26, 2014 at 02:47:58PM +0100, Rafael J. Wysocki wrote:
> > Sounds good. Only thing I'm not sure about is the fact that
> > __gpiod_request() releases the lock when it calls chip driver callbacks
> > (and takes it back of course). Is that acceptable practice to take the lock
> > outside of a function and release it inside for a while?
> 
> Yes, you can do that.
> 
> There even are sparse annotations for that: __releases() and __acquires()
> (__rpm_callback() in drivers/base/power/runtime.c uses them among other things).

Ah, good to know. Thanks!

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

[Linus Walleij]

On Tue, Feb 25, 2014 at 12:00 AM, 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 usecase is ACPI ASL code that should be able
> to toggle GPIOs through GPIO operation regions.
>
> We can't really use gpio_request() and its counterparts because it will pin
> the module to the kernel forever (as it calls module_get()). Instead we
> provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> work the same as gpio_request() but don't manipulate module refrence count.
>
> Since it's the GPIO chip driver who requests the GPIOs in the first place
> we can be sure that it cannot be unloaded without the driver knowing about
> that. Furthermore we only limit this functionality to be available only
> inside gpiolib.
>
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>

I fully trust you in doing the ACPI stuff in patches 2-n but on this patch
in particular I want Alexandre's review tag as well, as he's working
actively with the descriptor API and I don't want to add too many quirks
without his consent.

So Alexandre, what do you say about this?

Yours,
Linus Walleij

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

[Mika Westerberg]

On Wed, Mar 05, 2014 at 10:49:41AM +0800, Linus Walleij wrote:
> On Tue, Feb 25, 2014 at 12:00 AM, 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 usecase is ACPI ASL code that should be able
> > to toggle GPIOs through GPIO operation regions.
> >
> > We can't really use gpio_request() and its counterparts because it will pin
> > the module to the kernel forever (as it calls module_get()). Instead we
> > provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> > work the same as gpio_request() but don't manipulate module refrence count.
> >
> > Since it's the GPIO chip driver who requests the GPIOs in the first place
> > we can be sure that it cannot be unloaded without the driver knowing about
> > that. Furthermore we only limit this functionality to be available only
> > inside gpiolib.
> >
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> 
> I fully trust you in doing the ACPI stuff in patches 2-n but on this patch
> in particular I want Alexandre's review tag as well, as he's working
> actively with the descriptor API and I don't want to add too many quirks
> without his consent.

Thanks for your trust :)

> So Alexandre, what do you say about this?

I'm about to send v2 of the series with Rafael's comments taken into
account. However, I stumbled to another locking problem:

I'm going to move taking the gpio_lock outside of __gpiod_request() and
have __gpiod_request() to release that lock, so that we can call
chip->request() safely.

Since we are using _irqsave()/_irqrestore() versions, it means that I need
to pass flags as a pointer from gpiod_request() to __gpiod_request() like:

	spin_lock_irqsave(&gpio_lock, flags);
	if (try_module_get(chip->owner)) {
		ret = __gpiod_request(desc, label, &flags);
		...

Is that acceptable or can you guys suggest some alternative? One
alternative that I can think about is to have __gpiod_request() to take the
lock and move try_module_get() outside to gpiod_request():

__gpiod_request()
{
	unsigned long flags;

	spin_lock_irqsave(&gpio_lock, flags);
	...
}

gpiod_request():
{
	...
	if (try_module_get(chip->owner)) {
		ret = __gpiod_request(desc, label);
		...
}

Thoughts?

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

[Rafael J. Wysocki]

On Wednesday, March 05, 2014 02:05:50 PM Mika Westerberg wrote:
> On Wed, Mar 05, 2014 at 10:49:41AM +0800, Linus Walleij wrote:
> > On Tue, Feb 25, 2014 at 12:00 AM, 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 usecase is ACPI ASL code that should be able
> > > to toggle GPIOs through GPIO operation regions.
> > >
> > > We can't really use gpio_request() and its counterparts because it will pin
> > > the module to the kernel forever (as it calls module_get()). Instead we
> > > provide a gpiolib internal functions gpiochip_request/free_own_desc() that
> > > work the same as gpio_request() but don't manipulate module refrence count.
> > >
> > > Since it's the GPIO chip driver who requests the GPIOs in the first place
> > > we can be sure that it cannot be unloaded without the driver knowing about
> > > that. Furthermore we only limit this functionality to be available only
> > > inside gpiolib.
> > >
> > > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > 
> > I fully trust you in doing the ACPI stuff in patches 2-n but on this patch
> > in particular I want Alexandre's review tag as well, as he's working
> > actively with the descriptor API and I don't want to add too many quirks
> > without his consent.
> 
> Thanks for your trust :)
> 
> > So Alexandre, what do you say about this?
> 
> I'm about to send v2 of the series with Rafael's comments taken into
> account. However, I stumbled to another locking problem:
> 
> I'm going to move taking the gpio_lock outside of __gpiod_request() and
> have __gpiod_request() to release that lock, so that we can call
> chip->request() safely.
> 
> Since we are using _irqsave()/_irqrestore() versions, it means that I need
> to pass flags as a pointer from gpiod_request() to __gpiod_request() like:
> 
> 	spin_lock_irqsave(&gpio_lock, flags);
> 	if (try_module_get(chip->owner)) {
> 		ret = __gpiod_request(desc, label, &flags);

Ouch.  Sorry for overlooking that.

> 		...
> 
> Is that acceptable or can you guys suggest some alternative? One
> alternative that I can think about is to have __gpiod_request() to take the
> lock and move try_module_get() outside to gpiod_request():
> 
> __gpiod_request()
> {
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&gpio_lock, flags);
> 	...
> }
> 
> gpiod_request():
> {
> 	...
> 	if (try_module_get(chip->owner)) {
> 		ret = __gpiod_request(desc, label);
> 		...
> }
> 
> Thoughts?

If that works, it would be better than passing the flags IMO.

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

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

[Alexandre Courbot]

On Wed, Mar 5, 2014 at 11:49 AM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Tue, Feb 25, 2014 at 12:00 AM, 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 usecase is ACPI ASL code that should be able
>> to toggle GPIOs through GPIO operation regions.
>>
>> We can't really use gpio_request() and its counterparts because it will pin
>> the module to the kernel forever (as it calls module_get()). Instead we
>> provide a gpiolib internal functions gpiochip_request/free_own_desc() that
>> work the same as gpio_request() but don't manipulate module refrence count.
>>
>> Since it's the GPIO chip driver who requests the GPIOs in the first place
>> we can be sure that it cannot be unloaded without the driver knowing about
>> that. Furthermore we only limit this functionality to be available only
>> inside gpiolib.
>>
>> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
>
> I fully trust you in doing the ACPI stuff in patches 2-n but on this patch
> in particular I want Alexandre's review tag as well, as he's working
> actively with the descriptor API and I don't want to add too many quirks
> without his consent.
>
> So Alexandre, what do you say about this?

I will wait for v2 to give it a real review, but since the new
functions are not exposed outside of gpiolib and the patch seems to
serve a real purpose I have no objection to it. Especially if Rafael's
suggestions can be applied.

[PATCH 2/6] 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.

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index b7db098ba060..5f5f107c2099 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
+ * @achip:      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,9 +101,10 @@ 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 *achip)
 {
 	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
+	struct gpio_chip *chip = achip->chip;
 	struct acpi_resource *res;
 	acpi_handle handle, evt_handle;
 	struct list_head *evt_pins = NULL;
@@ -123,12 +129,7 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 		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;
-			}
+			achip->evt_pins = evt_pins;
 		}
 	}
 
@@ -197,30 +198,24 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
 
 /**
  * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
- * @chip:      gpio chip
+ * @achip:      ACPI GPIO chip
  *
  * Free interrupts associated with the _EVT method for the given GPIO chip.
  *
  * The remaining ACPI event interrupts associated with the chip are freed
  * automatically.
  */
-static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
+static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
 {
-	acpi_handle handle;
-	acpi_status status;
 	struct list_head *evt_pins;
 	struct acpi_gpio_evt_pin *evt_pin, *ep;
+	struct gpio_chip *chip = achip->chip;
 
-	if (!chip->dev || !chip->to_irq)
-		return;
-
-	handle = ACPI_HANDLE(chip->dev);
-	if (!handle)
+	if (!chip->dev || !chip->to_irq || !achip->evt_pins)
 		return;
 
-	status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
-	if (ACPI_FAILURE(status))
-		return;
+	evt_pins = achip->evt_pins;
+	achip->evt_pins = NULL;
 
 	list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
 		devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
@@ -228,7 +223,6 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 		kfree(evt_pin);
 	}
 
-	acpi_detach_data(handle, acpi_gpio_evt_dh);
 	kfree(evt_pins);
 }
 
@@ -312,10 +306,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 *achip;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(chip->dev);
+	if (!handle)
+		return;
+
+	achip = kzalloc(sizeof(*achip), GFP_KERNEL);
+	if (!achip) {
+		dev_err(chip->dev,
+			"Failed to allocate memory for ACPI GPIO chip\n");
+		return;
+	}
+
+	achip->chip = chip;
+
+	status = acpi_attach_data(handle, acpi_gpio_chip_dh, achip);
+	if (ACPI_FAILURE(status)) {
+		dev_err(chip->dev, "Failed to attach ACPI GPIO chip\n");
+		kfree(achip);
+		return;
+	}
+
+	acpi_gpiochip_request_interrupts(achip);
 }
 
 void acpi_gpiochip_remove(struct gpio_chip *chip)
 {
-	acpi_gpiochip_free_interrupts(chip);
+	struct acpi_gpio_chip *achip;
+	acpi_handle handle;
+	acpi_status status;
+
+	handle = ACPI_HANDLE(chip->dev);
+	if (!handle)
+		return;
+
+	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&achip);
+	if (ACPI_FAILURE(status)) {
+		dev_warn(chip->dev, "Failed to retrieve ACPI GPIO chip\n");
+		return;
+	}
+
+	acpi_gpiochip_free_interrupts(achip);
+
+	acpi_detach_data(handle, acpi_gpio_chip_dh);
+	kfree(achip);
 }
-- 
1.9.0.rc3

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

[Rafael J. Wysocki]

On Monday, February 24, 2014 06:00:07 PM Mika Westerberg wrote:
> 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.
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 83 ++++++++++++++++++++++++++++++++-------------
>  1 file changed, 59 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index b7db098ba060..5f5f107c2099 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;

Hmm.  Why exactly evt_pins has to be a pointer?

> +};
> +
>  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
> + * @achip:      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,9 +101,10 @@ 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 *achip)

I would call the argument "acpi_gpio" instead of achip (and analogously below),
because the structure is a "chip plus some additional info".

>  {
>  	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> +	struct gpio_chip *chip = achip->chip;
>  	struct acpi_resource *res;
>  	acpi_handle handle, evt_handle;
>  	struct list_head *evt_pins = NULL;
> @@ -123,12 +129,7 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
>  		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;
> -			}
> +			achip->evt_pins = evt_pins;

What about doing INIT_LIST_HEAD(&acpi_gpio->evt_pins) instead (if it's not a
pointer)?

>  		}
>  	}
>  
> @@ -197,30 +198,24 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
>  
>  /**
>   * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> - * @chip:      gpio chip
> + * @achip:      ACPI GPIO chip
>   *
>   * Free interrupts associated with the _EVT method for the given GPIO chip.
>   *
>   * The remaining ACPI event interrupts associated with the chip are freed
>   * automatically.
>   */
> -static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
> +static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)



>  {
> -	acpi_handle handle;
> -	acpi_status status;
>  	struct list_head *evt_pins;
>  	struct acpi_gpio_evt_pin *evt_pin, *ep;
> +	struct gpio_chip *chip = achip->chip;
>  
> -	if (!chip->dev || !chip->to_irq)
> -		return;
> -
> -	handle = ACPI_HANDLE(chip->dev);
> -	if (!handle)
> +	if (!chip->dev || !chip->to_irq || !achip->evt_pins)
>  		return;
>  
> -	status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
> -	if (ACPI_FAILURE(status))
> -		return;
> +	evt_pins = achip->evt_pins;
> +	achip->evt_pins = NULL;
>  
>  	list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
>  		devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
> @@ -228,7 +223,6 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
>  		kfree(evt_pin);
>  	}
>  
> -	acpi_detach_data(handle, acpi_gpio_evt_dh);
>  	kfree(evt_pins);
>  }
>  
> @@ -312,10 +306,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 *achip;
> +	acpi_handle handle;
> +	acpi_status status;
> +
> +	handle = ACPI_HANDLE(chip->dev);
> +	if (!handle)
> +		return;
> +
> +	achip = kzalloc(sizeof(*achip), GFP_KERNEL);
> +	if (!achip) {
> +		dev_err(chip->dev,
> +			"Failed to allocate memory for ACPI GPIO chip\n");
> +		return;
> +	}
> +
> +	achip->chip = chip;
> +
> +	status = acpi_attach_data(handle, acpi_gpio_chip_dh, achip);

To be honest, I'd prefer that to be associated with struct acpi_device rather
than with the handle, but that's not a big deal for now.

> +	if (ACPI_FAILURE(status)) {
> +		dev_err(chip->dev, "Failed to attach ACPI GPIO chip\n");
> +		kfree(achip);
> +		return;
> +	}
> +
> +	acpi_gpiochip_request_interrupts(achip);
>  }
>  
>  void acpi_gpiochip_remove(struct gpio_chip *chip)
>  {
> -	acpi_gpiochip_free_interrupts(chip);
> +	struct acpi_gpio_chip *achip;
> +	acpi_handle handle;
> +	acpi_status status;
> +
> +	handle = ACPI_HANDLE(chip->dev);
> +	if (!handle)
> +		return;
> +
> +	status = acpi_get_data(handle, acpi_gpio_chip_dh, (void **)&achip);
> +	if (ACPI_FAILURE(status)) {
> +		dev_warn(chip->dev, "Failed to retrieve ACPI GPIO chip\n");
> +		return;
> +	}
> +
> +	acpi_gpiochip_free_interrupts(achip);
> +
> +	acpi_detach_data(handle, acpi_gpio_chip_dh);
> +	kfree(achip);
>  }
> 

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

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

[Mika Westerberg]

On Tue, Feb 25, 2014 at 03:21:55PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:07 PM Mika Westerberg wrote:
> > 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.
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/gpio/gpiolib-acpi.c | 83 ++++++++++++++++++++++++++++++++-------------
> >  1 file changed, 59 insertions(+), 24 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index b7db098ba060..5f5f107c2099 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;
> 
> Hmm.  Why exactly evt_pins has to be a pointer?
> 
> > +};
> > +
> >  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
> > + * @achip:      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,9 +101,10 @@ 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 *achip)
> 
> I would call the argument "acpi_gpio" instead of achip (and analogously below),
> because the structure is a "chip plus some additional info".

OK.

> 
> >  {
> >  	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> > +	struct gpio_chip *chip = achip->chip;
> >  	struct acpi_resource *res;
> >  	acpi_handle handle, evt_handle;
> >  	struct list_head *evt_pins = NULL;
> > @@ -123,12 +129,7 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
> >  		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;
> > -			}
> > +			achip->evt_pins = evt_pins;
> 
> What about doing INIT_LIST_HEAD(&acpi_gpio->evt_pins) instead (if it's not a
> pointer)?
> 
> >  		}
> >  	}
> >  
> > @@ -197,30 +198,24 @@ static void acpi_gpiochip_request_interrupts(struct gpio_chip *chip)
> >  
> >  /**
> >   * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> > - * @chip:      gpio chip
> > + * @achip:      ACPI GPIO chip
> >   *
> >   * Free interrupts associated with the _EVT method for the given GPIO chip.
> >   *
> >   * The remaining ACPI event interrupts associated with the chip are freed
> >   * automatically.
> >   */
> > -static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
> > +static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> 
> 
> 
> >  {
> > -	acpi_handle handle;
> > -	acpi_status status;
> >  	struct list_head *evt_pins;
> >  	struct acpi_gpio_evt_pin *evt_pin, *ep;
> > +	struct gpio_chip *chip = achip->chip;
> >  
> > -	if (!chip->dev || !chip->to_irq)
> > -		return;
> > -
> > -	handle = ACPI_HANDLE(chip->dev);
> > -	if (!handle)
> > +	if (!chip->dev || !chip->to_irq || !achip->evt_pins)
> >  		return;
> >  
> > -	status = acpi_get_data(handle, acpi_gpio_evt_dh, (void **)&evt_pins);
> > -	if (ACPI_FAILURE(status))
> > -		return;
> > +	evt_pins = achip->evt_pins;
> > +	achip->evt_pins = NULL;
> >  
> >  	list_for_each_entry_safe_reverse(evt_pin, ep, evt_pins, node) {
> >  		devm_free_irq(chip->dev, evt_pin->irq, evt_pin);
> > @@ -228,7 +223,6 @@ static void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
> >  		kfree(evt_pin);
> >  	}
> >  
> > -	acpi_detach_data(handle, acpi_gpio_evt_dh);
> >  	kfree(evt_pins);
> >  }
> >  
> > @@ -312,10 +306,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 *achip;
> > +	acpi_handle handle;
> > +	acpi_status status;
> > +
> > +	handle = ACPI_HANDLE(chip->dev);
> > +	if (!handle)
> > +		return;
> > +
> > +	achip = kzalloc(sizeof(*achip), GFP_KERNEL);
> > +	if (!achip) {
> > +		dev_err(chip->dev,
> > +			"Failed to allocate memory for ACPI GPIO chip\n");
> > +		return;
> > +	}
> > +
> > +	achip->chip = chip;
> > +
> > +	status = acpi_attach_data(handle, acpi_gpio_chip_dh, achip);
> 
> To be honest, I'd prefer that to be associated with struct acpi_device rather
> than with the handle, but that's not a big deal for now.

OK, we can do that later if needed.

[PATCH 3/6] 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 we
make the structure name reflect that we are dealing with any event, not
just _EVT.

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 5f5f107c2099..cb99dc2c552e 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -19,7 +19,7 @@
 
 #include "gpiolib.h"
 
-struct acpi_gpio_evt_pin {
+struct acpi_gpio_event {
 	struct list_head node;
 	acpi_handle *evt_handle;
 	unsigned int pin;
@@ -28,7 +28,7 @@ struct acpi_gpio_evt_pin {
 
 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->evt_handle, NULL, event->pin);
 
 	return IRQ_HANDLED;
 }
@@ -107,7 +107,7 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 	struct gpio_chip *chip = achip->chip;
 	struct acpi_resource *res;
 	acpi_handle handle, evt_handle;
-	struct list_head *evt_pins = NULL;
+	struct list_head *events = NULL;
 	acpi_status status;
 	unsigned int pin;
 	int irq, ret;
@@ -126,10 +126,10 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 
 	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);
-			achip->evt_pins = evt_pins;
+		events = kzalloc(sizeof(*events), GFP_KERNEL);
+		if (events) {
+			INIT_LIST_HEAD(events);
+			achip->events = events;
 		}
 	}
 
@@ -168,19 +168,19 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 				data = ev_handle;
 			}
 		}
-		if (!handler && evt_pins) {
-			struct acpi_gpio_evt_pin *evt_pin;
+		if (!handler && events) {
+			struct acpi_gpio_event *event;
 
-			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, evt_pins);
-			evt_pin->evt_handle = evt_handle;
-			evt_pin->pin = pin;
-			evt_pin->irq = irq;
+			list_add_tail(&event->node, events);
+			event->evt_handle = evt_handle;
+			event->pin = pin;
+			event->irq = irq;
 			handler = acpi_gpio_irq_handler_evt;
-			data = evt_pin;
+			data = event;
 		}
 		if (!handler)
 			continue;
@@ -207,23 +207,23 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
  */
 static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
 {
-	struct list_head *evt_pins;
-	struct acpi_gpio_evt_pin *evt_pin, *ep;
+	struct list_head *events;
+	struct acpi_gpio_event *event, *ep;
 	struct gpio_chip *chip = achip->chip;
 
-	if (!chip->dev || !chip->to_irq || !achip->evt_pins)
+	if (!chip->dev || !chip->to_irq || !achip->events)
 		return;
 
-	evt_pins = achip->evt_pins;
-	achip->evt_pins = NULL;
+	events = achip->events;
+	achip->events = NULL;
 
-	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);
+	list_for_each_entry_safe_reverse(event, ep, events, node) {
+		devm_free_irq(chip->dev, event->irq, event);
+		list_del(&event->node);
+		kfree(event);
 	}
 
-	kfree(evt_pins);
+	kfree(events);
 }
 
 struct acpi_gpio_lookup {
-- 
1.9.0.rc3

Re: [PATCH 3/6] gpio / ACPI: Rename acpi_gpio_evt_pin to acpi_gpio_event

[Rafael J. Wysocki]

On Monday, February 24, 2014 06:00:08 PM Mika Westerberg wrote:
> In order to consolidate _Exx, _Lxx and _EVT to use the same structure we
> make the structure name reflect that we are dealing with any event, not
> just _EVT.

It's just a rename, right?  Then please say in the changelog that no functional
changes should result from this.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 56 ++++++++++++++++++++++-----------------------
>  1 file changed, 28 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index 5f5f107c2099..cb99dc2c552e 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -19,7 +19,7 @@
>  
>  #include "gpiolib.h"
>  
> -struct acpi_gpio_evt_pin {
> +struct acpi_gpio_event {
>  	struct list_head node;
>  	acpi_handle *evt_handle;
>  	unsigned int pin;
> @@ -28,7 +28,7 @@ struct acpi_gpio_evt_pin {
>  
>  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->evt_handle, NULL, event->pin);
>  
>  	return IRQ_HANDLED;
>  }
> @@ -107,7 +107,7 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  	struct gpio_chip *chip = achip->chip;
>  	struct acpi_resource *res;
>  	acpi_handle handle, evt_handle;
> -	struct list_head *evt_pins = NULL;
> +	struct list_head *events = NULL;
>  	acpi_status status;
>  	unsigned int pin;
>  	int irq, ret;
> @@ -126,10 +126,10 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  
>  	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);
> -			achip->evt_pins = evt_pins;
> +		events = kzalloc(sizeof(*events), GFP_KERNEL);
> +		if (events) {
> +			INIT_LIST_HEAD(events);
> +			achip->events = events;
>  		}
>  	}
>  
> @@ -168,19 +168,19 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  				data = ev_handle;
>  			}
>  		}
> -		if (!handler && evt_pins) {
> -			struct acpi_gpio_evt_pin *evt_pin;
> +		if (!handler && events) {
> +			struct acpi_gpio_event *event;
>  
> -			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, evt_pins);
> -			evt_pin->evt_handle = evt_handle;
> -			evt_pin->pin = pin;
> -			evt_pin->irq = irq;
> +			list_add_tail(&event->node, events);
> +			event->evt_handle = evt_handle;
> +			event->pin = pin;
> +			event->irq = irq;
>  			handler = acpi_gpio_irq_handler_evt;
> -			data = evt_pin;
> +			data = event;
>  		}
>  		if (!handler)
>  			continue;
> @@ -207,23 +207,23 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>   */
>  static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
>  {
> -	struct list_head *evt_pins;
> -	struct acpi_gpio_evt_pin *evt_pin, *ep;
> +	struct list_head *events;
> +	struct acpi_gpio_event *event, *ep;
>  	struct gpio_chip *chip = achip->chip;
>  
> -	if (!chip->dev || !chip->to_irq || !achip->evt_pins)
> +	if (!chip->dev || !chip->to_irq || !achip->events)
>  		return;
>  
> -	evt_pins = achip->evt_pins;
> -	achip->evt_pins = NULL;
> +	events = achip->events;
> +	achip->events = NULL;
>  
> -	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);
> +	list_for_each_entry_safe_reverse(event, ep, events, node) {
> +		devm_free_irq(chip->dev, event->irq, event);
> +		list_del(&event->node);
> +		kfree(event);
>  	}
>  
> -	kfree(evt_pins);
> +	kfree(events);
>  }
>  
>  struct acpi_gpio_lookup {
> 

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

Re: [PATCH 3/6] gpio / ACPI: Rename acpi_gpio_evt_pin to acpi_gpio_event

[Mika Westerberg]

On Tue, Feb 25, 2014 at 03:23:51PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:08 PM Mika Westerberg wrote:
> > In order to consolidate _Exx, _Lxx and _EVT to use the same structure we
> > make the structure name reflect that we are dealing with any event, not
> > just _EVT.
> 
> It's just a rename, right?  Then please say in the changelog that no functional
> changes should result from this.

OK, and I'm also going to rename field evt_handle to just handle in the
next version.

[PATCH 4/6] gpio / ACPI: Embed events list directly into struct acpi_gpio_chip

[Mika Westerberg]

It is not necessary to have events as a pointer to list in struct
acpi_gpio_chip. Instead we can embed the list_head directly to struct
acpi_gpio_chip itself. This makes event handling a bit simpler because now
we don't need to check whether the pointer is NULL or not.

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

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index cb99dc2c552e..c60db4ddc166 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -28,7 +28,7 @@ struct acpi_gpio_event {
 
 struct acpi_gpio_chip {
 	struct gpio_chip *chip;
-	struct list_head *events;
+	struct list_head events;
 };
 
 static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
@@ -106,8 +106,7 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
 	struct gpio_chip *chip = achip->chip;
 	struct acpi_resource *res;
-	acpi_handle handle, evt_handle;
-	struct list_head *events = NULL;
+	acpi_handle handle;
 	acpi_status status;
 	unsigned int pin;
 	int irq, ret;
@@ -120,19 +119,12 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 	if (!handle)
 		return;
 
+	INIT_LIST_HEAD(&achip->events);
+
 	status = acpi_get_event_resources(handle, &buf);
 	if (ACPI_FAILURE(status))
 		return;
 
-	status = acpi_get_handle(handle, "_EVT", &evt_handle);
-	if (ACPI_SUCCESS(status)) {
-		events = kzalloc(sizeof(*events), GFP_KERNEL);
-		if (events) {
-			INIT_LIST_HEAD(events);
-			achip->events = events;
-		}
-	}
-
 	/*
 	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
 	 * present, set up an interrupt handler that calls the ACPI event
@@ -168,14 +160,19 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 				data = ev_handle;
 			}
 		}
-		if (!handler && events) {
+		if (!handler) {
 			struct acpi_gpio_event *event;
+			acpi_handle evt_handle;
+
+			status = acpi_get_handle(handle, "_EVT", &evt_handle);
+			if (ACPI_FAILURE(status))
+				continue;
 
 			event = kzalloc(sizeof(*event), GFP_KERNEL);
 			if (!event)
 				continue;
 
-			list_add_tail(&event->node, events);
+			list_add_tail(&event->node, &achip->events);
 			event->evt_handle = evt_handle;
 			event->pin = pin;
 			event->irq = irq;
@@ -207,23 +204,17 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
  */
 static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
 {
-	struct list_head *events;
 	struct acpi_gpio_event *event, *ep;
 	struct gpio_chip *chip = achip->chip;
 
-	if (!chip->dev || !chip->to_irq || !achip->events)
+	if (!chip->dev || !chip->to_irq)
 		return;
 
-	events = achip->events;
-	achip->events = NULL;
-
-	list_for_each_entry_safe_reverse(event, ep, events, node) {
+	list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
 		devm_free_irq(chip->dev, event->irq, event);
 		list_del(&event->node);
 		kfree(event);
 	}
-
-	kfree(events);
 }
 
 struct acpi_gpio_lookup {
-- 
1.9.0.rc3

Re: [PATCH 4/6] gpio / ACPI: Embed events list directly into struct acpi_gpio_chip

[Rafael J. Wysocki]

On Monday, February 24, 2014 06:00:09 PM Mika Westerberg wrote:
> It is not necessary to have events as a pointer to list in struct
> acpi_gpio_chip. Instead we can embed the list_head directly to struct
> acpi_gpio_chip itself. This makes event handling a bit simpler because now
> we don't need to check whether the pointer is NULL or not.

It looks like I should have reviewed the whole series before commenting [2/6].

Well, my modified comment would be "Why don't you fold this one into [2/6]?", then.

> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 35 +++++++++++++----------------------
>  1 file changed, 13 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index cb99dc2c552e..c60db4ddc166 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -28,7 +28,7 @@ struct acpi_gpio_event {
>  
>  struct acpi_gpio_chip {
>  	struct gpio_chip *chip;
> -	struct list_head *events;
> +	struct list_head events;
>  };
>  
>  static int acpi_gpiochip_find(struct gpio_chip *gc, void *data)
> @@ -106,8 +106,7 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
>  	struct gpio_chip *chip = achip->chip;
>  	struct acpi_resource *res;
> -	acpi_handle handle, evt_handle;
> -	struct list_head *events = NULL;
> +	acpi_handle handle;
>  	acpi_status status;
>  	unsigned int pin;
>  	int irq, ret;
> @@ -120,19 +119,12 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  	if (!handle)
>  		return;
>  
> +	INIT_LIST_HEAD(&achip->events);
> +
>  	status = acpi_get_event_resources(handle, &buf);
>  	if (ACPI_FAILURE(status))
>  		return;
>  
> -	status = acpi_get_handle(handle, "_EVT", &evt_handle);
> -	if (ACPI_SUCCESS(status)) {
> -		events = kzalloc(sizeof(*events), GFP_KERNEL);
> -		if (events) {
> -			INIT_LIST_HEAD(events);
> -			achip->events = events;
> -		}
> -	}
> -
>  	/*
>  	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
>  	 * present, set up an interrupt handler that calls the ACPI event
> @@ -168,14 +160,19 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  				data = ev_handle;
>  			}
>  		}
> -		if (!handler && events) {
> +		if (!handler) {
>  			struct acpi_gpio_event *event;
> +			acpi_handle evt_handle;
> +
> +			status = acpi_get_handle(handle, "_EVT", &evt_handle);
> +			if (ACPI_FAILURE(status))
> +				continue;
>  
>  			event = kzalloc(sizeof(*event), GFP_KERNEL);
>  			if (!event)
>  				continue;
>  
> -			list_add_tail(&event->node, events);
> +			list_add_tail(&event->node, &achip->events);
>  			event->evt_handle = evt_handle;
>  			event->pin = pin;
>  			event->irq = irq;
> @@ -207,23 +204,17 @@ static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>   */
>  static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
>  {
> -	struct list_head *events;
>  	struct acpi_gpio_event *event, *ep;
>  	struct gpio_chip *chip = achip->chip;
>  
> -	if (!chip->dev || !chip->to_irq || !achip->events)
> +	if (!chip->dev || !chip->to_irq)
>  		return;
>  
> -	events = achip->events;
> -	achip->events = NULL;
> -
> -	list_for_each_entry_safe_reverse(event, ep, events, node) {
> +	list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
>  		devm_free_irq(chip->dev, event->irq, event);
>  		list_del(&event->node);
>  		kfree(event);
>  	}
> -
> -	kfree(events);
>  }
>  
>  struct acpi_gpio_lookup {
> 

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

Re: [PATCH 4/6] gpio / ACPI: Embed events list directly into struct acpi_gpio_chip

[Mika Westerberg]

On Tue, Feb 25, 2014 at 03:26:12PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:09 PM Mika Westerberg wrote:
> > It is not necessary to have events as a pointer to list in struct
> > acpi_gpio_chip. Instead we can embed the list_head directly to struct
> > acpi_gpio_chip itself. This makes event handling a bit simpler because now
> > we don't need to check whether the pointer is NULL or not.
> 
> It looks like I should have reviewed the whole series before commenting [2/6].
> 
> Well, my modified comment would be "Why don't you fold this one into [2/6]?", then.

I will in the next version, thanks.

[PATCH 5/6] 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>
---
 drivers/gpio/gpiolib-acpi.c | 221 ++++++++++++++++++++++++++------------------
 drivers/gpio/gpiolib.c      |   3 +-
 2 files changed, 132 insertions(+), 92 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index c60db4ddc166..275735f390af 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -21,7 +21,7 @@
 
 struct acpi_gpio_event {
 	struct list_head node;
-	acpi_handle *evt_handle;
+	acpi_handle evt_handle;
 	unsigned int pin;
 	unsigned int irq;
 };
@@ -70,9 +70,9 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
 
 static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
 {
-	acpi_handle handle = data;
+	struct acpi_gpio_event *event = data;
 
-	acpi_evaluate_object(handle, NULL, NULL, NULL);
+	acpi_evaluate_object(event->evt_handle, NULL, NULL, NULL);
 
 	return IRQ_HANDLED;
 }
@@ -91,6 +91,120 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
 	/* The address of this function is used as a key. */
 }
 
+static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
+						   void *context)
+{
+	struct acpi_gpio_chip *achip = context;
+	struct gpio_chip *chip = achip->chip;
+	struct acpi_resource_gpio *agpio;
+	acpi_handle handle, evt_handle;
+	struct acpi_gpio_event *event;
+	irq_handler_t handler = NULL;
+	struct gpio_desc *desc;
+	unsigned long irqflags;
+	int ret, pin, irq;
+
+	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
+		return AE_OK;
+
+	agpio = &ares->data.gpio;
+	if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
+		return AE_OK;
+
+	handle = ACPI_HANDLE(chip->dev);
+	pin = agpio->pin_table[0];
+
+	if (pin <= 255) {
+		char ev_name[5];
+		sprintf(ev_name, "_%c%02X",
+			agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
+			pin);
+		if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
+			handler = acpi_gpio_irq_handler;
+	}
+	if (!handler) {
+		if (ACPI_SUCCESS(acpi_get_handle(handle, "_EVT", &evt_handle)))
+			handler = acpi_gpio_irq_handler_evt;
+	}
+	if (!handler)
+		return AE_BAD_PARAMETER;
+
+	desc = gpiochip_get_desc(chip, pin);
+	if (IS_ERR(desc)) {
+		dev_err(chip->dev, "Failed to get GPIO descriptor\n");
+		return AE_ERROR;
+	}
+
+	ret = gpiochip_request_own_desc(desc, "ACPI:Event");
+	if (ret) {
+		dev_err(chip->dev, "Failed to request GPIO\n");
+		return AE_ERROR;
+	}
+
+	gpiod_direction_input(desc);
+
+	ret = gpiod_lock_as_irq(desc);
+	if (ret) {
+		dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
+		goto fail_free_desc;
+	}
+
+	irq = gpiod_to_irq(desc);
+	if (irq < 0) {
+		dev_err(chip->dev, "Failed to translate GPIO to IRQ\n");
+		goto fail_unlock_irq;
+	}
+
+	irqflags = IRQF_ONESHOT;
+	if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
+		if (agpio->polarity == ACPI_ACTIVE_HIGH)
+			irqflags |= IRQF_TRIGGER_HIGH;
+		else
+			irqflags |= IRQF_TRIGGER_LOW;
+	} else {
+		switch (agpio->polarity) {
+		case ACPI_ACTIVE_HIGH:
+			irqflags |= IRQF_TRIGGER_RISING;
+			break;
+		case ACPI_ACTIVE_LOW:
+			irqflags |= IRQF_TRIGGER_FALLING;
+			break;
+		default:
+			irqflags |= IRQF_TRIGGER_RISING |
+				    IRQF_TRIGGER_FALLING;
+			break;
+		}
+	}
+
+	event = kzalloc(sizeof(*event), GFP_KERNEL);
+	if (!event)
+		goto fail_unlock_irq;
+
+	event->evt_handle = evt_handle;
+	event->irq = irq;
+	event->pin = pin;
+
+	ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
+				   "ACPI:Event", event);
+	if (ret) {
+		dev_err(chip->dev, "Failed to setup interrupt handler for %d\n",
+			event->irq);
+		goto fail_free_event;
+	}
+
+	list_add_tail(&event->node, &achip->events);
+	return AE_OK;
+
+fail_free_event:
+	kfree(event);
+fail_unlock_irq:
+	gpiod_unlock_as_irq(desc);
+fail_free_desc:
+	gpiochip_free_own_desc(desc);
+
+	return AE_ERROR;
+}
+
 /**
  * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events
  * @achip:      ACPI GPIO chip
@@ -103,104 +217,22 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
  */
 static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
 {
-	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
 	struct gpio_chip *chip = achip->chip;
-	struct acpi_resource *res;
-	acpi_handle handle;
-	acpi_status status;
-	unsigned int pin;
-	int irq, ret;
-	char ev_name[5];
 
 	if (!chip->dev || !chip->to_irq)
 		return;
 
-	handle = ACPI_HANDLE(chip->dev);
-	if (!handle)
-		return;
-
 	INIT_LIST_HEAD(&achip->events);
-
-	status = acpi_get_event_resources(handle, &buf);
-	if (ACPI_FAILURE(status))
-		return;
-
-	/*
-	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
-	 * present, set up an interrupt handler that calls the ACPI event
-	 * handler.
-	 */
-	for (res = buf.pointer;
-	     res && (res->type != ACPI_RESOURCE_TYPE_END_TAG);
-	     res = ACPI_NEXT_RESOURCE(res)) {
-		irq_handler_t handler = NULL;
-		void *data;
-
-		if (res->type != ACPI_RESOURCE_TYPE_GPIO ||
-		    res->data.gpio.connection_type !=
-		    ACPI_RESOURCE_GPIO_TYPE_INT)
-			continue;
-
-		pin = res->data.gpio.pin_table[0];
-		if (pin > chip->ngpio)
-			continue;
-
-		irq = chip->to_irq(chip, pin);
-		if (irq < 0)
-			continue;
-
-		if (pin <= 255) {
-			acpi_handle ev_handle;
-
-			sprintf(ev_name, "_%c%02X",
-				res->data.gpio.triggering ? 'E' : 'L', pin);
-			status = acpi_get_handle(handle, ev_name, &ev_handle);
-			if (ACPI_SUCCESS(status)) {
-				handler = acpi_gpio_irq_handler;
-				data = ev_handle;
-			}
-		}
-		if (!handler) {
-			struct acpi_gpio_event *event;
-			acpi_handle evt_handle;
-
-			status = acpi_get_handle(handle, "_EVT", &evt_handle);
-			if (ACPI_FAILURE(status))
-				continue;
-
-			event = kzalloc(sizeof(*event), GFP_KERNEL);
-			if (!event)
-				continue;
-
-			list_add_tail(&event->node, &achip->events);
-			event->evt_handle = evt_handle;
-			event->pin = pin;
-			event->irq = irq;
-			handler = acpi_gpio_irq_handler_evt;
-			data = event;
-		}
-		if (!handler)
-			continue;
-
-		/* Assume BIOS sets the triggering, so no flags */
-		ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler,
-						0, "GPIO-signaled-ACPI-event",
-						data);
-		if (ret)
-			dev_err(chip->dev,
-				"Failed to request IRQ %d ACPI event handler\n",
-				irq);
-	}
+	acpi_walk_resources(ACPI_HANDLE(chip->dev), "_AEI",
+			    acpi_gpiochip_request_interrupt, achip);
 }
 
 /**
- * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
+ * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
  * @achip:      ACPI GPIO chip
  *
- * Free interrupts associated with the _EVT method for the given GPIO chip.
- *
- * The remaining ACPI event interrupts associated with the chip are freed
- * automatically.
+ * Free interrupts associated with GPIO ACPI event method for the given
+ * GPIO chip.
  */
 static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
 {
@@ -211,7 +243,14 @@ static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
 		return;
 
 	list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
-		devm_free_irq(chip->dev, event->irq, event);
+		struct gpio_desc *desc;
+
+		free_irq(event->irq, event);
+		desc = gpiochip_get_desc(chip, event->pin);
+		if (WARN_ON(IS_ERR(desc)))
+			continue;
+		gpiod_unlock_as_irq(desc);
+		gpiochip_free_own_desc(desc);
 		list_del(&event->node);
 		kfree(event);
 	}
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 489a63524eb6..474f7d1eb7d7 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -1266,11 +1266,12 @@ int gpiochip_remove(struct gpio_chip *chip)
 	int		status = 0;
 	unsigned	id;
 
+	acpi_gpiochip_remove(chip);
+
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	gpiochip_remove_pin_ranges(chip);
 	of_gpiochip_remove(chip);
-	acpi_gpiochip_remove(chip);
 
 	for (id = 0; id < chip->ngpio; id++) {
 		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
-- 
1.9.0.rc3

Re: [PATCH 5/6] gpio / ACPI: Rework ACPI GPIO event handling

[Rafael J. Wysocki]

On Monday, February 24, 2014 06:00:10 PM Mika Westerberg wrote:
> The current ACPI GPIO event handling code was never tested against real
> hardware with functioning GPIO triggered events (at the time such hardware
> wasn't available). Thus it misses certain things like requesting the GPIOs
> properly, passing correct flags to the interrupt handler and so on.
> 
> This patch reworks ACPI GPIO event handling so that we:
> 
>  1) Use struct acpi_gpio_event for all GPIO signaled events.
>  2) Switch to use GPIO descriptor API and request GPIOs by calling
>     gpiochip_request_own_desc() that we added in a previous patch.
>  3) Pass proper flags from ACPI GPIO resource to request_threaded_irq().
> 
> Also instead of open-coding the _AEI iteration loop we can use
> acpi_walk_resources(). This simplifies the code a bit and fixes memory leak
> that was caused by missing kfree() for buffer returned by
> acpi_get_event_resources().
> 
> Since the remove path now calls gpiochip_free_own_desc() which takes GPIO
> spinlock we need to call acpi_gpiochip_remove() outside of that lock
> (analogous to acpi_gpiochip_add() path where the lock is released before
> those funtions are called).
> 
> Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 221 ++++++++++++++++++++++++++------------------
>  drivers/gpio/gpiolib.c      |   3 +-
>  2 files changed, 132 insertions(+), 92 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index c60db4ddc166..275735f390af 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -21,7 +21,7 @@
>  
>  struct acpi_gpio_event {
>  	struct list_head node;
> -	acpi_handle *evt_handle;
> +	acpi_handle evt_handle;
>  	unsigned int pin;
>  	unsigned int irq;
>  };
> @@ -70,9 +70,9 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
>  
>  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
>  {
> -	acpi_handle handle = data;
> +	struct acpi_gpio_event *event = data;
>  
> -	acpi_evaluate_object(handle, NULL, NULL, NULL);
> +	acpi_evaluate_object(event->evt_handle, NULL, NULL, NULL);

This is a threaded irq handler, isn't it?

>  
>  	return IRQ_HANDLED;
>  }
> @@ -91,6 +91,120 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
>  	/* The address of this function is used as a key. */
>  }
>  
> +static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> +						   void *context)
> +{
> +	struct acpi_gpio_chip *achip = context;
> +	struct gpio_chip *chip = achip->chip;
> +	struct acpi_resource_gpio *agpio;
> +	acpi_handle handle, evt_handle;
> +	struct acpi_gpio_event *event;
> +	irq_handler_t handler = NULL;
> +	struct gpio_desc *desc;
> +	unsigned long irqflags;
> +	int ret, pin, irq;
> +
> +	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
> +		return AE_OK;
> +
> +	agpio = &ares->data.gpio;
> +	if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
> +		return AE_OK;
> +
> +	handle = ACPI_HANDLE(chip->dev);
> +	pin = agpio->pin_table[0];
> +
> +	if (pin <= 255) {
> +		char ev_name[5];
> +		sprintf(ev_name, "_%c%02X",
> +			agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
> +			pin);
> +		if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
> +			handler = acpi_gpio_irq_handler;
> +	}
> +	if (!handler) {
> +		if (ACPI_SUCCESS(acpi_get_handle(handle, "_EVT", &evt_handle)))
> +			handler = acpi_gpio_irq_handler_evt;
> +	}
> +	if (!handler)
> +		return AE_BAD_PARAMETER;
> +
> +	desc = gpiochip_get_desc(chip, pin);
> +	if (IS_ERR(desc)) {
> +		dev_err(chip->dev, "Failed to get GPIO descriptor\n");
> +		return AE_ERROR;
> +	}
> +
> +	ret = gpiochip_request_own_desc(desc, "ACPI:Event");
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to request GPIO\n");
> +		return AE_ERROR;
> +	}
> +
> +	gpiod_direction_input(desc);
> +
> +	ret = gpiod_lock_as_irq(desc);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
> +		goto fail_free_desc;
> +	}
> +
> +	irq = gpiod_to_irq(desc);
> +	if (irq < 0) {
> +		dev_err(chip->dev, "Failed to translate GPIO to IRQ\n");
> +		goto fail_unlock_irq;
> +	}
> +
> +	irqflags = IRQF_ONESHOT;
> +	if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
> +		if (agpio->polarity == ACPI_ACTIVE_HIGH)
> +			irqflags |= IRQF_TRIGGER_HIGH;
> +		else
> +			irqflags |= IRQF_TRIGGER_LOW;
> +	} else {
> +		switch (agpio->polarity) {
> +		case ACPI_ACTIVE_HIGH:
> +			irqflags |= IRQF_TRIGGER_RISING;
> +			break;
> +		case ACPI_ACTIVE_LOW:
> +			irqflags |= IRQF_TRIGGER_FALLING;
> +			break;
> +		default:
> +			irqflags |= IRQF_TRIGGER_RISING |
> +				    IRQF_TRIGGER_FALLING;
> +			break;
> +		}
> +	}
> +
> +	event = kzalloc(sizeof(*event), GFP_KERNEL);
> +	if (!event)
> +		goto fail_unlock_irq;
> +
> +	event->evt_handle = evt_handle;
> +	event->irq = irq;
> +	event->pin = pin;
> +
> +	ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
> +				   "ACPI:Event", event);
> +	if (ret) {
> +		dev_err(chip->dev, "Failed to setup interrupt handler for %d\n",
> +			event->irq);
> +		goto fail_free_event;
> +	}
> +
> +	list_add_tail(&event->node, &achip->events);
> +	return AE_OK;
> +
> +fail_free_event:
> +	kfree(event);
> +fail_unlock_irq:
> +	gpiod_unlock_as_irq(desc);
> +fail_free_desc:
> +	gpiochip_free_own_desc(desc);
> +
> +	return AE_ERROR;
> +}
> +
>  /**
>   * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events
>   * @achip:      ACPI GPIO chip
> @@ -103,104 +217,22 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
>   */
>  static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
>  {
> -	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
>  	struct gpio_chip *chip = achip->chip;
> -	struct acpi_resource *res;
> -	acpi_handle handle;
> -	acpi_status status;
> -	unsigned int pin;
> -	int irq, ret;
> -	char ev_name[5];
>  
>  	if (!chip->dev || !chip->to_irq)
>  		return;
>  
> -	handle = ACPI_HANDLE(chip->dev);
> -	if (!handle)
> -		return;
> -
>  	INIT_LIST_HEAD(&achip->events);
> -
> -	status = acpi_get_event_resources(handle, &buf);
> -	if (ACPI_FAILURE(status))
> -		return;
> -
> -	/*
> -	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
> -	 * present, set up an interrupt handler that calls the ACPI event
> -	 * handler.
> -	 */
> -	for (res = buf.pointer;
> -	     res && (res->type != ACPI_RESOURCE_TYPE_END_TAG);
> -	     res = ACPI_NEXT_RESOURCE(res)) {
> -		irq_handler_t handler = NULL;
> -		void *data;
> -
> -		if (res->type != ACPI_RESOURCE_TYPE_GPIO ||
> -		    res->data.gpio.connection_type !=
> -		    ACPI_RESOURCE_GPIO_TYPE_INT)
> -			continue;
> -
> -		pin = res->data.gpio.pin_table[0];
> -		if (pin > chip->ngpio)
> -			continue;
> -
> -		irq = chip->to_irq(chip, pin);
> -		if (irq < 0)
> -			continue;
> -
> -		if (pin <= 255) {
> -			acpi_handle ev_handle;
> -
> -			sprintf(ev_name, "_%c%02X",
> -				res->data.gpio.triggering ? 'E' : 'L', pin);
> -			status = acpi_get_handle(handle, ev_name, &ev_handle);
> -			if (ACPI_SUCCESS(status)) {
> -				handler = acpi_gpio_irq_handler;
> -				data = ev_handle;
> -			}
> -		}
> -		if (!handler) {
> -			struct acpi_gpio_event *event;
> -			acpi_handle evt_handle;
> -
> -			status = acpi_get_handle(handle, "_EVT", &evt_handle);
> -			if (ACPI_FAILURE(status))
> -				continue;
> -
> -			event = kzalloc(sizeof(*event), GFP_KERNEL);
> -			if (!event)
> -				continue;
> -
> -			list_add_tail(&event->node, &achip->events);
> -			event->evt_handle = evt_handle;
> -			event->pin = pin;
> -			event->irq = irq;
> -			handler = acpi_gpio_irq_handler_evt;
> -			data = event;
> -		}
> -		if (!handler)
> -			continue;
> -
> -		/* Assume BIOS sets the triggering, so no flags */
> -		ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler,
> -						0, "GPIO-signaled-ACPI-event",
> -						data);
> -		if (ret)
> -			dev_err(chip->dev,
> -				"Failed to request IRQ %d ACPI event handler\n",
> -				irq);
> -	}
> +	acpi_walk_resources(ACPI_HANDLE(chip->dev), "_AEI",
> +			    acpi_gpiochip_request_interrupt, achip);
>  }
>  
>  /**
> - * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> + * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
>   * @achip:      ACPI GPIO chip
>   *
> - * Free interrupts associated with the _EVT method for the given GPIO chip.
> - *
> - * The remaining ACPI event interrupts associated with the chip are freed
> - * automatically.
> + * Free interrupts associated with GPIO ACPI event method for the given
> + * GPIO chip.
>   */
>  static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
>  {
> @@ -211,7 +243,14 @@ static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
>  		return;
>  
>  	list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
> -		devm_free_irq(chip->dev, event->irq, event);
> +		struct gpio_desc *desc;
> +
> +		free_irq(event->irq, event);
> +		desc = gpiochip_get_desc(chip, event->pin);
> +		if (WARN_ON(IS_ERR(desc)))
> +			continue;
> +		gpiod_unlock_as_irq(desc);
> +		gpiochip_free_own_desc(desc);
>  		list_del(&event->node);
>  		kfree(event);
>  	}
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 489a63524eb6..474f7d1eb7d7 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -1266,11 +1266,12 @@ int gpiochip_remove(struct gpio_chip *chip)
>  	int		status = 0;
>  	unsigned	id;
>  
> +	acpi_gpiochip_remove(chip);
> +
>  	spin_lock_irqsave(&gpio_lock, flags);
>  
>  	gpiochip_remove_pin_ranges(chip);
>  	of_gpiochip_remove(chip);
> -	acpi_gpiochip_remove(chip);
>  
>  	for (id = 0; id < chip->ngpio; id++) {
>  		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
> 

Looks good to me.

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

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

Re: [PATCH 5/6] gpio / ACPI: Rework ACPI GPIO event handling

[Mika Westerberg]

On Tue, Feb 25, 2014 at 03:44:52PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:10 PM Mika Westerberg wrote:
> > The current ACPI GPIO event handling code was never tested against real
> > hardware with functioning GPIO triggered events (at the time such hardware
> > wasn't available). Thus it misses certain things like requesting the GPIOs
> > properly, passing correct flags to the interrupt handler and so on.
> > 
> > This patch reworks ACPI GPIO event handling so that we:
> > 
> >  1) Use struct acpi_gpio_event for all GPIO signaled events.
> >  2) Switch to use GPIO descriptor API and request GPIOs by calling
> >     gpiochip_request_own_desc() that we added in a previous patch.
> >  3) Pass proper flags from ACPI GPIO resource to request_threaded_irq().
> > 
> > Also instead of open-coding the _AEI iteration loop we can use
> > acpi_walk_resources(). This simplifies the code a bit and fixes memory leak
> > that was caused by missing kfree() for buffer returned by
> > acpi_get_event_resources().
> > 
> > Since the remove path now calls gpiochip_free_own_desc() which takes GPIO
> > spinlock we need to call acpi_gpiochip_remove() outside of that lock
> > (analogous to acpi_gpiochip_add() path where the lock is released before
> > those funtions are called).
> > 
> > Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
> > ---
> >  drivers/gpio/gpiolib-acpi.c | 221 ++++++++++++++++++++++++++------------------
> >  drivers/gpio/gpiolib.c      |   3 +-
> >  2 files changed, 132 insertions(+), 92 deletions(-)
> > 
> > diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> > index c60db4ddc166..275735f390af 100644
> > --- a/drivers/gpio/gpiolib-acpi.c
> > +++ b/drivers/gpio/gpiolib-acpi.c
> > @@ -21,7 +21,7 @@
> >  
> >  struct acpi_gpio_event {
> >  	struct list_head node;
> > -	acpi_handle *evt_handle;
> > +	acpi_handle evt_handle;
> >  	unsigned int pin;
> >  	unsigned int irq;
> >  };
> > @@ -70,9 +70,9 @@ static struct gpio_desc *acpi_get_gpiod(char *path, int pin)
> >  
> >  static irqreturn_t acpi_gpio_irq_handler(int irq, void *data)
> >  {
> > -	acpi_handle handle = data;
> > +	struct acpi_gpio_event *event = data;
> >  
> > -	acpi_evaluate_object(handle, NULL, NULL, NULL);
> > +	acpi_evaluate_object(event->evt_handle, NULL, NULL, NULL);
> 
> This is a threaded irq handler, isn't it?

Yes.

> >  
> >  	return IRQ_HANDLED;
> >  }
> > @@ -91,6 +91,120 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
> >  	/* The address of this function is used as a key. */
> >  }
> >  
> > +static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
> > +						   void *context)
> > +{
> > +	struct acpi_gpio_chip *achip = context;
> > +	struct gpio_chip *chip = achip->chip;
> > +	struct acpi_resource_gpio *agpio;
> > +	acpi_handle handle, evt_handle;
> > +	struct acpi_gpio_event *event;
> > +	irq_handler_t handler = NULL;
> > +	struct gpio_desc *desc;
> > +	unsigned long irqflags;
> > +	int ret, pin, irq;
> > +
> > +	if (ares->type != ACPI_RESOURCE_TYPE_GPIO)
> > +		return AE_OK;
> > +
> > +	agpio = &ares->data.gpio;
> > +	if (agpio->connection_type != ACPI_RESOURCE_GPIO_TYPE_INT)
> > +		return AE_OK;
> > +
> > +	handle = ACPI_HANDLE(chip->dev);
> > +	pin = agpio->pin_table[0];
> > +
> > +	if (pin <= 255) {
> > +		char ev_name[5];
> > +		sprintf(ev_name, "_%c%02X",
> > +			agpio->triggering == ACPI_EDGE_SENSITIVE ? 'E' : 'L',
> > +			pin);
> > +		if (ACPI_SUCCESS(acpi_get_handle(handle, ev_name, &evt_handle)))
> > +			handler = acpi_gpio_irq_handler;
> > +	}
> > +	if (!handler) {
> > +		if (ACPI_SUCCESS(acpi_get_handle(handle, "_EVT", &evt_handle)))
> > +			handler = acpi_gpio_irq_handler_evt;
> > +	}
> > +	if (!handler)
> > +		return AE_BAD_PARAMETER;
> > +
> > +	desc = gpiochip_get_desc(chip, pin);
> > +	if (IS_ERR(desc)) {
> > +		dev_err(chip->dev, "Failed to get GPIO descriptor\n");
> > +		return AE_ERROR;
> > +	}
> > +
> > +	ret = gpiochip_request_own_desc(desc, "ACPI:Event");
> > +	if (ret) {
> > +		dev_err(chip->dev, "Failed to request GPIO\n");
> > +		return AE_ERROR;
> > +	}
> > +
> > +	gpiod_direction_input(desc);
> > +
> > +	ret = gpiod_lock_as_irq(desc);
> > +	if (ret) {
> > +		dev_err(chip->dev, "Failed to lock GPIO as interrupt\n");
> > +		goto fail_free_desc;
> > +	}
> > +
> > +	irq = gpiod_to_irq(desc);
> > +	if (irq < 0) {
> > +		dev_err(chip->dev, "Failed to translate GPIO to IRQ\n");
> > +		goto fail_unlock_irq;
> > +	}
> > +
> > +	irqflags = IRQF_ONESHOT;
> > +	if (agpio->triggering == ACPI_LEVEL_SENSITIVE) {
> > +		if (agpio->polarity == ACPI_ACTIVE_HIGH)
> > +			irqflags |= IRQF_TRIGGER_HIGH;
> > +		else
> > +			irqflags |= IRQF_TRIGGER_LOW;
> > +	} else {
> > +		switch (agpio->polarity) {
> > +		case ACPI_ACTIVE_HIGH:
> > +			irqflags |= IRQF_TRIGGER_RISING;
> > +			break;
> > +		case ACPI_ACTIVE_LOW:
> > +			irqflags |= IRQF_TRIGGER_FALLING;
> > +			break;
> > +		default:
> > +			irqflags |= IRQF_TRIGGER_RISING |
> > +				    IRQF_TRIGGER_FALLING;
> > +			break;
> > +		}
> > +	}
> > +
> > +	event = kzalloc(sizeof(*event), GFP_KERNEL);
> > +	if (!event)
> > +		goto fail_unlock_irq;
> > +
> > +	event->evt_handle = evt_handle;
> > +	event->irq = irq;
> > +	event->pin = pin;
> > +
> > +	ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
> > +				   "ACPI:Event", event);
> > +	if (ret) {
> > +		dev_err(chip->dev, "Failed to setup interrupt handler for %d\n",
> > +			event->irq);
> > +		goto fail_free_event;
> > +	}
> > +
> > +	list_add_tail(&event->node, &achip->events);
> > +	return AE_OK;
> > +
> > +fail_free_event:
> > +	kfree(event);
> > +fail_unlock_irq:
> > +	gpiod_unlock_as_irq(desc);
> > +fail_free_desc:
> > +	gpiochip_free_own_desc(desc);
> > +
> > +	return AE_ERROR;
> > +}
> > +
> >  /**
> >   * acpi_gpiochip_request_interrupts() - Register isr for gpio chip ACPI events
> >   * @achip:      ACPI GPIO chip
> > @@ -103,104 +217,22 @@ static void acpi_gpio_chip_dh(acpi_handle handle, void *data)
> >   */
> >  static void acpi_gpiochip_request_interrupts(struct acpi_gpio_chip *achip)
> >  {
> > -	struct acpi_buffer buf = {ACPI_ALLOCATE_BUFFER, NULL};
> >  	struct gpio_chip *chip = achip->chip;
> > -	struct acpi_resource *res;
> > -	acpi_handle handle;
> > -	acpi_status status;
> > -	unsigned int pin;
> > -	int irq, ret;
> > -	char ev_name[5];
> >  
> >  	if (!chip->dev || !chip->to_irq)
> >  		return;
> >  
> > -	handle = ACPI_HANDLE(chip->dev);
> > -	if (!handle)
> > -		return;
> > -
> >  	INIT_LIST_HEAD(&achip->events);
> > -
> > -	status = acpi_get_event_resources(handle, &buf);
> > -	if (ACPI_FAILURE(status))
> > -		return;
> > -
> > -	/*
> > -	 * If a GPIO interrupt has an ACPI event handler method, or _EVT is
> > -	 * present, set up an interrupt handler that calls the ACPI event
> > -	 * handler.
> > -	 */
> > -	for (res = buf.pointer;
> > -	     res && (res->type != ACPI_RESOURCE_TYPE_END_TAG);
> > -	     res = ACPI_NEXT_RESOURCE(res)) {
> > -		irq_handler_t handler = NULL;
> > -		void *data;
> > -
> > -		if (res->type != ACPI_RESOURCE_TYPE_GPIO ||
> > -		    res->data.gpio.connection_type !=
> > -		    ACPI_RESOURCE_GPIO_TYPE_INT)
> > -			continue;
> > -
> > -		pin = res->data.gpio.pin_table[0];
> > -		if (pin > chip->ngpio)
> > -			continue;
> > -
> > -		irq = chip->to_irq(chip, pin);
> > -		if (irq < 0)
> > -			continue;
> > -
> > -		if (pin <= 255) {
> > -			acpi_handle ev_handle;
> > -
> > -			sprintf(ev_name, "_%c%02X",
> > -				res->data.gpio.triggering ? 'E' : 'L', pin);
> > -			status = acpi_get_handle(handle, ev_name, &ev_handle);
> > -			if (ACPI_SUCCESS(status)) {
> > -				handler = acpi_gpio_irq_handler;
> > -				data = ev_handle;
> > -			}
> > -		}
> > -		if (!handler) {
> > -			struct acpi_gpio_event *event;
> > -			acpi_handle evt_handle;
> > -
> > -			status = acpi_get_handle(handle, "_EVT", &evt_handle);
> > -			if (ACPI_FAILURE(status))
> > -				continue;
> > -
> > -			event = kzalloc(sizeof(*event), GFP_KERNEL);
> > -			if (!event)
> > -				continue;
> > -
> > -			list_add_tail(&event->node, &achip->events);
> > -			event->evt_handle = evt_handle;
> > -			event->pin = pin;
> > -			event->irq = irq;
> > -			handler = acpi_gpio_irq_handler_evt;
> > -			data = event;
> > -		}
> > -		if (!handler)
> > -			continue;
> > -
> > -		/* Assume BIOS sets the triggering, so no flags */
> > -		ret = devm_request_threaded_irq(chip->dev, irq, NULL, handler,
> > -						0, "GPIO-signaled-ACPI-event",
> > -						data);
> > -		if (ret)
> > -			dev_err(chip->dev,
> > -				"Failed to request IRQ %d ACPI event handler\n",
> > -				irq);
> > -	}
> > +	acpi_walk_resources(ACPI_HANDLE(chip->dev), "_AEI",
> > +			    acpi_gpiochip_request_interrupt, achip);
> >  }
> >  
> >  /**
> > - * acpi_gpiochip_free_interrupts() - Free GPIO _EVT ACPI event interrupts.
> > + * acpi_gpiochip_free_interrupts() - Free GPIO ACPI event interrupts.
> >   * @achip:      ACPI GPIO chip
> >   *
> > - * Free interrupts associated with the _EVT method for the given GPIO chip.
> > - *
> > - * The remaining ACPI event interrupts associated with the chip are freed
> > - * automatically.
> > + * Free interrupts associated with GPIO ACPI event method for the given
> > + * GPIO chip.
> >   */
> >  static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> >  {
> > @@ -211,7 +243,14 @@ static void acpi_gpiochip_free_interrupts(struct acpi_gpio_chip *achip)
> >  		return;
> >  
> >  	list_for_each_entry_safe_reverse(event, ep, &achip->events, node) {
> > -		devm_free_irq(chip->dev, event->irq, event);
> > +		struct gpio_desc *desc;
> > +
> > +		free_irq(event->irq, event);
> > +		desc = gpiochip_get_desc(chip, event->pin);
> > +		if (WARN_ON(IS_ERR(desc)))
> > +			continue;
> > +		gpiod_unlock_as_irq(desc);
> > +		gpiochip_free_own_desc(desc);
> >  		list_del(&event->node);
> >  		kfree(event);
> >  	}
> > diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> > index 489a63524eb6..474f7d1eb7d7 100644
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -1266,11 +1266,12 @@ int gpiochip_remove(struct gpio_chip *chip)
> >  	int		status = 0;
> >  	unsigned	id;
> >  
> > +	acpi_gpiochip_remove(chip);
> > +
> >  	spin_lock_irqsave(&gpio_lock, flags);
> >  
> >  	gpiochip_remove_pin_ranges(chip);
> >  	of_gpiochip_remove(chip);
> > -	acpi_gpiochip_remove(chip);
> >  
> >  	for (id = 0; id < chip->ngpio; id++) {
> >  		if (test_bit(FLAG_REQUESTED, &chip->desc[id].flags)) {
> > 
> 
> Looks good to me.
> 
> Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Thanks!

[PATCH 6/6] 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)
         }
     }
 }

The sensor hub (SHUB) device uses GPIO connection SHD3 to power the device
whenever the GPIO operation region is available.

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 275735f390af..0133d38c447f 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 *achip;
@@ -361,6 +515,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
 	}
 
 	acpi_gpiochip_request_interrupts(achip);
+	acpi_gpiochip_request_regions(achip);
 }
 
 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(achip);
 	acpi_gpiochip_free_interrupts(achip);
 
 	acpi_detach_data(handle, acpi_gpio_chip_dh);
-- 
1.9.0.rc3

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

[Rafael J. Wysocki]

On Monday, February 24, 2014 06:00:11 PM Mika Westerberg 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.
> 
> 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)
>          }
>      }
>  }
> 
> The sensor hub (SHUB) device uses GPIO connection SHD3 to power the device
> whenever the GPIO operation region is available.

I would add more explanation of the ASL above here.  Basically, how it is
supposed to work and what's the handler's role in it.

> 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.

The patch itself looks good to me.

> 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 275735f390af..0133d38c447f 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 *achip;
> @@ -361,6 +515,7 @@ void acpi_gpiochip_add(struct gpio_chip *chip)
>  	}
>  
>  	acpi_gpiochip_request_interrupts(achip);
> +	acpi_gpiochip_request_regions(achip);
>  }
>  
>  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(achip);
>  	acpi_gpiochip_free_interrupts(achip);
>  
>  	acpi_detach_data(handle, acpi_gpio_chip_dh);
> 

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

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

[Mika Westerberg]

On Tue, Feb 25, 2014 at 03:55:02PM +0100, Rafael J. Wysocki wrote:
> On Monday, February 24, 2014 06:00:11 PM Mika Westerberg 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.
> > 
> > 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)
> >          }
> >      }
> >  }
> > 
> > The sensor hub (SHUB) device uses GPIO connection SHD3 to power the device
> > whenever the GPIO operation region is available.
> 
> I would add more explanation of the ASL above here.  Basically, how it is
> supposed to work and what's the handler's role in it.

OK, I will do that in the next revision.

> > 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.
> 
> The patch itself looks good to me.

Thanks for reviewing this :)