[PATCH] i2c-designware: Interrupt sharing and SDA hold time

[PATCH] i2c-designware: Interrupt sharing and SDA hold time

[Christian Ruppert]

This patch implements interrupt sharing and SDA hold time configuration in
the designware i2c driver. Both functions are enabled through platform data
or device tree. Tested with Linux-3.8rc4 (Synopsys ARC port, see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline).

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   18 ++++++++++
 drivers/i2c/busses/i2c-designware-core.c           |    6 +++
 drivers/i2c/busses/i2c-designware-core.h           |    2 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |   34 +++++++++++++++++++-
 include/linux/i2c/i2c-designware.h                 |   13 +++++++
 5 files changed, 72 insertions(+), 1 deletions(-)
 create mode 100644 include/linux/i2c/i2c-designware.h

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..474386b 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,12 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - interrupts-shared : If this property is present, the interrupt line is
+   considered shared with other devices.
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+   be initialised with its value.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +26,15 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		interrupts-shared;
+		clock-frequency = <400000>;
+		sda-hold-time = <0x64>;
+	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index cbba7db..36ba0b5 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -34,6 +34,7 @@
 #include <linux/io.h>
 #include <linux/pm_runtime.h>
 #include <linux/delay.h>
+#include <linux/i2c/i2c-designware.h>
 #include "i2c-designware-core.h"
 
 /*
@@ -66,6 +67,7 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_TYPE		0xfc
@@ -309,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SET_HOLDTIME)
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..ef20477 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,8 @@ struct dw_i2c_dev {
 	u32			master_cfg;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
+	u32			sda_hold_time;
+	unsigned int		pdat_flags;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 343357a..2ad2da3 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,11 +34,13 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
 #include <linux/io.h>
 #include <linux/slab.h>
+#include <linux/i2c/i2c-designware.h>
 #include "i2c-designware-core.h"
 
 static struct i2c_algorithm i2c_dw_algo = {
@@ -56,6 +58,28 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct resource *mem, *ioarea;
 	int irq, r;
+	unsigned long irqf;
+	struct i2c_designware_platform_data *pdat = pdev->dev.platform_data;
+
+	if ((!pdat) && pdev->dev.of_node) {
+		struct device_node *node = pdev->dev.of_node;
+
+		pdat = kmalloc(sizeof(struct i2c_designware_platform_data),
+				GFP_KERNEL);
+		if (!pdat)
+			return -ENOMEM;
+
+		pdat->flags = 0;
+
+		if (of_get_property(node, "interrupts-shared", NULL))
+			pdat->flags |= I2C_DW_PDAT_FLAGS_SHARE_IRQ;
+
+		if (!of_property_read_u32(node, "sda-hold-time",
+					&pdat->sda_hold_time))
+			pdat->flags |= I2C_DW_PDAT_FLAGS_SET_HOLDTIME;
+
+		pdev->dev.platform_data = pdat;
+	}
 
 	/* NOTE: driver uses the static register mapping */
 	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
@@ -120,12 +144,20 @@ static int dw_i2c_probe(struct platform_device *pdev)
 		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
 		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
 	}
+	dev->sda_hold_time = pdat->sda_hold_time;
+	dev->pdat_flags = pdat->flags;
+
 	r = i2c_dw_init(dev);
 	if (r)
 		goto err_iounmap;
 
 	i2c_dw_disable_int(dev);
-	r = request_irq(dev->irq, i2c_dw_isr, IRQF_DISABLED, pdev->name, dev);
+
+	irqf = IRQF_DISABLED;
+	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SHARE_IRQ)
+		irqf |= IRQF_SHARED;
+
+	r = request_irq(dev->irq, i2c_dw_isr, irqf, pdev->name, dev);
 	if (r) {
 		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
 		goto err_iounmap;
diff --git a/include/linux/i2c/i2c-designware.h b/include/linux/i2c/i2c-designware.h
new file mode 100644
index 0000000..ea8192a
--- /dev/null
+++ b/include/linux/i2c/i2c-designware.h
@@ -0,0 +1,13 @@
+#ifndef __I2C_DESIGNWARE_H__
+#define __I2C_DESIGNWARE_H__
+
+#include <linux/platform_device.h>
+
+struct i2c_designware_platform_data {
+	unsigned int flags;
+#define I2C_DW_PDAT_FLAGS_SHARE_IRQ    (1L<<0)
+#define I2C_DW_PDAT_FLAGS_SET_HOLDTIME (1L<<1)
+	u32          sda_hold_time;
+};
+
+#endif /* __I2C_DESIGNWARE_H__ */
-- 
1.7.1

Re: [PATCH] i2c-designware: Interrupt sharing and SDA hold time

[Mika Westerberg]

On Fri, Feb 15, 2013 at 11:11:50AM +0100, Christian Ruppert wrote:
> This patch implements interrupt sharing and SDA hold time configuration in
> the designware i2c driver. Both functions are enabled through platform data
> or device tree. Tested with Linux-3.8rc4 (Synopsys ARC port, see
> https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline).
> 
> Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   18 ++++++++++
>  drivers/i2c/busses/i2c-designware-core.c           |    6 +++
>  drivers/i2c/busses/i2c-designware-core.h           |    2 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |   34 +++++++++++++++++++-
>  include/linux/i2c/i2c-designware.h                 |   13 +++++++
>  5 files changed, 72 insertions(+), 1 deletions(-)
>  create mode 100644 include/linux/i2c/i2c-designware.h
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index e42a2ee..474386b 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -10,6 +10,12 @@ Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
> +Optional properties :
> + - interrupts-shared : If this property is present, the interrupt line is
> +   considered shared with other devices.
> + - sda-hold-time : If this property is present, the register SDA_HOLD will
> +   be initialised with its value.
> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -20,3 +26,15 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> +
> +	i2c@1120000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "snps,designware-i2c";
> +		reg = <0x1120000 0x1000>;
> +		interrupt-parent = <&ictl>;
> +		interrupts = <12 1>;
> +		interrupts-shared;
> +		clock-frequency = <400000>;
> +		sda-hold-time = <0x64>;
> +	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index cbba7db..36ba0b5 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -34,6 +34,7 @@
>  #include <linux/io.h>
>  #include <linux/pm_runtime.h>
>  #include <linux/delay.h>
> +#include <linux/i2c/i2c-designware.h>
>  #include "i2c-designware-core.h"
>  
>  /*
> @@ -66,6 +67,7 @@
>  #define DW_IC_STATUS		0x70
>  #define DW_IC_TXFLR		0x74
>  #define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
>  #define DW_IC_TX_ABRT_SOURCE	0x80
>  #define DW_IC_COMP_PARAM_1	0xf4
>  #define DW_IC_COMP_TYPE		0xfc
> @@ -309,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
> +	/* Configure SDA Hold Time if required */
> +	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SET_HOLDTIME)
> +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);

AFAIK, DW_IC_SDA_HOLD minimum value is 1 so you could just do:

	if (dev->sda_hold_time)
		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);

Also should this be configurable for standard and fast modes both?
Something like sm_sda_hold_time and fm_sda_hold_time?

> +
>  	/* Configure Tx/Rx FIFO threshold levels */
>  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
>  	dw_writel(dev, 0, DW_IC_RX_TL);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 9c1840e..ef20477 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -88,6 +88,8 @@ struct dw_i2c_dev {
>  	u32			master_cfg;
>  	unsigned int		tx_fifo_depth;
>  	unsigned int		rx_fifo_depth;
> +	u32			sda_hold_time;
> +	unsigned int		pdat_flags;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 343357a..2ad2da3 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -34,11 +34,13 @@
>  #include <linux/sched.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
>  #include <linux/io.h>
>  #include <linux/slab.h>
> +#include <linux/i2c/i2c-designware.h>
>  #include "i2c-designware-core.h"
>  
>  static struct i2c_algorithm i2c_dw_algo = {
> @@ -56,6 +58,28 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	struct i2c_adapter *adap;
>  	struct resource *mem, *ioarea;
>  	int irq, r;
> +	unsigned long irqf;
> +	struct i2c_designware_platform_data *pdat = pdev->dev.platform_data;
> +
> +	if ((!pdat) && pdev->dev.of_node) {
> +		struct device_node *node = pdev->dev.of_node;
> +
> +		pdat = kmalloc(sizeof(struct i2c_designware_platform_data),
> +				GFP_KERNEL);
> +		if (!pdat)
> +			return -ENOMEM;
> +
> +		pdat->flags = 0;
> +
> +		if (of_get_property(node, "interrupts-shared", NULL))
> +			pdat->flags |= I2C_DW_PDAT_FLAGS_SHARE_IRQ;
> +
> +		if (!of_property_read_u32(node, "sda-hold-time",
> +					&pdat->sda_hold_time))
> +			pdat->flags |= I2C_DW_PDAT_FLAGS_SET_HOLDTIME;
> +
> +		pdev->dev.platform_data = pdat;

I do not understand why you need a separate platform data at all? Can't you
do it in a same way than the ACPI code?

And this probably doesn't apply to the i2c/next branch because we added the
ACPI enumeration there.

> +	}
>  
>  	/* NOTE: driver uses the static register mapping */
>  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> @@ -120,12 +144,20 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
>  		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
>  	}
> +	dev->sda_hold_time = pdat->sda_hold_time;
> +	dev->pdat_flags = pdat->flags;
> +
>  	r = i2c_dw_init(dev);
>  	if (r)
>  		goto err_iounmap;
>  
>  	i2c_dw_disable_int(dev);
> -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_DISABLED, pdev->name, dev);
> +
> +	irqf = IRQF_DISABLED;
> +	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SHARE_IRQ)
> +		irqf |= IRQF_SHARED;

Please check Wolfram's i2c/next. This is set already there.

> +
> +	r = request_irq(dev->irq, i2c_dw_isr, irqf, pdev->name, dev);
>  	if (r) {
>  		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
>  		goto err_iounmap;
> diff --git a/include/linux/i2c/i2c-designware.h b/include/linux/i2c/i2c-designware.h
> new file mode 100644
> index 0000000..ea8192a
> --- /dev/null
> +++ b/include/linux/i2c/i2c-designware.h
> @@ -0,0 +1,13 @@
> +#ifndef __I2C_DESIGNWARE_H__
> +#define __I2C_DESIGNWARE_H__
> +
> +#include <linux/platform_device.h>
> +
> +struct i2c_designware_platform_data {
> +	unsigned int flags;
> +#define I2C_DW_PDAT_FLAGS_SHARE_IRQ    (1L<<0)
> +#define I2C_DW_PDAT_FLAGS_SET_HOLDTIME (1L<<1)
> +	u32          sda_hold_time;
> +};
> +
> +#endif /* __I2C_DESIGNWARE_H__ */
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH] i2c-designware: Interrupt sharing and SDA hold time

[Christian Ruppert]

On Fri, Feb 15, 2013 at 01:50:37PM +0200, Mika Westerberg wrote:
> On Fri, Feb 15, 2013 at 11:11:50AM +0100, Christian Ruppert wrote:
> > This patch implements interrupt sharing and SDA hold time configuration in
> > the designware i2c driver. Both functions are enabled through platform data
> > or device tree. Tested with Linux-3.8rc4 (Synopsys ARC port, see
> > https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline).
> > 
> > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > ---
> >  .../devicetree/bindings/i2c/i2c-designware.txt     |   18 ++++++++++
> >  drivers/i2c/busses/i2c-designware-core.c           |    6 +++
> >  drivers/i2c/busses/i2c-designware-core.h           |    2 +
> >  drivers/i2c/busses/i2c-designware-platdrv.c        |   34 +++++++++++++++++++-
> >  include/linux/i2c/i2c-designware.h                 |   13 +++++++
> >  5 files changed, 72 insertions(+), 1 deletions(-)
> >  create mode 100644 include/linux/i2c/i2c-designware.h
> > 
> > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > index e42a2ee..474386b 100644
> > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > @@ -10,6 +10,12 @@ Recommended properties :
> >  
> >   - clock-frequency : desired I2C bus clock frequency in Hz.
> >  
> > +Optional properties :
> > + - interrupts-shared : If this property is present, the interrupt line is
> > +   considered shared with other devices.
> > + - sda-hold-time : If this property is present, the register SDA_HOLD will
> > +   be initialised with its value.
> > +
> >  Example :
> >  
> >  	i2c@f0000 {
> > @@ -20,3 +26,15 @@ Example :
> >  		interrupts = <11>;
> >  		clock-frequency = <400000>;
> >  	};
> > +
> > +	i2c@1120000 {
> > +		#address-cells = <1>;
> > +		#size-cells = <0>;
> > +		compatible = "snps,designware-i2c";
> > +		reg = <0x1120000 0x1000>;
> > +		interrupt-parent = <&ictl>;
> > +		interrupts = <12 1>;
> > +		interrupts-shared;
> > +		clock-frequency = <400000>;
> > +		sda-hold-time = <0x64>;
> > +	};
> > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > index cbba7db..36ba0b5 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.c
> > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > @@ -34,6 +34,7 @@
> >  #include <linux/io.h>
> >  #include <linux/pm_runtime.h>
> >  #include <linux/delay.h>
> > +#include <linux/i2c/i2c-designware.h>
> >  #include "i2c-designware-core.h"
> >  
> >  /*
> > @@ -66,6 +67,7 @@
> >  #define DW_IC_STATUS		0x70
> >  #define DW_IC_TXFLR		0x74
> >  #define DW_IC_RXFLR		0x78
> > +#define DW_IC_SDA_HOLD		0x7c
> >  #define DW_IC_TX_ABRT_SOURCE	0x80
> >  #define DW_IC_COMP_PARAM_1	0xf4
> >  #define DW_IC_COMP_TYPE		0xfc
> > @@ -309,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> >  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
> >  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
> >  
> > +	/* Configure SDA Hold Time if required */
> > +	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SET_HOLDTIME)
> > +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> 
> AFAIK, DW_IC_SDA_HOLD minimum value is 1 so you could just do:
> 
> 	if (dev->sda_hold_time)
> 		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);

This is correct, I'll modify it.

> Also should this be configurable for standard and fast modes both?
> Something like sm_sda_hold_time and fm_sda_hold_time?

AFAIK the requirements for data hold time do not differ between standard
and fast mode. The hardware seems to have been designed in the same
understanding since it provides two separate register sets for clock
timings in standard and fast modes but only one register for data hold
time. Am I missing something here?

> > +
> >  	/* Configure Tx/Rx FIFO threshold levels */
> >  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
> >  	dw_writel(dev, 0, DW_IC_RX_TL);
> > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> > index 9c1840e..ef20477 100644
> > --- a/drivers/i2c/busses/i2c-designware-core.h
> > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > @@ -88,6 +88,8 @@ struct dw_i2c_dev {
> >  	u32			master_cfg;
> >  	unsigned int		tx_fifo_depth;
> >  	unsigned int		rx_fifo_depth;
> > +	u32			sda_hold_time;
> > +	unsigned int		pdat_flags;
> >  };
> >  
> >  #define ACCESS_SWAP		0x00000001
> > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > index 343357a..2ad2da3 100644
> > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > @@ -34,11 +34,13 @@
> >  #include <linux/sched.h>
> >  #include <linux/err.h>
> >  #include <linux/interrupt.h>
> > +#include <linux/of.h>
> >  #include <linux/of_i2c.h>
> >  #include <linux/platform_device.h>
> >  #include <linux/pm.h>
> >  #include <linux/io.h>
> >  #include <linux/slab.h>
> > +#include <linux/i2c/i2c-designware.h>
> >  #include "i2c-designware-core.h"
> >  
> >  static struct i2c_algorithm i2c_dw_algo = {
> > @@ -56,6 +58,28 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  	struct i2c_adapter *adap;
> >  	struct resource *mem, *ioarea;
> >  	int irq, r;
> > +	unsigned long irqf;
> > +	struct i2c_designware_platform_data *pdat = pdev->dev.platform_data;
> > +
> > +	if ((!pdat) && pdev->dev.of_node) {
> > +		struct device_node *node = pdev->dev.of_node;
> > +
> > +		pdat = kmalloc(sizeof(struct i2c_designware_platform_data),
> > +				GFP_KERNEL);
> > +		if (!pdat)
> > +			return -ENOMEM;
> > +
> > +		pdat->flags = 0;
> > +
> > +		if (of_get_property(node, "interrupts-shared", NULL))
> > +			pdat->flags |= I2C_DW_PDAT_FLAGS_SHARE_IRQ;
> > +
> > +		if (!of_property_read_u32(node, "sda-hold-time",
> > +					&pdat->sda_hold_time))
> > +			pdat->flags |= I2C_DW_PDAT_FLAGS_SET_HOLDTIME;
> > +
> > +		pdev->dev.platform_data = pdat;
> 
> I do not understand why you need a separate platform data at all? Can't you
> do it in a same way than the ACPI code?

I suppose you are referring to dw_i2c_acpi_configure() in i2c/next which
directly sets the values dev->... This is perfectly feasible and would
greatly simplify the patch. The reason I introduced the platform data
indirection layer is compatibility with legacy instantiations and
platforms that do not yet support device tree. We do not internally need
that backwards compatibility but does anybody else? Otherwise I'll
remove it.

> And this probably doesn't apply to the i2c/next branch because we added the
> ACPI enumeration there.

No problem, I'll check the next patch iteration wrt. i2c/next which I
should have probably done in the first place.

> > +	}
> >  
> >  	/* NOTE: driver uses the static register mapping */
> >  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > @@ -120,12 +144,20 @@ static int dw_i2c_probe(struct platform_device *pdev)
> >  		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> >  		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> >  	}
> > +	dev->sda_hold_time = pdat->sda_hold_time;
> > +	dev->pdat_flags = pdat->flags;
> > +
> >  	r = i2c_dw_init(dev);
> >  	if (r)
> >  		goto err_iounmap;
> >  
> >  	i2c_dw_disable_int(dev);
> > -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_DISABLED, pdev->name, dev);
> > +
> > +	irqf = IRQF_DISABLED;
> > +	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SHARE_IRQ)
> > +		irqf |= IRQF_SHARED;
> 
> Please check Wolfram's i2c/next. This is set already there.

Perfect. My fault for not looking there first. Thanks for highlighting
this.

> > +
> > +	r = request_irq(dev->irq, i2c_dw_isr, irqf, pdev->name, dev);
> >  	if (r) {
> >  		dev_err(&pdev->dev, "failure requesting irq %i\n", dev->irq);
> >  		goto err_iounmap;
> > diff --git a/include/linux/i2c/i2c-designware.h b/include/linux/i2c/i2c-designware.h
> > new file mode 100644
> > index 0000000..ea8192a
> > --- /dev/null
> > +++ b/include/linux/i2c/i2c-designware.h
> > @@ -0,0 +1,13 @@
> > +#ifndef __I2C_DESIGNWARE_H__
> > +#define __I2C_DESIGNWARE_H__
> > +
> > +#include <linux/platform_device.h>
> > +
> > +struct i2c_designware_platform_data {
> > +	unsigned int flags;
> > +#define I2C_DW_PDAT_FLAGS_SHARE_IRQ    (1L<<0)
> > +#define I2C_DW_PDAT_FLAGS_SET_HOLDTIME (1L<<1)
> > +	u32          sda_hold_time;
> > +};
> > +
> > +#endif /* __I2C_DESIGNWARE_H__ */
> > -- 
> > 1.7.1
> > 
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at  http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at  http://www.tux.org/lkml/

-- 
  Christian Ruppert              ,          <christian.ruppert@abilis.com>
                                /|
  Tel: +41/(0)22 816 19-42     //|                 3, Chemin du Pré-Fleuri
                             _// | bilis Systems   CH-1228 Plan-les-Ouates

Re: [PATCH] i2c-designware: Interrupt sharing and SDA hold time

[Mika Westerberg]

On Fri, Feb 15, 2013 at 02:05:10PM +0100, Christian Ruppert wrote:
> On Fri, Feb 15, 2013 at 01:50:37PM +0200, Mika Westerberg wrote:
> > On Fri, Feb 15, 2013 at 11:11:50AM +0100, Christian Ruppert wrote:
> > > This patch implements interrupt sharing and SDA hold time configuration in
> > > the designware i2c driver. Both functions are enabled through platform data
> > > or device tree. Tested with Linux-3.8rc4 (Synopsys ARC port, see
> > > https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline).
> > > 
> > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > > ---
> > >  .../devicetree/bindings/i2c/i2c-designware.txt     |   18 ++++++++++
> > >  drivers/i2c/busses/i2c-designware-core.c           |    6 +++
> > >  drivers/i2c/busses/i2c-designware-core.h           |    2 +
> > >  drivers/i2c/busses/i2c-designware-platdrv.c        |   34 +++++++++++++++++++-
> > >  include/linux/i2c/i2c-designware.h                 |   13 +++++++
> > >  5 files changed, 72 insertions(+), 1 deletions(-)
> > >  create mode 100644 include/linux/i2c/i2c-designware.h
> > > 
> > > diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > index e42a2ee..474386b 100644
> > > --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> > > @@ -10,6 +10,12 @@ Recommended properties :
> > >  
> > >   - clock-frequency : desired I2C bus clock frequency in Hz.
> > >  
> > > +Optional properties :
> > > + - interrupts-shared : If this property is present, the interrupt line is
> > > +   considered shared with other devices.
> > > + - sda-hold-time : If this property is present, the register SDA_HOLD will
> > > +   be initialised with its value.
> > > +
> > >  Example :
> > >  
> > >  	i2c@f0000 {
> > > @@ -20,3 +26,15 @@ Example :
> > >  		interrupts = <11>;
> > >  		clock-frequency = <400000>;
> > >  	};
> > > +
> > > +	i2c@1120000 {
> > > +		#address-cells = <1>;
> > > +		#size-cells = <0>;
> > > +		compatible = "snps,designware-i2c";
> > > +		reg = <0x1120000 0x1000>;
> > > +		interrupt-parent = <&ictl>;
> > > +		interrupts = <12 1>;
> > > +		interrupts-shared;
> > > +		clock-frequency = <400000>;
> > > +		sda-hold-time = <0x64>;
> > > +	};
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> > > index cbba7db..36ba0b5 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.c
> > > +++ b/drivers/i2c/busses/i2c-designware-core.c
> > > @@ -34,6 +34,7 @@
> > >  #include <linux/io.h>
> > >  #include <linux/pm_runtime.h>
> > >  #include <linux/delay.h>
> > > +#include <linux/i2c/i2c-designware.h>
> > >  #include "i2c-designware-core.h"
> > >  
> > >  /*
> > > @@ -66,6 +67,7 @@
> > >  #define DW_IC_STATUS		0x70
> > >  #define DW_IC_TXFLR		0x74
> > >  #define DW_IC_RXFLR		0x78
> > > +#define DW_IC_SDA_HOLD		0x7c
> > >  #define DW_IC_TX_ABRT_SOURCE	0x80
> > >  #define DW_IC_COMP_PARAM_1	0xf4
> > >  #define DW_IC_COMP_TYPE		0xfc
> > > @@ -309,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
> > >  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
> > >  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
> > >  
> > > +	/* Configure SDA Hold Time if required */
> > > +	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SET_HOLDTIME)
> > > +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> > 
> > AFAIK, DW_IC_SDA_HOLD minimum value is 1 so you could just do:
> > 
> > 	if (dev->sda_hold_time)
> > 		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> 
> This is correct, I'll modify it.
> 
> > Also should this be configurable for standard and fast modes both?
> > Something like sm_sda_hold_time and fm_sda_hold_time?
> 
> AFAIK the requirements for data hold time do not differ between standard
> and fast mode. The hardware seems to have been designed in the same
> understanding since it provides two separate register sets for clock
> timings in standard and fast modes but only one register for data hold
> time. Am I missing something here?

No, you are probably right. We can always add support for the other mode in
the driver if there is need for that.

> > > +
> > >  	/* Configure Tx/Rx FIFO threshold levels */
> > >  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
> > >  	dw_writel(dev, 0, DW_IC_RX_TL);
> > > diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> > > index 9c1840e..ef20477 100644
> > > --- a/drivers/i2c/busses/i2c-designware-core.h
> > > +++ b/drivers/i2c/busses/i2c-designware-core.h
> > > @@ -88,6 +88,8 @@ struct dw_i2c_dev {
> > >  	u32			master_cfg;
> > >  	unsigned int		tx_fifo_depth;
> > >  	unsigned int		rx_fifo_depth;
> > > +	u32			sda_hold_time;
> > > +	unsigned int		pdat_flags;
> > >  };
> > >  
> > >  #define ACCESS_SWAP		0x00000001
> > > diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > index 343357a..2ad2da3 100644
> > > --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> > > +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> > > @@ -34,11 +34,13 @@
> > >  #include <linux/sched.h>
> > >  #include <linux/err.h>
> > >  #include <linux/interrupt.h>
> > > +#include <linux/of.h>
> > >  #include <linux/of_i2c.h>
> > >  #include <linux/platform_device.h>
> > >  #include <linux/pm.h>
> > >  #include <linux/io.h>
> > >  #include <linux/slab.h>
> > > +#include <linux/i2c/i2c-designware.h>
> > >  #include "i2c-designware-core.h"
> > >  
> > >  static struct i2c_algorithm i2c_dw_algo = {
> > > @@ -56,6 +58,28 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > >  	struct i2c_adapter *adap;
> > >  	struct resource *mem, *ioarea;
> > >  	int irq, r;
> > > +	unsigned long irqf;
> > > +	struct i2c_designware_platform_data *pdat = pdev->dev.platform_data;
> > > +
> > > +	if ((!pdat) && pdev->dev.of_node) {
> > > +		struct device_node *node = pdev->dev.of_node;
> > > +
> > > +		pdat = kmalloc(sizeof(struct i2c_designware_platform_data),
> > > +				GFP_KERNEL);
> > > +		if (!pdat)
> > > +			return -ENOMEM;
> > > +
> > > +		pdat->flags = 0;
> > > +
> > > +		if (of_get_property(node, "interrupts-shared", NULL))
> > > +			pdat->flags |= I2C_DW_PDAT_FLAGS_SHARE_IRQ;
> > > +
> > > +		if (!of_property_read_u32(node, "sda-hold-time",
> > > +					&pdat->sda_hold_time))
> > > +			pdat->flags |= I2C_DW_PDAT_FLAGS_SET_HOLDTIME;
> > > +
> > > +		pdev->dev.platform_data = pdat;
> > 
> > I do not understand why you need a separate platform data at all? Can't you
> > do it in a same way than the ACPI code?
> 
> I suppose you are referring to dw_i2c_acpi_configure() in i2c/next which
> directly sets the values dev->... This is perfectly feasible and would
> greatly simplify the patch. The reason I introduced the platform data
> indirection layer is compatibility with legacy instantiations and
> platforms that do not yet support device tree. We do not internally need
> that backwards compatibility but does anybody else? Otherwise I'll
> remove it.

Well, there wasn't any platform data before so I'm not sure if it breaks
anything. And if you only instantiate the new fields once you know that the
device has been enumerated from DT, you should be fine.

> 
> > And this probably doesn't apply to the i2c/next branch because we added the
> > ACPI enumeration there.
> 
> No problem, I'll check the next patch iteration wrt. i2c/next which I
> should have probably done in the first place.

Cool.

> 
> > > +	}
> > >  
> > >  	/* NOTE: driver uses the static register mapping */
> > >  	mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > > @@ -120,12 +144,20 @@ static int dw_i2c_probe(struct platform_device *pdev)
> > >  		dev->tx_fifo_depth = ((param1 >> 16) & 0xff) + 1;
> > >  		dev->rx_fifo_depth = ((param1 >> 8)  & 0xff) + 1;
> > >  	}
> > > +	dev->sda_hold_time = pdat->sda_hold_time;
> > > +	dev->pdat_flags = pdat->flags;
> > > +
> > >  	r = i2c_dw_init(dev);
> > >  	if (r)
> > >  		goto err_iounmap;
> > >  
> > >  	i2c_dw_disable_int(dev);
> > > -	r = request_irq(dev->irq, i2c_dw_isr, IRQF_DISABLED, pdev->name, dev);
> > > +
> > > +	irqf = IRQF_DISABLED;
> > > +	if (dev->pdat_flags & I2C_DW_PDAT_FLAGS_SHARE_IRQ)
> > > +		irqf |= IRQF_SHARED;
> > 
> > Please check Wolfram's i2c/next. This is set already there.
> 
> Perfect. My fault for not looking there first. Thanks for highlighting
> this.

No problem :)

And thanks for adding SDA_HOLD support - we also have need for that on
Lynxpoint platform.

[PATCH v2] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

This patch makes the SDA hold time configurable through device tree.
It was tested on a hybrid of i2c/next and arc-3.8-baseline (see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline) since our platform is ARC based and this architecture is not yet
supported by i2c/next.

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   15 +++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |    8 ++++++++
 4 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..fb2eac8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,10 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+   be initialised with its value.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +24,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		sda-hold-time = <0x64>;
+	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..ba40e14 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_TYPE		0xfc
@@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->sda_hold_time)
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
 	u32			master_cfg;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d2a33e9..9cb0100 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -136,6 +137,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	}
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node) {
+		if (of_property_read_u32(pdev->dev.of_node, "sda-hold-time",
+						&dev->sda_hold_time))
+			dev->sda_hold_time = 0;
+	} else
+		dev->sda_hold_time = 0;
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

Re: [PATCH v2] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Mon, Feb 18, 2013 at 11:14:27AM +0100, Christian Ruppert wrote:
> This patch makes the SDA hold time configurable through device tree.
> It was tested on a hybrid of i2c/next and arc-3.8-baseline (see
> https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline) since our platform is ARC based and this architecture is not yet
> supported by i2c/next.
> 
> Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>

Looks good but I have still one minor comment. See below.

> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   15 +++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
>  drivers/i2c/busses/i2c-designware-core.h           |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |    8 ++++++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index e42a2ee..fb2eac8 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -10,6 +10,10 @@ Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
> +Optional properties :
> + - sda-hold-time : If this property is present, the register SDA_HOLD will
> +   be initialised with its value.
> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -20,3 +24,14 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> +
> +	i2c@1120000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "snps,designware-i2c";
> +		reg = <0x1120000 0x1000>;
> +		interrupt-parent = <&ictl>;
> +		interrupts = <12 1>;
> +		clock-frequency = <400000>;
> +		sda-hold-time = <0x64>;
> +	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 94fd818..ba40e14 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -67,6 +67,7 @@
>  #define DW_IC_STATUS		0x70
>  #define DW_IC_TXFLR		0x74
>  #define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
>  #define DW_IC_TX_ABRT_SOURCE	0x80
>  #define DW_IC_COMP_PARAM_1	0xf4
>  #define DW_IC_COMP_TYPE		0xfc
> @@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
> +	/* Configure SDA Hold Time if required */
> +	if (dev->sda_hold_time)
> +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> +
>  	/* Configure Tx/Rx FIFO threshold levels */
>  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
>  	dw_writel(dev, 0, DW_IC_RX_TL);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 9c1840e..33dfec3 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -88,6 +88,7 @@ struct dw_i2c_dev {
>  	u32			master_cfg;
>  	unsigned int		tx_fifo_depth;
>  	unsigned int		rx_fifo_depth;
> +	u32			sda_hold_time;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index d2a33e9..9cb0100 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -34,6 +34,7 @@
>  #include <linux/sched.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -136,6 +137,13 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	}
>  	clk_prepare_enable(dev->clk);
>  
> +	if (pdev->dev.of_node) {
> +		if (of_property_read_u32(pdev->dev.of_node, "sda-hold-time",
> +						&dev->sda_hold_time))
> +			dev->sda_hold_time = 0;
> +	} else
> +		dev->sda_hold_time = 0;

You don't need to explicitly set this to 0 because the dev object is
allocated using kzalloc() which already zeroes out the whole structure.

[PATCH v3] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

This patch makes the SDA hold time configurable through device tree.
It was tested on a hybrid of i2c/next and arc-3.8-baseline (see
https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline) since our platform is ARC based and this architecture is not yet
supported by i2c/next.

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   15 +++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |    5 +++++
 4 files changed, 26 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..fb2eac8 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,10 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - sda-hold-time : If this property is present, the register SDA_HOLD will
+   be initialised with its value.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +24,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		sda-hold-time = <0x64>;
+	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..ba40e14 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_TYPE		0xfc
@@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->sda_hold_time)
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
 	u32			master_cfg;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index d2a33e9..cb44bae 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -136,6 +137,10 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	}
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node)
+		of_property_read_u32(pdev->dev.of_node, "sda-hold-time",
+						&dev->sda_hold_time);
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

Re: [PATCH v3] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Mon, Feb 18, 2013 at 01:51:45PM +0100, Christian Ruppert wrote:
> This patch makes the SDA hold time configurable through device tree.
> It was tested on a hybrid of i2c/next and arc-3.8-baseline (see
> https://github.com/foss-for-synopsys-dwc-arc-processors/linux/commits/arc-3.8-baseline) since our platform is ARC based and this architecture is not yet
> supported by i2c/next.
> 
> Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>

Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>

Thanks!

[PATCH v7] i2c-designware: make SDA hold time configurable

[Christian Ruppert]

Looks like this was eaten by the spam filter last time so i'm resending
it to the lists only:
This patch makes the SDA hold time configurable through device tree.

Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
---
 .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
 drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
 drivers/i2c/busses/i2c-designware-core.h           |    1 +
 drivers/i2c/busses/i2c-designware-platdrv.c        |    9 +++++++++
 4 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
index e42a2ee..21fabe7 100644
--- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
+++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
@@ -10,6 +10,9 @@ Recommended properties :
 
  - clock-frequency : desired I2C bus clock frequency in Hz.
 
+Optional properties :
+ - sda-hold-time : should contain the SDA hold time in nanoseconds.
+
 Example :
 
 	i2c@f0000 {
@@ -20,3 +23,14 @@ Example :
 		interrupts = <11>;
 		clock-frequency = <400000>;
 	};
+
+	i2c@1120000 {
+		#address-cells = <1>;
+		#size-cells = <0>;
+		compatible = "snps,designware-i2c";
+		reg = <0x1120000 0x1000>;
+		interrupt-parent = <&ictl>;
+		interrupts = <12 1>;
+		clock-frequency = <400000>;
+		sda-hold-time = <300>;
+	};
diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
index 94fd818..ba40e14 100644
--- a/drivers/i2c/busses/i2c-designware-core.c
+++ b/drivers/i2c/busses/i2c-designware-core.c
@@ -67,6 +67,7 @@
 #define DW_IC_STATUS		0x70
 #define DW_IC_TXFLR		0x74
 #define DW_IC_RXFLR		0x78
+#define DW_IC_SDA_HOLD		0x7c
 #define DW_IC_TX_ABRT_SOURCE	0x80
 #define DW_IC_COMP_PARAM_1	0xf4
 #define DW_IC_COMP_TYPE		0xfc
@@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
 	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
 	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
 
+	/* Configure SDA Hold Time if required */
+	if (dev->sda_hold_time)
+		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
+
 	/* Configure Tx/Rx FIFO threshold levels */
 	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
 	dw_writel(dev, 0, DW_IC_RX_TL);
diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
index 9c1840e..33dfec3 100644
--- a/drivers/i2c/busses/i2c-designware-core.h
+++ b/drivers/i2c/busses/i2c-designware-core.h
@@ -88,6 +88,7 @@ struct dw_i2c_dev {
 	u32			master_cfg;
 	unsigned int		tx_fifo_depth;
 	unsigned int		rx_fifo_depth;
+	u32			sda_hold_time;
 };
 
 #define ACCESS_SWAP		0x00000001
diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 0ceb6e1..0a6e29b 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -34,6 +34,7 @@
 #include <linux/sched.h>
 #include <linux/err.h>
 #include <linux/interrupt.h>
+#include <linux/of.h>
 #include <linux/of_i2c.h>
 #include <linux/platform_device.h>
 #include <linux/pm.h>
@@ -136,6 +137,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
 	}
 	clk_prepare_enable(dev->clk);
 
+	if (pdev->dev.of_node) {
+		u32 ht;
+		u32 ic_clk = dev->get_clk_rate_khz(dev);
+
+		of_property_read_u32(pdev->dev.of_node, "sda-hold-time", &ht);
+		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
+	}
+
 	dev->functionality =
 		I2C_FUNC_I2C |
 		I2C_FUNC_10BIT_ADDR |
-- 
1.7.1

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Tue, Apr 09, 2013 at 12:59:54PM +0200, Christian Ruppert wrote:
> This patch makes the SDA hold time configurable through device tree.
> 
> Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>

Hi Wolfram,

What happened to this patch? I don't see it merged for 3.10.

The reason I'm asking is that I would like to add ACPI support for the SDA
hold time parameter analogous to the DT version.

Thanks.

> ---
>  .../devicetree/bindings/i2c/i2c-designware.txt     |   14 ++++++++++++++
>  drivers/i2c/busses/i2c-designware-core.c           |    5 +++++
>  drivers/i2c/busses/i2c-designware-core.h           |    1 +
>  drivers/i2c/busses/i2c-designware-platdrv.c        |    9 +++++++++
>  4 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-designware.txt b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> index e42a2ee..21fabe7 100644
> --- a/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> +++ b/Documentation/devicetree/bindings/i2c/i2c-designware.txt
> @@ -10,6 +10,9 @@ Recommended properties :
>  
>   - clock-frequency : desired I2C bus clock frequency in Hz.
>  
> +Optional properties :
> + - sda-hold-time : should contain the SDA hold time in nanoseconds.
> +
>  Example :
>  
>  	i2c@f0000 {
> @@ -20,3 +23,14 @@ Example :
>  		interrupts = <11>;
>  		clock-frequency = <400000>;
>  	};
> +
> +	i2c@1120000 {
> +		#address-cells = <1>;
> +		#size-cells = <0>;
> +		compatible = "snps,designware-i2c";
> +		reg = <0x1120000 0x1000>;
> +		interrupt-parent = <&ictl>;
> +		interrupts = <12 1>;
> +		clock-frequency = <400000>;
> +		sda-hold-time = <300>;
> +	};
> diff --git a/drivers/i2c/busses/i2c-designware-core.c b/drivers/i2c/busses/i2c-designware-core.c
> index 94fd818..ba40e14 100644
> --- a/drivers/i2c/busses/i2c-designware-core.c
> +++ b/drivers/i2c/busses/i2c-designware-core.c
> @@ -67,6 +67,7 @@
>  #define DW_IC_STATUS		0x70
>  #define DW_IC_TXFLR		0x74
>  #define DW_IC_RXFLR		0x78
> +#define DW_IC_SDA_HOLD		0x7c
>  #define DW_IC_TX_ABRT_SOURCE	0x80
>  #define DW_IC_COMP_PARAM_1	0xf4
>  #define DW_IC_COMP_TYPE		0xfc
> @@ -310,6 +311,10 @@ int i2c_dw_init(struct dw_i2c_dev *dev)
>  	dw_writel(dev, lcnt, DW_IC_FS_SCL_LCNT);
>  	dev_dbg(dev->dev, "Fast-mode HCNT:LCNT = %d:%d\n", hcnt, lcnt);
>  
> +	/* Configure SDA Hold Time if required */
> +	if (dev->sda_hold_time)
> +		dw_writel(dev, dev->sda_hold_time, DW_IC_SDA_HOLD);
> +
>  	/* Configure Tx/Rx FIFO threshold levels */
>  	dw_writel(dev, dev->tx_fifo_depth - 1, DW_IC_TX_TL);
>  	dw_writel(dev, 0, DW_IC_RX_TL);
> diff --git a/drivers/i2c/busses/i2c-designware-core.h b/drivers/i2c/busses/i2c-designware-core.h
> index 9c1840e..33dfec3 100644
> --- a/drivers/i2c/busses/i2c-designware-core.h
> +++ b/drivers/i2c/busses/i2c-designware-core.h
> @@ -88,6 +88,7 @@ struct dw_i2c_dev {
>  	u32			master_cfg;
>  	unsigned int		tx_fifo_depth;
>  	unsigned int		rx_fifo_depth;
> +	u32			sda_hold_time;
>  };
>  
>  #define ACCESS_SWAP		0x00000001
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 0ceb6e1..0a6e29b 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -34,6 +34,7 @@
>  #include <linux/sched.h>
>  #include <linux/err.h>
>  #include <linux/interrupt.h>
> +#include <linux/of.h>
>  #include <linux/of_i2c.h>
>  #include <linux/platform_device.h>
>  #include <linux/pm.h>
> @@ -136,6 +137,14 @@ static int dw_i2c_probe(struct platform_device *pdev)
>  	}
>  	clk_prepare_enable(dev->clk);
>  
> +	if (pdev->dev.of_node) {
> +		u32 ht;
> +		u32 ic_clk = dev->get_clk_rate_khz(dev);
> +
> +		of_property_read_u32(pdev->dev.of_node, "sda-hold-time", &ht);
> +		dev->sda_hold_time = ((u64)ic_clk * ht + 500000) / 1000000;
> +	}
> +
>  	dev->functionality =
>  		I2C_FUNC_I2C |
>  		I2C_FUNC_10BIT_ADDR |
> -- 
> 1.7.1
> 

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

On Tue, May 14, 2013 at 02:07:45PM +0300, Mika Westerberg wrote:
> On Tue, Apr 09, 2013 at 12:59:54PM +0200, Christian Ruppert wrote:
> > This patch makes the SDA hold time configurable through device tree.
> > 
> > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> 
> Hi Wolfram,
> 
> What happened to this patch? I don't see it merged for 3.10.
> 
> The reason I'm asking is that I would like to add ACPI support for the SDA
> hold time parameter analogous to the DT version.

New bindings are complicated. I need to check if there is an
existing/similar one, check if it is generic enough for drivers still to
come, etc. I didn't have the time to do this for 3.10, so it is
3.11 material.

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Mika Westerberg]

On Fri, May 17, 2013 at 10:29:28AM +0200, Wolfram Sang wrote:
> On Tue, May 14, 2013 at 02:07:45PM +0300, Mika Westerberg wrote:
> > On Tue, Apr 09, 2013 at 12:59:54PM +0200, Christian Ruppert wrote:
> > > This patch makes the SDA hold time configurable through device tree.
> > > 
> > > Signed-off-by: Christian Ruppert <christian.ruppert@abilis.com>
> > > Signed-off-by: Pierrick Hascoet <pierrick.hascoet@abilis.com>
> > 
> > Hi Wolfram,
> > 
> > What happened to this patch? I don't see it merged for 3.10.
> > 
> > The reason I'm asking is that I would like to add ACPI support for the SDA
> > hold time parameter analogous to the DT version.
> 
> New bindings are complicated. I need to check if there is an
> existing/similar one, check if it is generic enough for drivers still to
> come, etc. I didn't have the time to do this for 3.10, so it is
> 3.11 material.

OK, thanks. I just wanted to be sure this patch wasn't forgotten as it is
useful for Haswell/Lynxpoint as well.

Re: [PATCH v7] i2c-designware: make SDA hold time configurable

[Wolfram Sang]

> > New bindings are complicated. I need to check if there is an
> > existing/similar one, check if it is generic enough for drivers still to
> > come, etc. I didn't have the time to do this for 3.10, so it is
> > 3.11 material.
> 
> OK, thanks. I just wanted to be sure this patch wasn't forgotten as it is
> useful for Haswell/Lynxpoint as well.

i2c list is monitored by patchwork which is rigorously not forgetting ;)