[PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Daniel Mack]

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 <daniel@caiaq.de>
Cc: Ville Syrjala <syrjala@sci.fi>
Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
---
 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

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Andrew Morton]

On Mon,  9 Mar 2009 17:02:10 +0100 Daniel Mack <daniel@caiaq.de> 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 <daniel@caiaq.de>
> Cc: Ville Syrjala <syrjala@sci.fi>
> Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> ---
>  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>?

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Daniel Mack]

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

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Andrew Morton]

On Tue, 10 Mar 2009 10:18:00 +0100
Daniel Mack <daniel@caiaq.de> 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)
_

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Ville Syrjälä]

On Tue, Mar 10, 2009 at 03:00:59PM -0700, Andrew Morton wrote:
> On Tue, 10 Mar 2009 10:18:00 +0100
> Daniel Mack <daniel@caiaq.de> 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ä
syrjala@sci.fi
http://www.sci.fi/~syrjala/

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Daniel Mack]

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

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Andrew Morton]

On Wed, 11 Mar 2009 01:10:48 +0100 Daniel Mack <daniel@caiaq.de> 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.

Re: [PATCH] drivers/w1/masters/w1-gpio.c: fix read_bit()

[Ville Syrjälä]

On Mon, Mar 09, 2009 at 07:14:19PM -0700, Andrew Morton wrote:
> On Mon,  9 Mar 2009 17:02:10 +0100 Daniel Mack <daniel@caiaq.de> 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 <daniel@caiaq.de>
> > Cc: Ville Syrjala <syrjala@sci.fi>
> > Cc: Evgeniy Polyakov <johnpol@2ka.mipt.ru>
> > ---
> >  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ä
syrjala@sci.fi
http://www.sci.fi/~syrjala/