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

[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 82d4b7f..da54fff 100755
--- 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

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

[Jean Delvare]

On Thu, 20 Mar 2008 20:06:08 -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.

This is only a problem if the device can operate in master mode and
slave mode concurrently. Is it the case? But even then, as you can't
have master and slave activity going on at the same time on a given I2C
bus, it only matters if some DaVinci boards have more than one i2c
buses. Is it the case?

I have to admit that I am a bit reluctant to remove this register read
if the documentation says it should be there. But if nobody objects I
can take this patch for 2.6.26.

> 
> 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 82d4b7f..da54fff 100755
> --- 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;


-- 
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

[Troy Kisky]

Jean Delvare wrote:
> On Thu, 20 Mar 2008 20:06:08 -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.
> 
> This is only a problem if the device can operate in master mode and
> slave mode concurrently. Is it the case? But even then, as you can't
> have master and slave activity going on at the same time on a given I2C
> bus, it only matters if some DaVinci boards have more than one i2c
> buses. Is it the case?
> 

There is only one i2c bus AFAIK, but how that is relevant escapes me.
And I realize that the host controller can not be master while it is
addressed as slave, but that does not prohibit a program from trying to
be master. Just because the interrupt that lets me know I am addressed
as slave is removed (and ignored,) the state of the bus is not suddenly
idle again. The documentation says to make sure no interrupts are pending
before starting a transfer. It should says "Service all pending interrupts, so
that the bus is idle before starting a transfer." The wait_bus_not_busy
will satisfy this in most cases. The exception being if the process is swapped
out, giving time for someone else on the bus to become master.


My removing the IVR read will not magically make the bus idle again either. But
at least, the interrupt routine will have a chance to do something.

Troy



_______________________________________________
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, 28 Mar 2008 12:37:11 -0700, Troy Kisky wrote:
> Jean Delvare wrote:
> > On Thu, 20 Mar 2008 20:06:08 -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.
> > 
> > This is only a problem if the device can operate in master mode and
> > slave mode concurrently. Is it the case? But even then, as you can't
> > have master and slave activity going on at the same time on a given I2C
> > bus, it only matters if some DaVinci boards have more than one i2c
> > buses. Is it the case?
> > 
> 
> There is only one i2c bus AFAIK, but how that is relevant escapes me.
> And I realize that the host controller can not be master while it is
> addressed as slave, but that does not prohibit a program from trying to
> be master. Just because the interrupt that lets me know I am addressed
> as slave is removed (and ignored,) the state of the bus is not suddenly
> idle again. The documentation says to make sure no interrupts are pending
> before starting a transfer. It should says "Service all pending interrupts, so
> that the bus is idle before starting a transfer."

Even that is just best effort... You can be addressed as a slave
between the time you serviced all pending interrupts and the time you
start a transfer as a master. There's no way to solve this problem
though as far as I can see.

> The wait_bus_not_busy
> will satisfy this in most cases. The exception being if the process is swapped
> out, giving time for someone else on the bus to become master.

The process being swapped out only gives the race more time to happen,
but it can happen even without it.

> 
> 
> My removing the IVR read will not magically make the bus idle again either. But
> at least, the interrupt routine will have a chance to do something.

OK, fine with me. But then don't you want to read the IVR register at
driver load time, to clear up everything that may have happened before
the driver is loaded?

-- 
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

[Troy Kisky]

Jean Delvare wrote:
> On Fri, 28 Mar 2008 12:37:11 -0700, Troy Kisky wrote:
>> Jean Delvare wrote:
>>> On Thu, 20 Mar 2008 20:06:08 -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.
>>> This is only a problem if the device can operate in master mode and
>>> slave mode concurrently. Is it the case? But even then, as you can't
>>> have master and slave activity going on at the same time on a given I2C
>>> bus, it only matters if some DaVinci boards have more than one i2c
>>> buses. Is it the case?
>>>
>> There is only one i2c bus AFAIK, but how that is relevant escapes me.
>> And I realize that the host controller can not be master while it is
>> addressed as slave, but that does not prohibit a program from trying to
>> be master. Just because the interrupt that lets me know I am addressed
>> as slave is removed (and ignored,) the state of the bus is not suddenly
>> idle again. The documentation says to make sure no interrupts are pending
>> before starting a transfer. It should says "Service all pending interrupts, so
>> that the bus is idle before starting a transfer."
> 
> Even that is just best effort... You can be addressed as a slave
> between the time you serviced all pending interrupts and the time you
> start a transfer as a master. There's no way to solve this problem
> though as far as I can see.

I don't think it needs to be solved.
> 
>> The wait_bus_not_busy
>> will satisfy this in most cases. The exception being if the process is swapped
>> out, giving time for someone else on the bus to become master.
> 
> The process being swapped out only gives the race more time to happen,
> but it can happen even without it.

If interrupt were locked out, then the bus checked to still be idle before
starting the transfer, that should guarantee that no interrupts where pending
before the transfer start. But that seems like overkill to me and I can still
lose arbitration anyway.

> 
>>
>> My removing the IVR read will not magically make the bus idle again either. But
>> at least, the interrupt routine will have a chance to do something.
> 
> OK, fine with me. But then don't you want to read the IVR register at
> driver load time, to clear up everything that may have happened before
> the driver is loaded?
> 

The reset function clear this register.

Thanks
Troy

_______________________________________________
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