[i2c] [patch 2.6.23-rc6] i2c-algo-bit whitespace fixes (+ NAK/ARB comments)

David Brownell david-b at pacbell.net
Tue Nov 6 19:48:11 CET 2007


On Wednesday 17 October 2007, Jean Delvare wrote:
> Please send an updated patch and I'll apply it.

Now that "checkpatch --file" exists, I ran that and fixed the
issues it reported.  Also I:

 - cleaned up the sendbytes() return path to distinguish NAK
   from other cases ... which today are only timeouts (errors)
   but someday should include lost arbitration (not errors).

 - fixed the bug where EFAULT was wrongly returned for NAK
   errors; it's not a bad user address.

Plus addressing your feedback.

- Dave


=======	CUT HERE
Fix *LOTS* of whitespace goofs and checkpatch.pl warnings, strangely
parenthesized ternary expressions, and other CodingStyle glitches.

Update comments and logging on return path for byte writes.  NAK is
an error, to be reported or optionally ignored.  Timeouts are always
errors.  Lost arbitration is not currently handled, so don't even list
it as an option in the error message.

Don't return bogus EFAULT code for inappropriate NAK; EIO is better,
there is no bad userspace address in question.

Signed-off-by: David Brownell <dbrownell at users.sourceforge.net>
---
 drivers/i2c/algos/i2c-algo-bit.c |  232 +++++++++++++++++++++------------------
 1 file changed, 128 insertions(+), 104 deletions(-)

--- g26.orig/drivers/i2c/algos/i2c-algo-bit.c	2007-11-05 22:19:43.000000000 -0800
+++ g26/drivers/i2c/algos/i2c-algo-bit.c	2007-11-06 10:32:42.000000000 -0800
@@ -1,7 +1,7 @@
-/* ------------------------------------------------------------------------- */
-/* i2c-algo-bit.c i2c driver algorithms for bit-shift adapters		     */
-/* ------------------------------------------------------------------------- */
-/*   Copyright (C) 1995-2000 Simon G. Vogl
+/* -------------------------------------------------------------------------
+ * i2c-algo-bit.c i2c driver algorithms for bit-shift adapters
+ * -------------------------------------------------------------------------
+ *   Copyright (C) 1995-2000 Simon G. Vogl
 
     This program is free software; you can redistribute it and/or modify
     it under the terms of the GNU General Public License as published by
@@ -15,8 +15,8 @@
 
     You should have received a copy of the GNU General Public License
     along with this program; if not, write to the Free Software
-    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.		     */
-/* ------------------------------------------------------------------------- */
+    Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
+ * ------------------------------------------------------------------------- */
 
 /* With some changes from Frodo Looijaard <frodol at dds.nl>, Kyösti Mälkki
    <kmalkki at cc.hut.fi> and Jean Delvare <khali at linux-fr.org> */
@@ -60,26 +60,26 @@ MODULE_PARM_DESC(i2c_debug,
 
 /* --- setting states on the bus with the right timing: ---------------	*/
 
-#define setsda(adap,val) adap->setsda(adap->data, val)
-#define setscl(adap,val) adap->setscl(adap->data, val)
-#define getsda(adap) adap->getsda(adap->data)
-#define getscl(adap) adap->getscl(adap->data)
+#define setsda(adap, val)	adap->setsda(adap->data, val)
+#define setscl(adap, val)	adap->setscl(adap->data, val)
+#define getsda(adap)		adap->getsda(adap->data)
+#define getscl(adap)		adap->getscl(adap->data)
 
 static inline void sdalo(struct i2c_algo_bit_data *adap)
 {
-	setsda(adap,0);
+	setsda(adap, 0);
 	udelay((adap->udelay + 1) / 2);
 }
 
 static inline void sdahi(struct i2c_algo_bit_data *adap)
 {
-	setsda(adap,1);
+	setsda(adap, 1);
 	udelay((adap->udelay + 1) / 2);
 }
 
 static inline void scllo(struct i2c_algo_bit_data *adap)
 {
-	setscl(adap,0);
+	setscl(adap, 0);
 	udelay(adap->udelay / 2);
 }
 
@@ -91,22 +91,21 @@ static int sclhi(struct i2c_algo_bit_dat
 {
 	unsigned long start;
 
-	setscl(adap,1);
+	setscl(adap, 1);
 
 	/* Not all adapters have scl sense line... */
 	if (!adap->getscl)
 		goto done;
 
-	start=jiffies;
-	while (! getscl(adap) ) {	
- 		/* the hw knows how to read the clock line,
- 		 * so we wait until it actually gets high.
- 		 * This is safer as some chips may hold it low
- 		 * while they are processing data internally. 
- 		 */
-		if (time_after_eq(jiffies, start+adap->timeout)) {
+	start = jiffies;
+	while (!getscl(adap)) {
+		/* This hw knows how to read the clock line, so we wait
+		 * until it actually gets high.  This is safer as some
+		 * chips may hold it low ("clock stretching") while they
+		 * are processing data internally.
+		 */
+		if (time_after_eq(jiffies, start + adap->timeout))
 			return -ETIMEDOUT;
-		}
 		cond_resched();
 	}
 #ifdef DEBUG
@@ -118,11 +117,11 @@ static int sclhi(struct i2c_algo_bit_dat
 done:
 	udelay(adap->udelay);
 	return 0;
-} 
+}
 
 
 /* --- other auxiliary functions --------------------------------------	*/
-static void i2c_start(struct i2c_algo_bit_data *adap) 
+static void i2c_start(struct i2c_algo_bit_data *adap)
 {
 	/* assert: scl, sda are high */
 	setsda(adap, 0);
@@ -130,7 +129,7 @@ static void i2c_start(struct i2c_algo_bi
 	scllo(adap);
 }
 
-static void i2c_repstart(struct i2c_algo_bit_data *adap) 
+static void i2c_repstart(struct i2c_algo_bit_data *adap)
 {
 	/* assert: scl is low */
 	sdahi(adap);
@@ -141,18 +140,18 @@ static void i2c_repstart(struct i2c_algo
 }
 
 
-static void i2c_stop(struct i2c_algo_bit_data *adap) 
+static void i2c_stop(struct i2c_algo_bit_data *adap)
 {
 	/* assert: scl is low */
 	sdalo(adap);
-	sclhi(adap); 
+	sclhi(adap);
 	setsda(adap, 1);
 	udelay(adap->udelay);
 }
 
 
 
-/* send a byte without start cond., look for arbitration, 
+/* send a byte without start cond., look for arbitration,
    check ackn. from slave */
 /* returns:
  * 1 if the device acknowledged
@@ -167,27 +166,33 @@ static int i2c_outb(struct i2c_adapter *
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 
 	/* assert: scl is low */
-	for ( i=7 ; i>=0 ; i-- ) {
+	for (i = 7; i >= 0; i--) {
 		sb = (c >> i) & 1;
-		setsda(adap,sb);
+		setsda(adap, sb);
 		udelay((adap->udelay + 1) / 2);
-		if (sclhi(adap)<0) { /* timed out */
+		if (sclhi(adap) < 0) { /* timed out */
 			bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 				"timeout at bit #%d\n", (int)c, i);
 			return -ETIMEDOUT;
-		};
-		/* do arbitration here: 
-		 * if ( sb && ! getsda(adap) ) -> ouch! Get out of here.
+		}
+		/* FIXME do arbitration here:
+		 * if (sb && !getsda(adap)) -> ouch! Get out of here.
+		 *
+		 * Report a unique code, so higher level code can retry
+		 * the whole (combined) message and *NOT* issue STOP.
 		 */
 		scllo(adap);
 	}
 	sdahi(adap);
-	if (sclhi(adap)<0){ /* timeout */
+	if (sclhi(adap) < 0) { /* timeout */
 		bit_dbg(1, &i2c_adap->dev, "i2c_outb: 0x%02x, "
 			"timeout at ack\n", (int)c);
 		return -ETIMEDOUT;
-	};
-	/* read ack: SDA should be pulled down by slave */
+	}
+
+	/* read ack: SDA should be pulled down by slave, or it may
+	 * NAK (usually to report problems with the data we wrote).
+	 */
 	ack = !getsda(adap);    /* ack: sda is pulled low -> success */
 	bit_dbg(2, &i2c_adap->dev, "i2c_outb: 0x%02x %s\n", (int)c,
 		ack ? "A" : "NA");
@@ -198,24 +203,24 @@ static int i2c_outb(struct i2c_adapter *
 }
 
 
-static int i2c_inb(struct i2c_adapter *i2c_adap) 
+static int i2c_inb(struct i2c_adapter *i2c_adap)
 {
 	/* read byte via i2c port, without start/stop sequence	*/
 	/* acknowledge is sent in i2c_read.			*/
 	int i;
-	unsigned char indata=0;
+	unsigned char indata = 0;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
 
 	/* assert: scl is low */
 	sdahi(adap);
-	for (i=0;i<8;i++) {
-		if (sclhi(adap)<0) { /* timeout */
+	for (i = 0; i < 8; i++) {
+		if (sclhi(adap) < 0) { /* timeout */
 			bit_dbg(1, &i2c_adap->dev, "i2c_inb: timeout at bit "
 				"#%d\n", 7 - i);
 			return -ETIMEDOUT;
-		};
+		}
 		indata *= 2;
-		if ( getsda(adap) ) 
+		if (getsda(adap))
 			indata |= 0x01;
 		setscl(adap, 0);
 		udelay(i == 7 ? adap->udelay / 2 : adap->udelay);
@@ -228,66 +233,67 @@ static int i2c_inb(struct i2c_adapter *i
  * Sanity check for the adapter hardware - check the reaction of
  * the bus lines only if it seems to be idle.
  */
-static int test_bus(struct i2c_algo_bit_data *adap, char* name) {
-	int scl,sda;
+static int test_bus(struct i2c_algo_bit_data *adap, char *name)
+{
+	int scl, sda;
 
-	if (adap->getscl==NULL)
+	if (adap->getscl == NULL)
 		pr_info("%s: Testing SDA only, SCL is not readable\n", name);
 
-	sda=getsda(adap);
-	scl=(adap->getscl==NULL?1:getscl(adap));
-	if (!scl || !sda ) {
+	sda = getsda(adap);
+	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
+	if (!scl || !sda) {
 		printk(KERN_WARNING "%s: bus seems to be busy\n", name);
 		goto bailout;
 	}
 
 	sdalo(adap);
-	sda=getsda(adap);
-	scl=(adap->getscl==NULL?1:getscl(adap));
-	if ( 0 != sda ) {
+	sda = getsda(adap);
+	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
+	if (sda) {
 		printk(KERN_WARNING "%s: SDA stuck high!\n", name);
 		goto bailout;
 	}
-	if ( 0 == scl ) {
+	if (!scl) {
 		printk(KERN_WARNING "%s: SCL unexpected low "
 		       "while pulling SDA low!\n", name);
 		goto bailout;
-	}		
+	}
 
 	sdahi(adap);
-	sda=getsda(adap);
-	scl=(adap->getscl==NULL?1:getscl(adap));
-	if ( 0 == sda ) {
+	sda = getsda(adap);
+	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
+	if (!sda) {
 		printk(KERN_WARNING "%s: SDA stuck low!\n", name);
 		goto bailout;
 	}
-	if ( 0 == scl ) {
+	if (!scl) {
 		printk(KERN_WARNING "%s: SCL unexpected low "
 		       "while pulling SDA high!\n", name);
 		goto bailout;
 	}
 
 	scllo(adap);
-	sda=getsda(adap);
-	scl=(adap->getscl==NULL?0:getscl(adap));
-	if ( 0 != scl ) {
+	sda = getsda(adap);
+	scl = (adap->getscl == NULL) ? 0 : getscl(adap);
+	if (scl) {
 		printk(KERN_WARNING "%s: SCL stuck high!\n", name);
 		goto bailout;
 	}
-	if ( 0 == sda ) {
+	if (!sda) {
 		printk(KERN_WARNING "%s: SDA unexpected low "
 		       "while pulling SCL low!\n", name);
 		goto bailout;
 	}
-	
+
 	sclhi(adap);
-	sda=getsda(adap);
-	scl=(adap->getscl==NULL?1:getscl(adap));
-	if ( 0 == scl ) {
+	sda = getsda(adap);
+	scl = (adap->getscl == NULL) ? 1 : getscl(adap);
+	if (!scl) {
 		printk(KERN_WARNING "%s: SCL stuck low!\n", name);
 		goto bailout;
 	}
-	if ( 0 == sda ) {
+	if (!sda) {
 		printk(KERN_WARNING "%s: SDA unexpected low "
 		       "while pulling SCL high!\n", name);
 		goto bailout;
@@ -314,9 +320,10 @@ 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;
-	for (i=0;i<=retries;i++) {
-		ret = i2c_outb(i2c_adap,addr);
+	int i, ret = -1;
+
+	for (i = 0; i <= retries; i++) {
+		ret = i2c_outb(i2c_adap, addr);
 		if (ret == 1 || i == retries)
 			break;
 		bit_dbg(3, &i2c_adap->dev, "emitting stop condition\n");
@@ -337,20 +344,38 @@ static int sendbytes(struct i2c_adapter 
 {
 	const unsigned char *temp = msg->buf;
 	int count = msg->len;
-	unsigned short nak_ok = msg->flags & I2C_M_IGNORE_NAK; 
+	unsigned short nak_ok = msg->flags & I2C_M_IGNORE_NAK;
 	int retval;
-	int wrcount=0;
+	int wrcount = 0;
 
 	while (count > 0) {
 		retval = i2c_outb(i2c_adap, *temp);
-		if ((retval>0) || (nak_ok && (retval==0)))  { /* ok or ignored NAK */
-			count--; 
+
+		/* OK/ACK; or ignored NAK */
+		if ((retval > 0) || (nak_ok && (retval == 0))) {
+			count--;
 			temp++;
 			wrcount++;
-		} else { /* arbitration or no acknowledge */
-			dev_err(&i2c_adap->dev, "sendbytes: error - bailout.\n");
-			return (retval<0)? retval : -EFAULT;
-			        /* got a better one ?? */
+
+		/* A slave NAKing the master means the slave didn't like
+		 * something about the data it saw.  For example, maybe
+		 * the SMBus PEC was wrong.
+		 */
+		} else if (retval == 0) {
+			dev_err(&i2c_adap->dev, "sendbytes: NAK bailout.\n");
+			return -EIO;
+
+		/* Timeout; or (someday) lost arbitration
+		 *
+		 * FIXME Lost ARB implies retrying the transaction from
+		 * the first message, after the "winning" master issues
+		 * its STOP.  As a rule, upper layer code has no reason
+		 * to know or care about this ... it is *NOT an error.
+		 */
+		} else {
+			dev_err(&i2c_adap->dev, "sendbytes: error %d\n",
+					retval);
+			return retval;
 		}
 	}
 	return wrcount;
@@ -375,14 +400,14 @@ static int acknak(struct i2c_adapter *i2
 static int readbytes(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
 {
 	int inval;
-	int rdcount=0;   	/* counts bytes read */
+	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);
-		if (inval>=0) {
+		if (inval >= 0) {
 			*temp = inval;
 			rdcount++;
 		} else {   /* read timed out */
@@ -393,7 +418,7 @@ static int readbytes(struct i2c_adapter 
 		count--;
 
 		/* Some SMBus transactions require that we receive the
-		   transaction length as the first read byte. */
+		 * 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))
@@ -403,8 +428,8 @@ static int readbytes(struct i2c_adapter 
 				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;
 		}
@@ -430,7 +455,7 @@ static int readbytes(struct i2c_adapter 
  * returns:
  *  0 everything went okay, the chip ack'ed, or IGNORE_NAK flag was set
  * -x an error occurred (like: -EREMOTEIO if the device did not answer, or
- *	-ETIMEDOUT, for example if the lines are stuck...) 
+ *	-ETIMEDOUT, for example if the lines are stuck...)
  */
 static int bit_doAddress(struct i2c_adapter *i2c_adap, struct i2c_msg *msg)
 {
@@ -442,10 +467,10 @@ static int bit_doAddress(struct i2c_adap
 	int ret, retries;
 
 	retries = nak_ok ? 0 : i2c_adap->retries;
-	
-	if ( (flags & I2C_M_TEN)  ) { 
+
+	if (flags & I2C_M_TEN) {
 		/* a ten bit address */
-		addr = 0xf0 | (( msg->addr >> 7) & 0x03);
+		addr = 0xf0 | ((msg->addr >> 7) & 0x03);
 		bit_dbg(2, &i2c_adap->dev, "addr0: %d\n", addr);
 		/* try extended address code...*/
 		ret = try_address(i2c_adap, addr, retries);
@@ -455,33 +480,33 @@ static int bit_doAddress(struct i2c_adap
 			return -EREMOTEIO;
 		}
 		/* the remaining 8 bit address */
-		ret = i2c_outb(i2c_adap,msg->addr & 0x7f);
+		ret = i2c_outb(i2c_adap, msg->addr & 0x7f);
 		if ((ret != 1) && !nak_ok) {
 			/* the chip did not ack / xmission error occurred */
 			dev_err(&i2c_adap->dev, "died at 2nd address code\n");
 			return -EREMOTEIO;
 		}
-		if ( flags & I2C_M_RD ) {
+		if (flags & I2C_M_RD) {
 			bit_dbg(3, &i2c_adap->dev, "emitting repeated "
 				"start condition\n");
 			i2c_repstart(adap);
 			/* okay, now switch into reading mode */
 			addr |= 0x01;
 			ret = try_address(i2c_adap, addr, retries);
-			if ((ret!=1) && !nak_ok) {
+			if ((ret != 1) && !nak_ok) {
 				dev_err(&i2c_adap->dev,
 					"died at repeated address code\n");
 				return -EREMOTEIO;
 			}
 		}
 	} else {		/* normal 7bit address	*/
-		addr = ( msg->addr << 1 );
-		if (flags & I2C_M_RD )
+		addr = (msg->addr << 1);
+		if (flags & I2C_M_RD)
 			addr |= 1;
-		if (flags & I2C_M_REV_DIR_ADDR )
+		if (flags & I2C_M_REV_DIR_ADDR)
 			addr ^= 1;
 		ret = try_address(i2c_adap, addr, retries);
-		if ((ret!=1) && !nak_ok)
+		if ((ret != 1) && !nak_ok)
 			return -EREMOTEIO;
 	}
 
@@ -493,15 +518,14 @@ static int bit_xfer(struct i2c_adapter *
 {
 	struct i2c_msg *pmsg;
 	struct i2c_algo_bit_data *adap = i2c_adap->algo_data;
-	
-	int i,ret;
+	int i, ret;
 	unsigned short nak_ok;
 
 	bit_dbg(3, &i2c_adap->dev, "emitting start condition\n");
 	i2c_start(adap);
-	for (i=0;i<num;i++) {
+	for (i = 0; i < num; i++) {
 		pmsg = &msgs[i];
-		nak_ok = pmsg->flags & I2C_M_IGNORE_NAK; 
+		nak_ok = pmsg->flags & I2C_M_IGNORE_NAK;
 		if (!(pmsg->flags & I2C_M_NOSTART)) {
 			if (i) {
 				bit_dbg(3, &i2c_adap->dev, "emitting "
@@ -516,7 +540,7 @@ static int bit_xfer(struct i2c_adapter *
 				goto bailout;
 			}
 		}
-		if (pmsg->flags & I2C_M_RD ) {
+		if (pmsg->flags & I2C_M_RD) {
 			/* read bytes into buffer*/
 			ret = readbytes(i2c_adap, pmsg);
 			if (ret >= 1)
@@ -550,7 +574,7 @@ bailout:
 
 static u32 bit_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL | 
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL |
 	       I2C_FUNC_SMBUS_READ_BLOCK_DATA |
 	       I2C_FUNC_SMBUS_BLOCK_PROC_CALL |
 	       I2C_FUNC_10BIT_ADDR | I2C_FUNC_PROTOCOL_MANGLING;
@@ -564,8 +588,8 @@ static const struct i2c_algorithm i2c_bi
 	.functionality	= bit_func,
 };
 
-/* 
- * registering functions to load algorithms at runtime 
+/*
+ * registering functions to load algorithms at runtime
  */
 static int i2c_bit_prepare_bus(struct i2c_adapter *adap)
 {
@@ -573,7 +597,7 @@ static int i2c_bit_prepare_bus(struct i2
 
 	if (bit_test) {
 		int ret = test_bus(bit_adap, adap->name);
-		if (ret<0)
+		if (ret < 0)
 			return -ENODEV;
 	}
 




More information about the i2c mailing list