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

[Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>]

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
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c