[PATCH 1/3] I2C: DaVinci: move dev_debug line for more output

[PATCH 1/3] I2C: DaVinci: move dev_debug line for more output

[Troy Kisky]

Previously the dev_dbg only printed if no error.
Printing also on an error is more useful

Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |    4 +---
 1 files changed, 1 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 7a60afb..fd97cca 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -341,12 +341,10 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 	for (i = 0; i < num; i++) {
 		ret = i2c_davinci_xfer_msg(adap, &msgs[i], (i == (num - 1)));
+		dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num, ret);
 		if (ret < 0)
 			return ret;
 	}
-
-	dev_dbg(dev->dev, "%s:%d ret: %d\n", __func__, __LINE__, ret);
-
 	return num;
 }
 
-- 
1.5.4


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH 2/3] I2C: DaVinci: initialize cmd_complete sooner

[Troy Kisky]

If an interrupt happens before an I2c master read/write,
complete is called on uninitialized structure.

Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index fd97cca..77c9822 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -260,7 +260,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CNT_REG, dev->buf_len);
 
-	init_completion(&dev->cmd_complete);
+	INIT_COMPLETION(dev->cmd_complete);
 	dev->cmd_err = 0;
 
 	/* Take I2C out of reset, configure it as master and set the
@@ -476,6 +476,7 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 		goto err_release_region;
 	}
 
+	init_completion(&dev->cmd_complete);
 	dev->dev = get_device(&pdev->dev);
 	dev->irq = irq->start;
 	platform_set_drvdata(pdev, dev);
-- 
1.5.4


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

[PATCH 3/3] I2C: DaVinci: fix signal handling bug

[Troy Kisky]

If wait_for_completion_interruptible_timeout exits due
to a signal, the i2c bus was locking up.

Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   62 +++++++++++++++++++++++++++++++------
 1 files changed, 52 insertions(+), 10 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 77c9822..c44ef4a 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -85,6 +85,7 @@
 #define DAVINCI_I2C_MDR_MST	(1 << 10)
 #define DAVINCI_I2C_MDR_TRX	(1 << 9)
 #define DAVINCI_I2C_MDR_XA	(1 << 8)
+#define DAVINCI_I2C_MDR_RM	(1 << 7)
 #define DAVINCI_I2C_MDR_IRS	(1 << 5)
 
 #define DAVINCI_I2C_IMR_AAS	(1 << 6)
@@ -112,6 +113,7 @@ struct davinci_i2c_dev {
 	u8			*buf;
 	size_t			buf_len;
 	int			irq;
+	u8			terminate;
 	struct i2c_adapter	adapter;
 };
 
@@ -283,20 +285,34 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 		MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 1);
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
 
+	dev->terminate = 0;
 	/* write the data into mode register */
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
 
 	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
 						      DAVINCI_I2C_TIMEOUT);
-	dev->buf_len = 0;
-	if (r < 0)
-		return r;
-
 	if (r == 0) {
 		dev_err(dev->dev, "controller timed out\n");
 		i2c_davinci_init(dev);
+		dev->buf_len = 0;
 		return -ETIMEDOUT;
 	}
+	if (dev->buf_len) {
+		/* This should be 0 if all bytes were transferred
+		 * or dev->cmd_err denotes an error.
+		 * A signal may have aborted the transfer.
+		 */
+		if (r >= 0) {
+			dev_err(dev->dev, "abnormal termination buf_len=%i\n",
+				dev->buf_len);
+			r = -EREMOTEIO;
+		}
+		dev->terminate = 1;
+		wmb();
+		dev->buf_len = 0;
+	}
+	if (r < 0)
+		return r;
 
 	/* no error */
 	if (likely(!dev->cmd_err))
@@ -353,6 +369,27 @@ static u32 i2c_davinci_func(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
 }
 
+static void terminate_read(struct davinci_i2c_dev *dev)
+{
+	u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+	w |= DAVINCI_I2C_MDR_NACK;
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
+
+	/* Throw away data */
+	davinci_i2c_read_reg(dev, DAVINCI_I2C_DRR_REG);
+	if (!dev->terminate)
+		dev_err(dev->dev, "RDR IRQ while no data requested\n");
+}
+static void terminate_write(struct davinci_i2c_dev *dev)
+{
+	u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+	w |= DAVINCI_I2C_MDR_RM | DAVINCI_I2C_MDR_STP;
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
+
+	if (!dev->terminate)
+		dev_err(dev->dev, "TDR IRQ while no data to send\n");
+}
+
 /*
  * Interrupt service routine. This gets called whenever an I2C interrupt
  * occurs.
@@ -373,12 +410,15 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 
 		switch (stat) {
 		case DAVINCI_I2C_IVR_AL:
+			/* Arbitration lost, must retry */
 			dev->cmd_err |= DAVINCI_I2C_STR_AL;
+			dev->buf_len = 0;
 			complete(&dev->cmd_complete);
 			break;
 
 		case DAVINCI_I2C_IVR_NACK:
 			dev->cmd_err |= DAVINCI_I2C_STR_NACK;
+			dev->buf_len = 0;
 			complete(&dev->cmd_complete);
 			break;
 
@@ -400,9 +440,10 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 				davinci_i2c_write_reg(dev,
 					DAVINCI_I2C_STR_REG,
 					DAVINCI_I2C_IMR_RRDY);
-			} else
-				dev_err(dev->dev, "RDR IRQ while no "
-					"data requested\n");
+			} else {
+				/* signal can terminate transfer */
+				terminate_read(dev);
+			}
 			break;
 
 		case DAVINCI_I2C_IVR_XRDY:
@@ -419,9 +460,10 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 				davinci_i2c_write_reg(dev,
 						      DAVINCI_I2C_IMR_REG,
 						      w);
-			} else
-				dev_err(dev->dev, "TDR IRQ while no data to "
-					"send\n");
+			} else {
+				/* signal can terminate transfer */
+				terminate_write(dev);
+			}
 			break;
 
 		case DAVINCI_I2C_IVR_SCD:
-- 
1.5.4


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 3/3] I2C: DaVinci: fix signal handling bug

[Jean Delvare]

Hi Troy,

On Wed, 30 Apr 2008 16:53:58 -0700, Troy Kisky wrote:
> If wait_for_completion_interruptible_timeout exits due
> to a signal, the i2c bus was locking up.
> 
> Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   62 +++++++++++++++++++++++++++++++------
>  1 files changed, 52 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 77c9822..c44ef4a 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -85,6 +85,7 @@
>  #define DAVINCI_I2C_MDR_MST	(1 << 10)
>  #define DAVINCI_I2C_MDR_TRX	(1 << 9)
>  #define DAVINCI_I2C_MDR_XA	(1 << 8)
> +#define DAVINCI_I2C_MDR_RM	(1 << 7)
>  #define DAVINCI_I2C_MDR_IRS	(1 << 5)
>  
>  #define DAVINCI_I2C_IMR_AAS	(1 << 6)
> @@ -112,6 +113,7 @@ struct davinci_i2c_dev {
>  	u8			*buf;
>  	size_t			buf_len;
>  	int			irq;
> +	u8			terminate;
>  	struct i2c_adapter	adapter;
>  };
>  
> @@ -283,20 +285,34 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  		MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 1);
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>  
> +	dev->terminate = 0;
>  	/* write the data into mode register */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>  
>  	r = wait_for_completion_interruptible_timeout(&dev->cmd_complete,
>  						      DAVINCI_I2C_TIMEOUT);
> -	dev->buf_len = 0;
> -	if (r < 0)
> -		return r;
> -
>  	if (r == 0) {
>  		dev_err(dev->dev, "controller timed out\n");
>  		i2c_davinci_init(dev);
> +		dev->buf_len = 0;
>  		return -ETIMEDOUT;
>  	}
> +	if (dev->buf_len) {
> +		/* This should be 0 if all bytes were transferred
> +		 * or dev->cmd_err denotes an error.
> +		 * A signal may have aborted the transfer.
> +		 */
> +		if (r >= 0) {
> +			dev_err(dev->dev, "abnormal termination buf_len=%i\n",
> +				dev->buf_len);
> +			r = -EREMOTEIO;
> +		}
> +		dev->terminate = 1;
> +		wmb();
> +		dev->buf_len = 0;
> +	}
> +	if (r < 0)
> +		return r;
>  
>  	/* no error */
>  	if (likely(!dev->cmd_err))
> @@ -353,6 +369,27 @@ static u32 i2c_davinci_func(struct i2c_adapter *adap)
>  	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
>  }
>  
> +static void terminate_read(struct davinci_i2c_dev *dev)
> +{
> +	u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> +	w |= DAVINCI_I2C_MDR_NACK;
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> +
> +	/* Throw away data */
> +	davinci_i2c_read_reg(dev, DAVINCI_I2C_DRR_REG);
> +	if (!dev->terminate)
> +		dev_err(dev->dev, "RDR IRQ while no data requested\n");
> +}
> +static void terminate_write(struct davinci_i2c_dev *dev)
> +{
> +	u16 w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> +	w |= DAVINCI_I2C_MDR_RM | DAVINCI_I2C_MDR_STP;
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
> +
> +	if (!dev->terminate)
> +		dev_err(dev->dev, "TDR IRQ while no data to send\n");
> +}
> +
>  /*
>   * Interrupt service routine. This gets called whenever an I2C interrupt
>   * occurs.
> @@ -373,12 +410,15 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  
>  		switch (stat) {
>  		case DAVINCI_I2C_IVR_AL:
> +			/* Arbitration lost, must retry */
>  			dev->cmd_err |= DAVINCI_I2C_STR_AL;
> +			dev->buf_len = 0;
>  			complete(&dev->cmd_complete);
>  			break;
>  
>  		case DAVINCI_I2C_IVR_NACK:
>  			dev->cmd_err |= DAVINCI_I2C_STR_NACK;
> +			dev->buf_len = 0;
>  			complete(&dev->cmd_complete);
>  			break;
>  
> @@ -400,9 +440,10 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  				davinci_i2c_write_reg(dev,
>  					DAVINCI_I2C_STR_REG,
>  					DAVINCI_I2C_IMR_RRDY);
> -			} else
> -				dev_err(dev->dev, "RDR IRQ while no "
> -					"data requested\n");
> +			} else {
> +				/* signal can terminate transfer */
> +				terminate_read(dev);
> +			}
>  			break;
>  
>  		case DAVINCI_I2C_IVR_XRDY:
> @@ -419,9 +460,10 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  				davinci_i2c_write_reg(dev,
>  						      DAVINCI_I2C_IMR_REG,
>  						      w);
> -			} else
> -				dev_err(dev->dev, "TDR IRQ while no data to "
> -					"send\n");
> +			} else {
> +				/* signal can terminate transfer */
> +				terminate_write(dev);
> +			}
>  			break;
>  
>  		case DAVINCI_I2C_IVR_SCD:

Looks much better than your first attempt. I'm taking this in my i2c
tree, thanks.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 3/3] I2C: DaVinci: fix signal handling bug

[Troy Kisky]

Jean Delvare wrote:
> 
> Looks much better than your first attempt. I'm taking this in my i2c
> tree, thanks.
> 

Thanks for all your efforts in improving this driver.

Troy

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 3/3] I2C: DaVinci: fix signal handling bug

[Jean Delvare]

On Fri, 02 May 2008 13:41:43 -0700, Troy Kisky wrote:
> Jean Delvare wrote:
> > 
> > Looks much better than your first attempt. I'm taking this in my i2c
> > tree, thanks.
> > 
> 
> Thanks for all your efforts in improving this driver.

Well, thank _you_ for your patience in improving the patches to make
them better :)

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

Re: [PATCH 1/3] I2C: DaVinci: move dev_debug line for more output

[Jean Delvare]

On Wed, 30 Apr 2008 16:53:56 -0700, Troy Kisky wrote:
> Previously the dev_dbg only printed if no error.
> Printing also on an error is more useful
> 
> Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
> Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |    4 +---
>  1 files changed, 1 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 7a60afb..fd97cca 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -341,12 +341,10 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  	for (i = 0; i < num; i++) {
>  		ret = i2c_davinci_xfer_msg(adap, &msgs[i], (i == (num - 1)));
> +		dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num, ret);
>  		if (ret < 0)
>  			return ret;
>  	}
> -
> -	dev_dbg(dev->dev, "%s:%d ret: %d\n", __func__, __LINE__, ret);
> -
>  	return num;
>  }
>  

Applied (after folding the long line), thanks.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c