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

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

[David Brownell]

Tighten error paths used by various other i2c adapters so they
return real fault/errno codes instead of a literal "-1" (which
is most often interpreted as "-EPERM").  Build tested.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
NOTE there are many other adapter drivers to audit and update; and
they're not just within the drivers/i2c tree.  This patch is a PC
oriented start on some bottom-up improvement of fault reporting.
The i2c-gpio code looks be good too.

 drivers/i2c/algos/i2c-algo-bit.c      |   11 +++++---
 drivers/i2c/busses/i2c-ali1535.c      |   22 +++++++---------
 drivers/i2c/busses/i2c-ali1563.c      |    4 +--
 drivers/i2c/busses/i2c-ali15x3.c      |   19 +++++++-------
 drivers/i2c/busses/i2c-amd756-s4882.c |    4 +--
 drivers/i2c/busses/i2c-amd756.c       |   18 +++++++------
 drivers/i2c/busses/i2c-amd8111.c      |   44 +++++++++++++++++++++------------
 drivers/i2c/busses/i2c-i801.c         |   45 +++++++++++++++++-----------------
 drivers/i2c/busses/i2c-nforce2.c      |   25 +++++++++---------
 drivers/i2c/busses/i2c-piix4.c        |   20 ++++++++-------
 drivers/i2c/busses/i2c-sis5595.c      |   19 ++++++++------
 drivers/i2c/busses/i2c-sis630.c       |   43 ++++++++++++++++++--------------
 drivers/i2c/busses/i2c-sis96x.c       |   20 ++++++++-------
 drivers/i2c/busses/i2c-stub.c         |    4 +--
 drivers/i2c/busses/i2c-viapro.c       |   20 ++++++++-------
 15 files changed, 176 insertions(+), 142 deletions(-)

--- g26.orig/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:02:58.000000000 -0700
@@ -317,10 +317,10 @@ bailout:
  * -x transmission error
  */
 static int try_address(struct i2c_adapter *i2c_adap,
-		       unsigned char addr, int retries)
+		       unsigned char addr, unsigned retries)
 {
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
-	int i, ret = -1;
+	int i, ret;
 
 	for (i = 0; i <= retries; i++) {
 		ret = i2c_outb(i2c_adap, addr);
@@ -465,9 +465,12 @@ static int bit_doAddress(struct i2c_adap
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 
 	unsigned char addr;
-	int ret, retries;
+	int ret;
+	unsigned retries;
 
-	retries = nak_ok ? 0 : i2c_adap->retries;
+	retries = (nak_ok || i2c_adap->retries < 0)
+			? 0
+			: i2c_adap->retries;
 
 	if (flags & I2C_M_TEN) {
 		/* a ten bit address */
--- g26.orig/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:01:39.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:02:58.000000000 -0700
@@ -259,7 +259,7 @@ static int ali1535_transaction(struct i2
 			dev_err(&adap->dev,
 				"SMBus reset failed! (0x%02x) - controller or "
 				"device on bus is probably hung\n", temp);
-			return -1;
+			return -EBUSY;
 		}
 	} else {
 		/* check and clear done bit */
@@ -281,12 +281,12 @@ static int ali1535_transaction(struct i2
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&adap->dev, "SMBus Timeout!\n");
 	}
 
 	if (temp & ALI1535_STS_FAIL) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
 	}
 
@@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2
 	 * do a printk.  This means that bus collisions go unreported.
 	 */
 	if (temp & ALI1535_STS_BUSERR) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev,
 			"Error: no response or bus collision ADD=%02x\n",
 			inb_p(SMBHSTADD));
@@ -303,13 +303,13 @@ static int ali1535_transaction(struct i2
 
 	/* haven't ever seen this */
 	if (temp & ALI1535_STS_DEV) {
-		result = -1;
+		result = -EIO;
 		dev_err(&adap->dev, "Error: device error\n");
 	}
 
 	/* check to see if the "command complete" indication is set */
 	if (!(temp & ALI1535_STS_DONE)) {
-		result = -1;
+		result = -EIO;
 		dev_err(&adap->dev, "Error: command never completed\n");
 	}
 
@@ -332,7 +332,7 @@ static int ali1535_transaction(struct i2
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 ali1535_access(struct i2c_adapter *adap, u16 addr,
 			  unsigned short flags, char read_write, u8 command,
 			  int size, union i2c_smbus_data *data)
@@ -359,7 +359,7 @@ static s32 ali1535_access(struct i2c_ada
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-		result = -1;
+		result = -EOPNOTSUPP;
 		goto EXIT;
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
@@ -420,11 +420,9 @@ static s32 ali1535_access(struct i2c_ada
 		break;
 	}
 
-	if (ali1535_transaction(adap)) {
-		/* Error in transaction */
-		result = -1;
+	result = ali1535_transaction(adap);
+	if (result)
 		goto EXIT;
-	}
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == ALI1535_QUICK)) {
 		result = 0;
--- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-01 16:01:39.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-01 16:02:58.000000000 -0700
@@ -122,7 +122,7 @@ static int ali1563_transaction(struct i2
 		outb_p(0x0,SMB_HST_CNTL2);
 	}
 
-	return -1;
+	return -EIO;
 }
 
 static int ali1563_block_start(struct i2c_adapter * a)
@@ -170,7 +170,7 @@ static int ali1563_block_start(struct i2
 		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 -EIO;
 }
 
 static int ali1563_block(struct i2c_adapter * a, union i2c_smbus_data * data, u8 rw)
--- g26.orig/drivers/i2c/busses/i2c-ali15x3.c	2008-05-01 16:01:39.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali15x3.c	2008-05-01 16:02:58.000000000 -0700
@@ -282,7 +282,7 @@ static int ali15x3_transaction(struct i2
 			dev_err(&adap->dev, "SMBus reset failed! (0x%02x) - "
 				"controller or device on bus is probably hung\n",
 				temp);
-			return -1;
+			return -EBUSY;
 		}
 	} else {
 		/* check and clear done bit */
@@ -304,12 +304,12 @@ static int ali15x3_transaction(struct i2
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&adap->dev, "SMBus Timeout!\n");
 	}
 
 	if (temp & ALI15X3_STS_TERM) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
 	}
 
@@ -320,7 +320,7 @@ static int ali15x3_transaction(struct i2
 	  This means that bus collisions go unreported.
 	*/
 	if (temp & ALI15X3_STS_COLL) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev,
 			"Error: no response or bus collision ADD=%02x\n",
 			inb_p(SMBHSTADD));
@@ -328,7 +328,7 @@ static int ali15x3_transaction(struct i2
 
 	/* haven't ever seen this */
 	if (temp & ALI15X3_STS_DEV) {
-		result = -1;
+		result = -EIO;
 		dev_err(&adap->dev, "Error: device error\n");
 	}
 	dev_dbg(&adap->dev, "Transaction (post): STS=%02x, CNT=%02x, CMD=%02x, "
@@ -338,7 +338,7 @@ static int ali15x3_transaction(struct i2
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 ali15x3_access(struct i2c_adapter * adap, u16 addr,
 		   unsigned short flags, char read_write, u8 command,
 		   int size, union i2c_smbus_data * data)
@@ -364,7 +364,7 @@ static s32 ali15x3_access(struct i2c_ada
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-		return -1;
+		return -EOPNOTSUPP;
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD);
@@ -421,8 +421,9 @@ static s32 ali15x3_access(struct i2c_ada
 
 	outb_p(size, SMBHSTCNT);	/* output command */
 
-	if (ali15x3_transaction(adap))	/* Error in transaction */
-		return -1;
+	temp = ali15x3_transaction(adap);
+	if (temp)
+		return temp;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == ALI15X3_QUICK))
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:02:58.000000000 -0700
@@ -58,7 +58,7 @@ static s32 amd756_access_virt0(struct i2
 	/* We exclude the multiplexed addresses */
 	if (addr == 0x4c || (addr & 0xfc) == 0x50 || (addr & 0xfc) == 0x30
 	 || addr == 0x18)
-		return -1;
+		return -EINVAL;
 
 	mutex_lock(&amd756_lock);
 
@@ -86,7 +86,7 @@ static inline s32 amd756_access_channel(
 
 	/* We exclude the non-multiplexed addresses */
 	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
-		return -1;
+		return -EINVAL;
 
 	mutex_lock(&amd756_lock);
 
--- g26.orig/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:01:39.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:02:58.000000000 -0700
@@ -151,17 +151,17 @@ static int amd756_transaction(struct i2c
 	}
 
 	if (temp & GS_PRERR_STS) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev, "SMBus Protocol error (no response)!\n");
 	}
 
 	if (temp & GS_COL_STS) {
-		result = -1;
+		result = -EIO;
 		dev_warn(&adap->dev, "SMBus collision!\n");
 	}
 
 	if (temp & GS_TO_STS) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_dbg(&adap->dev, "SMBus protocol timeout!\n");
 	}
 
@@ -189,22 +189,23 @@ static int amd756_transaction(struct i2c
 	outw_p(inw(SMB_GLOBAL_ENABLE) | GE_ABORT, SMB_GLOBAL_ENABLE);
 	msleep(100);
 	outw_p(GS_CLEAR_STS, SMB_GLOBAL_STATUS);
-	return -1;
+	return -EIO;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 amd756_access(struct i2c_adapter * adap, u16 addr,
 		  unsigned short flags, char read_write,
 		  u8 command, int size, union i2c_smbus_data * data)
 {
 	int i, len;
+	int status;
 
 	/** TODO: Should I supporte the 10-bit transfers? */
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_dbg(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
 		/* TODO: Well... It is supported, I'm just not sure what to do here... */
-		return -1;
+		return -EOPNOTSUPP;
 	case I2C_SMBUS_QUICK:
 		outw_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMB_HOST_ADDRESS);
@@ -256,8 +257,9 @@ static s32 amd756_access(struct i2c_adap
 	/* How about enabling interrupts... */
 	outw_p(size & GE_CYC_TYPE_MASK, SMB_GLOBAL_ENABLE);
 
-	if (amd756_transaction(adap))	/* Error in transaction */
-		return -1;
+	status = amd756_transaction(adap);
+	if (status)
+		return status;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == AMD756_QUICK))
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-amd8111.c	2008-05-01 16:01:39.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-amd8111.c	2008-05-01 16:02:58.000000000 -0700
@@ -77,7 +77,7 @@ static unsigned int amd_ec_wait_write(st
 	if (!timeout) {
 		dev_warn(&smbus->dev->dev,
 			 "Timeout while waiting for IBF to clear\n");
-		return -1;
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -93,7 +93,7 @@ static unsigned int amd_ec_wait_read(str
 	if (!timeout) {
 		dev_warn(&smbus->dev->dev,
 			 "Timeout while waiting for OBF to set\n");
-		return -1;
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -102,16 +102,21 @@ static unsigned int amd_ec_wait_read(str
 static unsigned int amd_ec_read(struct amd_smbus *smbus, unsigned char address,
 		unsigned char *data)
 {
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	int status;
+
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(AMD_EC_CMD_RD, smbus->base + AMD_EC_CMD);
 
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(address, smbus->base + AMD_EC_DATA);
 
-	if (amd_ec_wait_read(smbus))
-		return -1;
+	status = amd_ec_wait_read(smbus);
+	if (status)
+		return status;
 	*data = inb(smbus->base + AMD_EC_DATA);
 
 	return 0;
@@ -120,16 +125,21 @@ static unsigned int amd_ec_read(struct a
 static unsigned int amd_ec_write(struct amd_smbus *smbus, unsigned char address,
 		unsigned char data)
 {
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	int status;
+
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(AMD_EC_CMD_WR, smbus->base + AMD_EC_CMD);
 
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(address, smbus->base + AMD_EC_DATA);
 
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(data, smbus->base + AMD_EC_DATA);
 
 	return 0;
@@ -185,6 +195,7 @@ static s32 amd8111_access(struct i2c_ada
 	struct amd_smbus *smbus = adap->algo_data;
 	unsigned char protocol, len, pec, temp[2];
 	int i;
+	int status;
 
 	protocol = (read_write == I2C_SMBUS_READ) ? AMD_SMB_PRTCL_READ
 						  : AMD_SMB_PRTCL_WRITE;
@@ -267,12 +278,13 @@ static s32 amd8111_access(struct i2c_ada
 
 		default:
 			dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
-			return -1;
+			return -EOPNOTSUPP;
 	}
 
 	amd_ec_write(smbus, AMD_SMB_ADDR, addr << 1);
 	amd_ec_write(smbus, AMD_SMB_PRTCL, protocol);
 
+	/* FIXME this discards status from ec_read() ... */
 	amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
 
 	if (~temp[0] & AMD_SMB_STS_DONE) {
@@ -286,7 +298,7 @@ static s32 amd8111_access(struct i2c_ada
 	}
 
 	if ((~temp[0] & AMD_SMB_STS_DONE) || (temp[0] & AMD_SMB_STS_STATUS))
-		return -1;
+		return -EIO;
 
 	if (read_write == I2C_SMBUS_WRITE)
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:02:58.000000000 -0700
@@ -151,7 +151,7 @@ static int i801_transaction(int xact)
 		outb_p(temp, SMBHSTSTS);
 		if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
 			dev_dbg(&I801_dev->dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&I801_dev->dev, "Successful!\n");
 		}
@@ -170,7 +170,7 @@ static int i801_transaction(int xact)
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&I801_dev->dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 		/* try to stop the current command */
 		dev_dbg(&I801_dev->dev, "Terminating the current operation\n");
 		outb_p(inb_p(SMBHSTCNT) | SMBHSTCNT_KILL, SMBHSTCNT);
@@ -179,19 +179,19 @@ static int i801_transaction(int xact)
 	}
 
 	if (temp & SMBHSTSTS_FAILED) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&I801_dev->dev, "Error: Failed bus transaction\n");
 	}
 
 	if (temp & SMBHSTSTS_BUS_ERR) {
-		result = -1;
+		result = -EIO;
 		dev_err(&I801_dev->dev, "Bus collision! SMBus may be locked "
 			"until next hard reset. (sorry!)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
 	}
 
 	if (temp & SMBHSTSTS_DEV_ERR) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&I801_dev->dev, "Error: no response!\n");
 	}
 
@@ -231,6 +231,7 @@ static int i801_block_transaction_by_blo
 					   char read_write, int hwpec)
 {
 	int i, len;
+	int status;
 
 	inb_p(SMBHSTCNT); /* reset the data buffer index */
 
@@ -242,14 +243,15 @@ static int i801_block_transaction_by_blo
 			outb_p(data->block[i+1], SMBBLKDAT);
 	}
 
-	if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
-			     I801_PEC_EN * hwpec))
-		return -1;
+	status = i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
+			     I801_PEC_EN * hwpec);
+	if (status)
+		return status;
 
 	if (read_write == I2C_SMBUS_READ) {
 		len = inb_p(SMBHSTDAT0);
 		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
-			return -1;
+			return -EILSEQ;
 
 		data->block[0] = len;
 		for (i = 0; i < len; i++)
@@ -314,11 +316,11 @@ static int i801_block_transaction_byte_b
 			if (((temp = inb_p(SMBHSTSTS)) & errmask) != 0x00) {
 				dev_err(&I801_dev->dev,
 					"Reset failed! (%02x)\n", temp);
-				return -1;
+				return -EBUSY;
 			}
 			if (i != 1)
 				/* if die in middle of block transaction, fail */
-				return -1;
+				return -EIO;
 		}
 
 		if (i == 1)
@@ -342,19 +344,19 @@ static int i801_block_transaction_byte_b
 			msleep(1);
 			outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL),
 				SMBHSTCNT);
-			result = -1;
+			result = -ETIMEDOUT;
 			dev_dbg(&I801_dev->dev, "SMBus Timeout!\n");
 		}
 
 		if (temp & SMBHSTSTS_FAILED) {
-			result = -1;
+			result = -EIO;
 			dev_dbg(&I801_dev->dev,
 				"Error: Failed bus transaction\n");
 		} else if (temp & SMBHSTSTS_BUS_ERR) {
-			result = -1;
+			result = -EIO;
 			dev_err(&I801_dev->dev, "Bus collision!\n");
 		} else if (temp & SMBHSTSTS_DEV_ERR) {
-			result = -1;
+			result = -EIO;
 			dev_dbg(&I801_dev->dev, "Error: no response!\n");
 		}
 
@@ -362,7 +364,7 @@ static int i801_block_transaction_byte_b
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
 			len = inb_p(SMBHSTDAT0);
 			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
-				return -1;
+				return -EILSEQ;
 			data->block[0] = len;
 		}
 
@@ -383,7 +385,6 @@ static int i801_block_transaction_byte_b
 			"ADD=%02x, DAT0=%02x, DAT1=%02x, BLKDAT=%02x\n", i,
 			inb_p(SMBHSTCNT), inb_p(SMBHSTCMD), inb_p(SMBHSTADD),
 			inb_p(SMBHSTDAT0), inb_p(SMBHSTDAT1), inb_p(SMBBLKDAT));
-
 		if (result < 0)
 			return result;
 	}
@@ -394,7 +395,7 @@ static int i801_set_block_buffer_mode(vo
 {
 	outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
 	if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0)
-		return -1;
+		return -EIO;
 	return 0;
 }
 
@@ -414,7 +415,7 @@ static int i801_block_transaction(union 
 		} else if (!(i801_features & FEATURE_I2C_BLOCK_READ)) {
 			dev_err(&I801_dev->dev,
 				"I2C block read is unsupported!\n");
-			return -1;
+			return -EOPNOTSUPP;
 		}
 	}
 
@@ -449,7 +450,7 @@ static int i801_block_transaction(union 
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 i801_access(struct i2c_adapter * adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data * data)
@@ -514,7 +515,7 @@ static s32 i801_access(struct i2c_adapte
 	case I2C_SMBUS_PROC_CALL:
 	default:
 		dev_err(&I801_dev->dev, "Unsupported transaction %d\n", size);
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	if (hwpec)	/* enable/disable hardware PEC */
@@ -537,7 +538,7 @@ static s32 i801_access(struct i2c_adapte
 	if(block)
 		return ret;
 	if(ret)
-		return -1;
+		return ret;
 	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
 		return 0;
 
--- g26.orig/drivers/i2c/busses/i2c-nforce2.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-nforce2.c	2008-05-01 16:02:58.000000000 -0700
@@ -145,16 +145,16 @@ static int nforce2_check_status(struct i
 		dev_dbg(&adap->dev, "SMBus Timeout!\n");
 		if (smbus->can_abort)
 			nforce2_abort(adap);
-		return -1;
+		return -ETIMEDOUT;
 	}
 	if (!(temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
 		dev_dbg(&adap->dev, "Transaction failed (0x%02x)!\n", temp);
-		return -1;
+		return -EIO;
 	}
 	return 0;
 }
 
-/* Return -1 on error */
+/* Return negative errno 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)
@@ -162,7 +162,7 @@ static s32 nforce2_access(struct i2c_ada
 	struct nforce2_smbus *smbus = adap->algo_data;
 	unsigned char protocol, pec;
 	u8 len;
-	int i;
+	int i, status;
 
 	protocol = (read_write == I2C_SMBUS_READ) ? NVIDIA_SMB_PRTCL_READ :
 		NVIDIA_SMB_PRTCL_WRITE;
@@ -206,7 +206,7 @@ static s32 nforce2_access(struct i2c_ada
 						"Transaction failed "
 						"(requested block size: %d)\n",
 						len);
-					return -1;
+					return -E2BIG;
 				}
 				outb_p(len, NVIDIA_SMB_BCNT);
 				for (i = 0; i < I2C_SMBUS_BLOCK_MAX; i++)
@@ -218,14 +218,15 @@ static s32 nforce2_access(struct i2c_ada
 
 		default:
 			dev_err(&adap->dev, "Unsupported transaction %d\n", size);
-			return -1;
+			return -EOPNOTSUPP;
 	}
 
 	outb_p((addr & 0x7f) << 1, NVIDIA_SMB_ADDR);
 	outb_p(protocol, NVIDIA_SMB_PRTCL);
 
-	if (nforce2_check_status(adap))
-		return -1;
+	status = nforce2_check_status(adap);
+	if (status)
+		return status;
 
 	if (read_write == I2C_SMBUS_WRITE)
 		return 0;
@@ -247,7 +248,7 @@ static s32 nforce2_access(struct i2c_ada
 				dev_err(&adap->dev, "Transaction failed "
 					"(received block size: 0x%02x)\n",
 					len);
-				return -1;
+				return -EILSEQ;
 			}
 			for (i = 0; i < len; i++)
 				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
@@ -308,7 +309,7 @@ static int __devinit nforce2_probe_smb (
 		    != PCIBIOS_SUCCESSFUL) {
 			dev_err(&dev->dev, "Error reading PCI config for %s\n",
 				name);
-			return -1;
+			return -EIO;
 		}
 
 		smbus->base = iobase & PCI_BASE_ADDRESS_IO_MASK;
@@ -318,7 +319,7 @@ static int __devinit nforce2_probe_smb (
 	if (!request_region(smbus->base, smbus->size, nforce2_driver.name)) {
 		dev_err(&smbus->adapter.dev, "Error requesting region %02x .. %02X for %s\n",
 			smbus->base, smbus->base+smbus->size-1, name);
-		return -1;
+		return -EBUSY;
 	}
 	smbus->adapter.owner = THIS_MODULE;
 	smbus->adapter.id = I2C_HW_SMBUS_NFORCE2;
@@ -333,7 +334,7 @@ static int __devinit nforce2_probe_smb (
 	if (error) {
 		dev_err(&smbus->adapter.dev, "Failed to register adapter.\n");
 		release_region(smbus->base, smbus->size);
-		return -1;
+		return error;
 	}
 	dev_info(&smbus->adapter.dev, "nForce2 SMBus adapter at %#x\n", smbus->base);
 	return 0;
--- g26.orig/drivers/i2c/busses/i2c-piix4.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-piix4.c	2008-05-01 16:02:58.000000000 -0700
@@ -220,7 +220,7 @@ static int piix4_transaction(void)
 		outb_p(temp, SMBHSTSTS);
 		if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
 			dev_err(&piix4_adapter.dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&piix4_adapter.dev, "Successful!\n");
 		}
@@ -238,23 +238,23 @@ static int piix4_transaction(void)
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x10) {
-		result = -1;
+		result = -EIO;
 		dev_err(&piix4_adapter.dev, "Error: Failed bus transaction\n");
 	}
 
 	if (temp & 0x08) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&piix4_adapter.dev, "Bus collision! SMBus may be "
 			"locked until next hard reset. (sorry!)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
 	}
 
 	if (temp & 0x04) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&piix4_adapter.dev, "Error: no response!\n");
 	}
 
@@ -272,17 +272,18 @@ static int piix4_transaction(void)
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 		 unsigned short flags, char read_write,
 		 u8 command, int size, union i2c_smbus_data * data)
 {
 	int i, len;
+	int status;
 
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-		return -1;
+		return -EOPNOTSUPP;
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD);
@@ -334,8 +335,9 @@ static s32 piix4_access(struct i2c_adapt
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	if (piix4_transaction())	/* Error in transaction */
-		return -1;
+	status = piix4_transaction();
+	if (status)
+		return status;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == PIIX4_QUICK))
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-sis5595.c	2008-05-01 16:01:39.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-sis5595.c	2008-05-01 16:07:18.000000000 -0700
@@ -236,7 +236,7 @@ static int sis5595_transaction(struct i2
 		sis5595_write(SMB_STS_HI, temp >> 8);
 		if ((temp = sis5595_read(SMB_STS_LO) + (sis5595_read(SMB_STS_HI) << 8)) != 0x00) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
@@ -254,19 +254,19 @@ static int sis5595_transaction(struct i2
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&adap->dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x10) {
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
-		result = -1;
+		result = -EIO;
 	}
 
 	if (temp & 0x20) {
 		dev_err(&adap->dev, "Bus collision! SMBus may be locked until "
 			"next hard reset (or not...)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
-		result = -1;
+		result = -EIO;
 	}
 
 	temp = sis5595_read(SMB_STS_LO) + (sis5595_read(SMB_STS_HI) << 8);
@@ -282,11 +282,13 @@ static int sis5595_transaction(struct i2
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 sis5595_access(struct i2c_adapter *adap, u16 addr,
 			  unsigned short flags, char read_write,
 			  u8 command, int size, union i2c_smbus_data *data)
 {
+	int status;
+
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		sis5595_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
@@ -318,13 +320,14 @@ static s32 sis5595_access(struct i2c_ada
 		break;
 	default:
 		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	sis5595_write(SMB_CTL_LO, ((size & 0x0E)));
 
-	if (sis5595_transaction(adap))
-		return -1;
+	status = sis5595_transaction(adap);
+	if (status)
+		return status;
 
 	if ((size != SIS5595_PROC_CALL) &&
 	    ((read_write == I2C_SMBUS_WRITE) || (size == SIS5595_QUICK)))
--- g26.orig/drivers/i2c/busses/i2c-sis630.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-sis630.c	2008-05-01 16:02:58.000000000 -0700
@@ -134,7 +134,7 @@ static int sis630_transaction_start(stru
 
 		if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
                 } else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
@@ -177,17 +177,17 @@ static int sis630_transaction_wait(struc
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&adap->dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x02) {
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
-		result = -1;
+		result = -EIO;
 	}
 
 	if (temp & 0x04) {
 		dev_err(&adap->dev, "Bus collision!\n");
-		result = -1;
+		result = -EIO;
 		/*
 		  TBD: Datasheet say:
 		  the software should clear this bit and restart SMBUS operation.
@@ -250,8 +250,10 @@ static int sis630_block_data(struct i2c_
 			if (i==8 || (len<8 && i==len)) {
 				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
 				/* first transaction */
-				if (sis630_transaction_start(adap, SIS630_BLOCK_DATA, &oldclock))
-					return -1;
+				rc = sis630_transaction_start(adap,
+						SIS630_BLOCK_DATA, &oldclock);
+				if (rc)
+					return rc;
 			}
 			else if ((i-1)%8 == 7 || i==len) {
 				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
@@ -264,9 +266,10 @@ static int sis630_block_data(struct i2c_
 					*/
 					sis630_write(SMB_STS,0x10);
 				}
-				if (sis630_transaction_wait(adap, SIS630_BLOCK_DATA)) {
+				rc = sis630_transaction_wait(adap,
+						SIS630_BLOCK_DATA);
+				if (rc) {
 					dev_dbg(&adap->dev, "trans_wait failed\n");
-					rc = -1;
 					break;
 				}
 			}
@@ -275,13 +278,14 @@ static int sis630_block_data(struct i2c_
 	else {
 		/* read request */
 		data->block[0] = len = 0;
-		if (sis630_transaction_start(adap, SIS630_BLOCK_DATA, &oldclock)) {
-			return -1;
-		}
+		rc = sis630_transaction_start(adap,
+				SIS630_BLOCK_DATA, &oldclock);
+		if (rc)
+			return rc;
 		do {
-			if (sis630_transaction_wait(adap, SIS630_BLOCK_DATA)) {
+			rc = sis630_transaction_wait(adap, SIS630_BLOCK_DATA);
+			if (rc) {
 				dev_dbg(&adap->dev, "trans_wait failed\n");
-				rc = -1;
 				break;
 			}
 			/* if this first transaction then read byte count */
@@ -311,11 +315,13 @@ static int sis630_block_data(struct i2c_
 	return rc;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 			 unsigned short flags, char read_write,
 			 u8 command, int size, union i2c_smbus_data *data)
 {
+	int status;
+
 	switch (size) {
 		case I2C_SMBUS_QUICK:
 			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
@@ -351,12 +357,13 @@ static s32 sis630_access(struct i2c_adap
 			return sis630_block_data(adap, data, read_write);
 		default:
 			printk("Unsupported I2C size\n");
-			return -1;
+			return -EMSGSIZE;
 			break;
 	}
 
-	if (sis630_transaction(adap, size))
-		return -1;
+	status = sis630_transaction(adap, size);
+	if (status)
+		return status;
 
 	if ((size != SIS630_PCALL) &&
 		((read_write == I2C_SMBUS_WRITE) || (size == SIS630_QUICK))) {
@@ -373,7 +380,7 @@ static s32 sis630_access(struct i2c_adap
 			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
 			break;
 		default:
-			return -1;
+			return -EOPNOTSUPP;
 			break;
 	}
 
--- g26.orig/drivers/i2c/busses/i2c-sis96x.c	2008-05-01 16:01:38.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-sis96x.c	2008-05-01 16:02:58.000000000 -0700
@@ -111,7 +111,7 @@ static int sis96x_transaction(int size)
 		/* check it again */
 		if (((temp = sis96x_read(SMB_CNT)) & 0x03) != 0x00) {
 			dev_dbg(&sis96x_adapter.dev, "Failed (0x%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&sis96x_adapter.dev, "Successful\n");
 		}
@@ -136,19 +136,19 @@ static int sis96x_transaction(int size)
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&sis96x_adapter.dev, "SMBus Timeout! (0x%02x)\n", temp);
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	/* device error - probably missing ACK */
 	if (temp & 0x02) {
 		dev_dbg(&sis96x_adapter.dev, "Failed bus transaction!\n");
-		result = -1;
+		result = -EIO;
 	}
 
 	/* bus collision */
 	if (temp & 0x04) {
 		dev_dbg(&sis96x_adapter.dev, "Bus collision!\n");
-		result = -1;
+		result = -EIO;
 	}
 
 	/* Finish up by resetting the bus */
@@ -161,11 +161,12 @@ static int sis96x_transaction(int size)
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 sis96x_access(struct i2c_adapter * adap, u16 addr,
 			 unsigned short flags, char read_write,
 			 u8 command, int size, union i2c_smbus_data * data)
 {
+	int status;
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -203,17 +204,18 @@ static s32 sis96x_access(struct i2c_adap
 	case I2C_SMBUS_BLOCK_DATA:
 		/* TO DO: */
 		dev_info(&adap->dev, "SMBus block not implemented!\n");
-		return -1;
+		return -EOPNOTSUPP;
 		break;
 
 	default:
 		dev_info(&adap->dev, "Unsupported I2C size\n");
-		return -1;
+		return -EMSGSIZE;
 		break;
 	}
 
-	if (sis96x_transaction(size))
-		return -1;
+	status = sis96x_transaction(size);
+	if (status)
+		return status;
 
 	if ((size != SIS96x_PROC_CALL) &&
 		((read_write == I2C_SMBUS_WRITE) || (size == SIS96x_QUICK)))
--- g26.orig/drivers/i2c/busses/i2c-stub.c	2008-05-01 16:10:15.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-stub.c	2008-05-01 16:10:46.000000000 -0700
@@ -43,7 +43,7 @@ struct stub_chip {
 
 static struct stub_chip *stub_chips;
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 stub_xfer(struct i2c_adapter * adap, u16 addr, unsigned short flags,
 	char read_write, u8 command, int size, union i2c_smbus_data * data)
 {
@@ -120,7 +120,7 @@ static s32 stub_xfer(struct i2c_adapter 
 
 	default:
 		dev_dbg(&adap->dev, "Unsupported I2C/SMBus command\n");
-		ret = -1;
+		ret = -EOPNOTSUPP;
 		break;
 	} /* switch (size) */
 
--- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-01 17:12:45.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-01 17:15:03.000000000 -0700
@@ -152,7 +152,7 @@ static int vt596_transaction(u8 size)
 		if ((temp = inb_p(SMBHSTSTS)) & 0x1F) {
 			dev_err(&vt596_adapter.dev, "SMBus reset failed! "
 				"(0x%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		}
 	}
 
@@ -167,24 +167,24 @@ static int vt596_transaction(u8 size)
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
 	}
 
 	if (temp & 0x10) {
-		result = -1;
+		result = -EIO;
 		dev_err(&vt596_adapter.dev, "Transaction failed (0x%02x)\n",
 			size);
 	}
 
 	if (temp & 0x08) {
-		result = -1;
+		result = -EIO;
 		dev_err(&vt596_adapter.dev, "SMBus collision!\n");
 	}
 
 	if (temp & 0x04) {
 		int read = inb_p(SMBHSTADD) & 0x01;
-		result = -1;
+		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. */
@@ -202,12 +202,13 @@ static int vt596_transaction(u8 size)
 	return result;
 }
 
-/* Return -1 on error, 0 on success */
+/* Return negative errno on error, 0 on success */
 static s32 vt596_access(struct i2c_adapter *adap, u16 addr,
 		unsigned short flags, char read_write, u8 command,
 		int size, union i2c_smbus_data *data)
 {
 	int i;
+	int status;
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -258,8 +259,9 @@ static s32 vt596_access(struct i2c_adapt
 
 	outb_p(((addr & 0x7f) << 1) | read_write, SMBHSTADD);
 
-	if (vt596_transaction(size)) /* Error in transaction */
-		return -1;
+	status = vt596_transaction(size);
+	if (status)
+		return status;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
 		return 0;
@@ -287,7 +289,7 @@ static s32 vt596_access(struct i2c_adapt
 exit_unsupported:
 	dev_warn(&vt596_adapter.dev, "Unsupported command invoked! (0x%02x)\n",
 		 size);
-	return -1;
+	return -EOPNOTSUPP;
 }
 
 static u32 vt596_func(struct i2c_adapter *adapter)

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi David,

On Thu, 1 May 2008 20:46:07 -0700, David Brownell wrote:
> Tighten error paths used by various other i2c adapters so they
> return real fault/errno codes instead of a literal "-1" (which
> is most often interpreted as "-EPERM").  Build tested.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> NOTE there are many other adapter drivers to audit and update; and
> they're not just within the drivers/i2c tree.  This patch is a PC
> oriented start on some bottom-up improvement of fault reporting.
> The i2c-gpio code looks be good too.
> 
>  drivers/i2c/algos/i2c-algo-bit.c      |   11 +++++---
>  drivers/i2c/busses/i2c-ali1535.c      |   22 +++++++---------
>  drivers/i2c/busses/i2c-ali1563.c      |    4 +--
>  drivers/i2c/busses/i2c-ali15x3.c      |   19 +++++++-------
>  drivers/i2c/busses/i2c-amd756-s4882.c |    4 +--
>  drivers/i2c/busses/i2c-amd756.c       |   18 +++++++------
>  drivers/i2c/busses/i2c-amd8111.c      |   44 +++++++++++++++++++++------------
>  drivers/i2c/busses/i2c-i801.c         |   45 +++++++++++++++++-----------------
>  drivers/i2c/busses/i2c-nforce2.c      |   25 +++++++++---------
>  drivers/i2c/busses/i2c-piix4.c        |   20 ++++++++-------
>  drivers/i2c/busses/i2c-sis5595.c      |   19 ++++++++------
>  drivers/i2c/busses/i2c-sis630.c       |   43 ++++++++++++++++++--------------
>  drivers/i2c/busses/i2c-sis96x.c       |   20 ++++++++-------
>  drivers/i2c/busses/i2c-stub.c         |    4 +--
>  drivers/i2c/busses/i2c-viapro.c       |   20 ++++++++-------
>  15 files changed, 176 insertions(+), 142 deletions(-)

I am in the process of reviewing and testing this patch. I think it
would help me if you could list your error value choices for the common
error conditions of I2C and SMBus controllers (bus busy, arbitration
lost, transaction timeout, etc.) With such a list I could check the
different drivers for consistency, and maybe this could even become
documentation for future driver authors.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 10 May 2008, Jean Delvare wrote:
> I am in the process of reviewing and testing this patch. I think it
> would help me if you could list your error value choices for the common
> error conditions of I2C and SMBus controllers (bus busy, arbitration
> lost, transaction timeout, etc.) With such a list I could check the
> different drivers for consistency, and maybe this could even become
> documentation for future driver authors.

Yeah, I'll write something up.  Agreed that would be handy ... and
since not all the fault codes indicate errors (losing arbitration,
for example), it's important to agree on codes where we think that
drivers should eventually be able to kick in recovery strategies.

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 10 May 2008, Jean Delvare wrote:
> I am in the process of reviewing and testing this patch. I think it
> would help me if you could list your error value choices for the common
> error conditions of I2C and SMBus controllers (bus busy, arbitration
> lost, transaction timeout, etc.) With such a list I could check the
> different drivers for consistency, and maybe this could even become
> documentation for future driver authors.

Here's a patch adding such information ... against 2.6.26-rc2 and
in synch with both the "-Errno not -1" patches I've sent.

- Dave

=============== CUT HERE
Create Documentation/i2c/fault-codes.txt to help standardize
fault/error code usage in the I2C stack.  It turns out that
returning -1 (-EPERM) for everything was not at all helpful.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 Documentation/i2c/fault-codes.txt |  118 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 118 insertions(+)

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ g26/Documentation/i2c/fault-codes.txt	2008-05-11 15:06:35.000000000 -0700
@@ -0,0 +1,118 @@
+This is a summary of the most important conventions for use of fault
+codes in the I2C/SMBus stack.
+
+
+A "Fault" is not always an "Error"
+----------------------------------
+Not all fault reports imply errors; "page faults" should be a familiar
+example.   Software often retries idempotent operations after transient
+faults.  There may be fancier recovery schemes that are appropriate in
+some cases, such as re-initializing (and maybe resetting).  After such
+recovery, triggered by a fault report, there is no error.
+
+In a similar way, sometimes a "fault" code just reports one defined
+result for an operation ... it doesn't indicate that anything is wrong
+at all, just that the the outcome wasn't on the "golden path".
+
+In short, your I2C driver code may need to know these codes in order
+to respond correctly.  Other code may need to rely on YOUR code reporting
+the right fault code, so that it can (in turn) behave correctly.
+
+
+I2C and SMBus fault codes
+-------------------------
+These are returned as negative numbers from most calls, with zero or
+some positive number indicating a non-fault return.  The specific
+numbers associated with these symbols differ between architectures,
+though most Linux systems use <asm-generic/errno*.h> numbering.
+
+Note that the descriptions here are not exhaustive.  There are other
+codes that may be returned, and other cases where these codes should
+be returned.  However, drivers should not return other codes for these
+cases (unless the hardware doesn't provide unique fault reports).
+
+Also, codes returned by adapter probe methods follow rules which are
+specific to their host bus (such as PCI, or the platform bus).
+
+
+EAGAIN
+	Returned by I2C adapters when they lose arbitration in master
+	transmit mode:  some other master was transmitting different
+	data at the same time.
+
+	Also returned when trying to invoke an I2C operation in an
+	atomic context, when some task is already using that I2C bus
+	to execture some other operation.
+
+EBADMSG
+	Returned by SMBus logic when an invalid Packet Error Code byte
+	is received.  This code is a CRC covering all bytes in the
+	transaction, and is sent before the terminating STOP.  This
+	fault is only reported on read transactions; the SMBus slave
+	may have a way to report PEC mismatches on writes from the
+	host.  Note that even if PECs are in use, you should not rely
+	on these as the only way to detect incorrect data transfers.
+
+EBUSY
+	Returned by SMBus adapters when the bus was busy for longer
+	than allowed.  This usually indicates some device (maybe the
+	SMBus adapter) needs some fault recovery (such as resetting),
+	or that the reset was attempted but failed.
+
+EINVAL
+	This rather vague error means an invalid parameter has been
+	detected before any I/O operation was started.  Use a more
+	specific fault code when you can.
+
+	One example would be a driver trying an SMBus Block Write
+	with block size outside the range of 1-32 bytes.
+
+EIO
+	This rather vague error means something went wrong when
+	performing an I/O operation.  Use a more specific fault
+	code when you can.
+
+ENODEV
+	Returned by driver probe() methods,   This is a bit more
+	specific than ENXIO, implying the problem isn't with the
+	address, but with the device found there.  Driver probes
+	often do more than just verify that something responds to
+	an address; they may also verify the *correct* responses.
+
+ENOMEM
+	Returned by any component that can't allocate memory when
+	it needs to do so.
+
+ENXIO
+	Returned by I2C adapters to indicate that the address phase
+	of a transfer didn't get an ACK.  While it might just mean
+	an I2C device was temporarily not responding, usually  it
+	means there's nothing listening at that address.
+
+	Returned by driver probe() methods to indicate that they
+	found no device to bind to.  (ENODEV may also be used.)
+
+EOPNOTSUPP
+	Returned by an adapter when asked to perform an operation
+	that it doesn't, or can't, support.  For example, if an
+	adapter doesn't support SMBus block transfers, this would
+	be returned when it is asked to issue one.  Or if an I2C
+	adapter can't execute all legal I2C messages, it should
+	return this in some cases.
+
+EPROTO
+	Returned when the length of an SMBus Block Read data response
+	(from the SMBus slave) is outside the range 1-32 bytes.
+
+ETIMEDOUT
+	This is returned by drivers when an operation took too much
+	time, and was aborted before it completed.  (However, use
+	ENXIO for timeouts when sending the first address byte.)
+
+	SMBus adapters may return it when an operation took more
+	time than allowed by the SMBus specification; for example,
+	when a slave stretches clocks too far.  I2C has no such
+	timeouts, but it's normal for I2C adapters to impose some
+	arbitrary limits (much longer than SMBus!) too.
+
+


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi David,

On Mon, 12 May 2008 09:48:23 -0700, David Brownell wrote:
> On Saturday 10 May 2008, Jean Delvare wrote:
> > I am in the process of reviewing and testing this patch. I think it
> > would help me if you could list your error value choices for the common
> > error conditions of I2C and SMBus controllers (bus busy, arbitration
> > lost, transaction timeout, etc.) With such a list I could check the
> > different drivers for consistency, and maybe this could even become
> > documentation for future driver authors.
> 
> Here's a patch adding such information ... against 2.6.26-rc2 and
> in synch with both the "-Errno not -1" patches I've sent.

Thanks for doing this. This document will be very helpful both for me
when reviewing your patches, and for both I2C bus and I2C device driver
authors later.

> 
> - Dave
> 
> =============== CUT HERE
> Create Documentation/i2c/fault-codes.txt to help standardize
> fault/error code usage in the I2C stack.  It turns out that
> returning -1 (-EPERM) for everything was not at all helpful.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  Documentation/i2c/fault-codes.txt |  118 ++++++++++++++++++++++++++++++++++++++

No file in this directory has a .txt suffix, so I guess we'd just name
the file fault-codes for consistency.

>  1 file changed, 118 insertions(+)
> 
> --- /dev/null	1970-01-01 00:00:00.000000000 +0000
> +++ g26/Documentation/i2c/fault-codes.txt	2008-05-11 15:06:35.000000000 -0700
> @@ -0,0 +1,118 @@
> +This is a summary of the most important conventions for use of fault
> +codes in the I2C/SMBus stack.
> +
> +
> +A "Fault" is not always an "Error"
> +----------------------------------
> +Not all fault reports imply errors; "page faults" should be a familiar
> +example.   Software often retries idempotent operations after transient

Too many spaces after dot. I'm fine with 2 if you like it, but 3 is to
many.

> +faults.  There may be fancier recovery schemes that are appropriate in
> +some cases, such as re-initializing (and maybe resetting).  After such
> +recovery, triggered by a fault report, there is no error.
> +
> +In a similar way, sometimes a "fault" code just reports one defined
> +result for an operation ... it doesn't indicate that anything is wrong
> +at all, just that the the outcome wasn't on the "golden path".

Doubled "the".

> +
> +In short, your I2C driver code may need to know these codes in order
> +to respond correctly.  Other code may need to rely on YOUR code reporting
> +the right fault code, so that it can (in turn) behave correctly.
> +
> +
> +I2C and SMBus fault codes
> +-------------------------
> +These are returned as negative numbers from most calls, with zero or
> +some positive number indicating a non-fault return.  The specific
> +numbers associated with these symbols differ between architectures,
> +though most Linux systems use <asm-generic/errno*.h> numbering.
> +
> +Note that the descriptions here are not exhaustive.  There are other
> +codes that may be returned, and other cases where these codes should
> +be returned.  However, drivers should not return other codes for these
> +cases (unless the hardware doesn't provide unique fault reports).
> +
> +Also, codes returned by adapter probe methods follow rules which are
> +specific to their host bus (such as PCI, or the platform bus).
> +
> +
> +EAGAIN
> +	Returned by I2C adapters when they lose arbitration in master
> +	transmit mode:  some other master was transmitting different
> +	data at the same time.
> +
> +	Also returned when trying to invoke an I2C operation in an
> +	atomic context, when some task is already using that I2C bus
> +	to execture some other operation.

excecute?

> +
> +EBADMSG
> +	Returned by SMBus logic when an invalid Packet Error Code byte
> +	is received.  This code is a CRC covering all bytes in the
> +	transaction, and is sent before the terminating STOP.  This
> +	fault is only reported on read transactions; the SMBus slave
> +	may have a way to report PEC mismatches on writes from the
> +	host.  Note that even if PECs are in use, you should not rely
> +	on these as the only way to detect incorrect data transfers.

As far as I can see, only the i2c-core is returning this error when
SMBus is emulated over I2C, SMBus master drivers do not? If it is a
hardware limitation of most SMBus controllers then I wonder if it is
worth having a separate error code just for i2c-core.

> +
> +EBUSY
> +	Returned by SMBus adapters when the bus was busy for longer
> +	than allowed.  This usually indicates some device (maybe the
> +	SMBus adapter) needs some fault recovery (such as resetting),
> +	or that the reset was attempted but failed.
> +
> +EINVAL
> +	This rather vague error means an invalid parameter has been
> +	detected before any I/O operation was started.  Use a more
> +	specific fault code when you can.
> +
> +	One example would be a driver trying an SMBus Block Write
> +	with block size outside the range of 1-32 bytes.
> +
> +EIO
> +	This rather vague error means something went wrong when
> +	performing an I/O operation.  Use a more specific fault
> +	code when you can.
> +
> +ENODEV
> +	Returned by driver probe() methods,   This is a bit more

I guess you want a dot rather than comma, and one less space.

> +	specific than ENXIO, implying the problem isn't with the
> +	address, but with the device found there.  Driver probes
> +	often do more than just verify that something responds to
> +	an address; they may also verify the *correct* responses.

In fact driver probes should never verify that something responds to an
address. That's not their job; by the time they are called, either
i2c-core has done a probe, or some code guaranteed that there was a
device at this address, or (for legacy devices) the user explicitly
asked for the device presence check to be skipped.

It may happen that, while they are checking something about the device,
they discover that apparently there is no device at all, but this is
only a side effect.

> +
> +ENOMEM
> +	Returned by any component that can't allocate memory when
> +	it needs to do so.
> +
> +ENXIO
> +	Returned by I2C adapters to indicate that the address phase
> +	of a transfer didn't get an ACK.  While it might just mean
> +	an I2C device was temporarily not responding, usually  it

Doubled space.

> +	means there's nothing listening at that address.
> +
> +	Returned by driver probe() methods to indicate that they
> +	found no device to bind to.  (ENODEV may also be used.)
> +
> +EOPNOTSUPP
> +	Returned by an adapter when asked to perform an operation
> +	that it doesn't, or can't, support.  For example, if an
> +	adapter doesn't support SMBus block transfers, this would
> +	be returned when it is asked to issue one.  Or if an I2C
> +	adapter can't execute all legal I2C messages, it should
> +	return this in some cases.

It might be worth mentioning here that I2C device drivers are supposed
to check the adapter functionality before they run any transaction. So,
in most cases, I2C bus drivers would be in their own right to return
-EINVAL when requested to perform operations they don't support. We use
-EOPNOTSUPP only because it's nice to have a dedicated error value for
these cases.

The only (rare) case where EOPNOTSUPP is legitimately returned by a bus
driver is when the bus driver doesn't fully support a given transaction
type, but cannot express this limitation in the functionality flags
(for example a bus driver supporting I2C Block transactions but only
for ceertain block lengths.)

> +
> +EPROTO
> +	Returned when the length of an SMBus Block Read data response
> +	(from the SMBus slave) is outside the range 1-32 bytes.

While this is the only care where we return this error code currently,
I suppose that there could be others in the future, for example for an
SMBus block read-block write process call transaction. So maybe it
would be better to define this error code as being returned when data
sent by a slave doesn't comply with the protocol in general, and then
quote the SMBus Block Read length byte as the most common example.

> +
> +ETIMEDOUT
> +	This is returned by drivers when an operation took too much
> +	time, and was aborted before it completed.  (However, use
> +	ENXIO for timeouts when sending the first address byte.)

No. A missing ack on the address byte leads to no timeout. The master
controls the clock, so either ack or nack takes exactly one clock
cycle. As far as I remember, clock stretching by a slave during the
address byte is not even allowed.

So, there is no need to mention ENXIO and the address byte here.

> +
> +	SMBus adapters may return it when an operation took more
> +	time than allowed by the SMBus specification; for example,
> +	when a slave stretches clocks too far.  I2C has no such
> +	timeouts, but it's normal for I2C adapters to impose some
> +	arbitrary limits (much longer than SMBus!) too.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Wednesday 14 May 2008, Jean Delvare wrote:
> Hi David,
> 
> On Mon, 12 May 2008 09:48:23 -0700, David Brownell wrote:
> > On Saturday 10 May 2008, Jean Delvare wrote:
> > > ... list your error value choices for the common
> > > error conditions of I2C and SMBus controllers ...
> > 
> > Here's a patch adding such information ... against 2.6.26-rc2 and
> > in synch with both the "-Errno not -1" patches I've sent.
> 
> Thanks for doing this. This document will be very helpful both for me
> when reviewing your patches, and for both I2C bus and I2C device driver
> authors later.

Exactly.  ;)

I addressed your comments; no further responses except as below.


> > +EBADMSG
> > +	Returned by SMBus logic when an invalid Packet Error Code byte
> > +	is received.  This code is a CRC covering all bytes in the
> > +	transaction, and is sent before the terminating STOP.  This
> > +	fault is only reported on read transactions; the SMBus slave
> > +	may have a way to report PEC mismatches on writes from the
> > +	host.  Note that even if PECs are in use, you should not rely
> > +	on these as the only way to detect incorrect data transfers.
> 
> As far as I can see, only the i2c-core is returning this error when
> SMBus is emulated over I2C, SMBus master drivers do not? 

I didn't check hardware specs or driver details for the three
adapters currently reporting I2C_FUNC_SMBUS_PEC:  amd8111, i801,
and nforce2.  So I don't know what they report ...


> If it is a 
> hardware limitation of most SMBus controllers then I wonder if it is
> worth having a separate error code just for i2c-core.

As a rule, it's important to know that an error is "soft" (like
PEC corruption) or not ... soft errors are very amenable to retries.

So even if those SMBus controllers have braindead designs which
don't let drivers know if the failure was from PEC, I'd think that
it's worth having such a code.  After all, it's not like there's
anything *preventing* sane nardware.  (Right?!?)


> > +EOPNOTSUPP
> > +	Returned by an adapter when asked to perform an operation
> > +	that it doesn't, or can't, support.  For example, if an
> > +	adapter doesn't support SMBus block transfers, this would
> > +	be returned when it is asked to issue one.  Or if an I2C
> > +	adapter can't execute all legal I2C messages, it should
> > +	return this in some cases.
> 
> It might be worth mentioning here that I2C device drivers are supposed
> to check the adapter functionality before they run any transaction. 

Done.


> So, 
> in most cases, I2C bus drivers would be in their own right to return
> -EINVAL when requested to perform operations they don't support.

I'll disagree.  The request was valid, and the only thing that
was inappropriate was sending it to *this* adapter.  Whether a
request is valid (or not) should be minimally context-sensitive.

It's an error to make a given valid request to certain adapters;
sure.  But the *reason* is that the adapter doesn't support such
calls ... not that the request itself is invalid.


>	As far as I remember, clock stretching by a slave during the
> address byte is not even allowed.

It's allowed anywhere.  Remember that one of the reasons to stretch
is to make sure the device has enough time to react ... and also
that multi-master configurations will effectively need to arbitrate
the clock signal (looks just like clock stretching), not just the
data signal.


> So, there is no need to mention ENXIO and the address byte here.

Right, I realized that but it was time to send for review rather
than make more edits.  ;)

Updated patch in a separate message.

- Dave




_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Wednesday 14 May 2008, Jean Delvare wrote:
> 
> > Here's a patch adding such information ... against 2.6.26-rc2 and
> > in synch with both the "-Errno not -1" patches I've sent.
> 
> Thanks for doing this. This document will be very helpful both for me
> when reviewing your patches, and for both I2C bus and I2C device driver
> authors later.

Updated version.

======== CUT HERE
Create Documentation/i2c/fault-codes to help standardize
fault/error code usage in the I2C stack.  It turns out that
returning -1 (-EPERM) for everything was not at all helpful.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
 Documentation/i2c/fault-codes |  127 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ g26/Documentation/i2c/fault-codes	2008-05-14 07:49:55.000000000 -0700
@@ -0,0 +1,127 @@
+This is a summary of the most important conventions for use of fault
+codes in the I2C/SMBus stack.
+
+
+A "Fault" is not always an "Error"
+----------------------------------
+Not all fault reports imply errors; "page faults" should be a familiar
+example.  Software often retries idempotent operations after transient
+faults.  There may be fancier recovery schemes that are appropriate in
+some cases, such as re-initializing (and maybe resetting).  After such
+recovery, triggered by a fault report, there is no error.
+
+In a similar way, sometimes a "fault" code just reports one defined
+result for an operation ... it doesn't indicate that anything is wrong
+at all, just that the outcome wasn't on the "golden path".
+
+In short, your I2C driver code may need to know these codes in order
+to respond correctly.  Other code may need to rely on YOUR code reporting
+the right fault code, so that it can (in turn) behave correctly.
+
+
+I2C and SMBus fault codes
+-------------------------
+These are returned as negative numbers from most calls, with zero or
+some positive number indicating a non-fault return.  The specific
+numbers associated with these symbols differ between architectures,
+though most Linux systems use <asm-generic/errno*.h> numbering.
+
+Note that the descriptions here are not exhaustive.  There are other
+codes that may be returned, and other cases where these codes should
+be returned.  However, drivers should not return other codes for these
+cases (unless the hardware doesn't provide unique fault reports).
+
+Also, codes returned by adapter probe methods follow rules which are
+specific to their host bus (such as PCI, or the platform bus).
+
+
+EAGAIN
+	Returned by I2C adapters when they lose arbitration in master
+	transmit mode:  some other master was transmitting different
+	data at the same time.
+
+	Also returned when trying to invoke an I2C operation in an
+	atomic context, when some task is already using that I2C bus
+	to execute some other operation.
+
+EBADMSG
+	Returned by SMBus logic when an invalid Packet Error Code byte
+	is received.  This code is a CRC covering all bytes in the
+	transaction, and is sent before the terminating STOP.  This
+	fault is only reported on read transactions; the SMBus slave
+	may have a way to report PEC mismatches on writes from the
+	host.  Note that even if PECs are in use, you should not rely
+	on these as the only way to detect incorrect data transfers.
+
+EBUSY
+	Returned by SMBus adapters when the bus was busy for longer
+	than allowed.  This usually indicates some device (maybe the
+	SMBus adapter) needs some fault recovery (such as resetting),
+	or that the reset was attempted but failed.
+
+EINVAL
+	This rather vague error means an invalid parameter has been
+	detected before any I/O operation was started.  Use a more
+	specific fault code when you can.
+
+	One example would be a driver trying an SMBus Block Write
+	with block size outside the range of 1-32 bytes.
+
+EIO
+	This rather vague error means something went wrong when
+	performing an I/O operation.  Use a more specific fault
+	code when you can.
+
+ENODEV
+	Returned by driver probe() methods.  This is a bit more
+	specific than ENXIO, implying the problem isn't with the
+	address, but with the device found there.  Driver probes
+	may verify the device returns *correct* responses, and
+	return this as appropriate.  (The driver core will warn
+	about probe faults other than ENXIO and ENODEV.)
+
+ENOMEM
+	Returned by any component that can't allocate memory when
+	it needs to do so.
+
+ENXIO
+	Returned by I2C adapters to indicate that the address phase
+	of a transfer didn't get an ACK.  While it might just mean
+	an I2C device was temporarily not responding, usually it
+	means there's nothing listening at that address.
+
+	Returned by driver probe() methods to indicate that they
+	found no device to bind to.  (ENODEV may also be used.)
+
+EOPNOTSUPP
+	Returned by an adapter when asked to perform an operation
+	that it doesn't, or can't, support.
+
+	For example, this would be returned when an adapter that
+	doesn't support SMBus block transfers is asked to execute
+	one.  In that case, the driver making that request should
+	have verified that functionality was supported before it
+	made that block transfer request.
+
+	Similarly, if an I2C adapter can't execute all legal I2C
+	messages, it should return this when asked to perform a
+	transaction it can't.  (These limitations can't be seen in
+	the adapter's functionality mask, since the assumption is
+	that if an adapter supports I2C it supports all of I2C.)
+
+EPROTO
+	Returned when slave does not conform to the relevant I2C
+	or SMBus (or chip-specific) protocol specifications.  One
+	case is when the length of an SMBus block data response
+	(from the SMBus slave) is outside the range 1-32 bytes.
+
+ETIMEDOUT
+	This is returned by drivers when an operation took too much
+	time, and was aborted before it completed.
+
+	SMBus adapters may return it when an operation took more
+	time than allowed by the SMBus specification; for example,
+	when a slave stretches clocks too far.  I2C has no such
+	timeouts, but it's normal for I2C adapters to impose some
+	arbitrary limits (much longer than SMBus!) too.
+


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Wed, 14 May 2008 07:50:49 -0700, David Brownell wrote:
> On Wednesday 14 May 2008, Jean Delvare wrote:
> > 
> > > Here's a patch adding such information ... against 2.6.26-rc2 and
> > > in synch with both the "-Errno not -1" patches I've sent.
> > 
> > Thanks for doing this. This document will be very helpful both for me
> > when reviewing your patches, and for both I2C bus and I2C device driver
> > authors later.
> 
> Updated version.
> 
> ======== CUT HERE
> Create Documentation/i2c/fault-codes to help standardize
> fault/error code usage in the I2C stack.  It turns out that
> returning -1 (-EPERM) for everything was not at all helpful.
> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
>  Documentation/i2c/fault-codes |  127 ++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)

Applied, thanks.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi David,

On Thu, 1 May 2008 20:46:07 -0700, David Brownell wrote:
> Tighten error paths used by various other i2c adapters so they
> return real fault/errno codes instead of a literal "-1" (which
> is most often interpreted as "-EPERM").  Build tested.

Thanks for doing this, this was very much needed.

> 
> Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
> ---
> NOTE there are many other adapter drivers to audit and update; and
> they're not just within the drivers/i2c tree.  This patch is a PC
> oriented start on some bottom-up improvement of fault reporting.
> The i2c-gpio code looks be good too.
> 
>  drivers/i2c/algos/i2c-algo-bit.c      |   11 +++++---
>  drivers/i2c/busses/i2c-ali1535.c      |   22 +++++++---------
>  drivers/i2c/busses/i2c-ali1563.c      |    4 +--
>  drivers/i2c/busses/i2c-ali15x3.c      |   19 +++++++-------
>  drivers/i2c/busses/i2c-amd756-s4882.c |    4 +--
>  drivers/i2c/busses/i2c-amd756.c       |   18 +++++++------
>  drivers/i2c/busses/i2c-amd8111.c      |   44 +++++++++++++++++++++------------
>  drivers/i2c/busses/i2c-i801.c         |   45 +++++++++++++++++-----------------
>  drivers/i2c/busses/i2c-nforce2.c      |   25 +++++++++---------
>  drivers/i2c/busses/i2c-piix4.c        |   20 ++++++++-------
>  drivers/i2c/busses/i2c-sis5595.c      |   19 ++++++++------
>  drivers/i2c/busses/i2c-sis630.c       |   43 ++++++++++++++++++--------------
>  drivers/i2c/busses/i2c-sis96x.c       |   20 ++++++++-------
>  drivers/i2c/busses/i2c-stub.c         |    4 +--
>  drivers/i2c/busses/i2c-viapro.c       |   20 ++++++++-------
>  15 files changed, 176 insertions(+), 142 deletions(-)
> 
> --- g26.orig/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:02:58.000000000 -0700
> @@ -317,10 +317,10 @@ bailout:
>   * -x transmission error
>   */
>  static int try_address(struct i2c_adapter *i2c_adap,
> -		       unsigned char addr, int retries)
> +		       unsigned char addr, unsigned retries)
>  {
>  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> -	int i, ret = -1;
> +	int i, ret;
>  
>  	for (i = 0; i <= retries; i++) {
>  		ret = i2c_outb(i2c_adap, addr);
> @@ -465,9 +465,12 @@ static int bit_doAddress(struct i2c_adap
>  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
>  
>  	unsigned char addr;
> -	int ret, retries;
> +	int ret;
> +	unsigned retries;
>  
> -	retries = nak_ok ? 0 : i2c_adap->retries;
> +	retries = (nak_ok || i2c_adap->retries < 0)
> +			? 0
> +			: i2c_adap->retries;
>  
>  	if (flags & I2C_M_TEN) {
>  		/* a ten bit address */

I'd rather not bother with the adap->retries stuff which we agreed
should be removed anyway. The err = -1 above was there just to shut up the
compiler I guess, make it err = 0 and you're done. That's what it
should have been anyway (if we never try, we just get no ack.)

> --- g26.orig/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:01:39.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:02:58.000000000 -0700
> @@ -259,7 +259,7 @@ static int ali1535_transaction(struct i2
>  			dev_err(&adap->dev,
>  				"SMBus reset failed! (0x%02x) - controller or "
>  				"device on bus is probably hung\n", temp);
> -			return -1;
> +			return -EBUSY;
>  		}
>  	} else {
>  		/* check and clear done bit */
> @@ -281,12 +281,12 @@ static int ali1535_transaction(struct i2
>  
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
> -		result = -1;
> +		result = -ETIMEDOUT;
>  		dev_err(&adap->dev, "SMBus Timeout!\n");
>  	}
>  
>  	if (temp & ALI1535_STS_FAIL) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
>  	}
>  
> @@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2
>  	 * do a printk.  This means that bus collisions go unreported.
>  	 */
>  	if (temp & ALI1535_STS_BUSERR) {
> -		result = -1;
> +		result = -EIO;

Given the comment above I'd make this -ENODEV. 99% of the time, no ack
means no device.

>  		dev_dbg(&adap->dev,
>  			"Error: no response or bus collision ADD=%02x\n",
>  			inb_p(SMBHSTADD));
> @@ -303,13 +303,13 @@ static int ali1535_transaction(struct i2
>  
>  	/* haven't ever seen this */
>  	if (temp & ALI1535_STS_DEV) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&adap->dev, "Error: device error\n");
>  	}
>  
>  	/* check to see if the "command complete" indication is set */
>  	if (!(temp & ALI1535_STS_DONE)) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&adap->dev, "Error: command never completed\n");
>  	}
>  
> @@ -332,7 +332,7 @@ static int ali1535_transaction(struct i2
>  	return result;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 ali1535_access(struct i2c_adapter *adap, u16 addr,
>  			  unsigned short flags, char read_write, u8 command,
>  			  int size, union i2c_smbus_data *data)
> @@ -359,7 +359,7 @@ static s32 ali1535_access(struct i2c_ada
>  	switch (size) {
>  	case I2C_SMBUS_PROC_CALL:
>  		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> -		result = -1;
> +		result = -EOPNOTSUPP;
>  		goto EXIT;

Side note for a next patch: there's a bug here, this should be handled
by a catch-all at the end on the switch, but I see it is missing.

>  	case I2C_SMBUS_QUICK:
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
> @@ -420,11 +420,9 @@ static s32 ali1535_access(struct i2c_ada
>  		break;
>  	}
>  
> -	if (ali1535_transaction(adap)) {
> -		/* Error in transaction */
> -		result = -1;
> +	result = ali1535_transaction(adap);
> +	if (result)
>  		goto EXIT;
> -	}
>  
>  	if ((read_write == I2C_SMBUS_WRITE) || (size == ALI1535_QUICK)) {
>  		result = 0;
> --- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-01 16:01:39.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-01 16:02:58.000000000 -0700
> @@ -122,7 +122,7 @@ static int ali1563_transaction(struct i2
>  		outb_p(0x0,SMB_HST_CNTL2);
>  	}
>  
> -	return -1;
> +	return -EIO;
>  }

Here too, I think -ENODEV would be more appropriate in the "bus
error" (no answer case).

>  
>  static int ali1563_block_start(struct i2c_adapter * a)
> @@ -170,7 +170,7 @@ static int ali1563_block_start(struct i2
>  		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 -EIO;
>  }

And same here. That's a little more work, admittedly.

>  
>  static int ali1563_block(struct i2c_adapter * a, union i2c_smbus_data * data, u8 rw)
> --- g26.orig/drivers/i2c/busses/i2c-ali15x3.c	2008-05-01 16:01:39.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-ali15x3.c	2008-05-01 16:02:58.000000000 -0700
> @@ -282,7 +282,7 @@ static int ali15x3_transaction(struct i2
>  			dev_err(&adap->dev, "SMBus reset failed! (0x%02x) - "
>  				"controller or device on bus is probably hung\n",
>  				temp);
> -			return -1;
> +			return -EBUSY;
>  		}
>  	} else {
>  		/* check and clear done bit */
> @@ -304,12 +304,12 @@ static int ali15x3_transaction(struct i2
>  
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
> -		result = -1;
> +		result = -ETIMEDOUT;
>  		dev_err(&adap->dev, "SMBus Timeout!\n");
>  	}
>  
>  	if (temp & ALI15X3_STS_TERM) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
>  	}
>  
> @@ -320,7 +320,7 @@ static int ali15x3_transaction(struct i2
>  	  This means that bus collisions go unreported.
>  	*/
>  	if (temp & ALI15X3_STS_COLL) {
> -		result = -1;
> +		result = -EIO;

Here too, -ENODEV.

>  		dev_dbg(&adap->dev,
>  			"Error: no response or bus collision ADD=%02x\n",
>  			inb_p(SMBHSTADD));
> @@ -328,7 +328,7 @@ static int ali15x3_transaction(struct i2
>  
>  	/* haven't ever seen this */
>  	if (temp & ALI15X3_STS_DEV) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&adap->dev, "Error: device error\n");
>  	}
>  	dev_dbg(&adap->dev, "Transaction (post): STS=%02x, CNT=%02x, CMD=%02x, "
> @@ -338,7 +338,7 @@ static int ali15x3_transaction(struct i2
>  	return result;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 ali15x3_access(struct i2c_adapter * adap, u16 addr,
>  		   unsigned short flags, char read_write, u8 command,
>  		   int size, union i2c_smbus_data * data)
> @@ -364,7 +364,7 @@ static s32 ali15x3_access(struct i2c_ada
>  	switch (size) {
>  	case I2C_SMBUS_PROC_CALL:
>  		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> -		return -1;
> +		return -EOPNOTSUPP;

Same bug as i2c-ali1535 (not your fault of course.) I guess we should
check all drivers...

>  	case I2C_SMBUS_QUICK:
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>  		       SMBHSTADD);
> @@ -421,8 +421,9 @@ static s32 ali15x3_access(struct i2c_ada
>  
>  	outb_p(size, SMBHSTCNT);	/* output command */
>  
> -	if (ali15x3_transaction(adap))	/* Error in transaction */
> -		return -1;
> +	temp = ali15x3_transaction(adap);
> +	if (temp)
> +		return temp;
>  
>  	if ((read_write == I2C_SMBUS_WRITE) || (size == ALI15X3_QUICK))
>  		return 0;
> --- g26.orig/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:02:58.000000000 -0700
> @@ -58,7 +58,7 @@ static s32 amd756_access_virt0(struct i2
>  	/* We exclude the multiplexed addresses */
>  	if (addr == 0x4c || (addr & 0xfc) == 0x50 || (addr & 0xfc) == 0x30
>  	 || addr == 0x18)
> -		return -1;
> +		return -EINVAL;
>  
>  	mutex_lock(&amd756_lock);
>  
> @@ -86,7 +86,7 @@ static inline s32 amd756_access_channel(
>  
>  	/* We exclude the non-multiplexed addresses */
>  	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
> -		return -1;
> +		return -EINVAL;
>  
>  	mutex_lock(&amd756_lock);

For these two I'd go for -ENODEV as well. It's a little unfair to
return -EINVAL, given that we are hiding the devices from the caller on
purpose and they couldn't guess.

>  
> --- g26.orig/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:01:39.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:02:58.000000000 -0700
> @@ -151,17 +151,17 @@ static int amd756_transaction(struct i2c
>  	}
>  
>  	if (temp & GS_PRERR_STS) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&adap->dev, "SMBus Protocol error (no response)!\n");

This one would be -ENODEV.

>  	}
>  
>  	if (temp & GS_COL_STS) {
> -		result = -1;
> +		result = -EIO;
>  		dev_warn(&adap->dev, "SMBus collision!\n");
>  	}
>  
>  	if (temp & GS_TO_STS) {
> -		result = -1;
> +		result = -ETIMEDOUT;
>  		dev_dbg(&adap->dev, "SMBus protocol timeout!\n");
>  	}
>  
> @@ -189,22 +189,23 @@ static int amd756_transaction(struct i2c
>  	outw_p(inw(SMB_GLOBAL_ENABLE) | GE_ABORT, SMB_GLOBAL_ENABLE);
>  	msleep(100);
>  	outw_p(GS_CLEAR_STS, SMB_GLOBAL_STATUS);
> -	return -1;
> +	return -EIO;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 amd756_access(struct i2c_adapter * adap, u16 addr,
>  		  unsigned short flags, char read_write,
>  		  u8 command, int size, union i2c_smbus_data * data)
>  {
>  	int i, len;
> +	int status;
>  
>  	/** TODO: Should I supporte the 10-bit transfers? */
>  	switch (size) {
>  	case I2C_SMBUS_PROC_CALL:
>  		dev_dbg(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
>  		/* TODO: Well... It is supported, I'm just not sure what to do here... */
> -		return -1;
> +		return -EOPNOTSUPP;
>  	case I2C_SMBUS_QUICK:
>  		outw_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>  		       SMB_HOST_ADDRESS);
> @@ -256,8 +257,9 @@ static s32 amd756_access(struct i2c_adap
>  	/* How about enabling interrupts... */
>  	outw_p(size & GE_CYC_TYPE_MASK, SMB_GLOBAL_ENABLE);
>  
> -	if (amd756_transaction(adap))	/* Error in transaction */
> -		return -1;
> +	status = amd756_transaction(adap);
> +	if (status)
> +		return status;
>  
>  	if ((read_write == I2C_SMBUS_WRITE) || (size == AMD756_QUICK))
>  		return 0;
> --- g26.orig/drivers/i2c/busses/i2c-amd8111.c	2008-05-01 16:01:39.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-amd8111.c	2008-05-01 16:02:58.000000000 -0700
> @@ -77,7 +77,7 @@ static unsigned int amd_ec_wait_write(st
>  	if (!timeout) {
>  		dev_warn(&smbus->dev->dev,
>  			 "Timeout while waiting for IBF to clear\n");
> -		return -1;
> +		return -ETIMEDOUT;
>  	}
>  
>  	return 0;
> @@ -93,7 +93,7 @@ static unsigned int amd_ec_wait_read(str
>  	if (!timeout) {
>  		dev_warn(&smbus->dev->dev,
>  			 "Timeout while waiting for OBF to set\n");
> -		return -1;
> +		return -ETIMEDOUT;
>  	}
>  
>  	return 0;
> @@ -102,16 +102,21 @@ static unsigned int amd_ec_wait_read(str
>  static unsigned int amd_ec_read(struct amd_smbus *smbus, unsigned char address,
>  		unsigned char *data)
>  {
> -	if (amd_ec_wait_write(smbus))
> -		return -1;
> +	int status;
> +
> +	status = amd_ec_wait_write(smbus);
> +	if (status)
> +		return status;
>  	outb(AMD_EC_CMD_RD, smbus->base + AMD_EC_CMD);
>  
> -	if (amd_ec_wait_write(smbus))
> -		return -1;
> +	status = amd_ec_wait_write(smbus);
> +	if (status)
> +		return status;
>  	outb(address, smbus->base + AMD_EC_DATA);
>  
> -	if (amd_ec_wait_read(smbus))
> -		return -1;
> +	status = amd_ec_wait_read(smbus);
> +	if (status)
> +		return status;
>  	*data = inb(smbus->base + AMD_EC_DATA);
>  
>  	return 0;
> @@ -120,16 +125,21 @@ static unsigned int amd_ec_read(struct a
>  static unsigned int amd_ec_write(struct amd_smbus *smbus, unsigned char address,
>  		unsigned char data)
>  {
> -	if (amd_ec_wait_write(smbus))
> -		return -1;
> +	int status;
> +
> +	status = amd_ec_wait_write(smbus);
> +	if (status)
> +		return status;
>  	outb(AMD_EC_CMD_WR, smbus->base + AMD_EC_CMD);
>  
> -	if (amd_ec_wait_write(smbus))
> -		return -1;
> +	status = amd_ec_wait_write(smbus);
> +	if (status)
> +		return status;
>  	outb(address, smbus->base + AMD_EC_DATA);
>  
> -	if (amd_ec_wait_write(smbus))
> -		return -1;
> +	status = amd_ec_wait_write(smbus);
> +	if (status)
> +		return status;
>  	outb(data, smbus->base + AMD_EC_DATA);
>  
>  	return 0;
> @@ -185,6 +195,7 @@ static s32 amd8111_access(struct i2c_ada
>  	struct amd_smbus *smbus = adap->algo_data;
>  	unsigned char protocol, len, pec, temp[2];
>  	int i;
> +	int status;
>  
>  	protocol = (read_write == I2C_SMBUS_READ) ? AMD_SMB_PRTCL_READ
>  						  : AMD_SMB_PRTCL_WRITE;
> @@ -267,12 +278,13 @@ static s32 amd8111_access(struct i2c_ada
>  
>  		default:
>  			dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> -			return -1;
> +			return -EOPNOTSUPP;
>  	}
>  
>  	amd_ec_write(smbus, AMD_SMB_ADDR, addr << 1);
>  	amd_ec_write(smbus, AMD_SMB_PRTCL, protocol);
>  
> +	/* FIXME this discards status from ec_read() ... */
>  	amd_ec_read(smbus, AMD_SMB_STS, temp + 0);

Same is true of almost all amd_ec_write and amd_ec_read calls, so I
don't get the point of adding a comment for this one occurrence in
particular.

>  
>  	if (~temp[0] & AMD_SMB_STS_DONE) {
> @@ -286,7 +298,7 @@ static s32 amd8111_access(struct i2c_ada
>  	}
>  
>  	if ((~temp[0] & AMD_SMB_STS_DONE) || (temp[0] & AMD_SMB_STS_STATUS))
> -		return -1;
> +		return -EIO;
>  
>  	if (read_write == I2C_SMBUS_WRITE)
>  		return 0;
> --- g26.orig/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:02:58.000000000 -0700
> @@ -151,7 +151,7 @@ static int i801_transaction(int xact)
>  		outb_p(temp, SMBHSTSTS);
>  		if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
>  			dev_dbg(&I801_dev->dev, "Failed! (%02x)\n", temp);
> -			return -1;
> +			return -EBUSY;
>  		} else {
>  			dev_dbg(&I801_dev->dev, "Successful!\n");
>  		}
> @@ -170,7 +170,7 @@ static int i801_transaction(int xact)
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
>  		dev_dbg(&I801_dev->dev, "SMBus Timeout!\n");
> -		result = -1;
> +		result = -ETIMEDOUT;
>  		/* try to stop the current command */
>  		dev_dbg(&I801_dev->dev, "Terminating the current operation\n");
>  		outb_p(inb_p(SMBHSTCNT) | SMBHSTCNT_KILL, SMBHSTCNT);
> @@ -179,19 +179,19 @@ static int i801_transaction(int xact)
>  	}
>  
>  	if (temp & SMBHSTSTS_FAILED) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&I801_dev->dev, "Error: Failed bus transaction\n");
>  	}
>  
>  	if (temp & SMBHSTSTS_BUS_ERR) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&I801_dev->dev, "Bus collision! SMBus may be locked "
>  			"until next hard reset. (sorry!)\n");
>  		/* Clock stops and slave is stuck in mid-transmission */
>  	}
>  
>  	if (temp & SMBHSTSTS_DEV_ERR) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&I801_dev->dev, "Error: no response!\n");

-ENODEV

>  	}
>  
> @@ -231,6 +231,7 @@ static int i801_block_transaction_by_blo
>  					   char read_write, int hwpec)
>  {
>  	int i, len;
> +	int status;
>  
>  	inb_p(SMBHSTCNT); /* reset the data buffer index */
>  
> @@ -242,14 +243,15 @@ static int i801_block_transaction_by_blo
>  			outb_p(data->block[i+1], SMBBLKDAT);
>  	}
>  
> -	if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
> -			     I801_PEC_EN * hwpec))
> -		return -1;
> +	status = i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
> +			     I801_PEC_EN * hwpec);

Please preserve the alignment on opening parenthesis.

> +	if (status)
> +		return status;
>  
>  	if (read_write == I2C_SMBUS_READ) {
>  		len = inb_p(SMBHSTDAT0);
>  		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
> -			return -1;
> +			return -EILSEQ;

Huh, definitely not. This error code has to so with multi-byte
character encoding, that's inappropriate here. I'd return -EINVAL, or
-EREMOTEIO if you think EINVAL is unfair for the caller. But most
likely, if the returned length is not within the SMBus specifications,
it means that the caller ran an SMBus block transaction at the wrong
offset or on a device that doesn't support it, so -EINVAL seems
reasonable to me.

>  
>  		data->block[0] = len;
>  		for (i = 0; i < len; i++)
> @@ -314,11 +316,11 @@ static int i801_block_transaction_byte_b
>  			if (((temp = inb_p(SMBHSTSTS)) & errmask) != 0x00) {
>  				dev_err(&I801_dev->dev,
>  					"Reset failed! (%02x)\n", temp);
> -				return -1;
> +				return -EBUSY;
>  			}
>  			if (i != 1)
>  				/* if die in middle of block transaction, fail */
> -				return -1;
> +				return -EIO;
>  		}
>  
>  		if (i == 1)
> @@ -342,19 +344,19 @@ static int i801_block_transaction_byte_b
>  			msleep(1);
>  			outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL),
>  				SMBHSTCNT);
> -			result = -1;
> +			result = -ETIMEDOUT;
>  			dev_dbg(&I801_dev->dev, "SMBus Timeout!\n");
>  		}
>  
>  		if (temp & SMBHSTSTS_FAILED) {
> -			result = -1;
> +			result = -EIO;
>  			dev_dbg(&I801_dev->dev,
>  				"Error: Failed bus transaction\n");
>  		} else if (temp & SMBHSTSTS_BUS_ERR) {
> -			result = -1;
> +			result = -EIO;
>  			dev_err(&I801_dev->dev, "Bus collision!\n");
>  		} else if (temp & SMBHSTSTS_DEV_ERR) {
> -			result = -1;
> +			result = -EIO;
>  			dev_dbg(&I801_dev->dev, "Error: no response!\n");

-ENODEV

>  		}
>  
> @@ -362,7 +364,7 @@ static int i801_block_transaction_byte_b
>  		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
>  			len = inb_p(SMBHSTDAT0);
>  			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
> -				return -1;
> +				return -EILSEQ;

Same as above.

>  			data->block[0] = len;
>  		}
>  
> @@ -383,7 +385,6 @@ static int i801_block_transaction_byte_b
>  			"ADD=%02x, DAT0=%02x, DAT1=%02x, BLKDAT=%02x\n", i,
>  			inb_p(SMBHSTCNT), inb_p(SMBHSTCMD), inb_p(SMBHSTADD),
>  			inb_p(SMBHSTDAT0), inb_p(SMBHSTDAT1), inb_p(SMBBLKDAT));
> -
>  		if (result < 0)
>  			return result;
>  	}

Pointless, please revert.

> @@ -394,7 +395,7 @@ static int i801_set_block_buffer_mode(vo
>  {
>  	outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
>  	if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0)
> -		return -1;
> +		return -EIO;
>  	return 0;
>  }
>  
> @@ -414,7 +415,7 @@ static int i801_block_transaction(union 
>  		} else if (!(i801_features & FEATURE_I2C_BLOCK_READ)) {
>  			dev_err(&I801_dev->dev,
>  				"I2C block read is unsupported!\n");
> -			return -1;
> +			return -EOPNOTSUPP;
>  		}
>  	}
>  
> @@ -449,7 +450,7 @@ static int i801_block_transaction(union 
>  	return result;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 i801_access(struct i2c_adapter * adap, u16 addr,
>  		       unsigned short flags, char read_write, u8 command,
>  		       int size, union i2c_smbus_data * data)
> @@ -514,7 +515,7 @@ static s32 i801_access(struct i2c_adapte
>  	case I2C_SMBUS_PROC_CALL:
>  	default:
>  		dev_err(&I801_dev->dev, "Unsupported transaction %d\n", size);
> -		return -1;
> +		return -EOPNOTSUPP;
>  	}
>  
>  	if (hwpec)	/* enable/disable hardware PEC */
> @@ -537,7 +538,7 @@ static s32 i801_access(struct i2c_adapte
>  	if(block)
>  		return ret;
>  	if(ret)
> -		return -1;
> +		return ret;
>  	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
>  		return 0;
>  
> --- g26.orig/drivers/i2c/busses/i2c-nforce2.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-nforce2.c	2008-05-01 16:02:58.000000000 -0700
> @@ -145,16 +145,16 @@ static int nforce2_check_status(struct i
>  		dev_dbg(&adap->dev, "SMBus Timeout!\n");
>  		if (smbus->can_abort)
>  			nforce2_abort(adap);
> -		return -1;
> +		return -ETIMEDOUT;
>  	}
>  	if (!(temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
>  		dev_dbg(&adap->dev, "Transaction failed (0x%02x)!\n", temp);
> -		return -1;
> +		return -EIO;
>  	}
>  	return 0;
>  }
>  
> -/* Return -1 on error */
> +/* Return negative errno 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)
> @@ -162,7 +162,7 @@ static s32 nforce2_access(struct i2c_ada
>  	struct nforce2_smbus *smbus = adap->algo_data;
>  	unsigned char protocol, pec;
>  	u8 len;
> -	int i;
> +	int i, status;
>  
>  	protocol = (read_write == I2C_SMBUS_READ) ? NVIDIA_SMB_PRTCL_READ :
>  		NVIDIA_SMB_PRTCL_WRITE;
> @@ -206,7 +206,7 @@ static s32 nforce2_access(struct i2c_ada
>  						"Transaction failed "
>  						"(requested block size: %d)\n",
>  						len);
> -					return -1;
> +					return -E2BIG;

-E2BIG means argument list too long, it hardly applied here. Again,
-EINVAL seems appropriate to me.

>  				}
>  				outb_p(len, NVIDIA_SMB_BCNT);
>  				for (i = 0; i < I2C_SMBUS_BLOCK_MAX; i++)
> @@ -218,14 +218,15 @@ static s32 nforce2_access(struct i2c_ada
>  
>  		default:
>  			dev_err(&adap->dev, "Unsupported transaction %d\n", size);
> -			return -1;
> +			return -EOPNOTSUPP;
>  	}
>  
>  	outb_p((addr & 0x7f) << 1, NVIDIA_SMB_ADDR);
>  	outb_p(protocol, NVIDIA_SMB_PRTCL);
>  
> -	if (nforce2_check_status(adap))
> -		return -1;
> +	status = nforce2_check_status(adap);
> +	if (status)
> +		return status;
>  
>  	if (read_write == I2C_SMBUS_WRITE)
>  		return 0;
> @@ -247,7 +248,7 @@ static s32 nforce2_access(struct i2c_ada
>  				dev_err(&adap->dev, "Transaction failed "
>  					"(received block size: 0x%02x)\n",
>  					len);
> -				return -1;
> +				return -EILSEQ;
>  			}

See above.

>  			for (i = 0; i < len; i++)
>  				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
> @@ -308,7 +309,7 @@ static int __devinit nforce2_probe_smb (
>  		    != PCIBIOS_SUCCESSFUL) {
>  			dev_err(&dev->dev, "Error reading PCI config for %s\n",
>  				name);
> -			return -1;
> +			return -EIO;
>  		}
>  
>  		smbus->base = iobase & PCI_BASE_ADDRESS_IO_MASK;
> @@ -318,7 +319,7 @@ static int __devinit nforce2_probe_smb (
>  	if (!request_region(smbus->base, smbus->size, nforce2_driver.name)) {
>  		dev_err(&smbus->adapter.dev, "Error requesting region %02x .. %02X for %s\n",
>  			smbus->base, smbus->base+smbus->size-1, name);
> -		return -1;
> +		return -EBUSY;
>  	}
>  	smbus->adapter.owner = THIS_MODULE;
>  	smbus->adapter.id = I2C_HW_SMBUS_NFORCE2;
> @@ -333,7 +334,7 @@ static int __devinit nforce2_probe_smb (
>  	if (error) {
>  		dev_err(&smbus->adapter.dev, "Failed to register adapter.\n");
>  		release_region(smbus->base, smbus->size);
> -		return -1;
> +		return error;
>  	}
>  	dev_info(&smbus->adapter.dev, "nForce2 SMBus adapter at %#x\n", smbus->base);
>  	return 0;
> --- g26.orig/drivers/i2c/busses/i2c-piix4.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-piix4.c	2008-05-01 16:02:58.000000000 -0700
> @@ -220,7 +220,7 @@ static int piix4_transaction(void)
>  		outb_p(temp, SMBHSTSTS);
>  		if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
>  			dev_err(&piix4_adapter.dev, "Failed! (%02x)\n", temp);
> -			return -1;
> +			return -EBUSY;
>  		} else {
>  			dev_dbg(&piix4_adapter.dev, "Successful!\n");
>  		}
> @@ -238,23 +238,23 @@ static int piix4_transaction(void)
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
>  		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
> -		result = -1;
> +		result = -ETIMEDOUT;
>  	}
>  
>  	if (temp & 0x10) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&piix4_adapter.dev, "Error: Failed bus transaction\n");
>  	}
>  
>  	if (temp & 0x08) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&piix4_adapter.dev, "Bus collision! SMBus may be "
>  			"locked until next hard reset. (sorry!)\n");
>  		/* Clock stops and slave is stuck in mid-transmission */
>  	}
>  
>  	if (temp & 0x04) {
> -		result = -1;
> +		result = -EIO;
>  		dev_dbg(&piix4_adapter.dev, "Error: no response!\n");

-ENODEV

>  	}
>  
> @@ -272,17 +272,18 @@ static int piix4_transaction(void)
>  	return result;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
>  		 unsigned short flags, char read_write,
>  		 u8 command, int size, union i2c_smbus_data * data)
>  {
>  	int i, len;
> +	int status;
>  
>  	switch (size) {
>  	case I2C_SMBUS_PROC_CALL:
>  		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> -		return -1;
> +		return -EOPNOTSUPP;
>  	case I2C_SMBUS_QUICK:
>  		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
>  		       SMBHSTADD);
> @@ -334,8 +335,9 @@ static s32 piix4_access(struct i2c_adapt
>  
>  	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
>  
> -	if (piix4_transaction())	/* Error in transaction */
> -		return -1;
> +	status = piix4_transaction();
> +	if (status)
> +		return status;
>  
>  	if ((read_write == I2C_SMBUS_WRITE) || (size == PIIX4_QUICK))
>  		return 0;
> --- g26.orig/drivers/i2c/busses/i2c-sis5595.c	2008-05-01 16:01:39.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-sis5595.c	2008-05-01 16:07:18.000000000 -0700
> @@ -236,7 +236,7 @@ static int sis5595_transaction(struct i2
>  		sis5595_write(SMB_STS_HI, temp >> 8);
>  		if ((temp = sis5595_read(SMB_STS_LO) + (sis5595_read(SMB_STS_HI) << 8)) != 0x00) {
>  			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
> -			return -1;
> +			return -EBUSY;
>  		} else {
>  			dev_dbg(&adap->dev, "Successful!\n");
>  		}
> @@ -254,19 +254,19 @@ static int sis5595_transaction(struct i2
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
>  		dev_dbg(&adap->dev, "SMBus Timeout!\n");
> -		result = -1;
> +		result = -ETIMEDOUT;
>  	}
>  
>  	if (temp & 0x10) {
>  		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
> -		result = -1;
> +		result = -EIO;

-ENODEV

>  	}
>  
>  	if (temp & 0x20) {
>  		dev_err(&adap->dev, "Bus collision! SMBus may be locked until "
>  			"next hard reset (or not...)\n");
>  		/* Clock stops and slave is stuck in mid-transmission */
> -		result = -1;
> +		result = -EIO;
>  	}
>  
>  	temp = sis5595_read(SMB_STS_LO) + (sis5595_read(SMB_STS_HI) << 8);
> @@ -282,11 +282,13 @@ static int sis5595_transaction(struct i2
>  	return result;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 sis5595_access(struct i2c_adapter *adap, u16 addr,
>  			  unsigned short flags, char read_write,
>  			  u8 command, int size, union i2c_smbus_data *data)
>  {
> +	int status;
> +
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
>  		sis5595_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
> @@ -318,13 +320,14 @@ static s32 sis5595_access(struct i2c_ada
>  		break;
>  	default:
>  		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
> -		return -1;
> +		return -EOPNOTSUPP;
>  	}
>  
>  	sis5595_write(SMB_CTL_LO, ((size & 0x0E)));
>  
> -	if (sis5595_transaction(adap))
> -		return -1;
> +	status = sis5595_transaction(adap);
> +	if (status)
> +		return status;
>  
>  	if ((size != SIS5595_PROC_CALL) &&
>  	    ((read_write == I2C_SMBUS_WRITE) || (size == SIS5595_QUICK)))
> --- g26.orig/drivers/i2c/busses/i2c-sis630.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-sis630.c	2008-05-01 16:02:58.000000000 -0700
> @@ -134,7 +134,7 @@ static int sis630_transaction_start(stru
>  
>  		if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
>  			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
> -			return -1;
> +			return -EBUSY;
>                  } else {
>  			dev_dbg(&adap->dev, "Successful!\n");
>  		}
> @@ -177,17 +177,17 @@ static int sis630_transaction_wait(struc
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
>  		dev_dbg(&adap->dev, "SMBus Timeout!\n");
> -		result = -1;
> +		result = -ETIMEDOUT;
>  	}
>  
>  	if (temp & 0x02) {
>  		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
> -		result = -1;
> +		result = -EIO;
>  	}

-ENODEV

>  
>  	if (temp & 0x04) {
>  		dev_err(&adap->dev, "Bus collision!\n");
> -		result = -1;
> +		result = -EIO;
>  		/*
>  		  TBD: Datasheet say:
>  		  the software should clear this bit and restart SMBUS operation.
> @@ -250,8 +250,10 @@ static int sis630_block_data(struct i2c_
>  			if (i==8 || (len<8 && i==len)) {
>  				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
>  				/* first transaction */
> -				if (sis630_transaction_start(adap, SIS630_BLOCK_DATA, &oldclock))
> -					return -1;
> +				rc = sis630_transaction_start(adap,
> +						SIS630_BLOCK_DATA, &oldclock);
> +				if (rc)
> +					return rc;
>  			}
>  			else if ((i-1)%8 == 7 || i==len) {
>  				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
> @@ -264,9 +266,10 @@ static int sis630_block_data(struct i2c_
>  					*/
>  					sis630_write(SMB_STS,0x10);
>  				}
> -				if (sis630_transaction_wait(adap, SIS630_BLOCK_DATA)) {
> +				rc = sis630_transaction_wait(adap,
> +						SIS630_BLOCK_DATA);
> +				if (rc) {
>  					dev_dbg(&adap->dev, "trans_wait failed\n");
> -					rc = -1;
>  					break;
>  				}
>  			}
> @@ -275,13 +278,14 @@ static int sis630_block_data(struct i2c_
>  	else {
>  		/* read request */
>  		data->block[0] = len = 0;
> -		if (sis630_transaction_start(adap, SIS630_BLOCK_DATA, &oldclock)) {
> -			return -1;
> -		}
> +		rc = sis630_transaction_start(adap,
> +				SIS630_BLOCK_DATA, &oldclock);
> +		if (rc)
> +			return rc;
>  		do {
> -			if (sis630_transaction_wait(adap, SIS630_BLOCK_DATA)) {
> +			rc = sis630_transaction_wait(adap, SIS630_BLOCK_DATA);
> +			if (rc) {
>  				dev_dbg(&adap->dev, "trans_wait failed\n");
> -				rc = -1;
>  				break;
>  			}
>  			/* if this first transaction then read byte count */
> @@ -311,11 +315,13 @@ static int sis630_block_data(struct i2c_
>  	return rc;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
>  			 unsigned short flags, char read_write,
>  			 u8 command, int size, union i2c_smbus_data *data)
>  {
> +	int status;
> +
>  	switch (size) {
>  		case I2C_SMBUS_QUICK:
>  			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
> @@ -351,12 +357,13 @@ static s32 sis630_access(struct i2c_adap
>  			return sis630_block_data(adap, data, read_write);
>  		default:
>  			printk("Unsupported I2C size\n");
> -			return -1;
> +			return -EMSGSIZE;
>  			break;

Don't get fooled by the name of the variable and the log message...
"size" is an SMBus transaction type, it doesn't have much to do with a
size. Use -EOPNOTSUPP as for the other drivers.

>  	}
>  
> -	if (sis630_transaction(adap, size))
> -		return -1;
> +	status = sis630_transaction(adap, size);
> +	if (status)
> +		return status;
>  
>  	if ((size != SIS630_PCALL) &&
>  		((read_write == I2C_SMBUS_WRITE) || (size == SIS630_QUICK))) {
> @@ -373,7 +380,7 @@ static s32 sis630_access(struct i2c_adap
>  			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
>  			break;
>  		default:
> -			return -1;
> +			return -EOPNOTSUPP;
>  			break;
>  	}
>  
> --- g26.orig/drivers/i2c/busses/i2c-sis96x.c	2008-05-01 16:01:38.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-sis96x.c	2008-05-01 16:02:58.000000000 -0700
> @@ -111,7 +111,7 @@ static int sis96x_transaction(int size)
>  		/* check it again */
>  		if (((temp = sis96x_read(SMB_CNT)) & 0x03) != 0x00) {
>  			dev_dbg(&sis96x_adapter.dev, "Failed (0x%02x)\n", temp);
> -			return -1;
> +			return -EBUSY;
>  		} else {
>  			dev_dbg(&sis96x_adapter.dev, "Successful\n");
>  		}
> @@ -136,19 +136,19 @@ static int sis96x_transaction(int size)
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
>  		dev_dbg(&sis96x_adapter.dev, "SMBus Timeout! (0x%02x)\n", temp);
> -		result = -1;
> +		result = -ETIMEDOUT;
>  	}
>  
>  	/* device error - probably missing ACK */
>  	if (temp & 0x02) {
>  		dev_dbg(&sis96x_adapter.dev, "Failed bus transaction!\n");
> -		result = -1;
> +		result = -EIO;
>  	}
>  
>  	/* bus collision */
>  	if (temp & 0x04) {
>  		dev_dbg(&sis96x_adapter.dev, "Bus collision!\n");
> -		result = -1;
> +		result = -EIO;
>  	}

-ENODEV

>  
>  	/* Finish up by resetting the bus */
> @@ -161,11 +161,12 @@ static int sis96x_transaction(int size)
>  	return result;
>  }
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 sis96x_access(struct i2c_adapter * adap, u16 addr,
>  			 unsigned short flags, char read_write,
>  			 u8 command, int size, union i2c_smbus_data * data)
>  {
> +	int status;
>  
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
> @@ -203,17 +204,18 @@ static s32 sis96x_access(struct i2c_adap
>  	case I2C_SMBUS_BLOCK_DATA:
>  		/* TO DO: */
>  		dev_info(&adap->dev, "SMBus block not implemented!\n");
> -		return -1;
> +		return -EOPNOTSUPP;
>  		break;
>  
>  	default:
>  		dev_info(&adap->dev, "Unsupported I2C size\n");
> -		return -1;
> +		return -EMSGSIZE;

-EOPNOTSUPP

>  		break;
>  	}
>  
> -	if (sis96x_transaction(size))
> -		return -1;
> +	status = sis96x_transaction(size);
> +	if (status)
> +		return status;
>  
>  	if ((size != SIS96x_PROC_CALL) &&
>  		((read_write == I2C_SMBUS_WRITE) || (size == SIS96x_QUICK)))
> --- g26.orig/drivers/i2c/busses/i2c-stub.c	2008-05-01 16:10:15.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-stub.c	2008-05-01 16:10:46.000000000 -0700
> @@ -43,7 +43,7 @@ struct stub_chip {
>  
>  static struct stub_chip *stub_chips;
>  
> -/* Return -1 on error. */
> +/* Return negative errno on error. */
>  static s32 stub_xfer(struct i2c_adapter * adap, u16 addr, unsigned short flags,
>  	char read_write, u8 command, int size, union i2c_smbus_data * data)
>  {
> @@ -120,7 +120,7 @@ static s32 stub_xfer(struct i2c_adapter 
>  
>  	default:
>  		dev_dbg(&adap->dev, "Unsupported I2C/SMBus command\n");
> -		ret = -1;
> +		ret = -EOPNOTSUPP;
>  		break;
>  	} /* switch (size) */
>  
> --- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-01 17:12:45.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-01 17:15:03.000000000 -0700
> @@ -152,7 +152,7 @@ static int vt596_transaction(u8 size)
>  		if ((temp = inb_p(SMBHSTSTS)) & 0x1F) {
>  			dev_err(&vt596_adapter.dev, "SMBus reset failed! "
>  				"(0x%02x)\n", temp);
> -			return -1;
> +			return -EBUSY;
>  		}
>  	}
>  
> @@ -167,24 +167,24 @@ static int vt596_transaction(u8 size)
>  
>  	/* If the SMBus is still busy, we give up */
>  	if (timeout >= MAX_TIMEOUT) {
> -		result = -1;
> +		result = -ETIMEDOUT;
>  		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
>  	}
>  
>  	if (temp & 0x10) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&vt596_adapter.dev, "Transaction failed (0x%02x)\n",
>  			size);
>  	}
>  
>  	if (temp & 0x08) {
> -		result = -1;
> +		result = -EIO;
>  		dev_err(&vt596_adapter.dev, "SMBus collision!\n");
>  	}
>  
>  	if (temp & 0x04) {
>  		int read = inb_p(SMBHSTADD) & 0x01;
> -		result = -1;
> +		result = -EIO;

-ENODEV

>  		/* 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. */
> @@ -202,12 +202,13 @@ static int vt596_transaction(u8 size)
>  	return result;
>  }
>  
> -/* Return -1 on error, 0 on success */
> +/* Return negative errno on error, 0 on success */
>  static s32 vt596_access(struct i2c_adapter *adap, u16 addr,
>  		unsigned short flags, char read_write, u8 command,
>  		int size, union i2c_smbus_data *data)
>  {
>  	int i;
> +	int status;
>  
>  	switch (size) {
>  	case I2C_SMBUS_QUICK:
> @@ -258,8 +259,9 @@ static s32 vt596_access(struct i2c_adapt
>  
>  	outb_p(((addr & 0x7f) << 1) | read_write, SMBHSTADD);
>  
> -	if (vt596_transaction(size)) /* Error in transaction */
> -		return -1;
> +	status = vt596_transaction(size);
> +	if (status)
> +		return status;
>  
>  	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
>  		return 0;
> @@ -287,7 +289,7 @@ static s32 vt596_access(struct i2c_adapt
>  exit_unsupported:
>  	dev_warn(&vt596_adapter.dev, "Unsupported command invoked! (0x%02x)\n",
>  		 size);
> -	return -1;
> +	return -EOPNOTSUPP;
>  }
>  
>  static u32 vt596_func(struct i2c_adapter *adapter)

Please send an updated patch and I'll push it to -next and -mm so that
it gets some testing.

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 10 May 2008, Jean Delvare wrote:
> >  drivers/i2c/.............................
> >  15 files changed, 176 insertions(+), 142 deletions(-)

So I'll probably respond to your comments in smaller chunks.  ;)

Arguably this should become a bunch of smaller patches, but that 
would of course make more confusion during initial review for
this type change ... harder to notice the patterns.


> > --- g26.orig/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:01:38.000000000 -0700
> > +++ g26/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:02:58.000000000 -0700
> > 		...
> > @@ -465,9 +465,12 @@ static int bit_doAddress(struct i2c_adap
> >  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> >  
> >  	unsigned char addr;
> > -	int ret, retries;
> > +	int ret;
> > +	unsigned retries;
> >  
> > -	retries = nak_ok ? 0 : i2c_adap->retries;
> > +	retries = (nak_ok || i2c_adap->retries < 0)
> > +			? 0
> > +			: i2c_adap->retries;
> >  
> >  	if (flags & I2C_M_TEN) {
> >  		/* a ten bit address */
> 
> I'd rather not bother with the adap->retries stuff which we agreed
> should be removed anyway.

Until it's gone, I just wanted to be sane with it.  Or should
we just rip it out of this driver?  Negative retries is a very
nonsensical notion, and that's the only way I recall algo-bit
seemed to have obvious (albeit potential) fault code goofiness.

(Other than returning an odd code for devices that don't ACK
their address; see below.)


> > --- g26.orig/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:01:39.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:02:58.000000000 -0700
> > 			...
> > @@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2
> >  	 * do a printk.  This means that bus collisions go unreported.
> >  	 */
> >  	if (temp & ALI1535_STS_BUSERR) {
> > -		result = -1;
> > +		result = -EIO;
> 
> Given the comment above I'd make this -ENODEV. 99% of the time, no ack
> means no device.

Either ENODEV or ENXIO for "it didn't ACK the address".  Any
other code will make the driver core emit diagnostics when the
probe() fails ... diagnostics we want to avoid, when probe()
code does the Right Thing and just returns such fault codes.

That's probably an issue with most of these updates.


>		...
> > @@ -359,7 +359,7 @@ static s32 ali1535_access(struct i2c_ada
> >  	switch (size) {
> >  	case I2C_SMBUS_PROC_CALL:
> >  		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
> > -		result = -1;
> > +		result = -EOPNOTSUPP;
> >  		goto EXIT;
> 
> Side note for a next patch: there's a bug here, this should be handled
> by a catch-all at the end on the switch, but I see it is missing.

Right, several drivers copied the same goofy template and
its bugs.  (So assume I agree on repeatis of those issues,
in the interest of brevity.)


> > --- g26.orig/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:01:38.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:02:58.000000000 -0700
> > @@ -58,7 +58,7 @@ static s32 amd756_access_virt0(struct i2
> >  	/* We exclude the multiplexed addresses */
> >  	if (addr == 0x4c || (addr & 0xfc) == 0x50 || (addr & 0xfc) == 0x30
> >  	 || addr == 0x18)
> > -		return -1;
> > +		return -EINVAL;
> >  
> >  	mutex_lock(&amd756_lock);
> >  
> > @@ -86,7 +86,7 @@ static inline s32 amd756_access_channel(
> >  
> >  	/* We exclude the non-multiplexed addresses */
> >  	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
> > -		return -1;
> > +		return -EINVAL;
> >  
> >  	mutex_lock(&amd756_lock);
> 
> For these two I'd go for -ENODEV as well. It's a little unfair to
> return -EINVAL, given that we are hiding the devices from the caller on
> purpose and they couldn't guess.

That might be a case where -ENXIO could provide a useful distinction.
"No such device *OR ADDRESS*" vs "No such device".  ;)

 
> > --- g26.orig/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:01:39.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:02:58.000000000 -0700
> > @@ -151,17 +151,17 @@ static int amd756_transaction(struct i2c
> >  	}
> >  
> >  	if (temp & GS_PRERR_STS) {
> > -		result = -1;
> > +		result = -EIO;
> >  		dev_dbg(&adap->dev, "SMBus Protocol error (no response)!\n");
> 
> This one would be -ENODEV.

How are you knowing all these things?  I wouldn't interpret
"protocol error" as "no device".  And while I do happen to
have amd756 docs around (board broken though, sigh) I didn't
dig that info out ... I'd be less concerned with reporting
needlessly generic (EIO) errors than with wrongly reporting
ones that are too specific (ENODEV).

That's kind of a general issue with this patch.  I think my
first idea was to stop reporting "-EPERM" for everything and
provide plausible codes, but allow updates later.  I can't
imagine every fault report, the first time around, will be
the best one we could be reporting.


> > --- g26.orig/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:01:38.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:02:58.000000000 -0700
> > 			....

Though didn't I see a patch removing this driver?


> > @@ -179,19 +179,19 @@ static int i801_transaction(int xact)
> >  	}
> >  
> >  	if (temp & SMBHSTSTS_FAILED) {
> > -		result = -1;
> > +		result = -EIO;
> >  		dev_dbg(&I801_dev->dev, "Error: Failed bus transaction\n");
> >  	}
> >  
> >  	if (temp & SMBHSTSTS_BUS_ERR) {
> > -		result = -1;
> > +		result = -EIO;
> >  		dev_err(&I801_dev->dev, "Bus collision! SMBus may be locked "
> >  			"until next hard reset. (sorry!)\n");
> >  		/* Clock stops and slave is stuck in mid-transmission */
> >  	}
> >  
> >  	if (temp & SMBHSTSTS_DEV_ERR) {
> > -		result = -1;
> > +		result = -EIO;
> >  		dev_dbg(&I801_dev->dev, "Error: no response!\n");
> 
> -ENODEV

Is that *really* a "no response to address" error?

But there's another issue with that type of code.  This
particular one looks maybe sane ... in the sense that it
seems to replace generic fault codes with more specific
ones (maybe), but I don't think all the I2C adapters were
doing that.  Some of them looked like they were testing
the specific faults first, then clobbering them with vague
generic idicators...


> > +	if (status)
> > +		return status;
> >  
> >  	if (read_write == I2C_SMBUS_READ) {
> >  		len = inb_p(SMBHSTDAT0);
> >  		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
> > -			return -1;
> > +			return -EILSEQ;
> 
> Huh, definitely not. This error code has to so with multi-byte
> character encoding, that's inappropriate here.

No; most of the errno values get recycled.  "Illegal Byte Sequence"
is exactly what we got there, in fact!  I know that USB has used
EILSEQ for some years (indicating a lowlevel bit protocol failure),
and MMC is using it too.

So if you want to say "not", it should be for some better reason
than that...


> I'd return -EINVAL, or 
> -EREMOTEIO if you think EINVAL is unfair for the caller.

It's not an I/O error; the I/O actually worked, it's the bytes
that were wrong.  It's not an "argument" provided by the caller.
So neither of those codes seems appropriate.


> But most 
> likely, if the returned length is not within the SMBus specifications,
> it means that the caller ran an SMBus block transaction at the wrong
> offset or on a device that doesn't support it, so -EINVAL seems
> reasonable to me.

That's only a guess though.  All we *know* is that we saw an
illegal byte sequence...

 
... enough for now.

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi David,

Before I forget: I just noticed that your patch adds the following
warning:

  CC [M]  drivers/i2c/busses/i2c-amd8111.o
drivers/i2c/busses/i2c-amd8111.c: In function ‘amd8111_access’:
drivers/i2c/busses/i2c-amd8111.c:198: warning: unused variable ‘status’

Please fix.

On Sun, 11 May 2008 01:17:22 -0700, David Brownell wrote:
> On Saturday 10 May 2008, Jean Delvare wrote:
> > >  drivers/i2c/.............................
> > >  15 files changed, 176 insertions(+), 142 deletions(-)
> 
> So I'll probably respond to your comments in smaller chunks.  ;)
> 
> Arguably this should become a bunch of smaller patches, but that 
> would of course make more confusion during initial review for
> this type change ... harder to notice the patterns.

I am totally fine with all the drivers being fixed in the same patch, no
problem with this.

> > > --- g26.orig/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:01:38.000000000 -0700
> > > +++ g26/drivers/i2c/algos/i2c-algo-bit.c	2008-05-01 16:02:58.000000000 -0700
> > > 		...
> > > @@ -465,9 +465,12 @@ static int bit_doAddress(struct i2c_adap
> > >  	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
> > >  
> > >  	unsigned char addr;
> > > -	int ret, retries;
> > > +	int ret;
> > > +	unsigned retries;
> > >  
> > > -	retries = nak_ok ? 0 : i2c_adap->retries;
> > > +	retries = (nak_ok || i2c_adap->retries < 0)
> > > +			? 0
> > > +			: i2c_adap->retries;
> > >  
> > >  	if (flags & I2C_M_TEN) {
> > >  		/* a ten bit address */
> > 
> > I'd rather not bother with the adap->retries stuff which we agreed
> > should be removed anyway.
> 
> Until it's gone, I just wanted to be sane with it.

To make it sane, just change "ret = -1" to "ret = 0" in try_address().
It does the "right thing" if the i2c adapter provided a negative number
of retries (which nobody does anyway.) That's the minimal fix and the
only one I am willing to take.

> Or should we just rip it out of this driver? Negative retries is a
> very nonsensical notion, and that's the only way I recall algo-bit
> seemed to have obvious (albeit potential) fault code goofiness.

Not in the middle of your -1 -> -errno patches, no. But once we're done
with them and the dust has settled down, yes, chip drivers will
hopefully have a reliable way to know when then want to retry a failed
read, and we can get rid of this automatic retry mechanism at the
adapter level.

> > > --- g26.orig/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:01:39.000000000 -0700
> > > +++ g26/drivers/i2c/busses/i2c-ali1535.c	2008-05-01 16:02:58.000000000 -0700
> > > 			...
> > > @@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2
> > >  	 * do a printk.  This means that bus collisions go unreported.
> > >  	 */
> > >  	if (temp & ALI1535_STS_BUSERR) {
> > > -		result = -1;
> > > +		result = -EIO;
> > 
> > Given the comment above I'd make this -ENODEV. 99% of the time, no ack
> > means no device.
> 
> Either ENODEV or ENXIO for "it didn't ACK the address".  Any
> other code will make the driver core emit diagnostics when the
> probe() fails ... diagnostics we want to avoid, when probe()
> code does the Right Thing and just returns such fault codes.
> 
> That's probably an issue with most of these updates.

My point exactly. I am fine with both -ENXIO and -ENODEV, pick the one
you like, then make sure all drivers are using it consistently.

> > > --- g26.orig/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:01:38.000000000 -0700
> > > +++ g26/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-01 16:02:58.000000000 -0700
> > > @@ -58,7 +58,7 @@ static s32 amd756_access_virt0(struct i2
> > >  	/* We exclude the multiplexed addresses */
> > >  	if (addr == 0x4c || (addr & 0xfc) == 0x50 || (addr & 0xfc) == 0x30
> > >  	 || addr == 0x18)
> > > -		return -1;
> > > +		return -EINVAL;
> > >  
> > >  	mutex_lock(&amd756_lock);
> > >  
> > > @@ -86,7 +86,7 @@ static inline s32 amd756_access_channel(
> > >  
> > >  	/* We exclude the non-multiplexed addresses */
> > >  	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
> > > -		return -1;
> > > +		return -EINVAL;
> > >  
> > >  	mutex_lock(&amd756_lock);
> > 
> > For these two I'd go for -ENODEV as well. It's a little unfair to
> > return -EINVAL, given that we are hiding the devices from the caller on
> > purpose and they couldn't guess.
> 
> That might be a case where -ENXIO could provide a useful distinction.
> "No such device *OR ADDRESS*" vs "No such device".  ;)

What matters here is to return the same code that other bus drivers
return when they don't get a ack on a given transaction. The caller
shouldn't be able to differentiate between both cases, multiplexing
should be completely transparent to the chip drivers.

> > > --- g26.orig/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:01:39.000000000 -0700
> > > +++ g26/drivers/i2c/busses/i2c-amd756.c	2008-05-01 16:02:58.000000000 -0700
> > > @@ -151,17 +151,17 @@ static int amd756_transaction(struct i2c
> > >  	}
> > >  
> > >  	if (temp & GS_PRERR_STS) {
> > > -		result = -1;
> > > +		result = -EIO;
> > >  		dev_dbg(&adap->dev, "SMBus Protocol error (no response)!\n");
> > 
> > This one would be -ENODEV.
> 
> How are you knowing all these things?  I wouldn't interpret
> "protocol error" as "no device".  And while I do happen to
> have amd756 docs around (board broken though, sigh) I didn't
> dig that info out ... I'd be less concerned with reporting
> needlessly generic (EIO) errors than with wrongly reporting
> ones that are too specific (ENODEV).
> 
> That's kind of a general issue with this patch.  I think my
> first idea was to stop reporting "-EPERM" for everything and
> provide plausible codes, but allow updates later.  I can't
> imagine every fault report, the first time around, will be
> the best one we could be reporting.

I agree that it's not easy for you (or anyone else, including me) to
figure out the right error code all the time, and I am fine with using
-EIO as the default error value when there's no other obvious value to
return.

As for "how do I know": check whether the code is using dev_err() or
dev_dbg(). Each time a driver uses dev_dbg(), chances are good that the
error in question isn't really an error, and we've turned the error
message into a debug message to avoid log fills and user complaints. So
basically, when I see a dev_dbg() in these SMBus "access" functions, I
can be almost sure that the error in question is what that controller
returns when a transaction isn't acked. But that's only a heuristic - I
could be wrong, too.

A variant of this is when a driver uses dev_err() but hides the error
message on SMBus quick command and sometimes on SMBus receive byte as
well. These are the transactions used by i2cdetect and sensors-detect
to probe for I2C devices. When you see this you can be certain that
it's the error that is returned on no-ack.

> > > --- g26.orig/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:01:38.000000000 -0700
> > > +++ g26/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:02:58.000000000 -0700
> 
> Though didn't I see a patch removing this driver?

Hell no. You saw i2c-i810 go away, not i2c-i801. i2c-i801 drives the
Intel mainboards' SMBus, i2c-i810 was a bit-banging driver for the
integrated Intel video chips' DDC and I2C buses. i2c-i801 is there to
stay.

> > >  	if (temp & SMBHSTSTS_DEV_ERR) {
> > > -		result = -1;
> > > +		result = -EIO;
> > >  		dev_dbg(&I801_dev->dev, "Error: no response!\n");
> > 
> > -ENODEV
> 
> Is that *really* a "no response to address" error?

It uses dev_dbg() ;)

> But there's another issue with that type of code.  This
> particular one looks maybe sane ... in the sense that it
> seems to replace generic fault codes with more specific
> ones (maybe), but I don't think all the I2C adapters were
> doing that.  Some of them looked like they were testing
> the specific faults first, then clobbering them with vague
> generic idicators...

You're right, I have noticed this too... most of these drivers check
for many error conditions and overwrite the error value each time, so
the last error checked wins. We might have to reorder the checks so
that the best error code is kept, but honestly I'm not sure I would be
able to tell right away what the best order would be each time. So I am
fine leaving things as they are for now and fixing them later if they
really need to be.

> > > +	if (status)
> > > +		return status;
> > >  
> > >  	if (read_write == I2C_SMBUS_READ) {
> > >  		len = inb_p(SMBHSTDAT0);
> > >  		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
> > > -			return -1;
> > > +			return -EILSEQ;
> > 
> > Huh, definitely not. This error code has to so with multi-byte
> > character encoding, that's inappropriate here.
> 
> No; most of the errno values get recycled.  "Illegal Byte Sequence"
> is exactly what we got there, in fact!  I know that USB has used
> EILSEQ for some years (indicating a lowlevel bit protocol failure),
> and MMC is using it too.
> 
> So if you want to say "not", it should be for some better reason
> than that...

When calling strerror on EILSEQ, you get: "Invalid or incomplete
multibyte or wide character". This has nothing to do with the error in
question, and that's what I am worried about. I don't know whether it
makes sense or not in the USB and MMC code, but in the I2C/SMBus
context, I'm sure it does not.

> > I'd return -EINVAL, or 
> > -EREMOTEIO if you think EINVAL is unfair for the caller.
> 
> It's not an I/O error; the I/O actually worked, it's the bytes

True...

> that were wrong.  It's not an "argument" provided by the caller.
> So neither of those codes seems appropriate.

The returned value depends on an argument provided by the caller,
though.

> > But most 
> > likely, if the returned length is not within the SMBus specifications,
> > it means that the caller ran an SMBus block transaction at the wrong
> > offset or on a device that doesn't support it, so -EINVAL seems
> > reasonable to me.
> 
> That's only a guess though.  All we *know* is that we saw an
> illegal byte sequence...

That's not an illegal "sequence", sorry. The length byte by itself is
either valid or not valid. What we saw is an illegal byte value.

Yes, -EINVAL is a guess, but according to my experience, this guess
will always be correct. Leaving actual I/O errors aside, there are only
two ways that the SMBus block length byte we receive from an SMBus chip
can be incorrect: either the chip in question is returning invalid data
to a proper request, or the request was incorrect. The former would
mean that the chip is basically unusable, so it's very unlikely to
happen. As a matter of fact, I've never seen it (not that I have seen
that many chips using SMBus block reads, though.) That's why I say that
the latter will always be true in practice, and -EINVAL makes sense
then even if it's not perfect.

If you are still worried that -EINVAL has a small chance of not being
correct, I'd rather go for -EMSGSIZE (Message too long), although the
error message would be confusing in the 0 case. Even -EOVERFLOW (Value
too large for defined data type) would be more appropriate than
-EILSEQ, I think.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Sunday 11 May 2008, you wrote:

> > > Given the comment above I'd make this -ENODEV. 99% of the time, no ack
> > > means no device.
> > 
> > Either ENODEV or ENXIO for "it didn't ACK the address".  Any
> > other code will make the driver core emit diagnostics when the
> > probe() fails ... diagnostics we want to avoid, when probe()
> > code does the Right Thing and just returns such fault codes.
> > 
> > That's probably an issue with most of these updates.
> 
> My point exactly. I am fine with both -ENXIO and -ENODEV, pick the one
> you like, then make sure all drivers are using it consistently.

I'll go with ENXIO "no such device or address" as being a bit more
appropriate for addressing issues.  Driver probe() code can return
ENODEV in the case where there is a device there ... but the wrong
one for that driver.


> As for "how do I know": check whether the code is using dev_err() or
> dev_dbg(). Each time a driver uses dev_dbg(), chances are good that the
> error in question isn't really an error, and we've turned the error
> message into a debug message to avoid log fills and user complaints. So
> basically, when I see a dev_dbg() in these SMBus "access" functions, I
> can be almost sure that the error in question is what that controller
> returns when a transaction isn't acked. But that's only a heuristic - I
> could be wrong, too.

Makes sense to me.


> > > > --- g26.orig/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:01:38.000000000 -0700
> > > > +++ g26/drivers/i2c/busses/i2c-i801.c	2008-05-01 16:02:58.000000000 -0700
> > 
> > Though didn't I see a patch removing this driver?
> 
> Hell no. You saw i2c-i810 go away, not i2c-i801.

10, 01 ... it's just an off-by-one error (in binary).  ;)


> > > > -			return -1;
> > > > +			return -EILSEQ;
> > > 
> > > Huh, definitely not. This error code has to so with multi-byte
> > > character encoding, that's inappropriate here.
> > 
> > No; most of the errno values get recycled.  "Illegal Byte Sequence"
> > is exactly what we got there, in fact!  I know that USB has used
> > EILSEQ for some years (indicating a lowlevel bit protocol failure),
> > and MMC is using it too.
> > 
> > So if you want to say "not", it should be for some better reason
> > than that...
> 
> When calling strerror on EILSEQ, you get: "Invalid or incomplete
> multibyte or wide character".

While "man 1 errno" says "Illegal byte sequence" ...

In any case:  as a rule, strerror() is not expected to give good
diagnostics.  Those "standard strings" are generated without any
context whatever, and so their messages have mostly been useful for
distinguishing faults rather than diagnosing them.

You may recall the case of "ENOTTY" --> "Not a TTY", for example.
That's also a good example of how code semantics evolve over time.
(Sure it's not a TTY ... that code means *other* things too.)


> This has nothing to do with the error in 
> question, and that's what I am worried about. I don't know whether it
> makes sense or not in the USB and MMC code, but in the I2C/SMBus
> context, I'm sure it does not.

It most certainly *is* an illegal byte sequence though...

 
> > > I'd return -EINVAL, or 
> > > -EREMOTEIO if you think EINVAL is unfair for the caller.
> > 
> > It's not an I/O error; the I/O actually worked, it's the bytes
> 
> True...
> 
> > that were wrong.  It's not an "argument" provided by the caller.
> > So neither of those codes seems appropriate.
> 
> The returned value depends on an argument provided by the caller,
> though.

Among an arbitrary number of other things.  It could also be
just bad device firmware ... I have in fact seen such cases:
perfectly valid SMBus requests generating bogus responses.

(This happened to be on shipping hardware, but of course it's
also easy to imagine that in developer setups too...)

Rule of thumb:  make the fault report reflect observation, not
guesses about what caused the behavior.  It's harder to get it
wrong that way.


> > > But most 
> > > likely, if the returned length is not within the SMBus specifications,
> > > it means that the caller ran an SMBus block transaction at the wrong
> > > offset or on a device that doesn't support it, so -EINVAL seems
> > > reasonable to me.
> > 
> > That's only a guess though.  All we *know* is that we saw an
> > illegal byte sequence...
> 
> That's not an illegal "sequence", sorry. The length byte by itself is
> either valid or not valid. What we saw is an illegal byte value.

It's part of a sequence.  The length byte will usually be the fourth
byte of the sequence:

    S addr/w [A] command [A] S addr/r [A] [length] A ... P

That value is perfectly legal ... it's just that in that location
within *THIS* byte sequence, it's trouble.  Mostly because that
byte is being interpreted; in other contexts, that sequence is fine.


> If you are still worried that -EINVAL has a small chance of not being
> correct, I'd rather go for -EMSGSIZE (Message too long), although the
> error message would be confusing in the 0 case.

Right...

> Even -EOVERFLOW (Value too large for defined data type)

... also confusing in the 0 case, and also wrong for PMBus (where
values 1..255 are all valid so it can't be "too large").


> would be more appropriate than -EILSEQ, I think.

How about "-EPROTO" for protocol error, then, if you're so
strongly opposed to "illegal byte sequence"?  The reason I
avoided EPROTO in that case is that it's so generic; while
we could use EILSEQ to indicate this specific case.

- Dave


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sun, 11 May 2008 09:23:43 -0700, David Brownell wrote:
> On Sunday 11 May 2008, you wrote:
> > My point exactly. I am fine with both -ENXIO and -ENODEV, pick the one
> > you like, then make sure all drivers are using it consistently.
> 
> I'll go with ENXIO "no such device or address" as being a bit more
> appropriate for addressing issues.  Driver probe() code can return
> ENODEV in the case where there is a device there ... but the wrong
> one for that driver.

OK, fine with me.

> > (...)
> > When calling strerror on EILSEQ, you get: "Invalid or incomplete
> > multibyte or wide character".
> 
> While "man 1 errno" says "Illegal byte sequence" ...

Yes, it's unfortunately inconsistent. Ideally man 1 errno would be
generated automatically by calling strerror on all error codes.

> > (...)
> > The returned value depends on an argument provided by the caller,
> > though.
> 
> Among an arbitrary number of other things.  It could also be
> just bad device firmware ... I have in fact seen such cases:
> perfectly valid SMBus requests generating bogus responses.
> 
> (This happened to be on shipping hardware, but of course it's
> also easy to imagine that in developer setups too...)
> 
> Rule of thumb:  make the fault report reflect observation, not
> guesses about what caused the behavior.  It's harder to get it
> wrong that way.

I have to agree...

> > (...)
> > That's not an illegal "sequence", sorry. The length byte by itself is
> > either valid or not valid. What we saw is an illegal byte value.
> 
> It's part of a sequence.  The length byte will usually be the fourth
> byte of the sequence:
> 
>     S addr/w [A] command [A] S addr/r [A] [length] A ... P
> 
> That value is perfectly legal ... it's just that in that location
> within *THIS* byte sequence, it's trouble.  Mostly because that
> byte is being interpreted; in other contexts, that sequence is fine.

Going down that route, everything is part of a sequence, so we would
have to stop using -EINVAL and use -EILSEQ everywhere. Numbers by
themselves are never invalid, what makes them invalid is the context in
which they are encountered, and for a computer, a given context has to
come from a sequence of some kind.

So I'm not buying this, sorry. Especially when the first 3 bytes are
emitted by the master and the 4th one comes from the slave, calling
them a sequence as a whole is almost dishonest ;)

> > If you are still worried that -EINVAL has a small chance of not being
> > correct, I'd rather go for -EMSGSIZE (Message too long), although the
> > error message would be confusing in the 0 case.
> 
> Right...
> 
> > Even -EOVERFLOW (Value too large for defined data type)
> 
> ... also confusing in the 0 case, and also wrong for PMBus (where
> values 1..255 are all valid so it can't be "too large").
> 
> > would be more appropriate than -EILSEQ, I think.
> 
> How about "-EPROTO" for protocol error, then, if you're so
> strongly opposed to "illegal byte sequence"?  The reason I
> avoided EPROTO in that case is that it's so generic; while
> we could use EILSEQ to indicate this specific case.

-EPROTO sounds good to me. We're not using it anywhere in the i2c
subsystem so there's no need to worry about it being "too generic".

Thanks,
-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Monday 12 May 2008, Jean Delvare wrote:
> 
> > > would be more appropriate than -EILSEQ, I think.
> > 
> > How about "-EPROTO" for protocol error, then, if you're so
> > strongly opposed to "illegal byte sequence"?  The reason I
> > avoided EPROTO in that case is that it's so generic; while
> > we could use EILSEQ to indicate this specific case.
> 
> -EPROTO sounds good to me. We're not using it anywhere in the i2c
> subsystem so there's no need to worry about it being "too generic".

I'd be happier with EILSEQ, matching related use elsewhere in
Linux, but it's now been switched to EPROTO.



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 10 May 2008, you wrote:
> >  
> >  static int ali1563_block_start(struct i2c_adapter * a)
> > @@ -170,7 +170,7 @@ static int ali1563_block_start(struct i2
> >               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 -EIO;
> >  }
> 
> And same here. That's a little more work, admittedly.

Returning ENXIO for BUSERR/no-response doesn't follow your
heuristic of trusting the logspam-eliminators ... plus, it's
not clear that's not a different failure mode on that path.

NVidia seems to not have ALI 1563 specs on line (sigh) so
for now I'll just stick to your heuristic.
 
I did make the "timeout" fault unique there, since that
fault was clearly distinguishable.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sun, 11 May 2008 10:13:25 -0700, David Brownell wrote:
> On Saturday 10 May 2008, you wrote:
> > >  
> > >  static int ali1563_block_start(struct i2c_adapter * a)
> > > @@ -170,7 +170,7 @@ static int ali1563_block_start(struct i2
> > >               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 -EIO;
> > >  }
> > 
> > And same here. That's a little more work, admittedly.
> 
> Returning ENXIO for BUSERR/no-response doesn't follow your
> heuristic of trusting the logspam-eliminators ... plus, it's
> not clear that's not a different failure mode on that path.

Elsewhere in this driver there is:

	/* device error - no response, ignore the autodetection case */
	if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
		dev_err(&a->dev, "Device error!\n");
	}

which makes it clear to me that HST_STS_DEVERR is what you get when no
device acks a transaction. So returning -ENODEV would make sense.

> NVidia seems to not have ALI 1563 specs on line (sigh) so
> for now I'll just stick to your heuristic.

Not sure why nVidia would host ALi datasheets in the first place? Let
alone the fact that nVidia is notoriously known to _not_ publish
datasheets anyway :(

> I did make the "timeout" fault unique there, since that
> fault was clearly distinguishable.

Agreed.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Monday 12 May 2008, Jean Delvare wrote:
> On Sun, 11 May 2008 10:13:25 -0700, David Brownell wrote:
> > On Saturday 10 May 2008, you wrote:
> > > >  
> > > >  static int ali1563_block_start(struct i2c_adapter * a)
> > > > @@ -170,7 +170,7 @@ static int ali1563_block_start(struct i2
> > > >               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 -EIO;
> > > >  }
> > > 
> > > And same here. That's a little more work, admittedly.
> > 
> > Returning ENXIO for BUSERR/no-response doesn't follow your
> > heuristic of trusting the logspam-eliminators ... plus, it's
> > not clear that's not a different failure mode on that path.
> 
> Elsewhere in this driver there is:
> 
> 	/* device error - no response, ignore the autodetection case */
> 	if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
> 		dev_err(&a->dev, "Device error!\n");
> 	}
> 
> which makes it clear to me that HST_STS_DEVERR is what you get when no
> device acks a transaction. So returning -ENODEV would make sense.

In that case, yes.  That one is fixed.  (DEVERR and QUICK report
as ENXIO.)

But it looks like a case of bundling faults into one bit, ergo
my comment.  ONLY that case with QUICK is special cased per your
heuristic.  That other case isn't special cased... it could well
be exposing some *other* and more significant error.


> > NVidia seems to not have ALI 1563 specs on line (sigh) so
> > for now I'll just stick to your heuristic.
> 
> Not sure why nVidia would host ALi datasheets in the first place? Let
> alone the fact that nVidia is notoriously known to _not_ publish
> datasheets anyway :(

ALI renamed itself to ULI, and NVidia bought ULI.


> > I did make the "timeout" fault unique there, since that
> > fault was clearly distinguishable.
> 
> Agreed.
> 



_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Mon, 12 May 2008 09:25:33 -0700, David Brownell wrote:
> On Monday 12 May 2008, Jean Delvare wrote:
> > Elsewhere in this driver there is:
> > 
> > 	/* device error - no response, ignore the autodetection case */
> > 	if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
> > 		dev_err(&a->dev, "Device error!\n");
> > 	}
> > 
> > which makes it clear to me that HST_STS_DEVERR is what you get when no
> > device acks a transaction. So returning -ENODEV would make sense.
> 
> In that case, yes.  That one is fixed.  (DEVERR and QUICK report
> as ENXIO.)
> 
> But it looks like a case of bundling faults into one bit, ergo
> my comment.  ONLY that case with QUICK is special cased per your
> heuristic.  That other case isn't special cased... it could well
> be exposing some *other* and more significant error.

Just to make sure it's clear: the Quick command case isn't special as
far as the returned error value is concerned. If nobody acks a Quick
command, that's still -ENXIO.

What is special about the Quick command is that we don't want to log the
error in this case, because it's used for detection purposes so it
happens frequently and it isn't something the user should worry about.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Monday 12 May 2008, Jean Delvare wrote:
> 
> > But it looks like a case of bundling faults into one bit, ergo
> > my comment.  ONLY that case with QUICK is special cased per your
> > heuristic.  That other case isn't special cased... it could well
> > be exposing some *other* and more significant error.
> 
> Just to make sure it's clear: the Quick command case isn't special as
> far as the returned error value is concerned. If nobody acks a Quick
> command, that's still -ENXIO.
> 
> What is special about the Quick command is that we don't want to log the
> error in this case, because it's used for detection purposes so it
> happens frequently and it isn't something the user should worry about.

Close...

The desired out come is that whenever the first address doesn't
get ACKed, we report ENXIO.  Arguably the same should be true of
the address byte after all repeated START bits too ... but then
we run into other error reporting problems.

Logging is orthogonal.  We don't want to log probing errors, as
a rule, and Quick is (for our purposes) basically about probing.

Then there's also the "what does this hardware bit mean" issue.
In this case, absent hardware docs, we're guessing in all cases
except for the QUICK case.  With only a few bits to report all
faults, it's clear there's some degree of fault bundling.  Ergo
my caution:  I didn't restructure all fault paths in that driver
to treat that one bit as if it only means "no address ACK", only
the one where it's clear that was how the driver interpreted it.

- Dave
 

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 10 May 2008, Jean Delvare wrote:
> >       amd_ec_write(smbus, AMD_SMB_ADDR, addr << 1);
> >       amd_ec_write(smbus, AMD_SMB_PRTCL, protocol);
> >  
> > +     /* FIXME this discards status from ec_read() ... */
> >       amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
> 
> Same is true of almost all amd_ec_write and amd_ec_read calls, so I
> don't get the point of adding a comment for this one occurrence in
> particular.
> 
> >  
> >       if (~temp[0] & AMD_SMB_STS_DONE) {

I updated that comment.  The issue is that temp[0] will
have random stack garbage at that point, so the rest of
that routine is at best dicey.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Saturday 10 May 2008, Jean Delvare wrote:
> Please send an updated patch and I'll push it to -next and -mm so that
> it gets some testing.

Appended ... agaist 2.6.26-rc2.

========= CUT HERE
Tighten error paths used by various i2c adapters (mostly x86) so
they return real fault/errno codes instead of a "-1" (which is
most often interpreted as "-EPERM").  Build tested, with eyeball
review.

One minor initial goal is to have adapters consistently return
the code "-ENXIO" when addressing a device doesn't get an ACK
response, at least in the probe paths where they are already
good at stifling related logspam.

Signed-off-by: David Brownell <dbrownell-Rn4VEauK+AKRv+LV9MX5uipxlwaOVQ5f@public.gmane.org>
---
UPDATED per feedback

NOTE there are many other adapter drivers to audit and update; and
they're not just within the drivers/i2c tree.  This patch is a PC
oriented start on some bottom-up improvement of fault reporting.
The i2c-gpio code looks be good too.

 drivers/i2c/algos/i2c-algo-bit.c      |    4 +-
 drivers/i2c/busses/i2c-ali1535.c      |   22 +++++++--------
 drivers/i2c/busses/i2c-ali1563.c      |   16 ++++++++---
 drivers/i2c/busses/i2c-ali15x3.c      |   19 +++++++------
 drivers/i2c/busses/i2c-amd756-s4882.c |    4 +-
 drivers/i2c/busses/i2c-amd756.c       |   18 +++++++------
 drivers/i2c/busses/i2c-amd8111.c      |   47 ++++++++++++++++++++++------------
 drivers/i2c/busses/i2c-i801.c         |   44 ++++++++++++++++---------------
 drivers/i2c/busses/i2c-nforce2.c      |   25 +++++++++---------
 drivers/i2c/busses/i2c-piix4.c        |   20 +++++++-------
 drivers/i2c/busses/i2c-sis5595.c      |   19 +++++++------
 drivers/i2c/busses/i2c-sis630.c       |   47 ++++++++++++++++++----------------
 drivers/i2c/busses/i2c-sis96x.c       |   23 ++++++++--------
 drivers/i2c/busses/i2c-stub.c         |    4 +-
 drivers/i2c/busses/i2c-viapro.c       |   22 +++++++++------
 15 files changed, 188 insertions(+), 146 deletions(-)

--- g26.orig/drivers/i2c/algos/i2c-algo-bit.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/algos/i2c-algo-bit.c	2008-05-11 17:55:08.000000000 -0700
@@ -320,7 +320,7 @@ static int try_address(struct i2c_adapte
 		       unsigned char addr, int retries)
 {
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
-	int i, ret = -1;
+	int i, ret = 0;
 
 	for (i = 0; i <= retries; i++) {
 		ret = i2c_outb(i2c_adap, addr);
@@ -508,7 +508,7 @@ static int bit_doAddress(struct i2c_adap
 			addr ^= 1;
 		ret = try_address(i2c_adap, addr, retries);
 		if ((ret != 1) && !nak_ok)
-			return -EREMOTEIO;
+			return -ENXIO;
 	}
 
 	return 0;
--- g26.orig/drivers/i2c/busses/i2c-ali1535.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali1535.c	2008-05-11 17:55:08.000000000 -0700
@@ -259,7 +259,7 @@ static int ali1535_transaction(struct i2
 			dev_err(&adap->dev,
 				"SMBus reset failed! (0x%02x) - controller or "
 				"device on bus is probably hung\n", temp);
-			return -1;
+			return -EBUSY;
 		}
 	} else {
 		/* check and clear done bit */
@@ -281,12 +281,12 @@ static int ali1535_transaction(struct i2
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&adap->dev, "SMBus Timeout!\n");
 	}
 
 	if (temp & ALI1535_STS_FAIL) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
 	}
 
@@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2
 	 * do a printk.  This means that bus collisions go unreported.
 	 */
 	if (temp & ALI1535_STS_BUSERR) {
-		result = -1;
+		result = -ENXIO;
 		dev_dbg(&adap->dev,
 			"Error: no response or bus collision ADD=%02x\n",
 			inb_p(SMBHSTADD));
@@ -303,13 +303,13 @@ static int ali1535_transaction(struct i2
 
 	/* haven't ever seen this */
 	if (temp & ALI1535_STS_DEV) {
-		result = -1;
+		result = -EIO;
 		dev_err(&adap->dev, "Error: device error\n");
 	}
 
 	/* check to see if the "command complete" indication is set */
 	if (!(temp & ALI1535_STS_DONE)) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&adap->dev, "Error: command never completed\n");
 	}
 
@@ -332,7 +332,7 @@ static int ali1535_transaction(struct i2
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 ali1535_access(struct i2c_adapter *adap, u16 addr,
 			  unsigned short flags, char read_write, u8 command,
 			  int size, union i2c_smbus_data *data)
@@ -359,7 +359,7 @@ static s32 ali1535_access(struct i2c_ada
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-		result = -1;
+		result = -EOPNOTSUPP;
 		goto EXIT;
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
@@ -420,11 +420,9 @@ static s32 ali1535_access(struct i2c_ada
 		break;
 	}
 
-	if (ali1535_transaction(adap)) {
-		/* Error in transaction */
-		result = -1;
+	result = ali1535_transaction(adap);
+	if (result)
 		goto EXIT;
-	}
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == ALI1535_QUICK)) {
 		result = 0;
--- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:08.000000000 -0700
@@ -106,8 +106,11 @@ static int ali1563_transaction(struct i2
  	}
 
 	/* device error - no response, ignore the autodetection case */
-	if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
-		dev_err(&a->dev, "Device error!\n");
+	if (data & HST_STS_DEVERR) {
+		if (size != HST_CNTL2_QUICK)
+			dev_err(&a->dev, "Device error!\n");
+		else
+			return -ENXIO;
 	}
 
 	/* bus collision */
@@ -122,13 +125,14 @@ static int ali1563_transaction(struct i2
 		outb_p(0x0,SMB_HST_CNTL2);
 	}
 
-	return -1;
+	return -EIO;
 }
 
 static int ali1563_block_start(struct i2c_adapter * a)
 {
 	u32 data;
 	int timeout;
+	int status = -EIO;
 
 	dev_dbg(&a->dev, "Block (pre): STS=%02x, CNTL1=%02x, "
 		"CNTL2=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
@@ -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;
+
 	dev_err(&a->dev, "SMBus Error: %s%s%s%s%s\n",
 		timeout ? "Timeout " : "",
 		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;
 }
 
 static int ali1563_block(struct i2c_adapter * a, union i2c_smbus_data * data, u8 rw)
--- g26.orig/drivers/i2c/busses/i2c-ali15x3.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-ali15x3.c	2008-05-11 17:55:08.000000000 -0700
@@ -282,7 +282,7 @@ static int ali15x3_transaction(struct i2
 			dev_err(&adap->dev, "SMBus reset failed! (0x%02x) - "
 				"controller or device on bus is probably hung\n",
 				temp);
-			return -1;
+			return -EBUSY;
 		}
 	} else {
 		/* check and clear done bit */
@@ -304,12 +304,12 @@ static int ali15x3_transaction(struct i2
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&adap->dev, "SMBus Timeout!\n");
 	}
 
 	if (temp & ALI15X3_STS_TERM) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
 	}
 
@@ -320,7 +320,7 @@ static int ali15x3_transaction(struct i2
 	  This means that bus collisions go unreported.
 	*/
 	if (temp & ALI15X3_STS_COLL) {
-		result = -1;
+		result = -ENXIO;
 		dev_dbg(&adap->dev,
 			"Error: no response or bus collision ADD=%02x\n",
 			inb_p(SMBHSTADD));
@@ -328,7 +328,7 @@ static int ali15x3_transaction(struct i2
 
 	/* haven't ever seen this */
 	if (temp & ALI15X3_STS_DEV) {
-		result = -1;
+		result = -EIO;
 		dev_err(&adap->dev, "Error: device error\n");
 	}
 	dev_dbg(&adap->dev, "Transaction (post): STS=%02x, CNT=%02x, CMD=%02x, "
@@ -338,7 +338,7 @@ static int ali15x3_transaction(struct i2
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 ali15x3_access(struct i2c_adapter * adap, u16 addr,
 		   unsigned short flags, char read_write, u8 command,
 		   int size, union i2c_smbus_data * data)
@@ -364,7 +364,7 @@ static s32 ali15x3_access(struct i2c_ada
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-		return -1;
+		return -EOPNOTSUPP;
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD);
@@ -421,8 +421,9 @@ static s32 ali15x3_access(struct i2c_ada
 
 	outb_p(size, SMBHSTCNT);	/* output command */
 
-	if (ali15x3_transaction(adap))	/* Error in transaction */
-		return -1;
+	temp = ali15x3_transaction(adap);
+	if (temp)
+		return temp;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == ALI15X3_QUICK))
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-amd756-s4882.c	2008-05-11 17:55:08.000000000 -0700
@@ -58,7 +58,7 @@ static s32 amd756_access_virt0(struct i2
 	/* We exclude the multiplexed addresses */
 	if (addr == 0x4c || (addr & 0xfc) == 0x50 || (addr & 0xfc) == 0x30
 	 || addr == 0x18)
-		return -1;
+		return -ENXIO;
 
 	mutex_lock(&amd756_lock);
 
@@ -86,7 +86,7 @@ static inline s32 amd756_access_channel(
 
 	/* We exclude the non-multiplexed addresses */
 	if (addr != 0x4c && (addr & 0xfc) != 0x50 && (addr & 0xfc) != 0x30)
-		return -1;
+		return -ENXIO;
 
 	mutex_lock(&amd756_lock);
 
--- g26.orig/drivers/i2c/busses/i2c-amd756.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-amd756.c	2008-05-11 17:55:08.000000000 -0700
@@ -151,17 +151,17 @@ static int amd756_transaction(struct i2c
 	}
 
 	if (temp & GS_PRERR_STS) {
-		result = -1;
+		result = -ENXIO;
 		dev_dbg(&adap->dev, "SMBus Protocol error (no response)!\n");
 	}
 
 	if (temp & GS_COL_STS) {
-		result = -1;
+		result = -EIO;
 		dev_warn(&adap->dev, "SMBus collision!\n");
 	}
 
 	if (temp & GS_TO_STS) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_dbg(&adap->dev, "SMBus protocol timeout!\n");
 	}
 
@@ -189,22 +189,23 @@ static int amd756_transaction(struct i2c
 	outw_p(inw(SMB_GLOBAL_ENABLE) | GE_ABORT, SMB_GLOBAL_ENABLE);
 	msleep(100);
 	outw_p(GS_CLEAR_STS, SMB_GLOBAL_STATUS);
-	return -1;
+	return -EIO;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 amd756_access(struct i2c_adapter * adap, u16 addr,
 		  unsigned short flags, char read_write,
 		  u8 command, int size, union i2c_smbus_data * data)
 {
 	int i, len;
+	int status;
 
 	/** TODO: Should I supporte the 10-bit transfers? */
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_dbg(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
 		/* TODO: Well... It is supported, I'm just not sure what to do here... */
-		return -1;
+		return -EOPNOTSUPP;
 	case I2C_SMBUS_QUICK:
 		outw_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMB_HOST_ADDRESS);
@@ -256,8 +257,9 @@ static s32 amd756_access(struct i2c_adap
 	/* How about enabling interrupts... */
 	outw_p(size & GE_CYC_TYPE_MASK, SMB_GLOBAL_ENABLE);
 
-	if (amd756_transaction(adap))	/* Error in transaction */
-		return -1;
+	status = amd756_transaction(adap);
+	if (status)
+		return status;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == AMD756_QUICK))
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-amd8111.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-amd8111.c	2008-05-11 17:55:08.000000000 -0700
@@ -77,7 +77,7 @@ static unsigned int amd_ec_wait_write(st
 	if (!timeout) {
 		dev_warn(&smbus->dev->dev,
 			 "Timeout while waiting for IBF to clear\n");
-		return -1;
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -93,7 +93,7 @@ static unsigned int amd_ec_wait_read(str
 	if (!timeout) {
 		dev_warn(&smbus->dev->dev,
 			 "Timeout while waiting for OBF to set\n");
-		return -1;
+		return -ETIMEDOUT;
 	}
 
 	return 0;
@@ -102,16 +102,21 @@ static unsigned int amd_ec_wait_read(str
 static unsigned int amd_ec_read(struct amd_smbus *smbus, unsigned char address,
 		unsigned char *data)
 {
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	int status;
+
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(AMD_EC_CMD_RD, smbus->base + AMD_EC_CMD);
 
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(address, smbus->base + AMD_EC_DATA);
 
-	if (amd_ec_wait_read(smbus))
-		return -1;
+	status = amd_ec_wait_read(smbus);
+	if (status)
+		return status;
 	*data = inb(smbus->base + AMD_EC_DATA);
 
 	return 0;
@@ -120,16 +125,21 @@ static unsigned int amd_ec_read(struct a
 static unsigned int amd_ec_write(struct amd_smbus *smbus, unsigned char address,
 		unsigned char data)
 {
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	int status;
+
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(AMD_EC_CMD_WR, smbus->base + AMD_EC_CMD);
 
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(address, smbus->base + AMD_EC_DATA);
 
-	if (amd_ec_wait_write(smbus))
-		return -1;
+	status = amd_ec_wait_write(smbus);
+	if (status)
+		return status;
 	outb(data, smbus->base + AMD_EC_DATA);
 
 	return 0;
@@ -267,12 +277,17 @@ static s32 amd8111_access(struct i2c_ada
 
 		default:
 			dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
-			return -1;
+			return -EOPNOTSUPP;
 	}
 
 	amd_ec_write(smbus, AMD_SMB_ADDR, addr << 1);
 	amd_ec_write(smbus, AMD_SMB_PRTCL, protocol);
 
+	/* FIXME this discards status from ec_read(); so temp[0] will
+	 * hold stack garbage ... the rest of this routine will act
+	 * nonsensically.  Ignored ec_write() status might explain
+	 * some such failures...
+	 */
 	amd_ec_read(smbus, AMD_SMB_STS, temp + 0);
 
 	if (~temp[0] & AMD_SMB_STS_DONE) {
@@ -286,7 +301,7 @@ static s32 amd8111_access(struct i2c_ada
 	}
 
 	if ((~temp[0] & AMD_SMB_STS_DONE) || (temp[0] & AMD_SMB_STS_STATUS))
-		return -1;
+		return -EIO;
 
 	if (read_write == I2C_SMBUS_WRITE)
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-i801.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-i801.c	2008-05-11 17:55:08.000000000 -0700
@@ -151,7 +151,7 @@ static int i801_transaction(int xact)
 		outb_p(temp, SMBHSTSTS);
 		if ((temp = (0x1f & inb_p(SMBHSTSTS))) != 0x00) {
 			dev_dbg(&I801_dev->dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&I801_dev->dev, "Successful!\n");
 		}
@@ -170,7 +170,7 @@ static int i801_transaction(int xact)
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&I801_dev->dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 		/* try to stop the current command */
 		dev_dbg(&I801_dev->dev, "Terminating the current operation\n");
 		outb_p(inb_p(SMBHSTCNT) | SMBHSTCNT_KILL, SMBHSTCNT);
@@ -179,19 +179,19 @@ static int i801_transaction(int xact)
 	}
 
 	if (temp & SMBHSTSTS_FAILED) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&I801_dev->dev, "Error: Failed bus transaction\n");
 	}
 
 	if (temp & SMBHSTSTS_BUS_ERR) {
-		result = -1;
+		result = -EIO;
 		dev_err(&I801_dev->dev, "Bus collision! SMBus may be locked "
 			"until next hard reset. (sorry!)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
 	}
 
 	if (temp & SMBHSTSTS_DEV_ERR) {
-		result = -1;
+		result = -ENXIO;
 		dev_dbg(&I801_dev->dev, "Error: no response!\n");
 	}
 
@@ -231,6 +231,7 @@ static int i801_block_transaction_by_blo
 					   char read_write, int hwpec)
 {
 	int i, len;
+	int status;
 
 	inb_p(SMBHSTCNT); /* reset the data buffer index */
 
@@ -242,14 +243,15 @@ static int i801_block_transaction_by_blo
 			outb_p(data->block[i+1], SMBBLKDAT);
 	}
 
-	if (i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
-			     I801_PEC_EN * hwpec))
-		return -1;
+	status = i801_transaction(I801_BLOCK_DATA | ENABLE_INT9 |
+				  I801_PEC_EN * hwpec);
+	if (status)
+		return status;
 
 	if (read_write == I2C_SMBUS_READ) {
 		len = inb_p(SMBHSTDAT0);
 		if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
-			return -1;
+			return -EPROTO;
 
 		data->block[0] = len;
 		for (i = 0; i < len; i++)
@@ -314,11 +316,11 @@ static int i801_block_transaction_byte_b
 			if (((temp = inb_p(SMBHSTSTS)) & errmask) != 0x00) {
 				dev_err(&I801_dev->dev,
 					"Reset failed! (%02x)\n", temp);
-				return -1;
+				return -EBUSY;
 			}
 			if (i != 1)
 				/* if die in middle of block transaction, fail */
-				return -1;
+				return -EIO;
 		}
 
 		if (i == 1)
@@ -342,19 +344,19 @@ static int i801_block_transaction_byte_b
 			msleep(1);
 			outb_p(inb_p(SMBHSTCNT) & (~SMBHSTCNT_KILL),
 				SMBHSTCNT);
-			result = -1;
+			result = -ETIMEDOUT;
 			dev_dbg(&I801_dev->dev, "SMBus Timeout!\n");
 		}
 
 		if (temp & SMBHSTSTS_FAILED) {
-			result = -1;
+			result = -EIO;
 			dev_dbg(&I801_dev->dev,
 				"Error: Failed bus transaction\n");
 		} else if (temp & SMBHSTSTS_BUS_ERR) {
-			result = -1;
+			result = -EIO;
 			dev_err(&I801_dev->dev, "Bus collision!\n");
 		} else if (temp & SMBHSTSTS_DEV_ERR) {
-			result = -1;
+			result = -ENXIO;
 			dev_dbg(&I801_dev->dev, "Error: no response!\n");
 		}
 
@@ -362,7 +364,7 @@ static int i801_block_transaction_byte_b
 		 && command != I2C_SMBUS_I2C_BLOCK_DATA) {
 			len = inb_p(SMBHSTDAT0);
 			if (len < 1 || len > I2C_SMBUS_BLOCK_MAX)
-				return -1;
+				return -EPROTO;
 			data->block[0] = len;
 		}
 
@@ -394,7 +396,7 @@ static int i801_set_block_buffer_mode(vo
 {
 	outb_p(inb_p(SMBAUXCTL) | SMBAUXCTL_E32B, SMBAUXCTL);
 	if ((inb_p(SMBAUXCTL) & SMBAUXCTL_E32B) == 0)
-		return -1;
+		return -EIO;
 	return 0;
 }
 
@@ -414,7 +416,7 @@ static int i801_block_transaction(union 
 		} else if (!(i801_features & FEATURE_I2C_BLOCK_READ)) {
 			dev_err(&I801_dev->dev,
 				"I2C block read is unsupported!\n");
-			return -1;
+			return -EOPNOTSUPP;
 		}
 	}
 
@@ -449,7 +451,7 @@ static int i801_block_transaction(union 
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 i801_access(struct i2c_adapter * adap, u16 addr,
 		       unsigned short flags, char read_write, u8 command,
 		       int size, union i2c_smbus_data * data)
@@ -514,7 +516,7 @@ static s32 i801_access(struct i2c_adapte
 	case I2C_SMBUS_PROC_CALL:
 	default:
 		dev_err(&I801_dev->dev, "Unsupported transaction %d\n", size);
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	if (hwpec)	/* enable/disable hardware PEC */
@@ -537,7 +539,7 @@ static s32 i801_access(struct i2c_adapte
 	if(block)
 		return ret;
 	if(ret)
-		return -1;
+		return ret;
 	if ((read_write == I2C_SMBUS_WRITE) || (xact == I801_QUICK))
 		return 0;
 
--- g26.orig/drivers/i2c/busses/i2c-nforce2.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-nforce2.c	2008-05-11 17:55:08.000000000 -0700
@@ -145,16 +145,16 @@ static int nforce2_check_status(struct i
 		dev_dbg(&adap->dev, "SMBus Timeout!\n");
 		if (smbus->can_abort)
 			nforce2_abort(adap);
-		return -1;
+		return -ETIMEDOUT;
 	}
 	if (!(temp & NVIDIA_SMB_STS_DONE) || (temp & NVIDIA_SMB_STS_STATUS)) {
 		dev_dbg(&adap->dev, "Transaction failed (0x%02x)!\n", temp);
-		return -1;
+		return -EIO;
 	}
 	return 0;
 }
 
-/* Return -1 on error */
+/* Return negative errno 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)
@@ -162,7 +162,7 @@ static s32 nforce2_access(struct i2c_ada
 	struct nforce2_smbus *smbus = adap->algo_data;
 	unsigned char protocol, pec;
 	u8 len;
-	int i;
+	int i, status;
 
 	protocol = (read_write == I2C_SMBUS_READ) ? NVIDIA_SMB_PRTCL_READ :
 		NVIDIA_SMB_PRTCL_WRITE;
@@ -206,7 +206,7 @@ static s32 nforce2_access(struct i2c_ada
 						"Transaction failed "
 						"(requested block size: %d)\n",
 						len);
-					return -1;
+					return -EINVAL;
 				}
 				outb_p(len, NVIDIA_SMB_BCNT);
 				for (i = 0; i < I2C_SMBUS_BLOCK_MAX; i++)
@@ -218,14 +218,15 @@ static s32 nforce2_access(struct i2c_ada
 
 		default:
 			dev_err(&adap->dev, "Unsupported transaction %d\n", size);
-			return -1;
+			return -EOPNOTSUPP;
 	}
 
 	outb_p((addr & 0x7f) << 1, NVIDIA_SMB_ADDR);
 	outb_p(protocol, NVIDIA_SMB_PRTCL);
 
-	if (nforce2_check_status(adap))
-		return -1;
+	status = nforce2_check_status(adap);
+	if (status)
+		return status;
 
 	if (read_write == I2C_SMBUS_WRITE)
 		return 0;
@@ -247,7 +248,7 @@ static s32 nforce2_access(struct i2c_ada
 				dev_err(&adap->dev, "Transaction failed "
 					"(received block size: 0x%02x)\n",
 					len);
-				return -1;
+				return -EPROTO;
 			}
 			for (i = 0; i < len; i++)
 				data->block[i+1] = inb_p(NVIDIA_SMB_DATA + i);
@@ -308,7 +309,7 @@ static int __devinit nforce2_probe_smb (
 		    != PCIBIOS_SUCCESSFUL) {
 			dev_err(&dev->dev, "Error reading PCI config for %s\n",
 				name);
-			return -1;
+			return -EIO;
 		}
 
 		smbus->base = iobase & PCI_BASE_ADDRESS_IO_MASK;
@@ -318,7 +319,7 @@ static int __devinit nforce2_probe_smb (
 	if (!request_region(smbus->base, smbus->size, nforce2_driver.name)) {
 		dev_err(&smbus->adapter.dev, "Error requesting region %02x .. %02X for %s\n",
 			smbus->base, smbus->base+smbus->size-1, name);
-		return -1;
+		return -EBUSY;
 	}
 	smbus->adapter.owner = THIS_MODULE;
 	smbus->adapter.id = I2C_HW_SMBUS_NFORCE2;
@@ -333,7 +334,7 @@ static int __devinit nforce2_probe_smb (
 	if (error) {
 		dev_err(&smbus->adapter.dev, "Failed to register adapter.\n");
 		release_region(smbus->base, smbus->size);
-		return -1;
+		return error;
 	}
 	dev_info(&smbus->adapter.dev, "nForce2 SMBus adapter at %#x\n", smbus->base);
 	return 0;
--- g26.orig/drivers/i2c/busses/i2c-piix4.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-piix4.c	2008-05-11 17:55:08.000000000 -0700
@@ -253,7 +253,7 @@ static int piix4_transaction(void)
 		outb_p(temp, SMBHSTSTS);
 		if ((temp = inb_p(SMBHSTSTS)) != 0x00) {
 			dev_err(&piix4_adapter.dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&piix4_adapter.dev, "Successful!\n");
 		}
@@ -275,23 +275,23 @@ static int piix4_transaction(void)
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_err(&piix4_adapter.dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x10) {
-		result = -1;
+		result = -EIO;
 		dev_err(&piix4_adapter.dev, "Error: Failed bus transaction\n");
 	}
 
 	if (temp & 0x08) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&piix4_adapter.dev, "Bus collision! SMBus may be "
 			"locked until next hard reset. (sorry!)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
 	}
 
 	if (temp & 0x04) {
-		result = -1;
+		result = -ENXIO;
 		dev_dbg(&piix4_adapter.dev, "Error: no response!\n");
 	}
 
@@ -309,17 +309,18 @@ static int piix4_transaction(void)
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 piix4_access(struct i2c_adapter * adap, u16 addr,
 		 unsigned short flags, char read_write,
 		 u8 command, int size, union i2c_smbus_data * data)
 {
 	int i, len;
+	int status;
 
 	switch (size) {
 	case I2C_SMBUS_PROC_CALL:
 		dev_err(&adap->dev, "I2C_SMBUS_PROC_CALL not supported!\n");
-		return -1;
+		return -EOPNOTSUPP;
 	case I2C_SMBUS_QUICK:
 		outb_p(((addr & 0x7f) << 1) | (read_write & 0x01),
 		       SMBHSTADD);
@@ -371,8 +372,9 @@ static s32 piix4_access(struct i2c_adapt
 
 	outb_p((size & 0x1C) + (ENABLE_INT9 & 1), SMBHSTCNT);
 
-	if (piix4_transaction())	/* Error in transaction */
-		return -1;
+	status = piix4_transaction();
+	if (status)
+		return status;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == PIIX4_QUICK))
 		return 0;
--- g26.orig/drivers/i2c/busses/i2c-sis5595.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-sis5595.c	2008-05-11 17:55:08.000000000 -0700
@@ -236,7 +236,7 @@ static int sis5595_transaction(struct i2
 		sis5595_write(SMB_STS_HI, temp >> 8);
 		if ((temp = sis5595_read(SMB_STS_LO) + (sis5595_read(SMB_STS_HI) << 8)) != 0x00) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
@@ -254,19 +254,19 @@ static int sis5595_transaction(struct i2
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&adap->dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x10) {
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
-		result = -1;
+		result = -ENXIO;
 	}
 
 	if (temp & 0x20) {
 		dev_err(&adap->dev, "Bus collision! SMBus may be locked until "
 			"next hard reset (or not...)\n");
 		/* Clock stops and slave is stuck in mid-transmission */
-		result = -1;
+		result = -EIO;
 	}
 
 	temp = sis5595_read(SMB_STS_LO) + (sis5595_read(SMB_STS_HI) << 8);
@@ -282,11 +282,13 @@ static int sis5595_transaction(struct i2
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 sis5595_access(struct i2c_adapter *adap, u16 addr,
 			  unsigned short flags, char read_write,
 			  u8 command, int size, union i2c_smbus_data *data)
 {
+	int status;
+
 	switch (size) {
 	case I2C_SMBUS_QUICK:
 		sis5595_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
@@ -318,13 +320,14 @@ static s32 sis5595_access(struct i2c_ada
 		break;
 	default:
 		dev_warn(&adap->dev, "Unsupported transaction %d\n", size);
-		return -1;
+		return -EOPNOTSUPP;
 	}
 
 	sis5595_write(SMB_CTL_LO, ((size & 0x0E)));
 
-	if (sis5595_transaction(adap))
-		return -1;
+	status = sis5595_transaction(adap);
+	if (status)
+		return status;
 
 	if ((size != SIS5595_PROC_CALL) &&
 	    ((read_write == I2C_SMBUS_WRITE) || (size == SIS5595_QUICK)))
--- g26.orig/drivers/i2c/busses/i2c-sis630.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-sis630.c	2008-05-11 17:55:08.000000000 -0700
@@ -134,7 +134,7 @@ static int sis630_transaction_start(stru
 
 		if ((temp = sis630_read(SMB_CNT) & 0x03) != 0x00) {
 			dev_dbg(&adap->dev, "Failed! (%02x)\n", temp);
-			return -1;
+			return -EBUSY;
                 } else {
 			dev_dbg(&adap->dev, "Successful!\n");
 		}
@@ -177,17 +177,17 @@ static int sis630_transaction_wait(struc
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&adap->dev, "SMBus Timeout!\n");
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	if (temp & 0x02) {
 		dev_dbg(&adap->dev, "Error: Failed bus transaction\n");
-		result = -1;
+		result = -ENXIO;
 	}
 
 	if (temp & 0x04) {
 		dev_err(&adap->dev, "Bus collision!\n");
-		result = -1;
+		result = -EIO;
 		/*
 		  TBD: Datasheet say:
 		  the software should clear this bit and restart SMBUS operation.
@@ -250,8 +250,10 @@ static int sis630_block_data(struct i2c_
 			if (i==8 || (len<8 && i==len)) {
 				dev_dbg(&adap->dev, "start trans len=%d i=%d\n",len ,i);
 				/* first transaction */
-				if (sis630_transaction_start(adap, SIS630_BLOCK_DATA, &oldclock))
-					return -1;
+				rc = sis630_transaction_start(adap,
+						SIS630_BLOCK_DATA, &oldclock);
+				if (rc)
+					return rc;
 			}
 			else if ((i-1)%8 == 7 || i==len) {
 				dev_dbg(&adap->dev, "trans_wait len=%d i=%d\n",len,i);
@@ -264,9 +266,10 @@ static int sis630_block_data(struct i2c_
 					*/
 					sis630_write(SMB_STS,0x10);
 				}
-				if (sis630_transaction_wait(adap, SIS630_BLOCK_DATA)) {
+				rc = sis630_transaction_wait(adap,
+						SIS630_BLOCK_DATA);
+				if (rc) {
 					dev_dbg(&adap->dev, "trans_wait failed\n");
-					rc = -1;
 					break;
 				}
 			}
@@ -275,13 +278,14 @@ static int sis630_block_data(struct i2c_
 	else {
 		/* read request */
 		data->block[0] = len = 0;
-		if (sis630_transaction_start(adap, SIS630_BLOCK_DATA, &oldclock)) {
-			return -1;
-		}
+		rc = sis630_transaction_start(adap,
+				SIS630_BLOCK_DATA, &oldclock);
+		if (rc)
+			return rc;
 		do {
-			if (sis630_transaction_wait(adap, SIS630_BLOCK_DATA)) {
+			rc = sis630_transaction_wait(adap, SIS630_BLOCK_DATA);
+			if (rc) {
 				dev_dbg(&adap->dev, "trans_wait failed\n");
-				rc = -1;
 				break;
 			}
 			/* if this first transaction then read byte count */
@@ -311,11 +315,13 @@ static int sis630_block_data(struct i2c_
 	return rc;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 sis630_access(struct i2c_adapter *adap, u16 addr,
 			 unsigned short flags, char read_write,
 			 u8 command, int size, union i2c_smbus_data *data)
 {
+	int status;
+
 	switch (size) {
 		case I2C_SMBUS_QUICK:
 			sis630_write(SMB_ADDR, ((addr & 0x7f) << 1) | (read_write & 0x01));
@@ -350,13 +356,13 @@ static s32 sis630_access(struct i2c_adap
 			size = SIS630_BLOCK_DATA;
 			return sis630_block_data(adap, data, read_write);
 		default:
-			printk("Unsupported I2C size\n");
-			return -1;
-			break;
+			printk("Unsupported SMBus operation\n");
+			return -EOPNOTSUPP;
 	}
 
-	if (sis630_transaction(adap, size))
-		return -1;
+	status = sis630_transaction(adap, size);
+	if (status)
+		return status;
 
 	if ((size != SIS630_PCALL) &&
 		((read_write == I2C_SMBUS_WRITE) || (size == SIS630_QUICK))) {
@@ -373,8 +379,7 @@ static s32 sis630_access(struct i2c_adap
 			data->word = sis630_read(SMB_BYTE) + (sis630_read(SMB_BYTE + 1) << 8);
 			break;
 		default:
-			return -1;
-			break;
+			return -EOPNOTSUPP;
 	}
 
 	return 0;
--- g26.orig/drivers/i2c/busses/i2c-sis96x.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-sis96x.c	2008-05-11 17:55:08.000000000 -0700
@@ -111,7 +111,7 @@ static int sis96x_transaction(int size)
 		/* check it again */
 		if (((temp = sis96x_read(SMB_CNT)) & 0x03) != 0x00) {
 			dev_dbg(&sis96x_adapter.dev, "Failed (0x%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		} else {
 			dev_dbg(&sis96x_adapter.dev, "Successful\n");
 		}
@@ -136,19 +136,19 @@ static int sis96x_transaction(int size)
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
 		dev_dbg(&sis96x_adapter.dev, "SMBus Timeout! (0x%02x)\n", temp);
-		result = -1;
+		result = -ETIMEDOUT;
 	}
 
 	/* device error - probably missing ACK */
 	if (temp & 0x02) {
 		dev_dbg(&sis96x_adapter.dev, "Failed bus transaction!\n");
-		result = -1;
+		result = -ENXIO;
 	}
 
 	/* bus collision */
 	if (temp & 0x04) {
 		dev_dbg(&sis96x_adapter.dev, "Bus collision!\n");
-		result = -1;
+		result = -EIO;
 	}
 
 	/* Finish up by resetting the bus */
@@ -161,11 +161,12 @@ static int sis96x_transaction(int size)
 	return result;
 }
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 sis96x_access(struct i2c_adapter * adap, u16 addr,
 			 unsigned short flags, char read_write,
 			 u8 command, int size, union i2c_smbus_data * data)
 {
+	int status;
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -203,17 +204,17 @@ static s32 sis96x_access(struct i2c_adap
 	case I2C_SMBUS_BLOCK_DATA:
 		/* TO DO: */
 		dev_info(&adap->dev, "SMBus block not implemented!\n");
-		return -1;
+		return -EOPNOTSUPP;
 		break;
 
 	default:
-		dev_info(&adap->dev, "Unsupported I2C size\n");
-		return -1;
-		break;
+		dev_info(&adap->dev, "Unsupported SMBus operation\n");
+		return -EOPNOTSUPP;
 	}
 
-	if (sis96x_transaction(size))
-		return -1;
+	status = sis96x_transaction(size);
+	if (status)
+		return status;
 
 	if ((size != SIS96x_PROC_CALL) &&
 		((read_write == I2C_SMBUS_WRITE) || (size == SIS96x_QUICK)))
--- g26.orig/drivers/i2c/busses/i2c-stub.c	2008-05-11 17:55:00.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-stub.c	2008-05-11 17:55:08.000000000 -0700
@@ -43,7 +43,7 @@ struct stub_chip {
 
 static struct stub_chip *stub_chips;
 
-/* Return -1 on error. */
+/* Return negative errno on error. */
 static s32 stub_xfer(struct i2c_adapter * adap, u16 addr, unsigned short flags,
 	char read_write, u8 command, int size, union i2c_smbus_data * data)
 {
@@ -120,7 +120,7 @@ static s32 stub_xfer(struct i2c_adapter 
 
 	default:
 		dev_dbg(&adap->dev, "Unsupported I2C/SMBus command\n");
-		ret = -1;
+		ret = -EOPNOTSUPP;
 		break;
 	} /* switch (size) */
 
--- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:01.000000000 -0700
+++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:08.000000000 -0700
@@ -152,7 +152,7 @@ static int vt596_transaction(u8 size)
 		if ((temp = inb_p(SMBHSTSTS)) & 0x1F) {
 			dev_err(&vt596_adapter.dev, "SMBus reset failed! "
 				"(0x%02x)\n", temp);
-			return -1;
+			return -EBUSY;
 		}
 	}
 
@@ -167,30 +167,32 @@ static int vt596_transaction(u8 size)
 
 	/* If the SMBus is still busy, we give up */
 	if (timeout >= MAX_TIMEOUT) {
-		result = -1;
+		result = -ETIMEDOUT;
 		dev_err(&vt596_adapter.dev, "SMBus timeout!\n");
 	}
 
 	if (temp & 0x10) {
-		result = -1;
+		result = -EIO;
 		dev_err(&vt596_adapter.dev, "Transaction failed (0x%02x)\n",
 			size);
 	}
 
 	if (temp & 0x08) {
-		result = -1;
+		result = -EIO;
 		dev_err(&vt596_adapter.dev, "SMBus collision!\n");
 	}
 
 	if (temp & 0x04) {
 		int read = inb_p(SMBHSTADD) & 0x01;
-		result = -1;
+		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;
 	}
 
 	/* Resetting status register */
@@ -202,12 +204,13 @@ static int vt596_transaction(u8 size)
 	return result;
 }
 
-/* Return -1 on error, 0 on success */
+/* Return negative errno on error, 0 on success */
 static s32 vt596_access(struct i2c_adapter *adap, u16 addr,
 		unsigned short flags, char read_write, u8 command,
 		int size, union i2c_smbus_data *data)
 {
 	int i;
+	int status;
 
 	switch (size) {
 	case I2C_SMBUS_QUICK:
@@ -258,8 +261,9 @@ static s32 vt596_access(struct i2c_adapt
 
 	outb_p(((addr & 0x7f) << 1) | read_write, SMBHSTADD);
 
-	if (vt596_transaction(size)) /* Error in transaction */
-		return -1;
+	status = vt596_transaction(size);
+	if (status)
+		return status;
 
 	if ((read_write == I2C_SMBUS_WRITE) || (size == VT596_QUICK))
 		return 0;
@@ -287,7 +291,7 @@ static s32 vt596_access(struct i2c_adapt
 exit_unsupported:
 	dev_warn(&vt596_adapter.dev, "Unsupported command invoked! (0x%02x)\n",
 		 size);
-	return -1;
+	return -EOPNOTSUPP;
 }
 
 static u32 vt596_func(struct i2c_adapter *adapter)

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi David,

On Mon, 12 May 2008 09:43:04 -0700, David Brownell wrote:
> --- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:01.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:08.000000000 -0700
> @@ -106,8 +106,11 @@ static int ali1563_transaction(struct i2
>   	}
>  
>  	/* device error - no response, ignore the autodetection case */
> -	if ((data & HST_STS_DEVERR) && (size != HST_CNTL2_QUICK)) {
> -		dev_err(&a->dev, "Device error!\n");
> +	if (data & HST_STS_DEVERR) {
> +		if (size != HST_CNTL2_QUICK)
> +			dev_err(&a->dev, "Device error!\n");
> +		else
> +			return -ENXIO;
>  	}

This one doesn't look right. You should return -ENXIO in all cases (but
only print the error message on size != HST_CNTL2_QUICK.) The right way
to do this, I think, would be to move this specific check at the end of
the function, so that the other error checks are still done in all
cases.

>  
>  	/* bus collision */
> @@ -122,13 +125,14 @@ static int ali1563_transaction(struct i2
>  		outb_p(0x0,SMB_HST_CNTL2);
>  	}
>  
> -	return -1;
> +	return -EIO;
>  }
>  
>  static int ali1563_block_start(struct i2c_adapter * a)
>  {
>  	u32 data;
>  	int timeout;
> +	int status = -EIO;
>  
>  	dev_dbg(&a->dev, "Block (pre): STS=%02x, CNTL1=%02x, "
>  		"CNTL2=%02x, CMD=%02x, ADD=%02x, DAT0=%02x, DAT1=%02x\n",
> @@ -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".

> +
>  	dev_err(&a->dev, "SMBus Error: %s%s%s%s%s\n",
>  		timeout ? "Timeout " : "",

BTW, isn't there a bug here? I think the test should be timeout == 0.

>  		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()?

> --- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:01.000000000 -0700
> +++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:08.000000000 -0700
> @@ -152,7 +152,7 @@ static int vt596_transaction(u8 size)
> (...)
>  	if (temp & 0x04) {
>  		int read = inb_p(SMBHSTADD) & 0x01;
> -		result = -1;
> +		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;
>  	}

This is not correct, the result is always -ENXIO, whether you display
an error message or not.

All the rest looks fine now. BTW, I can test i2c-piix4, i2c-i801,
i2c-viapro and i2c-nforce2 here, and will do soon.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[David Brownell]

On Thursday 15 May 2008, Jean Delvare wrote:
> Hi David,
> 
> On Mon, 12 May 2008 09:43:04 -0700, David Brownell wrote:
> > --- g26.orig/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:01.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-ali1563.c	2008-05-11 17:55:08.000000000 -0700
> > 	...
> 
> This one doesn't look right. You should return -ENXIO in all cases (but
> only print the error message on size != HST_CNTL2_QUICK.) The right way
> to do this, I think, would be to move this specific check at the end of
> the function, so that the other error checks are still done in all
> cases.

OK, done ...


> > @@ -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.


> > +
> >  	dev_err(&a->dev, "SMBus Error: %s%s%s%s%s\n",
> >  		timeout ? "Timeout " : "",
> 
> BTW, isn't there a bug here? I think the test should be timeout == 0.

Right, fixed.

> >  		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 ... ;)

 
> > --- g26.orig/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:01.000000000 -0700
> > +++ g26/drivers/i2c/busses/i2c-viapro.c	2008-05-11 17:55:08.000000000 -0700
> > @@ -152,7 +152,7 @@ static int vt596_transaction(u8 size)
> > (...)
> >  	if (temp & 0x04) {
> >  		int read = inb_p(SMBHSTADD) & 0x01;
> > -		result = -1;
> > +		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;
> >  	}
> 
> This is not correct, the result is always -ENXIO, whether you display
> an error message or not.

Fixed.


> All the rest looks fine now. BTW, I can test i2c-piix4, i2c-i801,
> i2c-viapro and i2c-nforce2 here, and will do soon.

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 */

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

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