Skip to contentSkip to conversation

simple gpio driver

PATCH Heiko Schocher 3 participants 5 messages Aug 13, 2009 – Aug 18, 2009 PowerPC

lore
Share & tools
lore

Heiko Schocher

author
#0 lore raw

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 solved this issue (if it is) with the following patch:

arch/powerpc/sysdev/simple_gpio.c +1 -1
index 43c4569..bb0d79c 100644
@@ -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)

bye
Heiko

Signature · 2 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
#1 lore raw
Quoted 18 lines · Heiko Schocher

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 =A0 =A0return
base =A0 =A0value
0 =A0 =A0 =A0 0/0x01
1 =A0 =A0 =A0 0/0x02
2 =A0 =A0 =A0 0/0x04
3 =A0 =A0 =A0 0/0x08
4 =A0 =A0 =A0 0/0x10
5 =A0 =A0 =A0 0/0x20
6 =A0 =A0 =A0 0/0x40
7 =A0 =A0 =A0 0/0x80

Quoted 4 lines

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.

arch/powerpc/sysdev/simp=

Quoted 4 lines

I solved this issue (if it is) with the following patch:

arch/powerpc/sysdev/simp= renamed from arch/powerpc/sysdev/simple_gpio.c +0 -0

le_gpio.c

arch/powerpc/sysdev/simple_gpio.c static int u8_gpio_get(struct gpio_chip *gc, unsigned i=

Quoted 4 lines

index 43c4569..bb0d79c 100644

arch/powerpc/sysdev/simple_gpio.c +0 -0
@@ -46,7 +46,7 @@ static int u8_gpio_get(struct gpio_chip *gc, unsigned i=

nt gpio)

arch/powerpc/sysdev/simple_gpio.c static int u8_gpio_get(struct gpio_chip *gc, unsigned i= · line 49

Quoted 5 lines
arch/powerpc/sysdev/simple_gpio.c +1 -1
 =A0{
 =A0 =A0 =A0 =A0struct of_mm_gpio_chip *mm_gc =3D to_of_mm_gpio_chip(gc);
 
- =A0 =A0 =A0 return in_8(mm_gc->regs) & u8_pin2mask(gpio);
+ =A0 =A0 =A0 return (in_8(mm_gc->regs) & u8_pin2mask(gpio) ? 1 : 0);

For clarity, the brackets should be just around the & operands, and "!=3D 0" instead of "? 1 : 0" might result in slightly smaller code.

return (in_8(mm_gc->regs) & u8_pin2mask(gpio)) !=3D 0;

#2 lore raw

Oops, I missed that patch, sorry.

Quoted 19 lines · Grant Likely
Quoted 18 lines · Heiko Schocher

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

Quoted 2 lines
Quoted 2 lines

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.

Quoted 6 lines
Quoted 2 lines

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,

Signature · 3 lines
Anton Vorontsov
email: [email protected]
irc://irc.freenode.net/bd2
author
#4 lore raw
In reply to Grant Likely

Hello Grant,

Quoted 19 lines · Grant Likely
Quoted 18 lines · Heiko Schocher

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
Quoted 6 lines
Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 10 lines
Quoted 10 lines

I solved this issue (if it is) with the following patch:

arch/powerpc/sysdev/simple_gpio.c +0 -0
index 43c4569..bb0d79c 100644
@@ -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);
Quoted 7 lines

arch/powerpc/sysdev/simple_gpio.c static int u8_gpio_get(struct gpio_chip *gc, unsigned int gpio) · line 49

Quoted 2 lines
arch/powerpc/sysdev/simple_gpio.c +1 -1
-       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

Signature · 2 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
author
#3 lore raw
In reply to Anton Vorontsov

Hello Anton,

Quoted 22 lines · Anton Vorontsov

Oops, I missed that patch, sorry.

Quoted 19 lines · Grant Likely
Quoted 18 lines · Heiko Schocher

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
Quoted 5 lines
Quoted 2 lines
Quoted 2 lines

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.

Quoted 7 lines
Quoted 5 lines
Quoted 2 lines

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

Signature · 2 lines
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany