W1 master implementations are expected to return 0 or 1 from their
read_bit() function. However, not all platforms do return these values
from gpio_get_value() - namely PXAs won't. Hence the w1 gpio-master
needs to break the result down to 0 or 1 itself.
Signed-off-by: Daniel Mack <[email protected]>
Cc: Ville Syrjala <[email protected]>
Cc: Evgeniy Polyakov <[email protected]>
---
drivers/w1/masters/w1-gpio.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
index 9e1138a..a411702 100644
--- a/drivers/w1/masters/w1-gpio.c
+++ b/drivers/w1/masters/w1-gpio.c
@@ -39,7 +39,7 @@ static u8 w1_gpio_read_bit(void *data)
{
struct w1_gpio_platform_data *pdata = data;
- return gpio_get_value(pdata->pin);
+ return gpio_get_value(pdata->pin) ? 1 : 0;
}
static int __init w1_gpio_probe(struct platform_device *pdev)
--
1.6.1.3
On Mon, 9 Mar 2009 17:02:10 +0100 Daniel Mack <[email protected]> wrote:
> W1 master implementations are expected to return 0 or 1 from their
> read_bit() function. However, not all platforms do return these values
> from gpio_get_value() - namely PXAs won't. Hence the w1 gpio-master
> needs to break the result down to 0 or 1 itself.
>
> Signed-off-by: Daniel Mack <[email protected]>
> Cc: Ville Syrjala <[email protected]>
> Cc: Evgeniy Polyakov <[email protected]>
> ---
> drivers/w1/masters/w1-gpio.c | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
> index 9e1138a..a411702 100644
> --- a/drivers/w1/masters/w1-gpio.c
> +++ b/drivers/w1/masters/w1-gpio.c
> @@ -39,7 +39,7 @@ static u8 w1_gpio_read_bit(void *data)
> {
> struct w1_gpio_platform_data *pdata = data;
>
> - return gpio_get_value(pdata->pin);
> + return gpio_get_value(pdata->pin) ? 1 : 0;
> }
>
> static int __init w1_gpio_probe(struct platform_device *pdev)
We recently merged a patch (I forget where) which fixed one
gpio_get_value() implementation so that it always returns 0 or 1.
>From which I deduce that the correct fix for <whatever problem you're
seeing> is to fix <whichever driver that is>?
On Mon, Mar 09, 2009 at 07:14:19PM -0700, Andrew Morton wrote:
> > --- a/drivers/w1/masters/w1-gpio.c
> > +++ b/drivers/w1/masters/w1-gpio.c
> > @@ -39,7 +39,7 @@ static u8 w1_gpio_read_bit(void *data)
> > {
> > struct w1_gpio_platform_data *pdata = data;
> >
> > - return gpio_get_value(pdata->pin);
> > + return gpio_get_value(pdata->pin) ? 1 : 0;
> > }
> >
> > static int __init w1_gpio_probe(struct platform_device *pdev)
>
> We recently merged a patch (I forget where) which fixed one
> gpio_get_value() implementation so that it always returns 0 or 1.
>
> From which I deduce that the correct fix for <whatever problem you're
> seeing> is to fix <whichever driver that is>?
I agree those functions should return 0 and 1 only, but my patch fixes
the w1-gpio driver for all platforms at once, so people can use it.
On the other hand, I will submit a patch which modifies PXA's
gpio_get_value() and see what the maintainers say, but I can't go thru
all the implemenations of all architectures to do this.
So for the time being, the above patch helps many users of that driver.
Daniel
On Mon, Mar 09, 2009 at 07:14:19PM -0700, Andrew Morton wrote:
> On Mon, 9 Mar 2009 17:02:10 +0100 Daniel Mack <[email protected]> wrote:
>
> > W1 master implementations are expected to return 0 or 1 from their
> > read_bit() function. However, not all platforms do return these values
> > from gpio_get_value() - namely PXAs won't. Hence the w1 gpio-master
> > needs to break the result down to 0 or 1 itself.
> >
> > Signed-off-by: Daniel Mack <[email protected]>
> > Cc: Ville Syrjala <[email protected]>
> > Cc: Evgeniy Polyakov <[email protected]>
> > ---
> > drivers/w1/masters/w1-gpio.c | 2 +-
> > 1 files changed, 1 insertions(+), 1 deletions(-)
> >
> > diff --git a/drivers/w1/masters/w1-gpio.c b/drivers/w1/masters/w1-gpio.c
> > index 9e1138a..a411702 100644
> > --- a/drivers/w1/masters/w1-gpio.c
> > +++ b/drivers/w1/masters/w1-gpio.c
> > @@ -39,7 +39,7 @@ static u8 w1_gpio_read_bit(void *data)
> > {
> > struct w1_gpio_platform_data *pdata = data;
> >
> > - return gpio_get_value(pdata->pin);
> > + return gpio_get_value(pdata->pin) ? 1 : 0;
> > }
> >
> > static int __init w1_gpio_probe(struct platform_device *pdev)
>
> We recently merged a patch (I forget where) which fixed one
> gpio_get_value() implementation so that it always returns 0 or 1.
>
> From which I deduce that the correct fix for <whatever problem you're
> seeing> is to fix <whichever driver that is>?
The documentation should be fixed to match if that's the desired
behaviour. From Documentation/gpio.txt:
/* GPIO INPUT: return zero or nonzero */
int gpio_get_value(unsigned gpio);
Maybe the gpio_get_value() return value should be changed to bool to
make things clear.
w1 itself is a bit odd as the documentation says that read_bit() must
return 0 or 1, but the core uses it like this 'read_bit() & 0x1'. Not
sure what the idea is here. Perhaps read_bit() returns the contents of
some shift register on some masters. But if the documentation is to be
trusted the '& 0x1' should be moved to the master drivers that need it.
--
Ville Syrj?l?
[email protected]
http://www.sci.fi/~syrjala/
On Tue, 10 Mar 2009 10:18:00 +0100
Daniel Mack <[email protected]> wrote:
> On Mon, Mar 09, 2009 at 07:14:19PM -0700, Andrew Morton wrote:
> > > --- a/drivers/w1/masters/w1-gpio.c
> > > +++ b/drivers/w1/masters/w1-gpio.c
> > > @@ -39,7 +39,7 @@ static u8 w1_gpio_read_bit(void *data)
> > > {
> > > struct w1_gpio_platform_data *pdata = data;
> > >
> > > - return gpio_get_value(pdata->pin);
> > > + return gpio_get_value(pdata->pin) ? 1 : 0;
> > > }
> > >
> > > static int __init w1_gpio_probe(struct platform_device *pdev)
> >
> > We recently merged a patch (I forget where) which fixed one
> > gpio_get_value() implementation so that it always returns 0 or 1.
> >
> > From which I deduce that the correct fix for <whatever problem you're
> > seeing> is to fix <whichever driver that is>?
>
> I agree those functions should return 0 and 1 only, but my patch fixes
> the w1-gpio driver for all platforms at once, so people can use it.
>
> On the other hand, I will submit a patch which modifies PXA's
> gpio_get_value() and see what the maintainers say, but I can't go thru
> all the implemenations of all architectures to do this.
>
> So for the time being, the above patch helps many users of that driver.
>
Problem is, the patch will just conceal bugs.
How about this?
--- a/drivers/w1/masters/w1-gpio.c~a
+++ a/drivers/w1/masters/w1-gpio.c
@@ -38,8 +38,12 @@ static void w1_gpio_write_bit_val(void *
static u8 w1_gpio_read_bit(void *data)
{
struct w1_gpio_platform_data *pdata = data;
+ u8 ret;
- return gpio_get_value(pdata->pin);
+ ret = gpio_get_value(pdata->pin);
+ if (WARN_ONCE(ret > 1, "gpio_get_value(): invalid return: %u\n", ret))
+ ret = !!ret
+ return ret;
}
static int __init w1_gpio_probe(struct platform_device *pdev)
_
On Tue, Mar 10, 2009 at 03:00:59PM -0700, Andrew Morton wrote:
> On Tue, 10 Mar 2009 10:18:00 +0100
> Daniel Mack <[email protected]> wrote:
>
> > On Mon, Mar 09, 2009 at 07:14:19PM -0700, Andrew Morton wrote:
> > > > --- a/drivers/w1/masters/w1-gpio.c
> > > > +++ b/drivers/w1/masters/w1-gpio.c
> > > > @@ -39,7 +39,7 @@ static u8 w1_gpio_read_bit(void *data)
> > > > {
> > > > struct w1_gpio_platform_data *pdata = data;
> > > >
> > > > - return gpio_get_value(pdata->pin);
> > > > + return gpio_get_value(pdata->pin) ? 1 : 0;
> > > > }
> > > >
> > > > static int __init w1_gpio_probe(struct platform_device *pdev)
> > >
> > > We recently merged a patch (I forget where) which fixed one
> > > gpio_get_value() implementation so that it always returns 0 or 1.
> > >
> > > From which I deduce that the correct fix for <whatever problem you're
> > > seeing> is to fix <whichever driver that is>?
> >
> > I agree those functions should return 0 and 1 only, but my patch fixes
> > the w1-gpio driver for all platforms at once, so people can use it.
> >
> > On the other hand, I will submit a patch which modifies PXA's
> > gpio_get_value() and see what the maintainers say, but I can't go thru
> > all the implemenations of all architectures to do this.
> >
> > So for the time being, the above patch helps many users of that driver.
> >
>
> Problem is, the patch will just conceal bugs.
>
> How about this?
>
> --- a/drivers/w1/masters/w1-gpio.c~a
> +++ a/drivers/w1/masters/w1-gpio.c
> @@ -38,8 +38,12 @@ static void w1_gpio_write_bit_val(void *
> static u8 w1_gpio_read_bit(void *data)
> {
> struct w1_gpio_platform_data *pdata = data;
> + u8 ret;
>
> - return gpio_get_value(pdata->pin);
> + ret = gpio_get_value(pdata->pin);
> + if (WARN_ONCE(ret > 1, "gpio_get_value(): invalid return: %u\n", ret))
> + ret = !!ret
> + return ret;
> }
>
> static int __init w1_gpio_probe(struct platform_device *pdev)
I think if you want to go that route you should add such check into
gpiolib rather than some random driver.
--
Ville Syrj?l?
[email protected]
http://www.sci.fi/~syrjala/
On Tue, Mar 10, 2009 at 03:00:59PM -0700, Andrew Morton wrote:
> > > We recently merged a patch (I forget where) which fixed one
> > > gpio_get_value() implementation so that it always returns 0 or 1.
> > >
> > > From which I deduce that the correct fix for <whatever problem you're
> > > seeing> is to fix <whichever driver that is>?
> >
> > I agree those functions should return 0 and 1 only, but my patch fixes
> > the w1-gpio driver for all platforms at once, so people can use it.
> >
> > On the other hand, I will submit a patch which modifies PXA's
> > gpio_get_value() and see what the maintainers say, but I can't go thru
> > all the implemenations of all architectures to do this.
> >
> > So for the time being, the above patch helps many users of that driver.
> >
>
> Problem is, the patch will just conceal bugs.
There is a small discussion about that on the arm-linux mailing list and
what people pointed out there is that gpio_get_value() is _not_ supposed
to return 0 or 1 only, also according to Documentation/gpio.txt:
Use these calls to access such GPIOs:
/* GPIO INPUT: return zero or nonzero */
int gpio_get_value(unsigned gpio);
Hence, any implementation of gpio_get_value() which returns 0 and 1 only
is conform to the docs, but PXA's (which doesn't follow that rule) is as
well. And that means that any driver using that function has to deal
with values > 1 being returned by it, right?
Correct me if I missed the point, but I don't see how my patch will
conceal any bug?
Daniel
On Wed, 11 Mar 2009 01:10:48 +0100 Daniel Mack <[email protected]> wrote:
> On Tue, Mar 10, 2009 at 03:00:59PM -0700, Andrew Morton wrote:
> > > > We recently merged a patch (I forget where) which fixed one
> > > > gpio_get_value() implementation so that it always returns 0 or 1.
> > > >
> > > > From which I deduce that the correct fix for <whatever problem you're
> > > > seeing> is to fix <whichever driver that is>?
> > >
> > > I agree those functions should return 0 and 1 only, but my patch fixes
> > > the w1-gpio driver for all platforms at once, so people can use it.
> > >
> > > On the other hand, I will submit a patch which modifies PXA's
> > > gpio_get_value() and see what the maintainers say, but I can't go thru
> > > all the implemenations of all architectures to do this.
> > >
> > > So for the time being, the above patch helps many users of that driver.
> > >
> >
> > Problem is, the patch will just conceal bugs.
>
> There is a small discussion about that on the arm-linux mailing list and
> what people pointed out there is that gpio_get_value() is _not_ supposed
> to return 0 or 1 only, also according to Documentation/gpio.txt:
>
> Use these calls to access such GPIOs:
>
> /* GPIO INPUT: return zero or nonzero */
> int gpio_get_value(unsigned gpio);
>
> Hence, any implementation of gpio_get_value() which returns 0 and 1 only
> is conform to the docs, but PXA's (which doesn't follow that rule) is as
> well. And that means that any driver using that function has to deal
> with values > 1 being returned by it, right?
>
> Correct me if I missed the point, but I don't see how my patch will
> conceal any bug?
>
Actually, I misremembered the discussion: http://lkml.org/lkml/2009/2/15/114
This "optimisation" has caused two bugs so far. And it's forcing
callers of the "optimised" function to perform a test-n-branch for
something which the low-level function could have done with a shift.
Sigh, what a crock. I'll go dig out your original fix.