[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

This patch adds device tree support to the pnx-i2c driver by using platform
resources for memory region and irq and removing dependency on mach includes.

The following platforms are affected:

* PNX
* LPC31xx (WIP)
* LPC32xx

The patch is based on a patch by Jon Smirl, working on lpc31xx integration

Signed-off-by: Roland Stigge <stigge@antcom.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>

---

Applies to v3.4-rc3 + already applied i2c-pnx.c patches from this series

NOTE: I separated out this patch from the first patch series of the LPC32xx
device tree conversion because most of those patches are already applied to
-next. This is the last patch of this first series, a precondition for the
LPC32xx device tree switch.

Changes since v3:
* Documentation/devicetree/bindings/i2c/pnx.txt:
  - Documented interrupt-parent as required
  - Documented slave-addr default in hex

 Documentation/devicetree/bindings/i2c/pnx.txt |   40 ++++++++++++++++
 drivers/i2c/busses/i2c-pnx.c                  |   65 +++++++++++++++++++-------
 include/linux/i2c-pnx.h                       |    1 
 3 files changed, 89 insertions(+), 17 deletions(-)

--- /dev/null
+++ linux-2.6/Documentation/devicetree/bindings/i2c/pnx.txt
@@ -0,0 +1,40 @@
+* NXP PNX I2C Controller
+
+Required properties:
+
+ - reg: Offset and length of the register set for the device
+ - compatible: should be "nxp,pnx-i2c"
+ - interrupts: configure one interrupt line
+ - #address-cells: always 1 (for i2c addresses)
+ - #size-cells: always 0
+ - interrupt-parent: the phandle for the interrupt controller that
+   services interrupts for this device.
+
+Optional properties:
+
+ - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz
+ - pnx,timeout: I2C bus timeout in milliseconds, Default: 10 ms
+ - slave-addr: Address used by the controller, Hardware default: 0x6e
+
+Examples:
+
+	i2c1: i2c@400a0000 {
+		compatible = "nxp,pnx-i2c";
+		reg = <0x400a0000 0x100>;
+		interrupt-parent = <&mic>;
+		interrupts = <51 0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	i2c2: i2c@400a8000 {
+		compatible = "nxp,pnx-i2c";
+		reg = <0x400a8000 0x100>;
+		interrupt-parent = <&mic>;
+		interrupts = <50 0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clock-frequency = <0x186a0>;
+		pnx,timeout = <0x64>;
+		slave-addr = <0x11>;
+	};
--- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
+++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
@@ -23,10 +23,11 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
+#include <linux/of_i2c.h>
 
-#define I2C_PNX_TIMEOUT		10 /* msec */
-#define I2C_PNX_SPEED_KHZ	100
-#define I2C_PNX_REGION_SIZE	0x100
+#define I2C_PNX_TIMEOUT_DEFAULT		10 /* msec */
+#define I2C_PNX_SPEED_KHZ_DEFAULT	100
+#define I2C_PNX_REGION_SIZE		0x100
 
 enum {
 	mstatus_tdi = 0x00000001,
@@ -74,8 +75,9 @@ enum {
 #define I2C_REG_TXS(a)	((a)->ioaddr + 0x28)	/* Tx slave FIFO (RO) */
 #define I2C_REG_STFL(a)	((a)->ioaddr + 0x2c)	/* Tx slave FIFO level (RO) */
 
-static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *data)
+static inline int wait_timeout(struct i2c_pnx_algo_data *data)
 {
+	long timeout = data->timeout;
 	while (timeout > 0 &&
 			(ioread32(I2C_REG_STS(data)) & mstatus_active)) {
 		mdelay(1);
@@ -84,8 +86,9 @@ static inline int wait_timeout(long time
 	return (timeout <= 0);
 }
 
-static inline int wait_reset(long timeout, struct i2c_pnx_algo_data *data)
+static inline int wait_reset(struct i2c_pnx_algo_data *data)
 {
+	long timeout = data->timeout;
 	while (timeout > 0 &&
 			(ioread32(I2C_REG_CTL(data)) & mcntrl_reset)) {
 		mdelay(1);
@@ -97,7 +100,7 @@ static inline int wait_reset(long timeou
 static inline void i2c_pnx_arm_timer(struct i2c_pnx_algo_data *alg_data)
 {
 	struct timer_list *timer = &alg_data->mif.timer;
-	unsigned long expires = msecs_to_jiffies(I2C_PNX_TIMEOUT);
+	unsigned long expires = msecs_to_jiffies(alg_data->timeout);
 
 	if (expires <= 1)
 		expires = 2;
@@ -135,7 +138,7 @@ static int i2c_pnx_start(unsigned char s
 	}
 
 	/* First, make sure bus is idle */
-	if (wait_timeout(I2C_PNX_TIMEOUT, alg_data)) {
+	if (wait_timeout(alg_data)) {
 		/* Somebody else is monopolizing the bus */
 		dev_err(&alg_data->adapter.dev,
 			"%s: Bus busy. Slave addr = %02x, cntrl = %x, stat = %x\n",
@@ -228,7 +231,7 @@ static int i2c_pnx_master_xmit(struct i2
 		if (alg_data->mif.len == 0) {
 			if (alg_data->last) {
 				/* Wait until the STOP is seen. */
-				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
+				if (wait_timeout(alg_data))
 					dev_err(&alg_data->adapter.dev,
 						"The bus is still active after timeout\n");
 			}
@@ -326,7 +329,7 @@ static int i2c_pnx_master_rcv(struct i2c
 		if (alg_data->mif.len == 0) {
 			if (alg_data->last)
 				/* Wait until the STOP is seen. */
-				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
+				if (wait_timeout(alg_data))
 					dev_err(&alg_data->adapter.dev,
 						"The bus is still active after timeout\n");
 
@@ -442,7 +445,7 @@ static void i2c_pnx_timeout(unsigned lon
 
 	ctl |= mcntrl_reset;
 	iowrite32(ctl, I2C_REG_CTL(alg_data));
-	wait_reset(I2C_PNX_TIMEOUT, alg_data);
+	wait_reset(alg_data);
 	alg_data->mif.ret = -EIO;
 	complete(&alg_data->mif.complete);
 }
@@ -457,18 +460,18 @@ static inline void bus_reset_if_active(s
 			alg_data->adapter.name);
 		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
 			  I2C_REG_CTL(alg_data));
-		wait_reset(I2C_PNX_TIMEOUT, alg_data);
+		wait_reset(alg_data);
 	} else if (!(stat & mstatus_rfe) || !(stat & mstatus_tfe)) {
 		/* If there is data in the fifo's after transfer,
 		 * flush fifo's by reset.
 		 */
 		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
 			  I2C_REG_CTL(alg_data));
-		wait_reset(I2C_PNX_TIMEOUT, alg_data);
+		wait_reset(alg_data);
 	} else if (stat & mstatus_nai) {
 		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
 			  I2C_REG_CTL(alg_data));
-		wait_reset(I2C_PNX_TIMEOUT, alg_data);
+		wait_reset(alg_data);
 	}
 }
 
@@ -612,6 +615,8 @@ static int __devinit i2c_pnx_probe(struc
 	struct i2c_pnx_algo_data *alg_data;
 	unsigned long freq;
 	struct resource *res;
+	u32 speed = I2C_PNX_SPEED_KHZ_DEFAULT * 1000;
+	u32 slave_addr = ~0;
 
 	alg_data = kzalloc(sizeof(*alg_data), GFP_KERNEL);
 	if (!alg_data) {
@@ -626,6 +631,18 @@ static int __devinit i2c_pnx_probe(struc
 	alg_data->adapter.algo_data = alg_data;
 	alg_data->adapter.nr = pdev->id;
 
+	alg_data->timeout = I2C_PNX_TIMEOUT_DEFAULT;
+#ifdef CONFIG_OF
+	alg_data->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
+	if (pdev->dev.of_node) {
+		of_property_read_u32(pdev->dev.of_node, "pnx,timeout",
+				     &alg_data->timeout);
+		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				     &speed);
+		of_property_read_u32(pdev->dev.of_node, "slave-addr",
+				     &slave_addr);
+	}
+#endif
 	alg_data->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(alg_data->clk)) {
 		ret = PTR_ERR(alg_data->clk);
@@ -651,7 +668,7 @@ static int __devinit i2c_pnx_probe(struc
 		dev_err(&pdev->dev,
 		       "I/O region 0x%08x for I2C already in use.\n",
 		       res->start);
-		ret = -ENODEV;
+		ret = -ENOMEM;
 		goto out_clkget;
 	}
 
@@ -667,6 +684,9 @@ static int __devinit i2c_pnx_probe(struc
 	if (ret)
 		goto out_unmap;
 
+	if (slave_addr != ~0)
+		iowrite32(slave_addr, I2C_REG_ADR(alg_data));
+
 	freq = clk_get_rate(alg_data->clk);
 
 	/*
@@ -680,14 +700,14 @@ static int __devinit i2c_pnx_probe(struc
 	 * the deglitching filter length.
 	 */
 
-	tmp = ((freq / 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
+	tmp = (freq / speed) / 2 - 2;
 	if (tmp > 0x3FF)
 		tmp = 0x3FF;
 	iowrite32(tmp, I2C_REG_CKH(alg_data));
 	iowrite32(tmp, I2C_REG_CKL(alg_data));
 
 	iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
-	if (wait_reset(I2C_PNX_TIMEOUT, alg_data)) {
+	if (wait_reset(alg_data)) {
 		ret = -ENODEV;
 		goto out_clock;
 	}
@@ -699,7 +719,7 @@ static int __devinit i2c_pnx_probe(struc
 		goto out_irq;
 	}
 	ret = request_irq(alg_data->irq, i2c_pnx_interrupt,
-			0, pdev->name, alg_data);
+			  0, pdev->name, alg_data);
 	if (ret)
 		goto out_clock;
 
@@ -710,6 +730,8 @@ static int __devinit i2c_pnx_probe(struc
 		goto out_irq;
 	}
 
+	of_i2c_register_devices(&alg_data->adapter);
+
 	dev_dbg(&pdev->dev, "%s: Master at %#8x, irq %d.\n",
 		alg_data->adapter.name, res->start, alg_data->irq);
 
@@ -748,10 +770,19 @@ static int __devexit i2c_pnx_remove(stru
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id i2c_pnx_of_match[] = {
+	{ .compatible = "nxp,pnx-i2c" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, i2c_pnx_of_match);
+#endif
+
 static struct platform_driver i2c_pnx_driver = {
 	.driver = {
 		.name = "pnx-i2c",
 		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(i2c_pnx_of_match),
 	},
 	.probe = i2c_pnx_probe,
 	.remove = __devexit_p(i2c_pnx_remove),
--- linux-2.6.orig/include/linux/i2c-pnx.h
+++ linux-2.6/include/linux/i2c-pnx.h
@@ -32,6 +32,7 @@ struct i2c_pnx_algo_data {
 	struct i2c_adapter	adapter;
 	phys_addr_t		base;
 	int			irq;
+	u32			timeout;
 };
 
 #endif /* __I2C_PNX_H__ */

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

This patch adds device tree support to the pnx-i2c driver by using platform
resources for memory region and irq and removing dependency on mach includes.

The following platforms are affected:

* PNX
* LPC31xx (WIP)
* LPC32xx

The patch is based on a patch by Jon Smirl, working on lpc31xx integration

Signed-off-by: Roland Stigge <stigge@antcom.de>
Reviewed-by: Arnd Bergmann <arnd@arndb.de>

---

Applies to v3.4-rc3 + already applied i2c-pnx.c patches from this series

NOTE: I separated out this patch from the first patch series of the LPC32xx
device tree conversion because most of those patches are already applied to
-next. This is the last patch of this first series, a precondition for the
LPC32xx device tree switch.

Changes since v3:
* Documentation/devicetree/bindings/i2c/pnx.txt:
  - Documented interrupt-parent as required
  - Documented slave-addr default in hex

 Documentation/devicetree/bindings/i2c/pnx.txt |   40 ++++++++++++++++
 drivers/i2c/busses/i2c-pnx.c                  |   65 +++++++++++++++++++-------
 include/linux/i2c-pnx.h                       |    1 
 3 files changed, 89 insertions(+), 17 deletions(-)

--- /dev/null
+++ linux-2.6/Documentation/devicetree/bindings/i2c/pnx.txt
@@ -0,0 +1,40 @@
+* NXP PNX I2C Controller
+
+Required properties:
+
+ - reg: Offset and length of the register set for the device
+ - compatible: should be "nxp,pnx-i2c"
+ - interrupts: configure one interrupt line
+ - #address-cells: always 1 (for i2c addresses)
+ - #size-cells: always 0
+ - interrupt-parent: the phandle for the interrupt controller that
+   services interrupts for this device.
+
+Optional properties:
+
+ - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz
+ - pnx,timeout: I2C bus timeout in milliseconds, Default: 10 ms
+ - slave-addr: Address used by the controller, Hardware default: 0x6e
+
+Examples:
+
+	i2c1: i2c at 400a0000 {
+		compatible = "nxp,pnx-i2c";
+		reg = <0x400a0000 0x100>;
+		interrupt-parent = <&mic>;
+		interrupts = <51 0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+	};
+
+	i2c2: i2c at 400a8000 {
+		compatible = "nxp,pnx-i2c";
+		reg = <0x400a8000 0x100>;
+		interrupt-parent = <&mic>;
+		interrupts = <50 0>;
+		#address-cells = <1>;
+		#size-cells = <0>;
+		clock-frequency = <0x186a0>;
+		pnx,timeout = <0x64>;
+		slave-addr = <0x11>;
+	};
--- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
+++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
@@ -23,10 +23,11 @@
 #include <linux/err.h>
 #include <linux/clk.h>
 #include <linux/slab.h>
+#include <linux/of_i2c.h>
 
-#define I2C_PNX_TIMEOUT		10 /* msec */
-#define I2C_PNX_SPEED_KHZ	100
-#define I2C_PNX_REGION_SIZE	0x100
+#define I2C_PNX_TIMEOUT_DEFAULT		10 /* msec */
+#define I2C_PNX_SPEED_KHZ_DEFAULT	100
+#define I2C_PNX_REGION_SIZE		0x100
 
 enum {
 	mstatus_tdi = 0x00000001,
@@ -74,8 +75,9 @@ enum {
 #define I2C_REG_TXS(a)	((a)->ioaddr + 0x28)	/* Tx slave FIFO (RO) */
 #define I2C_REG_STFL(a)	((a)->ioaddr + 0x2c)	/* Tx slave FIFO level (RO) */
 
-static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *data)
+static inline int wait_timeout(struct i2c_pnx_algo_data *data)
 {
+	long timeout = data->timeout;
 	while (timeout > 0 &&
 			(ioread32(I2C_REG_STS(data)) & mstatus_active)) {
 		mdelay(1);
@@ -84,8 +86,9 @@ static inline int wait_timeout(long time
 	return (timeout <= 0);
 }
 
-static inline int wait_reset(long timeout, struct i2c_pnx_algo_data *data)
+static inline int wait_reset(struct i2c_pnx_algo_data *data)
 {
+	long timeout = data->timeout;
 	while (timeout > 0 &&
 			(ioread32(I2C_REG_CTL(data)) & mcntrl_reset)) {
 		mdelay(1);
@@ -97,7 +100,7 @@ static inline int wait_reset(long timeou
 static inline void i2c_pnx_arm_timer(struct i2c_pnx_algo_data *alg_data)
 {
 	struct timer_list *timer = &alg_data->mif.timer;
-	unsigned long expires = msecs_to_jiffies(I2C_PNX_TIMEOUT);
+	unsigned long expires = msecs_to_jiffies(alg_data->timeout);
 
 	if (expires <= 1)
 		expires = 2;
@@ -135,7 +138,7 @@ static int i2c_pnx_start(unsigned char s
 	}
 
 	/* First, make sure bus is idle */
-	if (wait_timeout(I2C_PNX_TIMEOUT, alg_data)) {
+	if (wait_timeout(alg_data)) {
 		/* Somebody else is monopolizing the bus */
 		dev_err(&alg_data->adapter.dev,
 			"%s: Bus busy. Slave addr = %02x, cntrl = %x, stat = %x\n",
@@ -228,7 +231,7 @@ static int i2c_pnx_master_xmit(struct i2
 		if (alg_data->mif.len == 0) {
 			if (alg_data->last) {
 				/* Wait until the STOP is seen. */
-				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
+				if (wait_timeout(alg_data))
 					dev_err(&alg_data->adapter.dev,
 						"The bus is still active after timeout\n");
 			}
@@ -326,7 +329,7 @@ static int i2c_pnx_master_rcv(struct i2c
 		if (alg_data->mif.len == 0) {
 			if (alg_data->last)
 				/* Wait until the STOP is seen. */
-				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
+				if (wait_timeout(alg_data))
 					dev_err(&alg_data->adapter.dev,
 						"The bus is still active after timeout\n");
 
@@ -442,7 +445,7 @@ static void i2c_pnx_timeout(unsigned lon
 
 	ctl |= mcntrl_reset;
 	iowrite32(ctl, I2C_REG_CTL(alg_data));
-	wait_reset(I2C_PNX_TIMEOUT, alg_data);
+	wait_reset(alg_data);
 	alg_data->mif.ret = -EIO;
 	complete(&alg_data->mif.complete);
 }
@@ -457,18 +460,18 @@ static inline void bus_reset_if_active(s
 			alg_data->adapter.name);
 		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
 			  I2C_REG_CTL(alg_data));
-		wait_reset(I2C_PNX_TIMEOUT, alg_data);
+		wait_reset(alg_data);
 	} else if (!(stat & mstatus_rfe) || !(stat & mstatus_tfe)) {
 		/* If there is data in the fifo's after transfer,
 		 * flush fifo's by reset.
 		 */
 		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
 			  I2C_REG_CTL(alg_data));
-		wait_reset(I2C_PNX_TIMEOUT, alg_data);
+		wait_reset(alg_data);
 	} else if (stat & mstatus_nai) {
 		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
 			  I2C_REG_CTL(alg_data));
-		wait_reset(I2C_PNX_TIMEOUT, alg_data);
+		wait_reset(alg_data);
 	}
 }
 
@@ -612,6 +615,8 @@ static int __devinit i2c_pnx_probe(struc
 	struct i2c_pnx_algo_data *alg_data;
 	unsigned long freq;
 	struct resource *res;
+	u32 speed = I2C_PNX_SPEED_KHZ_DEFAULT * 1000;
+	u32 slave_addr = ~0;
 
 	alg_data = kzalloc(sizeof(*alg_data), GFP_KERNEL);
 	if (!alg_data) {
@@ -626,6 +631,18 @@ static int __devinit i2c_pnx_probe(struc
 	alg_data->adapter.algo_data = alg_data;
 	alg_data->adapter.nr = pdev->id;
 
+	alg_data->timeout = I2C_PNX_TIMEOUT_DEFAULT;
+#ifdef CONFIG_OF
+	alg_data->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
+	if (pdev->dev.of_node) {
+		of_property_read_u32(pdev->dev.of_node, "pnx,timeout",
+				     &alg_data->timeout);
+		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
+				     &speed);
+		of_property_read_u32(pdev->dev.of_node, "slave-addr",
+				     &slave_addr);
+	}
+#endif
 	alg_data->clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(alg_data->clk)) {
 		ret = PTR_ERR(alg_data->clk);
@@ -651,7 +668,7 @@ static int __devinit i2c_pnx_probe(struc
 		dev_err(&pdev->dev,
 		       "I/O region 0x%08x for I2C already in use.\n",
 		       res->start);
-		ret = -ENODEV;
+		ret = -ENOMEM;
 		goto out_clkget;
 	}
 
@@ -667,6 +684,9 @@ static int __devinit i2c_pnx_probe(struc
 	if (ret)
 		goto out_unmap;
 
+	if (slave_addr != ~0)
+		iowrite32(slave_addr, I2C_REG_ADR(alg_data));
+
 	freq = clk_get_rate(alg_data->clk);
 
 	/*
@@ -680,14 +700,14 @@ static int __devinit i2c_pnx_probe(struc
 	 * the deglitching filter length.
 	 */
 
-	tmp = ((freq / 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
+	tmp = (freq / speed) / 2 - 2;
 	if (tmp > 0x3FF)
 		tmp = 0x3FF;
 	iowrite32(tmp, I2C_REG_CKH(alg_data));
 	iowrite32(tmp, I2C_REG_CKL(alg_data));
 
 	iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
-	if (wait_reset(I2C_PNX_TIMEOUT, alg_data)) {
+	if (wait_reset(alg_data)) {
 		ret = -ENODEV;
 		goto out_clock;
 	}
@@ -699,7 +719,7 @@ static int __devinit i2c_pnx_probe(struc
 		goto out_irq;
 	}
 	ret = request_irq(alg_data->irq, i2c_pnx_interrupt,
-			0, pdev->name, alg_data);
+			  0, pdev->name, alg_data);
 	if (ret)
 		goto out_clock;
 
@@ -710,6 +730,8 @@ static int __devinit i2c_pnx_probe(struc
 		goto out_irq;
 	}
 
+	of_i2c_register_devices(&alg_data->adapter);
+
 	dev_dbg(&pdev->dev, "%s: Master at %#8x, irq %d.\n",
 		alg_data->adapter.name, res->start, alg_data->irq);
 
@@ -748,10 +770,19 @@ static int __devexit i2c_pnx_remove(stru
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id i2c_pnx_of_match[] = {
+	{ .compatible = "nxp,pnx-i2c" },
+	{ },
+};
+MODULE_DEVICE_TABLE(of, i2c_pnx_of_match);
+#endif
+
 static struct platform_driver i2c_pnx_driver = {
 	.driver = {
 		.name = "pnx-i2c",
 		.owner = THIS_MODULE,
+		.of_match_table = of_match_ptr(i2c_pnx_of_match),
 	},
 	.probe = i2c_pnx_probe,
 	.remove = __devexit_p(i2c_pnx_remove),
--- linux-2.6.orig/include/linux/i2c-pnx.h
+++ linux-2.6/include/linux/i2c-pnx.h
@@ -32,6 +32,7 @@ struct i2c_pnx_algo_data {
 	struct i2c_adapter	adapter;
 	phys_addr_t		base;
 	int			irq;
+	u32			timeout;
 };
 
 #endif /* __I2C_PNX_H__ */

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 10472 bytes --]

On Thu, Apr 19, 2012 at 05:50:12PM +0200, Roland Stigge wrote:
> This patch adds device tree support to the pnx-i2c driver by using platform
> resources for memory region and irq and removing dependency on mach includes.
> 
> The following platforms are affected:
> 
> * PNX
> * LPC31xx (WIP)
> * LPC32xx
> 
> The patch is based on a patch by Jon Smirl, working on lpc31xx integration
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> ---
> 
> Applies to v3.4-rc3 + already applied i2c-pnx.c patches from this series
> 
> NOTE: I separated out this patch from the first patch series of the LPC32xx
> device tree conversion because most of those patches are already applied to
> -next. This is the last patch of this first series, a precondition for the
> LPC32xx device tree switch.
> 
> Changes since v3:
> * Documentation/devicetree/bindings/i2c/pnx.txt:
>   - Documented interrupt-parent as required
>   - Documented slave-addr default in hex
> 
>  Documentation/devicetree/bindings/i2c/pnx.txt |   40 ++++++++++++++++
>  drivers/i2c/busses/i2c-pnx.c                  |   65 +++++++++++++++++++-------
>  include/linux/i2c-pnx.h                       |    1 
>  3 files changed, 89 insertions(+), 17 deletions(-)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/i2c/pnx.txt
> @@ -0,0 +1,40 @@
> +* NXP PNX I2C Controller
> +
> +Required properties:
> +
> + - reg: Offset and length of the register set for the device
> + - compatible: should be "nxp,pnx-i2c"
> + - interrupts: configure one interrupt line
> + - #address-cells: always 1 (for i2c addresses)
> + - #size-cells: always 0
> + - interrupt-parent: the phandle for the interrupt controller that
> +   services interrupts for this device.
> +
> +Optional properties:
> +
> + - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz
> + - pnx,timeout: I2C bus timeout in milliseconds, Default: 10 ms

I'd like to repeat my question to the devicetree folks here: Can we have
timeout generic? It doesn't make sense to me to have that per vendor
again and again. I searched devicetree.org and EPAPR but couldn't find a
hint.

> + - slave-addr: Address used by the controller, Hardware default: 0x6e
> +
> +Examples:
> +
> +	i2c1: i2c@400a0000 {
> +		compatible = "nxp,pnx-i2c";
> +		reg = <0x400a0000 0x100>;
> +		interrupt-parent = <&mic>;
> +		interrupts = <51 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	i2c2: i2c@400a8000 {
> +		compatible = "nxp,pnx-i2c";
> +		reg = <0x400a8000 0x100>;
> +		interrupt-parent = <&mic>;
> +		interrupts = <50 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clock-frequency = <0x186a0>;
> +		pnx,timeout = <0x64>;

Did you change this, too? Timeouts are better readable in dec :)

> +		slave-addr = <0x11>;
> +	};
> --- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
> +++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
> @@ -23,10 +23,11 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/slab.h>
> +#include <linux/of_i2c.h>
>  
> -#define I2C_PNX_TIMEOUT		10 /* msec */
> -#define I2C_PNX_SPEED_KHZ	100
> -#define I2C_PNX_REGION_SIZE	0x100
> +#define I2C_PNX_TIMEOUT_DEFAULT		10 /* msec */
> +#define I2C_PNX_SPEED_KHZ_DEFAULT	100
> +#define I2C_PNX_REGION_SIZE		0x100
>  
>  enum {
>  	mstatus_tdi = 0x00000001,
> @@ -74,8 +75,9 @@ enum {
>  #define I2C_REG_TXS(a)	((a)->ioaddr + 0x28)	/* Tx slave FIFO (RO) */
>  #define I2C_REG_STFL(a)	((a)->ioaddr + 0x2c)	/* Tx slave FIFO level (RO) */
>  
> -static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *data)
> +static inline int wait_timeout(struct i2c_pnx_algo_data *data)
>  {
> +	long timeout = data->timeout;
>  	while (timeout > 0 &&
>  			(ioread32(I2C_REG_STS(data)) & mstatus_active)) {
>  		mdelay(1);
> @@ -84,8 +86,9 @@ static inline int wait_timeout(long time
>  	return (timeout <= 0);
>  }
>  
> -static inline int wait_reset(long timeout, struct i2c_pnx_algo_data *data)
> +static inline int wait_reset(struct i2c_pnx_algo_data *data)
>  {
> +	long timeout = data->timeout;
>  	while (timeout > 0 &&
>  			(ioread32(I2C_REG_CTL(data)) & mcntrl_reset)) {
>  		mdelay(1);
> @@ -97,7 +100,7 @@ static inline int wait_reset(long timeou
>  static inline void i2c_pnx_arm_timer(struct i2c_pnx_algo_data *alg_data)
>  {
>  	struct timer_list *timer = &alg_data->mif.timer;
> -	unsigned long expires = msecs_to_jiffies(I2C_PNX_TIMEOUT);
> +	unsigned long expires = msecs_to_jiffies(alg_data->timeout);
>  
>  	if (expires <= 1)
>  		expires = 2;
> @@ -135,7 +138,7 @@ static int i2c_pnx_start(unsigned char s
>  	}
>  
>  	/* First, make sure bus is idle */
> -	if (wait_timeout(I2C_PNX_TIMEOUT, alg_data)) {
> +	if (wait_timeout(alg_data)) {
>  		/* Somebody else is monopolizing the bus */
>  		dev_err(&alg_data->adapter.dev,
>  			"%s: Bus busy. Slave addr = %02x, cntrl = %x, stat = %x\n",
> @@ -228,7 +231,7 @@ static int i2c_pnx_master_xmit(struct i2
>  		if (alg_data->mif.len == 0) {
>  			if (alg_data->last) {
>  				/* Wait until the STOP is seen. */
> -				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
> +				if (wait_timeout(alg_data))
>  					dev_err(&alg_data->adapter.dev,
>  						"The bus is still active after timeout\n");
>  			}
> @@ -326,7 +329,7 @@ static int i2c_pnx_master_rcv(struct i2c
>  		if (alg_data->mif.len == 0) {
>  			if (alg_data->last)
>  				/* Wait until the STOP is seen. */
> -				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
> +				if (wait_timeout(alg_data))
>  					dev_err(&alg_data->adapter.dev,
>  						"The bus is still active after timeout\n");
>  
> @@ -442,7 +445,7 @@ static void i2c_pnx_timeout(unsigned lon
>  
>  	ctl |= mcntrl_reset;
>  	iowrite32(ctl, I2C_REG_CTL(alg_data));
> -	wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +	wait_reset(alg_data);
>  	alg_data->mif.ret = -EIO;
>  	complete(&alg_data->mif.complete);
>  }
> @@ -457,18 +460,18 @@ static inline void bus_reset_if_active(s
>  			alg_data->adapter.name);
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	} else if (!(stat & mstatus_rfe) || !(stat & mstatus_tfe)) {
>  		/* If there is data in the fifo's after transfer,
>  		 * flush fifo's by reset.
>  		 */
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	} else if (stat & mstatus_nai) {
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	}
>  }
>  
> @@ -612,6 +615,8 @@ static int __devinit i2c_pnx_probe(struc
>  	struct i2c_pnx_algo_data *alg_data;
>  	unsigned long freq;
>  	struct resource *res;
> +	u32 speed = I2C_PNX_SPEED_KHZ_DEFAULT * 1000;
> +	u32 slave_addr = ~0;
>  
>  	alg_data = kzalloc(sizeof(*alg_data), GFP_KERNEL);
>  	if (!alg_data) {
> @@ -626,6 +631,18 @@ static int __devinit i2c_pnx_probe(struc
>  	alg_data->adapter.algo_data = alg_data;
>  	alg_data->adapter.nr = pdev->id;
>  
> +	alg_data->timeout = I2C_PNX_TIMEOUT_DEFAULT;
> +#ifdef CONFIG_OF
> +	alg_data->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> +	if (pdev->dev.of_node) {
> +		of_property_read_u32(pdev->dev.of_node, "pnx,timeout",
> +				     &alg_data->timeout);
> +		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +				     &speed);
> +		of_property_read_u32(pdev->dev.of_node, "slave-addr",
> +				     &slave_addr);
> +	}
> +#endif
>  	alg_data->clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(alg_data->clk)) {
>  		ret = PTR_ERR(alg_data->clk);
> @@ -651,7 +668,7 @@ static int __devinit i2c_pnx_probe(struc
>  		dev_err(&pdev->dev,
>  		       "I/O region 0x%08x for I2C already in use.\n",
>  		       res->start);
> -		ret = -ENODEV;
> +		ret = -ENOMEM;
>  		goto out_clkget;
>  	}
>  
> @@ -667,6 +684,9 @@ static int __devinit i2c_pnx_probe(struc
>  	if (ret)
>  		goto out_unmap;
>  
> +	if (slave_addr != ~0)
> +		iowrite32(slave_addr, I2C_REG_ADR(alg_data));
> +
>  	freq = clk_get_rate(alg_data->clk);
>  
>  	/*
> @@ -680,14 +700,14 @@ static int __devinit i2c_pnx_probe(struc
>  	 * the deglitching filter length.
>  	 */
>  
> -	tmp = ((freq / 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
> +	tmp = (freq / speed) / 2 - 2;
>  	if (tmp > 0x3FF)
>  		tmp = 0x3FF;
>  	iowrite32(tmp, I2C_REG_CKH(alg_data));
>  	iowrite32(tmp, I2C_REG_CKL(alg_data));
>  
>  	iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
> -	if (wait_reset(I2C_PNX_TIMEOUT, alg_data)) {
> +	if (wait_reset(alg_data)) {
>  		ret = -ENODEV;
>  		goto out_clock;
>  	}
> @@ -699,7 +719,7 @@ static int __devinit i2c_pnx_probe(struc
>  		goto out_irq;
>  	}
>  	ret = request_irq(alg_data->irq, i2c_pnx_interrupt,
> -			0, pdev->name, alg_data);
> +			  0, pdev->name, alg_data);
>  	if (ret)
>  		goto out_clock;
>  
> @@ -710,6 +730,8 @@ static int __devinit i2c_pnx_probe(struc
>  		goto out_irq;
>  	}
>  
> +	of_i2c_register_devices(&alg_data->adapter);
> +
>  	dev_dbg(&pdev->dev, "%s: Master at %#8x, irq %d.\n",
>  		alg_data->adapter.name, res->start, alg_data->irq);
>  
> @@ -748,10 +770,19 @@ static int __devexit i2c_pnx_remove(stru
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id i2c_pnx_of_match[] = {
> +	{ .compatible = "nxp,pnx-i2c" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, i2c_pnx_of_match);
> +#endif
> +
>  static struct platform_driver i2c_pnx_driver = {
>  	.driver = {
>  		.name = "pnx-i2c",
>  		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(i2c_pnx_of_match),
>  	},
>  	.probe = i2c_pnx_probe,
>  	.remove = __devexit_p(i2c_pnx_remove),
> --- linux-2.6.orig/include/linux/i2c-pnx.h
> +++ linux-2.6/include/linux/i2c-pnx.h
> @@ -32,6 +32,7 @@ struct i2c_pnx_algo_data {
>  	struct i2c_adapter	adapter;
>  	phys_addr_t		base;
>  	int			irq;
> +	u32			timeout;
>  };
>  
>  #endif /* __I2C_PNX_H__ */

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 10522 bytes --]

On Thu, Apr 19, 2012 at 05:50:12PM +0200, Roland Stigge wrote:
> This patch adds device tree support to the pnx-i2c driver by using platform
> resources for memory region and irq and removing dependency on mach includes.
> 
> The following platforms are affected:
> 
> * PNX
> * LPC31xx (WIP)
> * LPC32xx
> 
> The patch is based on a patch by Jon Smirl, working on lpc31xx integration
> 
> Signed-off-by: Roland Stigge <stigge-uj/7R2tJ6VmzQB+pC5nmwQ@public.gmane.org>
> Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
> 
> ---
> 
> Applies to v3.4-rc3 + already applied i2c-pnx.c patches from this series
> 
> NOTE: I separated out this patch from the first patch series of the LPC32xx
> device tree conversion because most of those patches are already applied to
> -next. This is the last patch of this first series, a precondition for the
> LPC32xx device tree switch.
> 
> Changes since v3:
> * Documentation/devicetree/bindings/i2c/pnx.txt:
>   - Documented interrupt-parent as required
>   - Documented slave-addr default in hex
> 
>  Documentation/devicetree/bindings/i2c/pnx.txt |   40 ++++++++++++++++
>  drivers/i2c/busses/i2c-pnx.c                  |   65 +++++++++++++++++++-------
>  include/linux/i2c-pnx.h                       |    1 
>  3 files changed, 89 insertions(+), 17 deletions(-)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/i2c/pnx.txt
> @@ -0,0 +1,40 @@
> +* NXP PNX I2C Controller
> +
> +Required properties:
> +
> + - reg: Offset and length of the register set for the device
> + - compatible: should be "nxp,pnx-i2c"
> + - interrupts: configure one interrupt line
> + - #address-cells: always 1 (for i2c addresses)
> + - #size-cells: always 0
> + - interrupt-parent: the phandle for the interrupt controller that
> +   services interrupts for this device.
> +
> +Optional properties:
> +
> + - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz
> + - pnx,timeout: I2C bus timeout in milliseconds, Default: 10 ms

I'd like to repeat my question to the devicetree folks here: Can we have
timeout generic? It doesn't make sense to me to have that per vendor
again and again. I searched devicetree.org and EPAPR but couldn't find a
hint.

> + - slave-addr: Address used by the controller, Hardware default: 0x6e
> +
> +Examples:
> +
> +	i2c1: i2c@400a0000 {
> +		compatible = "nxp,pnx-i2c";
> +		reg = <0x400a0000 0x100>;
> +		interrupt-parent = <&mic>;
> +		interrupts = <51 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	i2c2: i2c@400a8000 {
> +		compatible = "nxp,pnx-i2c";
> +		reg = <0x400a8000 0x100>;
> +		interrupt-parent = <&mic>;
> +		interrupts = <50 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clock-frequency = <0x186a0>;
> +		pnx,timeout = <0x64>;

Did you change this, too? Timeouts are better readable in dec :)

> +		slave-addr = <0x11>;
> +	};
> --- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
> +++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
> @@ -23,10 +23,11 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/slab.h>
> +#include <linux/of_i2c.h>
>  
> -#define I2C_PNX_TIMEOUT		10 /* msec */
> -#define I2C_PNX_SPEED_KHZ	100
> -#define I2C_PNX_REGION_SIZE	0x100
> +#define I2C_PNX_TIMEOUT_DEFAULT		10 /* msec */
> +#define I2C_PNX_SPEED_KHZ_DEFAULT	100
> +#define I2C_PNX_REGION_SIZE		0x100
>  
>  enum {
>  	mstatus_tdi = 0x00000001,
> @@ -74,8 +75,9 @@ enum {
>  #define I2C_REG_TXS(a)	((a)->ioaddr + 0x28)	/* Tx slave FIFO (RO) */
>  #define I2C_REG_STFL(a)	((a)->ioaddr + 0x2c)	/* Tx slave FIFO level (RO) */
>  
> -static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *data)
> +static inline int wait_timeout(struct i2c_pnx_algo_data *data)
>  {
> +	long timeout = data->timeout;
>  	while (timeout > 0 &&
>  			(ioread32(I2C_REG_STS(data)) & mstatus_active)) {
>  		mdelay(1);
> @@ -84,8 +86,9 @@ static inline int wait_timeout(long time
>  	return (timeout <= 0);
>  }
>  
> -static inline int wait_reset(long timeout, struct i2c_pnx_algo_data *data)
> +static inline int wait_reset(struct i2c_pnx_algo_data *data)
>  {
> +	long timeout = data->timeout;
>  	while (timeout > 0 &&
>  			(ioread32(I2C_REG_CTL(data)) & mcntrl_reset)) {
>  		mdelay(1);
> @@ -97,7 +100,7 @@ static inline int wait_reset(long timeou
>  static inline void i2c_pnx_arm_timer(struct i2c_pnx_algo_data *alg_data)
>  {
>  	struct timer_list *timer = &alg_data->mif.timer;
> -	unsigned long expires = msecs_to_jiffies(I2C_PNX_TIMEOUT);
> +	unsigned long expires = msecs_to_jiffies(alg_data->timeout);
>  
>  	if (expires <= 1)
>  		expires = 2;
> @@ -135,7 +138,7 @@ static int i2c_pnx_start(unsigned char s
>  	}
>  
>  	/* First, make sure bus is idle */
> -	if (wait_timeout(I2C_PNX_TIMEOUT, alg_data)) {
> +	if (wait_timeout(alg_data)) {
>  		/* Somebody else is monopolizing the bus */
>  		dev_err(&alg_data->adapter.dev,
>  			"%s: Bus busy. Slave addr = %02x, cntrl = %x, stat = %x\n",
> @@ -228,7 +231,7 @@ static int i2c_pnx_master_xmit(struct i2
>  		if (alg_data->mif.len == 0) {
>  			if (alg_data->last) {
>  				/* Wait until the STOP is seen. */
> -				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
> +				if (wait_timeout(alg_data))
>  					dev_err(&alg_data->adapter.dev,
>  						"The bus is still active after timeout\n");
>  			}
> @@ -326,7 +329,7 @@ static int i2c_pnx_master_rcv(struct i2c
>  		if (alg_data->mif.len == 0) {
>  			if (alg_data->last)
>  				/* Wait until the STOP is seen. */
> -				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
> +				if (wait_timeout(alg_data))
>  					dev_err(&alg_data->adapter.dev,
>  						"The bus is still active after timeout\n");
>  
> @@ -442,7 +445,7 @@ static void i2c_pnx_timeout(unsigned lon
>  
>  	ctl |= mcntrl_reset;
>  	iowrite32(ctl, I2C_REG_CTL(alg_data));
> -	wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +	wait_reset(alg_data);
>  	alg_data->mif.ret = -EIO;
>  	complete(&alg_data->mif.complete);
>  }
> @@ -457,18 +460,18 @@ static inline void bus_reset_if_active(s
>  			alg_data->adapter.name);
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	} else if (!(stat & mstatus_rfe) || !(stat & mstatus_tfe)) {
>  		/* If there is data in the fifo's after transfer,
>  		 * flush fifo's by reset.
>  		 */
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	} else if (stat & mstatus_nai) {
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	}
>  }
>  
> @@ -612,6 +615,8 @@ static int __devinit i2c_pnx_probe(struc
>  	struct i2c_pnx_algo_data *alg_data;
>  	unsigned long freq;
>  	struct resource *res;
> +	u32 speed = I2C_PNX_SPEED_KHZ_DEFAULT * 1000;
> +	u32 slave_addr = ~0;
>  
>  	alg_data = kzalloc(sizeof(*alg_data), GFP_KERNEL);
>  	if (!alg_data) {
> @@ -626,6 +631,18 @@ static int __devinit i2c_pnx_probe(struc
>  	alg_data->adapter.algo_data = alg_data;
>  	alg_data->adapter.nr = pdev->id;
>  
> +	alg_data->timeout = I2C_PNX_TIMEOUT_DEFAULT;
> +#ifdef CONFIG_OF
> +	alg_data->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> +	if (pdev->dev.of_node) {
> +		of_property_read_u32(pdev->dev.of_node, "pnx,timeout",
> +				     &alg_data->timeout);
> +		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +				     &speed);
> +		of_property_read_u32(pdev->dev.of_node, "slave-addr",
> +				     &slave_addr);
> +	}
> +#endif
>  	alg_data->clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(alg_data->clk)) {
>  		ret = PTR_ERR(alg_data->clk);
> @@ -651,7 +668,7 @@ static int __devinit i2c_pnx_probe(struc
>  		dev_err(&pdev->dev,
>  		       "I/O region 0x%08x for I2C already in use.\n",
>  		       res->start);
> -		ret = -ENODEV;
> +		ret = -ENOMEM;
>  		goto out_clkget;
>  	}
>  
> @@ -667,6 +684,9 @@ static int __devinit i2c_pnx_probe(struc
>  	if (ret)
>  		goto out_unmap;
>  
> +	if (slave_addr != ~0)
> +		iowrite32(slave_addr, I2C_REG_ADR(alg_data));
> +
>  	freq = clk_get_rate(alg_data->clk);
>  
>  	/*
> @@ -680,14 +700,14 @@ static int __devinit i2c_pnx_probe(struc
>  	 * the deglitching filter length.
>  	 */
>  
> -	tmp = ((freq / 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
> +	tmp = (freq / speed) / 2 - 2;
>  	if (tmp > 0x3FF)
>  		tmp = 0x3FF;
>  	iowrite32(tmp, I2C_REG_CKH(alg_data));
>  	iowrite32(tmp, I2C_REG_CKL(alg_data));
>  
>  	iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
> -	if (wait_reset(I2C_PNX_TIMEOUT, alg_data)) {
> +	if (wait_reset(alg_data)) {
>  		ret = -ENODEV;
>  		goto out_clock;
>  	}
> @@ -699,7 +719,7 @@ static int __devinit i2c_pnx_probe(struc
>  		goto out_irq;
>  	}
>  	ret = request_irq(alg_data->irq, i2c_pnx_interrupt,
> -			0, pdev->name, alg_data);
> +			  0, pdev->name, alg_data);
>  	if (ret)
>  		goto out_clock;
>  
> @@ -710,6 +730,8 @@ static int __devinit i2c_pnx_probe(struc
>  		goto out_irq;
>  	}
>  
> +	of_i2c_register_devices(&alg_data->adapter);
> +
>  	dev_dbg(&pdev->dev, "%s: Master at %#8x, irq %d.\n",
>  		alg_data->adapter.name, res->start, alg_data->irq);
>  
> @@ -748,10 +770,19 @@ static int __devexit i2c_pnx_remove(stru
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id i2c_pnx_of_match[] = {
> +	{ .compatible = "nxp,pnx-i2c" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, i2c_pnx_of_match);
> +#endif
> +
>  static struct platform_driver i2c_pnx_driver = {
>  	.driver = {
>  		.name = "pnx-i2c",
>  		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(i2c_pnx_of_match),
>  	},
>  	.probe = i2c_pnx_probe,
>  	.remove = __devexit_p(i2c_pnx_remove),
> --- linux-2.6.orig/include/linux/i2c-pnx.h
> +++ linux-2.6/include/linux/i2c-pnx.h
> @@ -32,6 +32,7 @@ struct i2c_pnx_algo_data {
>  	struct i2c_adapter	adapter;
>  	phys_addr_t		base;
>  	int			irq;
> +	u32			timeout;
>  };
>  
>  #endif /* __I2C_PNX_H__ */

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

On Thu, Apr 19, 2012 at 05:50:12PM +0200, Roland Stigge wrote:
> This patch adds device tree support to the pnx-i2c driver by using platform
> resources for memory region and irq and removing dependency on mach includes.
> 
> The following platforms are affected:
> 
> * PNX
> * LPC31xx (WIP)
> * LPC32xx
> 
> The patch is based on a patch by Jon Smirl, working on lpc31xx integration
> 
> Signed-off-by: Roland Stigge <stigge@antcom.de>
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> 
> ---
> 
> Applies to v3.4-rc3 + already applied i2c-pnx.c patches from this series
> 
> NOTE: I separated out this patch from the first patch series of the LPC32xx
> device tree conversion because most of those patches are already applied to
> -next. This is the last patch of this first series, a precondition for the
> LPC32xx device tree switch.
> 
> Changes since v3:
> * Documentation/devicetree/bindings/i2c/pnx.txt:
>   - Documented interrupt-parent as required
>   - Documented slave-addr default in hex
> 
>  Documentation/devicetree/bindings/i2c/pnx.txt |   40 ++++++++++++++++
>  drivers/i2c/busses/i2c-pnx.c                  |   65 +++++++++++++++++++-------
>  include/linux/i2c-pnx.h                       |    1 
>  3 files changed, 89 insertions(+), 17 deletions(-)
> 
> --- /dev/null
> +++ linux-2.6/Documentation/devicetree/bindings/i2c/pnx.txt
> @@ -0,0 +1,40 @@
> +* NXP PNX I2C Controller
> +
> +Required properties:
> +
> + - reg: Offset and length of the register set for the device
> + - compatible: should be "nxp,pnx-i2c"
> + - interrupts: configure one interrupt line
> + - #address-cells: always 1 (for i2c addresses)
> + - #size-cells: always 0
> + - interrupt-parent: the phandle for the interrupt controller that
> +   services interrupts for this device.
> +
> +Optional properties:
> +
> + - clock-frequency: desired I2C bus clock frequency in Hz, Default: 100000 Hz
> + - pnx,timeout: I2C bus timeout in milliseconds, Default: 10 ms

I'd like to repeat my question to the devicetree folks here: Can we have
timeout generic? It doesn't make sense to me to have that per vendor
again and again. I searched devicetree.org and EPAPR but couldn't find a
hint.

> + - slave-addr: Address used by the controller, Hardware default: 0x6e
> +
> +Examples:
> +
> +	i2c1: i2c at 400a0000 {
> +		compatible = "nxp,pnx-i2c";
> +		reg = <0x400a0000 0x100>;
> +		interrupt-parent = <&mic>;
> +		interrupts = <51 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +	};
> +
> +	i2c2: i2c at 400a8000 {
> +		compatible = "nxp,pnx-i2c";
> +		reg = <0x400a8000 0x100>;
> +		interrupt-parent = <&mic>;
> +		interrupts = <50 0>;
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		clock-frequency = <0x186a0>;
> +		pnx,timeout = <0x64>;

Did you change this, too? Timeouts are better readable in dec :)

> +		slave-addr = <0x11>;
> +	};
> --- linux-2.6.orig/drivers/i2c/busses/i2c-pnx.c
> +++ linux-2.6/drivers/i2c/busses/i2c-pnx.c
> @@ -23,10 +23,11 @@
>  #include <linux/err.h>
>  #include <linux/clk.h>
>  #include <linux/slab.h>
> +#include <linux/of_i2c.h>
>  
> -#define I2C_PNX_TIMEOUT		10 /* msec */
> -#define I2C_PNX_SPEED_KHZ	100
> -#define I2C_PNX_REGION_SIZE	0x100
> +#define I2C_PNX_TIMEOUT_DEFAULT		10 /* msec */
> +#define I2C_PNX_SPEED_KHZ_DEFAULT	100
> +#define I2C_PNX_REGION_SIZE		0x100
>  
>  enum {
>  	mstatus_tdi = 0x00000001,
> @@ -74,8 +75,9 @@ enum {
>  #define I2C_REG_TXS(a)	((a)->ioaddr + 0x28)	/* Tx slave FIFO (RO) */
>  #define I2C_REG_STFL(a)	((a)->ioaddr + 0x2c)	/* Tx slave FIFO level (RO) */
>  
> -static inline int wait_timeout(long timeout, struct i2c_pnx_algo_data *data)
> +static inline int wait_timeout(struct i2c_pnx_algo_data *data)
>  {
> +	long timeout = data->timeout;
>  	while (timeout > 0 &&
>  			(ioread32(I2C_REG_STS(data)) & mstatus_active)) {
>  		mdelay(1);
> @@ -84,8 +86,9 @@ static inline int wait_timeout(long time
>  	return (timeout <= 0);
>  }
>  
> -static inline int wait_reset(long timeout, struct i2c_pnx_algo_data *data)
> +static inline int wait_reset(struct i2c_pnx_algo_data *data)
>  {
> +	long timeout = data->timeout;
>  	while (timeout > 0 &&
>  			(ioread32(I2C_REG_CTL(data)) & mcntrl_reset)) {
>  		mdelay(1);
> @@ -97,7 +100,7 @@ static inline int wait_reset(long timeou
>  static inline void i2c_pnx_arm_timer(struct i2c_pnx_algo_data *alg_data)
>  {
>  	struct timer_list *timer = &alg_data->mif.timer;
> -	unsigned long expires = msecs_to_jiffies(I2C_PNX_TIMEOUT);
> +	unsigned long expires = msecs_to_jiffies(alg_data->timeout);
>  
>  	if (expires <= 1)
>  		expires = 2;
> @@ -135,7 +138,7 @@ static int i2c_pnx_start(unsigned char s
>  	}
>  
>  	/* First, make sure bus is idle */
> -	if (wait_timeout(I2C_PNX_TIMEOUT, alg_data)) {
> +	if (wait_timeout(alg_data)) {
>  		/* Somebody else is monopolizing the bus */
>  		dev_err(&alg_data->adapter.dev,
>  			"%s: Bus busy. Slave addr = %02x, cntrl = %x, stat = %x\n",
> @@ -228,7 +231,7 @@ static int i2c_pnx_master_xmit(struct i2
>  		if (alg_data->mif.len == 0) {
>  			if (alg_data->last) {
>  				/* Wait until the STOP is seen. */
> -				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
> +				if (wait_timeout(alg_data))
>  					dev_err(&alg_data->adapter.dev,
>  						"The bus is still active after timeout\n");
>  			}
> @@ -326,7 +329,7 @@ static int i2c_pnx_master_rcv(struct i2c
>  		if (alg_data->mif.len == 0) {
>  			if (alg_data->last)
>  				/* Wait until the STOP is seen. */
> -				if (wait_timeout(I2C_PNX_TIMEOUT, alg_data))
> +				if (wait_timeout(alg_data))
>  					dev_err(&alg_data->adapter.dev,
>  						"The bus is still active after timeout\n");
>  
> @@ -442,7 +445,7 @@ static void i2c_pnx_timeout(unsigned lon
>  
>  	ctl |= mcntrl_reset;
>  	iowrite32(ctl, I2C_REG_CTL(alg_data));
> -	wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +	wait_reset(alg_data);
>  	alg_data->mif.ret = -EIO;
>  	complete(&alg_data->mif.complete);
>  }
> @@ -457,18 +460,18 @@ static inline void bus_reset_if_active(s
>  			alg_data->adapter.name);
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	} else if (!(stat & mstatus_rfe) || !(stat & mstatus_tfe)) {
>  		/* If there is data in the fifo's after transfer,
>  		 * flush fifo's by reset.
>  		 */
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	} else if (stat & mstatus_nai) {
>  		iowrite32(ioread32(I2C_REG_CTL(alg_data)) | mcntrl_reset,
>  			  I2C_REG_CTL(alg_data));
> -		wait_reset(I2C_PNX_TIMEOUT, alg_data);
> +		wait_reset(alg_data);
>  	}
>  }
>  
> @@ -612,6 +615,8 @@ static int __devinit i2c_pnx_probe(struc
>  	struct i2c_pnx_algo_data *alg_data;
>  	unsigned long freq;
>  	struct resource *res;
> +	u32 speed = I2C_PNX_SPEED_KHZ_DEFAULT * 1000;
> +	u32 slave_addr = ~0;
>  
>  	alg_data = kzalloc(sizeof(*alg_data), GFP_KERNEL);
>  	if (!alg_data) {
> @@ -626,6 +631,18 @@ static int __devinit i2c_pnx_probe(struc
>  	alg_data->adapter.algo_data = alg_data;
>  	alg_data->adapter.nr = pdev->id;
>  
> +	alg_data->timeout = I2C_PNX_TIMEOUT_DEFAULT;
> +#ifdef CONFIG_OF
> +	alg_data->adapter.dev.of_node = of_node_get(pdev->dev.of_node);
> +	if (pdev->dev.of_node) {
> +		of_property_read_u32(pdev->dev.of_node, "pnx,timeout",
> +				     &alg_data->timeout);
> +		of_property_read_u32(pdev->dev.of_node, "clock-frequency",
> +				     &speed);
> +		of_property_read_u32(pdev->dev.of_node, "slave-addr",
> +				     &slave_addr);
> +	}
> +#endif
>  	alg_data->clk = clk_get(&pdev->dev, NULL);
>  	if (IS_ERR(alg_data->clk)) {
>  		ret = PTR_ERR(alg_data->clk);
> @@ -651,7 +668,7 @@ static int __devinit i2c_pnx_probe(struc
>  		dev_err(&pdev->dev,
>  		       "I/O region 0x%08x for I2C already in use.\n",
>  		       res->start);
> -		ret = -ENODEV;
> +		ret = -ENOMEM;
>  		goto out_clkget;
>  	}
>  
> @@ -667,6 +684,9 @@ static int __devinit i2c_pnx_probe(struc
>  	if (ret)
>  		goto out_unmap;
>  
> +	if (slave_addr != ~0)
> +		iowrite32(slave_addr, I2C_REG_ADR(alg_data));
> +
>  	freq = clk_get_rate(alg_data->clk);
>  
>  	/*
> @@ -680,14 +700,14 @@ static int __devinit i2c_pnx_probe(struc
>  	 * the deglitching filter length.
>  	 */
>  
> -	tmp = ((freq / 1000) / I2C_PNX_SPEED_KHZ) / 2 - 2;
> +	tmp = (freq / speed) / 2 - 2;
>  	if (tmp > 0x3FF)
>  		tmp = 0x3FF;
>  	iowrite32(tmp, I2C_REG_CKH(alg_data));
>  	iowrite32(tmp, I2C_REG_CKL(alg_data));
>  
>  	iowrite32(mcntrl_reset, I2C_REG_CTL(alg_data));
> -	if (wait_reset(I2C_PNX_TIMEOUT, alg_data)) {
> +	if (wait_reset(alg_data)) {
>  		ret = -ENODEV;
>  		goto out_clock;
>  	}
> @@ -699,7 +719,7 @@ static int __devinit i2c_pnx_probe(struc
>  		goto out_irq;
>  	}
>  	ret = request_irq(alg_data->irq, i2c_pnx_interrupt,
> -			0, pdev->name, alg_data);
> +			  0, pdev->name, alg_data);
>  	if (ret)
>  		goto out_clock;
>  
> @@ -710,6 +730,8 @@ static int __devinit i2c_pnx_probe(struc
>  		goto out_irq;
>  	}
>  
> +	of_i2c_register_devices(&alg_data->adapter);
> +
>  	dev_dbg(&pdev->dev, "%s: Master at %#8x, irq %d.\n",
>  		alg_data->adapter.name, res->start, alg_data->irq);
>  
> @@ -748,10 +770,19 @@ static int __devexit i2c_pnx_remove(stru
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id i2c_pnx_of_match[] = {
> +	{ .compatible = "nxp,pnx-i2c" },
> +	{ },
> +};
> +MODULE_DEVICE_TABLE(of, i2c_pnx_of_match);
> +#endif
> +
>  static struct platform_driver i2c_pnx_driver = {
>  	.driver = {
>  		.name = "pnx-i2c",
>  		.owner = THIS_MODULE,
> +		.of_match_table = of_match_ptr(i2c_pnx_of_match),
>  	},
>  	.probe = i2c_pnx_probe,
>  	.remove = __devexit_p(i2c_pnx_remove),
> --- linux-2.6.orig/include/linux/i2c-pnx.h
> +++ linux-2.6/include/linux/i2c-pnx.h
> @@ -32,6 +32,7 @@ struct i2c_pnx_algo_data {
>  	struct i2c_adapter	adapter;
>  	phys_addr_t		base;
>  	int			irq;
> +	u32			timeout;
>  };
>  
>  #endif /* __I2C_PNX_H__ */

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120419/3888eeab/attachment.sig>

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

Hi,

On 04/19/2012 06:07 PM, Wolfram Sang wrote:
>> + - clock-frequency: desired I2C bus clock frequency in Hz,
>> Default: 100000 Hz + - pnx,timeout: I2C bus timeout in
>> milliseconds, Default: 10 ms
> 
> I'd like to repeat my question to the devicetree folks here: Can we
> have timeout generic? It doesn't make sense to me to have that per
> vendor again and again.

Sounds completely reasonable.

To help make the devicetree conversion into 3.5, I would prefer to
integrate the "vendor,timeout" as-is if a longish standardization
process would block this, considering that we luckily only have 2-3 of
such timeout definitions currently.

I'd volunteer to clean up later by preparing a patch when there is a
standard timeout defined.

If we have a solution soon, I will prepare a new version of the patch,
of course, in the next days.

>> +	i2c2: i2c@400a8000 { +		compatible = "nxp,pnx-i2c"; +		reg =
>> <0x400a8000 0x100>; +		interrupt-parent = <&mic>; +		interrupts =
>> <50 0>; +		#address-cells = <1>; +		#size-cells = <0>; +
>> clock-frequency = <0x186a0>; +		pnx,timeout = <0x64>;
> 
> Did you change this, too? Timeouts are better readable in dec :)

Right. But even when removing the "0x" in the timeout line above, it's
still hex, see Documentation/devicetree/booting-without-of.txt

Or did I get sth. wrong?

Thanks,

Roland

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

Hi,

On 04/19/2012 06:07 PM, Wolfram Sang wrote:
>> + - clock-frequency: desired I2C bus clock frequency in Hz,
>> Default: 100000 Hz + - pnx,timeout: I2C bus timeout in
>> milliseconds, Default: 10 ms
> 
> I'd like to repeat my question to the devicetree folks here: Can we
> have timeout generic? It doesn't make sense to me to have that per
> vendor again and again.

Sounds completely reasonable.

To help make the devicetree conversion into 3.5, I would prefer to
integrate the "vendor,timeout" as-is if a longish standardization
process would block this, considering that we luckily only have 2-3 of
such timeout definitions currently.

I'd volunteer to clean up later by preparing a patch when there is a
standard timeout defined.

If we have a solution soon, I will prepare a new version of the patch,
of course, in the next days.

>> +	i2c2: i2c@400a8000 { +		compatible = "nxp,pnx-i2c"; +		reg =
>> <0x400a8000 0x100>; +		interrupt-parent = <&mic>; +		interrupts =
>> <50 0>; +		#address-cells = <1>; +		#size-cells = <0>; +
>> clock-frequency = <0x186a0>; +		pnx,timeout = <0x64>;
> 
> Did you change this, too? Timeouts are better readable in dec :)

Right. But even when removing the "0x" in the timeout line above, it's
still hex, see Documentation/devicetree/booting-without-of.txt

Or did I get sth. wrong?

Thanks,

Roland

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

Hi,

On 04/19/2012 06:07 PM, Wolfram Sang wrote:
>> + - clock-frequency: desired I2C bus clock frequency in Hz,
>> Default: 100000 Hz + - pnx,timeout: I2C bus timeout in
>> milliseconds, Default: 10 ms
> 
> I'd like to repeat my question to the devicetree folks here: Can we
> have timeout generic? It doesn't make sense to me to have that per
> vendor again and again.

Sounds completely reasonable.

To help make the devicetree conversion into 3.5, I would prefer to
integrate the "vendor,timeout" as-is if a longish standardization
process would block this, considering that we luckily only have 2-3 of
such timeout definitions currently.

I'd volunteer to clean up later by preparing a patch when there is a
standard timeout defined.

If we have a solution soon, I will prepare a new version of the patch,
of course, in the next days.

>> +	i2c2: i2c at 400a8000 { +		compatible = "nxp,pnx-i2c"; +		reg =
>> <0x400a8000 0x100>; +		interrupt-parent = <&mic>; +		interrupts =
>> <50 0>; +		#address-cells = <1>; +		#size-cells = <0>; +
>> clock-frequency = <0x186a0>; +		pnx,timeout = <0x64>;
> 
> Did you change this, too? Timeouts are better readable in dec :)

Right. But even when removing the "0x" in the timeout line above, it's
still hex, see Documentation/devicetree/booting-without-of.txt

Or did I get sth. wrong?

Thanks,

Roland

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]

On Thu, Apr 19, 2012 at 06:32:12PM +0200, Roland Stigge wrote:
> Hi,
> 
> On 04/19/2012 06:07 PM, Wolfram Sang wrote:
> >> + - clock-frequency: desired I2C bus clock frequency in Hz,
> >> Default: 100000 Hz + - pnx,timeout: I2C bus timeout in
> >> milliseconds, Default: 10 ms
> > 
> > I'd like to repeat my question to the devicetree folks here: Can we
> > have timeout generic? It doesn't make sense to me to have that per
> > vendor again and again.
> 
> Sounds completely reasonable.
> 
> To help make the devicetree conversion into 3.5, I would prefer to
> integrate the "vendor,timeout" as-is if a longish standardization
> process would block this, considering that we luckily only have 2-3 of
> such timeout definitions currently.

The "blocking" argument will be true for most dt-conversions. Only thing
that might change is that the argument will then be "we already have
9-10 of such timeouts, so we can have another one" ;)

I've been there before, if you make one exception once, other people
will nail you on that. I'd like to avoid that.

Let's just hope we can agree on "timeout" quickly and all will be fine.

> I'd volunteer to clean up later by preparing a patch when there is a
> standard timeout defined.

The problem with bindings is that you must support them forever once
introduced. Old device-trees should still work with newer kernels.
 
> If we have a solution soon, I will prepare a new version of the patch,
> of course, in the next days.

Thanks. One question, though: Will it really block dt-conversion? The
whole conversion should not be depending on the i2c-driver?

> > Did you change this, too? Timeouts are better readable in dec :)
> 
> Right. But even when removing the "0x" in the timeout line above, it's
> still hex, see Documentation/devicetree/booting-without-of.txt
> 
> Or did I get sth. wrong?

I think the document is probably outdated :( "clock-frequency" is also
without 0x and dec.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2167 bytes --]

On Thu, Apr 19, 2012 at 06:32:12PM +0200, Roland Stigge wrote:
> Hi,
> 
> On 04/19/2012 06:07 PM, Wolfram Sang wrote:
> >> + - clock-frequency: desired I2C bus clock frequency in Hz,
> >> Default: 100000 Hz + - pnx,timeout: I2C bus timeout in
> >> milliseconds, Default: 10 ms
> > 
> > I'd like to repeat my question to the devicetree folks here: Can we
> > have timeout generic? It doesn't make sense to me to have that per
> > vendor again and again.
> 
> Sounds completely reasonable.
> 
> To help make the devicetree conversion into 3.5, I would prefer to
> integrate the "vendor,timeout" as-is if a longish standardization
> process would block this, considering that we luckily only have 2-3 of
> such timeout definitions currently.

The "blocking" argument will be true for most dt-conversions. Only thing
that might change is that the argument will then be "we already have
9-10 of such timeouts, so we can have another one" ;)

I've been there before, if you make one exception once, other people
will nail you on that. I'd like to avoid that.

Let's just hope we can agree on "timeout" quickly and all will be fine.

> I'd volunteer to clean up later by preparing a patch when there is a
> standard timeout defined.

The problem with bindings is that you must support them forever once
introduced. Old device-trees should still work with newer kernels.
 
> If we have a solution soon, I will prepare a new version of the patch,
> of course, in the next days.

Thanks. One question, though: Will it really block dt-conversion? The
whole conversion should not be depending on the i2c-driver?

> > Did you change this, too? Timeouts are better readable in dec :)
> 
> Right. But even when removing the "0x" in the timeout line above, it's
> still hex, see Documentation/devicetree/booting-without-of.txt
> 
> Or did I get sth. wrong?

I think the document is probably outdated :( "clock-frequency" is also
without 0x and dec.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

On Thu, Apr 19, 2012 at 06:32:12PM +0200, Roland Stigge wrote:
> Hi,
> 
> On 04/19/2012 06:07 PM, Wolfram Sang wrote:
> >> + - clock-frequency: desired I2C bus clock frequency in Hz,
> >> Default: 100000 Hz + - pnx,timeout: I2C bus timeout in
> >> milliseconds, Default: 10 ms
> > 
> > I'd like to repeat my question to the devicetree folks here: Can we
> > have timeout generic? It doesn't make sense to me to have that per
> > vendor again and again.
> 
> Sounds completely reasonable.
> 
> To help make the devicetree conversion into 3.5, I would prefer to
> integrate the "vendor,timeout" as-is if a longish standardization
> process would block this, considering that we luckily only have 2-3 of
> such timeout definitions currently.

The "blocking" argument will be true for most dt-conversions. Only thing
that might change is that the argument will then be "we already have
9-10 of such timeouts, so we can have another one" ;)

I've been there before, if you make one exception once, other people
will nail you on that. I'd like to avoid that.

Let's just hope we can agree on "timeout" quickly and all will be fine.

> I'd volunteer to clean up later by preparing a patch when there is a
> standard timeout defined.

The problem with bindings is that you must support them forever once
introduced. Old device-trees should still work with newer kernels.
 
> If we have a solution soon, I will prepare a new version of the patch,
> of course, in the next days.

Thanks. One question, though: Will it really block dt-conversion? The
whole conversion should not be depending on the i2c-driver?

> > Did you change this, too? Timeouts are better readable in dec :)
> 
> Right. But even when removing the "0x" in the timeout line above, it's
> still hex, see Documentation/devicetree/booting-without-of.txt
> 
> Or did I get sth. wrong?

I think the document is probably outdated :( "clock-frequency" is also
without 0x and dec.

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120419/eb09c00a/attachment.sig>

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

Hi!

On 19/04/12 22:39, Wolfram Sang wrote:
>> If we have a solution soon, I will prepare a new version of the 
>> patch, of course, in the next days.
> 
> Thanks. One question, though: Will it really block dt-conversion? 
> The whole conversion should not be depending on the i2c-driver?

Technically, it would certainly be possible to convert some drivers
and leave others. But I would highly prefer to convert i2c also. With
LPC32xx, there are typically many I2C clients depending on DT on a
reasonable DT enabled mach.

I'm fine with "timeout" also, when you accept it into pnx-i2c.c and
will certainly provide a "timeout" patch update so you can pick the
version which suits best.

Another solution would be to not use timeout with the dt enabled
i2c-pnx for now (using the hard coded default timeout as the current
i2c-pnx.c does) and possibly introduce the (anyway optional) "timeout"
later.

Actually, that would be my favour.

What do you think?

>>> Did you change this, too? Timeouts are better readable in dec
>>> :)
>> 
>> Right. But even when removing the "0x" in the timeout line
>> above, it's still hex, see 
>> Documentation/devicetree/booting-without-of.txt
>> 
>> Or did I get sth. wrong?
> 
> I think the document is probably outdated :( "clock-frequency" is 
> also without 0x and dec.

Interesting! When the documentation is outdated - how does the parser
actually decide between hex (e.g. regs/addresses) and dec (e.g.
clock-frequency) in the absence of "0x"?

Thanks in advance,

Roland

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

Hi!

On 19/04/12 22:39, Wolfram Sang wrote:
>> If we have a solution soon, I will prepare a new version of the 
>> patch, of course, in the next days.
> 
> Thanks. One question, though: Will it really block dt-conversion? 
> The whole conversion should not be depending on the i2c-driver?

Technically, it would certainly be possible to convert some drivers
and leave others. But I would highly prefer to convert i2c also. With
LPC32xx, there are typically many I2C clients depending on DT on a
reasonable DT enabled mach.

I'm fine with "timeout" also, when you accept it into pnx-i2c.c and
will certainly provide a "timeout" patch update so you can pick the
version which suits best.

Another solution would be to not use timeout with the dt enabled
i2c-pnx for now (using the hard coded default timeout as the current
i2c-pnx.c does) and possibly introduce the (anyway optional) "timeout"
later.

Actually, that would be my favour.

What do you think?

>>> Did you change this, too? Timeouts are better readable in dec
>>> :)
>> 
>> Right. But even when removing the "0x" in the timeout line
>> above, it's still hex, see 
>> Documentation/devicetree/booting-without-of.txt
>> 
>> Or did I get sth. wrong?
> 
> I think the document is probably outdated :( "clock-frequency" is 
> also without 0x and dec.

Interesting! When the documentation is outdated - how does the parser
actually decide between hex (e.g. regs/addresses) and dec (e.g.
clock-frequency) in the absence of "0x"?

Thanks in advance,

Roland

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

Hi!

On 19/04/12 22:39, Wolfram Sang wrote:
>> If we have a solution soon, I will prepare a new version of the 
>> patch, of course, in the next days.
> 
> Thanks. One question, though: Will it really block dt-conversion? 
> The whole conversion should not be depending on the i2c-driver?

Technically, it would certainly be possible to convert some drivers
and leave others. But I would highly prefer to convert i2c also. With
LPC32xx, there are typically many I2C clients depending on DT on a
reasonable DT enabled mach.

I'm fine with "timeout" also, when you accept it into pnx-i2c.c and
will certainly provide a "timeout" patch update so you can pick the
version which suits best.

Another solution would be to not use timeout with the dt enabled
i2c-pnx for now (using the hard coded default timeout as the current
i2c-pnx.c does) and possibly introduce the (anyway optional) "timeout"
later.

Actually, that would be my favour.

What do you think?

>>> Did you change this, too? Timeouts are better readable in dec
>>> :)
>> 
>> Right. But even when removing the "0x" in the timeout line
>> above, it's still hex, see 
>> Documentation/devicetree/booting-without-of.txt
>> 
>> Or did I get sth. wrong?
> 
> I think the document is probably outdated :( "clock-frequency" is 
> also without 0x and dec.

Interesting! When the documentation is outdated - how does the parser
actually decide between hex (e.g. regs/addresses) and dec (e.g.
clock-frequency) in the absence of "0x"?

Thanks in advance,

Roland

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]


> Another solution would be to not use timeout with the dt enabled
> i2c-pnx for now (using the hard coded default timeout as the current
> i2c-pnx.c does) and possibly introduce the (anyway optional) "timeout"
> later.

Yeah, let's do it like this.

> Interesting! When the documentation is outdated - how does the parser
> actually decide between hex (e.g. regs/addresses) and dec (e.g.
> clock-frequency) in the absence of "0x"?

Are there regs without 0x?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 659 bytes --]


> Another solution would be to not use timeout with the dt enabled
> i2c-pnx for now (using the hard coded default timeout as the current
> i2c-pnx.c does) and possibly introduce the (anyway optional) "timeout"
> later.

Yeah, let's do it like this.

> Interesting! When the documentation is outdated - how does the parser
> actually decide between hex (e.g. regs/addresses) and dec (e.g.
> clock-frequency) in the absence of "0x"?

Are there regs without 0x?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Wolfram Sang]

> Another solution would be to not use timeout with the dt enabled
> i2c-pnx for now (using the hard coded default timeout as the current
> i2c-pnx.c does) and possibly introduce the (anyway optional) "timeout"
> later.

Yeah, let's do it like this.

> Interesting! When the documentation is outdated - how does the parser
> actually decide between hex (e.g. regs/addresses) and dec (e.g.
> clock-frequency) in the absence of "0x"?

Are there regs without 0x?

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 198 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120420/40b6643a/attachment.sig>

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

On 04/20/2012 09:28 AM, Wolfram Sang wrote:
>> Another solution would be to not use timeout with the dt enabled 
>> i2c-pnx for now (using the hard coded default timeout as the
>> current i2c-pnx.c does) and possibly introduce the (anyway
>> optional) "timeout" later.
> 
> Yeah, let's do it like this.

Good, will do!

>> Interesting! When the documentation is outdated - how does the
>> parser actually decide between hex (e.g. regs/addresses) and dec
>> (e.g. clock-frequency) in the absence of "0x"?
> 
> Are there regs without 0x?

Lots of, in Documentation/devicetree/booting-without-of.txt ;-)
Admittedly, not in arch/arm/boot/dts/*...

I would like to clarify the hex/dec issue and the various examples to
prevent confusion of others. Will post a patch.

Roland

Re: [PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

On 04/20/2012 09:28 AM, Wolfram Sang wrote:
>> Another solution would be to not use timeout with the dt enabled 
>> i2c-pnx for now (using the hard coded default timeout as the
>> current i2c-pnx.c does) and possibly introduce the (anyway
>> optional) "timeout" later.
> 
> Yeah, let's do it like this.

Good, will do!

>> Interesting! When the documentation is outdated - how does the
>> parser actually decide between hex (e.g. regs/addresses) and dec
>> (e.g. clock-frequency) in the absence of "0x"?
> 
> Are there regs without 0x?

Lots of, in Documentation/devicetree/booting-without-of.txt ;-)
Admittedly, not in arch/arm/boot/dts/*...

I would like to clarify the hex/dec issue and the various examples to
prevent confusion of others. Will post a patch.

Roland

[PATCH v4] i2c: Add device tree support to i2c-pnx.c

[Roland Stigge]

On 04/20/2012 09:28 AM, Wolfram Sang wrote:
>> Another solution would be to not use timeout with the dt enabled 
>> i2c-pnx for now (using the hard coded default timeout as the
>> current i2c-pnx.c does) and possibly introduce the (anyway
>> optional) "timeout" later.
> 
> Yeah, let's do it like this.

Good, will do!

>> Interesting! When the documentation is outdated - how does the
>> parser actually decide between hex (e.g. regs/addresses) and dec
>> (e.g. clock-frequency) in the absence of "0x"?
> 
> Are there regs without 0x?

Lots of, in Documentation/devicetree/booting-without-of.txt ;-)
Admittedly, not in arch/arm/boot/dts/*...

I would like to clarify the hex/dec issue and the various examples to
prevent confusion of others. Will post a patch.

Roland