[i2c] [PATCH for 2.6.18-mm3][RFC] cleanup of i2c-nforce2, 2nd iteration

Jean Delvare khali at linux-fr.org
Wed Oct 11 17:27:13 CEST 2006


Hans-Frieder,

> as announced in my mail that I wrote a few minutes ago, here is another patch 
> to fix and cleanup the i2c-nforce2 I2C bus driver. This patch introduces a 
> fix and generally cleans up the driver (thanks to Jean for his suggestions 
> and support). However, the main feature is an enabled block data transfer, 
> that allows up to 31 (unfortunately not 32) byte transfer.
> I have created the patch against 2.6.18-mm3, but of course I can provide it 
> also against 2.6.19-mm1, if not this is the same anyway.

Your mailer wraps long lines, I couldn't apply your patch. Please fix.

> Here is a summary of the changes:
> 
> - fixes:
>    o legacy I/O region size is 64 bytes, not 8 bytes
> - general cleanup:
>    o removed code for the unsupported I2C block transfer mode
>    o removed detail warnings about unsupported modes that are
>      covered in a general warning (unsupported transaction...)
>      anyway
>    o removed necessity of a definition of struct i2c_adapter
>    o moved definition of struct i2c_algorithm, making forward
>      declarations of nforce2_access and nforce2_func unnecessary
>    o introduces new function nforce2_check_status to better structure the       
>       status checking and to avoid duplication of code
>    o changed algorithm for status checking to make it less rigid and to enable 
>       distinguishing between failed transfers and timeouts (although the 
>       consequences are at the moment the same: a freezing SMBus)
>    o introduce a check for the block data transfer length and gracefully fail 
>       for requested transfer lengths <= 0 and > 31. This is to avoid a 
>       freezing SMBus for these cases
> - minor changes:
>    o changes my e-mail address in MODULE_AUTHOR

This starts being a rather long list of changes, so I'd rather not add
more changes to the patch already in the works. Instead the additional
changes could be moved to a different patch.

> --- linux-2.6.18-mm3/drivers/i2c/busses/i2c-nforce2.c	2006-10-08 13:10:38.177378328 +0200
> +++ linux-2.6.18-mm3-test/drivers/i2c/busses/i2c-nforce2.c	2006-10-08 23:58:47.466748493 +0200
> @@ -30,12 +30,12 @@
>      nForce3 Pro150 MCP		00D4
>      nForce3 250Gb MCP		00E4
>      nForce4 MCP			0052
> -    nForce4 MCP-04		0034
> -    nForce4 MCP51		0264
> -    nForce4 MCP55		0368
> +    nForce MCP04		0034
> +    nForce MCP51		0264
> +    nForce MCP55		0368
>  
>      This driver supports the 2 SMBuses that are included in the MCP of the
> -    nForce2/3/4 chipsets.
> +    nForce2/3/4/5xx chipsets.
>  */

These changes are not listed above and I find them confusing. What's
wrong with the current list?

>  
>  /* Note: we assume there can only be one nForce2, with two SMBus interfaces 
> */
> @@ -52,8 +52,8 @@
>  #include <asm/io.h>
>  
>  MODULE_LICENSE("GPL");
> -MODULE_AUTHOR ("Hans-Frieder Vogt <hfvogt at arcor.de>");
> -MODULE_DESCRIPTION("nForce2 SMBus driver");
> +MODULE_AUTHOR ("Hans-Frieder Vogt <hfvogt at gmx.net>");
> +MODULE_DESCRIPTION("nForce2/3/4/5xx SMBus driver");
>  
>  
>  struct nforce2_smbus {
> @@ -98,29 +98,37 @@ struct nforce2_smbus {
>  #define NVIDIA_SMB_PRTCL_BLOCK_DATA		0x0a
>  #define NVIDIA_SMB_PRTCL_PROC_CALL		0x0c
>  #define NVIDIA_SMB_PRTCL_BLOCK_PROC_CALL	0x0d
> -#define NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA		0x4a
>  #define NVIDIA_SMB_PRTCL_PEC			0x80
>  
> +/* Misc definitions */
> +#define MAX_TIMEOUT	1000
> +
> +
>  static struct pci_driver nforce2_driver;
>  
> -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);
> -static u32 nforce2_func(struct i2c_adapter *adapter);
>  
> +static int nforce2_check_status (struct i2c_adapter * adap) {
> +	struct nforce2_smbus *smbus = adap->algo_data;
> +	int timeout = 0;
> +	unsigned char temp;
>  
> -static const struct i2c_algorithm smbus_algorithm = {
> -	.smbus_xfer = nforce2_access,
> -	.functionality = nforce2_func,
> -};
> +	do {
> +		msleep(1);
> +		temp = inb_p(NVIDIA_SMB_STS);
> +	} while ((!temp) && (timeout++ < MAX_TIMEOUT));
> +
> +	if ((~temp & NVIDIA_SMB_STS_DONE) && (timeout >= MAX_TIMEOUT)) {
> +		dev_dbg(&adap->dev, "SMBus Timeout (0x%02x)!\n", temp);
> +		return -1;
> +	}
> +	if ((~temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
> +		dev_dbg(&adap->dev, "Transaction failed (0x%02x)!\n", temp);
> +		return -1;
> +	}
> +	return 0;
> +}

This one should be a patch on its own.

>  
> -static struct i2c_adapter nforce2_adapter = {
> -	.owner          = THIS_MODULE,
> -	.class          = I2C_CLASS_HWMON,
> -	.algo           = &smbus_algorithm,
> -};
>  
> -/* Return -1 on error. See smbus.h for more information */

Only the second part of this comment it wrong.

>  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)
> @@ -164,34 +172,42 @@ static s32 nforce2_access(struct i2c_ada
>  			break;
>  
>  		case I2C_SMBUS_BLOCK_DATA:
> -			outb_p(command, NVIDIA_SMB_CMD);
>  			if (read_write == I2C_SMBUS_WRITE) {
> -				len = min_t(u8, data->block[0], 32);
> +				outb_p(command, NVIDIA_SMB_CMD);
> +				len = data->block[0];
> +				/* nforce2+ SMBus controller seems to support
> +				   only lengths between 1 and 31 */
> +				if ((len == 0) || (len > 0x1f)) {
> +					dev_err(&adap->dev, "Transaction failed (requested block size: 0x%02x)\n", len);

Line too long.

> +					return -1;
> +				}
>  				outb_p(len, NVIDIA_SMB_BCNT);
>  				for (i = 0; i < len; i++)
>  					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> +			} else {
> +				/* Work around to prevent the smbus to block in
> +				   case of requests for blocks sizes with 0 and
> +				   > 31 bytes.
> +				   These requests are still not accepted, but
> +				   at least now they gracefully fail */
> +				outb_p(command, NVIDIA_SMB_CMD);
> +				outb_p((addr & 0x7f) << 1, NVIDIA_SMB_ADDR);
> +				outb_p(NVIDIA_SMB_PRTCL_BYTE_DATA |
> +				       NVIDIA_SMB_PRTCL_READ, NVIDIA_SMB_PRTCL);
> +				if (nforce2_check_status(adap))
> +					return -1;
> +				temp = inb_p(NVIDIA_SMB_DATA);
> +				if ((temp == 0) || (temp > 0x1f)) {
> +					dev_err(&adap->dev, "Transaction failed (requested block size: 
> 0x%02x)\n", temp);
> +					return -1;
> +				}
> +				/* Work around finished, therefore continue with
> +				   requested function */
> +				outb_p(command, NVIDIA_SMB_CMD);
>  			}

Eeek, what a mess. If I understand correctly, you are doing a 1-byte
data read first, in the hope that the chip will give your the block
length, then if the length is ok you do the real N-byte data read.
Right?

It's clever, but I just can't accept it, because the first read may
have side effects. For example, a chip could do something everytime a
read is issued to this address - and the thing would happen twice
instead of once. Or a chip could adjust some internal pointer on every
transaction, much like EEPROMs do. Furthermore, you can't be sure that
a chip will always return the same number of bytes for a block read
from that address. The actual length might depend on some changing
state.

So all in all I fear that this workaround is more likely to cause
trouble than the original problem was.

>  			protocol |= NVIDIA_SMB_PRTCL_BLOCK_DATA | pec;
>  			break;
>  
> -		case I2C_SMBUS_I2C_BLOCK_DATA:
> -			len = min_t(u8, data->block[0], 32);
> -			outb_p(command, NVIDIA_SMB_CMD);
> -			outb_p(len, NVIDIA_SMB_BCNT);
> -			if (read_write == I2C_SMBUS_WRITE)
> -				for (i = 0; i < len; i++)
> -					outb_p(data->block[i + 1], NVIDIA_SMB_DATA+i);
> -			protocol |= NVIDIA_SMB_PRTCL_I2C_BLOCK_DATA;
> -			break;
> -
> -		case I2C_SMBUS_PROC_CALL:
> -			dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> -			return -1;
> -
> -		case I2C_SMBUS_BLOCK_PROC_CALL:
> -			dev_err(&adap->dev, "I2C_SMBUS_BLOCK_PROC_CALL not supported!\n");
> -			return -1;
> -
>  		default:
>  			dev_err(&adap->dev, "Unsupported transaction %d\n", size);
>  			return -1;
> @@ -200,21 +216,8 @@ static s32 nforce2_access(struct i2c_ada
>  	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;
> -	}
>  
>  	if (read_write == I2C_SMBUS_WRITE)
>  		return 0;
> @@ -227,15 +230,11 @@ static s32 nforce2_access(struct i2c_ada
>  			break;
>  
>  		case I2C_SMBUS_WORD_DATA:
> -		/* case I2C_SMBUS_PROC_CALL: not supported */
>  			data->word = inb_p(NVIDIA_SMB_DATA) | (inb_p(NVIDIA_SMB_DATA+1) << 8);
>  			break;
>  
>  		case I2C_SMBUS_BLOCK_DATA:
> -		/* case I2C_SMBUS_BLOCK_PROC_CALL: not supported */
> -			len = inb_p(NVIDIA_SMB_BCNT);
> -			len = min_t(u8, len, 32);
> -		case I2C_SMBUS_I2C_BLOCK_DATA:
> +			len = inb_p(NVIDIA_SMB_BCNT) & 0x1f;
>  			for (i = 0; i < len; i++)
>  				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
>  			data->block[0] = len;
> @@ -250,10 +249,14 @@ static u32 nforce2_func(struct i2c_adapt
>  {
>  	/* other functionality might be possible, but is not tested */
>  	return I2C_FUNC_SMBUS_QUICK | I2C_FUNC_SMBUS_BYTE |
> -	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA /* |
> -	    I2C_FUNC_SMBUS_BLOCK_DATA */;
> +	    I2C_FUNC_SMBUS_BYTE_DATA | I2C_FUNC_SMBUS_WORD_DATA |
> +	    I2C_FUNC_SMBUS_BLOCK_DATA;
>  }
>  
> +static struct i2c_algorithm smbus_algorithm = {
> +	.smbus_xfer = nforce2_access,
> +	.functionality = nforce2_func,
> +};
>  
>  static struct pci_device_id nforce2_ids[] = {
>  	{ PCI_DEVICE(PCI_VENDOR_ID_NVIDIA, PCI_DEVICE_ID_NVIDIA_NFORCE2_SMBUS) },
> @@ -291,7 +294,7 @@ static int __devinit nforce2_probe_smb (
>  		}
>  
>  		smbus->base = iobase & PCI_BASE_ADDRESS_IO_MASK;
> -		smbus->size = 8;
> +		smbus->size = 64;
>  	}
>  	smbus->dev = dev;
>  
> @@ -300,7 +303,9 @@ static int __devinit nforce2_probe_smb (
>  			smbus->base, smbus->base+smbus->size-1, name);
>  		return -1;
>  	}
> -	smbus->adapter = nforce2_adapter;
> +	smbus->adapter.owner = THIS_MODULE;
> +	smbus->adapter.class = I2C_CLASS_HWMON;
> +	smbus->adapter.algo = &smbus_algorithm;
>  	smbus->adapter.algo_data = smbus;
>  	smbus->adapter.dev.parent = &dev->dev;
>  	snprintf(smbus->adapter.name, I2C_NAME_SIZE,

As a conclusion:
* I am OK with the SMBus block _write_ workaround if you want it in.
* I am _not_ OK with the SMBus block _read_ workaround, it's too
  complex and too risky.
* Please make the introduction of nforce2_check_status() a separate
  patch if you still want it in although it'll only be called once.

Thanks,
-- 
Jean Delvare



More information about the i2c mailing list