simple gpio driver

simple gpio driver

[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

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:

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)

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

Re: simple gpio driver

[Grant Likely]

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 =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
>
> 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/simp=
le_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 i=
nt gpio)
> =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;

Re: simple gpio driver

[Anton Vorontsov]

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,

-- 
Anton Vorontsov
email: cbouatmailru@gmail.com
irc://irc.freenode.net/bd2

Re: simple gpio driver

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany

Re: simple gpio driver

[Heiko Schocher]

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
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany