[PATCH 0/6] davinci i2c updates for 2.6.34

[PATCH 0/6] davinci i2c updates for 2.6.34

[Kevin Hilman]

This series ia a set of proposed I2C updates for the DaVinci platform
for 2.6.34.  These have been tested and approved as part of the
DaVinci tree.

Note that patch 6 has a dependency on changes to the DaVinci core so
is has been agreed to be merged it via davinci git.

Ben, upon successful review, please let me know if you'd like
me to merge the others via davinci git as well.

Thanks,

Kevin

Chaithrika U S (4):
  i2c: davinci: Remove MOD_REG_BIT and IO_ADDRESS usage
  i2c: davinci: Add helper functions
  i2c: davinci: Add suspend/resume support
  i2c: davinci: Add cpufreq support

Dirk Behme (1):
  i2c: davinci: Fix smbus Oops with AIC33 usage

Philby John (1):
  i2c: davinci: bus recovery procedure to clear the bus

 drivers/i2c/busses/i2c-davinci.c |  312 ++++++++++++++++++++++++++++++--------
 1 files changed, 249 insertions(+), 63 deletions(-)

[PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Kevin Hilman]

From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>

This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
addresses.

Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
 1 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index c89687a..444a9f2 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -112,6 +112,7 @@ struct davinci_i2c_dev {
 	u8			*buf;
 	size_t			buf_len;
 	int			irq;
+	int			stop;
 	u8			terminate;
 	struct i2c_adapter	adapter;
 };
@@ -249,9 +250,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	u16 w;
 	int r;
 
-	if (msg->len == 0)
-		return -EINVAL;
-
 	if (!pdata)
 		pdata = &davinci_i2c_platform_data_default;
 	/* Introduce a delay, required for some boards (e.g Davinci EVM) */
@@ -263,6 +261,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 
 	dev->buf = msg->buf;
 	dev->buf_len = msg->len;
+	dev->stop = stop;
 
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CNT_REG, dev->buf_len);
 
@@ -280,6 +279,10 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 		flag |= DAVINCI_I2C_MDR_TRX;
 	if (stop)
 		flag |= DAVINCI_I2C_MDR_STP;
+	if (msg->len == 0) {
+		flag |= DAVINCI_I2C_MDR_RM;
+		flag &= ~DAVINCI_I2C_MDR_STP;
+	}
 
 	/* Enable receive or transmit interrupts */
 	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
@@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
 
 	dev->terminate = 0;
+
+	/* First byte should be set here, not after interrupt,
+	 * because transmit-data-ready interrupt can come before
+	 * NACK-interrupt during sending of previous message and
+	 * ICDXR may have wrong data */
+	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
+		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
+		dev->buf_len--;
+	}
+
 	/* write the data into mode register */
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
 
@@ -371,7 +384,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 
 static u32 i2c_davinci_func(struct i2c_adapter *adap)
 {
-	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
 }
 
 static void terminate_read(struct davinci_i2c_dev *dev)
@@ -430,6 +443,14 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 		case DAVINCI_I2C_IVR_ARDY:
 			davinci_i2c_write_reg(dev,
 				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ARDY);
+			if (((dev->buf_len == 0) && (dev->stop != 0)) ||
+			    (dev->cmd_err & DAVINCI_I2C_STR_NACK)) {
+				w = davinci_i2c_read_reg(dev,
+							 DAVINCI_I2C_MDR_REG);
+				w |= DAVINCI_I2C_MDR_STP;
+				davinci_i2c_write_reg(dev,
+						      DAVINCI_I2C_MDR_REG, w);
+			}
 			complete(&dev->cmd_complete);
 			break;
 
-- 
1.6.6

[PATCH 2/6] i2c: davinci: Remove MOD_REG_BIT and IO_ADDRESS usage

[Kevin Hilman]

From: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>

Cleanup the DaVinci I2C driver. Remove MOD_REG_BIT macro.
Also use ioremap instead of IO_ADDRESS macro.

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   77 +++++++++++++++++++-------------------
 1 files changed, 38 insertions(+), 39 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 444a9f2..34a48d0 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -37,7 +37,6 @@
 #include <linux/io.h>
 
 #include <mach/hardware.h>
-
 #include <mach/i2c.h>
 
 /* ----- global defines ----------------------------------------------- */
@@ -71,37 +70,29 @@
 #define DAVINCI_I2C_IVR_NACK	0x02
 #define DAVINCI_I2C_IVR_AL	0x01
 
-#define DAVINCI_I2C_STR_BB	(1 << 12)
-#define DAVINCI_I2C_STR_RSFULL	(1 << 11)
-#define DAVINCI_I2C_STR_SCD	(1 << 5)
-#define DAVINCI_I2C_STR_ARDY	(1 << 2)
-#define DAVINCI_I2C_STR_NACK	(1 << 1)
-#define DAVINCI_I2C_STR_AL	(1 << 0)
-
-#define DAVINCI_I2C_MDR_NACK	(1 << 15)
-#define DAVINCI_I2C_MDR_STT	(1 << 13)
-#define DAVINCI_I2C_MDR_STP	(1 << 11)
-#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)
-#define DAVINCI_I2C_IMR_SCD	(1 << 5)
-#define DAVINCI_I2C_IMR_XRDY	(1 << 4)
-#define DAVINCI_I2C_IMR_RRDY	(1 << 3)
-#define DAVINCI_I2C_IMR_ARDY	(1 << 2)
-#define DAVINCI_I2C_IMR_NACK	(1 << 1)
-#define DAVINCI_I2C_IMR_AL	(1 << 0)
-
-#define MOD_REG_BIT(val, mask, set) do { \
-	if (set) { \
-		val |= mask; \
-	} else { \
-		val &= ~mask; \
-	} \
-} while (0)
+#define DAVINCI_I2C_STR_BB	BIT(12)
+#define DAVINCI_I2C_STR_RSFULL	BIT(11)
+#define DAVINCI_I2C_STR_SCD	BIT(5)
+#define DAVINCI_I2C_STR_ARDY	BIT(2)
+#define DAVINCI_I2C_STR_NACK	BIT(1)
+#define DAVINCI_I2C_STR_AL	BIT(0)
+
+#define DAVINCI_I2C_MDR_NACK	BIT(15)
+#define DAVINCI_I2C_MDR_STT	BIT(13)
+#define DAVINCI_I2C_MDR_STP	BIT(11)
+#define DAVINCI_I2C_MDR_MST	BIT(10)
+#define DAVINCI_I2C_MDR_TRX	BIT(9)
+#define DAVINCI_I2C_MDR_XA	BIT(8)
+#define DAVINCI_I2C_MDR_RM	BIT(7)
+#define DAVINCI_I2C_MDR_IRS	BIT(5)
+
+#define DAVINCI_I2C_IMR_AAS	BIT(6)
+#define DAVINCI_I2C_IMR_SCD	BIT(5)
+#define DAVINCI_I2C_IMR_XRDY	BIT(4)
+#define DAVINCI_I2C_IMR_RRDY	BIT(3)
+#define DAVINCI_I2C_IMR_ARDY	BIT(2)
+#define DAVINCI_I2C_IMR_NACK	BIT(1)
+#define DAVINCI_I2C_IMR_AL	BIT(0)
 
 struct davinci_i2c_dev {
 	struct device           *dev;
@@ -155,7 +146,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 
 	/* put I2C into reset */
 	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
+	w &= ~DAVINCI_I2C_MDR_IRS;
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 
 	/* NOTE: I2C Clock divider programming info
@@ -205,7 +196,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 
 	/* Take the I2C module out of reset: */
 	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
+	w |= DAVINCI_I2C_MDR_IRS;
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 
 	/* Enable interrupts */
@@ -287,9 +278,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 	/* Enable receive or transmit interrupts */
 	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
 	if (msg->flags & I2C_M_RD)
-		MOD_REG_BIT(w, DAVINCI_I2C_IMR_RRDY, 1);
+		w |= DAVINCI_I2C_IMR_RRDY;
 	else
-		MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 1);
+		w |= DAVINCI_I2C_IMR_XRDY;
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
 
 	dev->terminate = 0;
@@ -346,7 +337,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 			return msg->len;
 		if (stop) {
 			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-			MOD_REG_BIT(w, DAVINCI_I2C_MDR_STP, 1);
+			w |= DAVINCI_I2C_MDR_STP;
 			davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 		}
 		return -EREMOTEIO;
@@ -482,7 +473,7 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 
 				w = davinci_i2c_read_reg(dev,
 							 DAVINCI_I2C_IMR_REG);
-				MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 0);
+				w &= ~DAVINCI_I2C_IMR_XRDY;
 				davinci_i2c_write_reg(dev,
 						      DAVINCI_I2C_IMR_REG,
 						      w);
@@ -561,7 +552,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	}
 	clk_enable(dev->clk);
 
-	dev->base = (void __iomem *)IO_ADDRESS(mem->start);
+	dev->base = ioremap(mem->start, resource_size(mem));
+	if (!dev->base) {
+		r = -EBUSY;
+		goto err_mem_ioremap;
+	}
+
 	i2c_davinci_init(dev);
 
 	r = request_irq(dev->irq, i2c_davinci_isr, 0, pdev->name, dev);
@@ -591,6 +587,8 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 err_free_irq:
 	free_irq(dev->irq, dev);
 err_unuse_clocks:
+	iounmap(dev->base);
+err_mem_ioremap:
 	clk_disable(dev->clk);
 	clk_put(dev->clk);
 	dev->clk = NULL;
@@ -619,6 +617,7 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
 	free_irq(IRQ_I2C, dev);
+	iounmap(dev->base);
 	kfree(dev);
 
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
-- 
1.6.6

[PATCH 3/6] i2c: davinci: Add helper functions

[Kevin Hilman]

From: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>

Add i2c reset control and clock divider calculation functions
which will be useful for power management features.

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   56 +++++++++++++++++++++++++-------------
 1 files changed, 37 insertions(+), 19 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 34a48d0..aacef7b 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -125,12 +125,21 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
 	return __raw_readw(i2c_dev->base + reg);
 }
 
-/*
- * This functions configures I2C and brings I2C out of reset.
- * This function is called during I2C init function. This function
- * also gets called if I2C encounters any errors.
- */
-static int i2c_davinci_init(struct davinci_i2c_dev *dev)
+static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
+								int val)
+{
+	u16 w;
+
+	w = davinci_i2c_read_reg(i2c_dev, DAVINCI_I2C_MDR_REG);
+	if (!val)	/* put I2C into reset */
+		w &= ~DAVINCI_I2C_MDR_IRS;
+	else		/* take I2C out of reset */
+		w |= DAVINCI_I2C_MDR_IRS;
+
+	davinci_i2c_write_reg(i2c_dev, DAVINCI_I2C_MDR_REG, w);
+}
+
+static void i2c_davinci_calc_clk_dividers(struct davinci_i2c_dev *dev)
 {
 	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
 	u16 psc;
@@ -139,15 +148,6 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 	u32 clkh;
 	u32 clkl;
 	u32 input_clock = clk_get_rate(dev->clk);
-	u16 w;
-
-	if (!pdata)
-		pdata = &davinci_i2c_platform_data_default;
-
-	/* put I2C into reset */
-	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	w &= ~DAVINCI_I2C_MDR_IRS;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
 
 	/* NOTE: I2C Clock divider programming info
 	 * As per I2C specs the following formulas provide prescaler
@@ -179,12 +179,32 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKH_REG, clkh);
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_CLKL_REG, clkl);
 
+	dev_dbg(dev->dev, "input_clock = %d, CLK = %d\n", input_clock, clk);
+}
+
+/*
+ * This function configures I2C and brings I2C out of reset.
+ * This function is called during I2C init function. This function
+ * also gets called if I2C encounters any errors.
+ */
+static int i2c_davinci_init(struct davinci_i2c_dev *dev)
+{
+	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
+
+	if (!pdata)
+		pdata = &davinci_i2c_platform_data_default;
+
+	/* put I2C into reset */
+	davinci_i2c_reset_ctrl(dev, 0);
+
+	/* compute clock dividers */
+	i2c_davinci_calc_clk_dividers(dev);
+
 	/* Respond at reserved "SMBus Host" slave address" (and zero);
 	 * we seem to have no option to not respond...
 	 */
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_OAR_REG, 0x08);
 
-	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",
@@ -195,9 +215,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
 		pdata->bus_freq, pdata->bus_delay);
 
 	/* Take the I2C module out of reset: */
-	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
-	w |= DAVINCI_I2C_MDR_IRS;
-	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
+	davinci_i2c_reset_ctrl(dev, 1);
 
 	/* Enable interrupts */
 	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, I2C_DAVINCI_INTR_ALL);
-- 
1.6.6

[PATCH 4/6] i2c: davinci: Add suspend/resume support

[Kevin Hilman]

From: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>

Add suspend and resume callbacks to DaVinci I2C driver.
This has been tested on DA850/OMAP-L138 EVM. The SoC specific
suspend-to-RAM support patch series [1] is needed to test this feature.

[1] http://linux.davincidsp.com/pipermail/davinci-linux-open-source/
2009-November/016958.html

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index aacef7b..66b07bf 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -643,6 +643,41 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#ifdef CONFIG_PM
+static int davinci_i2c_suspend(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct davinci_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	/* put I2C into reset */
+	davinci_i2c_reset_ctrl(i2c_dev, 0);
+	clk_disable(i2c_dev->clk);
+
+	return 0;
+}
+
+static int davinci_i2c_resume(struct device *dev)
+{
+	struct platform_device *pdev = to_platform_device(dev);
+	struct davinci_i2c_dev *i2c_dev = platform_get_drvdata(pdev);
+
+	clk_enable(i2c_dev->clk);
+	/* take I2C out of reset */
+	davinci_i2c_reset_ctrl(i2c_dev, 1);
+
+	return 0;
+}
+
+static struct dev_pm_ops davinci_i2c_pm = {
+	.suspend        = davinci_i2c_suspend,
+	.resume         = davinci_i2c_resume,
+};
+
+#define davinci_i2c_pm_ops (&davinci_i2c_pm)
+#else
+#define davinci_i2c_pm_ops NULL
+#endif
+
 /* work with hotplug and coldplug */
 MODULE_ALIAS("platform:i2c_davinci");
 
@@ -652,6 +687,7 @@ static struct platform_driver davinci_i2c_driver = {
 	.driver		= {
 		.name	= "i2c_davinci",
 		.owner	= THIS_MODULE,
+		.pm	= davinci_i2c_pm_ops,
 	},
 };
 
-- 
1.6.6

[PATCH 5/6] i2c: davinci: Add cpufreq support

[Kevin Hilman]

From: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>

Add cpufreq support for DaVinci I2C driver.
Tested on DA850/OMAP-L138 EVM. For the purpose of testing, the patches
which add cpufreq support [1] for this SoC are needed.

[1]http://linux.davincidsp.com/pipermail/davinci-linux-open-source/
2009-September/016118.html

Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   63 ++++++++++++++++++++++++++++++++++++++
 1 files changed, 63 insertions(+), 0 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 66b07bf..35f9daa 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -35,6 +35,7 @@
 #include <linux/interrupt.h>
 #include <linux/platform_device.h>
 #include <linux/io.h>
+#include <linux/cpufreq.h>
 
 #include <mach/hardware.h>
 #include <mach/i2c.h>
@@ -106,6 +107,10 @@ struct davinci_i2c_dev {
 	int			stop;
 	u8			terminate;
 	struct i2c_adapter	adapter;
+#ifdef CONFIG_CPU_FREQ
+	struct completion	xfr_complete;
+	struct notifier_block	freq_transition;
+#endif
 };
 
 /* default platform data to use if not supplied in the platform_device */
@@ -388,6 +393,11 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
 		if (ret < 0)
 			return ret;
 	}
+
+#ifdef CONFIG_CPU_FREQ
+	complete(&dev->xfr_complete);
+#endif
+
 	return num;
 }
 
@@ -520,6 +530,48 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
 	return count ? IRQ_HANDLED : IRQ_NONE;
 }
 
+#ifdef CONFIG_CPU_FREQ
+static int i2c_davinci_cpufreq_transition(struct notifier_block *nb,
+				     unsigned long val, void *data)
+{
+	struct davinci_i2c_dev *dev;
+
+	dev = container_of(nb, struct davinci_i2c_dev, freq_transition);
+	if (val == CPUFREQ_PRECHANGE) {
+		wait_for_completion(&dev->xfr_complete);
+		davinci_i2c_reset_ctrl(dev, 0);
+	} else if (val == CPUFREQ_POSTCHANGE) {
+		i2c_davinci_calc_clk_dividers(dev);
+		davinci_i2c_reset_ctrl(dev, 1);
+	}
+
+	return 0;
+}
+
+static inline int i2c_davinci_cpufreq_register(struct davinci_i2c_dev *dev)
+{
+	dev->freq_transition.notifier_call = i2c_davinci_cpufreq_transition;
+
+	return cpufreq_register_notifier(&dev->freq_transition,
+					 CPUFREQ_TRANSITION_NOTIFIER);
+}
+
+static inline void i2c_davinci_cpufreq_deregister(struct davinci_i2c_dev *dev)
+{
+	cpufreq_unregister_notifier(&dev->freq_transition,
+				    CPUFREQ_TRANSITION_NOTIFIER);
+}
+#else
+static inline int i2c_davinci_cpufreq_register(struct davinci_i2c_dev *dev)
+{
+	return 0;
+}
+
+static inline void i2c_davinci_cpufreq_deregister(struct davinci_i2c_dev *dev)
+{
+}
+#endif
+
 static struct i2c_algorithm i2c_davinci_algo = {
 	.master_xfer	= i2c_davinci_xfer,
 	.functionality	= i2c_davinci_func,
@@ -559,6 +611,9 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 	}
 
 	init_completion(&dev->cmd_complete);
+#ifdef CONFIG_CPU_FREQ
+	init_completion(&dev->xfr_complete);
+#endif
 	dev->dev = get_device(&pdev->dev);
 	dev->irq = irq->start;
 	platform_set_drvdata(pdev, dev);
@@ -584,6 +639,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
 		goto err_unuse_clocks;
 	}
 
+	r = i2c_davinci_cpufreq_register(dev);
+	if (r) {
+		dev_err(&pdev->dev, "failed to register cpufreq\n");
+		goto err_free_irq;
+	}
+
 	adap = &dev->adapter;
 	i2c_set_adapdata(adap, dev);
 	adap->owner = THIS_MODULE;
@@ -625,6 +686,8 @@ static int davinci_i2c_remove(struct platform_device *pdev)
 	struct davinci_i2c_dev *dev = platform_get_drvdata(pdev);
 	struct resource *mem;
 
+	i2c_davinci_cpufreq_deregister(dev);
+
 	platform_set_drvdata(pdev, NULL);
 	i2c_del_adapter(&dev->adapter);
 	put_device(&pdev->dev);
-- 
1.6.6

[PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Kevin Hilman]

From: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>

Come out of i2c time out condition by following the
bus recovery procedure outlined in the i2c protocol v3 spec.
The kernel must be robust enough to gracefully recover
from i2c bus failure without having to reset the machine.
This is done by first NACKing the slave, pulsing the SCL
line 9 times and then sending the stop command.

This patch has been tested on a DM6446 and DM355

Signed-off-by: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
Signed-off-by: Srinivasan, Nageswari <nageswari-l0cyMroinI0@public.gmane.org>
Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |   57 +++++++++++++++++++++++++++++++++++--
 1 files changed, 53 insertions(+), 4 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 35f9daa..5459065 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -36,6 +36,7 @@
 #include <linux/platform_device.h>
 #include <linux/io.h>
 #include <linux/cpufreq.h>
+#include <linux/gpio.h>
 
 #include <mach/hardware.h>
 #include <mach/i2c.h>
@@ -43,6 +44,7 @@
 /* ----- global defines ----------------------------------------------- */
 
 #define DAVINCI_I2C_TIMEOUT	(1*HZ)
+#define DAVINCI_I2C_MAX_TRIES	2
 #define I2C_DAVINCI_INTR_ALL    (DAVINCI_I2C_IMR_AAS | \
 				 DAVINCI_I2C_IMR_SCD | \
 				 DAVINCI_I2C_IMR_ARDY | \
@@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
 	return __raw_readw(i2c_dev->base + reg);
 }
 
+/* Generate a pulse on the i2c clock pin. */
+static void generic_i2c_clock_pulse(unsigned int scl_pin)
+{
+	u16 i;
+
+	if (scl_pin) {
+		/* Send high and low on the SCL line */
+		for (i = 0; i < 9; i++) {
+			gpio_set_value(scl_pin, 0);
+			udelay(20);
+			gpio_set_value(scl_pin, 1);
+			udelay(20);
+		}
+	}
+}
+
+/* This routine does i2c bus recovery as specified in the
+ * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
+ */
+static void i2c_recover_bus(struct davinci_i2c_dev *dev)
+{
+	u32 flag = 0;
+	struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
+
+	dev_err(dev->dev, "initiating i2c bus recovery\n");
+	/* Send NACK to the slave */
+	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+	flag |=  DAVINCI_I2C_MDR_NACK;
+	/* write the data into mode register */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+	if (pdata)
+		generic_i2c_clock_pulse(pdata->scl_pin);
+	/* Send STOP */
+	flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
+	flag |= DAVINCI_I2C_MDR_STP;
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+}
+
 static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
 								int val)
 {
@@ -235,14 +275,22 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
 					 char allow_sleep)
 {
 	unsigned long timeout;
+	static u16 to_cnt;
 
 	timeout = jiffies + dev->adapter.timeout;
 	while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
 	       & DAVINCI_I2C_STR_BB) {
-		if (time_after(jiffies, timeout)) {
-			dev_warn(dev->dev,
-				 "timeout waiting for bus ready\n");
-			return -ETIMEDOUT;
+		if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
+			if (time_after(jiffies, timeout)) {
+				dev_warn(dev->dev,
+				"timeout waiting for bus ready\n");
+				to_cnt++;
+				return -ETIMEDOUT;
+			} else {
+				to_cnt = 0;
+				i2c_recover_bus(dev);
+				i2c_davinci_init(dev);
+			}
 		}
 		if (allow_sleep)
 			schedule_timeout(1);
@@ -324,6 +372,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 						      dev->adapter.timeout);
 	if (r == 0) {
 		dev_err(dev->dev, "controller timed out\n");
+		i2c_recover_bus(dev);
 		i2c_davinci_init(dev);
 		dev->buf_len = 0;
 		return -ETIMEDOUT;
-- 
1.6.6

RE: [PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Sudhakar Rajashekhara]

Hi,

On Wed, Jan 27, 2010 at 05:11:28, Kevin Hilman wrote:
> From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> 
> This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
> addresses.
> 
> Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
>  1 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index c89687a..444a9f2 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c

[...]

> @@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>  
>  	dev->terminate = 0;
> +
> +	/* First byte should be set here, not after interrupt,
> +	 * because transmit-data-ready interrupt can come before
> +	 * NACK-interrupt during sending of previous message and
> +	 * ICDXR may have wrong data */
> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
> +		dev->buf_len--;
> +	}
> +

I found an issue on DA850 while using this patch. I think the above code should come after the
below lines because an I2C transaction is being carried out before configuring the I2C mode
register (which has bits to configure Master, Start condition etc), which causes undefined
behavior.


>  	/* write the data into mode register */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>  

Regards,
Sudhakar

Re: [PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Kevin Hilman]

"Sudhakar Rajashekhara" <sudhakar.raj-l0cyMroinI0@public.gmane.org> writes:

> Hi,
>
> On Wed, Jan 27, 2010 at 05:11:28, Kevin Hilman wrote:
>> From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> 
>> This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
>> addresses.
>> 
>> Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
>> Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
>>  1 files changed, 25 insertions(+), 4 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index c89687a..444a9f2 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>
> [...]
>
>> @@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>>  
>>  	dev->terminate = 0;
>> +
>> +	/* First byte should be set here, not after interrupt,
>> +	 * because transmit-data-ready interrupt can come before
>> +	 * NACK-interrupt during sending of previous message and
>> +	 * ICDXR may have wrong data */
>> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
>> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
>> +		dev->buf_len--;
>> +	}
>> +
>
> I found an issue on DA850 while using this patch. 

ok, I'll drop this one for 2.6.34 until we figure this out.

> I think the above code should come after the
> below lines because an I2C transaction is being carried out before configuring the I2C mode
> register (which has bits to configure Master, Start condition etc), which causes undefined
> behavior.

hmm, are you seeing this behavior on davinci git too?  This is the same
patch that has been in davinci git for some time?

Kevin

>
>>  	/* write the data into mode register */
>>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>>  
>
> Regards,
> Sudhakar

Re: [PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Ben Dooks]

On Tue, Jan 26, 2010 at 03:41:28PM -0800, Kevin Hilman wrote:
> From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> 
> This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
> addresses.
> 
> Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
> Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
>  1 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index c89687a..444a9f2 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -112,6 +112,7 @@ struct davinci_i2c_dev {
>  	u8			*buf;
>  	size_t			buf_len;
>  	int			irq;
> +	int			stop;
>  	u8			terminate;
>  	struct i2c_adapter	adapter;
>  };
> @@ -249,9 +250,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	u16 w;
>  	int r;
>  
> -	if (msg->len == 0)
> -		return -EINVAL;
> -
>  	if (!pdata)
>  		pdata = &davinci_i2c_platform_data_default;
>  	/* Introduce a delay, required for some boards (e.g Davinci EVM) */
> @@ -263,6 +261,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  
>  	dev->buf = msg->buf;
>  	dev->buf_len = msg->len;
> +	dev->stop = stop;
>  
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_CNT_REG, dev->buf_len);
>  
> @@ -280,6 +279,10 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  		flag |= DAVINCI_I2C_MDR_TRX;
>  	if (stop)
>  		flag |= DAVINCI_I2C_MDR_STP;
> +	if (msg->len == 0) {
> +		flag |= DAVINCI_I2C_MDR_RM;
> +		flag &= ~DAVINCI_I2C_MDR_STP;
> +	}
>  
>  	/* Enable receive or transmit interrupts */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
> @@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>  
>  	dev->terminate = 0;
> +
> +	/* First byte should be set here, not after interrupt,
> +	 * because transmit-data-ready interrupt can come before
> +	 * NACK-interrupt during sending of previous message and
> +	 * ICDXR may have wrong data */
> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
> +		dev->buf_len--;
> +	}
> +
>  	/* write the data into mode register */
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
>  
> @@ -371,7 +384,7 @@ i2c_davinci_xfer(struct i2c_adapter *adap, struct i2c_msg msgs[], int num)
>  
>  static u32 i2c_davinci_func(struct i2c_adapter *adap)
>  {
> -	return I2C_FUNC_I2C | (I2C_FUNC_SMBUS_EMUL & ~I2C_FUNC_SMBUS_QUICK);
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>  }
>  
>  static void terminate_read(struct davinci_i2c_dev *dev)
> @@ -430,6 +443,14 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  		case DAVINCI_I2C_IVR_ARDY:
>  			davinci_i2c_write_reg(dev,
>  				DAVINCI_I2C_STR_REG, DAVINCI_I2C_STR_ARDY);
> +			if (((dev->buf_len == 0) && (dev->stop != 0)) ||

technically you don't need () around simple tests...

> +			    (dev->cmd_err & DAVINCI_I2C_STR_NACK)) {
> +				w = davinci_i2c_read_reg(dev,
> +							 DAVINCI_I2C_MDR_REG);
> +				w |= DAVINCI_I2C_MDR_STP;
> +				davinci_i2c_write_reg(dev,
> +						      DAVINCI_I2C_MDR_REG, w);
> +			}
>  			complete(&dev->cmd_complete);
>  			break;
>  
> -- 
> 1.6.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

Re: [PATCH 2/6] i2c: davinci: Remove MOD_REG_BIT and IO_ADDRESS usage

[Ben Dooks]

On Tue, Jan 26, 2010 at 03:41:29PM -0800, Kevin Hilman wrote:
> From: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> 
> Cleanup the DaVinci I2C driver. Remove MOD_REG_BIT macro.
> Also use ioremap instead of IO_ADDRESS macro.

would have been nicer to do each of these seperately.
 
> Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   77 +++++++++++++++++++-------------------
>  1 files changed, 38 insertions(+), 39 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 444a9f2..34a48d0 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -37,7 +37,6 @@
>  #include <linux/io.h>
>  
>  #include <mach/hardware.h>
> -
>  #include <mach/i2c.h>
>  
>  /* ----- global defines ----------------------------------------------- */
> @@ -71,37 +70,29 @@
>  #define DAVINCI_I2C_IVR_NACK	0x02
>  #define DAVINCI_I2C_IVR_AL	0x01
>  
> -#define DAVINCI_I2C_STR_BB	(1 << 12)
> -#define DAVINCI_I2C_STR_RSFULL	(1 << 11)
> -#define DAVINCI_I2C_STR_SCD	(1 << 5)
> -#define DAVINCI_I2C_STR_ARDY	(1 << 2)
> -#define DAVINCI_I2C_STR_NACK	(1 << 1)
> -#define DAVINCI_I2C_STR_AL	(1 << 0)
> -
> -#define DAVINCI_I2C_MDR_NACK	(1 << 15)
> -#define DAVINCI_I2C_MDR_STT	(1 << 13)
> -#define DAVINCI_I2C_MDR_STP	(1 << 11)
> -#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)
> -#define DAVINCI_I2C_IMR_SCD	(1 << 5)
> -#define DAVINCI_I2C_IMR_XRDY	(1 << 4)
> -#define DAVINCI_I2C_IMR_RRDY	(1 << 3)
> -#define DAVINCI_I2C_IMR_ARDY	(1 << 2)
> -#define DAVINCI_I2C_IMR_NACK	(1 << 1)
> -#define DAVINCI_I2C_IMR_AL	(1 << 0)
> -
> -#define MOD_REG_BIT(val, mask, set) do { \
> -	if (set) { \
> -		val |= mask; \
> -	} else { \
> -		val &= ~mask; \
> -	} \
> -} while (0)
> +#define DAVINCI_I2C_STR_BB	BIT(12)
> +#define DAVINCI_I2C_STR_RSFULL	BIT(11)
> +#define DAVINCI_I2C_STR_SCD	BIT(5)
> +#define DAVINCI_I2C_STR_ARDY	BIT(2)
> +#define DAVINCI_I2C_STR_NACK	BIT(1)
> +#define DAVINCI_I2C_STR_AL	BIT(0)
> +
> +#define DAVINCI_I2C_MDR_NACK	BIT(15)
> +#define DAVINCI_I2C_MDR_STT	BIT(13)
> +#define DAVINCI_I2C_MDR_STP	BIT(11)
> +#define DAVINCI_I2C_MDR_MST	BIT(10)
> +#define DAVINCI_I2C_MDR_TRX	BIT(9)
> +#define DAVINCI_I2C_MDR_XA	BIT(8)
> +#define DAVINCI_I2C_MDR_RM	BIT(7)
> +#define DAVINCI_I2C_MDR_IRS	BIT(5)
> +
> +#define DAVINCI_I2C_IMR_AAS	BIT(6)
> +#define DAVINCI_I2C_IMR_SCD	BIT(5)
> +#define DAVINCI_I2C_IMR_XRDY	BIT(4)
> +#define DAVINCI_I2C_IMR_RRDY	BIT(3)
> +#define DAVINCI_I2C_IMR_ARDY	BIT(2)
> +#define DAVINCI_I2C_IMR_NACK	BIT(1)
> +#define DAVINCI_I2C_IMR_AL	BIT(0)

surely 1 << x isn't too difficult to understand??
  
>  struct davinci_i2c_dev {
>  	struct device           *dev;
> @@ -155,7 +146,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  
>  	/* put I2C into reset */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
> +	w &= ~DAVINCI_I2C_MDR_IRS;
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>  
>  	/* NOTE: I2C Clock divider programming info
> @@ -205,7 +196,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>  
>  	/* Take the I2C module out of reset: */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
> +	w |= DAVINCI_I2C_MDR_IRS;
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>  
>  	/* Enable interrupts */
> @@ -287,9 +278,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  	/* Enable receive or transmit interrupts */
>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
>  	if (msg->flags & I2C_M_RD)
> -		MOD_REG_BIT(w, DAVINCI_I2C_IMR_RRDY, 1);
> +		w |= DAVINCI_I2C_IMR_RRDY;
>  	else
> -		MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 1);
> +		w |= DAVINCI_I2C_IMR_XRDY;
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>  
>  	dev->terminate = 0;
> @@ -346,7 +337,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  			return msg->len;
>  		if (stop) {
>  			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> -			MOD_REG_BIT(w, DAVINCI_I2C_MDR_STP, 1);
> +			w |= DAVINCI_I2C_MDR_STP;
>  			davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>  		}
>  		return -EREMOTEIO;
> @@ -482,7 +473,7 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>  
>  				w = davinci_i2c_read_reg(dev,
>  							 DAVINCI_I2C_IMR_REG);
> -				MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 0);
> +				w &= ~DAVINCI_I2C_IMR_XRDY;
>  				davinci_i2c_write_reg(dev,
>  						      DAVINCI_I2C_IMR_REG,
>  						      w);
> @@ -561,7 +552,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  	}
>  	clk_enable(dev->clk);
>  
> -	dev->base = (void __iomem *)IO_ADDRESS(mem->start);
> +	dev->base = ioremap(mem->start, resource_size(mem));
> +	if (!dev->base) {
> +		r = -EBUSY;
> +		goto err_mem_ioremap;
> +	}
> +

this looks like you've got >1 change per patch?

>  	i2c_davinci_init(dev);
>  
>  	r = request_irq(dev->irq, i2c_davinci_isr, 0, pdev->name, dev);
> @@ -591,6 +587,8 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>  err_free_irq:
>  	free_irq(dev->irq, dev);
>  err_unuse_clocks:
> +	iounmap(dev->base);
> +err_mem_ioremap:
>  	clk_disable(dev->clk);
>  	clk_put(dev->clk);
>  	dev->clk = NULL;
> @@ -619,6 +617,7 @@ static int davinci_i2c_remove(struct platform_device *pdev)
>  
>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
>  	free_irq(IRQ_I2C, dev);
> +	iounmap(dev->base);
>  	kfree(dev);
>  
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -- 
> 1.6.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

RE: [PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Sudhakar Rajashekhara]

On Wed, Jan 27, 2010 at 20:20:08, Kevin Hilman wrote:
> "Sudhakar Rajashekhara" <sudhakar.raj-l0cyMroinI0@public.gmane.org> writes:
> 
> > Hi,
> >
> > On Wed, Jan 27, 2010 at 05:11:28, Kevin Hilman wrote:
> >> From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> >> 
> >> This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
> >> addresses.
> >> 
> >> Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
> >> Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
> >>  1 files changed, 25 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> >> index c89687a..444a9f2 100644
> >> --- a/drivers/i2c/busses/i2c-davinci.c
> >> +++ b/drivers/i2c/busses/i2c-davinci.c
> >
> > [...]
> >
> >> @@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
> >>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
> >>  
> >>  	dev->terminate = 0;
> >> +
> >> +	/* First byte should be set here, not after interrupt,
> >> +	 * because transmit-data-ready interrupt can come before
> >> +	 * NACK-interrupt during sending of previous message and
> >> +	 * ICDXR may have wrong data */
> >> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> >> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
> >> +		dev->buf_len--;
> >> +	}
> >> +
> >
> > I found an issue on DA850 while using this patch. 
> 
> ok, I'll drop this one for 2.6.34 until we figure this out.
> 
> > I think the above code should come after the
> > below lines because an I2C transaction is being carried out before configuring the I2C mode
> > register (which has bits to configure Master, Start condition etc), which causes undefined
> > behavior.
> 
> hmm, are you seeing this behavior on davinci git too?  This is the same
> patch that has been in davinci git for some time?
> 

Yes, I am seeing this issue on davinci git.

Regards,
Sudhakar

RE: [PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Sudhakar Rajashekhara]

Kevin,

On Wed, Jan 27, 2010 at 20:20:08, Kevin Hilman wrote:
> "Sudhakar Rajashekhara" <sudhakar.raj-l0cyMroinI0@public.gmane.org> writes:
> 
> > Hi,
> >
> > On Wed, Jan 27, 2010 at 05:11:28, Kevin Hilman wrote:
> >> From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
> >> 
> >> This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
> >> addresses.
> >> 
> >> Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
> >> Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> >> Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
> >> ---
> >>  drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
> >>  1 files changed, 25 insertions(+), 4 deletions(-)
> >> 
> >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> >> index c89687a..444a9f2 100644
> >> --- a/drivers/i2c/busses/i2c-davinci.c
> >> +++ b/drivers/i2c/busses/i2c-davinci.c
> >
> > [...]
> >
> >> @@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
> >>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
> >>  
> >>  	dev->terminate = 0;
> >> +
> >> +	/* First byte should be set here, not after interrupt,
> >> +	 * because transmit-data-ready interrupt can come before
> >> +	 * NACK-interrupt during sending of previous message and
> >> +	 * ICDXR may have wrong data */
> >> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
> >> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
> >> +		dev->buf_len--;
> >> +	}
> >> +
> >
> > I found an issue on DA850 while using this patch. 
> 
> ok, I'll drop this one for 2.6.34 until we figure this out.
> 
> > I think the above code should come after the
> > below lines because an I2C transaction is being carried out before configuring the I2C mode
> > register (which has bits to configure Master, Start condition etc), which causes undefined
> > behavior.
> 
> hmm, are you seeing this behavior on davinci git too?  This is the same
> patch that has been in davinci git for some time?
> 

The following patch fixes the issue I reported. I have tested it extensively on DA850/OMAP-L138.
You can roll-in these changes into the current patch.

Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj-l0cyMroinI0@public.gmane.org>
---
 drivers/i2c/busses/i2c-davinci.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
index 35f9daa..8526bce 100644
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c
@@ -308,6 +308,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 
 	dev->terminate = 0;
 
+	/* write the data into mode register */
+	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
+
 	/* First byte should be set here, not after interrupt,
 	 * because transmit-data-ready interrupt can come before
 	 * NACK-interrupt during sending of previous message and
@@ -317,9 +320,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
 		dev->buf_len--;
 	}
 
-	/* 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,
 						      dev->adapter.timeout);
 	if (r == 0) {
--

Regards,
Sudhakar

Re: [PATCH 1/6] i2c: davinci: Fix smbus Oops with AIC33 usage

[Kevin Hilman]

"Sudhakar Rajashekhara" <sudhakar.raj-l0cyMroinI0@public.gmane.org> writes:

> Kevin,
>
> On Wed, Jan 27, 2010 at 20:20:08, Kevin Hilman wrote:
>> "Sudhakar Rajashekhara" <sudhakar.raj-l0cyMroinI0@public.gmane.org> writes:
>> 
>> > Hi,
>> >
>> > On Wed, Jan 27, 2010 at 05:11:28, Kevin Hilman wrote:
>> >> From: Dirk Behme <dirk.behme-gM/Ye1E23mwN+BqQ9rBEUg@public.gmane.org>
>> >> 
>> >> This fixes Oops at kernel startup while "scanning" for TLV320AIC23IDx
>> >> addresses.
>> >> 
>> >> Signed-off-by: Alexander Vasiliev <alexvasiljev-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> Signed-off-by: Brad Griffis <bgriffis-l0cyMroinI0@public.gmane.org>
>> >> Signed-off-by: Dirk Behme <dirk.behme-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
>> >> Acked-by: Kevin Hilman <khilman-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>
>> >> ---
>> >>  drivers/i2c/busses/i2c-davinci.c |   29 +++++++++++++++++++++++++----
>> >>  1 files changed, 25 insertions(+), 4 deletions(-)
>> >> 
>> >> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> >> index c89687a..444a9f2 100644
>> >> --- a/drivers/i2c/busses/i2c-davinci.c
>> >> +++ b/drivers/i2c/busses/i2c-davinci.c
>> >
>> > [...]
>> >
>> >> @@ -290,6 +293,16 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>> >>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>> >>  
>> >>  	dev->terminate = 0;
>> >> +
>> >> +	/* First byte should be set here, not after interrupt,
>> >> +	 * because transmit-data-ready interrupt can come before
>> >> +	 * NACK-interrupt during sending of previous message and
>> >> +	 * ICDXR may have wrong data */
>> >> +	if ((!(msg->flags & I2C_M_RD)) && dev->buf_len) {
>> >> +		davinci_i2c_write_reg(dev, DAVINCI_I2C_DXR_REG, *dev->buf++);
>> >> +		dev->buf_len--;
>> >> +	}
>> >> +
>> >
>> > I found an issue on DA850 while using this patch. 
>> 
>> ok, I'll drop this one for 2.6.34 until we figure this out.
>> 
>> > I think the above code should come after the
>> > below lines because an I2C transaction is being carried out before configuring the I2C mode
>> > register (which has bits to configure Master, Start condition etc), which causes undefined
>> > behavior.
>> 
>> hmm, are you seeing this behavior on davinci git too?  This is the same
>> patch that has been in davinci git for some time?
>> 
>
> The following patch fixes the issue I reported. I have tested it extensively on DA850/OMAP-L138.
> You can roll-in these changes into the current patch.

Thanks, I rolled it into the original, and updated the 'davinci-upstream-submitted'
branch.

Kevin

> Signed-off-by: Sudhakar Rajashekhara <sudhakar.raj-l0cyMroinI0@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 35f9daa..8526bce 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -308,6 +308,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  
>  	dev->terminate = 0;
>  
> +	/* write the data into mode register */
> +	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> +
>  	/* First byte should be set here, not after interrupt,
>  	 * because transmit-data-ready interrupt can come before
>  	 * NACK-interrupt during sending of previous message and
> @@ -317,9 +320,6 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>  		dev->buf_len--;
>  	}
>  
> -	/* 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,
>  						      dev->adapter.timeout);
>  	if (r == 0) {
> --
>
> Regards,
> Sudhakar

Re: [PATCH 2/6] i2c: davinci: Remove MOD_REG_BIT and IO_ADDRESS usage

[Kevin Hilman]

Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org> writes:

> On Tue, Jan 26, 2010 at 03:41:29PM -0800, Kevin Hilman wrote:
>> From: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
>> 
>> Cleanup the DaVinci I2C driver. Remove MOD_REG_BIT macro.
>> Also use ioremap instead of IO_ADDRESS macro.
>
> would have been nicer to do each of these seperately.

ok, good to note for next time.  Can this version go through 
through as is.

Kevin

>> Signed-off-by: Chaithrika U S <chaithrika-l0cyMroinI0@public.gmane.org>
>> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-davinci.c |   77 +++++++++++++++++++-------------------
>>  1 files changed, 38 insertions(+), 39 deletions(-)
>> 
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 444a9f2..34a48d0 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -37,7 +37,6 @@
>>  #include <linux/io.h>
>>  
>>  #include <mach/hardware.h>
>> -
>>  #include <mach/i2c.h>
>>  
>>  /* ----- global defines ----------------------------------------------- */
>> @@ -71,37 +70,29 @@
>>  #define DAVINCI_I2C_IVR_NACK	0x02
>>  #define DAVINCI_I2C_IVR_AL	0x01
>>  
>> -#define DAVINCI_I2C_STR_BB	(1 << 12)
>> -#define DAVINCI_I2C_STR_RSFULL	(1 << 11)
>> -#define DAVINCI_I2C_STR_SCD	(1 << 5)
>> -#define DAVINCI_I2C_STR_ARDY	(1 << 2)
>> -#define DAVINCI_I2C_STR_NACK	(1 << 1)
>> -#define DAVINCI_I2C_STR_AL	(1 << 0)
>> -
>> -#define DAVINCI_I2C_MDR_NACK	(1 << 15)
>> -#define DAVINCI_I2C_MDR_STT	(1 << 13)
>> -#define DAVINCI_I2C_MDR_STP	(1 << 11)
>> -#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)
>> -#define DAVINCI_I2C_IMR_SCD	(1 << 5)
>> -#define DAVINCI_I2C_IMR_XRDY	(1 << 4)
>> -#define DAVINCI_I2C_IMR_RRDY	(1 << 3)
>> -#define DAVINCI_I2C_IMR_ARDY	(1 << 2)
>> -#define DAVINCI_I2C_IMR_NACK	(1 << 1)
>> -#define DAVINCI_I2C_IMR_AL	(1 << 0)
>> -
>> -#define MOD_REG_BIT(val, mask, set) do { \
>> -	if (set) { \
>> -		val |= mask; \
>> -	} else { \
>> -		val &= ~mask; \
>> -	} \
>> -} while (0)
>> +#define DAVINCI_I2C_STR_BB	BIT(12)
>> +#define DAVINCI_I2C_STR_RSFULL	BIT(11)
>> +#define DAVINCI_I2C_STR_SCD	BIT(5)
>> +#define DAVINCI_I2C_STR_ARDY	BIT(2)
>> +#define DAVINCI_I2C_STR_NACK	BIT(1)
>> +#define DAVINCI_I2C_STR_AL	BIT(0)
>> +
>> +#define DAVINCI_I2C_MDR_NACK	BIT(15)
>> +#define DAVINCI_I2C_MDR_STT	BIT(13)
>> +#define DAVINCI_I2C_MDR_STP	BIT(11)
>> +#define DAVINCI_I2C_MDR_MST	BIT(10)
>> +#define DAVINCI_I2C_MDR_TRX	BIT(9)
>> +#define DAVINCI_I2C_MDR_XA	BIT(8)
>> +#define DAVINCI_I2C_MDR_RM	BIT(7)
>> +#define DAVINCI_I2C_MDR_IRS	BIT(5)
>> +
>> +#define DAVINCI_I2C_IMR_AAS	BIT(6)
>> +#define DAVINCI_I2C_IMR_SCD	BIT(5)
>> +#define DAVINCI_I2C_IMR_XRDY	BIT(4)
>> +#define DAVINCI_I2C_IMR_RRDY	BIT(3)
>> +#define DAVINCI_I2C_IMR_ARDY	BIT(2)
>> +#define DAVINCI_I2C_IMR_NACK	BIT(1)
>> +#define DAVINCI_I2C_IMR_AL	BIT(0)
>
> surely 1 << x isn't too difficult to understand??
>   
>>  struct davinci_i2c_dev {
>>  	struct device           *dev;
>> @@ -155,7 +146,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>>  
>>  	/* put I2C into reset */
>>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 0);
>> +	w &= ~DAVINCI_I2C_MDR_IRS;
>>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>  
>>  	/* NOTE: I2C Clock divider programming info
>> @@ -205,7 +196,7 @@ static int i2c_davinci_init(struct davinci_i2c_dev *dev)
>>  
>>  	/* Take the I2C module out of reset: */
>>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> -	MOD_REG_BIT(w, DAVINCI_I2C_MDR_IRS, 1);
>> +	w |= DAVINCI_I2C_MDR_IRS;
>>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>  
>>  	/* Enable interrupts */
>> @@ -287,9 +278,9 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>  	/* Enable receive or transmit interrupts */
>>  	w = davinci_i2c_read_reg(dev, DAVINCI_I2C_IMR_REG);
>>  	if (msg->flags & I2C_M_RD)
>> -		MOD_REG_BIT(w, DAVINCI_I2C_IMR_RRDY, 1);
>> +		w |= DAVINCI_I2C_IMR_RRDY;
>>  	else
>> -		MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 1);
>> +		w |= DAVINCI_I2C_IMR_XRDY;
>>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_IMR_REG, w);
>>  
>>  	dev->terminate = 0;
>> @@ -346,7 +337,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>>  			return msg->len;
>>  		if (stop) {
>>  			w = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
>> -			MOD_REG_BIT(w, DAVINCI_I2C_MDR_STP, 1);
>> +			w |= DAVINCI_I2C_MDR_STP;
>>  			davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, w);
>>  		}
>>  		return -EREMOTEIO;
>> @@ -482,7 +473,7 @@ static irqreturn_t i2c_davinci_isr(int this_irq, void *dev_id)
>>  
>>  				w = davinci_i2c_read_reg(dev,
>>  							 DAVINCI_I2C_IMR_REG);
>> -				MOD_REG_BIT(w, DAVINCI_I2C_IMR_XRDY, 0);
>> +				w &= ~DAVINCI_I2C_IMR_XRDY;
>>  				davinci_i2c_write_reg(dev,
>>  						      DAVINCI_I2C_IMR_REG,
>>  						      w);
>> @@ -561,7 +552,12 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>  	}
>>  	clk_enable(dev->clk);
>>  
>> -	dev->base = (void __iomem *)IO_ADDRESS(mem->start);
>> +	dev->base = ioremap(mem->start, resource_size(mem));
>> +	if (!dev->base) {
>> +		r = -EBUSY;
>> +		goto err_mem_ioremap;
>> +	}
>> +
>
> this looks like you've got >1 change per patch?
>
>>  	i2c_davinci_init(dev);
>>  
>>  	r = request_irq(dev->irq, i2c_davinci_isr, 0, pdev->name, dev);
>> @@ -591,6 +587,8 @@ static int davinci_i2c_probe(struct platform_device *pdev)
>>  err_free_irq:
>>  	free_irq(dev->irq, dev);
>>  err_unuse_clocks:
>> +	iounmap(dev->base);
>> +err_mem_ioremap:
>>  	clk_disable(dev->clk);
>>  	clk_put(dev->clk);
>>  	dev->clk = NULL;
>> @@ -619,6 +617,7 @@ static int davinci_i2c_remove(struct platform_device *pdev)
>>  
>>  	davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, 0);
>>  	free_irq(IRQ_I2C, dev);
>> +	iounmap(dev->base);
>>  	kfree(dev);
>>  
>>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> -- 
>> 1.6.6
>> 
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
> -- 
> Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)
>
>   'a smiley only costs 4 bytes'

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Nori, Sekhar]

Hi Philby,

On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
> From: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>
> Come out of i2c time out condition by following the
> bus recovery procedure outlined in the i2c protocol v3 spec.
> The kernel must be robust enough to gracefully recover
> from i2c bus failure without having to reset the machine.
> This is done by first NACKing the slave, pulsing the SCL
> line 9 times and then sending the stop command.
>
> This patch has been tested on a DM6446 and DM355
>
> Signed-off-by: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
> Signed-off-by: Srinivasan, Nageswari <nageswari-l0cyMroinI0@public.gmane.org>
> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   57 +++++++++++++++++++++++++++++++++++--
>  1 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
> index 35f9daa..5459065 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -36,6 +36,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/cpufreq.h>
> +#include <linux/gpio.h>
>
>  #include <mach/hardware.h>
>  #include <mach/i2c.h>
> @@ -43,6 +44,7 @@
>  /* ----- global defines ----------------------------------------------- */
>
>  #define DAVINCI_I2C_TIMEOUT  (1*HZ)
> +#define DAVINCI_I2C_MAX_TRIES        2
>  #define I2C_DAVINCI_INTR_ALL    (DAVINCI_I2C_IMR_AAS | \
>                                DAVINCI_I2C_IMR_SCD | \
>                                DAVINCI_I2C_IMR_ARDY | \
> @@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>       return __raw_readw(i2c_dev->base + reg);
>  }
>
> +/* Generate a pulse on the i2c clock pin. */
> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
> +{
> +     u16 i;
> +
> +     if (scl_pin) {
> +             /* Send high and low on the SCL line */
> +             for (i = 0; i < 9; i++) {
> +                     gpio_set_value(scl_pin, 0);
> +                     udelay(20);
> +                     gpio_set_value(scl_pin, 1);
> +                     udelay(20);
> +             }

Before using the pins as GPIO, you would have to set the
functionality of these pins as GPIO. You had this code in
previous incarnations of this patch - not sure why it is
dropped now.

Couple of good to haves:

It will be good to do a gpio_request() before using the pins
as GPIO - though I can see it may have been deemed unnecessary
- the pins are owned by I2C already - even so it may help catch
system configuration errors in later platforms.

The I2C peripheral on da8xx itself contains a mode where its
pins could be used as GPIO - so no need for SoC level muxing
and need for the platform data. This seems to be missing from
DM355 though. Thankfully there is a revision id within the I2C
memory map which will help you differentiate the two cases
(revision 0x5 vs 0x6)

Thanks,
Sekhar

> +     }
> +}
> +
> +/* This routine does i2c bus recovery as specified in the
> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> + */
> +static void i2c_recover_bus(struct davinci_i2c_dev *dev)
> +{
> +     u32 flag = 0;
> +     struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
> +
> +     dev_err(dev->dev, "initiating i2c bus recovery\n");
> +     /* Send NACK to the slave */
> +     flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> +     flag |=  DAVINCI_I2C_MDR_NACK;
> +     /* write the data into mode register */
> +     davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> +     if (pdata)
> +             generic_i2c_clock_pulse(pdata->scl_pin);
> +     /* Send STOP */
> +     flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> +     flag |= DAVINCI_I2C_MDR_STP;
> +     davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> +}
> +
>  static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>                                                               int val)
>  {
> @@ -235,14 +275,22 @@ static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
>                                        char allow_sleep)
>  {
>       unsigned long timeout;
> +     static u16 to_cnt;
>
>       timeout = jiffies + dev->adapter.timeout;
>       while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
>              & DAVINCI_I2C_STR_BB) {
> -             if (time_after(jiffies, timeout)) {
> -                     dev_warn(dev->dev,
> -                              "timeout waiting for bus ready\n");
> -                     return -ETIMEDOUT;
> +             if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
> +                     if (time_after(jiffies, timeout)) {
> +                             dev_warn(dev->dev,
> +                             "timeout waiting for bus ready\n");
> +                             to_cnt++;
> +                             return -ETIMEDOUT;
> +                     } else {
> +                             to_cnt = 0;
> +                             i2c_recover_bus(dev);
> +                             i2c_davinci_init(dev);
> +                     }
>               }
>               if (allow_sleep)
>                       schedule_timeout(1);
> @@ -324,6 +372,7 @@ i2c_davinci_xfer_msg(struct i2c_adapter *adap, struct i2c_msg *msg, int stop)
>                                                     dev->adapter.timeout);
>       if (r == 0) {
>               dev_err(dev->dev, "controller timed out\n");
> +             i2c_recover_bus(dev);
>               i2c_davinci_init(dev);
>               dev->buf_len = 0;
>               return -ETIMEDOUT;
> --
> 1.6.6
>
> _______________________________________________
> Davinci-linux-open-source mailing list
> Davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org
> http://linux.davincidsp.com/mailman/listinfo/davinci-linux-open-source
>

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Kevin Hilman]

"Nori, Sekhar" <nsekhar-l0cyMroinI0@public.gmane.org> writes:

> Hi Philby,
>
> On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
>> From: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>>
>> Come out of i2c time out condition by following the
>> bus recovery procedure outlined in the i2c protocol v3 spec.
>> The kernel must be robust enough to gracefully recover
>> from i2c bus failure without having to reset the machine.
>> This is done by first NACKing the slave, pulsing the SCL
>> line 9 times and then sending the stop command.
>>
>> This patch has been tested on a DM6446 and DM355
>>
>> Signed-off-by: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>> Signed-off-by: Srinivasan, Nageswari <nageswari-l0cyMroinI0@public.gmane.org>
>> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-davinci.c |   57 +++++++++++++++++++++++++++++++++++--
>>  1 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 35f9daa..5459065 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/io.h>
>>  #include <linux/cpufreq.h>
>> +#include <linux/gpio.h>
>>
>>  #include <mach/hardware.h>
>>  #include <mach/i2c.h>
>> @@ -43,6 +44,7 @@
>>  /* ----- global defines ----------------------------------------------- */
>>
>>  #define DAVINCI_I2C_TIMEOUT  (1*HZ)
>> +#define DAVINCI_I2C_MAX_TRIES        2
>>  #define I2C_DAVINCI_INTR_ALL    (DAVINCI_I2C_IMR_AAS | \
>>                                DAVINCI_I2C_IMR_SCD | \
>>                                DAVINCI_I2C_IMR_ARDY | \
>> @@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>>       return __raw_readw(i2c_dev->base + reg);
>>  }
>>
>> +/* Generate a pulse on the i2c clock pin. */
>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>> +{
>> +     u16 i;
>> +
>> +     if (scl_pin) {
>> +             /* Send high and low on the SCL line */
>> +             for (i = 0; i < 9; i++) {
>> +                     gpio_set_value(scl_pin, 0);
>> +                     udelay(20);
>> +                     gpio_set_value(scl_pin, 1);
>> +                     udelay(20);
>> +             }
>
> Before using the pins as GPIO, you would have to set the
> functionality of these pins as GPIO. You had this code in
> previous incarnations of this patch - not sure why it is
> dropped now.
>
> Couple of good to haves:
>
> It will be good to do a gpio_request() before using the pins
> as GPIO - though I can see it may have been deemed unnecessary
> - the pins are owned by I2C already - even so it may help catch
> system configuration errors in later platforms.
>
> The I2C peripheral on da8xx itself contains a mode where its
> pins could be used as GPIO - so no need for SoC level muxing
> and need for the platform data. This seems to be missing from
> DM355 though. Thankfully there is a revision id within the I2C
> memory map which will help you differentiate the two cases
> (revision 0x5 vs 0x6)
>

Philby,

Please update with comments from Sekhar soon so I can get this one queued
for 2.6.34.

Kevin

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

Hello Sekhar,

My apologies for the late mail. Had trouble with our mail server and I
seem to have lost mails. Pulling this thread from the list. Comments
inline...


On Mon, Feb 1, 2010 at 11:27 AM, Nori, Sekhar <nsekhar-l0cyMroinI0@public.gmane.org> wrote:
> Hi Philby,
>
> On Wed, Jan 27, 2010 at 05:11:33, Kevin Hilman wrote:
>> From: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>>
>> Come out of i2c time out condition by following the
>> bus recovery procedure outlined in the i2c protocol v3 spec.
>> The kernel must be robust enough to gracefully recover
>> from i2c bus failure without having to reset the machine.
>> This is done by first NACKing the slave, pulsing the SCL
>> line 9 times and then sending the stop command.
>>
>> This patch has been tested on a DM6446 and DM355
>>
>> Signed-off-by: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>> Signed-off-by: Srinivasan, Nageswari <nageswari-l0cyMroinI0@public.gmane.org>
>> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
>> ---
>>  drivers/i2c/busses/i2c-davinci.c |   57 +++++++++++++++++++++++++++++++++++--
>>  1 files changed, 53 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-davinci.c b/drivers/i2c/busses/i2c-davinci.c
>> index 35f9daa..5459065 100644
>> --- a/drivers/i2c/busses/i2c-davinci.c
>> +++ b/drivers/i2c/busses/i2c-davinci.c
>> @@ -36,6 +36,7 @@
>>  #include <linux/platform_device.h>
>>  #include <linux/io.h>
>>  #include <linux/cpufreq.h>
>> +#include <linux/gpio.h>
>>
>>  #include <mach/hardware.h>
>>  #include <mach/i2c.h>
>> @@ -43,6 +44,7 @@
>>  /* ----- global defines ----------------------------------------------- */
>>
>>  #define DAVINCI_I2C_TIMEOUT  (1*HZ)
>> +#define DAVINCI_I2C_MAX_TRIES        2
>>  #define I2C_DAVINCI_INTR_ALL    (DAVINCI_I2C_IMR_AAS | \
>>                                DAVINCI_I2C_IMR_SCD | \
>>                                DAVINCI_I2C_IMR_ARDY | \
>> @@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct davinci_i2c_dev *i2c_dev, int reg)
>>       return __raw_readw(i2c_dev->base + reg);
>>  }
>>
>> +/* Generate a pulse on the i2c clock pin. */
>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>> +{
>> +     u16 i;
>> +
>> +     if (scl_pin) {
>> +             /* Send high and low on the SCL line */
>> +             for (i = 0; i < 9; i++) {
>> +                     gpio_set_value(scl_pin, 0);
>> +                     udelay(20);
>> +                     gpio_set_value(scl_pin, 1);
>> +                     udelay(20);
>> +             }
>
> Before using the pins as GPIO, you would have to set the
> functionality of these pins as GPIO. You had this code in
> previous incarnations of this patch - not sure why it is
> dropped now.
>

Don't seem to remember having the code in the old versions at least
not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
enable_i2c_pins() were discarded as the i2c protocol spec. did not
specify the need. Moreover bus recovered without it. (Tested on DM355
and Dm6446).

> Couple of good to haves:
>
> It will be good to do a gpio_request() before using the pins
> as GPIO - though I can see it may have been deemed unnecessary
> - the pins are owned by I2C already - even so it may help catch
> system configuration errors in later platforms.

Yes, I could use gpio_request() in generic_i2c_clock_pulse().

>
> The I2C peripheral on da8xx itself contains a mode where its
> pins could be used as GPIO - so no need for SoC level muxing
> and need for the platform data. This seems to be missing from
> DM355 though. Thankfully there is a revision id within the I2C
> memory map which will help you differentiate the two cases
> (revision 0x5 vs 0x6)

I did not entirely follow your above statement. Will usage of
gpio_request() solve the problem for da8xx and DM355 or does it
require a if else condition? A reminder that the present code will
only work for DM6446 and DM355. I do not have a DA8xx to test specific
functionality if it were to be added.

Regards,
Philby

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Nori, Sekhar]

Hi Philby,

On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
> Hello Sekhar,
>

[...]

> >> +/* Generate a pulse on the i2c clock pin. */
> >> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
> >> +{
> >> +     u16 i;
> >> +
> >> +     if (scl_pin) {
> >> +             /* Send high and low on the SCL line */
> >> +             for (i = 0; i < 9; i++) {
> >> +                     gpio_set_value(scl_pin, 0);
> >> +                     udelay(20);
> >> +                     gpio_set_value(scl_pin, 1);
> >> +                     udelay(20);
> >> +             }
> >
> > Before using the pins as GPIO, you would have to set the
> > functionality of these pins as GPIO. You had this code in
> > previous incarnations of this patch - not sure why it is
> > dropped now.
> >
>
> Don't seem to remember having the code in the old versions at least
> not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
> enable_i2c_pins() were discarded as the i2c protocol spec. did not
> specify the need. Moreover bus recovered without it. (Tested on DM355
> and Dm6446).

Yes, I was referring to the davinci_cfg_reg() calls in
{disable|enable}_i2c_pins() functions. Per the specification
of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
it is to be used as GPIO controlled by GPIO module. It may
have worked on couple of devices but cannot be guaranteed to
work on all DaVinci devices (esp. DA8XX ones).

>
> > Couple of good to haves:
> >

[...]

> >
> > The I2C peripheral on da8xx itself contains a mode where its
> > pins could be used as GPIO - so no need for SoC level muxing
> > and need for the platform data. This seems to be missing from
> > DM355 though. Thankfully there is a revision id within the I2C
> > memory map which will help you differentiate the two cases
> > (revision 0x5 vs 0x6)
>
> I did not entirely follow your above statement. Will usage of
> gpio_request() solve the problem for da8xx and DM355 or does it
> require a if else condition?

You require special casing for I2C version 0x6.
Have a look here: http://focus.ti.com/lit/ug/sprufl9a/sprufl9a.pdf
I was referring to registers described in sections
3.15-3.20

> A reminder that the present code will
> only work for DM6446 and DM355. I do not have a DA8xx to test specific
> functionality if it were to be added.

Right. It is only an enhancement (and only good
to have at that). This should not stop the current
patch from getting in.

Thanks,
Sekhar

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

Hello Sekhar,

On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
> Hi Philby,
>
> On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
>> Hello Sekhar,
>>
>
> [...]
>
>>>> +/* Generate a pulse on the i2c clock pin. */
>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>> +{
>>>> +     u16 i;
>>>> +
>>>> +     if (scl_pin) {
>>>> +             /* Send high and low on the SCL line */
>>>> +             for (i = 0; i<  9; i++) {
>>>> +                     gpio_set_value(scl_pin, 0);
>>>> +                     udelay(20);
>>>> +                     gpio_set_value(scl_pin, 1);
>>>> +                     udelay(20);
>>>> +             }
>>>
>>> Before using the pins as GPIO, you would have to set the
>>> functionality of these pins as GPIO. You had this code in
>>> previous incarnations of this patch - not sure why it is
>>> dropped now.
>>>
>>
>> Don't seem to remember having the code in the old versions at least
>> not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
>> enable_i2c_pins() were discarded as the i2c protocol spec. did not
>> specify the need. Moreover bus recovered without it. (Tested on DM355
>> and Dm6446).
>
> Yes, I was referring to the davinci_cfg_reg() calls in
> {disable|enable}_i2c_pins() functions. Per the specification
> of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
> it is to be used as GPIO controlled by GPIO module. It may
> have worked on couple of devices but cannot be guaranteed to
> work on all DaVinci devices (esp. DA8XX ones).
>
>>
>>> Couple of good to haves:
>>>
>
> [...]
>
>>>
>>> The I2C peripheral on da8xx itself contains a mode where its
>>> pins could be used as GPIO - so no need for SoC level muxing
>>> and need for the platform data. This seems to be missing from
>>> DM355 though. Thankfully there is a revision id within the I2C
>>> memory map which will help you differentiate the two cases
>>> (revision 0x5 vs 0x6)
>>
>> I did not entirely follow your above statement. Will usage of
>> gpio_request() solve the problem for da8xx and DM355 or does it
>> require a if else condition?
>
> You require special casing for I2C version 0x6.
> Have a look here: http://focus.ti.com/lit/ug/sprufl9a/sprufl9a.pdf
> I was referring to registers described in sections
> 3.15-3.20
>
>> A reminder that the present code will
>> only work for DM6446 and DM355. I do not have a DA8xx to test specific
>> functionality if it were to be added.
>
> Right. It is only an enhancement (and only good
> to have at that). This should not stop the current
> patch from getting in.

Thanks for clarifying this. I will get a patch out for this in a day or two.

Regards,
Philby

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

Hello Sekhar,

On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>> +{
>>>> +     u16 i;
>>>> +
>>>> +     if (scl_pin) {
>>>> +             /* Send high and low on the SCL line */
>>>> +             for (i = 0; i<  9; i++) {
>>>> +                     gpio_set_value(scl_pin, 0);
>>>> +                     udelay(20);
>>>> +                     gpio_set_value(scl_pin, 1);
>>>> +                     udelay(20);
>>>> +             }
>>>
>>> Before using the pins as GPIO, you would have to set the
>>> functionality of these pins as GPIO. You had this code in
>>> previous incarnations of this patch - not sure why it is
>>> dropped now.
>>>
>>
>> Don't seem to remember having the code in the old versions at least
>> not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
>> enable_i2c_pins() were discarded as the i2c protocol spec. did not
>> specify the need. Moreover bus recovered without it. (Tested on DM355
>> and Dm6446).
>
> Yes, I was referring to the davinci_cfg_reg() calls in
> {disable|enable}_i2c_pins() functions. Per the specification
> of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
> it is to be used as GPIO controlled by GPIO module. It may
> have worked on couple of devices but cannot be guaranteed to
> work on all DaVinci devices (esp. DA8XX ones).


I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the 
wrong place to put it. This would require adding davinci_cfg_reg() for 
all know davinci platforms. The i2c recovery procedure is correct to 
assume that it owns the SCL line at that very moment.

Instead I believe pinmuxing using davinci_cfg_reg(), should be done way 
early, just like we do for DM6446 in devices.c --> davinci_init_i2c(), 
for all other platforms. What I could do in function 
generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request() 
by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.

Let me know what you think.

Regards,
Philby

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Nori, Sekhar]

On Mon, Feb 08, 2010 at 20:43:27, Philby John wrote:
> Hello Sekhar,
>
> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
> >>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
> >>>> +{
> >>>> +     u16 i;
> >>>> +
> >>>> +     if (scl_pin) {
> >>>> +             /* Send high and low on the SCL line */
> >>>> +             for (i = 0; i<  9; i++) {
> >>>> +                     gpio_set_value(scl_pin, 0);
> >>>> +                     udelay(20);
> >>>> +                     gpio_set_value(scl_pin, 1);
> >>>> +                     udelay(20);
> >>>> +             }
> >>>
> >>> Before using the pins as GPIO, you would have to set the
> >>> functionality of these pins as GPIO. You had this code in
> >>> previous incarnations of this patch - not sure why it is
> >>> dropped now.
> >>>
> >>
> >> Don't seem to remember having the code in the old versions at least
> >> not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
> >> enable_i2c_pins() were discarded as the i2c protocol spec. did not
> >> specify the need. Moreover bus recovered without it. (Tested on DM355
> >> and Dm6446).
> >
> > Yes, I was referring to the davinci_cfg_reg() calls in
> > {disable|enable}_i2c_pins() functions. Per the specification
> > of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
> > it is to be used as GPIO controlled by GPIO module. It may
> > have worked on couple of devices but cannot be guaranteed to
> > work on all DaVinci devices (esp. DA8XX ones).
>
>
> I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
> wrong place to put it. This would require adding davinci_cfg_reg() for
> all know davinci platforms. The i2c recovery procedure is correct to
> assume that it owns the SCL line at that very moment.
>
> Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
> early, just like we do for DM6446 in devices.c --> davinci_init_i2c(),
> for all other platforms. What I could do in function
> generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
> by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.

But, the pins should remain as I2C pins till you actually
hit a bus lock-up. That's when you need to convert them to GPIO
pins and start the recovery by pulsing SCL.

It you make them GPIO right at the start, they wont be usable
as I2C pins for normal transfers?

Thanks,
Sekhar

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

On 02/08/2010 09:33 PM, Nori, Sekhar wrote:
> On Mon, Feb 08, 2010 at 20:43:27, Philby John wrote:
>> Hello Sekhar,
>>
>> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
>>>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>>>> +{
>>>>>> +     u16 i;
>>>>>> +
>>>>>> +     if (scl_pin) {
>>>>>> +             /* Send high and low on the SCL line */
>>>>>> +             for (i = 0; i<   9; i++) {
>>>>>> +                     gpio_set_value(scl_pin, 0);
>>>>>> +                     udelay(20);
>>>>>> +                     gpio_set_value(scl_pin, 1);
>>>>>> +                     udelay(20);
>>>>>> +             }
>>>>>
>>>>> Before using the pins as GPIO, you would have to set the
>>>>> functionality of these pins as GPIO. You had this code in
>>>>> previous incarnations of this patch - not sure why it is
>>>>> dropped now.
>>>>>
>>>>
>>>> Don't seem to remember having the code in the old versions at least
>>>> not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
>>>> enable_i2c_pins() were discarded as the i2c protocol spec. did not
>>>> specify the need. Moreover bus recovered without it. (Tested on DM355
>>>> and Dm6446).
>>>
>>> Yes, I was referring to the davinci_cfg_reg() calls in
>>> {disable|enable}_i2c_pins() functions. Per the specification
>>> of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
>>> it is to be used as GPIO controlled by GPIO module. It may
>>> have worked on couple of devices but cannot be guaranteed to
>>> work on all DaVinci devices (esp. DA8XX ones).
>>
>>
>> I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
>> wrong place to put it. This would require adding davinci_cfg_reg() for
>> all know davinci platforms. The i2c recovery procedure is correct to
>> assume that it owns the SCL line at that very moment.
>>
>> Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
>> early, just like we do for DM6446 in devices.c -->  davinci_init_i2c(),
>> for all other platforms. What I could do in function
>> generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
>> by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.
>
> But, the pins should remain as I2C pins till you actually
> hit a bus lock-up. That's when you need to convert them to GPIO
> pins and start the recovery by pulsing SCL.
>
> It you make them GPIO right at the start, they wont be usable
> as I2C pins for normal transfers?


Right. I was also hoping to rid of cpu_is_xxx usage. The only other way 
I could think of is to add pinmux index into i2c platform data struct. 
What do you think is the best approach?


Regards,
Philby

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Nori, Sekhar]

On Tue, Feb 09, 2010 at 15:45:15, Philby John wrote:
> On 02/08/2010 09:33 PM, Nori, Sekhar wrote:
> > On Mon, Feb 08, 2010 at 20:43:27, Philby John wrote:
> >> Hello Sekhar,
> >>
> >> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
> >>>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
> >>>>>> +{
> >>>>>> +     u16 i;
> >>>>>> +
> >>>>>> +     if (scl_pin) {
> >>>>>> +             /* Send high and low on the SCL line */
> >>>>>> +             for (i = 0; i<   9; i++) {
> >>>>>> +                     gpio_set_value(scl_pin, 0);
> >>>>>> +                     udelay(20);
> >>>>>> +                     gpio_set_value(scl_pin, 1);
> >>>>>> +                     udelay(20);
> >>>>>> +             }
> >>>>>
> >>>>> Before using the pins as GPIO, you would have to set the
> >>>>> functionality of these pins as GPIO. You had this code in
> >>>>> previous incarnations of this patch - not sure why it is
> >>>>> dropped now.
> >>>>>
> >>>>
> >>>> Don't seem to remember having the code in the old versions at least
> >>>> not in generic_i2c_clock_pulse(). The functions disable_i2c_pins() and
> >>>> enable_i2c_pins() were discarded as the i2c protocol spec. did not
> >>>> specify the need. Moreover bus recovered without it. (Tested on DM355
> >>>> and Dm6446).
> >>>
> >>> Yes, I was referring to the davinci_cfg_reg() calls in
> >>> {disable|enable}_i2c_pins() functions. Per the specification
> >>> of the DaVinci devices, a pin needs to be muxed as 'GPIO' if
> >>> it is to be used as GPIO controlled by GPIO module. It may
> >>> have worked on couple of devices but cannot be guaranteed to
> >>> work on all DaVinci devices (esp. DA8XX ones).
> >>
> >>
> >> I think that using davinci_cfg_reg() in generic_i2c_clock_pulse() is the
> >> wrong place to put it. This would require adding davinci_cfg_reg() for
> >> all know davinci platforms. The i2c recovery procedure is correct to
> >> assume that it owns the SCL line at that very moment.
> >>
> >> Instead I believe pinmuxing using davinci_cfg_reg(), should be done way
> >> early, just like we do for DM6446 in devices.c -->  davinci_init_i2c(),
> >> for all other platforms. What I could do in function
> >> generic_i2c_clock_pulse() is, set SCL to output, and use gpio_request()
> >> by checking REVID2 register value (0x6) for DA8xx and 0x5 for others.
> >
> > But, the pins should remain as I2C pins till you actually
> > hit a bus lock-up. That's when you need to convert them to GPIO
> > pins and start the recovery by pulsing SCL.
> >
> > It you make them GPIO right at the start, they wont be usable
> > as I2C pins for normal transfers?
>
>
> Right. I was also hoping to rid of cpu_is_xxx usage. The only other way
> I could think of is to add pinmux index into i2c platform data struct.
> What do you think is the best approach?
>

I think passing pinmux index through platform data is fair.

Thanks,
Sekhar

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Griffis, Brad]

> > Right. I was also hoping to rid of cpu_is_xxx usage. The only other way
> > I could think of is to add pinmux index into i2c platform data struct.
> > What do you think is the best approach?
> >
> 
> I think passing pinmux index through platform data is fair.
> 
> Thanks,
> Sekhar

I recently was told of a clever solution for this issue which I documented here:

http://wiki.davincidsp.com/index.php/I2C_Tips#External_Slave_Device_Hanging_the_Bus_by_Holding_SDA_Low

Basically the solution was to switch to "free data format" and perform a read.  This will cause the I2C to start toggling SCL.  I mention it here because I think the "free data format" mode is available on most processors.  (The only device I know that does NOT support "free data format" is OMAP35x.)  You might have a lot less processor-specific code with this approach and it would be applicable to more devices.

I don't have any time to write the code/patch myself, but I thought I would at least toss the idea out there.

Brad

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
> Hi Philby,
>
> On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
>> Hello Sekhar,
>>
>
> [...]
>
>>>> +/* Generate a pulse on the i2c clock pin. */
>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>> +{
>>>> +     u16 i;
>>>> +
>>>> +     if (scl_pin) {
>>>> +             /* Send high and low on the SCL line */
>>>> +             for (i = 0; i<  9; i++) {
>>>> +                     gpio_set_value(scl_pin, 0);
>>>> +                     udelay(20);
>>>> +                     gpio_set_value(scl_pin, 1);
>>>> +                     udelay(20);
>>>> +             }
>>>
>>> Before using the pins as GPIO, you would have to set the
>>> functionality of these pins as GPIO. You had this code in
>>> previous incarnations of this patch - not sure why it is
>>> dropped now.
>>>

I now think that the previous versions were incorrect since 
davinci_cfg_reg() does not set the scl or sda pins for gpio 
functionality. Instead they set them as scl or sda which is not what we 
want at the time of pulsing. The previous versions used gpio_set_value() 
in disable_i2c_pins() and then called davinci_cfg_reg(). After which it 
called pulse_i2c_clock().

Please correct me if my interpretation of the code is incorrect.

Regards,
Philby

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

On 03/05/2010 08:50 PM, Griffis, Brad wrote:
>>> Right. I was also hoping to rid of cpu_is_xxx usage. The only other way
>>> I could think of is to add pinmux index into i2c platform data struct.
>>> What do you think is the best approach?
>>>
>>
>> I think passing pinmux index through platform data is fair.
>>
>> Thanks,
>> Sekhar
>
> I recently was told of a clever solution for this issue which I documented here:
>
> http://wiki.davincidsp.com/index.php/I2C_Tips#External_Slave_Device_Hanging_the_Bus_by_Holding_SDA_Low
>
> Basically the solution was to switch to "free data format" and perform a read.  This will cause the I2C to start toggling SCL.  I mention it here because I think the "free data format" mode is available on most processors.  (The only device I know that does NOT support "free data format" is OMAP35x.)  You might have a lot less processor-specific code with this approach and it would be applicable to more devices.
>
> I don't have any time to write the code/patch myself, but I thought I would at least toss the idea out there.
>
> Brad
>

I did go through your document, but what does "free data format" mean? 
It would be good to expand the procedure that enables you to move into 
this mode. If this wouldn't require modfying pinmux settings shouldn't 
it be part of the core i2c implementation?

At present we follow the i2c spec. recovery procedure which you 
explained in method 1, and as per AN10216-01 I2C Manual is ...

•SDA line is then non usable anymore because of the
“Slave-Transmitter”mode.
•Methods to recover the SDA line are:
–Reset the slave device (assuming the device has a Reset pin)
–Use a bus recovery sequence to leave the “Slave-Transmitter” mode
•Bus recovery sequence is done as following:
1-Send 9 clock pulses on SCL line
2-Ask the master to keep SDA High until the “Slave-Transmitter” releases
the SDA line to perform the ACK operation
3-Keeping SDA High during the ACK means that the “Master-Receiver”does
not acknowledge the previous byte receive
4-The “Slave-Transmitter” then goes in an idle state
5-The master then sends a STOP command initializing completely the bus

Regards,
Philby

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Griffis, Brad]

> -----Original Message-----
> From: Philby John [mailto:pjohn-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org]
> Sent: Monday, March 08, 2010 7:37 AM
> To: Griffis, Brad
> Cc: Nori, Sekhar; davinci-linux-open-source-VycZQUHpC/PFrsHnngEfi1aTQe2KTcn/@public.gmane.org; linux-
> i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Subject: Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the
> bus
> 
> I did go through your document, but what does "free data format" mean?
> It would be good to expand the procedure that enables you to move into
> this mode. If this wouldn't require modfying pinmux settings shouldn't
> it be part of the core i2c implementation?

Just to make sure my point was clear, I think the GPIO method is simpler/easier if you're just looking at a single device and assuming that device has muxed the I2C with GPIO.  That said, my method is a little more complicated/convoluted when looking at a single device, but I think the code would scale better across the entire Davinci tree since it requires no knowledge of whether the pin-muxing option is available and how the pin-muxing is implemented for that particular device.

You enter the "free data format" mode by setting the FDF bit in the ICMDR register.  It is described in Section 2.6.3 of the OMAP-L138 I2C Guide:

http://www.ti.com/lit/sprufl9

The advantage of using this mode is that you would not require any device-specific pin-muxing knowledge/changes.  The entire recovery can occur within the context of the I2C controller.

So to do a read in the "free data format" mode you would write ICMDR.FDF = 1 (free data format), ICMDR.TRX = 0 (read), ICMDR.STT = 1 (start), ICMDR.STP = 1.  This should cause it to clock in 8 bits of data plus an ack, freeing up the bus.

Brad

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Kevin Hilman]

Philby John <pjohn-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org> writes:

> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
>> Hi Philby,
>>
>> On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
>>> Hello Sekhar,
>>>
>>
>> [...]
>>
>>>>> +/* Generate a pulse on the i2c clock pin. */
>>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>>> +{
>>>>> +     u16 i;
>>>>> +
>>>>> +     if (scl_pin) {
>>>>> +             /* Send high and low on the SCL line */
>>>>> +             for (i = 0; i<  9; i++) {
>>>>> +                     gpio_set_value(scl_pin, 0);
>>>>> +                     udelay(20);
>>>>> +                     gpio_set_value(scl_pin, 1);
>>>>> +                     udelay(20);
>>>>> +             }
>>>>
>>>> Before using the pins as GPIO, you would have to set the
>>>> functionality of these pins as GPIO. You had this code in
>>>> previous incarnations of this patch - not sure why it is
>>>> dropped now.
>>>>
>
> I now think that the previous versions were incorrect since
> davinci_cfg_reg() does not set the scl or sda pins for gpio
> functionality. Instead they set them as scl or sda which is not what
> we want at the time of pulsing. The previous versions used
> gpio_set_value() in disable_i2c_pins() and then called
> davinci_cfg_reg(). After which it called pulse_i2c_clock().
>
> Please correct me if my interpretation of the code is incorrect.

Can we get some resolution here?

I have a queue of davinci i2c patches waiting to go upstream, and I'd
like to get them in for 2.6.35.

The current i2c series is in the 'davinci-i2c' branch of davinci git.

Please submit an updated version so we can get this stuff upstream.

Kevin

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

On 03/17/2010 02:20 AM, Kevin Hilman wrote:
> Philby John<pjohn-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>  writes:
>
>> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
>>> Hi Philby,
>>>
>>> On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
>>>> Hello Sekhar,
>>>>
>>>
>>> [...]
>>>
>>>>>> +/* Generate a pulse on the i2c clock pin. */
>>>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>>>> +{
>>>>>> +     u16 i;
>>>>>> +
>>>>>> +     if (scl_pin) {
>>>>>> +             /* Send high and low on the SCL line */
>>>>>> +             for (i = 0; i<   9; i++) {
>>>>>> +                     gpio_set_value(scl_pin, 0);
>>>>>> +                     udelay(20);
>>>>>> +                     gpio_set_value(scl_pin, 1);
>>>>>> +                     udelay(20);
>>>>>> +             }
>>>>>
>>>>> Before using the pins as GPIO, you would have to set the
>>>>> functionality of these pins as GPIO. You had this code in
>>>>> previous incarnations of this patch - not sure why it is
>>>>> dropped now.
>>>>>
>>
>> I now think that the previous versions were incorrect since
>> davinci_cfg_reg() does not set the scl or sda pins for gpio
>> functionality. Instead they set them as scl or sda which is not what
>> we want at the time of pulsing. The previous versions used
>> gpio_set_value() in disable_i2c_pins() and then called
>> davinci_cfg_reg(). After which it called pulse_i2c_clock().
>>
>> Please correct me if my interpretation of the code is incorrect.
>
> Can we get some resolution here?
>
> I have a queue of davinci i2c patches waiting to go upstream, and I'd
> like to get them in for 2.6.35.
>
> The current i2c series is in the 'davinci-i2c' branch of davinci git.

To quote Sekhar, "...Right. It is only an enhancement (and only good
to have at that). This should not stop the current patch from getting 
in." So this patch is good to make it to 2.6.35

>
> Please submit an updated version so we can get this stuff upstream.

Right, the enhancement has now been sent.

Regards,
Philby

RE: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Nori, Sekhar]

Hi Philby,

On Wed, Mar 17, 2010 at 16:58:44, Philby John wrote:
> On 03/17/2010 02:20 AM, Kevin Hilman wrote:
> > Philby John<pjohn-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>  writes:
> >
> >> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
> >>> Hi Philby,
> >>>
> >>> On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
> >>>> Hello Sekhar,
> >>>>
> >>>
> >>> [...]
> >>>
> >>>>>> +/* Generate a pulse on the i2c clock pin. */
> >>>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
> >>>>>> +{
> >>>>>> +     u16 i;
> >>>>>> +
> >>>>>> +     if (scl_pin) {
> >>>>>> +             /* Send high and low on the SCL line */
> >>>>>> +             for (i = 0; i<   9; i++) {
> >>>>>> +                     gpio_set_value(scl_pin, 0);
> >>>>>> +                     udelay(20);
> >>>>>> +                     gpio_set_value(scl_pin, 1);
> >>>>>> +                     udelay(20);
> >>>>>> +             }
> >>>>>
> >>>>> Before using the pins as GPIO, you would have to set the
> >>>>> functionality of these pins as GPIO. You had this code in
> >>>>> previous incarnations of this patch - not sure why it is
> >>>>> dropped now.
> >>>>>
> >>
> >> I now think that the previous versions were incorrect since
> >> davinci_cfg_reg() does not set the scl or sda pins for gpio
> >> functionality. Instead they set them as scl or sda which is not what
> >> we want at the time of pulsing. The previous versions used
> >> gpio_set_value() in disable_i2c_pins() and then called
> >> davinci_cfg_reg(). After which it called pulse_i2c_clock().
> >>
> >> Please correct me if my interpretation of the code is incorrect.
> >
> > Can we get some resolution here?
> >
> > I have a queue of davinci i2c patches waiting to go upstream, and I'd
> > like to get them in for 2.6.35.
> >
> > The current i2c series is in the 'davinci-i2c' branch of davinci git.
>
> To quote Sekhar, "...Right. It is only an enhancement (and only good
> to have at that). This should not stop the current patch from getting
> in." So this patch is good to make it to 2.6.35

How about the using the "free data format" method that Brad was
suggesting [1]? That sounds like a much simpler approach than the
GPIO muxing method that this patch is adopting.

I think we should try that method before accepting this approach
as the final one.

Thanks,
Sekhar

[1] http://www.mail-archive.com/linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org/msg02800.html

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

On 03/17/2010 06:48 PM, Nori, Sekhar wrote:
> Hi Philby,
>
> On Wed, Mar 17, 2010 at 16:58:44, Philby John wrote:
>> On 03/17/2010 02:20 AM, Kevin Hilman wrote:
>>> Philby John<pjohn-Igf4POYTYCDQT0dZR+AlfA@public.gmane.org>   writes:
>>>
>>>> On 02/08/2010 04:05 PM, Nori, Sekhar wrote:
>>>>> Hi Philby,
>>>>>
>>>>> On Fri, Feb 05, 2010 at 19:23:43, Philby John wrote:
>>>>>> Hello Sekhar,
>>>>>>
>>>>>
>>>>> [...]
>>>>>
>>>>>>>> +/* Generate a pulse on the i2c clock pin. */
>>>>>>>> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
>>>>>>>> +{
>>>>>>>> +     u16 i;
>>>>>>>> +
>>>>>>>> +     if (scl_pin) {
>>>>>>>> +             /* Send high and low on the SCL line */
>>>>>>>> +             for (i = 0; i<    9; i++) {
>>>>>>>> +                     gpio_set_value(scl_pin, 0);
>>>>>>>> +                     udelay(20);
>>>>>>>> +                     gpio_set_value(scl_pin, 1);
>>>>>>>> +                     udelay(20);
>>>>>>>> +             }
>>>>>>>
>>>>>>> Before using the pins as GPIO, you would have to set the
>>>>>>> functionality of these pins as GPIO. You had this code in
>>>>>>> previous incarnations of this patch - not sure why it is
>>>>>>> dropped now.
>>>>>>>
>>>>
>>>> I now think that the previous versions were incorrect since
>>>> davinci_cfg_reg() does not set the scl or sda pins for gpio
>>>> functionality. Instead they set them as scl or sda which is not what
>>>> we want at the time of pulsing. The previous versions used
>>>> gpio_set_value() in disable_i2c_pins() and then called
>>>> davinci_cfg_reg(). After which it called pulse_i2c_clock().
>>>>
>>>> Please correct me if my interpretation of the code is incorrect.
>>>
>>> Can we get some resolution here?
>>>
>>> I have a queue of davinci i2c patches waiting to go upstream, and I'd
>>> like to get them in for 2.6.35.
>>>
>>> The current i2c series is in the 'davinci-i2c' branch of davinci git.
>>
>> To quote Sekhar, "...Right. It is only an enhancement (and only good
>> to have at that). This should not stop the current patch from getting
>> in." So this patch is good to make it to 2.6.35
>
> How about the using the "free data format" method that Brad was
> suggesting [1]? That sounds like a much simpler approach than the
> GPIO muxing method that this patch is adopting.
>

I can't seem to agree with your argument. The recovery method adopted 
here is part of the standard i2c v3 spec.

> I think we should try that method before accepting this approach
> as the final one.

It would be wiser allowing this one to go in, since a lot of 
reviewing/testing has already been done. Later on we could try the "free 
data format" and hope that this would work. Otherwise, we would still be 
without a fix until "free data format" is done.


Regards,
Philby

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Pablo Bitton]

[-- Attachment #1.1: Type: text/plain, Size: 7205 bytes --]

Hi Philby,

On Wed, Jan 27, 2010 at 1:41 AM, Kevin Hilman
<khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>wrote:

> From: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
>
> Come out of i2c time out condition by following the
> bus recovery procedure outlined in the i2c protocol v3 spec.
> The kernel must be robust enough to gracefully recover
> from i2c bus failure without having to reset the machine.
> This is done by first NACKing the slave, pulsing the SCL
> line 9 times and then sending the stop command.
>
> This patch has been tested on a DM6446 and DM355
>
> Signed-off-by: Philby John <pjohn-k0rHJ+Hhz/SB+jHODAdFcQ@public.gmane.org>
> Signed-off-by: Srinivasan, Nageswari <nageswari-l0cyMroinI0@public.gmane.org>
> Acked-by: Kevin Hilman <khilman-1D3HCaltpLuhEniVeURVKkEOCMrvLtNR@public.gmane.org>
> ---
>  drivers/i2c/busses/i2c-davinci.c |   57
> +++++++++++++++++++++++++++++++++++--
>  1 files changed, 53 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-davinci.c
> b/drivers/i2c/busses/i2c-davinci.c
> index 35f9daa..5459065 100644
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> @@ -36,6 +36,7 @@
>  #include <linux/platform_device.h>
>  #include <linux/io.h>
>  #include <linux/cpufreq.h>
> +#include <linux/gpio.h>
>
>  #include <mach/hardware.h>
>  #include <mach/i2c.h>
> @@ -43,6 +44,7 @@
>  /* ----- global defines ----------------------------------------------- */
>
>  #define DAVINCI_I2C_TIMEOUT    (1*HZ)
> +#define DAVINCI_I2C_MAX_TRIES  2
>  #define I2C_DAVINCI_INTR_ALL    (DAVINCI_I2C_IMR_AAS | \
>                                 DAVINCI_I2C_IMR_SCD | \
>                                 DAVINCI_I2C_IMR_ARDY | \
> @@ -130,6 +132,44 @@ static inline u16 davinci_i2c_read_reg(struct
> davinci_i2c_dev *i2c_dev, int reg)
>        return __raw_readw(i2c_dev->base + reg);
>  }
>
> +/* Generate a pulse on the i2c clock pin. */
> +static void generic_i2c_clock_pulse(unsigned int scl_pin)
> +{
> +       u16 i;
> +
> +       if (scl_pin) {
> +               /* Send high and low on the SCL line */
> +               for (i = 0; i < 9; i++) {
> +                       gpio_set_value(scl_pin, 0);
> +                       udelay(20);
> +                       gpio_set_value(scl_pin, 1);
> +                       udelay(20);
> +               }
> +       }
> +}
> +
> +/* This routine does i2c bus recovery as specified in the
> + * i2c protocol Rev. 03 section 3.16 titled "Bus clear"
> + */
> +static void i2c_recover_bus(struct davinci_i2c_dev *dev)
> +{
> +       u32 flag = 0;
> +       struct davinci_i2c_platform_data *pdata = dev->dev->platform_data;
> +
> +       dev_err(dev->dev, "initiating i2c bus recovery\n");
> +       /* Send NACK to the slave */
> +       flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> +       flag |=  DAVINCI_I2C_MDR_NACK;
> +       /* write the data into mode register */
> +       davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> +       if (pdata)
> +               generic_i2c_clock_pulse(pdata->scl_pin);
> +       /* Send STOP */
> +       flag = davinci_i2c_read_reg(dev, DAVINCI_I2C_MDR_REG);
> +       flag |= DAVINCI_I2C_MDR_STP;
> +       davinci_i2c_write_reg(dev, DAVINCI_I2C_MDR_REG, flag);
> +}
> +
>  static inline void davinci_i2c_reset_ctrl(struct davinci_i2c_dev *i2c_dev,
>                                                                int val)
>  {
> @@ -235,14 +275,22 @@ static int i2c_davinci_wait_bus_not_busy(struct
> davinci_i2c_dev *dev,
>                                         char allow_sleep)
>  {
>        unsigned long timeout;
> +       static u16 to_cnt;
>
>        timeout = jiffies + dev->adapter.timeout;
>        while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
>               & DAVINCI_I2C_STR_BB) {
> -               if (time_after(jiffies, timeout)) {
> -                       dev_warn(dev->dev,
> -                                "timeout waiting for bus ready\n");
> -                       return -ETIMEDOUT;
> +               if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
> +                       if (time_after(jiffies, timeout)) {
> +                               dev_warn(dev->dev,
> +                               "timeout waiting for bus ready\n");
> +                               to_cnt++;
> +                               return -ETIMEDOUT;
> +                       } else {
> +                               to_cnt = 0;
> +                               i2c_recover_bus(dev);
> +                               i2c_davinci_init(dev);
> +                       }
>                }
>                if (allow_sleep)
>                        schedule_timeout(1);
>

The resulting loop has the following drawbacks:
1) If to_cnt reaches DAVINCI_I2C_MAX_TRIES+1 (which it currently can't, see
2) and the i2c bus collapses,
    the kernel will be stuck in the loop forever, especially if allow_sleep
is false.
2) The to_cnt static var never increments beyond 1. It's initialized to zero
and then kept at zero until the timeout expires.
    When the timeout expires, to_cnt is incremented once, until the next
call to the function, where it will be zeroed again.
 3) Do we really want to retry recovering the bus thousands of times, until
the timeout arrives?
    It seems to me that if the bus recovery procedure didn't help after one
or two tries, it probably never will.

I  also have the following nitpicks:
a) The timeout variable actually holds the finish time.
b) The i2c_recover_bus function uses dev_err to report a bus recovery
process,
    but if all recovery attempts failed and the timeout was reached, only
dev_warn is used to report the timeout.

Other than that the patch is very helpful, thanks a lot.

Below is my suggestion for the wait_bus_not_busy function. My patch has been
tested on a DM6446.

Signed-off-by: Pablo Bitton <pablo.bitton-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
--

diff --git a/drivers/i2c/busses/i2c-davinci.c
b/drivers/i2c/busses/i2c-davinci.c
--- a/drivers/i2c/busses/i2c-davinci.c
+++ b/drivers/i2c/busses/i2c-davinci.c

@@ -220,16 +261,24 @@ static int i2c_davinci_init(struct davinci_i2c_dev
*dev)
 static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
                               char allow_sleep)
 {
-     unsigned long timeout;
+     unsigned long finish_time;
+     unsigned long to_cnt = 0;

-     timeout = jiffies + dev->adapter.timeout;
+     finish_time = jiffies + dev->adapter.timeout;
+     /* While bus busy */
      while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
             & DAVINCI_I2C_STR_BB) {
-           if (time_after(jiffies, timeout)) {
+           if (time_after(jiffies, finish_time)) {
                  dev_warn(dev->dev,
                         "timeout waiting for bus ready\n");
                  return -ETIMEDOUT;
            }
+           else if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
+                 dev_warn(dev->dev, "bus busy, performing bus recovery\n");
+                   ++to_cnt;
+                       i2c_recover_bus(dev);
+                       i2c_davinci_init(dev);
+           }
            if (allow_sleep)
                  schedule_timeout(1);
      }

[-- Attachment #1.2: Type: text/html, Size: 10491 bytes --]

[-- Attachment #2: Type: text/plain, Size: 0 bytes --]

Re: [PATCH 6/6] i2c: davinci: bus recovery procedure to clear the bus

[Philby John]

Hello Pablo,

On Mon, 2010-09-13 at 16:23 +0200, Pablo Bitton wrote:
> Hi Philby,
>         i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
>                                                 char allow_sleep)
>          {
>                unsigned long timeout;
>         +       static u16 to_cnt;
>         
>                timeout = jiffies + dev->adapter.timeout;
>                while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
>                       & DAVINCI_I2C_STR_BB) {
>         -               if (time_after(jiffies, timeout)) {
>         -                       dev_warn(dev->dev,
>         -                                "timeout waiting for bus
>         ready\n");
>         -                       return -ETIMEDOUT;
>         +               if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
>         +                       if (time_after(jiffies, timeout)) {
>         +                               dev_warn(dev->dev,
>         +                               "timeout waiting for bus ready
>         \n");
>         +                               to_cnt++;
>         +                               return -ETIMEDOUT;
>         +                       } else {
>         +                               to_cnt = 0;
>         +                               i2c_recover_bus(dev);
>         +                               i2c_davinci_init(dev);
>         +                       }
>                        }
>                        if (allow_sleep)
>                                schedule_timeout(1);
>  
> The resulting loop has the following drawbacks:
> 1) If to_cnt reaches DAVINCI_I2C_MAX_TRIES+1 (which it currently
> can't, see 2) and the i2c bus collapses,
>     the kernel will be stuck in the loop forever, especially if
> allow_sleep is false.

I do not understand how to_cnt can reach DAVINCI_I2C_MAX_TRIES+1. You
seem to be contradicting your own statement, you also say that this
cannot happen!!

> 2) The to_cnt static var never increments beyond 1.
Precisely.

>  It's initialized to zero and then kept at zero until the timeout
> expires.
>     When the timeout expires, to_cnt is incremented once, until the
> next call to the function, where it will be zeroed again.

How then can your 1st assumption hold good?

>  3) Do we really want to retry recovering the bus thousands of times,
> until the timeout arrives?
>     It seems to me that if the bus recovery procedure didn't help
> after one or two tries, it probably never will.

Once a bus recovery is initiated, we are in the last phase of a recovery
and if that fails the user is left with no other choice but to reset the
target. From the very beginning the idea was to try until timeout.
Wouldn't you have a working system rather than to have to reset the
target?

> 
>  
> I  also have the following nitpicks:
> a) The timeout variable actually holds the finish time.
Well, that's just aesthetic makeover. I could say the timeout is 10
seconds and the finish time is 10 seconds, it sounds the same to me.

> b) The i2c_recover_bus function uses dev_err to report a bus recovery
> process,
>     but if all recovery attempts failed and the timeout was reached,
> only dev_warn is used to report the timeout.

Agreed. But your patch does not reflect this change.

> 
>  
> Other than that the patch is very helpful, thanks a lot.
> 
>  
> Below is my suggestion for the wait_bus_not_busy function. My patch
> has been tested on a DM6446.

All in all, your patch gives multiple checkpatch errors/warnings (spaces
instead of tabs etc). You have missed out parts of the code present in
the pristine kernel and so will not cleanly apply. To me, there are two
things that are of interest in your patch. First is you got rid of the
static variable definition and the other is usage of dev_err. I fail to
understand your assumption that the "kernel will be stuck in the loop
forever", hence I cannot tell how useful this patch really is.

> 
>  
> Signed-off-by: Pablo Bitton <pablo.bitton-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> --
> diff --git a/drivers/i2c/busses/i2c-davinci.c
> b/drivers/i2c/busses/i2c-davinci.c
> --- a/drivers/i2c/busses/i2c-davinci.c
> +++ b/drivers/i2c/busses/i2c-davinci.c
> 
> 
> 
> 
> 
> @@ -220,16 +261,24 @@ static int i2c_davinci_init(struct
> davinci_i2c_dev *dev)
>  static int i2c_davinci_wait_bus_not_busy(struct davinci_i2c_dev *dev,
>                                char allow_sleep)
>  {
> -     unsigned long timeout;
> +     unsigned long finish_time;
You have missed out on this line static u16 to_cnt;
> +     unsigned long to_cnt = 0;

>  
> -     timeout = jiffies + dev->adapter.timeout;
> +     finish_time = jiffies + dev->adapter.timeout;
> +     /* While bus busy */
>       while (davinci_i2c_read_reg(dev, DAVINCI_I2C_STR_REG)
>              & DAVINCI_I2C_STR_BB) {

Your patch misses out on this line. 
if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
Shouldn't you show removing it in the patch?
> -           if (time_after(jiffies, timeout)) {
> +           if (time_after(jiffies, finish_time)) {
>                   dev_warn(dev->dev,
>                          "timeout waiting for bus ready\n");
This is the part where the bus recover procedure would have failed. In
this case we could use dev_err here instead of in the function
i2c_recover_bus().

Also this line..
to_cnt++;
is missing from your patch.
You should show that you are removing it.
>                   return -ETIMEDOUT;
>             }
> +           else if (to_cnt <= DAVINCI_I2C_MAX_TRIES) {
> +                 dev_warn(dev->dev, "bus busy, performing bus
> recovery\n");
There is already a warning in the function i2c_recover_bus(). This can
be removed?
> +                   ++to_cnt;
Can we stick to to_cnt++; ?
> +                       i2c_recover_bus(dev);
> +                       i2c_davinci_init(dev);
> +           }
>             if (allow_sleep)
>                   schedule_timeout(1);
>       }

Regards,
Philby