On 12/21/06, Nicolas Pitre wrote: > On Thu, 21 Dec 2006, pHilipp Zabel wrote: > > > David suggested to have both inline and non-inline functions depending > > on whether gpio is constant. How is this patch? > > More comments below. > > > --- /dev/null 1970-01-01 00:00:00.000000000 +0000 > > +++ linux-2.6/include/asm-arm/arch-pxa/gpio.h 2006-12-21 > > 07:57:12.000000000 +0100 > > @@ -0,0 +1,86 @@ > [...] > > +static inline int gpio_direction_input(unsigned gpio) > > +{ > > + if (gpio > PXA_LAST_GPIO) > > + return -EINVAL; > > + pxa_gpio_mode(gpio | GPIO_IN); > > +} > > + > > +static inline int gpio_direction_output(unsigned gpio) > > +{ > > + if (gpio > PXA_LAST_GPIO) > > + return -EINVAL; > > + pxa_gpio_mode(gpio | GPIO_OUT); > > +} > > Please push this test against PXA_LAST_GPIO inside pxa_gpio_mode(). It > has no advantage to be inline if you're about to call a function anyway. > That would make pxa_gpio_mode() more reliable for those not calling it > through the generic API wrt that kind of error as well. I see. Originally I didn't want to touch generic.c at all. Now pxa_gpio_mode returns int instead of void. > > --- linux-2.6.orig/arch/arm/mach-pxa/generic.c 2006-12-16 > > +++ linux-2.6/arch/arm/mach-pxa/generic.c 2006-12-16 16:47:45.000000000 > > @@ -129,6 +129,29 @@ > > EXPORT_SYMBOL(pxa_gpio_mode); > > > > /* > > + * Return GPIO level, nonzero means high, zero is low > > + */ > > +int pxa_gpio_get_value(unsigned gpio) > > +{ > > + return GPLR(gpio) & GPIO_bit(gpio); > > +} > > + > > +EXPORT_SYMBOL(pxa_gpio_get_value); > > + > > +/* > > + * Set output GPIO level > > + */ > > +void pxa_gpio_set_value(unsigned gpio, int value) > > +{ > > + if (value) > > + GPSR(gpio) = GPIO_bit(gpio); > > + else > > + GPCR(gpio) = GPIO_bit(gpio); > > +} > > + > > +EXPORT_SYMBOL(pxa_gpio_set_value); > > Instead of duplicating code here, you probably should just reuse > __gpio_set_value() and __gpio_get_value() inside those functions. Probably? What I am wondering is this: can the compiler optimize away the range check that is duplicated in GPSR/GPCR and GPIO_bit for __gpio_set/get_value? Or could we optimize this case by expanding the macros in place (which would mean duplicating code from pxa-regs.h)... regards Philipp Index: linux-2.6/include/asm-arm/arch-pxa/gpio.h =================================================================== --- /dev/null 1970-01-01 00:00:00.000000000 +0000 +++ linux-2.6/include/asm-arm/arch-pxa/gpio.h 2006-12-21 20:07:48.000000000 +0100 @@ -0,0 +1,82 @@ +/* + * linux/include/asm-arm/arch-pxa/gpio.h + * + * PXA GPIO wrappers for arch-neutral GPIO calls + * + * Written by Philipp Zabel + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation; either version 2 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307 USA + * + */ + +#ifndef __ASM_ARCH_PXA_GPIO_H +#define __ASM_ARCH_PXA_GPIO_H + +#include +#include +#include + +#include + +static inline int gpio_request(unsigned gpio, const char *label) +{ + return 0; +} + +static inline void gpio_free(unsigned gpio) +{ + return; +} + +static inline int gpio_direction_input(unsigned gpio) +{ + return pxa_gpio_mode(gpio | GPIO_IN); +} + +static inline int gpio_direction_output(unsigned gpio) +{ + return pxa_gpio_mode(gpio | GPIO_OUT); +} + +static inline int __gpio_get_value(unsigned gpio) +{ + return GPLR(gpio) & GPIO_bit(gpio); +} + +#define gpio_get_value(gpio) \ + (__builtin_constant_p(gpio) ? \ + __gpioe_get_value(gpio) : \ + pxa_gpio_get_value(gpio)) + +static inline void __gpio_set_value(unsigned gpio, int value) +{ + if (value) + GPSR(gpio) = GPIO_bit(gpio); + else + GPCR(gpio) = GPIO_bit(gpio); +} + +#define gpio_set_value(gpio,value) \ + (__builtin_constant_p(gpio) ? \ + __gpio_set_value(gpio, value) : \ + pxa_gpio_set_value(gpio, value)) + +#include /* cansleep wrappers */ + +#define gpio_to_irq(gpio) IRQ_GPIO(gpio) +#define irq_to_gpio(irq) IRQ_TO_GPIO(irq) + + +#endif Index: linux-2.6/arch/arm/mach-pxa/generic.c =================================================================== --- linux-2.6.orig/arch/arm/mach-pxa/generic.c 2006-12-21 13:30:01.000000000 +0100 +++ linux-2.6/arch/arm/mach-pxa/generic.c 2006-12-21 20:06:16.000000000 +0100 @@ -36,6 +36,7 @@ #include #include +#include #include #include #include @@ -105,13 +106,16 @@ * Handy function to set GPIO alternate functions */ -void pxa_gpio_mode(int gpio_mode) +int pxa_gpio_mode(int gpio_mode) { unsigned long flags; int gpio = gpio_mode & GPIO_MD_MASK_NR; int fn = (gpio_mode & GPIO_MD_MASK_FN) >> 8; int gafr; + if (gpio > PXA_LAST_GPIO) + return -EINVAL; + local_irq_save(flags); if (gpio_mode & GPIO_DFLT_LOW) GPCR(gpio) = GPIO_bit(gpio); @@ -124,11 +128,33 @@ gafr = GAFR(gpio) & ~(0x3 << (((gpio) & 0xf)*2)); GAFR(gpio) = gafr | (fn << (((gpio) & 0xf)*2)); local_irq_restore(flags); + + return 0; } EXPORT_SYMBOL(pxa_gpio_mode); /* + * Return GPIO level + */ +int pxa_gpio_get_value(unsigned gpio) +{ + __gpio_get_value(gpio); +} + +EXPORT_SYMBOL(pxa_gpio_get_value); + +/* + * Set output GPIO level + */ +void pxa_gpio_set_value(unsigned gpio, int value) +{ + __gpio_set_value(gpio, value); +} + +EXPORT_SYMBOL(pxa_gpio_set_value); + +/* * Routine to safely enable or disable a clock in the CKEN */ void pxa_set_cken(int clock, int enable) Index: linux-2.6/include/asm-arm/arch-pxa/hardware.h =================================================================== --- linux-2.6.orig/include/asm-arm/arch-pxa/hardware.h 2006-12-21 13:30:01.000000000 +0100 +++ linux-2.6/include/asm-arm/arch-pxa/hardware.h 2006-12-21 20:03:55.000000000 +0100 @@ -65,7 +65,17 @@ /* * Handy routine to set GPIO alternate functions */ -extern void pxa_gpio_mode( int gpio_mode ); +extern int pxa_gpio_mode( int gpio_mode ); + +/* + * Return GPIO level, nonzero means high, zero is low + */ +extern int pxa_gpio_get_value(unsigned gpio); + +/* + * Set output GPIO level + */ +extern void pxa_gpio_set_value(unsigned gpio, int value); /* * Routine to enable or disable CKEN