[i2c] [patch 2.6.23-rc5 2/2] i2c-algo-bit read block data bugfix

David Brownell david-b at pacbell.net
Fri Sep 7 23:59:15 CEST 2007


> Also, I am worried that we no longer send a nack if we receive an
> invalid length from the slave. Probably not a big deal in practice as
> we'll send a stop condition right after that, but still I believe that
> the standard-compliant behavior would be to send a nack. Can you update
> your patch so that it does this? You may have to move the ack/nack
> generation code to a separate helper function.

See below -- sanity tested with correctly functioning slave firmware,
but not with the broken type.

============	CUT HERE
This fixes a bug in the way i2c-algo-bit handles I2C_M_RECV_LEN,
used to implement i2c_smbus_read_block_data().  Previously, in the
absence of PEC (rarely used!) it would NAK the "length" byte:

	S addr Rd [A] [length] NA

That prevents the subsequent data bytes from being read:

	S addr Rd [A] [length] { A [data] }* NA

The primary fix just reorders two code blocks, so the length used
in the "should I NAK now?" check incorporates the data which it
just read from the slave device.

However, that move also highlighted other fault handling glitches.
This fixes those by abstracting the RX path ack/nak logic, so it
can be used in more than one location.  Also, a few CodingStyle
issues were also resolved.

Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>

---
 drivers/i2c/algos/i2c-algo-bit.c |   62 +++++++++++++++++++++++----------------
 1 files changed, 38 insertions(+), 24 deletions(-)

--- at91.orig/drivers/i2c/algos/i2c-algo-bit.c	2007-09-05 16:10:35.000000000 -0700
+++ at91/drivers/i2c/algos/i2c-algo-bit.c	2007-09-07 10:12:31.000000000 -0700
@@ -357,13 +357,29 @@ static int sendbytes(struct i2c_adapter 
 	return wrcount;
 }
 
+static int acknak(struct i2c_adapter *i2c_adap, u8 byte, int is_ack)
+{
+	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+
+	/* assert: sda is high */
+	if (is_ack)		/* send ack */
+		setsda(adap, 0);
+	udelay((adap->udelay + 1) / 2);
+	if (sclhi(adap) < 0) {	/* timeout */
+		dev_err(&i2c_adap->dev, "readbytes: ack/nak timeout\n");
+		return -ETIMEDOUT;
+	}
+	scllo(adap);
+	return 0;
+}
+
 static int readbytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
 {
 	int inval;
-	int rdcount=0;   	/* counts bytes read */
-	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
+	int rdcount = 0;	/* counts bytes read */
 	unsigned char *temp = msg->buf;
 	int count = msg->len;
+	const unsigned flags = msg->flags;
 
 	while (count > 0) {
 		inval = i2c_inb(i2c_adap);
@@ -377,38 +393,36 @@ static int readbytes(struct i2c_adapter 
 		temp++;
 		count--;
 
-		if (msg->flags & I2C_M_NO_RD_ACK) {
-			bit_dbg(2, &i2c_adap->dev, "i2c_inb: 0x%02x\n",
-				inval);
-			continue;
-		}
-
-		/* assert: sda is high */
-		if (count)		/* send ack */
-			setsda(adap, 0);
-		udelay((adap->udelay + 1) / 2);
-		bit_dbg(2, &i2c_adap->dev, "i2c_inb: 0x%02x %s\n", inval,
-			count ? "A" : "NA");
-		if (sclhi(adap)<0) {	/* timeout */
-			dev_err(&i2c_adap->dev, "readbytes: timeout at ack\n");
-			return -ETIMEDOUT;
-		};
-		scllo(adap);
-
 		/* Some SMBus transactions require that we receive the
-		   transaction length as the first read byte. */
-		if (rdcount == 1 && (msg->flags & I2C_M_RECV_LEN)) {
+		 * transaction length as the first read byte.
+		 */
+		if (rdcount == 1 && (flags & I2C_M_RECV_LEN)) {
 			if (inval <= 0 || inval > I2C_SMBUS_BLOCK_MAX) {
+				if (!(flags & I2C_M_NO_RD_ACK))
+					acknak(i2c_adap, inval, 0);
 				dev_err(&i2c_adap->dev, "readbytes: invalid "
 					"block length (%d)\n", inval);
 				return -EREMOTEIO;
 			}
 			/* The original count value accounts for the extra
-			   bytes, that is, either 1 for a regular transaction,
-			   or 2 for a PEC transaction. */
+			 * bytes, that is, either 1 for a regular transaction,
+			 * or 2 for a PEC transaction.
+			 */
 			count += inval;
 			msg->len += inval;
 		}
+
+		bit_dbg(2, &i2c_adap->dev, "readbytes: 0x%02x %s\n",
+			inval,
+			(flags & I2C_M_NO_RD_ACK)
+				? "(no ack/nak)"
+				: (count ? "A" : "NA"));
+
+		if (!(flags & I2C_M_NO_RD_ACK)) {
+			inval = acknak(i2c_adap, inval, count);
+			if (inval < 0)
+				return inval;
+		}
 	}
 	return rdcount;
 }



More information about the i2c mailing list