Skip to contentSkip to conversation

i2c/gpio-i2c add: add DT support

PATCH Superseded Jean-Christophe PLAGNIOL-VILLARD 5 participants 12 messages Feb 5, 2012 – Feb 13, 2012

Subsystems (6)
Device tree bindings Arm (32-bit) GPIO I2C subsystem host drivers +2 more subsystems

v1 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Jean-Christophe PLAGNIOL-VILLARD

author
#0 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc linux-i2c at vger.kernel.org
Cc devicetree-discuss at lists.ozlabs.org
---
 .../devicetree/bindings/gpio/gpio_i2c.txt          |   33 ++++++++++++
 drivers/i2c/busses/i2c-gpio.c                      |   55 +++++++++++++++++++-
 2 files changed, 86 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/gpio_i2c.txt
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +33 -0
new file mode 100644
index 0000000..15f288da
@@ -0,0 +1,33 @@
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+	- compatible = "gpio-i2c";
+	- gpios: sda and scl gpio
+
+
+Optional properties:
+	- gpio-i2c,sda_is_open_drain: sda as open drain
+	- gpio-i2c,scl_is_open_drain: scl as open drain
+	- gpio-i2c,scl_is_output_only: scl as output only
+	- udelay: half clock cycle time in us (may depend on each platform)
+	- timeout: timeout to get data
+
+
+Example nodes:
+
+i2c-gpio at 0 {
+	compatible = "gpio-i2c";
+	gpios = <&pioA 23 0 /* sda */
+		 &pioA 24 0 /* scl */
+		>;
+	gpio-i2c,sda_is_open_drain;
+	gpio-i2c,scl_is_open_drain;
+	udelay = <2>;		/* ~100 kHz */
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	rv3029c2 at 56 {
+		compatible = "rv3029c2";
+		reg = <0x56>;
+	};
+};
drivers/i2c/busses/i2c-gpio.c +53 -2

103 lines not on this page · raw message

Signature · 1 line
1.7.7
author
#1 lore raw

PATCH 2/4 ARM: at91s: sam9g20 add i2c DT support

for now on use gpio-i2c driver on the same pin as the hardware IP

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc devicetree-discuss at lists.ozlabs.org
---
 arch/arm/boot/dts/at91sam9g20.dtsi |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
arch/arm/boot/dts/at91sam9g20.dtsi +13 -0
index f8f4627..d52e827 100644
@@ -220,6 +220,19 @@
 				status = "disabled";
 			};
 
+			i2c-gpio at 0 {
+				compatible = "gpio-i2c";
+				gpios = <&pioA 23 0 /* sda */
+					 &pioA 24 0 /* scl */
+					>;
+				gpio-i2c,sda_is_open_drain;
+				gpio-i2c,scl_is_open_drain;
+				udelay = <2>;		/* ~100 kHz */
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+
 		};
 	};
 };
Signature · 1 line
1.7.7
author
#2 lore raw

PATCH 3/4 ARM: at91: usb_a9g20 add DT i2c support

In reply to Jean-Christophe PLAGNIOL-VILLARD

use gpio-i2c and enable rv3029 RTC

enable the in the sam9g20 defconfig

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc devicetree-discuss at lists.ozlabs.org
---
 arch/arm/boot/dts/usb_a9g20.dts        |    9 +++++++++
 arch/arm/configs/at91sam9g20_defconfig |    3 +++
 2 files changed, 12 insertions(+), 0 deletions(-)
arch/arm/boot/dts/usb_a9g20.dts +9 -0
index 11be3e9..9d84374 100644
@@ -121,6 +121,15 @@
 
 			};
 
+			i2c-gpio at 0 {
+				status = "okay";
+
+				rv3029c2 at 56 {
+					compatible = "rv3029c2";
+					reg = <0x56>;
+				};
+			};
+
 		};
 	};
 };
arch/arm/configs/at91sam9g20_defconfig +3 -0
index 9123568..994d331 100644
@@ -74,6 +74,8 @@ CONFIG_LEGACY_PTY_COUNT=16
 CONFIG_SERIAL_ATMEL=y
 CONFIG_SERIAL_ATMEL_CONSOLE=y
 CONFIG_HW_RANDOM=y
+CONFIG_I2C=y
+CONFIG_I2C_GPIO=y
 CONFIG_SPI=y
 CONFIG_SPI_ATMEL=y
 CONFIG_SPI_SPIDEV=y

@@ -105,6 +107,7 @@ CONFIG_LEDS_TRIGGERS=y
 CONFIG_LEDS_TRIGGER_TIMER=y
 CONFIG_LEDS_TRIGGER_HEARTBEAT=y
 CONFIG_RTC_CLASS=y
+CONFIG_RTC_DRV_RV3029C2=y
 CONFIG_RTC_DRV_AT91SAM9=y
 CONFIG_EXT2_FS=y
 CONFIG_MSDOS_FS=y
Signature · 1 line
1.7.7
author
#3 lore raw

PATCH 4/4 ARM: at91: sam9g45 add i2c DT support

In reply to Jean-Christophe PLAGNIOL-VILLARD

for now on use gpio-i2c driver on the same pin as the hardware IP

Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>
Cc devicetree-discuss at lists.ozlabs.org
---
 arch/arm/boot/dts/at91sam9g45.dtsi |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)
arch/arm/boot/dts/at91sam9g45.dtsi +13 -0
index a12ebc4..951b2c8 100644
@@ -211,6 +211,19 @@
 				status = "disabled";
 			};
 
+			i2c-gpio at 0 {
+				compatible = "gpio-i2c";
+				gpios = <&pioA 20 0 /* sda */
+					 &pioA 21 0 /* scl */
+					>;
+				gpio-i2c,sda_is_open_drain;
+				gpio-i2c,scl_is_open_drain;
+				udelay = <5>;		/* ~100 kHz */
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+
 		};
 	};
 };
Signature · 1 line
1.7.7
#4 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

In reply to Jean-Christophe PLAGNIOL-VILLARD

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 25

Quoted 3 lines · Jean-Christophe PLAGNIOL-VILLARD
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2 -0
+	- udelay: half clock cycle time in us (may depend on each platform)
 
+	udelay = <2>;		/* ~100 kHz */

Why not specify this in kHz and do the conversion in the driver? It seems a more intuitive thing to be specifying. I appreciate that the platform data used udelay but it seems an entirely unintuitive thing from a user point of view even if it's what the implementation wants.

author
#5 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

Quoted 10 lines · Mark Brown

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 25

Quoted 3 lines · Jean-Christophe PLAGNIOL-VILLARD
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2 -0
+	- udelay: half clock cycle time in us (may depend on each platform)
 
+	udelay = <2>;		/* ~100 kHz */

Why not specify this in kHz and do the conversion in the driver? It seems a more intuitive thing to be specifying. I appreciate that the platform data used udelay but it seems an entirely unintuitive thing from a user point of view even if it's what the implementation wants.

because it's not accurate and on some platform you need to adapt it so we keep the udelay

as example due to latency when changing state of the gpio as we may access it via i2c or internal bus latency

Best Regards,
J.

#6 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

Quoted 11 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 8 lines · Mark Brown

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 25

Quoted 3 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2 -0
+	- udelay: half clock cycle time in us (may depend on each platform)
 
+	udelay = <2>;		/* ~100 kHz */

Why not specify this in kHz and do the conversion in the driver? It seems a more intuitive thing to be specifying. I appreciate that the platform data used udelay but it seems an entirely unintuitive thing from a user point of view even if it's what the implementation wants.

Quoted 2 lines

because it's not accurate and on some platform you need to adapt it so we keep the udelay

Then you should clarify that in the documentation, it's not the cycle
time but the delay between GPIO operations which isn't quite the same
thing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120207/7c9bba3f/attachment.sig>

#7 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

In reply to Jean-Christophe PLAGNIOL-VILLARD

On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:

Hi!

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +4 -0
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+	- compatible = "gpio-i2c";

Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of patch adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio - could we stick to that name?

[1] https://lkml.org/lkml/2011/2/23/584

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 11

Quoted 7 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +7 -0
+	- gpios: sda and scl gpio
+
+
+Optional properties:
+	- gpio-i2c,sda_is_open_drain: sda as open drain
+	- gpio-i2c,scl_is_open_drain: scl as open drain
+	- gpio-i2c,scl_is_output_only: scl as output only

Most of DT-properties I've seen used hyphen, not underscore. Could we stick to that convention?

(Nitpick: I think that "is" in property names is redundant too.)

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 12

Quoted 1 line
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +1 -0
+	- udelay: half clock cycle time in us (may depend on each platform)

Could we use "clock-frequency" as Grant have suggested during review of previous patch to i2c-gpio?

https://lkml.org/lkml/2011/2/24/220

Quoted 35 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +21 -0
+	- timeout: timeout to get data
+
+
+Example nodes:
+
+i2c-gpio at 0 {
+	compatible = "gpio-i2c";
+	gpios =<&pioA 23 0 /* sda */
+		&pioA 24 0 /* scl */
+		>;
+	gpio-i2c,sda_is_open_drain;
+	gpio-i2c,scl_is_open_drain;
+	udelay =<2>;		/* ~100 kHz */
+	#address-cells =<1>;
+	#size-cells =<0>;
+
+	rv3029c2 at 56 {
+		compatible = "rv3029c2";
+		reg =<0x56>;
+	};
+};
drivers/i2c/busses/i2c-gpio.c +2 -0
index a651779..6b5d794 100644
@@ -14,6 +14,8 @@
  #include<linux/module.h>
  #include<linux/slab.h>
  #include<linux/platform_device.h>
+#include<linux/of_gpio.h>
+#include<linux/of_i2c.h>
 
  #include<asm/gpio.h>

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 87

Quoted 8 lines
drivers/i2c/busses/i2c-gpio.c +5 -0
@@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
  	return gpio_get_value(pdata->scl_pin);
  }
 
+static int of_i2c_gpio_probe(struct device_node *np,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+

if (of_gpio_count(np) < 2)

	return -EINVAL;

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 89

Quoted 2 lines
drivers/i2c/busses/i2c-gpio.c +2 -0
+	pdata->sda_pin = of_get_gpio(np, 0);
+	pdata->scl_pin = of_get_gpio(np, 1);

if (pdata->sda_pin < 0 || pdata->scl_pin < 0)

	return -EINVAL;
Quoted 17 lines
drivers/i2c/busses/i2c-gpio.c +17 -0
+
+	if (of_property_read_u32(np, "udelay",&reg))
+		pdata->udelay = reg;
+
+	if (of_property_read_u32(np, "timeout",&reg))
+		pdata->timeout = reg;
+
+	pdata->sda_is_open_drain =
+		!!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
+	pdata->scl_is_open_drain =
+		!!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
+	pdata->scl_is_output_only =
+		!!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
+
+	return 0;
+}
+

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 115

Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +2 -1
  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
  {
  	struct i2c_gpio_platform_data *pdata;
  	struct i2c_algo_bit_data *bit_data;
  	struct i2c_adapter *adap;
  	int ret;
+	int len = sizeof(struct i2c_gpio_platform_data);
 
-	pdata = pdev->dev.platform_data;
+	pdata = kzalloc(len, GFP_KERNEL);

Could you also take into account Grant's comment about private/platform data?

https://lkml.org/lkml/2011/2/3/221

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 120

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c +4 -1
  	if (!pdata)
-		return -ENXIO;
+		return -ENOMEM;
+
+	if (pdev->dev.of_node) {
+		of_i2c_gpio_probe(pdev->dev.of_node, pdata);

Above might fail if configuration is corrupted.

Quoted 36 lines
drivers/i2c/busses/i2c-gpio.c +11 -0
+	} else {
+		if (!pdev->dev.platform_data) {
+			ret = -ENXIO;
+			goto err_alloc_adap;
+		}
+		memcpy(pdata, pdev->dev.platform_data, len);
+	}
 
  	ret = -ENOMEM;
  	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);

@@ -143,6 +180,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
  	adap->algo_data = bit_data;
  	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
  	adap->dev.parent =&pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
 
  	/*
  	 * If "dev->id" is negative we consider it as zero.

@@ -154,6 +192,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
  	if (ret)
  		goto err_add_bus;
 
+	of_i2c_register_devices(adap);
+
  	platform_set_drvdata(pdev, adap);
 
  	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",

@@ -172,6 +212,7 @@ err_request_sda:
  err_alloc_bit_data:
  	kfree(adap);
  err_alloc_adap:
+	kfree(pdata);
  	return ret;
  }

drivers/i2c/busses/i2c-gpio.c static int __devexit i2c_gpio_remove(struct platform_device *pdev) · line 238

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c +3 -0
@@ -192,10 +233,20 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
  	return 0;
  }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id gpio_i2c_dt_ids[] = {
+	{ .compatible = "gpio-i2c", },

There seem to be no good reason to make DT-compatible string different from driver's name that's already in use:

drivers/i2c/busses/i2c-gpio.c static int __devexit i2c_gpio_remove(struct platform_device *pdev) · line 247

Quoted 3 lines
drivers/i2c/busses/i2c-gpio.c +0 -0
  static struct platform_driver i2c_gpio_driver = {
  	.driver		= {
  		.name	= "i2c-gpio",

Regards,

Signature · 1 line
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
#8 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

Quoted 13 lines · Karol Lewandowski

On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:

Hi!

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +4 -0
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+	- compatible = "gpio-i2c";

Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of patch adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio - could we stick to that name?

Quoted 1 line

[1] https://lkml.org/lkml/2011/2/23/584

Yes, please. The gpio subsystem normalized its driver names to gpio-*, so we shouldn't use that anywhere else to avoid confusion.

Signature · 1 line
Jean Delvare
author
#9 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

In reply to Karol Lewandowski
Quoted 26 lines · Karol Lewandowski

On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:

Hi!

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +4 -0
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+	- compatible = "gpio-i2c";

Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of patch adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio - could we stick to that name?

[1] https://lkml.org/lkml/2011/2/23/584

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 11

Quoted 7 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +7 -0
+	- gpios: sda and scl gpio
+
+
+Optional properties:
+	- gpio-i2c,sda_is_open_drain: sda as open drain
+	- gpio-i2c,scl_is_open_drain: scl as open drain
+	- gpio-i2c,scl_is_output_only: scl as output only

Most of DT-properties I've seen used hyphen, not underscore. Could we stick to that convention?

Quoted 6 lines

(Nitpick: I think that "is" in property names is redundant too.)

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 12

Quoted 1 line
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +1 -0
+	- udelay: half clock cycle time in us (may depend on each platform)

Could we use "clock-frequency" as Grant have suggested during review of previous patch to i2c-gpio?

as exaplained no as for gpio the delay is platform dependent

Quoted 41 lines

https://lkml.org/lkml/2011/2/24/220

Quoted 38 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +21 -0
+	- timeout: timeout to get data
+
+
+Example nodes:
+
+i2c-gpio at 0 {
+	compatible = "gpio-i2c";
+	gpios =<&pioA 23 0 /* sda */
+		&pioA 24 0 /* scl */
+		>;
+	gpio-i2c,sda_is_open_drain;
+	gpio-i2c,scl_is_open_drain;
+	udelay =<2>;		/* ~100 kHz */
+	#address-cells =<1>;
+	#size-cells =<0>;
+
+	rv3029c2 at 56 {
+		compatible = "rv3029c2";
+		reg =<0x56>;
+	};
+};
drivers/i2c/busses/i2c-gpio.c +2 -0
index a651779..6b5d794 100644
@@ -14,6 +14,8 @@
 #include<linux/module.h>
 #include<linux/slab.h>
 #include<linux/platform_device.h>
+#include<linux/of_gpio.h>
+#include<linux/of_i2c.h>
 
 #include<asm/gpio.h>
 

@@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
Quoted 8 lines

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 87

Quoted 5 lines
drivers/i2c/busses/i2c-gpio.c +5 -0
+static int of_i2c_gpio_probe(struct device_node *np,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+

if (of_gpio_count(np) < 2)

	return -EINVAL;

ok

Quoted 103 lines

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 89

Quoted 3 lines
drivers/i2c/busses/i2c-gpio.c +2 -0
+	pdata->sda_pin = of_get_gpio(np, 0);
+	pdata->scl_pin = of_get_gpio(np, 1);

if (pdata->sda_pin < 0 || pdata->scl_pin < 0)

	return -EINVAL;
Quoted 17 lines
drivers/i2c/busses/i2c-gpio.c +17 -0
+
+	if (of_property_read_u32(np, "udelay",&reg))
+		pdata->udelay = reg;
+
+	if (of_property_read_u32(np, "timeout",&reg))
+		pdata->timeout = reg;
+
+	pdata->sda_is_open_drain =
+		!!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
+	pdata->scl_is_open_drain =
+		!!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
+	pdata->scl_is_output_only =
+		!!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
+
+	return 0;
+}
+

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 115

Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +2 -1
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 {
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
 	int ret;
+	int len = sizeof(struct i2c_gpio_platform_data);
 
-	pdata = pdev->dev.platform_data;
+	pdata = kzalloc(len, GFP_KERNEL);

Could you also take into account Grant's comment about private/platform data?

https://lkml.org/lkml/2011/2/3/221

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 120

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c +4 -1
 	if (!pdata)
-		return -ENXIO;
+		return -ENOMEM;
+
+	if (pdev->dev.of_node) {
+		of_i2c_gpio_probe(pdev->dev.of_node, pdata);

Above might fail if configuration is corrupted.

Quoted 36 lines
drivers/i2c/busses/i2c-gpio.c +11 -0
+	} else {
+		if (!pdev->dev.platform_data) {
+			ret = -ENXIO;
+			goto err_alloc_adap;
+		}
+		memcpy(pdata, pdev->dev.platform_data, len);
+	}
 
 	ret = -ENOMEM;
 	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);

@@ -143,6 +180,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 	adap->algo_data = bit_data;
 	adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
 	adap->dev.parent =&pdev->dev;
+	adap->dev.of_node = pdev->dev.of_node;
 
 	/*
 	 * If "dev->id" is negative we consider it as zero.

@@ -154,6 +192,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_add_bus;
 
+	of_i2c_register_devices(adap);
+
 	platform_set_drvdata(pdev, adap);
 
 	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",

@@ -172,6 +212,7 @@ err_request_sda:
 err_alloc_bit_data:
 	kfree(adap);
 err_alloc_adap:
+	kfree(pdata);
 	return ret;
 }

drivers/i2c/busses/i2c-gpio.c static int __devexit i2c_gpio_remove(struct platform_device *pdev) · line 238

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c +3 -0
@@ -192,10 +233,20 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 	return 0;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id gpio_i2c_dt_ids[] = {
+	{ .compatible = "gpio-i2c", },

There seem to be no good reason to make DT-compatible string different from driver's name that's already in use:

drivers/i2c/busses/i2c-gpio.c static int __devexit i2c_gpio_remove(struct platform_device *pdev) · line 247

Quoted 3 lines
drivers/i2c/busses/i2c-gpio.c +0 -0
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",

Regards,

Signature · 1 line
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
#10 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 4 lines · Karol Lewandowski

Review

Quoted 1 line
(quoted diff) +1 -0
+	- udelay: half clock cycle time in us (may depend on each platform)

Could we use "clock-frequency" as Grant have suggested during review of previous patch to i2c-gpio?

as exaplained no as for gpio the delay is platform dependent

Following sniplet from i2c-gpio suggests that in some cases both udelay and timeout can be calculated given presence of other parameters:

static int __devinit i2c_gpio_probe(struct platform_device *pdev)
...

	if (pdata->udelay)
		bit_data->udelay = pdata->udelay;
	else if (pdata->scl_is_output_only)
		bit_data->udelay = 50;			/* 10 kHz */
	else
		bit_data->udelay = 5;			/* 100 kHz */

	if (pdata->timeout)
		bit_data->timeout = pdata->timeout;
	else
		bit_data->timeout = HZ / 10;		/* 100 ms */

However, I find it more troubling that driver manually grabs parameters that are specific to i2c-algo-bit (timeout, udelay).

Thus, I have feeling that it should be generically addressed there. What do you think?

Quoted 58 lines
Quoted 41 lines
   https://lkml.org/lkml/2011/2/24/220
Quoted 38 lines
(quoted diff) +21 -0
+	- timeout: timeout to get data
+
+
+Example nodes:
+
+i2c-gpio at 0 {
+	compatible = "gpio-i2c";
+	gpios =<&pioA 23 0 /* sda */
+		&pioA 24 0 /* scl */
+		>;
+	gpio-i2c,sda_is_open_drain;
+	gpio-i2c,scl_is_open_drain;
+	udelay =<2>;		/* ~100 kHz */
+	#address-cells =<1>;
+	#size-cells =<0>;
+
+	rv3029c2 at 56 {
+		compatible = "rv3029c2";
+		reg =<0x56>;
+	};
+};
drivers/i2c/busses/i2c-gpio.c +2 -0
index a651779..6b5d794 100644
@@ -14,6 +14,8 @@
  #include<linux/module.h>
  #include<linux/slab.h>
  #include<linux/platform_device.h>
+#include<linux/of_gpio.h>
+#include<linux/of_i2c.h>
 
  #include<asm/gpio.h>
 

@@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
  	return gpio_get_value(pdata->scl_pin);
  }
Quoted 8 lines

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 87

Quoted 5 lines
drivers/i2c/busses/i2c-gpio.c +5 -0
+static int of_i2c_gpio_probe(struct device_node *np,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+

if (of_gpio_count(np)< 2)

	return -EINVAL;

ok

Quoted 8 lines

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 89

Quoted 3 lines
drivers/i2c/busses/i2c-gpio.c +2 -0
+	pdata->sda_pin = of_get_gpio(np, 0);
+	pdata->scl_pin = of_get_gpio(np, 1);

if (pdata->sda_pin< 0 || pdata->scl_pin< 0)

	return -EINVAL;
Quoted 6 lines
Quoted 6 lines

drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 95

Quoted 6 lines
drivers/i2c/busses/i2c-gpio.c +6 -0
+
+	if (of_property_read_u32(np, "udelay",&reg))
+		pdata->udelay = reg;
+
+	if (of_property_read_u32(np, "timeout",&reg))
+		pdata->timeout = reg;

One more thing missed in original review - of_property_read* rnegative value on error, so logic has to reversed here:

if (of_property_read_u32(np, "timeout",&reg) == 0)

or

if (!of_property_read_u32(np, "timeout",&reg))

As I have been testing your driver on real hardware I've also made changes (see below) which I have previously written about. If you would like to see these in git-am-able format please drop me a note.

drivers/i2c/busses/i2c-gpio.c +21 -5
index 6b5d794..d22c2c7 100644
@@ -80,29 +80,45 @@ static int i2c_gpio_getscl(void *data)
  	return gpio_get_value(pdata->scl_pin);
  }
 
+#ifdef CONFIG_OF
  static int of_i2c_gpio_probe(struct device_node *np,
  			     struct i2c_gpio_platform_data *pdata)
  {
  	u32 reg;
 
+	if (!np)
+		return -EINVAL;
+
+	if (of_gpio_count(np) < 2)
+		return -EINVAL;
+
  	pdata->sda_pin = of_get_gpio(np, 0);
  	pdata->scl_pin = of_get_gpio(np, 1);
 
-	if (of_property_read_u32(np, "udelay", &reg))
+	if (pdata->sda_pin < 0 || pdata->scl_pin < 0)
+		return -EINVAL;
+
+	if (!of_property_read_u32(np, "udelay", &reg))
  		pdata->udelay = reg;
 
-	if (of_property_read_u32(np, "timeout", &reg))
+	if (!of_property_read_u32(np, "timeout", &reg))
  		pdata->timeout = reg;
 
  	pdata->sda_is_open_drain =
-		!!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
+		!!of_get_property(np, "i2c-gpio,sda-open-drain", NULL);
  	pdata->scl_is_open_drain =
-		!!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
+		!!of_get_property(np, "i2c-gpio,scl-open-drain", NULL);
  	pdata->scl_is_output_only =
-		!!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
+		!!of_get_property(np, "i2c-gpio,scl-output-only", NULL);
 
  	return 0;
  }
+#else
+static int of_i2c_gpio_probe(struct device_node *np,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	return -EINVAL
+#endif
 
  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
  {

@@ -116,15 +132,11 @@ static int __devinit i2c_gpio_probe(struct

platform_device *pdev)
if (!pdata)
return -ENOMEM;

- if (pdev->dev.of_node) {
- of_i2c_gpio_probe(pdev->dev.of_node, pdata);
- } else {
- if (!pdev->dev.platform_data) {
- ret = -ENXIO;
- goto err_alloc_adap;
- }
+ ret = -ENXIO;
+ if ((ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata)) < 0 &&
!pdev->dev.platform_data)
+ goto err_alloc_adap;
+ else if (pdev->dev.platform_data)
memcpy(pdata, pdev->dev.platform_data, len);
- }

ret = -ENOMEM;
adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
@@ -235,7 +247,7 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)

#if defined(CONFIG_OF)
static const struct of_device_id gpio_i2c_dt_ids[] = {
- { .compatible = "gpio-i2c", },
+ { .compatible = "i2c-gpio", },
{ /* sentinel */ }
};

Regards,

Signature · 1 line
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
#11 lore raw

PATCH 1/4 i2c/gpio-i2c add: add DT support

In reply to Karol Lewandowski
Quoted 26 lines · Karol Lewandowski

On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:

Hi!

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 4

Quoted 4 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +4 -0
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+	- compatible = "gpio-i2c";

Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of patch adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio - could we stick to that name?

[1] https://lkml.org/lkml/2011/2/23/584

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 11

Quoted 7 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +7 -0
+	- gpios: sda and scl gpio
+
+
+Optional properties:
+	- gpio-i2c,sda_is_open_drain: sda as open drain
+	- gpio-i2c,scl_is_open_drain: scl as open drain
+	- gpio-i2c,scl_is_output_only: scl as output only

Most of DT-properties I've seen used hyphen, not underscore. Could we stick to that convention?

Quoted 6 lines

(Nitpick: I think that "is" in property names is redundant too.)

Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 12

Quoted 1 line
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +1 -0
+	- udelay: half clock cycle time in us (may depend on each platform)

Could we use "clock-frequency" as Grant have suggested during review of previous patch to i2c-gpio?

I'm with Grant on that, it would be nice to have a reasonably sane set of default i2c dt bindings that everyone uses.