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
> @@ -106,8 +106,11 @@ static int ali1563_transaction(struct i2
> }
>
> /* device error - no response, ignore the autodetection case */
> - if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
> - dev_err(&a->dev, "Device error!\n");
> + if (data & HST_STS_DEVERR) {
> + if (size != HST_CNTL2_QUICK)
> + dev_err(&a->dev, "Device error!\n");
> + else
> + return -ENXIO;
> }
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.
>
> /* bus collision */
> @@ -122,13 +125,14 @@ static int ali1563_transaction(struct i2
> outb_p(0x0,SMB_HST_CNTL2);
> }
>
> - return -1;
> + return -EIO;
> }
>
> static int ali1563_block_start(struct i2c_adapter * a)
> {
> u32 data;
> int timeout;
> + int status = -EIO;
>
> dev_dbg(&a->dev, "Block (pre): STS=%02x, CNTL1=%02x, "
> "CNTL2=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
> @@ -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".
> +
> 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.
> 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()?
> --- 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.
All the rest looks fine now. BTW, I can test i2c-piix4, i2c-i801,
i2c-viapro and i2c-nforce2 here, and will do soon.
--
Jean Delvare
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
