[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