| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <74d0deb30612212253s7d35cf92q80bbebe9d8ae9476@mail.gmail.com>
Date: Fri, 22 Dec 2006 07:53:04 +0100
From: "pHilipp Zabel" <philipp.zabel@...il.com>
To: "Nicolas Pitre" <nico@....org>
Cc: "David Brownell" <david-b@...bell.net>,
"Andrew Morton" <akpm@...l.org>,
"Linux Kernel list" <linux-kernel@...r.kernel.org>,
"Andrew Victor" <andrew@...people.com>,
"Bill Gatliff" <bgat@...lgatliff.com>,
"Haavard Skinnemoen" <hskinnemoen@...el.com>,
"Kevin Hilman" <khilman@...sta.com>,
"Russell King" <rmk@....linux.org.uk>,
"Tony Lindgren" <tony@...mide.com>
Subject: Re: [patch 2.6.20-rc1 4/6] PXA GPIO wrappers
On 12/21/06, Nicolas Pitre <nico@....org> wrote:
> On Thu, 21 Dec 2006, pHilipp Zabel wrote:
>
> > > > --- 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)...
>
> Sorry I don't quite follow you here. Why would you expand the macro in
> place?
And that is no surprise because I seem to have problems to follow
myself here now after a good night's rest. Basically I was thinking
that after expanding the macros in place the code could be optimized.
Of course, this doesn't have any advantage for the inlined functions
(gpio is constant, so most of the code will be optimized away anyway),
and shaving one or two words off pxa_gpio_setval is hardly worthwile.
> My suggestion is only about not duplicating the source code. The
> generated assembly will be the same.
>
> And your patch looks fine to me now, except for this:
>
> +int pxa_gpio_get_value(unsigned gpio)
> +{
> + __gpio_get_value(gpio);
> +}
>
> You certainly meant to add a "return" in there, right?
Oh yes. Sorry.
cheers
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@...il.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 21:25:24.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)
+{
+ return __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
View attachment "gpio-api-pxa.patch" of type "text/x-patch" (4626 bytes)
Powered by blists - more mailing lists