[PATCH] i2c-pcf8575: check for errors when accessing the chip

[PATCH] i2c-pcf8575: check for errors when accessing the chip

[Wolfram Sang]

Make the driver check and report error cases when reading from or writing to
the chip. It used to assume 'always success' and could even deliver bogus
values.

Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
---

The PCF on my system seems broken, it does not respond (and I can't probe as my
master doesn't support smbus_quick). The broken chip went unnoticed as the
driver reported some (= bogus) values. So, I can only test the properly
reported error cases, hopefully someone else can confirm the working case.
(Michael and Bart, sorry for double post)

 drivers/i2c/chips/pcf8575.c |   29 +++++++++++++++++++++--------
 1 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/i2c/chips/pcf8575.c b/drivers/i2c/chips/pcf8575.c
index 3ea08ac..4a4247d 100644
--- a/drivers/i2c/chips/pcf8575.c
+++ b/drivers/i2c/chips/pcf8575.c
@@ -67,15 +67,22 @@ static ssize_t show_read(struct device *dev, struct device_attribute *attr,
 			 char *buf)
 {
 	struct i2c_client *client = to_i2c_client(dev);
+	int status, retval = -EIO;
 	u16 val;
 	u8 iopin_state[2];
 
-	i2c_master_recv(client, iopin_state, 2);
+	status = i2c_master_recv(client, iopin_state, 2);
 
-	val = iopin_state[0];
-	val |= iopin_state[1] << 8;
+	if (status == 2) {
+		val = iopin_state[0];
+		val |= iopin_state[1] << 8;
+		return sprintf(buf, "%u\n", val);
+	}
+
+	if (status < 0)
+	    retval = status;
 
-	return sprintf(buf, "%u\n", val);
+	return retval;
 }
 
 static DEVICE_ATTR(read, S_IRUGO, show_read, NULL);
@@ -95,19 +102,25 @@ static ssize_t set_write(struct device *dev, struct device_attribute *attr,
 	struct i2c_client *client = to_i2c_client(dev);
 	struct pcf8575_data *data = i2c_get_clientdata(client);
 	unsigned long val = simple_strtoul(buf, NULL, 10);
+	int status, retval = -EIO;
 	u8 iopin_state[2];
 
 	if (val > 0xffff)
 		return -EINVAL;
 
-	data->write = val;
-
 	iopin_state[0] = val & 0xFF;
 	iopin_state[1] = val >> 8;
 
-	i2c_master_send(client, iopin_state, 2);
+	status = i2c_master_send(client, iopin_state, 2);
+
+	if (status == 2)
+		retval = count;
+	else if (status < 0)
+		retval = status;
+
+	data->write = (status == 2) ? val : retval;
 
-	return count;
+	return retval;
 }
 
 static DEVICE_ATTR(write, S_IWUSR | S_IRUGO, show_write, set_write);
-- 
1.5.5.1


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Jean Delvare]

Hi Wolfram,

On Thu, 15 May 2008 16:35:27 +0200, Wolfram Sang wrote:
> Make the driver check and report error cases when reading from or writing to
> the chip. It used to assume 'always success' and could even deliver bogus
> values.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> 
> The PCF on my system seems broken, it does not respond (and I can't probe as my
> master doesn't support smbus_quick). The broken chip went unnoticed as the
> driver reported some (= bogus) values. So, I can only test the properly
> reported error cases, hopefully someone else can confirm the working case.
> (Michael and Bart, sorry for double post)
> 
>  drivers/i2c/chips/pcf8575.c |   29 +++++++++++++++++++++--------
>  1 files changed, 21 insertions(+), 8 deletions(-)

This driver is deprecated and you should be using
drivers/gpio/pcf857x.c instead if you can. That driver properly handles
I/O errors as far as I can see.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 423 bytes --]

On Thu, May 15, 2008 at 08:54:16PM +0200, Jean Delvare wrote:

> This driver is deprecated and you should be using
> drivers/gpio/pcf857x.c instead if you can.
Well, sadly , I cannot :) The whole platform is kind of deprecated and
adding gpio-support for it is too much in this case.

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Jean Delvare]

On Thu, 15 May 2008 21:15:59 +0200, Wolfram Sang wrote:
> On Thu, May 15, 2008 at 08:54:16PM +0200, Jean Delvare wrote:
> 
> > This driver is deprecated and you should be using
> > drivers/gpio/pcf857x.c instead if you can.
> Well, sadly , I cannot :) The whole platform is kind of deprecated and
> adding gpio-support for it is too much in this case.

What will you do when I delete drivers/i2c/chips/pcf8575.c in kernel
2.6.28?

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Jean Delvare]

Hi Wolfram,

On Thu, 15 May 2008 16:35:27 +0200, Wolfram Sang wrote:
> Make the driver check and report error cases when reading from or writing to
> the chip. It used to assume 'always success' and could even deliver bogus
> values.
> 
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>
> ---
> 
> The PCF on my system seems broken, it does not respond (and I can't probe as my
> master doesn't support smbus_quick). The broken chip went unnoticed as the
> driver reported some (= bogus) values. So, I can only test the properly
> reported error cases, hopefully someone else can confirm the working case.
> (Michael and Bart, sorry for double post)
> 
>  drivers/i2c/chips/pcf8575.c |   29 +++++++++++++++++++++--------
>  1 files changed, 21 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/i2c/chips/pcf8575.c b/drivers/i2c/chips/pcf8575.c
> index 3ea08ac..4a4247d 100644
> --- a/drivers/i2c/chips/pcf8575.c
> +++ b/drivers/i2c/chips/pcf8575.c
> @@ -67,15 +67,22 @@ static ssize_t show_read(struct device *dev, struct device_attribute *attr,
>  			 char *buf)
>  {
>  	struct i2c_client *client = to_i2c_client(dev);
> +	int status, retval = -EIO;
>  	u16 val;
>  	u8 iopin_state[2];
>  
> -	i2c_master_recv(client, iopin_state, 2);
> +	status = i2c_master_recv(client, iopin_state, 2);
>  
> -	val = iopin_state[0];
> -	val |= iopin_state[1] << 8;
> +	if (status == 2) {
> +		val = iopin_state[0];
> +		val |= iopin_state[1] << 8;
> +		return sprintf(buf, "%u\n", val);
> +	}
> +
> +	if (status < 0)
> +	    retval = status;
>  
> -	return sprintf(buf, "%u\n", val);
> +	return retval;
>  }

By handling the errors returned by i2c_master_recv() and returning
quicly, I believe that you should be able to make a much smaller patch,
in particular no need to reindent the code and no need for 2 additional
local variables.

>  
>  static DEVICE_ATTR(read, S_IRUGO, show_read, NULL);
> @@ -95,19 +102,25 @@ static ssize_t set_write(struct device *dev, struct device_attribute *attr,
>  	struct i2c_client *client = to_i2c_client(dev);
>  	struct pcf8575_data *data = i2c_get_clientdata(client);
>  	unsigned long val = simple_strtoul(buf, NULL, 10);
> +	int status, retval = -EIO;
>  	u8 iopin_state[2];
>  
>  	if (val > 0xffff)
>  		return -EINVAL;
>  
> -	data->write = val;
> -
>  	iopin_state[0] = val & 0xFF;
>  	iopin_state[1] = val >> 8;
>  
> -	i2c_master_send(client, iopin_state, 2);
> +	status = i2c_master_send(client, iopin_state, 2);
> +
> +	if (status == 2)
> +		retval = count;
> +	else if (status < 0)
> +		retval = status;
> +
> +	data->write = (status == 2) ? val : retval;
>  
> -	return count;
> +	return retval;
>  }

While not as obvious as for the read case, here too I think that you
could make it slightly more simple. The fact that you have to test for
status == 2 twice while it is the most likely case, doesn't look good.

Another note: setting data->write to an error value may not be the best
thing to do. How does the chip react if write to it fails for some
reason? My guess is that the write will be ignored and the output will
stay whatever it was before. If I am right then leaving data->write
unchanged is the right thing to do on write error.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Wolfram Sang]

[-- Attachment #1.1: Type: text/plain, Size: 321 bytes --]

On Thu, May 15, 2008 at 09:21:08PM +0200, Jean Delvare wrote:

> What will you do when I delete drivers/i2c/chips/pcf8575.c in kernel
> 2.6.28?
Ask the customer for another budget :)

   Wolfram

-- 
  Dipl.-Ing. Wolfram Sang | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 189 bytes --]

[-- Attachment #2: Type: text/plain, Size: 157 bytes --]

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Bart Van Assche]

On Thu, May 15, 2008 at 4:29 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Make the driver check and report error cases when reading from or writing to
> the chip. It used to assume 'always success' and could even deliver bogus
> values.
>
> Signed-off-by: Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org>

It's definitely an improvement that the return value of i2c_master_*()
is now checked -- I did not have any defective PCF8575 devices around
when I submitted the PCF8575 patch. Can you please add one or two
lines to Documentation/i2c/chips/pcf8575 that explain what happens to
the /sys variables in case of a read or write error ?

Thanks,

Bart.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Bart Van Assche]

On Thu, May 15, 2008 at 4:29 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> Make the driver check and report error cases when reading from or writing to
> the chip. It used to assume 'always success' and could even deliver bogus
> values.

Hello Wolfram,

Do you have the time to update your patch with the formulated remarks ?

Bart.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH] i2c-pcf8575: check for errors when accessing the chip

[Robert Schwebel]

Hi Bart,

On Thu, May 22, 2008 at 01:14:00PM +0200, Bart Van Assche wrote:
> On Thu, May 15, 2008 at 4:29 PM, Wolfram Sang <w.sang-bIcnvbaLZ9MEGnE8C9+IrQ@public.gmane.org> wrote:
> > Make the driver check and report error cases when reading from or writing to
> > the chip. It used to assume 'always success' and could even deliver bogus
> > values.
> 
> Hello Wolfram,
> 
> Do you have the time to update your patch with the formulated remarks ?

Wolfram is on holiday for a week, so expect some latencies :-)

rsc
-- 
 Dipl.-Ing. Robert Schwebel | http://www.pengutronix.de
 Pengutronix - Linux Solutions for Science and Industry
   Handelsregister:  Amtsgericht Hildesheim, HRA 2686
     Hannoversche Str. 2, 31134 Hildesheim, Germany
   Phone: +49-5121-206917-0 |  Fax: +49-5121-206917-9


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c