Patchwork [5/7] i2c-designware: enable/disable the controller properly

login
register
mail settings
Submitter Mika Westerberg
Date March 21, 2013, 12:09 p.m.
Message ID <1363867800-23861-5-git-send-email-mika.westerberg@linux.intel.com>
Download mbox | patch
Permalink /patch/229645/
State Superseded
Headers show

Comments

Mika Westerberg - March 21, 2013, 12:09 p.m.
The correct way to disable or enable the controller is to wait until
DW_IC_ENABLE_STATUS register bit matches the bit we program to the
DW_IC_ENABLE register. This procedure is described in the DesignWare I2C
handbook.

By doing this we can be sure that the controller is in correct state once
the function returns.

Signed-off-by: Mika Westerberg <mika.westerberg@linux.intel.com>
---
 drivers/i2c/busses/i2c-designware-core.c |   29 +++++++++++++++++++++++------
 1 file changed, 23 insertions(+), 6 deletions(-)
Wolfram Sang - April 9, 2013, 9:09 a.m.
> +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> +{
> +	int timeout = 100;
> +
> +	do {
> +		dw_writel(dev, enable, DW_IC_ENABLE);
> +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> +			return;
> +
> +		usleep_range(25, 250);

This would wait 25ms max. Is there a timeout value specified in the docs?

> +	} while (timeout-- > 0);

while (timeout--)?

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg - April 9, 2013, 9:28 a.m.
On Tue, Apr 09, 2013 at 11:09:14AM +0200, Wolfram Sang wrote:
> 
> > +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> > +{
> > +	int timeout = 100;
> > +
> > +	do {
> > +		dw_writel(dev, enable, DW_IC_ENABLE);
> > +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > +			return;
> > +
> > +		usleep_range(25, 250);
> 
> This would wait 25ms max. Is there a timeout value specified in the docs?

The datasheet says something like:

	1. Define a timer interval (t_i2c_poll) equal 10 times the highest
	signaling period. For 400kHz this is 25us.

	2. Define max timeout parameter, MAX_T_POLL_COUNT, such that if any
	repeated operation exeeds this maximum, an error is reported.

In this case I have:

	t_i2c_poll = 25 (to 250 us)
	MAX_T_POLL_COUNT = 100

> > +	} while (timeout-- > 0);
> 
> while (timeout--)?

OK, thanks.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Wolfram Sang - April 9, 2013, 9:28 a.m.
On Tue, Apr 09, 2013 at 12:28:36PM +0300, Mika Westerberg wrote:
> On Tue, Apr 09, 2013 at 11:09:14AM +0200, Wolfram Sang wrote:
> > 
> > > +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> > > +{
> > > +	int timeout = 100;
> > > +
> > > +	do {
> > > +		dw_writel(dev, enable, DW_IC_ENABLE);
> > > +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > > +			return;
> > > +
> > > +		usleep_range(25, 250);
> > 
> > This would wait 25ms max. Is there a timeout value specified in the docs?
> 
> The datasheet says something like:
> 
> 	1. Define a timer interval (t_i2c_poll) equal 10 times the highest
> 	signaling period. For 400kHz this is 25us.
> 
> 	2. Define max timeout parameter, MAX_T_POLL_COUNT, such that if any
> 	repeated operation exeeds this maximum, an error is reported.
> 
> In this case I have:
> 
> 	t_i2c_poll = 25 (to 250 us)
> 	MAX_T_POLL_COUNT = 100

Maybe worth a comment?

Other than that, the series looks fine to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Mika Westerberg - April 9, 2013, 9:39 a.m.
On Tue, Apr 09, 2013 at 11:28:57AM +0200, Wolfram Sang wrote:
> On Tue, Apr 09, 2013 at 12:28:36PM +0300, Mika Westerberg wrote:
> > On Tue, Apr 09, 2013 at 11:09:14AM +0200, Wolfram Sang wrote:
> > > 
> > > > +static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
> > > > +{
> > > > +	int timeout = 100;
> > > > +
> > > > +	do {
> > > > +		dw_writel(dev, enable, DW_IC_ENABLE);
> > > > +		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
> > > > +			return;
> > > > +
> > > > +		usleep_range(25, 250);
> > > 
> > > This would wait 25ms max. Is there a timeout value specified in the docs?
> > 
> > The datasheet says something like:
> > 
> > 	1. Define a timer interval (t_i2c_poll) equal 10 times the highest
> > 	signaling period. For 400kHz this is 25us.
> > 
> > 	2. Define max timeout parameter, MAX_T_POLL_COUNT, such that if any
> > 	repeated operation exeeds this maximum, an error is reported.
> > 
> > In this case I have:
> > 
> > 	t_i2c_poll = 25 (to 250 us)
> > 	MAX_T_POLL_COUNT = 100
> 
> Maybe worth a comment?

I'll add it in the next revision.

> Other than that, the series looks fine to me.

Thanks!
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..0e4f531 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -68,6 +68,7 @@ 
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
 #define DW_IC_TX_ABRT_SOURCE	0x80
+#define DW_IC_ENABLE_STATUS	0x9c
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_TYPE		0xfc
 #define DW_IC_COMP_TYPE_VALUE	0x44570140
@@ -248,6 +249,22 @@  static u32 i2c_dw_scl_lcnt(u32 ic_clk, u32 tLOW, u32 tf, int offset)
 	return ((ic_clk * (tLOW + tf) + 5000) / 10000) - 1 + offset;
 }
 
+static void __i2c_dw_enable(struct dw_i2c_dev *dev, bool enable)
+{
+	int timeout = 100;
+
+	do {
+		dw_writel(dev, enable, DW_IC_ENABLE);
+		if ((dw_readl(dev, DW_IC_ENABLE_STATUS) & 1) == enable)
+			return;
+
+		usleep_range(25, 250);
+	} while (timeout-- > 0);
+
+	dev_warn(dev->dev, "timeout in %sabling adapter\n",
+		 enable ? "en" : "dis");
+}
+
 /**
  * i2c_dw_init() - initialize the designware i2c master hardware
  * @dev: device private data
@@ -278,7 +295,7 @@  int i2c_dw_init(struct dw_i2c_dev *dev)
 	}
 
 	/* Disable the adapter */
-	dw_writel(dev, 0, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, false);
 
 	/* set standard and fast speed deviders for high/low periods */
 
@@ -345,7 +362,7 @@  static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	u32 ic_con;
 
 	/* Disable the adapter */
-	dw_writel(dev, 0, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, false);
 
 	/* set the slave (target) address */
 	dw_writel(dev, msgs[dev->msg_write_idx].addr, DW_IC_TAR);
@@ -359,7 +376,7 @@  static void i2c_dw_xfer_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, ic_con, DW_IC_CON);
 
 	/* Enable the adapter */
-	dw_writel(dev, 1, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, true);
 
 	/* Enable interrupts */
 	dw_writel(dev, DW_IC_INTR_DEFAULT_MASK, DW_IC_INTR_MASK);
@@ -565,7 +582,7 @@  i2c_dw_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 	/* no error */
 	if (likely(!dev->cmd_err)) {
 		/* Disable the adapter */
-		dw_writel(dev, 0, DW_IC_ENABLE);
+		__i2c_dw_enable(dev, false);
 		ret = num;
 		goto done;
 	}
@@ -700,7 +717,7 @@  EXPORT_SYMBOL_GPL(i2c_dw_isr);
 void i2c_dw_enable(struct dw_i2c_dev *dev)
 {
        /* Enable the adapter */
-	dw_writel(dev, 1, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, true);
 }
 EXPORT_SYMBOL_GPL(i2c_dw_enable);
 
@@ -713,7 +730,7 @@  EXPORT_SYMBOL_GPL(i2c_dw_is_enabled);
 void i2c_dw_disable(struct dw_i2c_dev *dev)
 {
 	/* Disable controller */
-	dw_writel(dev, 0, DW_IC_ENABLE);
+	__i2c_dw_enable(dev, false);
 
 	/* Disable all interupts */
 	dw_writel(dev, 0, DW_IC_INTR_MASK);