From: Neil Zhang <zhangwm@marvell.com>
To: Gerhard Sittig <gsi@denx.de>
CC: "linus.walleij@linaro.org" <linus.walleij@linaro.org>,
        "gnurou@gmail.com" <gnurou@gmail.com>,
        "linux-gpio@vger.kernel.org" <linux-gpio@vger.kernel.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Date: Thu, 9 Jan 2014 18:04:18 -0800
Subject: RE: [PATCH] gpio: pxa: fix bug when get gpio value
Thread-Topic: [PATCH] gpio: pxa: fix bug when get gpio value
Thread-Index: Ac8NKAOINAv3OekORZaoM6zWeueMugAfw67Q
Message-ID: <175CCF5F49938B4D99B2E3EF7F558EBE540BD53A96@SC-VEXCH4.marvell.com>
References: <1389259557-1731-1-git-send-email-zhangwm@marvell.com>
 <20140109104604.GT20094@book.gsilab.sittig.org>
In-Reply-To: <20140109104604.GT20094@book.gsilab.sittig.org>
Accept-Language: en-US
Content-Language: en-US
acceptlanguage: en-US
Content-Type: text/plain; charset="gb2312"
MIME-Version: 1.0
Sender: linux-kernel-owner@vger.kernel.org
Content-Transfer-Encoding: 8bit

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?+???????+%?????ݶ??w??{.n?+????{??G?????{ay?ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m????????????I?