Re: [patch 2.6.25-git] i2c_adapters: return -Errno not -1

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

On Thursday 15 May 2008, Jean Delvare wrote:
> Hi David,
> 
> On Mon, 12 May 2008 09:43:04 -0700, David Brownell wrote:
> > --- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:01.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:08.000000000 -0700
> > 	...
> 
> This one doesn't look right. You should return -ENXIO in all cases (but
> only print the error message on size != HST_CNTL2_QUICK.) The right way
> to do this, I think, would be to move this specific check at the end of
> the function, so that the other error checks are still done in all
> cases.

OK, done ...


> > @@ -164,13 +168,17 @@ static int ali1563_block_start(struct i2
> >  
> >  	if (timeout && !(data & HST_STS_BAD))
> >  		return 0;
> > +
> > +	if (timeout == 0 && !(data & HST_STS_DONE))
> > +		status = -ETIMEDOUT;
> 
> That's pretty strange to check for both timeout == 0 and !(data &
> HST_STS_DONE), given that the former implies the latter if I read the
> code correctly. The same strangeness is present in the code below, if
> we print "Timeout" then we will also print "Transaction Never Finished".

Without chip docs, and knowing that it overloads lots of fault
cases into not enough bits, I wouldn't rely on such conclusions.
Instead, I just tried to make sure the ETIMEDOUT means exactly
what is promised in the faultcode writeup.


> > +
> >  	dev_err(&a->dev, "SMBus Error: %s%s%s%s%s\n",
> >  		timeout ? "Timeout " : "",
> 
> BTW, isn't there a bug here? I think the test should be timeout == 0.

Right, fixed.

> >  		data & HST_STS_FAIL ? "Transaction Failed " : "",
> >  		data & HST_STS_BUSERR ? "No response or Bus Collision " : "",
> >  		data & HST_STS_DEVERR ? "Device Error " : "",
> >  		!(data & HST_STS_DONE) ? "Transaction Never Finished " : "");
> > -	return -1;
> > +	return status;
> >  }
> 
> I thought we had agreed that we would return -ENXIO for HST_STS_DEVERR,
> as we already do in ali1563_transaction()?

I thought we'd agreed to not play guessing games about the behavior
of chips we don't have docs for ... ;)

 
> > --- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:01.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:08.000000000 -0700
> > @@ -152,7 +152,7 @@ static int vt596_transaction(u8 size)
> > (...)
> >  	if (temp & 0x04) {
> >  		int read = inb_p(SMBHSTADD) & 0x01;
> > -		result = -1;
> > +		result = -EIO;
> >  		/* The quick and receive byte commands are used to probe
> >  		   for chips, so errors are expected, and we don't want
> >  		   to frighten the user. */
> >  		if (!((size == VT596_QUICK && !read) ||
> >  		      (size == VT596_BYTE && read)))
> >  			dev_err(&vt596_adapter.dev, "Transaction error!\n");
> > +		else
> > +			result = -ENXIO;
> >  	}
> 
> This is not correct, the result is always -ENXIO, whether you display
> an error message or not.

Fixed.


> All the rest looks fine now. BTW, I can test i2c-piix4, i2c-i801,
> i2c-viapro and i2c-nforce2 here, and will do soon.

Appended is a small fixup patch addressing the issues above ...

- Dave


--- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-17 17:53:24.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-17 17:48:01.000000000 -0700
@@ -105,14 +105,6 @@ static int ali1563_transaction(struct i2
 		data = inb_p(SMB_HST_STS);
  	}
 
-	/* device error - no response, ignore the autodetection case */
-	if (data & HST_STS_DEVERR) {
-		if (size != HST_CNTL2_QUICK)
-			dev_err(&a->dev, "Device error!\n");
-		else
-			return -ENXIO;
-	}
-
 	/* bus collision */
 	if (data & HST_STS_BUSERR) {
 		dev_err(&a->dev, "Bus collision!\n");
@@ -125,6 +117,13 @@ static int ali1563_transaction(struct i2
 		outb_p(0x0,SMB_HST_CNTL2);
 	}
 
+	/* device error - no response, ignore the autodetection case */
+	if (data & HST_STS_DEVERR) {
+		if (size != HST_CNTL2_QUICK)
+			dev_err(&a->dev, "Device error!\n");
+		return -ENXIO;
+	}
+
 	return -EIO;
 }
 
@@ -173,7 +172,7 @@ static int ali1563_block_start(struct i2
 		status = -ETIMEDOUT;
 
 	dev_err(&a->dev, "SMBus Error: %s%s%s%s%s\n",
-		timeout ? "Timeout " : "",
+		timeout ? "" : "Timeout ",
 		data & HST_STS_FAIL ? "Transaction Failed " : "",
 		data & HST_STS_BUSERR ? "No response or Bus Collision " : "",
 		data & HST_STS_DEVERR ? "Device Error " : "",
--- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-17 17:53:24.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-17 17:45:52.000000000 -0700
@@ -184,15 +184,14 @@ static int vt596_transaction(u8 size)
 
 	if (temp & 0x04) {
 		int read = inb_p(SMBHSTADD) & 0x01;
-		result = -EIO;
+
 		/* The quick and receive byte commands are used to probe
 		   for chips, so errors are expected, and we don't want
 		   to frighten the user. */
 		if (!((size == VT596_QUICK && !read) ||
 		      (size == VT596_BYTE && read)))
 			dev_err(&vt596_adapter.dev, "Transaction error!\n");
-		else
-			result = -ENXIO;
+		result = -ENXIO;
 	}
 
 	/* Resetting status register */

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