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

From: Nicolas Pitre
Date: Thu Dec 21 2006 - 15:11:37 EST


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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/