[i2c] [PATCH 2 of 2] Adding an abort function to unwedge MCP51/55

[Jean Delvare]

Hi Oleg,

On Fri, 31 Aug 2007 00:21:33 -0700, Oleg Ryjkov wrote:
> This patch is to add an abort function that will bring back the MCP51/55
> controller if it was blocked by a block-read operation, in particular.
> (When a slave sends a wrong byte count on a byte read, the host gets
> locked up). I've only tested it on an MCP51 and MCP55. However, I'm
> almost certain it will also work on MCP65, I just did not have the board
> to test it on. Thus I for now the abort function will only be called
> if an MCP51/55 was detected.

Given that we currently allow block transactions only for the MCP51/55
(why, BTW?) it's fine to only implement the abort capability for these
chips. When we support block transactions for other chips, we will have
to support abort for these as well.

Your patch has many coding-style problems. Please run
scripts/checkpatch.pl, address the reported problems, and resend your
patch.

Do you think there's any chance that the abort feature will work on my
nForce2? I could test your patch there.

> 
> Signed-off-by: Oleg Ryjkov <olegr at google.com>
> 
> 
> --- linux-2.6-untouched/drivers/i2c/busses/i2c-nforce2.c	2007-08-30 23:30:02.000000000 -0700
> +++ linux-2.6/drivers/i2c/busses/i2c-nforce2.c	2007-08-30 23:45:10.000000000 -0700
> @@ -62,6 +62,7 @@
>  	int base;
>  	int size;
>  	int blockops;
> +	int can_abort;
>  };
>  
>  
> @@ -83,6 +84,14 @@
>  #define NVIDIA_SMB_DATA		(smbus->base + 0x04)	/* 32 data registers */
>  #define NVIDIA_SMB_BCNT		(smbus->base + 0x24)	/* number of data
>  							   bytes */
> +#define NVIDIA_SMB_STATUS_ABRT	(smbus->base + 0x3c)	/* register used to
> +							   reset controller */

Comment doesn't look right, this register is used to verify that the
reset was successful, not to do the reset itself?

> +#define NVIDIA_SMB_CTRL		(smbus->base + 0x3e)	/* control register */
> +
> +#define NVIDIA_SMB_STATUS_ABRT_STS	0x01            /* Bit to notify that
> +                                                           abort succeeded */
> +#define NVIDIA_SMB_CTRL_ABORT	0x20                    /* Bit used to issue an
> +                                                           abort command */

The name of the define is rather explicit, you can probably omit the
comment for this one.

>  
>  #define NVIDIA_SMB_STS_DONE	0x80
>  #define NVIDIA_SMB_STS_ALRM	0x40
> @@ -103,6 +112,25 @@
>  
>  static struct pci_driver nforce2_driver;
>  
> +static void nforce2_abort(struct i2c_adapter * adap) {
> +	struct nforce2_smbus *smbus = adap->algo_data;
> +	int timeout = 0;
> +	unsigned char temp;
> +
> +	dev_dbg(&adap->dev, "Aborting current transaction\n");
> +
> +	outb_p(NVIDIA_SMB_CTRL_ABORT, NVIDIA_SMB_CTRL);
> +	timeout = 0;

Double initialization.

> +	do {
> +		msleep(1);
> +		temp = inb_p(NVIDIA_SMB_STATUS_ABRT);
> +	} while ( !(temp & NVIDIA_SMB_STATUS_ABRT_STS) &&
> +			(timeout++ < MAX_TIMEOUT));
> +	if ((temp & NVIDIA_SMB_STATUS_ABRT_STS)==0)

Should be written !(temp & NVIDIA_SMB_STATUS_ABRT_STS) for consistency.

> +		dev_dbg(&adap->dev, "Can't reset the smbus\n");

This probably deserves a dev_err(). After all the SMBus is stuck until
next reboot, the user might want to know.

> +	outb_p(NVIDIA_SMB_STATUS_ABRT_STS, NVIDIA_SMB_STATUS_ABRT);
> +}
> +
>  static int nforce2_check_status(struct i2c_adapter * adap) {
>  	struct nforce2_smbus *smbus = adap->algo_data;
>  	int timeout = 0;
> @@ -115,6 +143,8 @@
>  
>  	if ((~temp & NVIDIA_SMB_STS_DONE) && (timeout >= MAX_TIMEOUT)) {
>  		dev_dbg(&adap->dev, "SMBus Timeout (0x%02x)!\n", temp);
> +		if (smbus->can_abort == 1)

Preferably written (smbus->can_abort).

> +			nforce2_abort(adap);
>  		return -1;
>  	}
>  	if ((~temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
> @@ -324,6 +354,8 @@
>  	case PCI_DEVICE_ID_NVIDIA_NFORCE_MCP55_SMBUS:
>  		smbuses[0].blockops = 1;
>  		smbuses[1].blockops = 1;
> +		smbuses[0].can_abort = 1;
> +		smbuses[1].can_abort = 1;
>  	}
>  
>  	/* SMBus adapter 1 */

Overall this patch looks very good, thanks for doing this. Making sure
that a bad block transaction won't lock the SMBus forever is a very
good thing!

-- 
Jean Delvare