[PATCH 0/2] gpio: acpi: some sanity fixes

[PATCH 0/2] gpio: acpi: some sanity fixes

[Alexandre Courbot]

These two small patches perform some sanity changes in gpiolib-acpi:

1) Stop assuming there is a direct 1:1 mapping between GPIO descriptors
and GPIO numbers. This should not be taken for granted and will likely
change in the future. GPIO descriptors should be handled like regulators
and other allocated resources, i.e. obtained and preserved for later use
instead of constantly requested using gpiochip_get_desc()

2) Change the prototype of gpiochip_request_own_desc() to match other
functions in driver.h. This results is a simplification of the related
code in gpiolib-acpi, which should be a good thing.

Mika, could I have your ack (or even better, tested-by) on these patches?
They change some parts of gpiolib-acpi which I cannot test by myself.
Thanks!

Alexandre Courbot (2):
  gpio: acpi: normalize use of gpiochip_get_desc()
  gpio: change gpiochip_request_own_desc() prototype

 Documentation/gpio/driver.txt |  3 ++-
 drivers/gpio/gpiolib-acpi.c   | 30 +++++++++++-------------------
 drivers/gpio/gpiolib.c        | 18 ++++++++++++++----
 include/linux/gpio/driver.h   |  3 ++-
 4 files changed, 29 insertions(+), 25 deletions(-)

-- 
2.0.4

[PATCH 1/2] gpio: acpi: normalize use of gpiochip_get_desc()

[Alexandre Courbot]

GPIO descriptors are changing from unique and permanent tokens to
allocated resources. Therefore gpiochip_get_desc() cannot be used as a
way to obtain a global GPIO descriptor anymore.

This patch updates the gpiolib ACPI support code to keep and use the
descriptor returned by a centralized call to gpiochip_get_desc().

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 drivers/gpio/gpiolib-acpi.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index d62eaaa75397..9596a1023579 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -25,10 +25,12 @@ struct acpi_gpio_event {
 	acpi_handle handle;
 	unsigned int pin;
 	unsigned int irq;
+	gpio_desc *desc;
 };
 
 struct acpi_gpio_connection {
 	struct list_head node;
+	unsigned int pin;
 	struct gpio_desc *desc;
 };
 
@@ -197,6 +199,7 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 	event->handle = evt_handle;
 	event->irq = irq;
 	event->pin = pin;
+	event->desc = desc;
 
 	ret = request_threaded_irq(event->irq, NULL, handler, irqflags,
 				   "ACPI:Event", event);
@@ -280,7 +283,7 @@ void acpi_gpiochip_free_interrupts(struct gpio_chip *chip)
 		struct gpio_desc *desc;
 
 		free_irq(event->irq, event);
-		desc = gpiochip_get_desc(chip, event->pin);
+		desc = event->desc;
 		if (WARN_ON(IS_ERR(desc)))
 			continue;
 		gpio_unlock_as_irq(chip, event->pin);
@@ -406,24 +409,26 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 		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) {
+			if (conn->pin == pin) {
 				found = true;
+				desc = conn->desc;
 				break;
 			}
 		}
 		if (!found) {
 			int ret;
 
+			desc = gpiochip_get_desc(chip, pin);
+			if (IS_ERR(desc)) {
+				status = AE_ERROR;
+				mutex_unlock(&achip->conn_lock);
+				goto out;
+			}
+
 			ret = gpiochip_request_own_desc(desc, "ACPI:OpRegion");
 			if (ret) {
 				status = AE_ERROR;
@@ -462,6 +467,7 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 				goto out;
 			}
 
+			conn->pin = pin;
 			conn->desc = desc;
 			list_add_tail(&conn->node, &achip->conns);
 		}
-- 
2.0.4

[PATCH 2/2] gpio: change gpiochip_request_own_desc() prototype

[Alexandre Courbot]

The current prototype of gpiochip_request_own_desc() requires to obtain
a pointer to a descriptor. This is in contradiction to all other GPIO
request schemes, and imposes an extra step of obtaining a descriptor to
drivers. Most drivers actually cannot even perform that step since the
function that does it (gpichip_get_desc()) is gpiolib-private.

Change gpiochip_request_own_desc() to return a descriptor from a
(chip, hwnum) tuple and update users of this function (currently
gpiolib-acpi only).

Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
---
 Documentation/gpio/driver.txt |  3 ++-
 drivers/gpio/gpiolib-acpi.c   | 20 +++-----------------
 drivers/gpio/gpiolib.c        | 18 ++++++++++++++----
 include/linux/gpio/driver.h   |  3 ++-
 4 files changed, 21 insertions(+), 23 deletions(-)

diff --git a/Documentation/gpio/driver.txt b/Documentation/gpio/driver.txt
index 18790c237977..23b751a10d7b 100644
--- a/Documentation/gpio/driver.txt
+++ b/Documentation/gpio/driver.txt
@@ -178,7 +178,8 @@ does not help since it pins the module to the kernel forever (it calls
 try_module_get()). A GPIO driver can use the following functions instead
 to request and free descriptors without being pinned to the kernel forever.
 
-	int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label)
+	struct gpio_desc *gpiochip_request_own_desc(struct gpio_desc *desc,
+						    const char *label)
 
 	void gpiochip_free_own_desc(struct gpio_desc *desc)
 
diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
index 9596a1023579..4edfb7a00e0a 100644
--- a/drivers/gpio/gpiolib-acpi.c
+++ b/drivers/gpio/gpiolib-acpi.c
@@ -145,14 +145,8 @@ static acpi_status acpi_gpiochip_request_interrupt(struct acpi_resource *ares,
 	if (!handler)
 		return AE_BAD_PARAMETER;
 
-	desc = gpiochip_get_desc(chip, pin);
+	desc = gpiochip_request_own_desc(chip, pin, "ACPI:Event");
 	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;
 	}
@@ -420,22 +414,14 @@ acpi_gpio_adr_space_handler(u32 function, acpi_physical_address address,
 			}
 		}
 		if (!found) {
-			int ret;
-
-			desc = gpiochip_get_desc(chip, pin);
+			desc = gpiochip_request_own_desc(chip, pin,
+							 "ACPI:OpRegion");
 			if (IS_ERR(desc)) {
 				status = AE_ERROR;
 				mutex_unlock(&achip->conn_lock);
 				goto out;
 			}
 
-			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);
diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 15cc0bb65dda..a5831d6a9b91 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -895,12 +895,22 @@ EXPORT_SYMBOL_GPL(gpiochip_is_requested);
  * 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)
+struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
+					    const char *label)
 {
-	if (!desc || !desc->chip)
-		return -EINVAL;
+	struct gpio_desc *desc = gpiochip_get_desc(chip, hwnum);
+	int err;
+
+	if (IS_ERR(desc)) {
+		chip_err(chip, "failed to get GPIO descriptor\n");
+		return desc;
+	}
+
+	err = __gpiod_request(desc, label);
+	if (err < 0)
+		return ERR_PTR(err);
 
-	return __gpiod_request(desc, label);
+	return desc;
 }
 EXPORT_SYMBOL_GPL(gpiochip_request_own_desc);
 
diff --git a/include/linux/gpio/driver.h b/include/linux/gpio/driver.h
index e78a2373e374..a2de58fffd19 100644
--- a/include/linux/gpio/driver.h
+++ b/include/linux/gpio/driver.h
@@ -166,7 +166,8 @@ int gpiochip_irqchip_add(struct gpio_chip *gpiochip,
 
 #endif /* CONFIG_GPIO_IRQCHIP */
 
-int gpiochip_request_own_desc(struct gpio_desc *desc, const char *label);
+struct gpio_desc *gpiochip_request_own_desc(struct gpio_chip *chip, u16 hwnum,
+					    const char *label);
 void gpiochip_free_own_desc(struct gpio_desc *desc);
 
 #else /* CONFIG_GPIOLIB */
-- 
2.0.4

Re: [PATCH 1/2] gpio: acpi: normalize use of gpiochip_get_desc()

[Mika Westerberg]

On Mon, Aug 18, 2014 at 09:50:56AM -0700, Alexandre Courbot wrote:
> GPIO descriptors are changing from unique and permanent tokens to
> allocated resources. Therefore gpiochip_get_desc() cannot be used as a
> way to obtain a global GPIO descriptor anymore.
> 
> This patch updates the gpiolib ACPI support code to keep and use the
> descriptor returned by a centralized call to gpiochip_get_desc().
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
> ---
>  drivers/gpio/gpiolib-acpi.c | 22 ++++++++++++++--------
>  1 file changed, 14 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
> index d62eaaa75397..9596a1023579 100644
> --- a/drivers/gpio/gpiolib-acpi.c
> +++ b/drivers/gpio/gpiolib-acpi.c
> @@ -25,10 +25,12 @@ struct acpi_gpio_event {
>  	acpi_handle handle;
>  	unsigned int pin;
>  	unsigned int irq;
> +	gpio_desc *desc;

This needs to be 
	struct gpio_desc *desc;

>  };

Tested on Asus T100TA and nothing seems to break, so:

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 2/2] gpio: change gpiochip_request_own_desc() prototype

[Mika Westerberg]

On Mon, Aug 18, 2014 at 09:50:57AM -0700, Alexandre Courbot wrote:
> The current prototype of gpiochip_request_own_desc() requires to obtain
> a pointer to a descriptor. This is in contradiction to all other GPIO
> request schemes, and imposes an extra step of obtaining a descriptor to
> drivers. Most drivers actually cannot even perform that step since the
> function that does it (gpichip_get_desc()) is gpiolib-private.
> 
> Change gpiochip_request_own_desc() to return a descriptor from a
> (chip, hwnum) tuple and update users of this function (currently
> gpiolib-acpi only).
> 
> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>

Tested on Asus T100TA and everything still seems to work so,

Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Re: [PATCH 1/2] gpio: acpi: normalize use of gpiochip_get_desc()

[Alexandre Courbot]

On Tue, Aug 19, 2014 at 2:32 AM, Mika Westerberg
<mika.westerberg@linux.intel.com> wrote:
> On Mon, Aug 18, 2014 at 09:50:56AM -0700, Alexandre Courbot wrote:
>> GPIO descriptors are changing from unique and permanent tokens to
>> allocated resources. Therefore gpiochip_get_desc() cannot be used as a
>> way to obtain a global GPIO descriptor anymore.
>>
>> This patch updates the gpiolib ACPI support code to keep and use the
>> descriptor returned by a centralized call to gpiochip_get_desc().
>>
>> Signed-off-by: Alexandre Courbot <acourbot@nvidia.com>
>> ---
>>  drivers/gpio/gpiolib-acpi.c | 22 ++++++++++++++--------
>>  1 file changed, 14 insertions(+), 8 deletions(-)
>>
>> diff --git a/drivers/gpio/gpiolib-acpi.c b/drivers/gpio/gpiolib-acpi.c
>> index d62eaaa75397..9596a1023579 100644
>> --- a/drivers/gpio/gpiolib-acpi.c
>> +++ b/drivers/gpio/gpiolib-acpi.c
>> @@ -25,10 +25,12 @@ struct acpi_gpio_event {
>>       acpi_handle handle;
>>       unsigned int pin;
>>       unsigned int irq;
>> +     gpio_desc *desc;
>
> This needs to be
>         struct gpio_desc *desc;
>
>>  };

Oops apologies for this, I should have least have compiled a tree with
this patch on... >_<;

>
> Tested on Asus T100TA and nothing seems to break, so:
>
> Tested-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks! Will send a v2 for Linus to merge, if he agrees with these two patches.
Alex.