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

[Nicolas Pitre <nico@cam.org>]

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?

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?


Nicolas