[PATCH 1/2] gpio-ich: Check for pin availability at request time

[PATCH 1/2] gpio-ich: Check for pin availability at request time

[Jean Delvare]

Stop checking for pin availability in get functions. These functions
can be called repeatedly, so checking every time is bad for
performance.

Instead, check at pin request time. This only happens once, and we can
let the caller know of the unavailability much earlier.

Signed-off-by: Jean Delvare <khali@linux-fr.org>
Cc: Peter Tyser <ptyser@xes-inc.com>
Cc: Grant Likely <grant.likely@secretlab.ca>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/gpio/gpio-ich.c |    9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

--- linux-3.7.orig/drivers/gpio/gpio-ich.c	2013-03-05 08:36:40.940199905 +0100
+++ linux-3.7/drivers/gpio/gpio-ich.c	2013-03-05 08:36:51.047437884 +0100
@@ -169,9 +169,6 @@ static int ichx_gpio_direction_output(st
 
 static int ichx_gpio_get(struct gpio_chip *chip, unsigned nr)
 {
-	if (!ichx_gpio_check_available(chip, nr))
-		return -ENXIO;
-
 	return ichx_read_bit(GPIO_LVL, nr);
 }
 
@@ -180,9 +177,6 @@ static int ich6_gpio_get(struct gpio_chi
 	unsigned long flags;
 	u32 data;
 
-	if (!ichx_gpio_check_available(chip, nr))
-		return -ENXIO;
-
 	/*
 	 * GPI 0 - 15 need to be read from the power management registers on
 	 * a ICH6/3100 bridge.
@@ -207,6 +201,9 @@ static int ich6_gpio_get(struct gpio_chi
 
 static int ichx_gpio_request(struct gpio_chip *chip, unsigned nr)
 {
+	if (!ichx_gpio_check_available(chip, nr))
+		return -ENXIO;
+
 	/*
 	 * Note we assume the BIOS properly set a bridge's USE value.  Some
 	 * chips (eg Intel 3100) have bogus USE values though, so first see if


-- 
Jean Delvare

Re: [PATCH 1/2] gpio-ich: Check for pin availability at request time

[Peter Tyser]

Hi Jean,

On Tue, 2013-03-05 at 09:06 +0100, Jean Delvare wrote:
> Stop checking for pin availability in get functions. These functions
> can be called repeatedly, so checking every time is bad for
> performance.
> 
> Instead, check at pin request time. This only happens once, and we can
> let the caller know of the unavailability much earlier.

Is there a reason to not remove the ichx_gpio_check_available() calls
from the ichx_gpio_direction_output() and ichx_gpio_direction_input()
functions in this change?

I had originally assumed these checks were to support the "legacy"
interface where users could use the GPIOs without first requesting the
GPIO, but after looking a bit further it looks like the gpiolib code to
set a GPIO as an input or output does auto-request the GPIO before
setting its direction.  So I'd assume your 1 check in 
ichx_gpio_request() would cover the "legacy" interface too.

Best,
Peter

Re: [PATCH 1/2] gpio-ich: Check for pin availability at request time

[Jean Delvare]

Hi Peter,

On Tue, 05 Mar 2013 12:16:54 -0600, Peter Tyser wrote:
> On Tue, 2013-03-05 at 09:06 +0100, Jean Delvare wrote:
> > Stop checking for pin availability in get functions. These functions
> > can be called repeatedly, so checking every time is bad for
> > performance.
> > 
> > Instead, check at pin request time. This only happens once, and we can
> > let the caller know of the unavailability much earlier.
> 
> Is there a reason to not remove the ichx_gpio_check_available() calls
> from the ichx_gpio_direction_output() and ichx_gpio_direction_input()
> functions in this change?

No valid reason other than me being a chicken ;)

I seemed to remember that requesting GPIOs was optional so I didn't
want to remove too many checks. However...

> I had originally assumed these checks were to support the "legacy"
> interface where users could use the GPIOs without first requesting the
> GPIO, but after looking a bit further it looks like the gpiolib code to
> set a GPIO as an input or output does auto-request the GPIO before
> setting its direction.  So I'd assume your 1 check in 
> ichx_gpio_request() would cover the "legacy" interface too.

... you are implying that this memory of mine refers to ancient times
and this is no longer true. Apparently the change happened in April
2009, not sure how I managed to miss that for so long.

Given that, I completely agree that the check can go away from
ichx_gpio_direction_output() and ichx_gpio_direction_input() as well.
The performance gain won't be as important as for ichx_gpio_get*() but
it's free so let's take it.

Thanks a lot for the review, I'll send v2 of this patch.

-- 
Jean Delvare