Skip to contentSkip to conversation

i2c: i2c-mv64xxx - Compilation

PATCH New Mark A. Greer 2 participants 6 messages Dec 7, 2005 – Dec 8, 2005

Subsystems (2)
I2C subsystem host drivers I2C MV64XXX Marvell and Allwinner driver
lore
Share & tools
lore

Mark A. Greer

author
#0 lore raw

PATCH 1/1 i2c: i2c-mv64xxx - Compilation

The busses/i2c-mv64xxx.c driver doesn't currently compile because of an incorrect argument to dev_err(). This patch fixes that.

[Since it currently doesn't compile, I'm hoping this can get in ASAP.]

Signed-off-by: Mark A. Greer <mgreer at mvista.com> --

drivers/i2c/busses/i2c-mv64xxx.c +6 -5
diff -Nur linux-2.6.15-rc5-mm1.orig/drivers/i2c/busses/i2c-mv64xxx.c linux-2.6.15-rc5-mm1-p1/drivers/i2c/busses/i2c-mv64xxx.c
@@ -529,14 +529,15 @@
 	i2c_set_adapdata(&drv_data->adapter, drv_data);
 
 	if (request_irq(drv_data->irq, mv64xxx_i2c_intr, 0,
-		MV64XXX_I2C_CTLR_NAME, drv_data)) {
-
-		dev_err(dev, "mv64xxx: Can't register intr handler "
-			"irq: %d\n", drv_data->irq);
+			MV64XXX_I2C_CTLR_NAME, drv_data)) {
+		dev_err(&drv_data->adapter.dev,
+			"mv64xxx: Can't register intr handler irq: %d\n",
+			drv_data->irq);
 		rc = -EINVAL;
 		goto exit_unmap_regs;
 	} else if ((rc = i2c_add_adapter(&drv_data->adapter)) != 0) {
-		dev_err(dev, "mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
+		dev_err(&drv_data->adapter.dev,
+			"mv64xxx: Can't add i2c adapter, rc: %d\n", -rc);
 		goto exit_free_irq;
 	}
author
#1 lore raw

PATCH 1/1 i2c: i2c-mv64xxx - Abort may

When the i2c-mv64xxx i2c driver is signalled to abort a transaction, it aborts it immediately by issuing a stop condition on the bus. This violates the i2c protocol and can cause what appears to be an i2c bus hang. This patch delays issuing the stop condition until the i2c device can reasonably expect a stop condition.

Also includes a minor fixup.

Signed-off-by: Mark A. Greer <mgreer at mvista.com> --

drivers/i2c/busses/i2c-mv64xxx.c +15 -18
diff -Nur linux-2.6.15-rc5-mm1-p1/drivers/i2c/busses/i2c-mv64xxx.c linux-2.6.15-rc5-mm1-p2/drivers/i2c/busses/i2c-mv64xxx.c
@@ -1,6 +1,4 @@
 /*
- * drivers/i2c/busses/i2c-mv64xxx.c
- * 
  * Driver for the i2c controller on the Marvell line of host bridges for MIPS
  * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
  *

@@ -65,7 +63,6 @@
 	MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
 	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
 	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
-	MV64XXX_I2C_STATE_ABORTING,
 };
 
 /* Driver actions */

@@ -85,6 +82,7 @@
 	int			irq;
 	u32			state;
 	u32			action;
+	u32			aborting;
 	u32			cntl_bits;
 	void __iomem		*reg_base;
 	u32			reg_base_p;

@@ -122,12 +120,6 @@
 		return;
 	}
 
-	if (drv_data->state = MV64XXX_I2C_STATE_ABORTING) {
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
-		drv_data->state = MV64XXX_I2C_STATE_IDLE;
-		return;
-	}
-
 	/* The status from the ctlr [mostly] tells us what to do next */
 	switch (status) {
 	/* Start condition interrupt */

@@ -148,14 +140,16 @@
 		/* FALLTHRU */
 	case MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK: /* 0xd0 */
 	case MV64XXX_I2C_STATUS_MAST_WR_ACK: /* 0x28 */
-		if (drv_data->bytes_left > 0) {
+		if ((drv_data->bytes_left = 0)
+				|| (drv_data->aborting
+					&& (drv_data->byte_posn != 0))) {
+			drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
+			drv_data->state = MV64XXX_I2C_STATE_IDLE;
+		} else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA;
 			drv_data->state  				MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK;
 			drv_data->bytes_left--;
-		} else {
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
-			drv_data->state = MV64XXX_I2C_STATE_IDLE;
 		}
 		break;
 

@@ -184,7 +178,7 @@
 		}
 		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA;
 
-		if (drv_data->bytes_left = 1)
+		if ((drv_data->bytes_left = 1) || drv_data->aborting)
 			drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_ACK;
 		break;
 

@@ -320,6 +314,7 @@
 	drv_data->msg = msg;
 	drv_data->byte_posn = 0;
 	drv_data->bytes_left = msg->len;
+	drv_data->aborting = 0;
 	drv_data->rc = 0;
 	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
 		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;

@@ -359,17 +354,19 @@
 	}
 
 	if (abort && drv_data->block) {
-		drv_data->state = MV64XXX_I2C_STATE_ABORTING;
+		drv_data->aborting = 1;
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 
 		time_left = wait_event_timeout(drv_data->waitq,
 			!drv_data->block,
 			msecs_to_jiffies(drv_data->adapter.timeout));
 
-		if (time_left <= 0) {
+		if ((time_left <= 0) && drv_data->block) {
 			drv_data->state = MV64XXX_I2C_STATE_IDLE;
 			dev_err(&drv_data->adapter.dev,
-				"mv64xxx: I2C bus locked\n");
+				"mv64xxx: I2C bus locked, block: %d,"
+				" time_left: %d, before: %d, after: %d\n",
+				drv_data->block, time_left);
 		}
 	} else
 		spin_unlock_irqrestore(&drv_data->lock, flags);

@@ -510,7 +507,7 @@
 		goto exit_kfree;
 	}
 
-	strncpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
+	strlcpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		I2C_NAME_SIZE);
 
 	init_waitqueue_head(&drv_data->waitq);
author
#2 lore raw

PATCH 1/1 i2c: i2c-mv64xxx -

In reply to Mark A. Greer
Quoted 7 lines · Mark A. Greer

The busses/i2c-mv64xxx.c driver doesn't currently compile because of an incorrect argument to dev_err(). This patch fixes that.

[Since it currently doesn't compile, I'm hoping this can get in ASAP.]

Signed-off-by: Mark A. Greer <mgreer at mvista.com> --

Oops, I meant to thank Jeremy Nadke <jenadke at wisc.edu> for pointing this out.

Mark

#3 lore raw

PATCH 1/1 i2c: i2c-mv64xxx - Abort

In reply to Mark A. Greer

Hi Mark,

Review

Quoted 5 lines
(quoted diff) +3 -1
 			dev_err(&drv_data->adapter.dev,
-				"mv64xxx: I2C bus locked\n");
+				"mv64xxx: I2C bus locked, block: %d,"
+				" time_left: %d, before: %d, after: %d\n",
+				drv_data->block, time_left);

Please put the middle space at the end of the first half of the string rather than the beginning of the second half. Following this arbitrary rule makes it easier to spot split strings missing their middle space(s).

The parameter count for dev_err() is also not correct, the string has four %d and you provide only two values. I'd be surprised it even compiles.

Signature · 1 line
Jean Delvare
author
#4 lore raw

PATCH 1/1 i2c: i2c-mv64xxx - Abort

In reply to Mark A. Greer
Quoted 13 lines · Jean Delvare

Hi Mark,

Review

Quoted 5 lines
(quoted diff) +3 -1
 			dev_err(&drv_data->adapter.dev,
-				"mv64xxx: I2C bus locked\n");
+				"mv64xxx: I2C bus locked, block: %d,"
+				" time_left: %d, before: %d, after: %d\n",
+				drv_data->block, time_left);

Please put the middle space at the end of the first half of the string rather than the beginning of the second half. Following this arbitrary rule makes it easier to spot split strings missing their middle space(s).

Quoted 3 lines

The parameter count for dev_err() is also not correct, the string has four %d and you provide only two values. I'd be surprised it even compiles.

Hrm, I guess I was in a little too much of a rush yesterday. Sorry Jean.

Below is the respin.

Mark
--

When the i2c-mv64xxx i2c driver is signalled to abort a transaction, it aborts it immediately by issuing a stop condition on the bus. This violates the i2c protocol and can cause what appears to be an i2c bus hang. This patch delays issuing the stop condition until the i2c device can reasonably expect a stop condition.

Also includes a minor fixup.

Signed-off-by: Mark A. Greer <mgreer at mvista.com> --

drivers/i2c/busses/i2c-mv64xxx.c +15 -18
diff -Nur linux-2.6.15-rc5-mm1-p1/drivers/i2c/busses/i2c-mv64xxx.c linux-2.6.15-rc5-mm1-p2/drivers/i2c/busses/i2c-mv64xxx.c
@@ -1,6 +1,4 @@
 /*
- * drivers/i2c/busses/i2c-mv64xxx.c
- * 
  * Driver for the i2c controller on the Marvell line of host bridges for MIPS
  * and PPC (e.g, gt642[46]0, mv643[46]0, mv644[46]0).
  *

@@ -65,7 +63,6 @@
 	MV64XXX_I2C_STATE_WAITING_FOR_ADDR_2_ACK,
 	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK,
 	MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA,
-	MV64XXX_I2C_STATE_ABORTING,
 };
 
 /* Driver actions */

@@ -85,6 +82,7 @@
 	int			irq;
 	u32			state;
 	u32			action;
+	u32			aborting;
 	u32			cntl_bits;
 	void __iomem		*reg_base;
 	u32			reg_base_p;

@@ -122,12 +120,6 @@
 		return;
 	}
 
-	if (drv_data->state = MV64XXX_I2C_STATE_ABORTING) {
-		drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
-		drv_data->state = MV64XXX_I2C_STATE_IDLE;
-		return;
-	}
-
 	/* The status from the ctlr [mostly] tells us what to do next */
 	switch (status) {
 	/* Start condition interrupt */

@@ -148,14 +140,16 @@
 		/* FALLTHRU */
 	case MV64XXX_I2C_STATUS_MAST_WR_ADDR_2_ACK: /* 0xd0 */
 	case MV64XXX_I2C_STATUS_MAST_WR_ACK: /* 0x28 */
-		if (drv_data->bytes_left > 0) {
+		if ((drv_data->bytes_left = 0)
+				|| (drv_data->aborting
+					&& (drv_data->byte_posn != 0))) {
+			drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
+			drv_data->state = MV64XXX_I2C_STATE_IDLE;
+		} else {
 			drv_data->action = MV64XXX_I2C_ACTION_SEND_DATA;
 			drv_data->state  				MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_ACK;
 			drv_data->bytes_left--;
-		} else {
-			drv_data->action = MV64XXX_I2C_ACTION_SEND_STOP;
-			drv_data->state = MV64XXX_I2C_STATE_IDLE;
 		}
 		break;
 

@@ -184,7 +178,7 @@
 		}
 		drv_data->state = MV64XXX_I2C_STATE_WAITING_FOR_SLAVE_DATA;
 
-		if (drv_data->bytes_left = 1)
+		if ((drv_data->bytes_left = 1) || drv_data->aborting)
 			drv_data->cntl_bits &= ~MV64XXX_I2C_REG_CONTROL_ACK;
 		break;
 

@@ -320,6 +314,7 @@
 	drv_data->msg = msg;
 	drv_data->byte_posn = 0;
 	drv_data->bytes_left = msg->len;
+	drv_data->aborting = 0;
 	drv_data->rc = 0;
 	drv_data->cntl_bits = MV64XXX_I2C_REG_CONTROL_ACK |
 		MV64XXX_I2C_REG_CONTROL_INTEN | MV64XXX_I2C_REG_CONTROL_TWSIEN;

@@ -359,17 +354,19 @@
 	}
 
 	if (abort && drv_data->block) {
-		drv_data->state = MV64XXX_I2C_STATE_ABORTING;
+		drv_data->aborting = 1;
 		spin_unlock_irqrestore(&drv_data->lock, flags);
 
 		time_left = wait_event_timeout(drv_data->waitq,
 			!drv_data->block,
 			msecs_to_jiffies(drv_data->adapter.timeout));
 
-		if (time_left <= 0) {
+		if ((time_left <= 0) && drv_data->block) {
 			drv_data->state = MV64XXX_I2C_STATE_IDLE;
 			dev_err(&drv_data->adapter.dev,
-				"mv64xxx: I2C bus locked\n");
+				"mv64xxx: I2C bus locked, block: %d, "
+				"time_left: %d\n", drv_data->block,
+				(int)time_left);
 		}
 	} else
 		spin_unlock_irqrestore(&drv_data->lock, flags);

@@ -510,7 +507,7 @@
 		goto exit_kfree;
 	}
 
-	strncpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
+	strlcpy(drv_data->adapter.name, MV64XXX_I2C_CTLR_NAME " adapter",
 		I2C_NAME_SIZE);
 
 	init_waitqueue_head(&drv_data->waitq);
Applied
#5 lore raw

PATCH 1/1 i2c: i2c-mv64xxx - Abort

In reply to Mark A. Greer

Hi Mark,

Quoted 4 lines

Hrm, I guess I was in a little too much of a rush yesterday. Sorry Jean.

Below is the respin.

Applied, thanks.

Signature · 1 line
Jean Delvare