i2c_adapters: return -Errno not -1

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 <[email protected]>

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

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.

Quoted 6 lines · Jean Delvare

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 <[email protected]>

---
 Documentation/i2c/fault-codes.txt |  118 ++++++++++++++++++++++++++++++++++++++

Documentation/i2c/fault-codes.txt new file +20 -0

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

Documentation/i2c/fault-codes.txt new file +12 -0

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

Hi David,

Quoted 4 lines · David Brownell

Quoted 2 lines · Jean Delvare

... list your error value choices for the common
error conditions of I2C and SMBus controllers ...

Quoted 2 lines

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.

Documentation/i2c/fault-codes.txt line 54

Quoted 8 lines

Documentation/i2c/fault-codes.txt new file +8 -0

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

Documentation/i2c/fault-codes.txt line 101

Quoted 7 lines

Documentation/i2c/fault-codes.txt new file +7 -0

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

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.

Quoted 3 lines

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.

Quoted 7 lines · Jean Delvare

Quoted 3 lines

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 <[email protected]>

---
 Documentation/i2c/fault-codes |  127 ++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

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 <[email protected]>

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

drivers/i2c/algos/i2c-algo-bit.c +2 -3

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

drivers/i2c/busses/i2c-ali1535.c +3 -3

@@ -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");
 	}
 

drivers/i2c/busses/i2c-ali1535.c +3 -3

 		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)

drivers/i2c/busses/i2c-ali1535.c +2 -4

 	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;

drivers/i2c/busses/i2c-ali1563.c +0 -0

 static int ali1563_block(struct i2c_adapter * a, union i2c_smbus_data * data, u8 rw)

drivers/i2c/busses/i2c-ali15x3.c +3 -3

@@ -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");
 	}
 

drivers/i2c/busses/i2c-ali15x3.c +2 -2

 		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)

drivers/i2c/busses/i2c-ali15x3.c +3 -2

 	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;

drivers/i2c/busses/i2c-amd756-s4882.c +1 -1

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

drivers/i2c/busses/i2c-amd756.c +9 -7

 	}
 
 	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;

drivers/i2c/busses/i2c-amd8111.c +25 -14

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

drivers/i2c/busses/i2c-amd8111.c +1 -1

 	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;

drivers/i2c/busses/i2c-i801.c +3 -4

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

drivers/i2c/busses/i2c-i801.c +1 -0

 	}
 

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

drivers/i2c/busses/i2c-i801.c +4 -4

 		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;

drivers/i2c/busses/i2c-i801.c +5 -5

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

drivers/i2c/busses/i2c-nforce2.c +4 -4

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

drivers/i2c/busses/i2c-nforce2.c +4 -3

 				}
 				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;

drivers/i2c/busses/i2c-nforce2.c +3 -3

 			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;

drivers/i2c/busses/i2c-piix4.c +3 -4

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

drivers/i2c/busses/i2c-piix4.c +6 -4

 	}
 

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

drivers/i2c/busses/i2c-sis5595.c +1 -1

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

drivers/i2c/busses/i2c-sis5595.c +8 -5

 	}
 
 	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)))

drivers/i2c/busses/i2c-sis630.c +1 -1

@@ -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) {

drivers/i2c/busses/i2c-sis630.c +17 -11

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

drivers/i2c/busses/i2c-sis630.c +4 -3

 	}
 
-	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;
 	}
 

drivers/i2c/busses/i2c-sis96x.c +2 -2

@@ -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");

drivers/i2c/busses/i2c-sis96x.c +2 -1

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

drivers/i2c/busses/i2c-sis96x.c +3 -2

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

drivers/i2c/busses/i2c-stub.c +2 -2

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

drivers/i2c/busses/i2c-viapro.c +3 -3

@@ -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);
 	}
 

drivers/i2c/busses/i2c-viapro.c +5 -3

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

Quoted 2 lines

drivers/i2c/.............................

 15 files changed, 176 insertions(+), 142 deletions(-)

Quoted 16 lines

drivers/i2c/algos/i2c-algo-bit.c +5 -2

...

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

drivers/i2c/algos/i2c-algo-bit.c static int bit_doAddress(struct i2c_adap · line 476

Quoted 2 lines

drivers/i2c/algos/i2c-algo-bit.c +0 -0

 	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.

Quoted 4 lines

drivers/i2c/busses/i2c-ali1535.c +0 -0

...

@@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2

drivers/i2c/busses/i2c-ali1535.c static int ali1535_transaction(struct i2 · line 298

Quoted 5 lines

drivers/i2c/busses/i2c-ali1535.c +1 -1

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

		...

drivers/i2c/busses/i2c-ali1535.c static s32 ali1535_access(struct i2c_ada · line 363

Quoted 7 lines

drivers/i2c/busses/i2c-ali1535.c +1 -1

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

Quoted 18 lines

drivers/i2c/busses/i2c-amd756-s4882.c +2 -2

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

drivers/i2c/busses/i2c-amd756-s4882.c static inline s32 amd756_access_channel( · line 91

Quoted 1 line

drivers/i2c/busses/i2c-amd756-s4882.c +0 -0

 	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.

drivers/i2c/busses/i2c-amd756.c static int amd756_transaction(struct i2c · line 155

Quoted 9 lines

drivers/i2c/busses/i2c-amd756.c +1 -1

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

drivers/i2c/busses/i2c-i801.c

Quoted 3 lines

drivers/i2c/busses/i2c-i801.c +0 -0

....

Quoted 17 lines

drivers/i2c/busses/i2c-i801.c +2 -2

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

drivers/i2c/busses/i2c-i801.c static int i801_transaction(int xact) · line 195

Quoted 4 lines

drivers/i2c/busses/i2c-i801.c +1 -1

 	if (temp & SMBHSTSTS_DEV_ERR) {
-		result = -1;
+		result = -EIO;
 		dev_dbg(&I801_dev->dev, "Error: no response!\n");

-ENODEV

drivers/i2c/busses/i2c-i801.c static int i801_block_transaction_by_blo · line 254

Quoted 8 lines

drivers/i2c/busses/i2c-i801.c +3 -1

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

Quoted 2 lines · Jean Delvare

Quoted 2 lines

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.

Quoted 16 lines

Quoted 16 lines

drivers/i2c/algos/i2c-algo-bit.c +5 -2

...

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

Quoted 5 lines

drivers/i2c/algos/i2c-algo-bit.c static int bit_doAddress(struct i2c_adap · line 476

Quoted 2 lines

drivers/i2c/algos/i2c-algo-bit.c +0 -0

 	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.

Quoted 4 lines

Quoted 4 lines

drivers/i2c/busses/i2c-ali1535.c +0 -0

...

@@ -295,7 +295,7 @@ static int ali1535_transaction(struct i2

Quoted 8 lines

drivers/i2c/busses/i2c-ali1535.c static int ali1535_transaction(struct i2 · line 298

Quoted 5 lines

drivers/i2c/busses/i2c-ali1535.c +1 -1

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

Quoted 18 lines

Quoted 18 lines

drivers/i2c/busses/i2c-amd756-s4882.c +2 -2

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

Quoted 5 lines

drivers/i2c/busses/i2c-amd756-s4882.c static inline s32 amd756_access_channel( · line 91

Quoted 1 line

drivers/i2c/busses/i2c-amd756-s4882.c +0 -0

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

Quoted 11 lines

drivers/i2c/busses/i2c-amd756.c static int amd756_transaction(struct i2c · line 155

Quoted 9 lines

drivers/i2c/busses/i2c-amd756.c +1 -1

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

Quoted 2 lines

drivers/i2c/busses/i2c-i801.c

Quoted 2 lines

drivers/i2c/busses/i2c-i801.c +0 -0

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

Quoted 6 lines

drivers/i2c/busses/i2c-i801.c static int i801_transaction(int xact) · line 195

Quoted 4 lines

drivers/i2c/busses/i2c-i801.c +1 -1

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

Quoted 11 lines

drivers/i2c/busses/i2c-i801.c static int i801_block_transaction_by_blo · line 254

Quoted 8 lines

drivers/i2c/busses/i2c-i801.c +3 -1

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

Quoted 2 lines

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.

Quoted 5 lines

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

Quoted 8 lines

Quoted 2 lines

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.

Quoted 1 line

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.

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.

Quoted 4 lines

Quoted 2 lines

drivers/i2c/busses/i2c-i801.c

Quoted 2 lines

drivers/i2c/busses/i2c-i801.c +0 -0

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

Hell no. You saw i2c-i810 go away, not i2c-i801.

Quoted 11 lines

Quoted 5 lines

drivers/i2c/busses/i2c-i801.c

Quoted 2 lines

drivers/i2c/busses/i2c-i801.c +1 -1

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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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

Quoted 2 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

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.

Quoted 2 lines · you

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.

Quoted 3 lines

(...)
When calling strerror on EILSEQ, you get: "Invalid or incomplete multibyte or wide character".

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

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 3 lines

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

Quoted 1 line

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

Quoted 1 line

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.

Quoted 7 lines

Quoted 2 lines

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

drivers/i2c/busses/i2c-ali1563.c static int ali1563_block_start(struct i2 · line 174

Quoted 9 lines

 static int ali1563_block_start(struct i2c_adapter * a)

drivers/i2c/busses/i2c-ali1563.c +1 -1

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

Quoted 11 lines · you

drivers/i2c/busses/i2c-ali1563.c static int ali1563_block_start(struct i2 · line 174

Quoted 9 lines

 static int ali1563_block_start(struct i2c_adapter * a)

drivers/i2c/busses/i2c-ali1563.c +1 -1

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

Quoted 13 lines · David Brownell

Quoted 11 lines · you

Review static int ali1563_block_start(struct i2 · line 174

Quoted 9 lines

 static int ali1563_block_start(struct i2c_adapter * a)

(quoted diff) +1 -1

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

Quoted 3 lines

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.

Quoted 2 lines

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 :(

Quoted 2 lines

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

Agreed.

Quoted 9 lines · Jean Delvare

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.

Quoted 5 lines

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.

Quoted 5 lines

      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.

Quoted 2 lines

      if (~temp[0] & AMD_SMB_STS_DONE) {

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