Re: [PATCH] I2C-MPC: Fix up error handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On Apr 25, 2006, at 8:22 AM, Jean Delvare wrote:

Hi Kumar,

Is there a datasheet available for this chip?

* If we have an Unfinished (MCF) or Arbitration Lost (MAL) error and
  the bus is still busy reset the controller.  This prevents the
  controller from getting in a hung state for transactions for other
  devices.

What "other devices" are you talking about? If the _bus_ is busy, it
might be caused by any chip on the bus. Resetting the controller may or
may not help. But it's hard for me to say more without technical
documentation. Can you explain what the CSR_MBB bit means exactly?
Please also explain the scenario you are trying to address here.

Here's the definintion of CSR_MBB:

Bus busy. Indicates the status of the bus. When a START condition is detected, MBB is set. If a STOP condition
is detected, it is cleared.

What I meant is that I have a I2C slave device which is hanging up the bus on some transactions. In those cases we will end up in one of the error conditions CSR_MCF, or CSR_MAL. If I don't reset the controller all future transactions regardless of which device they are to fail.

* Fixed up propogating the errors from i2c_wait.

Yes, I like this.

--- a/drivers/i2c/busses/i2c-mpc.c
+++ b/drivers/i2c/busses/i2c-mpc.c
@@ -115,11 +115,20 @@ static int i2c_wait(struct mpc_i2c *i2c,

 	if (!(x & CSR_MCF)) {
 		pr_debug("I2C: unfinished\n");
+
+		/* reset the controller if the bus is still busy */
+		if (x & CSR_MBB)
+			writeccr(i2c, 0);
+
 		return -EIO;
 	}

 	if (x & CSR_MAL) {
 		pr_debug("I2C: MAL\n");
+
+		/* reset the controller if the bus is still busy */
+		if (x & CSR_MBB)
+			writeccr(i2c, 0);
 		return -EIO;
 	}


Please try being consistent with your blank lines.

@@ -246,8 +259,13 @@ static int mpc_xfer(struct i2c_adapter *
 			return -EINTR;
 		}
 		if (time_after(jiffies, orig_jiffies + HZ)) {
-			pr_debug("I2C: timeout\n");
-			return -EIO;
+			writeccr(i2c, 0);
+
+			/* try one more time before we error */
+			if (readb(i2c->base + MPC_I2C_SR) & CSR_MBB) {
+				pr_debug("I2C: timeout\n");
+				return -EIO;
+			}
 		}
 		schedule();
 	}
@@ -325,6 +343,7 @@ static int fsl_i2c_probe(struct platform
 			goto fail_irq;
 		}

+	writeccr(i2c, 0);
 	mpc_i2c_setclock(i2c);
 	platform_set_drvdata(pdev, i2c);

These last two changes are not mentioned in your header comment. What
are they? Why are they needed? They look like hacks to me.

Sorry about that, figured they fell in a catch all.

The first is an attempt to reduce the errors related to the buggy slave device.

The second (writeccr(i2c, 0)) is just ensure the controller is in a known state when we startup.

The

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux