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

[Neil Zhang]

Gerhard

Firstly thanks for your comments.

> -----Original Message-----
> From: Gerhard Sittig [mailto:gsi@xxxxxxx]
> Sent: 2014年1月9日 18:46
> To: Neil Zhang
> Cc: linus.walleij@xxxxxxxxxx; gnurou@xxxxxxxxx; linux-gpio@xxxxxxxxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx
> 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@xxxxxxx

Best Regards,
Neil Zhang
韬{.n?????%?lzwm?b?Р骒r?zXЩ??{ay????j?f＂?????ア?⒎?:+v???????赙zZ+????"?!?O???v??m?鹈n?帼Y&—