Re: [patch 2.6.20-rc1 4/6] PXA GPIO wrappers

["pHilipp Zabel" <philipp.zabel@gmail.com>]

On 12/21/06, Nicolas Pitre <nico@cam.org> 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 <philipp.zabel@gmail.com>
+ *
+ * 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 <asm/arch/pxa-regs.h>
+#include <asm/arch/irqs.h>
+#include <asm/arch/hardware.h>
+
+#include <asm/errno.h>
+
+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 <asm-generic/gpio.h>			/* 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 <asm/mach/map.h>

#include <asm/arch/pxa-regs.h>
+#include <asm/arch/gpio.h>
#include <asm/arch/udc.h>
#include <asm/arch/pxafb.h>
#include <asm/arch/serial.h>
@@ -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

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 <philipp.zabel@gmail.com>
+ *
+ * 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 <asm/arch/pxa-regs.h>
+#include <asm/arch/irqs.h>
+#include <asm/arch/hardware.h>
+
+#include <asm/errno.h>
+
+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 <asm-generic/gpio.h>			/* 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 <asm/mach/map.h>
 
 #include <asm/arch/pxa-regs.h>
+#include <asm/arch/gpio.h>
 #include <asm/arch/udc.h>
 #include <asm/arch/pxafb.h>
 #include <asm/arch/serial.h>
@@ -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