lkml.org 
[lkml]   [2006]   [Dec]   [21]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    Patches in this message
    /
    Date
    From
    SubjectRe: [patch 2.6.20-rc1 4/6] PXA GPIO wrappers
    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 CKENIndex: 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
    \
     
     \ /
      Last update: 2006-12-21 20:35    [W:6.662 / U:0.204 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site