RE: [PATCH] gpio: pxa: fix bug when get gpio value

RE: [PATCH] gpio: pxa: fix bug when get gpio value

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 1438 bytes --]


> -----Original Message-----
> From: Neil Zhang [mailto:zhangwm@marvell.com]
> Sent: 2014Äê1ÔÂ9ÈÕ 17:26
> To: linus.walleij@linaro.org; gnurou@gmail.com; linux-gpio@vger.kernel.org;
> linux-kernel@vger.kernel.or
> Cc: Neil Zhang
> Subject: [PATCH] gpio: pxa: fix bug when get gpio value
> 
> gpio_get_value should return 0 or 1.
> 
> I have checked mach-mmp / mach-pxa / plat-pxa / plat-orion / mach-orion5x.
> It's OK for all of them to change this function to return 0 and 1.
> 
> Signed-off-by: Neil Zhang <zhangwm@marvell.com>
> ---
>  drivers/gpio/gpio-pxa.c |    3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpio/gpio-pxa.c b/drivers/gpio/gpio-pxa.c index
> cc13d1b..42e6e64 100644
> --- a/drivers/gpio/gpio-pxa.c
> +++ b/drivers/gpio/gpio-pxa.c
> @@ -263,7 +263,8 @@ static int pxa_gpio_direction_output(struct gpio_chip
> *chip,
> 
>  static int pxa_gpio_get(struct gpio_chip *chip, unsigned offset)  {
> -	return readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET) & (1 <<
> offset);
> +	u32 gplr = readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET);
> +	return !!(gplr & (1 << offset));
>  }
> 
>  static void pxa_gpio_set(struct gpio_chip *chip, unsigned offset, int value)
> --
> 1.7.9.5

Add right LKML.

Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] gpio: pxa: fix bug when get gpio value

[Gerhard Sittig]

On Thu, Jan 09, 2014 at 17:25 +0800, Neil Zhang wrote:
> 
> gpio_get_value should return 0 or 1.
> 
> I have checked mach-mmp / mach-pxa / plat-pxa / plat-orion / mach-orion5x.
> It's OK for all of them to change this function to return 0 and 1.
> 
> [ ... ]
>  
>  static int pxa_gpio_get(struct gpio_chip *chip, unsigned offset)
>  {
> -	return readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET) & (1 << offset);
> +	u32 gplr = readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET);
> +	return !!(gplr & (1 << offset));
>  }

This may be a stupid question, but isn't the "!!value" syntax
just replacing one kind of "zero or non-zero" with another kind
of "zero or non-zero"?  This phrase is used to normalize
booleans, but ISTR there is no guarantee that true needs to equal
the value of 1.

Here is why I'm asking:  Is there a need from GPIO get_value()
routines to return normalized values, and if so should not more
drivers receive an update?  Or need get_value() routines just
return the usual integer zero/non-zero values (as is the
convention in the C programming language) and GPIO using callers
should know that they are not exactly 0 and 1?

The latter would make this change for PXA unneeded (nice to have
but not essential), and both the commit message as well as the
subject line at least need to get re-phrased, as the "bug" is in
the caller's expectation and not in the GPIO chip driver.

Please note that I'm not questioning the patches' being
applicable, but am trying to find out what else needs to be done
which previously may have gone unnoticed.  A doc update maybe, to
make GPIO users aware that the return value may not be exactly 1,
and to clear up where such an assumption is wrongly made.

If the GPIO subsystem's API wants to guarantee values of 0 and 1
(which I think it doesn't), then I feel the adjustment should be
done in the gpio_get_value() routines (in all its public
variants, or a common routine which all of them pass through),
and certainly not in individual chip drivers.


virtually yours
Gerhard Sittig
-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

RE: [PATCH] gpio: pxa: fix bug when get gpio value

[Neil Zhang]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="gb2312", Size: 3184 bytes --]

Gerhard

Firstly thanks for your comments.

> -----Original Message-----
> From: Gerhard Sittig [mailto:gsi@denx.de]
> Sent: 2014Äê1ÔÂ9ÈÕ 18:46
> To: Neil Zhang
> Cc: linus.walleij@linaro.org; gnurou@gmail.com; linux-gpio@vger.kernel.org;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] gpio: pxa: fix bug when get gpio value
> 
> On Thu, Jan 09, 2014 at 17:25 +0800, Neil Zhang wrote:
> >
> > gpio_get_value should return 0 or 1.
> >
> > I have checked mach-mmp / mach-pxa / plat-pxa / plat-orion / mach-orion5x.
> > It's OK for all of them to change this function to return 0 and 1.
> >
> > [ ... ]
> >
> >  static int pxa_gpio_get(struct gpio_chip *chip, unsigned offset)  {
> > -	return readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET) & (1 <<
> offset);
> > +	u32 gplr = readl_relaxed(gpio_chip_base(chip) + GPLR_OFFSET);
> > +	return !!(gplr & (1 << offset));
> >  }
> 
> This may be a stupid question, but isn't the "!!value" syntax just replacing one
> kind of "zero or non-zero" with another kind of "zero or non-zero"?  This
> phrase is used to normalize booleans, but ISTR there is no guarantee that true
> needs to equal the value of 1.
> 
> Here is why I'm asking:  Is there a need from GPIO get_value() routines to
> return normalized values, and if so should not more drivers receive an update?
> Or need get_value() routines just return the usual integer zero/non-zero values
> (as is the convention in the C programming language) and GPIO using callers
> should know that they are not exactly 0 and 1?
>
> The latter would make this change for PXA unneeded (nice to have but not
> essential), and both the commit message as well as the subject line at least need
> to get re-phrased, as the "bug" is in the caller's expectation and not in the GPIO
> chip driver.
>

Yes, the API doesn't mandatory expect 0 and 1 as I can see.
But actually many driver will return a normalized value.
So people may think it's return value is 0 or 1.
It would be convenient to normalized the return value for gpio-pxa too.

You are right, it's not appropriate to use "bug" in the patch title, thanks.

> Please note that I'm not questioning the patches' being applicable, but am trying
> to find out what else needs to be done which previously may have gone
> unnoticed.  A doc update maybe, to make GPIO users aware that the return
> value may not be exactly 1, and to clear up where such an assumption is wrongly
> made.
> 
> If the GPIO subsystem's API wants to guarantee values of 0 and 1 (which I think it
> doesn't), then I feel the adjustment should be done in the gpio_get_value()
> routines (in all its public variants, or a common routine which all of them pass
> through), and certainly not in individual chip drivers.
> 
> 
> virtually yours
> Gerhard Sittig
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany
> Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office@denx.de

Best Regards,
Neil Zhang
ÿôèº{.nÇ+‰·Ÿ®‰­†+%ŠËÿ±éݶ\x17¥Šwÿº{.nÇ+‰·¥Š{±þG«éÿŠ{ayº\x1dʇڙë,j\a­¢f£¢·hšïêÿ‘êçz_è®\x03(­éšŽŠÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?™¨è­Ú&£ø§~á¶iO•æ¬z·švØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?–I¥

Re: [PATCH] gpio: pxa: fix bug when get gpio value

[Linus Walleij]

On Thu, Jan 9, 2014 at 11:46 AM, Gerhard Sittig <gsi@denx.de> wrote:

> Here is why I'm asking:  Is there a need from GPIO get_value()
> routines to return normalized values,

That totally depends.

All drivers calling gpio[d]_get_value() will be returned the
value directly from the driver without any clamping to [0,1] in
gpiolib. These are 496 occurences in the kernel, you'd have
to check them all to see if they expect this or not.

Hm. Maybe we should clamp it in gpiolib...

> and if so should not more
> drivers receive an update?

Probably. But on my part I want that more as a code
readability and maintenance hygiene thing, it gives a clear
sign that the driver author think about details.

(Possibly it gives the compiler a chance to optimize stuff
also, I don't quite know that.)

> If the GPIO subsystem's API wants to guarantee values of 0 and 1
> (which I think it doesn't), then I feel the adjustment should be
> done in the gpio_get_value() routines (in all its public
> variants, or a common routine which all of them pass through),
> and certainly not in individual chip drivers.

One does not exclude the other.

Yours,
Linus Walleij