Hi David,
On Sat, 17 May 2008 17:54:15 -0700, David Brownell wrote:
> On Thursday 15 May 2008, Jean Delvare wrote:
> > Hi David,
> > On Mon, 12 May 2008 09:43:04 -0700, David Brownell wrote:
> > > @@ -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.
This doesn't have anything to do with the hardware. The poll loop is:
timeout = ALI1563_MAX_TIMEOUT;
do
msleep(1);
while (!((data = inb_p(SMB_HST_STS)) & HST_STS_DONE) && --timeout);
Regardless of what the hardware does, it is simply impossible to have
timeout == 0 if you don't have !(data & HST_STS_DONE), because you
wouldn't decrease timeout if (data & HST_STS_DONE). This, testing for
just timeout == 0 after this loop is equivalent to testing for timeout
== 0 && !(data & HST_STS_DONE). As a matter of fact, the driver only
tests for timeout == 0 in ali1563_transaction() (although it doesn't
return -ETIMEDOUT there, we probably should fix that.)
> > > 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 ... ;)
The problem is that in ali1563_transaction() we map (data &
HST_STS_DEVERR) to -ENXIO, so not doing the same in
ali1563_block_start() is somewhat inconsistent.
> 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 */
OK, I've merged that. I'll add a couple minor fixes as discussed above,
and then your patch is ready for linux-next. Thanks!
--
Jean Delvare
_______________________________________________
i2c mailing list
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c
