Latest gpio gumph

Latest gpio gumph

[Ben Nizette]

Hey David,

Have you got a git/quilt repo somewhere with all the latest gpiolib (and
gpio framework) stuff glued in?  I've seen most of it hit -mm but
developing against -mm on (especially on obscure archs like AVR32 as I
do) is always fraught.

Of course I do build and test -mm on avr32 as regularly as I can (and
submit patches for the inevitable munging it's taken) but I don't really
want to develop against it.

Thx,
	--Ben.

Re: Latest gpio gumph

[David Brownell]

On Tuesday 27 May 2008, Ben Nizette wrote:
> Hey David,
> 
> Have you got a git/quilt repo somewhere with all the latest gpiolib (and
> gpio framework) stuff glued in?  I've seen most of it hit -mm but
> developing against -mm on (especially on obscure archs like AVR32 as I
> do) is always fraught.

All the relevant stuff is now upstream (2.6.26-rc4) except:

  - the userspace sysfs interface (which is in MM, and ISTR
    you were cc'd on that)

  - an avr32 patch, appended (no response on the avr32 list)

Nothing else touches core code; if you're using AVR32, then
you won't care about at91 gpiolib and inlining support, or the
patch sent this AM on LKML (which I've not yet reviewed).

- Dave



====== CUT HERE
From: David Brownell <dbrownell@users.sourceforge.net>
Subject: AVR32: minor GPIO handling updates

 * gpio_direction_output() should disable the pullups just like
   at32_select_gpio(... AT32_GPIOF_OUTPUT) does, for consistency
   between those alternative initialization paths.

 * On the odd chance some code uses a pin as a GPIO IRQ without
   calling gpio_request() or gpio_direction_input(), the debug
   dump should still show its pin status.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
---
 arch/avr32/mach-at32ap/pio.c |    3 +++
 1 files changed, 3 insertions(+)

--- a/arch/avr32/mach-at32ap/pio.c	2008-05-02 12:30:59.000000000 -0700
+++ b/arch/avr32/mach-at32ap/pio.c	2008-05-02 12:41:29.000000000 -0700
@@ -191,6 +191,7 @@ static int direction_output(struct gpio_
 		return -EINVAL;
 
 	gpio_set(chip, offset, value);
+	pio_writel(pio, PUDR, mask);
 	pio_writel(pio, OER, mask);
 	return 0;
 }
@@ -318,6 +319,8 @@ static void pio_bank_show(struct seq_fil
 		const char *label;
 
 		label = gpiochip_is_requested(chip, i);
+		if (!label && (imr & mask))
+			label = "[irq]";
 		if (!label)
 			continue;
 

Re: Latest gpio gumph

[Ben Nizette]

On Tue, 2008-05-27 at 21:44 -0700, David Brownell wrote:
> On Tuesday 27 May 2008, Ben Nizette wrote:
> > Hey David,
> > 
> > Have you got a git/quilt repo somewhere with all the latest gpiolib (and
> > gpio framework) stuff glued in?  I've seen most of it hit -mm but
> > developing against -mm on (especially on obscure archs like AVR32 as I
> > do) is always fraught.
> 
> All the relevant stuff is now upstream (2.6.26-rc4) except:
> 
>   - the userspace sysfs interface (which is in MM, and ISTR
>     you were cc'd on that)

Yup, indeed.

> 
>   - an avr32 patch, appended (no response on the avr32 list)
> 
> Nothing else touches core code; if you're using AVR32, then
> you won't care about at91 gpiolib and inlining support, or the
> patch sent this AM on LKML (which I've not yet reviewed).

Righteo, thanks.

	--Ben.

> 
> - Dave
> 
> 
> 
> ====== CUT HERE
> From: David Brownell <dbrownell@users.sourceforge.net>
> Subject: AVR32: minor GPIO handling updates
> 
>  * gpio_direction_output() should disable the pullups just like
>    at32_select_gpio(... AT32_GPIOF_OUTPUT) does, for consistency
>    between those alternative initialization paths.
> 
>  * On the odd chance some code uses a pin as a GPIO IRQ without
>    calling gpio_request() or gpio_direction_input(), the debug
>    dump should still show its pin status.
> 
> Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
> ---
>  arch/avr32/mach-at32ap/pio.c |    3 +++
>  1 files changed, 3 insertions(+)
> 
> --- a/arch/avr32/mach-at32ap/pio.c	2008-05-02 12:30:59.000000000 -0700
> +++ b/arch/avr32/mach-at32ap/pio.c	2008-05-02 12:41:29.000000000 -0700
> @@ -191,6 +191,7 @@ static int direction_output(struct gpio_
>  		return -EINVAL;
>  
>  	gpio_set(chip, offset, value);
> +	pio_writel(pio, PUDR, mask);
>  	pio_writel(pio, OER, mask);
>  	return 0;
>  }
> @@ -318,6 +319,8 @@ static void pio_bank_show(struct seq_fil
>  		const char *label;
>  
>  		label = gpiochip_is_requested(chip, i);
> +		if (!label && (imr & mask))
> +			label = "[irq]";
>  		if (!label)
>  			continue;
>  

Re: Latest gpio gumph

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> wrote:
>   - an avr32 patch, appended (no response on the avr32 list)

Sorry about the lack of response.

I'm not all that enthusiastic about this patch though...

> From: David Brownell <dbrownell@users.sourceforge.net>
> Subject: AVR32: minor GPIO handling updates
> 
>  * gpio_direction_output() should disable the pullups just like
>    at32_select_gpio(... AT32_GPIOF_OUTPUT) does, for consistency
>    between those alternative initialization paths.

But then we need to keep track of whether pullups used to be enabled so
that we can re-enable it in gpio_direction_input(), don't we?

I can't see the harm of keeping the pullup enabled while the port is
configured as output. For consistency, I'd rather honor the pullup flag
in at32_select_gpio() regardless of AT32_GPIOF_OUTPUT.

>  * On the odd chance some code uses a pin as a GPIO IRQ without
>    calling gpio_request() or gpio_direction_input(), the debug
>    dump should still show its pin status.

Hmm. I guess that makes sense, though I would be lying if I said I care
all that much. I think gpiolib is going pretty far to accommodate buggy
drivers that don't call gpio_request() as it is.

Haavard

Re: Latest gpio gumph

[David Brownell]

On Wednesday 28 May 2008, Haavard Skinnemoen wrote:
> > 
> >  * gpio_direction_output() should disable the pullups just like
> >    at32_select_gpio(... AT32_GPIOF_OUTPUT) does, for consistency
> >    between those alternative initialization paths.
> 
> But then we need to keep track of whether pullups used to be enabled so
> that we can re-enable it in gpio_direction_input(), don't we?

"Need"?  I'd figure that changing direction like that would be
uncommon without something determining signal level (like an
external driver or pullup) ... and if nothing did so, then it'd
be important to use the AVR32-private API with pullup control.


> I can't see the harm of keeping the pullup enabled while the port is
> configured as output. For consistency, I'd rather honor the pullup flag
> in at32_select_gpio() regardless of AT32_GPIOF_OUTPUT.

I guess I don't like the idea of facilitating the constant current
waste that implies if output is being driven low.  Even if it's not
a huge current waste!  (These pullups being a lot weaker than I'd
have expected, at typically 190 kOhm.)  No big deal here I guess.

For an open drain output it's probably less of an issue, unless
there are too many pullups.


> >  * On the odd chance some code uses a pin as a GPIO IRQ without
> >    calling gpio_request() or gpio_direction_input(), the debug
> >    dump should still show its pin status.
> 
> Hmm. I guess that makes sense, though I would be lying if I said I care
> all that much. I think gpiolib is going pretty far to accommodate buggy
> drivers that don't call gpio_request() as it is.

For diagnostic/debug code, I'd say it's reasonably useful to cope
with buglets like that. 

I actually observed that happening.  Setup code was passing the irq
to the driver, and everything worked fine because the reset default
was fine.  I happened to notice that /sys/kernel/debug/gpio output
didn't match up to /proc/interrupts (bug) ... but it would have been
much faster to see the bug if the listing for that pin had a "?" label
showing that it hadn't been requested.

- Dave

Re: Latest gpio gumph

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> wrote:
> On Wednesday 28 May 2008, Haavard Skinnemoen wrote:
> > > 
> > >  * gpio_direction_output() should disable the pullups just like
> > >    at32_select_gpio(... AT32_GPIOF_OUTPUT) does, for consistency
> > >    between those alternative initialization paths.
> > 
> > But then we need to keep track of whether pullups used to be enabled so
> > that we can re-enable it in gpio_direction_input(), don't we?
> 
> "Need"?  I'd figure that changing direction like that would be
> uncommon without something determining signal level (like an
> external driver or pullup) ... and if nothing did so, then it'd
> be important to use the AVR32-private API with pullup control.

If you enable the internal pullup during port configuration, it should
stay that way, I think. But I think at32_select_gpio() should be fixed
so that when the user specifies AT32_GPIOF_OUTPUT | AT32_GPIOF_PULLUP,
the pullup will be turned on.

> > I can't see the harm of keeping the pullup enabled while the port is
> > configured as output. For consistency, I'd rather honor the pullup flag
> > in at32_select_gpio() regardless of AT32_GPIOF_OUTPUT.
> 
> I guess I don't like the idea of facilitating the constant current
> waste that implies if output is being driven low.  Even if it's not
> a huge current waste!  (These pullups being a lot weaker than I'd
> have expected, at typically 190 kOhm.)  No big deal here I guess.

I don't think we're talking about a lot of pins that need to switch
direction on the fly, and the pullup is very weak as you say. And a
floating input might waste a lot more power than the pullup ever will.

> For an open drain output it's probably less of an issue, unless
> there are too many pullups.

The board designer should know this and set the AT32_GPIOF_PULLUP flag
as appropriate.

> > >  * On the odd chance some code uses a pin as a GPIO IRQ without
> > >    calling gpio_request() or gpio_direction_input(), the debug
> > >    dump should still show its pin status.
> > 
> > Hmm. I guess that makes sense, though I would be lying if I said I care
> > all that much. I think gpiolib is going pretty far to accommodate buggy
> > drivers that don't call gpio_request() as it is.
> 
> For diagnostic/debug code, I'd say it's reasonably useful to cope
> with buglets like that. 
> 
> I actually observed that happening.  Setup code was passing the irq
> to the driver, and everything worked fine because the reset default
> was fine.  I happened to notice that /sys/kernel/debug/gpio output
> didn't match up to /proc/interrupts (bug) ... but it would have been
> much faster to see the bug if the listing for that pin had a "?" label
> showing that it hadn't been requested.

Yes, but it will only catch that particular case, not missing
gpio_request() calls in general.

I'm not really opposed to the second change; I would have applied it if
it came separately. But I think the first change is wrong.

Haavard

Re: Latest gpio gumph

[David Brownell]

[ Re that one avr32 patch: ]

On Wednesday 28 May 2008, Haavard Skinnemoen wrote:
> I'm not really opposed to the second change; I would have applied it if
> it came separately. But I think the first change is wrong.

OK, then feel free to strip out the first chunk and just take that.

- Dave

Re: Latest gpio gumph

[Haavard Skinnemoen]

David Brownell <david-b@pacbell.net> wrote:
> [ Re that one avr32 patch: ]
> 
> On Wednesday 28 May 2008, Haavard Skinnemoen wrote:
> > I'm not really opposed to the second change; I would have applied it if
> > it came separately. But I think the first change is wrong.
> 
> OK, then feel free to strip out the first chunk and just take that.

Applied the patch below. Thanks.

Haavard

>From bab0d89419e2d30199420da0af29a8b5662af018 Mon Sep 17 00:00:00 2001
From: David Brownell <dbrownell@users.sourceforge.net>
Date: Tue, 10 Jun 2008 13:55:52 +0200
Subject: [PATCH] avr32: minor GPIO handling updates

On the odd chance some code uses a pin as a GPIO IRQ without calling
gpio_request() or gpio_direction_input(), the debug dump should still
show its pin status.

Signed-off-by: David Brownell <dbrownell@users.sourceforge.net>
Signed-off-by: Haavard Skinnemoen <haavard.skinnemoen@atmel.com>
---
 arch/avr32/mach-at32ap/pio.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/arch/avr32/mach-at32ap/pio.c b/arch/avr32/mach-at32ap/pio.c
index 38a8fa3..60da03b 100644
--- a/arch/avr32/mach-at32ap/pio.c
+++ b/arch/avr32/mach-at32ap/pio.c
@@ -318,6 +318,8 @@ static void pio_bank_show(struct seq_file *s, struct gpio_chip *chip)
 		const char *label;
 
 		label = gpiochip_is_requested(chip, i);
+		if (!label && (imr & mask))
+			label = "[irq]";
 		if (!label)
 			continue;
 
-- 
1.5.5.3