[PATCH 0/5]I2C: Davinci host controller changes

[PATCH 0/5]I2C: Davinci host controller changes

[Troy Kisky]

This is an updated version of the patch you kindly reviewed
earlier. Sorry it has take so long to resubmit..

Thanks
Troy

troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org



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

[PATCH 1/5] I2C: DaVinci: fix lost interrupt

[Troy Kisky]

DAVINCI_I2C_STR_REG is a write 1 to clear register,
so don't use a read/modify/write cycle.

Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
Acked-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   17 ++++++-----------
 1 files changed, 6 insertions(+), 11 deletions(-)
 mode change 100644 => 100755 drivers/i2c/busses/i2c-davinci.c

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
old mode 100644
new mode 100755
index cce5a61..fde2634
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -382,9 +382,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 			break;
 
 		case DAVINCI_I2C_IVR_ARDY:
-			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG);
-			MOD_REG_BIT(w, DAVINCI_I2C_STR_ARDY, 1);
-			davinci_i2c_write_reg(dev, DAVINCI_I2C_STR_REG, w);
+			davinci_i2c_write_reg(dev,
+				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ARDY);
 			complete(&dev->cmd_complete);
 			break;
 
@@ -397,12 +396,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 				if (dev->buf_len)
 					continue;
 
-				w = davinci_i2c_read_reg(dev,
-							 DAVINCI_I2C_STR_REG);
-				MOD_REG_BIT(w, DAVINCI_I2C_IMR_RRDY, 0);
 				davinci_i2c_write_reg(dev,
-						      DAVINCI_I2C_STR_REG,
-						      w);
+					DAVINCI_I2C_STR_REG,
+					DAVINCI_I2C_IMR_RRDY);
 			} else
 				dev_err(dev->dev, "RDR IRQ while no "
 					"data requested\n");
@@ -428,9 +424,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 			break;
 
 		case DAVINCI_I2C_IVR_SCD:
-			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG);
-			MOD_REG_BIT(w, DAVINCI_I2C_STR_SCD, 1);
-			davinci_i2c_write_reg(dev, DAVINCI_I2C_STR_REG, w);
+			davinci_i2c_write_reg(dev,
+				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_SCD);
 			complete(&dev->cmd_complete);
 			break;
 
-- 
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: 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 |   19 ++++++++++++-------
 1 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index fde2634..82d4b7f 100755
--- 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 7mHz clock, but max of 12mHz */
+	psc = (input_clock/7000000)-1;
+	if ((input_clock/(psc+1)) > 12000000)
+		psc++;
+	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);
@@ -338,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", __FUNCTION__, __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 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

[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 da54fff..2ac56a2 100755
--- 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 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

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

Re: [PATCH 2/5] I2C: DaVinci: clock between 7-12 Mhz

[Jean Delvare]

Hi Troy,

On Thu, 20 Mar 2008 20:06:07 -0700, Troy Kisky wrote:
> Ensure psc value gives a clock between 7-12 Mhz

Correct spelling is 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 |   19 ++++++++++++-------
>  1 files changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index fde2634..82d4b7f 100755
> --- 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 7mHz clock, but max of 12mHz */

Correct spelling is MHz.

> +	psc = (input_clock/7000000)-1;

The usual coding style is to leave spaces around all operators. Same
problem below.

Note: if input_clock < 7000000, you're in trouble.

> +	if ((input_clock/(psc+1)) > 12000000)
> +		psc++;

The only case where this will happen as far as I can see is if
input_clock is between 12 and 14 MHz. In this case, you'll get a clock
lower than 7 MHz (worst case is 6 MHz). Is this OK?

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

As I already wrote in my previous review: please come up with
consistent spacing in this debugging message. Having no space around
the first "=", but two spaced before and one after the second "=",
doesn't look good.

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

For consistency, you should add a comma after kHz.

> +		pdata->bus_freq, pdata->bus_delay);
>  
>  	/* Take the I2C module out of reset: */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> @@ -338,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", __FUNCTION__, __LINE__, ret);
> -
>  	return num;
>  }
>  

Not sure how this last chunk is supposed to belong to this patch?

-- 
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: clock between 7-12 Mhz

[Troy Kisky]

Jean Delvare wrote:
> Hi Troy,
> 
> On Thu, 20 Mar 2008 20:06:07 -0700, Troy Kisky wrote:
>> Ensure psc value gives a clock between 7-12 Mhz
> 
> Correct spelling is 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 |   19 ++++++++++++-------
>>  1 files changed, 12 insertions(+), 7 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index fde2634..82d4b7f 100755
>> --- 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 7mHz clock, but max of 12mHz */
> 
> Correct spelling is MHz.


Hmm... I seems weird that kHz and MHz are correct, as opposed to
kHz and mHz,  or KHz and MHz.

> 
>> +	psc = (input_clock/7000000)-1;
> 
> The usual coding style is to leave spaces around all operators. Same
> problem below.
> 
> Note: if input_clock < 7000000, you're in trouble.
> 
>> +	if ((input_clock/(psc+1)) > 12000000)
>> +		psc++;
> 
> The only case where this will happen as far as I can see is if
> input_clock is between 12 and 14 MHz. In this case, you'll get a clock
> lower than 7 MHz (worst case is 6 MHz). Is this OK?

Since it was running out of spec at 1 Mhz, I figured running under
spec is better 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);
> 
> As I already wrote in my previous review: please come up with
> consistent spacing in this debugging message. Having no space around
> the first "=", but two spaced before and one after the second "=",
> doesn't look good.

Agreed..
But the previous message was
> > +	dev_dbg(dev->dev, "input_clock=%d, CLK  = %d\n", input_clock,clk);
>
> Please come up with consistent spacing in this debugging message.
> Missing space after comma.
And I interpreted you incorrectly. I didn't ignore you.


> 
>>  	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",
> 
> For consistency, you should add a comma after kHz.
> 
>> +		pdata->bus_freq, pdata->bus_delay);
>>  
>>  	/* Take the I2C module out of reset: */
>>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> @@ -338,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", __FUNCTION__, __LINE__, ret);
>> -
>>  	return num;
>>  }
>>  
> 
> Not sure how this last chunk is supposed to belong to this patch?
> 
Do you want a separate patch? Or just a comment that a dev_dbg was moved so that
it prints always instead of just on no error?


Troy


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

Re: [PATCH 2/5] I2C: DaVinci: clock between 7-12 Mhz

[Jean Delvare]

On Fri, 28 Mar 2008 13:04:10 -0700, Troy Kisky wrote:
> Jean Delvare wrote:
> > Hi Troy,
> > 
> > On Thu, 20 Mar 2008 20:06:07 -0700, Troy Kisky wrote:
> >> Ensure psc value gives a clock between 7-12 Mhz
> > 
> > Correct spelling is 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 |   19 ++++++++++++-------
> >>  1 files changed, 12 insertions(+), 7 deletions(-)
> >>
> >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> >> index fde2634..82d4b7f 100755
> >> --- 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 7mHz clock, but max of 12mHz */
> > 
> > Correct spelling is MHz.
> 
> 
> Hmm... I seems weird that kHz and MHz are correct, as opposed to
> kHz and mHz,  or KHz and MHz.

These are the standard international unit prefixes. Please see:
http://en.wikipedia.org/wiki/SI_prefix#List_of_SI_prefixes
kilo- is "k", mega- is "M". "m" would be milli-, which isn't what you
want here.

> 
> > 
> >> +	psc = (input_clock/7000000)-1;
> > 
> > The usual coding style is to leave spaces around all operators. Same
> > problem below.
> > 
> > Note: if input_clock < 7000000, you're in trouble.
> > 
> >> +	if ((input_clock/(psc+1)) > 12000000)
> >> +		psc++;
> > 
> > The only case where this will happen as far as I can see is if
> > input_clock is between 12 and 14 MHz. In this case, you'll get a clock
> > lower than 7 MHz (worst case is 6 MHz). Is this OK?
> 
> Since it was running out of spec at 1 Mhz, I figured running under
> spec is better than over.

My question was rather, don't you prefer to fail and exit if you can't
get the frequency you want? It really depends on what the consequences
are. I have no idea what was happening with the clock wrongly running
at 1 MHz.

> 
> > 
> >> +	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);
> > 
> > As I already wrote in my previous review: please come up with
> > consistent spacing in this debugging message. Having no space around
> > the first "=", but two spaced before and one after the second "=",
> > doesn't look good.
> 
> Agreed..
> But the previous message was
> > > +	dev_dbg(dev->dev, "input_clock=%d, CLK  = %d\n", input_clock,clk);
> >
> > Please come up with consistent spacing in this debugging message.
> > Missing space after comma.
> And I interpreted you incorrectly. I didn't ignore you.
> 
> 
> > 
> >>  	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",
> > 
> > For consistency, you should add a comma after kHz.
> > 
> >> +		pdata->bus_freq, pdata->bus_delay);
> >>  
> >>  	/* Take the I2C module out of reset: */
> >>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> >> @@ -338,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", __FUNCTION__, __LINE__, ret);
> >> -
> >>  	return num;
> >>  }
> >>  
> > 
> > Not sure how this last chunk is supposed to belong to this patch?
> > 
> Do you want a separate patch? Or just a comment that a dev_dbg was moved so that
> it prints always instead of just on no error?

A separate patch would be better, yes. I know it might sound overkill,
but small patches doing just one thing are much easier to review,
merge, and backport if needs be.

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: clock between 7-12 Mhz

[Jean Delvare]

Hi Troy,

On Fri, 28 Mar 2008 21:36:49 +0100, Jean Delvare wrote:
> On Fri, 28 Mar 2008 13:04:10 -0700, Troy Kisky wrote:
> > Jean Delvare wrote:
> > > Hi Troy,
> > > 
> > > On Thu, 20 Mar 2008 20:06:07 -0700, Troy Kisky wrote:
> > >> Ensure psc value gives a clock between 7-12 Mhz
> > > 
> > > Correct spelling is 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 |   19 ++++++++++++-------
> > >>  1 files changed, 12 insertions(+), 7 deletions(-)
> > >>
> > >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> > >> index fde2634..82d4b7f 100755
> > >> --- 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 7mHz clock, but max of 12mHz */
> > > 
> > > Correct spelling is MHz.
> > 
> > 
> > Hmm... I seems weird that kHz and MHz are correct, as opposed to
> > kHz and mHz,  or KHz and MHz.
> 
> These are the standard international unit prefixes. Please see:
> http://en.wikipedia.org/wiki/SI_prefix#List_of_SI_prefixes
> kilo- is "k", mega- is "M". "m" would be milli-, which isn't what you
> want here.

<snip>

Any update available for this patch? If you want it (and patch 3/5 of
this series) to go in 2.6.26 we better hurry up.

-- 
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: clock between 7-12 Mhz

[Troy Kisky]

Jean Delvare wrote:
> Hi Troy,
> 
> On Fri, 28 Mar 2008 21:36:49 +0100, Jean Delvare wrote:
>> On Fri, 28 Mar 2008 13:04:10 -0700, Troy Kisky wrote:
>>> Jean Delvare wrote:
>>>> Hi Troy,
>>>>
>>>> On Thu, 20 Mar 2008 20:06:07 -0700, Troy Kisky wrote:
>>>>> Ensure psc value gives a clock between 7-12 Mhz
>>>> Correct spelling is 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 |   19 ++++++++++++-------
>>>>>  1 files changed, 12 insertions(+), 7 deletions(-)
>>>>>
>>>>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>>>>> index fde2634..82d4b7f 100755
>>>>> --- 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 7mHz clock, but max of 12mHz */
>>>> Correct spelling is MHz.
>>>
>>> Hmm... I seems weird that kHz and MHz are correct, as opposed to
>>> kHz and mHz,  or KHz and MHz.
>> These are the standard international unit prefixes. Please see:
>> http://en.wikipedia.org/wiki/SI_prefix#List_of_SI_prefixes
>> kilo- is "k", mega- is "M". "m" would be milli-, which isn't what you
>> want here.
> 
> <snip>
> 
> Any update available for this patch? If you want it (and patch 3/5 of
> this series) to go in 2.6.26 we better hurry up.
> 
Kevin's approval for 1 of the patches has temporarily been revoked.

I'm waiting to get it back.

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: clock between 7-12 Mhz

[Troy Kisky]

>> Any update available for this patch? If you want it (and patch 3/5 of
>> this series) to go in 2.6.26 we better hurry up.
>>
> Kevin's approval for 1 of the patches has temporarily been revoked.
> 
> I'm waiting to get it back.
> 
> Thanks
> Troy
> 
I just noticed it was back in his tree. Sorry for the noise.
I'll get on the next revision.

Thanks
Troy


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

Re: [PATCH 1/5] I2C: DaVinci: fix lost interrupt

[Jean Delvare]

Hi Troy,

On Thu, 20 Mar 2008 20:06:06 -0700, Troy Kisky wrote:
> DAVINCI_I2C_STR_REG is a write 1 to clear register,
> so don't use a read/modify/write cycle.
> 
> Signed-off-by: Troy Kisky <troy.kisky-Q5RJGjKts06CY9SHAMCTRUEOCMrvLtNR@public.gmane.org>
> Acked-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   17 ++++++-----------
>  1 files changed, 6 insertions(+), 11 deletions(-)
>  mode change 100644 => 100755 drivers/i2c/busses/i2c-davinci.c

This mode change is bogus, so I'm discarding it.

> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> old mode 100644
> new mode 100755
> index cce5a61..fde2634
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -382,9 +382,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  			break;
>  
>  		case DAVINCI_I2C_IVR_ARDY:
> -			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG);
> -			MOD_REG_BIT(w, DAVINCI_I2C_STR_ARDY, 1);
> -			davinci_i2c_write_reg(dev, DAVINCI_I2C_STR_REG, w);
> +			davinci_i2c_write_reg(dev,
> +				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ARDY);
>  			complete(&dev->cmd_complete);
>  			break;
>  
> @@ -397,12 +396,9 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  				if (dev->buf_len)
>  					continue;
>  
> -				w = davinci_i2c_read_reg(dev,
> -							 DAVINCI_I2C_STR_REG);
> -				MOD_REG_BIT(w, DAVINCI_I2C_IMR_RRDY, 0);
>  				davinci_i2c_write_reg(dev,
> -						      DAVINCI_I2C_STR_REG,
> -						      w);
> +					DAVINCI_I2C_STR_REG,
> +					DAVINCI_I2C_IMR_RRDY);
>  			} else
>  				dev_err(dev->dev, "RDR IRQ while no "
>  					"data requested\n");
> @@ -428,9 +424,8 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  			break;
>  
>  		case DAVINCI_I2C_IVR_SCD:
> -			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG);
> -			MOD_REG_BIT(w, DAVINCI_I2C_STR_SCD, 1);
> -			davinci_i2c_write_reg(dev, DAVINCI_I2C_STR_REG, w);
> +			davinci_i2c_write_reg(dev,
> +				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_SCD);
>  			complete(&dev->cmd_complete);
>  			break;
>  

Patch applied, thank you. It's queued for 2.6.25 if I have a chance to
push more patches to Linus before it's released. If not, it'll go in
2.6.26-rc1.

-- 
Jean Delvare

_______________________________________________
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

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