[PATCH 5/5] I2C: DaVinci: initialize cmd_complete sooner

[PATCH 5/5] 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 2ac56a2..f0b095f 100755
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -262,7 +262,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
@@ -518,6 +518,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 1/5] I2C: DaVinci: clock between 7-12 MHz

[Troy Kisky]

Ensure psc value gives a clock between 7-12 MHz
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 |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 7ecbfc4..0e5a39c 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -142,6 +142,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
 	u16 psc;
 	u32 clk;
+	u32 d;
 	u32 clkh;
 	u32 clkl;
 	u32 input_clock = clk_get_rate(dev->clk);
@@ -171,23 +172,29 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 	 *       if PSC > 1 , d = 5
 	 */
 
-	psc = 26; /* To get 1MHz clock */
+	/* get minimum of 7 MHz clock, but max of 12 MHz */
+	psc = (input_clock / 7000000) - 1;
+	if ((input_clock / (psc + 1)) > 12000000)
+		psc++;	/* better to run under spec than over */
+	d = (psc >= 2)? 5 : 7 - psc;
 
-	clk = ((input_clock / (psc + 1)) / (pdata->bus_freq * 1000)) - 10;
-	clkh = (50 * clk) / 100;
+	clk = ((input_clock / (psc + 1)) / (pdata->bus_freq * 1000)) - (d<<1);
+	clkh = clk>>1;
 	clkl = clk - clkh;
 
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_PSC_REG, psc);
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
 
-	dev_dbg(dev->dev, "CLK  = %d\n", clk);
+	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
 	dev_dbg(dev->dev, "PSC  = %d\n",
 		davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
 	dev_dbg(dev->dev, "CLKL = %d\n",
 		davinci_i2c_read_reg(dev, DAVINCI_I2C_CLKL_REG));
 	dev_dbg(dev->dev, "CLKH = %d\n",
 		davinci_i2c_read_reg(dev, DAVINCI_I2C_CLKH_REG));
+	dev_dbg(dev->dev, "bus_freq = %dkHz, bus_delay = %d\n",
+		pdata->bus_freq, pdata->bus_delay);
 
 	/* Take the I2C module out of reset: */
 	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-- 
1.5.4


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

[PATCH 2/5] 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 0e5a39c..318579b 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -345,12 +345,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 ret: %d\n", __func__, 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 3/5] I2C: DaVinci: remove useless IVR read

[Troy Kisky]

Interrupts are enabled at the point where
the DAVINCI_I2C_IVR_REG is read, so unless
an interrupt happened just at that moment,
no interrupt would be pending. Even though
documentation implies you should do this,
I see no reason. If slave support is added,
this read would cause a hard to reproduce bug.

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, 0 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 318579b..d9752ae 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -240,7 +240,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
 	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
 	u32 flag;
-	u32 stat;
 	u16 w;
 	int r;
 
@@ -264,9 +263,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	init_completion(&dev->cmd_complete);
 	dev->cmd_err = 0;
 
-	/* Clear any pending interrupts by reading the IVR */
-	stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG);
-
 	/* Take I2C out of reset, configure it as master and set the
 	 * start bit */
 	flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST | DAVINCI_I2C_MDR_STT;
-- 
1.5.4


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

[PATCH 4/5] 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 |   66 +++++++++++++++++++++++++++++++-------
 1 files changed, 54 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index d9752ae..6719cdb 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,31 @@ 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 = NULL;
 		return -ETIMEDOUT;
 	}
+	if (dev->buf_len) {
+		/* 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 = NULL;
+	}
+	if (r < 0)
+		return r;
 
 	/* no error */
 	if (likely(!dev->cmd_err))
@@ -353,6 +366,30 @@ static u32 i2c_davinci_func(struct i2c_adapter *adap)
 	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
 }
 
+static inline void terminate_read(struct davinci_i2c_dev *dev)
+{
+	if (dev->buf_len)
+		dev->buf_len--;
+	if (dev->buf_len) {
+		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 inline 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;
 
@@ -389,7 +429,7 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 			break;
 
 		case DAVINCI_I2C_IVR_RDR:
-			if (dev->buf_len) {
+			if (dev->buf && dev->buf_len) {
 				*dev->buf++ =
 				    davinci_i2c_read_reg(dev,
 							 DAVINCI_I2C_DRR_REG);
@@ -400,13 +440,14 @@ 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:
-			if (dev->buf_len) {
+			if (dev->buf && dev->buf_len) {
 				davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG,
 						      *dev->buf++);
 				dev->buf_len--;
@@ -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

[PATCH 5/5] 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 6719cdb..7d8f1b0 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -262,7 +262,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
@@ -518,6 +518,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

Re: [PATCH 1/5] I2C: DaVinci: clock between 7-12 MHz

[Jean Delvare]

On Fri, 25 Apr 2008 09:58:10 -0700, Troy Kisky wrote:
> Ensure psc value gives a clock between 7-12 MHz
> 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 |   15 +++++++++++----
>  1 files changed, 11 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 7ecbfc4..0e5a39c 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -142,6 +142,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>  	u16 psc;
>  	u32 clk;
> +	u32 d;
>  	u32 clkh;
>  	u32 clkl;
>  	u32 input_clock = clk_get_rate(dev->clk);
> @@ -171,23 +172,29 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  	 *       if PSC > 1 , d = 5
>  	 */
>  
> -	psc = 26; /* To get 1MHz clock */
> +	/* get minimum of 7 MHz clock, but max of 12 MHz */
> +	psc = (input_clock / 7000000) - 1;
> +	if ((input_clock / (psc + 1)) > 12000000)
> +		psc++;	/* better to run under spec than over */
> +	d = (psc >= 2)? 5 : 7 - psc;
>  
> -	clk = ((input_clock / (psc + 1)) / (pdata->bus_freq * 1000)) - 10;
> -	clkh = (50 * clk) / 100;
> +	clk = ((input_clock / (psc + 1)) / (pdata->bus_freq * 1000)) - (d<<1);
> +	clkh = clk>>1;
>  	clkl = clk - clkh;
>  
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_PSC_REG, psc);
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
>  
> -	dev_dbg(dev->dev, "CLK  = %d\n", clk);
> +	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
>  	dev_dbg(dev->dev, "PSC  = %d\n",
>  		davinci_i2c_read_reg(dev, DAVINCI_I2C_PSC_REG));
>  	dev_dbg(dev->dev, "CLKL = %d\n",
>  		davinci_i2c_read_reg(dev, DAVINCI_I2C_CLKL_REG));
>  	dev_dbg(dev->dev, "CLKH = %d\n",
>  		davinci_i2c_read_reg(dev, DAVINCI_I2C_CLKH_REG));
> +	dev_dbg(dev->dev, "bus_freq = %dkHz, bus_delay = %d\n",
> +		pdata->bus_freq, pdata->bus_delay);
>  
>  	/* Take the I2C module out of reset: */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);

Applied, with a few remaining coding-style cleanups, thanks.

-- 
Jean Delvare

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

Re: [PATCH 2/5] I2C: DaVinci: move dev_debug line for more output

[Jean Delvare]

Hi Troy,

On Fri, 25 Apr 2008 09:58:11 -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 0e5a39c..318579b 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -345,12 +345,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 ret: %d\n", __func__, ret);
>  		if (ret < 0)
>  			return ret;
>  	}

I would like to suggest an improvement. Right now, if a transaction has
several messages, you will print as many debug lines. If several
transactions happen in a row, all the lines in the logs will look
alike, and you won't be able to differentiate the various transactions.

I suggest the following:

		dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num, ret);

So you know how many messages there are in every transaction. What do
you think?

> -
> -	dev_dbg(dev->dev, "%s:%d ret: %d\n", __func__, __LINE__, ret);
> -
>  	return num;
>  }
>  


-- 
Jean Delvare

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

Re: [PATCH 3/5] I2C: DaVinci: remove useless IVR read

[Jean Delvare]

On Fri, 25 Apr 2008 09:58:12 -0700, Troy Kisky wrote:
> Interrupts are enabled at the point where
> the DAVINCI_I2C_IVR_REG is read, so unless
> an interrupt happened just at that moment,
> no interrupt would be pending. Even though
> documentation implies you should do this,
> I see no reason. If slave support is added,
> this read would cause a hard to reproduce bug.
> 
> 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, 0 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 318579b..d9752ae 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -240,7 +240,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	struct davinci_i2c_dev *dev = i2c_get_adapdata(adap);
>  	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
>  	u32 flag;
> -	u32 stat;
>  	u16 w;
>  	int r;
>  
> @@ -264,9 +263,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	init_completion(&dev->cmd_complete);
>  	dev->cmd_err = 0;
>  
> -	/* Clear any pending interrupts by reading the IVR */
> -	stat = davinci_i2c_read_reg(dev, DAVINCI_I2C_IVR_REG);
> -
>  	/* Take I2C out of reset, configure it as master and set the
>  	 * start bit */
>  	flag = DAVINCI_I2C_MDR_IRS | DAVINCI_I2C_MDR_MST | DAVINCI_I2C_MDR_STT;

Applied, thanks.

-- 
Jean Delvare

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

Re: [PATCH 2/5] I2C: DaVinci: move dev_debug line for more output

[Troy Kisky]

Jean Delvare wrote:
> Hi Troy,
> 
> On Fri, 25 Apr 2008 09:58:11 -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 0e5a39c..318579b 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -345,12 +345,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 ret: %d\n", __func__, ret);
>>  		if (ret < 0)
>>  			return ret;
>>  	}
> 
> I would like to suggest an improvement. Right now, if a transaction has
> several messages, you will print as many debug lines. If several
> transactions happen in a row, all the lines in the logs will look
> alike, and you won't be able to differentiate the various transactions.
> 
> I suggest the following:
> 
> 		dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num, ret);
> 
> So you know how many messages there are in every transaction. What do
> you think?
> 
>> -
>> -	dev_dbg(dev->dev, "%s:%d ret: %d\n", __func__, __LINE__, ret);
>> -
>>  	return num;
>>  }
>>  
> 
> 
Looks better to me.


Do you want to handle it, or me to send a patch?

Thanks

Troy

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

Re: [PATCH 2/5] I2C: DaVinci: move dev_debug line for more output

[Jean Delvare]

On Mon, 28 Apr 2008 09:20:33 -0700, Troy Kisky wrote:
> Jean Delvare wrote:
> > Hi Troy,
> > 
> > On Fri, 25 Apr 2008 09:58:11 -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 0e5a39c..318579b 100644
> >> --- a/drivers/i2c/busses/i2c-davinci.c
> >> +++ b/drivers/i2c/busses/i2c-davinci.c
> >> @@ -345,12 +345,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 ret: %d\n", __func__, ret);
> >>  		if (ret < 0)
> >>  			return ret;
> >>  	}
> > 
> > I would like to suggest an improvement. Right now, if a transaction has
> > several messages, you will print as many debug lines. If several
> > transactions happen in a row, all the lines in the logs will look
> > alike, and you won't be able to differentiate the various transactions.
> > 
> > I suggest the following:
> > 
> > 		dev_dbg(dev->dev, "%s [%d/%d] ret: %d\n", __func__, i + 1, num, ret);
> > 
> > So you know how many messages there are in every transaction. What do
> > you think?
> > 
> >> -
> >> -	dev_dbg(dev->dev, "%s:%d ret: %d\n", __func__, __LINE__, ret);
> >> -
> >>  	return num;
> >>  }
> >>  
> > 
> > 
> Looks better to me.
> 
> 
> Do you want to handle it, or me to send a patch?

Please send a patch, I can't even test-build i2c-davinci.

Thanks,
-- 
Jean Delvare

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

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

[Jean Delvare]

Hi Troy,

On Fri, 25 Apr 2008 09:58:13 -0700, Troy Kisky wrote:
> If wait_for_completion_interruptible_timeout exits due
> to a signal, the i2c bus was locking up.

What kind of signal (coming from where) are you talking about?

If you don't want to be interrupted, why don't you simply use
wait_for_completion_timeout() instead of
wait_for_completion_interruptible_timeout()?

> 
> 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 |   66 +++++++++++++++++++++++++++++++-------
>  1 files changed, 54 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index d9752ae..6719cdb 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,31 @@ 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 = NULL;
>  		return -ETIMEDOUT;
>  	}
> +	if (dev->buf_len) {
> +		/* 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 = NULL;
> +	}
> +	if (r < 0)
> +		return r;
>  
>  	/* no error */
>  	if (likely(!dev->cmd_err))
> @@ -353,6 +366,30 @@ static u32 i2c_davinci_func(struct i2c_adapter *adap)
>  	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
>  }
>  
> +static inline void terminate_read(struct davinci_i2c_dev *dev)
> +{
> +	if (dev->buf_len)
> +		dev->buf_len--;
> +	if (dev->buf_len) {

Please explain (in a comment) what you are doing here. Can't you just
test for (dev->buf_len > 1)?

> +		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 inline 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;

Coding-style: spaces around |.

> +	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");
> +}

I don't see the point of inlining these functions explicitly... They
are only called in an unlikely event so this is certainly not
performance-critical.

> +
>  /*
>   * 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;
>  
> @@ -389,7 +429,7 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  			break;
>  
>  		case DAVINCI_I2C_IVR_RDR:
> -			if (dev->buf_len) {
> +			if (dev->buf && dev->buf_len) {
>  				*dev->buf++ =
>  				    davinci_i2c_read_reg(dev,
>  							 DAVINCI_I2C_DRR_REG);
> @@ -400,13 +440,14 @@ 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:
> -			if (dev->buf_len) {
> +			if (dev->buf && dev->buf_len) {
>  				davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG,
>  						      *dev->buf++);
>  				dev->buf_len--;
> @@ -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:

Apparently you are using dev->buf_len = 0 and dev->buf = NULL to notify
particular conditions accross the driver? This should be clearly
documented, otherwise other developers are likely to break the driver
while working on it.

-- 
Jean Delvare

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

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

[Troy Kisky]

Jean Delvare wrote:
> Hi Troy,
> 
> On Fri, 25 Apr 2008 09:58:13 -0700, Troy Kisky wrote:
>> If wait_for_completion_interruptible_timeout exits due
>> to a signal, the i2c bus was locking up.
> 
> What kind of signal (coming from where) are you talking about?

With the user space i2c interface, a ^c was
locking the bus.

> 
> If you don't want to be interrupted, why don't you simply use
> wait_for_completion_timeout() instead of
> wait_for_completion_interruptible_timeout()?

I didn't make that choice. Perhaps wait_for_completion_timeout()
would be better. I just preferred fixing the lockup if a signal
happened. It seemed like a safer change to me. Can wait_for_completion_timeout()
return for any reason other the successful completion or timeout? Will
an explicit kill of the process return? Do you want it changed to
use wait_for_completion_timeout()?

>> +static inline void terminate_read(struct davinci_i2c_dev *dev)
>> +{
>> +	if (dev->buf_len)
>> +		dev->buf_len--;
>> +	if (dev->buf_len) {
> 
> Please explain (in a comment) what you are doing here. Can't you just
> test for (dev->buf_len > 1)?

Or maybe no test, just always set the nak bit and throw away the data??


> 
>> +		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 inline 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;
> 
> Coding-style: spaces around |.

OK
> 
>> +	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");
>> +}
> 
> I don't see the point of inlining these functions explicitly... They
> are only called in an unlikely event so this is certainly not
> performance-critical.

OK
>> @@ -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:
> 
> Apparently you are using dev->buf_len = 0 and dev->buf = NULL to notify
> particular conditions accross the driver? This should be clearly
> documented, otherwise other developers are likely to break the driver
> while working on it.
> 

Agreed.



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

Re: [PATCH 5/5] I2C: DaVinci: initialize cmd_complete sooner

[Jean Delvare]

Hi Troy,

On Fri, 25 Apr 2008 09:58:14 -0700, Troy Kisky wrote:
> If an interrupt happens before an I2c master read/write,
> complete is called on uninitialized structure.

I'm curious why an interrupt would happen if no transaction has been
started. And even then, it seems to me that the interrupt handler
should be fixed to simply do nothing if it doesn't have anything to do.
Nevertheless...

> 
> 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 6719cdb..7d8f1b0 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -262,7 +262,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
> @@ -518,6 +518,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);

... this patch is good to have from a performance point of view, so
I'll apply it.

-- 
Jean Delvare

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

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

[Jean Delvare]

On Mon, 28 Apr 2008 11:20:40 -0700, Troy Kisky wrote:
> Jean Delvare wrote:
> > Hi Troy,
> > 
> > On Fri, 25 Apr 2008 09:58:13 -0700, Troy Kisky wrote:
> >> If wait_for_completion_interruptible_timeout exits due
> >> to a signal, the i2c bus was locking up.
> > 
> > What kind of signal (coming from where) are you talking about?
> 
> With the user space i2c interface, a ^c was
> locking the bus.

Ah, OK. I admit I've never tested this on any i2c bus driver.

> > If you don't want to be interrupted, why don't you simply use
> > wait_for_completion_timeout() instead of
> > wait_for_completion_interruptible_timeout()?
> 
> I didn't make that choice. Perhaps wait_for_completion_timeout()
> would be better. I just preferred fixing the lockup if a signal
> happened. It seemed like a safer change to me. Can wait_for_completion_timeout()
> return for any reason other the successful completion or timeout?

I think not, but...

> Will an explicit kill of the process return?

I just don't know. I guess you'd have to try.

> Do you want it changed to use wait_for_completion_timeout()?

I'm suggesting this because it seems to be a much more simple way to
fix the problem. If that works for you, why do something more complex?

As a side note, I'm curious what happens if the call timeouts. Doesn't
the hardware lockup happen? From the hardware's perspective I can't see
any difference between the timeout case and the signal case.

> >> +static inline void terminate_read(struct davinci_i2c_dev *dev)
> >> +{
> >> +	if (dev->buf_len)
> >> +		dev->buf_len--;
> >> +	if (dev->buf_len) {
> > 
> > Please explain (in a comment) what you are doing here. Can't you just
> > test for (dev->buf_len > 1)?
> 
> Or maybe no test, just always set the nak bit and throw away the data??

You know the hardware, I don't, I just can't tell. My suggestion was
only based on logic.

> >> +		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);
> >> +	}

-- 
Jean Delvare

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

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

[Troy Kisky]

Jean Delvare wrote:
> On Mon, 28 Apr 2008 11:20:40 -0700, Troy Kisky wrote:
>> Jean Delvare wrote:
>>> Hi Troy,
>>>
>>> On Fri, 25 Apr 2008 09:58:13 -0700, Troy Kisky wrote:
>>>> If wait_for_completion_interruptible_timeout exits due
>>>> to a signal, the i2c bus was locking up.
>>> What kind of signal (coming from where) are you talking about?
>> With the user space i2c interface, a ^c was
>> locking the bus.
> 
> Ah, OK. I admit I've never tested this on any i2c bus driver.
> 
>>> If you don't want to be interrupted, why don't you simply use
>>> wait_for_completion_timeout() instead of
>>> wait_for_completion_interruptible_timeout()?
>> I didn't make that choice. Perhaps wait_for_completion_timeout()
>> would be better. I just preferred fixing the lockup if a signal
>> happened. It seemed like a safer change to me. Can wait_for_completion_timeout()
>> return for any reason other the successful completion or timeout?
> 
> I think not, but...
> 
>> Will an explicit kill of the process return?
> 
> I just don't know. I guess you'd have to try.
> 
>> Do you want it changed to use wait_for_completion_timeout()?
> 
> I'm suggesting this because it seems to be a much more simple way to
> fix the problem. If that works for you, why do something more complex?
> 

IMHO, if an i2c interrupt happens that says data is available to
read, that data should be read, regardless of whether or not we expected
data to be available. So, the ^c bug just nudged me to change it.
But my stance is not firm, let me know your preference.



> As a side note, I'm curious what happens if the call timeouts. Doesn't
> the hardware lockup happen? From the hardware's perspective I can't see
> any difference between the timeout case and the signal case.

The code checks explicitly for a timeout and resets the controller if found.
If you did this with a signal as well, I think the bus would be non-idle
until another I2C transfer is started by a reset controller(this one or another).

> 
>>>> +static inline void terminate_read(struct davinci_i2c_dev *dev)
>>>> +{
>>>> +	if (dev->buf_len)
>>>> +		dev->buf_len--;
>>>> +	if (dev->buf_len) {
>>> Please explain (in a comment) what you are doing here. Can't you just
>>> test for (dev->buf_len > 1)?
>> Or maybe no test, just always set the nak bit and throw away the data??
> 
> You know the hardware, I don't, I just can't tell. My suggestion was
> only based on logic.
> 
OK, I'll test it .

>>>> +		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);
>>>> +	}
> 

Thanks
Troy

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

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

[Jean Delvare]

Hi Troy,

On Mon, 28 Apr 2008 16:40:59 -0700, Troy Kisky wrote:
> Jean Delvare wrote:
> > On Mon, 28 Apr 2008 11:20:40 -0700, Troy Kisky wrote:
> >> Do you want it changed to use wait_for_completion_timeout()?
> > 
> > I'm suggesting this because it seems to be a much more simple way to
> > fix the problem. If that works for you, why do something more complex?
> 
> IMHO, if an i2c interrupt happens that says data is available to
> read, that data should be read, regardless of whether or not we expected
> data to be available. So, the ^c bug just nudged me to change it.
> But my stance is not firm, let me know your preference.

I have no preference. If you think that handling the signals the way
you first proposed is the way to go, that's fine with me. But then you
have to add comments to explain what you are doing, as suggested in my
original review.

-- 
Jean Delvare

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