[i2c] [PATCH 1 of 2] Clean up of i2c-nforce2

[Jean Delvare]

Hi Oleg,

Sorry for the late answer.

On Fri, 31 Aug 2007 00:15:16 -0700, Oleg Ryjkov wrote:
> This is the first part of the patch that adds a function to reset the
> nvidia MCP51/55 i2c controller, if something bad happens to it (i.e.
> a slave sends a wrong byte count during a block transaction).
> 
> This patch just adds nforce2_check_status function. It was originally
> written by Hans-Frieder Vogt <hfvogt at gmx.net>, and was posted on this
> list around October 2006 (see
> http://lists.lm-sensors.org/pipermail/i2c/2006-October/000380.html).
> However, unfortunately, he never got around to submitting it to the
> kernel.
> The reason that I'm the one sending it is is:
> - I relied on it for the second part of the patch,
> - It makes the driver code cleaner/better.

I agree that it makes the code nicer. For HZ >= 250, it also makes most
transactions faster. It makes the short transactions slower though
(those that were fitting in the udelay(500)) for all values of HZ but
that's probably not a big problem on practice.

> 
> Signed-off-by: Oleg Ryjkov <olegr at google.com>
> 
> --- linux-2.6-untouched/drivers/i2c/busses/i2c-nforce2.c	2007-08-13 18:28:26.000000000 -0700
> +++ linux-2.6/drivers/i2c/busses/i2c-nforce2.c	2007-08-30 19:38:13.000000000 -0700
> @@ -98,15 +98,39 @@
>  #define NVIDIA_SMB_PRTCL_BLOCK_DATA		0x0a
>  #define NVIDIA_SMB_PRTCL_PEC			0x80
>  
> +/* Misc definitions */
> +#define MAX_TIMEOUT	100
> +
>  static struct pci_driver nforce2_driver;
>  
> +static int nforce2_check_status(struct i2c_adapter * adap) {

Coding style: opening curly brace goes at the beginning of the next
line.

> +	struct nforce2_smbus *smbus = adap->algo_data;
> +	int timeout = 0;
> +	unsigned char temp;
> +
> +	do {
> +		msleep(1);
> +		temp = inb_p(NVIDIA_SMB_STS);
> +	} while ((!temp) && (timeout++ < MAX_TIMEOUT));
> +
> +	if ((~temp & NVIDIA_SMB_STS_DONE) && (timeout >= MAX_TIMEOUT)) {

(~temp & NVIDIA_SMB_STS_DONE) is better written
!(temp & NVIDIA_SMB_STS_DONE).

I think that this test can be simplified. Checking for the DONE bit not
being set should be enough. If that bit was set, then the while loop
would have exited before the timeout. Or am I missing something?

> +		dev_dbg(&adap->dev, "SMBus Timeout (0x%02x)!\n", temp);
> +		return -1;
> +	}
> +	if ((~temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {

Then this test can be simplified as well: no need to check again for
the DONE bit not being set, as you checked this above already.

> +		dev_dbg(&adap->dev, "Transaction failed (0x%02x)!\n", temp);
> +		return -1;
> +	}
> +	return 0;
> +}
> +
>  /* Return -1 on error */
>  static s32 nforce2_access(struct i2c_adapter * adap, u16 addr,
>  		unsigned short flags, char read_write,
>  		u8 command, int size, union i2c_smbus_data * data)
>  {
>  	struct nforce2_smbus *smbus = adap->algo_data;
> -	unsigned char protocol, pec, temp;
> +	unsigned char protocol, pec;
>  	u8 len;
>  	int i;
>  
> @@ -170,22 +194,9 @@
>  	outb_p((addr & 0x7f) << 1, NVIDIA_SMB_ADDR);
>  	outb_p(protocol, NVIDIA_SMB_PRTCL);
>  
> -	temp = inb_p(NVIDIA_SMB_STS);
> -
> -	if (~temp & NVIDIA_SMB_STS_DONE) {
> -		udelay(500);
> -		temp = inb_p(NVIDIA_SMB_STS);
> -	}
> -	if (~temp & NVIDIA_SMB_STS_DONE) {
> -		msleep(10);
> -		temp = inb_p(NVIDIA_SMB_STS);
> -	}
> -
> -	if ((~temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
> -		dev_dbg(&adap->dev, "SMBus Timeout! (0x%02x)\n", temp);
> +	if (nforce2_check_status(adap))
>  		return -1;
> -	}
> -
> +        

You patch is introducing trailing white space here, please clean up.

>  	if (read_write == I2C_SMBUS_WRITE)
>  		return 0;
>  
> @@ -202,7 +213,12 @@
>  
>  		case I2C_SMBUS_BLOCK_DATA:
>  			len = inb_p(NVIDIA_SMB_BCNT);
> -			len = min_t(u8, len, I2C_SMBUS_BLOCK_MAX);
> +			if ((len <= 0) || (len > I2C_SMBUS_BLOCK_MAX)) {
> +				dev_err(&adap->dev, "Transaction failed"

Missing space at end of string.

> +					"(requested block size: 0x%02x)"
> +					"\n", len);

Please move the \n at the end of the previous line.

> +				return -1;
> +			}
>  			for (i = 0; i < len; i++)
>  				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
>  			data->block[0] = len;


-- 
Jean Delvare