[patch 2.6.27-rc7] gpiolib: request/free hooks

[patch 2.6.27-rc7] gpiolib: request/free hooks

[David Brownell]

From: David Brownell <dbrownell@users.sourceforge.net>

Add a new internal mechanism to gpiolib to support low power
operations by letting gpio_chip instances see when their GPIOs
are in use.  When no GPIOs are active, chips may be able to 
enter lower powered runtime states by disabling clocks and/or
power domains.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 Documentation/gpio.txt     |    4 ++++
 drivers/gpio/gpiolib.c     |   42 ++++++++++++++++++++++++++++++------------
 include/asm-generic/gpio.h |    9 +++++++++
 3 files changed, 43 insertions(+), 12 deletions(-)

--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -240,6 +240,10 @@ signal, or (b) something wrongly believe
 needed to manage a signal that's in active use.  That is, requesting a
 GPIO can serve as a kind of lock.
 
+Some platforms may also use knowledge about what GPIOs are active for
+power management, such as by powering down unused chip sectors and, more
+easily, gating off unused clocks.
+
 These two calls are optional because not not all current Linux platforms
 offer such functionality in their GPIO support; a valid implementation
 could return success for all gpio_request() calls.  Unlike the other calls,
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -69,14 +69,18 @@ static inline void desc_set_label(struct
  * those calls have no teeth) we can't avoid autorequesting.  This nag
  * message should motivate switching to explicit requests...
  */
-static void gpio_ensure_requested(struct gpio_desc *desc)
+static void gpio_ensure_requested(struct gpio_desc *desc, unsigned offset)
 {
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
-		pr_warning("GPIO-%d autorequested\n", (int)(desc - gpio_desc));
+		struct gpio_chip *chip = desc->chip;
+		int gpio = chip->base + offset;
+
+		pr_warning("GPIO-%d autorequested\n", gpio);
 		desc_set_label(desc, "[auto]");
-		if (!try_module_get(desc->chip->owner))
-			pr_err("GPIO-%d: module can't be gotten \n",
-					(int)(desc - gpio_desc));
+		if (!try_module_get(chip->owner))
+			pr_err("GPIO-%d: module can't be gotten \n", gpio);
+		if (chip->request)
+			chip->request(chip, offset);
 	}
 }
 
@@ -781,6 +785,7 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
 int gpio_request(unsigned gpio, const char *label)
 {
 	struct gpio_desc	*desc;
+	struct gpio_chip	*chip;
 	int			status = -EINVAL;
 	unsigned long		flags;
 
@@ -789,22 +794,31 @@ int gpio_request(unsigned gpio, const ch
 	if (!gpio_is_valid(gpio))
 		goto done;
 	desc = &gpio_desc[gpio];
-	if (desc->chip == NULL)
+	chip = desc->chip;
+	if (chip == NULL)
 		goto done;
 
-	if (!try_module_get(desc->chip->owner))
+	if (!try_module_get(chip->owner))
 		goto done;
 
 	/* NOTE:  gpio_request() can be called in early boot,
-	 * before IRQs are enabled.
+	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
+	if (chip->request) {
+		status = chip->request(chip, gpio - chip->base);
+		if (status < 0) {
+			module_put(chip->owner);
+			goto done;
+		}
+	}
+
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
 		desc_set_label(desc, label ? : "?");
 		status = 0;
 	} else {
 		status = -EBUSY;
-		module_put(desc->chip->owner);
+		module_put(chip->owner);
 	}
 
 done:
@@ -820,6 +834,7 @@ void gpio_free(unsigned gpio)
 {
 	unsigned long		flags;
 	struct gpio_desc	*desc;
+	struct gpio_chip	*chip;
 
 	if (!gpio_is_valid(gpio)) {
 		WARN_ON(extra_checks);
@@ -831,8 +846,11 @@ void gpio_free(unsigned gpio)
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	desc = &gpio_desc[gpio];
-	if (desc->chip && test_and_clear_bit(FLAG_REQUESTED, &desc->flags)) {
+	chip = desc->chip;
+	if (chip && test_and_clear_bit(FLAG_REQUESTED, &desc->flags)) {
 		desc_set_label(desc, NULL);
+		if (chip->free)
+			chip->free(chip, gpio - chip->base);
 		module_put(desc->chip->owner);
 	} else
 		WARN_ON(extra_checks);
@@ -898,7 +916,7 @@ int gpio_direction_input(unsigned gpio)
 	gpio -= chip->base;
 	if (gpio >= chip->ngpio)
 		goto fail;
-	gpio_ensure_requested(desc);
+	gpio_ensure_requested(desc, gpio);
 
 	/* now we know the gpio is valid and chip won't vanish */
 
@@ -936,7 +954,7 @@ int gpio_direction_output(unsigned gpio,
 	gpio -= chip->base;
 	if (gpio >= chip->ngpio)
 		goto fail;
-	gpio_ensure_requested(desc);
+	gpio_ensure_requested(desc, gpio);
 
 	/* now we know the gpio is valid and chip won't vanish */
 
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -35,6 +35,10 @@ struct module;
  * @label: for diagnostics
  * @dev: optional device providing the GPIOs
  * @owner: helps prevent removal of modules exporting active GPIOs
+ * @request: optional hook for chip-specific activation, such as
+ *	enabling module power and clock; may sleep
+ * @free: optional hook for chip-specific deactivation, such as
+ *	disabling module power and clock; may sleep
  * @direction_input: configures signal "offset" as input, or returns error
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
@@ -67,6 +71,11 @@ struct gpio_chip {
 	struct device		*dev;
 	struct module		*owner;
 
+	int			(*request)(struct gpio_chip *chip,
+						unsigned offset);
+	void			(*free)(struct gpio_chip *chip,
+						unsigned offset);
+
 	int			(*direction_input)(struct gpio_chip *chip,
 						unsigned offset);
 	int			(*get)(struct gpio_chip *chip,

Re: [patch 2.6.27-rc7] gpiolib: request/free hooks

[Magnus Damm]

Hi David,

On Thu, Sep 25, 2008 at 7:08 AM, David Brownell <david-b@pacbell.net> wrote:
> From: David Brownell <dbrownell@users.sourceforge.net>
>
> Add a new internal mechanism to gpiolib to support low power
> operations by letting gpio_chip instances see when their GPIOs
> are in use.  When no GPIOs are active, chips may be able to
> enter lower powered runtime states by disabling clocks and/or
> power domains.
>
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>

Looking good. I'm currently hacking on some pinmuxed gpio code for
SuperH, and I'd like to use these request/free callbacks to select
proper pinmux state.

I have one comment below though:

> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -781,6 +785,7 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
>  int gpio_request(unsigned gpio, const char *label)
>  {
>        struct gpio_desc        *desc;
> +       struct gpio_chip        *chip;
>        int                     status = -EINVAL;
>        unsigned long           flags;
>
> @@ -789,22 +794,31 @@ int gpio_request(unsigned gpio, const ch
>        if (!gpio_is_valid(gpio))
>                goto done;
>        desc = &gpio_desc[gpio];
> -       if (desc->chip == NULL)
> +       chip = desc->chip;
> +       if (chip == NULL)
>                goto done;
>
> -       if (!try_module_get(desc->chip->owner))
> +       if (!try_module_get(chip->owner))
>                goto done;
>
>        /* NOTE:  gpio_request() can be called in early boot,
> -        * before IRQs are enabled.
> +        * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
>         */
>
> +       if (chip->request) {
> +               status = chip->request(chip, gpio - chip->base);
> +               if (status < 0) {
> +                       module_put(chip->owner);
> +                       goto done;
> +               }
> +       }
> +
>        if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
>                desc_set_label(desc, label ? : "?");
>                status = 0;
>        } else {
>                status = -EBUSY;
> -               module_put(desc->chip->owner);
> +               module_put(chip->owner);
>        }
>
>  done:

The code above doesn't catch double gpio_request() user calls
properly. Or rather, the user will receive an error but the
chip->request() callback may get called twice.

What about modifying the gpiolib code to handle that? I think that
sounds like a better idea than cover ing that case in the chip code...

Thanks!

/ magnus

Re: [patch 2.6.27-rc7] gpiolib: request/free hooks

[David Brownell]

On Saturday 27 September 2008, Magnus Damm wrote:
> 
> Looking good. I'm currently hacking on some pinmuxed gpio code for
> SuperH, and I'd like to use these request/free callbacks to select
> proper pinmux state.

I'm not so keen on that particular overloading, but I can
understand why it might be wanted on systems which have a
happy one-to-one mapping between GPIOs and pins.

If you do that, be ready to provide a pinmux-only interface
at some point when this "piggybacking" breaks something...

 
> I have one comment below though:
> 
> > --- a/drivers/gpio/gpiolib.c
> > +++ b/drivers/gpio/gpiolib.c
> > @@ -781,6 +785,7 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
> >  int gpio_request(unsigned gpio, const char *label)
> >  	...
> 
> The code above doesn't catch double gpio_request() user calls
> properly. Or rather, the user will receive an error but the
> chip->request() callback may get called twice.
> 
> What about modifying the gpiolib code to handle that? I think that
> sounds like a better idea than cover ing that case in the chip code...

Yeah.  Better to test-and-set the flag and then request, and backout
if it fails, than the other way.  Just who wrote that crap in the
first place?  Sigh.  (Notice it's done that way already in the code
path handling implicit requesting ... )

I'll send an updated patch along soonish.

- Dave

Re: [patch 2.6.27-rc7] gpiolib: request/free hooks

[Magnus Damm]

On Sun, Sep 28, 2008 at 3:29 AM, David Brownell <david-b@pacbell.net> wrote:
> On Saturday 27 September 2008, Magnus Damm wrote:
>>
>> Looking good. I'm currently hacking on some pinmuxed gpio code for
>> SuperH, and I'd like to use these request/free callbacks to select
>> proper pinmux state.
>
> I'm not so keen on that particular overloading, but I can
> understand why it might be wanted on systems which have a
> happy one-to-one mapping between GPIOs and pins.

Hm, sounds like you prefer to keep pinmuxing and GPIO code separated?

> If you do that, be ready to provide a pinmux-only interface
> at some point when this "piggybacking" breaks something...

In the SuperH case GPIO pin direction selection is done in the same
way as selecting pin function. So in and out directions can be seen as
two pin functions. And since we want to support GPIO pin direction we
may as well support setting pin functions as well.

But maybe there are better ways to integrate it, I'm not sure.

>> > --- a/drivers/gpio/gpiolib.c
>> > +++ b/drivers/gpio/gpiolib.c
>> > @@ -781,6 +785,7 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
>> >  int gpio_request(unsigned gpio, const char *label)
>> >     ...
>>
>> The code above doesn't catch double gpio_request() user calls
>> properly. Or rather, the user will receive an error but the
>> chip->request() callback may get called twice.
>>
>> What about modifying the gpiolib code to handle that? I think that
>> sounds like a better idea than cover ing that case in the chip code...
>
> Yeah.  Better to test-and-set the flag and then request, and backout
> if it fails, than the other way.  Just who wrote that crap in the
> first place?  Sigh.  (Notice it's done that way already in the code
> path handling implicit requesting ... )

Sounds good.

> I'll send an updated patch along soonish.

Thank you!

/ magnus

Re: [patch 2.6.27-rc7] gpiolib: request/free hooks

[David Brownell]

On Sunday 28 September 2008, Magnus Damm wrote:
> Hm, sounds like you prefer to keep pinmuxing and GPIO code separated?

Yep.

> > If you do that, be ready to provide a pinmux-only interface
> > at some point when this "piggybacking" breaks something...
> 
> In the SuperH case GPIO pin direction selection is done in the same
> way as selecting pin function. So in and out directions can be seen as
> two pin functions. And since we want to support GPIO pin direction we
> may as well support setting pin functions as well.
> 
> But maybe there are better ways to integrate it, I'm not sure.

Just don't expect the GPIO framework to address the problem of
how to configure one of those pins for a non-GPIO function.

Such pinmux problems deserve different programming interfaces.

\
> > Yeah.  Better to test-and-set the flag and then request, and backout
> > if it fails, than the other way.  Just who wrote that crap in the
> > first place?  Sigh.  (Notice it's done that way already in the code
> > path handling implicit requesting ... )
> 
> Sounds good.
> 
> > I'll send an updated patch along soonish.
> 
> Thank you!

... after I come up with a happy fix for the locking goofs;
those new methods may not be called under spinlock protection.

- Dave

Re: [patch 2.6.27-rc7] gpiolib: request/free hooks

[David Brownell]

> > > I'll send an updated patch along soonish.

This updated version seems to behave for me; against RC8.
It resolves the locking issues I noticed, as well as the
record-keeping one you commented on.  Comments?

- Dave

==================
From: David Brownell <dbrownell@users.sourceforge.net>

Add a new internal mechanism to gpiolib to support low power
operations by letting gpio_chip instances see when their GPIOs
are in use.  When no GPIOs are active, chips may be able to 
enter lower powered runtime states by disabling clocks and/or
power domains.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 Documentation/gpio.txt     |    4 +
 drivers/gpio/gpiolib.c     |   91 ++++++++++++++++++++++++++++++++++++-------
 include/asm-generic/gpio.h |    9 ++++
 3 files changed, 91 insertions(+), 13 deletions(-)

--- a/Documentation/gpio.txt
+++ b/Documentation/gpio.txt
@@ -240,6 +240,10 @@ signal, or (b) something wrongly believe
 needed to manage a signal that's in active use.  That is, requesting a
 GPIO can serve as a kind of lock.
 
+Some platforms may also use knowledge about what GPIOs are active for
+power management, such as by powering down unused chip sectors and, more
+easily, gating off unused clocks.
+
 These two calls are optional because not not all current Linux platforms
 offer such functionality in their GPIO support; a valid implementation
 could return success for all gpio_request() calls.  Unlike the other calls,
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -67,17 +67,28 @@ static inline void desc_set_label(struct
  * when setting direction, and otherwise illegal.  Until board setup code
  * and drivers use explicit requests everywhere (which won't happen when
  * those calls have no teeth) we can't avoid autorequesting.  This nag
- * message should motivate switching to explicit requests...
+ * message should motivate switching to explicit requests... so should
+ * the weaker cleanup after faults, compared to gpio_request().
  */
-static void gpio_ensure_requested(struct gpio_desc *desc)
+static int gpio_ensure_requested(struct gpio_desc *desc, unsigned offset)
 {
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
-		pr_warning("GPIO-%d autorequested\n", (int)(desc - gpio_desc));
+		struct gpio_chip *chip = desc->chip;
+		int gpio = chip->base + offset;
+
+		if (!try_module_get(chip->owner)) {
+			pr_err("GPIO-%d: module can't be gotten \n", gpio);
+			clear_bit(FLAG_REQUESTED, &desc->flags);
+			/* lose */
+			return -EIO;
+		}
+		pr_warning("GPIO-%d autorequested\n", gpio);
 		desc_set_label(desc, "[auto]");
-		if (!try_module_get(desc->chip->owner))
-			pr_err("GPIO-%d: module can't be gotten \n",
-					(int)(desc - gpio_desc));
+		/* caller must chip->request() w/o spinlock */
+		if (chip->request)
+			return 1;
 	}
+	return 0;
 }
 
 /* caller holds gpio_lock *OR* gpio is marked as requested */
@@ -752,6 +763,7 @@ EXPORT_SYMBOL_GPL(gpiochip_remove);
 int gpio_request(unsigned gpio, const char *label)
 {
 	struct gpio_desc	*desc;
+	struct gpio_chip	*chip;
 	int			status = -EINVAL;
 	unsigned long		flags;
 
@@ -760,14 +772,15 @@ int gpio_request(unsigned gpio, const ch
 	if (!gpio_is_valid(gpio))
 		goto done;
 	desc = &gpio_desc[gpio];
-	if (desc->chip == NULL)
+	chip = desc->chip;
+	if (chip == NULL)
 		goto done;
 
-	if (!try_module_get(desc->chip->owner))
+	if (!try_module_get(chip->owner))
 		goto done;
 
 	/* NOTE:  gpio_request() can be called in early boot,
-	 * before IRQs are enabled.
+	 * before IRQs are enabled, for non-sleeping (SOC) GPIOs.
 	 */
 
 	if (test_and_set_bit(FLAG_REQUESTED, &desc->flags) == 0) {
@@ -775,7 +788,20 @@ int gpio_request(unsigned gpio, const ch
 		status = 0;
 	} else {
 		status = -EBUSY;
-		module_put(desc->chip->owner);
+		module_put(chip->owner);
+	}
+
+	if (chip->request) {
+		/* chip->request may sleep */
+		spin_unlock_irqrestore(&gpio_lock, flags);
+		status = chip->request(chip, gpio - chip->base);
+		spin_lock_irqsave(&gpio_lock, flags);
+
+		if (status < 0) {
+			desc_set_label(desc, NULL);
+			module_put(chip->owner);
+			clear_bit(FLAG_REQUESTED, &desc->flags);
+		}
 	}
 
 done:
@@ -791,6 +817,7 @@ void gpio_free(unsigned gpio)
 {
 	unsigned long		flags;
 	struct gpio_desc	*desc;
+	struct gpio_chip	*chip;
 
 	if (!gpio_is_valid(gpio)) {
 		WARN_ON(extra_checks);
@@ -802,9 +829,17 @@ void gpio_free(unsigned gpio)
 	spin_lock_irqsave(&gpio_lock, flags);
 
 	desc = &gpio_desc[gpio];
-	if (desc->chip && test_and_clear_bit(FLAG_REQUESTED, &desc->flags)) {
+	chip = desc->chip;
+	if (chip && test_bit(FLAG_REQUESTED, &desc->flags)) {
+		if (chip->free) {
+			spin_unlock_irqrestore(&gpio_lock, flags);
+			might_sleep_if(extra_checks && chip->can_sleep);
+			chip->free(chip, gpio - chip->base);
+			spin_lock_irqsave(&gpio_lock, flags);
+		}
 		desc_set_label(desc, NULL);
 		module_put(desc->chip->owner);
+		clear_bit(FLAG_REQUESTED, &desc->flags);
 	} else
 		WARN_ON(extra_checks);
 
@@ -869,7 +904,9 @@ int gpio_direction_input(unsigned gpio)
 	gpio -= chip->base;
 	if (gpio >= chip->ngpio)
 		goto fail;
-	gpio_ensure_requested(desc);
+	status = gpio_ensure_requested(desc, gpio);
+	if (status < 0)
+		goto fail;
 
 	/* now we know the gpio is valid and chip won't vanish */
 
@@ -877,9 +914,22 @@ int gpio_direction_input(unsigned gpio)
 
 	might_sleep_if(extra_checks && chip->can_sleep);
 
+	if (status) {
+		status = chip->request(chip, gpio);
+		if (status < 0) {
+			pr_debug("GPIO-%d: chip request fail, %d\n",
+				chip->base + gpio, status);
+			/* and it's not available to anyone else ...
+			 * gpio_request() is the fully clean solution.
+			 */
+			goto lose;
+		}
+	}
+
 	status = chip->direction_input(chip, gpio);
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
+lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
@@ -907,7 +957,9 @@ int gpio_direction_output(unsigned gpio,
 	gpio -= chip->base;
 	if (gpio >= chip->ngpio)
 		goto fail;
-	gpio_ensure_requested(desc);
+	status = gpio_ensure_requested(desc, gpio);
+	if (status < 0)
+		goto fail;
 
 	/* now we know the gpio is valid and chip won't vanish */
 
@@ -915,9 +967,22 @@ int gpio_direction_output(unsigned gpio,
 
 	might_sleep_if(extra_checks && chip->can_sleep);
 
+	if (status) {
+		status = chip->request(chip, gpio);
+		if (status < 0) {
+			pr_debug("GPIO-%d: chip request fail, %d\n",
+				chip->base + gpio, status);
+			/* and it's not available to anyone else ...
+			 * gpio_request() is the fully clean solution.
+			 */
+			goto lose;
+		}
+	}
+
 	status = chip->direction_output(chip, gpio, value);
 	if (status == 0)
 		set_bit(FLAG_IS_OUT, &desc->flags);
+lose:
 	return status;
 fail:
 	spin_unlock_irqrestore(&gpio_lock, flags);
--- a/include/asm-generic/gpio.h
+++ b/include/asm-generic/gpio.h
@@ -35,6 +35,10 @@ struct module;
  * @label: for diagnostics
  * @dev: optional device providing the GPIOs
  * @owner: helps prevent removal of modules exporting active GPIOs
+ * @request: optional hook for chip-specific activation, such as
+ *	enabling module power and clock; may sleep
+ * @free: optional hook for chip-specific deactivation, such as
+ *	disabling module power and clock; may sleep
  * @direction_input: configures signal "offset" as input, or returns error
  * @get: returns value for signal "offset"; for output signals this
  *	returns either the value actually sensed, or zero
@@ -67,6 +71,11 @@ struct gpio_chip {
 	struct device		*dev;
 	struct module		*owner;
 
+	int			(*request)(struct gpio_chip *chip,
+						unsigned offset);
+	void			(*free)(struct gpio_chip *chip,
+						unsigned offset);
+
 	int			(*direction_input)(struct gpio_chip *chip,
 						unsigned offset);
 	int			(*get)(struct gpio_chip *chip,