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

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

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-i2c@vger.kernel.org
Cc: devicetree-discuss@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

diff --git a/Documentation/devicetree/bindings/gpio/gpio_i2c.txt b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
new file mode 100644
index 0000000..15f288da
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
@@ -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@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@56 {
+		compatible = "rv3029c2";
+		reg = <0x56>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index a651779..6b5d794 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -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);
 }
 
+static int of_i2c_gpio_probe(struct device_node *np,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+
+	pdata->sda_pin = of_get_gpio(np, 0);
+	pdata->scl_pin = of_get_gpio(np, 1);
+
+	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;
+}
+
 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);
 	if (!pdata)
-		return -ENXIO;
+		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;
+		}
+		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;
 }
 
@@ -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", },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, gpio_i2c_dt_ids);
+#endif
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(gpio_i2c_dt_ids),
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.7

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Mark Brown]

On Sun, Feb 05, 2012 at 11:38:53AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:

> +	- 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.

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:09 Mon 06 Feb     , Mark Brown wrote:
> On Sun, Feb 05, 2012 at 11:38:53AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> > +	- 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.

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Mark Brown]

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

On Tue, Feb 07, 2012 at 03:56:24AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:09 Mon 06 Feb     , Mark Brown wrote:

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

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.

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

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Karol Lewandowski]

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

Hi!

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

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

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

> +	- timeout: timeout to get data
> +
> +
> +Example nodes:
> +
> +i2c-gpio@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@56 {
> +		compatible = "rv3029c2";
> +		reg =<0x56>;
> +	};
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index a651779..6b5d794 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -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);
>   }
>
> +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;

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

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

>   	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.

> +	} 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;
>   }
>
> @@ -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:

>   static struct platform_driver i2c_gpio_driver = {
>   	.driver		= {
>   		.name	= "i2c-gpio",

Regards,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Jean Delvare]

On Mon, 06 Feb 2012 19:38:29 +0100, Karol Lewandowski wrote:
> On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> Hi!
> 
> > +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

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

-- 
Jean Delvare

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Jean-Christophe PLAGNIOL-VILLARD]

On 19:38 Mon 06 Feb     , Karol Lewandowski wrote:
> On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> Hi!
> 
> >+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
> 
> >+	- 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.)
> 
> >+	- 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
> 
>   https://lkml.org/lkml/2011/2/24/220
> 
> >+	- timeout: timeout to get data
> >+
> >+
> >+Example nodes:
> >+
> >+i2c-gpio@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@56 {
> >+		compatible = "rv3029c2";
> >+		reg =<0x56>;
> >+	};
> >+};
> >diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> >index a651779..6b5d794 100644
> >--- a/drivers/i2c/busses/i2c-gpio.c
> >+++ b/drivers/i2c/busses/i2c-gpio.c
> >@@ -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);
> >  }
> >
> >+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
> 
> >+	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;

> 
> >+
> >+	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;
> >+}
> >+
> >  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
> 
> >  	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.
> 
> >+	} 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;
> >  }
> >
> >@@ -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:
> 
> >  static struct platform_driver i2c_gpio_driver = {
> >  	.driver		= {
> >  		.name	= "i2c-gpio",
> 
> Regards,
> -- 
> Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Karol Lewandowski]

On 07.02.2012 04:25, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 19:38 Mon 06 Feb     , Karol Lewandowski wrote:

>>> +	- 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?


>>
>>    https://lkml.org/lkml/2011/2/24/220
>>
>>> +	- timeout: timeout to get data
>>> +
>>> +
>>> +Example nodes:
>>> +
>>> +i2c-gpio@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@56 {
>>> +		compatible = "rv3029c2";
>>> +		reg =<0x56>;
>>> +	};
>>> +};
>>> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
>>> index a651779..6b5d794 100644
>>> --- a/drivers/i2c/busses/i2c-gpio.c
>>> +++ b/drivers/i2c/busses/i2c-gpio.c
>>> @@ -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);
>>>   }
>>>
>>> +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
>>
>>> +	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;
>
>>
>>> +
>>> +	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.

diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 6b5d794..d22c2c7 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -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,
-- 
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform

Re: [PATCH 1/4] i2c/gpio-i2c add: add DT support

[Ben Dooks]

On Mon, Feb 06, 2012 at 07:38:29PM +0100, Karol Lewandowski wrote:
> On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> Hi!
> 
> >+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
> 
> >+	- 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.)
> 
> >+	- 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.