[PATCH] bcm47xx: Fix GPIO API return codes

[PATCH] bcm47xx: Fix GPIO API return codes

[Michael Buesch]

The GPIO API is supposed to return 0 or a negative error code,
but the SSB GPIO functions return the bitmask of the GPIO register.
Fix this by ignoring the bitmask and always returning 0. The SSB GPIO functions can't fail.

Signed-off-by: Michael Buesch <mb@bu3sch.de>

---

Index: linux-2.6/arch/mips/include/asm/mach-bcm47xx/gpio.h
===================================================================
--- linux-2.6.orig/arch/mips/include/asm/mach-bcm47xx/gpio.h	2009-01-01 19:27:06.000000000 +0100
+++ linux-2.6/arch/mips/include/asm/mach-bcm47xx/gpio.h	2009-02-14 21:26:14.000000000 +0100
@@ -31,24 +31,28 @@ static inline void gpio_set_value(unsign
 
 static inline int gpio_direction_input(unsigned gpio)
 {
-	return ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 0);
+	ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 0);
+	return 0;
 }
 
 static inline int gpio_direction_output(unsigned gpio, int value)
 {
-	return ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 1 << gpio);
+	ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 1 << gpio);
+	return 0;
 }
 
-static int gpio_intmask(unsigned gpio, int value)
+static inline int gpio_intmask(unsigned gpio, int value)
 {
-	return ssb_gpio_intmask(&ssb_bcm47xx, 1 << gpio,
-				value ? 1 << gpio : 0);
+	ssb_gpio_intmask(&ssb_bcm47xx, 1 << gpio,
+			 value ? 1 << gpio : 0);
+	return 0;
 }
 
-static int gpio_polarity(unsigned gpio, int value)
+static inline int gpio_polarity(unsigned gpio, int value)
 {
-	return ssb_gpio_polarity(&ssb_bcm47xx, 1 << gpio,
-				 value ? 1 << gpio : 0);
+	ssb_gpio_polarity(&ssb_bcm47xx, 1 << gpio,
+			  value ? 1 << gpio : 0);
+	return 0;
 }
 
 


-- 
Greetings, Michael.

Re: [OpenWrt-Devel] [PATCH] bcm47xx: Fix GPIO API return codes

[Florian Fainelli]

[-- Attachment #1: Type: text/plain, Size: 629 bytes --]

Hi Michael,

Le Saturday 14 February 2009 21:27:19 Michael Buesch, vous avez écrit :
> The GPIO API is supposed to return 0 or a negative error code,
> but the SSB GPIO functions return the bitmask of the GPIO register.
> Fix this by ignoring the bitmask and always returning 0. The SSB GPIO
> functions can't fail.

At some point we might want to convert the existing board code to use gpiolib. 
I will try to come up with a patch for this later this afternoon.

Otherwise, your patch looks good.
-- 
Best regards, Florian Fainelli
Email : florian@openwrt.org
http://openwrt.org
-------------------------------

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 197 bytes --]

Re: [OpenWrt-Devel] [PATCH] bcm47xx: Fix GPIO API return codes

[Michael Buesch]

On Sunday 15 February 2009 14:27:36 Florian Fainelli wrote:
> Hi Michael,
> 
> Le Saturday 14 February 2009 21:27:19 Michael Buesch, vous avez écrit :
> > The GPIO API is supposed to return 0 or a negative error code,
> > but the SSB GPIO functions return the bitmask of the GPIO register.
> > Fix this by ignoring the bitmask and always returning 0. The SSB GPIO
> > functions can't fail.
> 
> At some point we might want to convert the existing board code to use gpiolib. 
> I will try to come up with a patch for this later this afternoon.

Well, why? What's the advantage?
I think it would degrade performance even more, due to yet another few function
calls in the hotpath. And yes, performance does matter.

The only reason the optional inlined GPIO API exists is to avoid the gpiolib callbacks
on platforms where the GPIO numbers are written in stone anyway.
And that's the case for bcm47xx

-- 
Greetings, Michael.

Re: [PATCH] bcm47xx: Fix GPIO API return codes

[Andrew Morton]

On Sat, 14 Feb 2009 21:27:19 +0100
Michael Buesch <mb@bu3sch.de> wrote:

> The GPIO API is supposed to return 0 or a negative error code,
> but the SSB GPIO functions return the bitmask of the GPIO register.
> Fix this by ignoring the bitmask and always returning 0. The SSB GPIO functions can't fail.
> 
> Signed-off-by: Michael Buesch <mb@bu3sch.de>
> 
> ---
> 
> Index: linux-2.6/arch/mips/include/asm/mach-bcm47xx/gpio.h
> ===================================================================
> --- linux-2.6.orig/arch/mips/include/asm/mach-bcm47xx/gpio.h	2009-01-01 19:27:06.000000000 +0100
> +++ linux-2.6/arch/mips/include/asm/mach-bcm47xx/gpio.h	2009-02-14 21:26:14.000000000 +0100
> @@ -31,24 +31,28 @@ static inline void gpio_set_value(unsign
>  
>  static inline int gpio_direction_input(unsigned gpio)
>  {
> -	return ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 0);
> +	ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 0);
> +	return 0;
>  }
>  
>  static inline int gpio_direction_output(unsigned gpio, int value)
>  {
> -	return ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 1 << gpio);
> +	ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 1 << gpio);
> +	return 0;
>  }
>  
> -static int gpio_intmask(unsigned gpio, int value)
> +static inline int gpio_intmask(unsigned gpio, int value)
>  {
> -	return ssb_gpio_intmask(&ssb_bcm47xx, 1 << gpio,
> -				value ? 1 << gpio : 0);
> +	ssb_gpio_intmask(&ssb_bcm47xx, 1 << gpio,
> +			 value ? 1 << gpio : 0);
> +	return 0;
>  }
>  
> -static int gpio_polarity(unsigned gpio, int value)
> +static inline int gpio_polarity(unsigned gpio, int value)
>  {
> -	return ssb_gpio_polarity(&ssb_bcm47xx, 1 << gpio,
> -				 value ? 1 << gpio : 0);
> +	ssb_gpio_polarity(&ssb_bcm47xx, 1 << gpio,
> +			  value ? 1 << gpio : 0);
> +	return 0;
>  }

What are the consequences of the bug which you fixed?  User-visible
runtime failures?  Something else?

Please always include this information in the changelogs - without it I
cannot make which-kernel-needs-this decisions.

Re: [PATCH] bcm47xx: Fix GPIO API return codes

[Michael Buesch]

On Wednesday 18 February 2009 01:29:18 Andrew Morton wrote:
> On Sat, 14 Feb 2009 21:27:19 +0100
> Michael Buesch <mb@bu3sch.de> wrote:
> 
> > The GPIO API is supposed to return 0 or a negative error code,
> > but the SSB GPIO functions return the bitmask of the GPIO register.
> > Fix this by ignoring the bitmask and always returning 0. The SSB GPIO functions can't fail.
> > 
> > Signed-off-by: Michael Buesch <mb@bu3sch.de>
> > 
> > ---
> > 
> > Index: linux-2.6/arch/mips/include/asm/mach-bcm47xx/gpio.h
> > ===================================================================
> > --- linux-2.6.orig/arch/mips/include/asm/mach-bcm47xx/gpio.h	2009-01-01 19:27:06.000000000 +0100
> > +++ linux-2.6/arch/mips/include/asm/mach-bcm47xx/gpio.h	2009-02-14 21:26:14.000000000 +0100
> > @@ -31,24 +31,28 @@ static inline void gpio_set_value(unsign
> >  
> >  static inline int gpio_direction_input(unsigned gpio)
> >  {
> > -	return ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 0);
> > +	ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 0);
> > +	return 0;
> >  }
> >  
> >  static inline int gpio_direction_output(unsigned gpio, int value)
> >  {
> > -	return ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 1 << gpio);
> > +	ssb_gpio_outen(&ssb_bcm47xx, 1 << gpio, 1 << gpio);
> > +	return 0;
> >  }
> >  
> > -static int gpio_intmask(unsigned gpio, int value)
> > +static inline int gpio_intmask(unsigned gpio, int value)
> >  {
> > -	return ssb_gpio_intmask(&ssb_bcm47xx, 1 << gpio,
> > -				value ? 1 << gpio : 0);
> > +	ssb_gpio_intmask(&ssb_bcm47xx, 1 << gpio,
> > +			 value ? 1 << gpio : 0);
> > +	return 0;
> >  }
> >  
> > -static int gpio_polarity(unsigned gpio, int value)
> > +static inline int gpio_polarity(unsigned gpio, int value)
> >  {
> > -	return ssb_gpio_polarity(&ssb_bcm47xx, 1 << gpio,
> > -				 value ? 1 << gpio : 0);
> > +	ssb_gpio_polarity(&ssb_bcm47xx, 1 << gpio,
> > +			  value ? 1 << gpio : 0);
> > +	return 0;
> >  }
> 
> What are the consequences of the bug which you fixed?  User-visible
> runtime failures?  Something else?
> 
> Please always include this information in the changelogs - without it I
> cannot make which-kernel-needs-this decisions.

Well, it fixes all users of the API that don't ignore the return value.
"Fixes" means "turns them from completely not working at all into - working properly".
This is the case for gpio-spi, for example. gpio-spi doesn't work on bcm47xx without
this fix. gpio-spi is in 2.6.29.
I didn't search for other API-users that might be broken.

So I'd like to see this going to Linus as soon as possible.

-- 
Greetings, Michael.