| Message ID | 4A83A976.60608@denx.de (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
On Wed, Aug 12, 2009 at 11:49 PM, Heiko Schocher<hs@denx.de> wrote: > Hello Anton, > > i am trying to use the arch/powerpc/sysdev/simple_gpio.c driver, > for accessing some gpios, and found, that u8_gpio_get() > returns not only a 1 or a 0, instead it returns the real bit > position from the gpio: > > gpio return > base value > 0 0/0x01 > 1 0/0x02 > 2 0/0x04 > 3 0/0x08 > 4 0/0x10 > 5 0/0x20 > 6 0/0x40 > 7 0/0x80 > > I also use the arch/powerpc/platforms/52xx/mpc52xx_gpio.c and > mpc52xx_gpt.c drivers, they all return for a gpio just a 1 or 0, > which seems correct to me, because a gpio can have only 1 or 0 > as state ... what do you think? I think returning '1' is perhaps slightly 'better' (however you define that), but I don't think the caller should make any assumptions beyond zero/non-zero. > > I solved this issue (if it is) with the following patch: > > diff --git a/arch/powerpc/sysdev/simple_gpio.c b/arch/powerpc/sysdev/simple_gpio.c > index 43c4569..bb0d79c 100644 > --- a/arch/powerpc/sysdev/simple_gpio.c > +++ b/arch/powerpc/sysdev/simple_gpio.c > @@ -46,7 +46,7 @@ static int u8_gpio_get(struct gpio_chip *gc, unsigned int gpio) > { > struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); > > - return in_8(mm_gc->regs) & u8_pin2mask(gpio); > + return (in_8(mm_gc->regs) & u8_pin2mask(gpio) ? 1 : 0); For clarity, the brackets should be just around the & operands, and "!= 0" instead of "? 1 : 0" might result in slightly smaller code. return (in_8(mm_gc->regs) & u8_pin2mask(gpio)) != 0;
Oops, I missed that patch, sorry. On Mon, Aug 17, 2009 at 03:18:37PM -0600, Grant Likely wrote: > On Wed, Aug 12, 2009 at 11:49 PM, Heiko Schocher<hs@denx.de> wrote: > > Hello Anton, > > > > i am trying to use the arch/powerpc/sysdev/simple_gpio.c driver, > > for accessing some gpios, and found, that u8_gpio_get() > > returns not only a 1 or a 0, instead it returns the real bit > > position from the gpio: > > > > gpio return > > base value > > 0 0/0x01 > > 1 0/0x02 > > 2 0/0x04 > > 3 0/0x08 > > 4 0/0x10 > > 5 0/0x20 > > 6 0/0x40 > > 7 0/0x80 > > > > I also use the arch/powerpc/platforms/52xx/mpc52xx_gpio.c and > > mpc52xx_gpt.c drivers, they all return for a gpio just a 1 or 0, There is also arch/powerpc/sysdev/qe_lib/gpio.c and arch/powerpc/sysdev/mpc8xxx_gpio.c that don't do that. > > which seems correct to me, because a gpio can have only 1 or 0 > > as state ... what do you think? > > I think returning '1' is perhaps slightly 'better' (however you define > that), but I don't think the caller should make any assumptions beyond > zero/non-zero. Yep. So I don't think that the patch is needed. Thanks,
Hello Grant, Grant Likely wrote: > On Wed, Aug 12, 2009 at 11:49 PM, Heiko Schocher<hs@denx.de> wrote: >> Hello Anton, >> >> i am trying to use the arch/powerpc/sysdev/simple_gpio.c driver, >> for accessing some gpios, and found, that u8_gpio_get() >> returns not only a 1 or a 0, instead it returns the real bit >> position from the gpio: >> >> gpio return >> base value >> 0 0/0x01 >> 1 0/0x02 >> 2 0/0x04 >> 3 0/0x08 >> 4 0/0x10 >> 5 0/0x20 >> 6 0/0x40 >> 7 0/0x80 >> >> I also use the arch/powerpc/platforms/52xx/mpc52xx_gpio.c and >> mpc52xx_gpt.c drivers, they all return for a gpio just a 1 or 0, >> which seems correct to me, because a gpio can have only 1 or 0 >> as state ... what do you think? > > I think returning '1' is perhaps slightly 'better' (however you define Yep. > that), but I don't think the caller should make any assumptions beyond > zero/non-zero. Hmm... why? I think a gpio_pin can have as value only 0 or 1. Ah, if you say zero versus non zero ... hmm... okay. >> I solved this issue (if it is) with the following patch: >> >> diff --git a/arch/powerpc/sysdev/simple_gpio.c b/arch/powerpc/sysdev/simple_gpio.c >> index 43c4569..bb0d79c 100644 >> --- a/arch/powerpc/sysdev/simple_gpio.c >> +++ b/arch/powerpc/sysdev/simple_gpio.c >> @@ -46,7 +46,7 @@ static int u8_gpio_get(struct gpio_chip *gc, unsigned int gpio) >> { >> struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); >> >> - return in_8(mm_gc->regs) & u8_pin2mask(gpio); >> + return (in_8(mm_gc->regs) & u8_pin2mask(gpio) ? 1 : 0); > > For clarity, the brackets should be just around the & operands, and > "!= 0" instead of "? 1 : 0" might result in slightly smaller code. > > return (in_8(mm_gc->regs) & u8_pin2mask(gpio)) != 0; Yep, you are right, thanks for the info. bye Heiko
Hello Anton, Anton Vorontsov wrote: > Oops, I missed that patch, sorry. > > On Mon, Aug 17, 2009 at 03:18:37PM -0600, Grant Likely wrote: >> On Wed, Aug 12, 2009 at 11:49 PM, Heiko Schocher<hs@denx.de> wrote: >>> Hello Anton, >>> >>> i am trying to use the arch/powerpc/sysdev/simple_gpio.c driver, >>> for accessing some gpios, and found, that u8_gpio_get() >>> returns not only a 1 or a 0, instead it returns the real bit >>> position from the gpio: >>> >>> gpio return >>> base value >>> 0 0/0x01 >>> 1 0/0x02 >>> 2 0/0x04 >>> 3 0/0x08 >>> 4 0/0x10 >>> 5 0/0x20 >>> 6 0/0x40 >>> 7 0/0x80 >>> >>> I also use the arch/powerpc/platforms/52xx/mpc52xx_gpio.c and >>> mpc52xx_gpt.c drivers, they all return for a gpio just a 1 or 0, > > There is also arch/powerpc/sysdev/qe_lib/gpio.c and > arch/powerpc/sysdev/mpc8xxx_gpio.c that don't do that. Ah, okay. >>> which seems correct to me, because a gpio can have only 1 or 0 >>> as state ... what do you think? >> I think returning '1' is perhaps slightly 'better' (however you define >> that), but I don't think the caller should make any assumptions beyond >> zero/non-zero. > > Yep. So I don't think that the patch is needed. Yes, if the gpio lib only differs in zero versus non zero. Thanks for the info bye Heiko
diff --git a/arch/powerpc/sysdev/simple_gpio.c b/arch/powerpc/sysdev/simple_gpio.c index 43c4569..bb0d79c 100644 --- a/arch/powerpc/sysdev/simple_gpio.c +++ b/arch/powerpc/sysdev/simple_gpio.c @@ -46,7 +46,7 @@ static int u8_gpio_get(struct gpio_chip *gc, unsigned int gpio) { struct of_mm_gpio_chip *mm_gc = to_of_mm_gpio_chip(gc); - return in_8(mm_gc->regs) & u8_pin2mask(gpio); + return (in_8(mm_gc->regs) & u8_pin2mask(gpio) ? 1 : 0); } static void u8_gpio_set(struct gpio_chip *gc, unsigned int gpio, int val)