[PATCH] i2c-gpio: add devicetree support

[PATCH] i2c-gpio: add devicetree support

[Thomas Chou]

From: Albert Herranz <albert_herranz@yahoo.es>

This patch is based on an earlier patch from Albert Herranz,
http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
9854eb78607c641ab5ae85bcbe3c9d14ac113733

The dts binding is modified as Grant suggested. The of probing
is merged inline instead of a separate file. It uses the newer
of gpio probe.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 Documentation/devicetree/bindings/gpio/i2c.txt |   39 ++++++++++++++
 drivers/i2c/busses/i2c-gpio.c                  |   67 ++++++++++++++++++++++-
 2 files changed, 103 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/gpio/i2c.txt

diff --git a/Documentation/devicetree/bindings/gpio/i2c.txt b/Documentation/devicetree/bindings/gpio/i2c.txt
new file mode 100644
index 0000000..402569e
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/i2c.txt
@@ -0,0 +1,39 @@
+GPIO-based I2C
+
+Required properties:
+- compatible : should be "i2c-gpio".
+- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
+Optional properties:
+- sda-is-open-drain : present if SDA gpio is open-drain.
+- scl-is-open-drain : present if SCL gpio is open-drain.
+- scl-is-output-only : present if SCL is an output gpio only.
+- udelay : signal toggle delay. SCL frequency is (500 / udelay) kHz
+- timeout : clock stretching timeout in milliseconds.
+
+Example:
+
+gpio0: starlet-gpio@0d8000c0 {
+	compatible = "nintendo,starlet-gpio";
+	reg = <0d8000c0 4>;
+	gpio-controller;
+	#gpio-cells = <2>;
+};
+
+i2c-video {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "i2c-gpio";
+
+	gpios = <&gpio0 10 0	/* SDA line */
+		 &gpio0 11 0	/* SCL line */
+		>;
+	sda-is-open-drain;
+	scl-is-open-drain;
+	scl-is-output-only;
+	udelay = <2>;
+
+	audio-video-encoder {
+		compatible = "nintendo,wii-ave-rvl";
+		reg = <70>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index d9aa9a6..9648201 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,6 +14,9 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/of_i2c.h>
 
 #include <asm/gpio.h>
 
@@ -83,11 +86,52 @@ 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;
+	struct device_node *np = pdev->dev.of_node;
 	int ret;
 
 	pdata = pdev->dev.platform_data;
-	if (!pdata)
-		return -ENXIO;
+	if (!pdata) {
+		if (np && of_gpio_count(np) >= 2) {
+			const __be32 *prop;
+			int sda_pin, scl_pin;
+
+			sda_pin = of_get_gpio_flags(np, 0, NULL);
+			scl_pin = of_get_gpio_flags(np, 1, NULL);
+			if (sda_pin < 0 || scl_pin < 0) {
+				pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+				       np->full_name, sda_pin, scl_pin);
+				ret = -EINVAL;
+				goto err_gpio_pin;
+			}
+			pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+			if (!pdata) {
+				ret = -ENOMEM;
+				goto err_alloc_pdata;
+			}
+			pdata->sda_pin = sda_pin;
+			pdata->scl_pin = scl_pin;
+			prop = of_get_property(np, "sda-is-open-drain", NULL);
+			if (prop)
+				pdata->sda_is_open_drain = 1;
+			prop = of_get_property(np, "scl-is-open-drain", NULL);
+			if (prop)
+				pdata->scl_is_open_drain = 1;
+			prop = of_get_property(np, "scl-is-output-only", NULL);
+			if (prop)
+				pdata->scl_is_output_only = 1;
+			prop = of_get_property(np, "udelay", NULL);
+			if (prop)
+				pdata->udelay = be32_to_cpup(prop);
+			prop = of_get_property(np, "timeout", NULL);
+			if (prop) {
+				pdata->timeout =
+					msecs_to_jiffies(be32_to_cpup(prop));
+			}
+		} else {
+			ret = -ENXIO;
+			goto err_no_pdata;
+		}
+	}
 
 	ret = -ENOMEM;
 	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
@@ -143,6 +187,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 = np;
 
 	/*
 	 * If "dev->id" is negative we consider it as zero.
@@ -161,6 +206,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 		 pdata->scl_is_output_only
 		 ? ", no clock stretching" : "");
 
+	/* Now register all the child nodes */
+	of_i2c_register_devices(adap);
+
 	return 0;
 
 err_add_bus:
@@ -172,6 +220,9 @@ err_request_sda:
 err_alloc_bit_data:
 	kfree(adap);
 err_alloc_adap:
+err_no_pdata:
+err_alloc_pdata:
+err_gpio_pin:
 	return ret;
 }
 
@@ -179,23 +230,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 {
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_adapter *adap;
+	struct i2c_algo_bit_data *bit_data;
 
 	adap = platform_get_drvdata(pdev);
-	pdata = pdev->dev.platform_data;
+	bit_data = adap->algo_data;
+	pdata = bit_data->data;
 
 	i2c_del_adapter(adap);
 	gpio_free(pdata->scl_pin);
 	gpio_free(pdata->sda_pin);
 	kfree(adap->algo_data);
 	kfree(adap);
+	if (!pdev->dev.platform_data)
+		kfree(pdata);
 
 	return 0;
 }
 
+static const struct of_device_id i2c_gpio_match[] = {
+	{ .compatible = "i2c-gpio", },
+	{},
+};
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = i2c_gpio_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.3.5

Re: [PATCH] i2c-gpio: add devicetree support

[Håvard Skinnemoen]

Hi,

On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
> From: Albert Herranz <albert_herranz@yahoo.es>
>
> This patch is based on an earlier patch from Albert Herranz,
> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
> 9854eb78607c641ab5ae85bcbe3c9d14ac113733

That commit has a single-line description of which I don't understand
a single word (unless "wii" is what I think it is, which seems
likely). Could you please explain how that commit relates to this
patch?

> The dts binding is modified as Grant suggested. The of probing
> is merged inline instead of a separate file. It uses the newer
> of gpio probe.

It seems like a terrible idea to merge firmware-specific code into the
driver. Is there are reason why of-based platforms can't just pass the
data they need in pdata like everyone else?

Not saying that it necessarily _is_ a terrible idea, but I think the
reasoning behind it needs to be included in the patch description.

> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
>  Documentation/devicetree/bindings/gpio/i2c.txt |   39 ++++++++++++++
>  drivers/i2c/busses/i2c-gpio.c                  |   67 ++++++++++++++++++++++-
>  2 files changed, 103 insertions(+), 3 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/gpio/i2c.txt
>
> diff --git a/Documentation/devicetree/bindings/gpio/i2c.txt b/Documentation/devicetree/bindings/gpio/i2c.txt
> new file mode 100644
> index 0000000..402569e
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/i2c.txt

This looks a bit backwards. i2c-gpio is a i2c driver which happens to
utilize the gpio framework, not the other way around.

> @@ -0,0 +1,39 @@
> +GPIO-based I2C
> +
> +Required properties:
> +- compatible : should be "i2c-gpio".
> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> +Optional properties:
> +- sda-is-open-drain : present if SDA gpio is open-drain.
> +- scl-is-open-drain : present if SCL gpio is open-drain.
> +- scl-is-output-only : present if SCL is an output gpio only.

I think "present if the output driver for SCL cannot be turned off" is
more accurate. Might also be worth mentioning that this will prevent
clock stretching from working.

> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,6 +14,9 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/of_platform.h>
> +#include <linux/of_gpio.h>
> +#include <linux/of_i2c.h>

Do these headers provide stubs so non-of platforms won't break?

> @@ -83,11 +86,52 @@ 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;
> +       struct device_node *np = pdev->dev.of_node;

Would be nice if this could be eliminated on non-of platforms.

>        int ret;
>
>        pdata = pdev->dev.platform_data;
> -       if (!pdata)
> -               return -ENXIO;
> +       if (!pdata) {
> +               if (np && of_gpio_count(np) >= 2) {

If that expression somehow always evaluates to false on non-of
platforms, this might be ok. But please confirm if this is the case;
otherwise, it looks like a pretty large addition to an otherwise very
small driver.

How about a tiny bit of restructuring: Move the block below into a
separate function, which is only called if some constant expression
says that of is enabled. Then you can move the declaration above
either into the if block or into the function, depending on where you
want to do the conditional above.

>  static struct platform_driver i2c_gpio_driver = {
>        .driver         = {
>                .name   = "i2c-gpio",
>                .owner  = THIS_MODULE,
> +               .of_match_table = i2c_gpio_match,

Is this field always present even when of is disabled?

Havard

Re: [PATCH] i2c-gpio: add devicetree support

[Grant Likely]

On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen
<hskinnemoen@gmail.com> wrote:
> Hi,
>
> On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
>> From: Albert Herranz <albert_herranz@yahoo.es>
>>
>> This patch is based on an earlier patch from Albert Herranz,
>> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
>> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
>
> That commit has a single-line description of which I don't understand
> a single word (unless "wii" is what I think it is, which seems
> likely). Could you please explain how that commit relates to this
> patch?

The URL got wrapped.  Try this one (assuming my mailer doesn't wrap it):

http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/9854eb78607c641ab5ae85bcbe3c9d14ac113733

>
>> The dts binding is modified as Grant suggested. The of probing
>> is merged inline instead of a separate file. It uses the newer
>> of gpio probe.
>
> It seems like a terrible idea to merge firmware-specific code into the
> driver. Is there are reason why of-based platforms can't just pass the
> data they need in pdata like everyone else?

Overall Thomas is doing the right thing here.  The driver data has to
be decoded *somewhere*, but since that code is definitely
driver-specific (as opposed to platform, subsystem, or arch specific)
putting it into the driver is absolutely the right thing to do.  Quite
a few drivers now do exactly this.

It is however generally a wise practice to limit the of-support code
to a hook in the drivers probe hook, and as you point out, care must
be taken to make sure both CONFIG_OF and !CONFIG_OF kernel builds
work.

>
> Not saying that it necessarily _is_ a terrible idea, but I think the
> reasoning behind it needs to be included in the patch description.

Nah, he doesn't really need to defend this since it is a well
established pattern.  device tree support is in core code now (see
of_node an of_match_table in include/linux/device.h), and other
drivers do exactly this.

>> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
>> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
>> ---
>>  Documentation/devicetree/bindings/gpio/i2c.txt |   39 ++++++++++++++
>>  drivers/i2c/busses/i2c-gpio.c                  |   67 ++++++++++++++++++++++-
>>  2 files changed, 103 insertions(+), 3 deletions(-)
>>  create mode 100644 Documentation/devicetree/bindings/gpio/i2c.txt
>>
>> diff --git a/Documentation/devicetree/bindings/gpio/i2c.txt b/Documentation/devicetree/bindings/gpio/i2c.txt
>> new file mode 100644
>> index 0000000..402569e
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/gpio/i2c.txt
>
> This looks a bit backwards. i2c-gpio is a i2c driver which happens to
> utilize the gpio framework, not the other way around.

Yes, this should be in devicetree/bindings/i2c/i2c-gpio.txt

>
>> @@ -0,0 +1,39 @@
>> +GPIO-based I2C
>> +
>> +Required properties:
>> +- compatible : should be "i2c-gpio".
>> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
>> +Optional properties:
>> +- sda-is-open-drain : present if SDA gpio is open-drain.
>> +- scl-is-open-drain : present if SCL gpio is open-drain.
>> +- scl-is-output-only : present if SCL is an output gpio only.
>
> I think "present if the output driver for SCL cannot be turned off" is
> more accurate. Might also be worth mentioning that this will prevent
> clock stretching from working.
>
>> --- a/drivers/i2c/busses/i2c-gpio.c
>> +++ b/drivers/i2c/busses/i2c-gpio.c
>> @@ -14,6 +14,9 @@
>>  #include <linux/module.h>
>>  #include <linux/slab.h>
>>  #include <linux/platform_device.h>
>> +#include <linux/of_platform.h>
>> +#include <linux/of_gpio.h>
>> +#include <linux/of_i2c.h>
>
> Do these headers provide stubs so non-of platforms won't break?

yes.

>
>> @@ -83,11 +86,52 @@ 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;
>> +       struct device_node *np = pdev->dev.of_node;
>
> Would be nice if this could be eliminated on non-of platforms.

It's pretty benign.  However, for current mainline this needs to be
protected with a #ifdef CONFIG_OF.  In 2.6.29, the conditional can be
removed since of_node will be a permanent part of struct device.

>
>>        int ret;
>>
>>        pdata = pdev->dev.platform_data;
>> -       if (!pdata)
>> -               return -ENXIO;
>> +       if (!pdata) {
>> +               if (np && of_gpio_count(np) >= 2) {
>
> If that expression somehow always evaluates to false on non-of
> platforms, this might be ok. But please confirm if this is the case;
> otherwise, it looks like a pretty large addition to an otherwise very
> small driver.
>
> How about a tiny bit of restructuring: Move the block below into a
> separate function, which is only called if some constant expression
> says that of is enabled. Then you can move the declaration above
> either into the if block or into the function, depending on where you
> want to do the conditional above.

Yes, moving the dt decoding code to a separate function would keep the
dt-support better isolated from the core of the driver and would make
the CONFIG_OF/!CONFIG_OF handling better.

>
>>  static struct platform_driver i2c_gpio_driver = {
>>        .driver         = {
>>                .name   = "i2c-gpio",
>>                .owner  = THIS_MODULE,
>> +               .of_match_table = i2c_gpio_match,
>
> Is this field always present even when of is disabled?

No, not yet.  It will be in 2.6.29.

g.

-- 
Grant Likely, B.Sc., P.Eng.
Secret Lab Technologies Ltd.

Re: [PATCH] i2c-gpio: add devicetree support

[Jon Loeliger]

> > Is this field always present even when of is disabled?
> 
> No, not yet.  It will be in 2.6.29.

Perhaps even 2.6.39? :-)

jdl

[PATCH 1/3] of: define dummy of_get_property if not CONFIG_OF

[Thomas Chou]

This will help to reduce the ifdef CONFIG_OF needed in most
platform data probing.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 include/linux/of.h |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/include/linux/of.h b/include/linux/of.h
index cad7cf0..5e122cb 100644
--- a/include/linux/of.h
+++ b/include/linux/of.h
@@ -222,5 +222,13 @@ extern void of_attach_node(struct device_node *);
 extern void of_detach_node(struct device_node *);
 #endif
 
+#else /* !CONFIG_OF */
+
+static inline const void *of_get_property(const struct device_node *node,
+				const char *name, int *lenp)
+{
+	return NULL;
+}
+
 #endif /* CONFIG_OF */
 #endif /* _LINUX_OF_H */
-- 
1.7.3.5

Re: [PATCH 1/3] of: define dummy of_get_property if not CONFIG_OF

[Grant Likely]

On Mon, Jan 31, 2011 at 11:25:49PM +0800, Thomas Chou wrote:
> This will help to reduce the ifdef CONFIG_OF needed in most
> platform data probing.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>

With moving the dt reading into a separate function protected by
CONFIG_OF, this patch shouldn't be necessary.

g.

> ---
>  include/linux/of.h |    8 ++++++++
>  1 files changed, 8 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/of.h b/include/linux/of.h
> index cad7cf0..5e122cb 100644
> --- a/include/linux/of.h
> +++ b/include/linux/of.h
> @@ -222,5 +222,13 @@ extern void of_attach_node(struct device_node *);
>  extern void of_detach_node(struct device_node *);
>  #endif
>  
> +#else /* !CONFIG_OF */
> +
> +static inline const void *of_get_property(const struct device_node *node,
> +				const char *name, int *lenp)
> +{
> +	return NULL;
> +}
> +
>  #endif /* CONFIG_OF */
>  #endif /* _LINUX_OF_H */
> -- 
> 1.7.3.5
> 

[PATCH 2/3] of: of_gpiochip_add is needed only for gpiolib

[Thomas Chou]

Some gpio drivers may not use gpiolib. In this case, struct gpio_chip
is not defined and of_gpiochip_add/remove are not needed.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
 include/linux/of_gpio.h |    4 ++++
 1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
index 6598c04..fc96af0 100644
--- a/include/linux/of_gpio.h
+++ b/include/linux/of_gpio.h
@@ -71,9 +71,13 @@ static inline unsigned int of_gpio_count(struct device_node *np)
 	return 0;
 }
 
+#ifdef CONFIG_GPIOLIB
+
 static inline void of_gpiochip_add(struct gpio_chip *gc) { }
 static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
 
+#endif /* CONFIG_GPIOLIB */
+
 #endif /* CONFIG_OF_GPIO */
 
 /**
-- 
1.7.3.5

Re: [PATCH 2/3] of: of_gpiochip_add is needed only for gpiolib

[Grant Likely]

On Mon, Jan 31, 2011 at 11:25:50PM +0800, Thomas Chou wrote:
> Some gpio drivers may not use gpiolib. In this case, struct gpio_chip
> is not defined and of_gpiochip_add/remove are not needed.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>

I'm not thrilled with the idea of supporting a gpio subsystem that
doesn't use gpiolib.  These functions don't even make sense if gpiolib
isn't being used (gpio_chip is a gpiolib construct).

Have you posted a patch that depends on this change?  Patch 3 in this
series doesn't seem to need it.

g.

> ---
>  include/linux/of_gpio.h |    4 ++++
>  1 files changed, 4 insertions(+), 0 deletions(-)
> 
> diff --git a/include/linux/of_gpio.h b/include/linux/of_gpio.h
> index 6598c04..fc96af0 100644
> --- a/include/linux/of_gpio.h
> +++ b/include/linux/of_gpio.h
> @@ -71,9 +71,13 @@ static inline unsigned int of_gpio_count(struct device_node *np)
>  	return 0;
>  }
>  
> +#ifdef CONFIG_GPIOLIB
> +
>  static inline void of_gpiochip_add(struct gpio_chip *gc) { }
>  static inline void of_gpiochip_remove(struct gpio_chip *gc) { }
>  
> +#endif /* CONFIG_GPIOLIB */
> +
>  #endif /* CONFIG_OF_GPIO */
>  
>  /**
> -- 
> 1.7.3.5
> 

[PATCH 3/3 v2] i2c-gpio: add devicetree support

[Thomas Chou]

From: Albert Herranz <albert_herranz@yahoo.es>

This patch is based on an earlier patch from Albert Herranz,
http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
9854eb78607c641ab5ae85bcbe3c9d14ac113733

The dts binding is modified as Grant suggested. The of probing
is merged inline instead of a separate file. It uses the newer
of gpio probe.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
for v2.6.39
v2 move of nodes probing to a hook, which will be optimized out
     when devicetree is not used.
   use linux/gpio.h.
   move binding doc to i2c/i2c-gpio.txt.

 Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 +++++++++++
 drivers/i2c/busses/i2c-gpio.c                      |   73 +++++++++++++++++++-
 2 files changed, 110 insertions(+), 3 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
new file mode 100644
index 0000000..541fb10
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
@@ -0,0 +1,40 @@
+GPIO-based I2C
+
+Required properties:
+- compatible : should be "i2c-gpio".
+- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
+Optional properties:
+- sda-is-open-drain : present if SDA gpio is open-drain.
+- scl-is-open-drain : present if SCL gpio is open-drain.
+- scl-is-output-only : present if the output driver for SCL cannot be
+	turned off. this will prevent clock stretching from working.
+- udelay : signal toggle delay. SCL frequency is (500 / udelay) kHz
+- timeout : clock stretching timeout in milliseconds.
+
+Example:
+
+gpio0: starlet-gpio@0d8000c0 {
+	compatible = "nintendo,starlet-gpio";
+	reg = <0d8000c0 4>;
+	gpio-controller;
+	#gpio-cells = <2>;
+};
+
+i2c-video {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "i2c-gpio";
+
+	gpios = <&gpio0 10 0	/* SDA line */
+		 &gpio0 11 0	/* SCL line */
+		>;
+	sda-is-open-drain;
+	scl-is-open-drain;
+	scl-is-output-only;
+	udelay = <2>;
+
+	audio-video-encoder {
+		compatible = "nintendo,wii-ave-rvl";
+		reg = <70>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index d9aa9a6..dbf5b85 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,8 +14,10 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
-
-#include <asm/gpio.h>
+#include <linux/gpio.h>
+#include <linux/of_platform.h>
+#include <linux/of_gpio.h>
+#include <linux/of_i2c.h>
 
 /* Toggle SDA by changing the direction of the pin */
 static void i2c_gpio_setsda_dir(void *data, int state)
@@ -78,6 +80,53 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
+/* Check if devicetree nodes exist and build platform data */
+static struct i2c_gpio_platform_data *i2c_gpio_of_probe(
+	struct platform_device *pdev)
+{
+	struct i2c_gpio_platform_data *pdata = NULL;
+	struct device_node *np = pdev->dev.of_node;
+
+	if (np && of_gpio_count(np) >= 2) {
+		const __be32 *prop;
+		int sda_pin, scl_pin;
+
+		sda_pin = of_get_gpio_flags(np, 0, NULL);
+		scl_pin = of_get_gpio_flags(np, 1, NULL);
+		if (sda_pin < 0 || scl_pin < 0) {
+			pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+			       np->full_name, sda_pin, scl_pin);
+			goto err_gpio_pin;
+		}
+		pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+		if (!pdata)
+			goto err_alloc_pdata;
+		pdata->sda_pin = sda_pin;
+		pdata->scl_pin = scl_pin;
+		prop = of_get_property(np, "sda-is-open-drain", NULL);
+		if (prop)
+			pdata->sda_is_open_drain = 1;
+		prop = of_get_property(np, "scl-is-open-drain", NULL);
+		if (prop)
+			pdata->scl_is_open_drain = 1;
+		prop = of_get_property(np, "scl-is-output-only", NULL);
+		if (prop)
+			pdata->scl_is_output_only = 1;
+		prop = of_get_property(np, "udelay", NULL);
+		if (prop)
+			pdata->udelay = be32_to_cpup(prop);
+		prop = of_get_property(np, "timeout", NULL);
+		if (prop) {
+			pdata->timeout =
+				msecs_to_jiffies(be32_to_cpup(prop));
+		}
+	}
+
+err_gpio_pin:
+err_alloc_pdata:
+	return pdata;
+}
+
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 {
 	struct i2c_gpio_platform_data *pdata;
@@ -87,6 +136,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata)
+		pdata = i2c_gpio_of_probe(pdev);
+	if (!pdata)
 		return -ENXIO;
 
 	ret = -ENOMEM;
@@ -143,6 +194,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.
@@ -161,6 +213,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 		 pdata->scl_is_output_only
 		 ? ", no clock stretching" : "");
 
+	/* Now register all the child nodes */
+	of_i2c_register_devices(adap);
+
 	return 0;
 
 err_add_bus:
@@ -172,6 +227,8 @@ err_request_sda:
 err_alloc_bit_data:
 	kfree(adap);
 err_alloc_adap:
+	if (!pdev->dev.platform_data)
+		kfree(pdata);
 	return ret;
 }
 
@@ -179,23 +236,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 {
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_adapter *adap;
+	struct i2c_algo_bit_data *bit_data;
 
 	adap = platform_get_drvdata(pdev);
-	pdata = pdev->dev.platform_data;
+	bit_data = adap->algo_data;
+	pdata = bit_data->data;
 
 	i2c_del_adapter(adap);
 	gpio_free(pdata->scl_pin);
 	gpio_free(pdata->sda_pin);
 	kfree(adap->algo_data);
 	kfree(adap);
+	if (!pdev->dev.platform_data)
+		kfree(pdata);
 
 	return 0;
 }
 
+static const struct of_device_id i2c_gpio_match[] = {
+	{ .compatible = "i2c-gpio", },
+	{},
+};
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = i2c_gpio_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.3.5

Re: [3/3,v2] i2c-gpio: add devicetree support

[Milton Miller]

On Mon, 31 Jan 2011 about 15:25:51 -0000, Thomas Chou wrote:

> This patch is based on an earlier patch from Albert Herranz,
> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
> 
> The dts binding is modified as Grant suggested. The of probing
> is merged inline instead of a separate file. It uses the newer
> of gpio probe.

> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> new file mode 100644
> index 0000000..541fb10
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -0,0 +1,40 @@
> +GPIO-based I2C
> +
> +Required properties:
> +- compatible : should be "i2c-gpio".
> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> +Optional properties:
> +- sda-is-open-drain : present if SDA gpio is open-drain.
> +- scl-is-open-drain : present if SCL gpio is open-drain.
> +- scl-is-output-only : present if the output driver for SCL cannot be
> +	turned off. this will prevent clock stretching from working.
> +- udelay : signal toggle delay. SCL frequency is (500 / udelay) kHz
> +- timeout : clock stretching timeout in milliseconds.


This description needs a lot more text, specifically what happens
when these optional parameters are not present.

Why are the sda / sdl properties required, why can't the gpio description
describe them?   Should't they be part of the flags on the gpio?

And come to think about it, udelay is not a good property name.  The
fact linux uses it for a name of a function that causes a microsecond
delay not withstanding.


>  
> +/* Check if devicetree nodes exist and build platform data */
> +static struct i2c_gpio_platform_data *i2c_gpio_of_probe(
> +	struct platform_device *pdev)
> +{
> +	struct i2c_gpio_platform_data *pdata = NULL;
> +	struct device_node *np = pdev->dev.of_node;
> +
> +	if (np && of_gpio_count(np) >= 2) {
> +		const __be32 *prop;
> +		int sda_pin, scl_pin;
> +
> +		sda_pin = of_get_gpio_flags(np, 0, NULL);
> +		scl_pin = of_get_gpio_flags(np, 1, NULL);
> +		if (sda_pin < 0 || scl_pin < 0) {
> +			pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> +			       np->full_name, sda_pin, scl_pin);
> +			goto err_gpio_pin;
> +		}
> +		pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +		if (!pdata)
> +			goto err_alloc_pdata;
> +		pdata->sda_pin = sda_pin;
> +		pdata->scl_pin = scl_pin;
> +		prop = of_get_property(np, "sda-is-open-drain", NULL);
> +		if (prop)
> +			pdata->sda_is_open_drain = 1;
> +		prop = of_get_property(np, "scl-is-open-drain", NULL);
> +		if (prop)
> +			pdata->scl_is_open_drain = 1;
> +		prop = of_get_property(np, "scl-is-output-only", NULL);
> +		if (prop)
> +			pdata->scl_is_output_only = 1;
> +		prop = of_get_property(np, "udelay", NULL);
> +		if (prop)
> +			pdata->udelay = be32_to_cpup(prop);
> +		prop = of_get_property(np, "timeout", NULL);
> +		if (prop) {
> +			pdata->timeout =
> +				msecs_to_jiffies(be32_to_cpup(prop));
> +		}
> +	}
> +
> +err_gpio_pin:
> +err_alloc_pdata:
> +	return pdata;
> +}

This looks nicer but  we don't really need two labels for the same point.
Also, it seems the whole function could be shifted left one tab by
returning early if the gpio count doesn't exist.


> +
>  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  {
>  	struct i2c_gpio_platform_data *pdata;
> @@ -87,6 +136,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  
>  	pdata = pdev->dev.platform_data;
>  	if (!pdata)
> +		pdata = i2c_gpio_of_probe(pdev);
> +	if (!pdata)
>  		return -ENXIO;
>  
>  	ret = -ENOMEM;
> @@ -143,6 +194,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.
> @@ -161,6 +213,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  		 pdata->scl_is_output_only
>  		 ? ", no clock stretching" : "");
>  
> +	/* Now register all the child nodes */
> +	of_i2c_register_devices(adap);
> +
>  	return 0;
>  
>  err_add_bus:
> @@ -172,6 +227,8 @@ err_request_sda:
>  err_alloc_bit_data:
>  	kfree(adap);
>  err_alloc_adap:
> +	if (!pdev->dev.platform_data)
> +		kfree(pdata);

This looks better with the code move to the function above, but
I don't like the condition on this free here ...

>  	return ret;
>  }
>  
> @@ -179,23 +236,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
>  {
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_adapter *adap;
> +	struct i2c_algo_bit_data *bit_data;
>  
>  	adap = platform_get_drvdata(pdev);
> -	pdata = pdev->dev.platform_data;
> +	bit_data = adap->algo_data;
> +	pdata = bit_data->data;
>  
>  	i2c_del_adapter(adap);
>  	gpio_free(pdata->scl_pin);
>  	gpio_free(pdata->sda_pin);
>  	kfree(adap->algo_data);
>  	kfree(adap);
> +	if (!pdev->dev.platform_data)
> +		kfree(pdata);

and especially here ... it seems unrelated.

Looking at devices/base/platform.c, how about just allocating the
initial platform data struct on the stack in i2c_gpio_of_probe
and then calling platform_device_add_data?  That way the pdev
is responsible for freeing the data.

>  
>  	return 0;
>  }
>  
> +static const struct of_device_id i2c_gpio_match[] = {
> +	{ .compatible = "i2c-gpio", },
> +	{},
> +};
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = i2c_gpio_match,
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= __devexit_p(i2c_gpio_remove),

milton

Re: [3/3,v2] i2c-gpio: add devicetree support

[Grant Likely]

On Mon, Jan 31, 2011 at 2:14 PM, Milton Miller <miltonm@bga.com> wrote:
> On Mon, 31 Jan 2011 about 15:25:51 -0000, Thomas Chou wrote:
>
>> This patch is based on an earlier patch from Albert Herranz,
>> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
>> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
>>
>> The dts binding is modified as Grant suggested. The of probing
>> is merged inline instead of a separate file. It uses the newer
>> of gpio probe.
>
>> @@ -172,6 +227,8 @@ err_request_sda:
>>  err_alloc_bit_data:
>>       kfree(adap);
>>  err_alloc_adap:
>> +     if (!pdev->dev.platform_data)
>> +             kfree(pdata);
>
> This looks better with the code move to the function above, but
> I don't like the condition on this free here ...
>
>>       return ret;
>>  }
>>
>> @@ -179,23 +236,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
>>  {
>>       struct i2c_gpio_platform_data *pdata;
>>       struct i2c_adapter *adap;
>> +     struct i2c_algo_bit_data *bit_data;
>>
>>       adap = platform_get_drvdata(pdev);
>> -     pdata = pdev->dev.platform_data;
>> +     bit_data = adap->algo_data;
>> +     pdata = bit_data->data;
>>
>>       i2c_del_adapter(adap);
>>       gpio_free(pdata->scl_pin);
>>       gpio_free(pdata->sda_pin);
>>       kfree(adap->algo_data);
>>       kfree(adap);
>> +     if (!pdev->dev.platform_data)
>> +             kfree(pdata);
>
> and especially here ... it seems unrelated.
>
> Looking at devices/base/platform.c, how about just allocating the
> initial platform data struct on the stack in i2c_gpio_of_probe
> and then calling platform_device_add_data?  That way the pdev
> is responsible for freeing the data.

No, *do not* use platform_device_add_data().  It stores the pointer in
pdev->dev.platform_data which gets freed when the last reference to
the device is dropped (it gets removed from the bus).  Thomas wants
something that gets freed when the driver is unbound.

Driver code must never modify the pdev->dev.platform_data pointer.
Doing so causes all kinds of lifecycle ambiguity which messes up
driver unbinding/rebinding and module load/unload.  When a driver
needs a modifiable copy of platform data, then it must include a copy
of the platform_data in the driver private data structure.  It should
be straight forward to refactor this driver to do so.

g.

[PATCH] i2c-gpio: add devicetree support

[Thomas Chou]

This patch adds devicetree support to i2c-gpio driver. It converts
dts bindings and allocates a struct i2c_gpio_platform_data, which
will be freed on driver removal.

Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
---
for v2.6.39
v2 move of nodes probing to a hook, which will be optimized out
     when devicetree is not used.
   use linux/gpio.h.
   move binding doc to i2c/i2c-gpio.txt.
v3 use speed-hz instead of udelay in dts binding.
   condition the devicetree probe.

 Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 ++++++++++
 drivers/i2c/busses/i2c-gpio.c                      |   83 +++++++++++++++++++-
 2 files changed, 119 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
new file mode 100644
index 0000000..38ef4f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
@@ -0,0 +1,40 @@
+GPIO-based I2C
+
+Required properties:
+- compatible : should be "i2c-gpio".
+- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
+Optional properties:
+- sda-is-open-drain : present if SDA gpio is open-drain.
+- scl-is-open-drain : present if SCL gpio is open-drain.
+- scl-is-output-only : present if the output driver for SCL cannot be
+	turned off. this will prevent clock stretching from working.
+- speed-hz : SCL frequency.
+- timeout : clock stretching timeout in milliseconds.
+
+Example:
+
+gpio0: starlet-gpio@0d8000c0 {
+	compatible = "nintendo,starlet-gpio";
+	reg = <0d8000c0 4>;
+	gpio-controller;
+	#gpio-cells = <2>;
+};
+
+i2c-video {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "i2c-gpio";
+
+	gpios = <&gpio0 10 0	/* SDA line */
+		 &gpio0 11 0	/* SCL line */
+		>;
+	sda-is-open-drain;
+	scl-is-open-drain;
+	scl-is-output-only;
+	speed-hz = <250000>;
+
+	audio-video-encoder {
+		compatible = "nintendo,wii-ave-rvl";
+		reg = <70>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index d9aa9a6..4b31bbe 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,8 +14,8 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
-
-#include <asm/gpio.h>
+#include <linux/gpio.h>
+#include <linux/of_i2c.h>
 
 /* Toggle SDA by changing the direction of the pin */
 static void i2c_gpio_setsda_dir(void *data, int state)
@@ -78,15 +78,74 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
+#ifdef CONFIG_OF
+#include <linux/of_gpio.h>
+
+/* Check if devicetree nodes exist and build platform data */
+static struct i2c_gpio_platform_data *i2c_gpio_of_probe(
+	struct platform_device *pdev)
+{
+	struct i2c_gpio_platform_data *pdata = NULL;
+	struct device_node *np = pdev->dev.of_node;
+	const __be32 *prop;
+	int sda_pin, scl_pin;
+
+	if (!np || of_gpio_count(np) < 2)
+		goto err;
+
+	sda_pin = of_get_gpio_flags(np, 0, NULL);
+	scl_pin = of_get_gpio_flags(np, 1, NULL);
+	if (sda_pin < 0 || scl_pin < 0) {
+		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+		       np->full_name, sda_pin, scl_pin);
+		goto err;
+	}
+
+	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
+	if (!pdata)
+		goto err;
+
+	pdata->sda_pin = sda_pin;
+	pdata->scl_pin = scl_pin;
+	prop = of_get_property(np, "sda-is-open-drain", NULL);
+	if (prop)
+		pdata->sda_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-open-drain", NULL);
+	if (prop)
+		pdata->scl_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-output-only", NULL);
+	if (prop)
+		pdata->scl_is_output_only = 1;
+	prop = of_get_property(np, "speed-hz", NULL);
+	if (prop)
+		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
+	prop = of_get_property(np, "timeout", NULL);
+	if (prop) {
+		pdata->timeout =
+			msecs_to_jiffies(be32_to_cpup(prop));
+	}
+err:
+	return pdata;
+}
+#else
+static struct i2c_gpio_platform_data *i2c_gpio_of_probe(
+	struct platform_device *pdev)
+{
+	return NULL;
+}
+#endif
+
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 {
-	struct i2c_gpio_platform_data *pdata;
+	struct i2c_gpio_platform_data *pdata, *pdata_of = NULL;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
 	int ret;
 
 	pdata = pdev->dev.platform_data;
 	if (!pdata)
+		pdata = pdata_of = i2c_gpio_of_probe(pdev);
+	if (!pdata)
 		return -ENXIO;
 
 	ret = -ENOMEM;
@@ -143,6 +202,8 @@ 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;
+	if (pdata_of)
+		adap->dev.of_node = pdev->dev.of_node;
 
 	/*
 	 * If "dev->id" is negative we consider it as zero.
@@ -161,6 +222,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 		 pdata->scl_is_output_only
 		 ? ", no clock stretching" : "");
 
+	/* Now register all the child nodes */
+	of_i2c_register_devices(adap);
+
 	return 0;
 
 err_add_bus:
@@ -172,6 +236,7 @@ err_request_sda:
 err_alloc_bit_data:
 	kfree(adap);
 err_alloc_adap:
+	kfree(pdata_of);
 	return ret;
 }
 
@@ -179,23 +244,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 {
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_adapter *adap;
+	struct i2c_algo_bit_data *bit_data;
 
 	adap = platform_get_drvdata(pdev);
-	pdata = pdev->dev.platform_data;
+	bit_data = adap->algo_data;
+	pdata = bit_data->data;
 
 	i2c_del_adapter(adap);
 	gpio_free(pdata->scl_pin);
 	gpio_free(pdata->sda_pin);
 	kfree(adap->algo_data);
+	if (adap->dev.of_node)
+		kfree(pdata);
 	kfree(adap);
 
 	return 0;
 }
 
+static const struct of_device_id i2c_gpio_match[] = {
+	{ .compatible = "i2c-gpio", },
+	{},
+};
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = i2c_gpio_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.3.5

Re: [PATCH] i2c-gpio: add devicetree support

[Håvard Skinnemoen]

On Wed, Feb 2, 2011 at 6:26 PM, Thomas Chou <thomas@wytron.com.tw> wrote:
> This patch adds devicetree support to i2c-gpio driver. It converts
> dts bindings and allocates a struct i2c_gpio_platform_data, which
> will be freed on driver removal.
>
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>

Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>

Re: [PATCH] i2c-gpio: add devicetree support

[Grant Likely]

On Thu, Feb 03, 2011 at 10:26:20AM +0800, Thomas Chou wrote:
> This patch adds devicetree support to i2c-gpio driver. It converts
> dts bindings and allocates a struct i2c_gpio_platform_data, which
> will be freed on driver removal.
> 
> Signed-off-by: Albert Herranz <albert_herranz@yahoo.es>
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> ---
> for v2.6.39
> v2 move of nodes probing to a hook, which will be optimized out
>      when devicetree is not used.
>    use linux/gpio.h.
>    move binding doc to i2c/i2c-gpio.txt.
> v3 use speed-hz instead of udelay in dts binding.
>    condition the devicetree probe.
> 
>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 ++++++++++
>  drivers/i2c/busses/i2c-gpio.c                      |   83 +++++++++++++++++++-
>  2 files changed, 119 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> new file mode 100644
> index 0000000..38ef4f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -0,0 +1,40 @@
> +GPIO-based I2C
> +
> +Required properties:
> +- compatible : should be "i2c-gpio".
> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> +Optional properties:
> +- sda-is-open-drain : present if SDA gpio is open-drain.
> +- scl-is-open-drain : present if SCL gpio is open-drain.
> +- scl-is-output-only : present if the output driver for SCL cannot be
> +	turned off. this will prevent clock stretching from working.
> +- speed-hz : SCL frequency.
> +- timeout : clock stretching timeout in milliseconds.
> +
> +Example:
> +
> +gpio0: starlet-gpio@0d8000c0 {
> +	compatible = "nintendo,starlet-gpio";
> +	reg = <0d8000c0 4>;
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +};
> +
> +i2c-video {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "i2c-gpio";
> +
> +	gpios = <&gpio0 10 0	/* SDA line */
> +		 &gpio0 11 0	/* SCL line */
> +		>;
> +	sda-is-open-drain;
> +	scl-is-open-drain;
> +	scl-is-output-only;
> +	speed-hz = <250000>;
> +
> +	audio-video-encoder {
> +		compatible = "nintendo,wii-ave-rvl";
> +		reg = <70>;
> +	};
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index d9aa9a6..4b31bbe 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,8 +14,8 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> -
> -#include <asm/gpio.h>
> +#include <linux/gpio.h>
> +#include <linux/of_i2c.h>
>  
>  /* Toggle SDA by changing the direction of the pin */
>  static void i2c_gpio_setsda_dir(void *data, int state)
> @@ -78,15 +78,74 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> +#ifdef CONFIG_OF
> +#include <linux/of_gpio.h>
> +
> +/* Check if devicetree nodes exist and build platform data */
> +static struct i2c_gpio_platform_data *i2c_gpio_of_probe(
> +	struct platform_device *pdev)
> +{
> +	struct i2c_gpio_platform_data *pdata = NULL;
> +	struct device_node *np = pdev->dev.of_node;
> +	const __be32 *prop;
> +	int sda_pin, scl_pin;
> +
> +	if (!np || of_gpio_count(np) < 2)
> +		goto err;
> +
> +	sda_pin = of_get_gpio_flags(np, 0, NULL);
> +	scl_pin = of_get_gpio_flags(np, 1, NULL);
> +	if (sda_pin < 0 || scl_pin < 0) {
> +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> +		       np->full_name, sda_pin, scl_pin);
> +		goto err;
> +	}
> +
> +	pdata = kzalloc(sizeof(*pdata), GFP_KERNEL);
> +	if (!pdata)
> +		goto err;
> +
> +	pdata->sda_pin = sda_pin;
> +	pdata->scl_pin = scl_pin;
> +	prop = of_get_property(np, "sda-is-open-drain", NULL);
> +	if (prop)
> +		pdata->sda_is_open_drain = 1;
> +	prop = of_get_property(np, "scl-is-open-drain", NULL);
> +	if (prop)
> +		pdata->scl_is_open_drain = 1;
> +	prop = of_get_property(np, "scl-is-output-only", NULL);
> +	if (prop)
> +		pdata->scl_is_output_only = 1;
> +	prop = of_get_property(np, "speed-hz", NULL);
> +	if (prop)
> +		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
> +	prop = of_get_property(np, "timeout", NULL);
> +	if (prop) {
> +		pdata->timeout =
> +			msecs_to_jiffies(be32_to_cpup(prop));
> +	}
> +err:
> +	return pdata;
> +}
> +#else
> +static struct i2c_gpio_platform_data *i2c_gpio_of_probe(
> +	struct platform_device *pdev)
> +{
> +	return NULL;
> +}
> +#endif
> +
>  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  {
> -	struct i2c_gpio_platform_data *pdata;
> +	struct i2c_gpio_platform_data *pdata, *pdata_of = NULL;
>  	struct i2c_algo_bit_data *bit_data;
>  	struct i2c_adapter *adap;
>  	int ret;
>  
>  	pdata = pdev->dev.platform_data;
>  	if (!pdata)
> +		pdata = pdata_of = i2c_gpio_of_probe(pdev);
> +	if (!pdata)
>  		return -ENXIO;
>  
>  	ret = -ENOMEM;
> @@ -143,6 +202,8 @@ 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;
> +	if (pdata_of)
> +		adap->dev.of_node = pdev->dev.of_node;
>  
>  	/*
>  	 * If "dev->id" is negative we consider it as zero.
> @@ -161,6 +222,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  		 pdata->scl_is_output_only
>  		 ? ", no clock stretching" : "");
>  
> +	/* Now register all the child nodes */
> +	of_i2c_register_devices(adap);
> +
>  	return 0;
>  
>  err_add_bus:
> @@ -172,6 +236,7 @@ err_request_sda:
>  err_alloc_bit_data:
>  	kfree(adap);
>  err_alloc_adap:
> +	kfree(pdata_of);
>  	return ret;
>  }
>  
> @@ -179,23 +244,33 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
>  {
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_adapter *adap;
> +	struct i2c_algo_bit_data *bit_data;
>  
>  	adap = platform_get_drvdata(pdev);
> -	pdata = pdev->dev.platform_data;
> +	bit_data = adap->algo_data;
> +	pdata = bit_data->data;
>  
>  	i2c_del_adapter(adap);
>  	gpio_free(pdata->scl_pin);
>  	gpio_free(pdata->sda_pin);
>  	kfree(adap->algo_data);
> +	if (adap->dev.of_node)
> +		kfree(pdata);

Oh, wow.  Okay, so this driver does an odd thing.  It doesn't have
its own private data structure and instead uses pdata as private data.
Rather than doing these gymnastics with the pdata pointer, may I
suggest the following refactorization:

1) Add a private data structure:
struct i2c_gpio_private_data {
	struct i2c_adapter adap;
	struct i2c_algo_bit_data bit_data;
	struct i2c_gpio_platform_data pdata;
}

2) simplify the alloc/free of data in probe/remove:

alloc:
	struct i2c_gpio_private_data *priv;
	priv = kzalloc(sizeof(*priv), GFP_KERNEL);
	if (!priv)
		return -ENOMEM;
	adap = &priv->adap;
	bit_data = &priv->bit_data;
	pdata = &priv->pdata;
	...
	platform_set_drvdata(pdev, priv);

free:
	priv = platform_get_drvdata(pdev);
	kfree(priv);

3) Copy pdata into the allocated private data

	if (pdev->dev.platform_data)
		adap->pdata = *pdev->dev.platform_data;
	else
		if (i2c_gpio_of_probe(pdev, &adap->pdata))
			return -ENXIO;

Which should make the driver simpler and gets it away from using
platform_data directly.  It also gets rid of the pdata pointer
management gymnastics.

g.


>  	kfree(adap);
>  
>  	return 0;
>  }
>  
> +static const struct of_device_id i2c_gpio_match[] = {
> +	{ .compatible = "i2c-gpio", },
> +	{},
> +};
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = i2c_gpio_match,
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= __devexit_p(i2c_gpio_remove),
> -- 
> 1.7.3.5
> 

[PATCH v4] i2c-gpio: add devicetree support

[Thomas Chou]

This patch adds devicetree support to i2c-gpio driver. The allocation
of local data is integrated to a private structure, and use devm_*
helper for easy cleanup.

It is base on an earlier patch for gc-linux from
Albert Herranz <albert_herranz@yahoo.es>.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>
---
for v2.6.39
v2 move of nodes probing to a hook, which will be optimized out
     when devicetree is not used.
   use linux/gpio.h.
   move binding doc to i2c/i2c-gpio.txt.
v3 use speed-hz instead of udelay in dts binding.
   condition the devicetree probe.
v4 simplify private allocation.

 Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 +++++++
 drivers/i2c/busses/i2c-gpio.c                      |  108 ++++++++++++++++----
 2 files changed, 128 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
new file mode 100644
index 0000000..38ef4f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
@@ -0,0 +1,40 @@
+GPIO-based I2C
+
+Required properties:
+- compatible : should be "i2c-gpio".
+- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
+Optional properties:
+- sda-is-open-drain : present if SDA gpio is open-drain.
+- scl-is-open-drain : present if SCL gpio is open-drain.
+- scl-is-output-only : present if the output driver for SCL cannot be
+	turned off. this will prevent clock stretching from working.
+- speed-hz : SCL frequency.
+- timeout : clock stretching timeout in milliseconds.
+
+Example:
+
+gpio0: starlet-gpio@0d8000c0 {
+	compatible = "nintendo,starlet-gpio";
+	reg = <0d8000c0 4>;
+	gpio-controller;
+	#gpio-cells = <2>;
+};
+
+i2c-video {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "i2c-gpio";
+
+	gpios = <&gpio0 10 0	/* SDA line */
+		 &gpio0 11 0	/* SCL line */
+		>;
+	sda-is-open-drain;
+	scl-is-open-drain;
+	scl-is-output-only;
+	speed-hz = <250000>;
+
+	audio-video-encoder {
+		compatible = "nintendo,wii-ave-rvl";
+		reg = <70>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index d9aa9a6..3b2d694 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,8 +14,14 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_i2c.h>
 
-#include <asm/gpio.h>
+struct i2c_gpio_private_data {
+	struct i2c_adapter adap;
+	struct i2c_algo_bit_data bit_data;
+	struct i2c_gpio_platform_data pdata;
+};
 
 /* Toggle SDA by changing the direction of the pin */
 static void i2c_gpio_setsda_dir(void *data, int state)
@@ -78,24 +84,80 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
+#ifdef CONFIG_OF
+#include <linux/of_gpio.h>
+
+/* Check if devicetree nodes exist and build platform data */
+static int i2c_gpio_of_probe(struct platform_device *pdev,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const __be32 *prop;
+	int sda_pin, scl_pin;
+	int len;
+
+	if (!np || of_gpio_count(np) < 2)
+		return -ENXIO;
+
+	sda_pin = of_get_gpio_flags(np, 0, NULL);
+	scl_pin = of_get_gpio_flags(np, 1, NULL);
+	if (sda_pin < 0 || scl_pin < 0) {
+		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+		       np->full_name, sda_pin, scl_pin);
+		return -ENXIO;
+	}
+
+	pdata->sda_pin = sda_pin;
+	pdata->scl_pin = scl_pin;
+	prop = of_get_property(np, "sda-is-open-drain", NULL);
+	if (prop)
+		pdata->sda_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-open-drain", NULL);
+	if (prop)
+		pdata->scl_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-output-only", NULL);
+	if (prop)
+		pdata->scl_is_output_only = 1;
+	prop = of_get_property(np, "speed-hz", &len);
+	if (prop && len >= sizeof(__be32))
+		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
+	prop = of_get_property(np, "timeout", &len);
+	if (prop && len >= sizeof(__be32))
+		pdata->timeout = msecs_to_jiffies(be32_to_cpup(prop));
+
+	return 0;
+}
+#else
+static int i2c_gpio_of_probe(struct platform_device *pdev,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	return -ENXIO;
+}
+#endif
+
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 {
+	struct i2c_gpio_private_data *priv;
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
 	int ret;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata)
-		return -ENXIO;
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	adap = &priv->adap;
+	bit_data = &priv->bit_data;
+	pdata = &priv->pdata;
 
-	ret = -ENOMEM;
-	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
-	if (!adap)
-		goto err_alloc_adap;
-	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
-	if (!bit_data)
-		goto err_alloc_bit_data;
+	if (pdev->dev.platform_data) {
+		*pdata = *(struct i2c_gpio_platform_data *)
+			pdev->dev.platform_data;
+	} else {
+		ret = i2c_gpio_of_probe(pdev, pdata);
+		if (ret)
+			return ret;
+	}
 
 	ret = gpio_request(pdata->sda_pin, "sda");
 	if (ret)
@@ -143,6 +205,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,13 +217,16 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_add_bus;
 
-	platform_set_drvdata(pdev, adap);
+	platform_set_drvdata(pdev, priv);
 
 	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
 		 pdata->sda_pin, pdata->scl_pin,
 		 pdata->scl_is_output_only
 		 ? ", no clock stretching" : "");
 
+	/* Now register all the child nodes */
+	of_i2c_register_devices(adap);
+
 	return 0;
 
 err_add_bus:
@@ -168,34 +234,36 @@ err_add_bus:
 err_request_scl:
 	gpio_free(pdata->sda_pin);
 err_request_sda:
-	kfree(bit_data);
-err_alloc_bit_data:
-	kfree(adap);
-err_alloc_adap:
 	return ret;
 }
 
 static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 {
+	struct i2c_gpio_private_data *priv;
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_adapter *adap;
 
-	adap = platform_get_drvdata(pdev);
-	pdata = pdev->dev.platform_data;
+	priv = platform_get_drvdata(pdev);
+	adap = &priv->adap;
+	pdata = &priv->pdata;
 
 	i2c_del_adapter(adap);
 	gpio_free(pdata->scl_pin);
 	gpio_free(pdata->sda_pin);
-	kfree(adap->algo_data);
-	kfree(adap);
 
 	return 0;
 }
 
+static const struct of_device_id i2c_gpio_match[] = {
+	{ .compatible = "i2c-gpio", },
+	{},
+};
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = i2c_gpio_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.4

[PATCH v5] i2c-gpio: add devicetree support

[Thomas Chou]

From: Albert Herranz <albert_herranz@yahoo.es>

This patch adds devicetree support to i2c-gpio driver. The allocation
of local data is integrated to a private structure, and use devm_*
helper for easy cleanup.

It is base on an earlier patch for gc-linux from
Albert Herranz <albert_herranz@yahoo.es>.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
CC: Albert Herranz <albert_herranz@yahoo.es>
Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>
---
for v2.6.39
v2 move of nodes probing to a hook, which will be optimized out
     when devicetree is not used.
   use linux/gpio.h.
   move binding doc to i2c/i2c-gpio.txt.
v3 use speed-hz instead of udelay in dts binding.
   condition the devicetree probe.
v4 simplify private allocation.
v5 condition module device table export for of.

 Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 +++++++
 drivers/i2c/busses/i2c-gpio.c                      |  113 ++++++++++++++++----
 2 files changed, 133 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
new file mode 100644
index 0000000..38ef4f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
@@ -0,0 +1,40 @@
+GPIO-based I2C
+
+Required properties:
+- compatible : should be "i2c-gpio".
+- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
+Optional properties:
+- sda-is-open-drain : present if SDA gpio is open-drain.
+- scl-is-open-drain : present if SCL gpio is open-drain.
+- scl-is-output-only : present if the output driver for SCL cannot be
+	turned off. this will prevent clock stretching from working.
+- speed-hz : SCL frequency.
+- timeout : clock stretching timeout in milliseconds.
+
+Example:
+
+gpio0: starlet-gpio@0d8000c0 {
+	compatible = "nintendo,starlet-gpio";
+	reg = <0d8000c0 4>;
+	gpio-controller;
+	#gpio-cells = <2>;
+};
+
+i2c-video {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "i2c-gpio";
+
+	gpios = <&gpio0 10 0	/* SDA line */
+		 &gpio0 11 0	/* SCL line */
+		>;
+	sda-is-open-drain;
+	scl-is-open-drain;
+	scl-is-output-only;
+	speed-hz = <250000>;
+
+	audio-video-encoder {
+		compatible = "nintendo,wii-ave-rvl";
+		reg = <70>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index d9aa9a6..6cc5f15 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,8 +14,14 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_i2c.h>
 
-#include <asm/gpio.h>
+struct i2c_gpio_private_data {
+	struct i2c_adapter adap;
+	struct i2c_algo_bit_data bit_data;
+	struct i2c_gpio_platform_data pdata;
+};
 
 /* Toggle SDA by changing the direction of the pin */
 static void i2c_gpio_setsda_dir(void *data, int state)
@@ -78,24 +84,80 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
+#ifdef CONFIG_OF
+#include <linux/of_gpio.h>
+
+/* Check if devicetree nodes exist and build platform data */
+static int i2c_gpio_of_probe(struct platform_device *pdev,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const __be32 *prop;
+	int sda_pin, scl_pin;
+	int len;
+
+	if (!np || of_gpio_count(np) < 2)
+		return -ENXIO;
+
+	sda_pin = of_get_gpio_flags(np, 0, NULL);
+	scl_pin = of_get_gpio_flags(np, 1, NULL);
+	if (sda_pin < 0 || scl_pin < 0) {
+		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+		       np->full_name, sda_pin, scl_pin);
+		return -ENXIO;
+	}
+
+	pdata->sda_pin = sda_pin;
+	pdata->scl_pin = scl_pin;
+	prop = of_get_property(np, "sda-is-open-drain", NULL);
+	if (prop)
+		pdata->sda_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-open-drain", NULL);
+	if (prop)
+		pdata->scl_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-output-only", NULL);
+	if (prop)
+		pdata->scl_is_output_only = 1;
+	prop = of_get_property(np, "speed-hz", &len);
+	if (prop && len >= sizeof(__be32))
+		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
+	prop = of_get_property(np, "timeout", &len);
+	if (prop && len >= sizeof(__be32))
+		pdata->timeout = msecs_to_jiffies(be32_to_cpup(prop));
+
+	return 0;
+}
+#else
+static int i2c_gpio_of_probe(struct platform_device *pdev,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	return -ENXIO;
+}
+#endif
+
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 {
+	struct i2c_gpio_private_data *priv;
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
 	int ret;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata)
-		return -ENXIO;
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	adap = &priv->adap;
+	bit_data = &priv->bit_data;
+	pdata = &priv->pdata;
 
-	ret = -ENOMEM;
-	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
-	if (!adap)
-		goto err_alloc_adap;
-	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
-	if (!bit_data)
-		goto err_alloc_bit_data;
+	if (pdev->dev.platform_data) {
+		*pdata = *(struct i2c_gpio_platform_data *)
+			pdev->dev.platform_data;
+	} else {
+		ret = i2c_gpio_of_probe(pdev, pdata);
+		if (ret)
+			return ret;
+	}
 
 	ret = gpio_request(pdata->sda_pin, "sda");
 	if (ret)
@@ -143,6 +205,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,13 +217,16 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_add_bus;
 
-	platform_set_drvdata(pdev, adap);
+	platform_set_drvdata(pdev, priv);
 
 	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
 		 pdata->sda_pin, pdata->scl_pin,
 		 pdata->scl_is_output_only
 		 ? ", no clock stretching" : "");
 
+	/* Now register all the child nodes */
+	of_i2c_register_devices(adap);
+
 	return 0;
 
 err_add_bus:
@@ -168,34 +234,41 @@ err_add_bus:
 err_request_scl:
 	gpio_free(pdata->sda_pin);
 err_request_sda:
-	kfree(bit_data);
-err_alloc_bit_data:
-	kfree(adap);
-err_alloc_adap:
 	return ret;
 }
 
 static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 {
+	struct i2c_gpio_private_data *priv;
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_adapter *adap;
 
-	adap = platform_get_drvdata(pdev);
-	pdata = pdev->dev.platform_data;
+	priv = platform_get_drvdata(pdev);
+	adap = &priv->adap;
+	pdata = &priv->pdata;
 
 	i2c_del_adapter(adap);
 	gpio_free(pdata->scl_pin);
 	gpio_free(pdata->sda_pin);
-	kfree(adap->algo_data);
-	kfree(adap);
 
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id i2c_gpio_match[] = {
+	{ .compatible = "i2c-gpio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_gpio_match);
+#else /* CONFIG_OF */
+#define i2c_gpio_match NULL
+#endif /* CONFIG_OF */
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = i2c_gpio_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.4

Re: [PATCH v5] i2c-gpio: add devicetree support

[Grant Likely]

On Mon, Feb 14, 2011 at 10:30:10AM +0800, Thomas Chou wrote:
> From: Albert Herranz <albert_herranz@yahoo.es>
> 
> This patch adds devicetree support to i2c-gpio driver. The allocation
> of local data is integrated to a private structure, and use devm_*
> helper for easy cleanup.
> 
> It is base on an earlier patch for gc-linux from
> Albert Herranz <albert_herranz@yahoo.es>.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> CC: Albert Herranz <albert_herranz@yahoo.es>
> Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>

One minor nitpick below, but I'd be fine with cleaning that up via a
followup patch.  This one looks correct to me.

Acked-by: Grant Likely <grant.likely@secretlab.ca>

Ben, this one depends on a patch in my devicetree/next tree.  How do
you want to handle it?

g.

> ---
> for v2.6.39
> v2 move of nodes probing to a hook, which will be optimized out
>      when devicetree is not used.
>    use linux/gpio.h.
>    move binding doc to i2c/i2c-gpio.txt.
> v3 use speed-hz instead of udelay in dts binding.
>    condition the devicetree probe.
> v4 simplify private allocation.
> v5 condition module device table export for of.
> 
>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 +++++++
>  drivers/i2c/busses/i2c-gpio.c                      |  113 ++++++++++++++++----
>  2 files changed, 133 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> new file mode 100644
> index 0000000..38ef4f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -0,0 +1,40 @@
> +GPIO-based I2C
> +
> +Required properties:
> +- compatible : should be "i2c-gpio".
> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> +Optional properties:
> +- sda-is-open-drain : present if SDA gpio is open-drain.
> +- scl-is-open-drain : present if SCL gpio is open-drain.
> +- scl-is-output-only : present if the output driver for SCL cannot be
> +	turned off. this will prevent clock stretching from working.
> +- speed-hz : SCL frequency.
> +- timeout : clock stretching timeout in milliseconds.
> +
> +Example:
> +
> +gpio0: starlet-gpio@0d8000c0 {
> +	compatible = "nintendo,starlet-gpio";
> +	reg = <0d8000c0 4>;
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +};
> +
> +i2c-video {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "i2c-gpio";
> +
> +	gpios = <&gpio0 10 0	/* SDA line */
> +		 &gpio0 11 0	/* SCL line */
> +		>;
> +	sda-is-open-drain;
> +	scl-is-open-drain;
> +	scl-is-output-only;
> +	speed-hz = <250000>;
> +
> +	audio-video-encoder {
> +		compatible = "nintendo,wii-ave-rvl";
> +		reg = <70>;
> +	};
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index d9aa9a6..6cc5f15 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,8 +14,14 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of_i2c.h>
>  
> -#include <asm/gpio.h>
> +struct i2c_gpio_private_data {
> +	struct i2c_adapter adap;
> +	struct i2c_algo_bit_data bit_data;
> +	struct i2c_gpio_platform_data pdata;
> +};
>  
>  /* Toggle SDA by changing the direction of the pin */
>  static void i2c_gpio_setsda_dir(void *data, int state)
> @@ -78,24 +84,80 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> +#ifdef CONFIG_OF
> +#include <linux/of_gpio.h>

Instead of putting the include here, please add #ifdef CONFIG_OF
protection around the contents of linux/of_gpio.h itself.  I fully
support that change so that includes can remain at the top where they
belong.

> +
> +/* Check if devicetree nodes exist and build platform data */
> +static int i2c_gpio_of_probe(struct platform_device *pdev,
> +			     struct i2c_gpio_platform_data *pdata)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const __be32 *prop;
> +	int sda_pin, scl_pin;
> +	int len;
> +
> +	if (!np || of_gpio_count(np) < 2)
> +		return -ENXIO;
> +
> +	sda_pin = of_get_gpio_flags(np, 0, NULL);
> +	scl_pin = of_get_gpio_flags(np, 1, NULL);
> +	if (sda_pin < 0 || scl_pin < 0) {
> +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> +		       np->full_name, sda_pin, scl_pin);
> +		return -ENXIO;
> +	}
> +
> +	pdata->sda_pin = sda_pin;
> +	pdata->scl_pin = scl_pin;
> +	prop = of_get_property(np, "sda-is-open-drain", NULL);
> +	if (prop)
> +		pdata->sda_is_open_drain = 1;
> +	prop = of_get_property(np, "scl-is-open-drain", NULL);
> +	if (prop)
> +		pdata->scl_is_open_drain = 1;
> +	prop = of_get_property(np, "scl-is-output-only", NULL);
> +	if (prop)
> +		pdata->scl_is_output_only = 1;
> +	prop = of_get_property(np, "speed-hz", &len);
> +	if (prop && len >= sizeof(__be32))
> +		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
> +	prop = of_get_property(np, "timeout", &len);
> +	if (prop && len >= sizeof(__be32))
> +		pdata->timeout = msecs_to_jiffies(be32_to_cpup(prop));
> +
> +	return 0;
> +}
> +#else
> +static int i2c_gpio_of_probe(struct platform_device *pdev,
> +			     struct i2c_gpio_platform_data *pdata)
> +{
> +	return -ENXIO;
> +}
> +#endif
> +
>  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  {
> +	struct i2c_gpio_private_data *priv;
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_algo_bit_data *bit_data;
>  	struct i2c_adapter *adap;
>  	int ret;
>  
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata)
> -		return -ENXIO;
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	adap = &priv->adap;
> +	bit_data = &priv->bit_data;
> +	pdata = &priv->pdata;
>  
> -	ret = -ENOMEM;
> -	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> -	if (!adap)
> -		goto err_alloc_adap;
> -	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
> -	if (!bit_data)
> -		goto err_alloc_bit_data;
> +	if (pdev->dev.platform_data) {
> +		*pdata = *(struct i2c_gpio_platform_data *)
> +			pdev->dev.platform_data;
> +	} else {
> +		ret = i2c_gpio_of_probe(pdev, pdata);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = gpio_request(pdata->sda_pin, "sda");
>  	if (ret)
> @@ -143,6 +205,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,13 +217,16 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_add_bus;
>  
> -	platform_set_drvdata(pdev, adap);
> +	platform_set_drvdata(pdev, priv);
>  
>  	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
>  		 pdata->sda_pin, pdata->scl_pin,
>  		 pdata->scl_is_output_only
>  		 ? ", no clock stretching" : "");
>  
> +	/* Now register all the child nodes */
> +	of_i2c_register_devices(adap);
> +
>  	return 0;
>  
>  err_add_bus:
> @@ -168,34 +234,41 @@ err_add_bus:
>  err_request_scl:
>  	gpio_free(pdata->sda_pin);
>  err_request_sda:
> -	kfree(bit_data);
> -err_alloc_bit_data:
> -	kfree(adap);
> -err_alloc_adap:
>  	return ret;
>  }
>  
>  static int __devexit i2c_gpio_remove(struct platform_device *pdev)
>  {
> +	struct i2c_gpio_private_data *priv;
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_adapter *adap;
>  
> -	adap = platform_get_drvdata(pdev);
> -	pdata = pdev->dev.platform_data;
> +	priv = platform_get_drvdata(pdev);
> +	adap = &priv->adap;
> +	pdata = &priv->pdata;
>  
>  	i2c_del_adapter(adap);
>  	gpio_free(pdata->scl_pin);
>  	gpio_free(pdata->sda_pin);
> -	kfree(adap->algo_data);
> -	kfree(adap);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id i2c_gpio_match[] = {
> +	{ .compatible = "i2c-gpio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_gpio_match);
> +#else /* CONFIG_OF */
> +#define i2c_gpio_match NULL
> +#endif /* CONFIG_OF */
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = i2c_gpio_match,
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= __devexit_p(i2c_gpio_remove),
> -- 
> 1.7.4
> 

Re: [PATCH v5] i2c-gpio: add devicetree support

[Thomas Chou]

Hi Grant,

On 02/16/2011 01:49 PM, Grant Likely wrote:
> On Mon, Feb 14, 2011 at 10:30:10AM +0800, Thomas Chou wrote:
>> From: Albert Herranz<albert_herranz@yahoo.es>
>>
>> This patch adds devicetree support to i2c-gpio driver. The allocation
>> of local data is integrated to a private structure, and use devm_*
>> helper for easy cleanup.
>>
>> It is base on an earlier patch for gc-linux from
>> Albert Herranz<albert_herranz@yahoo.es>.
>>
>> Signed-off-by: Thomas Chou<thomas@wytron.com.tw>
>> CC: Albert Herranz<albert_herranz@yahoo.es>
>> Acked-by: Haavard Skinnemoen<hskinnemoen@gmail.com>
>
> One minor nitpick below, but I'd be fine with cleaning that up via a
> followup patch.  This one looks correct to me.
>
> Acked-by: Grant Likely<grant.likely@secretlab.ca>
>
> Ben, this one depends on a patch in my devicetree/next tree.  How do
> you want to handle it?
>
> g.
>

Thanks a lot.

>> +#ifdef CONFIG_OF
>> +#include<linux/of_gpio.h>
>
> Instead of putting the include here, please add #ifdef CONFIG_OF
> protection around the contents of linux/of_gpio.h itself.  I fully
> support that change so that includes can remain at the top where they
> belong.

Then, my earlier patch on Jan 31, might be another solution. This way 
of_gpio.h can be included at the top, and those of_gpio users will be 
optimized out when of or of_gpio is not configured.

[PATCH 2/3] of: of_gpiochip_add is needed only for gpiolib

Cheers,
Thomas

Re: [PATCH v5] i2c-gpio: add devicetree support

[Ben Dooks]

On Mon, Feb 14, 2011 at 10:30:10AM +0800, Thomas Chou wrote:
> From: Albert Herranz <albert_herranz@yahoo.es>
> 
> This patch adds devicetree support to i2c-gpio driver. The allocation
> of local data is integrated to a private structure, and use devm_*
> helper for easy cleanup.
> 
> It is base on an earlier patch for gc-linux from
> Albert Herranz <albert_herranz@yahoo.es>.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> CC: Albert Herranz <albert_herranz@yahoo.es>
> Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>
> ---
> for v2.6.39
> v2 move of nodes probing to a hook, which will be optimized out
>      when devicetree is not used.
>    use linux/gpio.h.
>    move binding doc to i2c/i2c-gpio.txt.
> v3 use speed-hz instead of udelay in dts binding.
>    condition the devicetree probe.
> v4 simplify private allocation.
> v5 condition module device table export for of.
> 
>  Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 +++++++
>  drivers/i2c/busses/i2c-gpio.c                      |  113 ++++++++++++++++----
>  2 files changed, 133 insertions(+), 20 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> 
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> new file mode 100644
> index 0000000..38ef4f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -0,0 +1,40 @@
> +GPIO-based I2C
> +
> +Required properties:
> +- compatible : should be "i2c-gpio".
> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> +Optional properties:
> +- sda-is-open-drain : present if SDA gpio is open-drain.
> +- scl-is-open-drain : present if SCL gpio is open-drain.
> +- scl-is-output-only : present if the output driver for SCL cannot be
> +	turned off. this will prevent clock stretching from working.
> +- speed-hz : SCL frequency.
> +- timeout : clock stretching timeout in milliseconds.
> +
> +Example:
> +
> +gpio0: starlet-gpio@0d8000c0 {
> +	compatible = "nintendo,starlet-gpio";
> +	reg = <0d8000c0 4>;
> +	gpio-controller;
> +	#gpio-cells = <2>;
> +};
> +
> +i2c-video {
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +	compatible = "i2c-gpio";
> +
> +	gpios = <&gpio0 10 0	/* SDA line */
> +		 &gpio0 11 0	/* SCL line */
> +		>;
> +	sda-is-open-drain;
> +	scl-is-open-drain;
> +	scl-is-output-only;
> +	speed-hz = <250000>;
> +
> +	audio-video-encoder {
> +		compatible = "nintendo,wii-ave-rvl";
> +		reg = <70>;
> +	};
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index d9aa9a6..6cc5f15 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,8 +14,14 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of_i2c.h>
>  
> -#include <asm/gpio.h>
> +struct i2c_gpio_private_data {
> +	struct i2c_adapter adap;
> +	struct i2c_algo_bit_data bit_data;
> +	struct i2c_gpio_platform_data pdata;
> +};
>  
>  /* Toggle SDA by changing the direction of the pin */
>  static void i2c_gpio_setsda_dir(void *data, int state)
> @@ -78,24 +84,80 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> +#ifdef CONFIG_OF
> +#include <linux/of_gpio.h>
> +
> +/* Check if devicetree nodes exist and build platform data */
> +static int i2c_gpio_of_probe(struct platform_device *pdev,
> +			     struct i2c_gpio_platform_data *pdata)
> +{
> +	struct device_node *np = pdev->dev.of_node;
> +	const __be32 *prop;
> +	int sda_pin, scl_pin;
> +	int len;
> +
> +	if (!np || of_gpio_count(np) < 2)
> +		return -ENXIO;

Would prefer to see different return code, most bus probe functions
tend to drop these without signalling to the user as they assume the
device was either never there or totally faulty.

> +	sda_pin = of_get_gpio_flags(np, 0, NULL);
> +	scl_pin = of_get_gpio_flags(np, 1, NULL);
> +	if (sda_pin < 0 || scl_pin < 0) {
> +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> +		       np->full_name, sda_pin, scl_pin);
> +		return -ENXIO;
> +	}

see same as above.

> +	pdata->sda_pin = sda_pin;
> +	pdata->scl_pin = scl_pin;
> +	prop = of_get_property(np, "sda-is-open-drain", NULL);
> +	if (prop)
> +		pdata->sda_is_open_drain = 1;
> +	prop = of_get_property(np, "scl-is-open-drain", NULL);
> +	if (prop)
> +		pdata->scl_is_open_drain = 1;
> +	prop = of_get_property(np, "scl-is-output-only", NULL);
> +	if (prop)
> +		pdata->scl_is_output_only = 1;
> +	prop = of_get_property(np, "speed-hz", &len);
> +	if (prop && len >= sizeof(__be32))
> +		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
> +	prop = of_get_property(np, "timeout", &len);
> +	if (prop && len >= sizeof(__be32))
> +		pdata->timeout = msecs_to_jiffies(be32_to_cpup(prop));
> +
> +	return 0;
> +}
> +#else
> +static int i2c_gpio_of_probe(struct platform_device *pdev,
> +			     struct i2c_gpio_platform_data *pdata)
> +{
> +	return -ENXIO;
> +}

might be valid here, however can we make this NULL if no support?

> +#endif
> +
>  static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  {
> +	struct i2c_gpio_private_data *priv;
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_algo_bit_data *bit_data;
>  	struct i2c_adapter *adap;
>  	int ret;
>  
> -	pdata = pdev->dev.platform_data;
> -	if (!pdata)
> -		return -ENXIO;
> +	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
> +	if (!priv)
> +		return -ENOMEM;
> +	adap = &priv->adap;
> +	bit_data = &priv->bit_data;
> +	pdata = &priv->pdata;
>  
> -	ret = -ENOMEM;
> -	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> -	if (!adap)
> -		goto err_alloc_adap;
> -	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
> -	if (!bit_data)
> -		goto err_alloc_bit_data;
> +	if (pdev->dev.platform_data) {
> +		*pdata = *(struct i2c_gpio_platform_data *)
> +			pdev->dev.platform_data;
> +	} else {
> +		ret = i2c_gpio_of_probe(pdev, pdata);
> +		if (ret)
> +			return ret;
> +	}
>  
>  	ret = gpio_request(pdata->sda_pin, "sda");
>  	if (ret)
> @@ -143,6 +205,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,13 +217,16 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_add_bus;
>  
> -	platform_set_drvdata(pdev, adap);
> +	platform_set_drvdata(pdev, priv);
>  
>  	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
>  		 pdata->sda_pin, pdata->scl_pin,
>  		 pdata->scl_is_output_only
>  		 ? ", no clock stretching" : "");
>  
> +	/* Now register all the child nodes */
> +	of_i2c_register_devices(adap);
> +
>  	return 0;
>  
>  err_add_bus:
> @@ -168,34 +234,41 @@ err_add_bus:
>  err_request_scl:
>  	gpio_free(pdata->sda_pin);
>  err_request_sda:
> -	kfree(bit_data);
> -err_alloc_bit_data:
> -	kfree(adap);
> -err_alloc_adap:
>  	return ret;
>  }
>  
>  static int __devexit i2c_gpio_remove(struct platform_device *pdev)
>  {
> +	struct i2c_gpio_private_data *priv;
>  	struct i2c_gpio_platform_data *pdata;
>  	struct i2c_adapter *adap;
>  
> -	adap = platform_get_drvdata(pdev);
> -	pdata = pdev->dev.platform_data;
> +	priv = platform_get_drvdata(pdev);
> +	adap = &priv->adap;
> +	pdata = &priv->pdata;
>  
>  	i2c_del_adapter(adap);
>  	gpio_free(pdata->scl_pin);
>  	gpio_free(pdata->sda_pin);
> -	kfree(adap->algo_data);
> -	kfree(adap);
>  
>  	return 0;
>  }
>  
> +#ifdef CONFIG_OF
> +static const struct of_device_id i2c_gpio_match[] = {
> +	{ .compatible = "i2c-gpio", },
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, i2c_gpio_match);
> +#else /* CONFIG_OF */
> +#define i2c_gpio_match NULL
> +#endif /* CONFIG_OF */
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table = i2c_gpio_match,
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= __devexit_p(i2c_gpio_remove),
> -- 
> 1.7.4
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/

Large Hadron Colada: A large Pina Colada that makes the universe disappear.

Re: [PATCH v5] i2c-gpio: add devicetree support

[Thomas Chou]

Hi Ben,

On 02/23/2011 09:12 AM, Ben Dooks wrote:
> On Mon, Feb 14, 2011 at 10:30:10AM +0800, Thomas Chou wrote:
>> From: Albert Herranz<albert_herranz@yahoo.es>
>>
>> This patch adds devicetree support to i2c-gpio driver. The allocation
>> of local data is integrated to a private structure, and use devm_*
>> helper for easy cleanup.
>>
>> It is base on an earlier patch for gc-linux from
>> Albert Herranz<albert_herranz@yahoo.es>.
>>
>> Signed-off-by: Thomas Chou<thomas@wytron.com.tw>
>> CC: Albert Herranz<albert_herranz@yahoo.es>
>> Acked-by: Haavard Skinnemoen<hskinnemoen@gmail.com>
>> ---
>> for v2.6.39
>> v2 move of nodes probing to a hook, which will be optimized out
>>       when devicetree is not used.
>>     use linux/gpio.h.
>>     move binding doc to i2c/i2c-gpio.txt.
>> v3 use speed-hz instead of udelay in dts binding.
>>     condition the devicetree probe.
>> v4 simplify private allocation.
>> v5 condition module device table export for of.
>>

>> +/* Check if devicetree nodes exist and build platform data */
>> +static int i2c_gpio_of_probe(struct platform_device *pdev,
>> +			     struct i2c_gpio_platform_data *pdata)
>> +{
>> +	struct device_node *np = pdev->dev.of_node;
>> +	const __be32 *prop;
>> +	int sda_pin, scl_pin;
>> +	int len;
>> +
>> +	if (!np || of_gpio_count(np)<  2)
>> +		return -ENXIO;
>
> Would prefer to see different return code, most bus probe functions
> tend to drop these without signalling to the user as they assume the
> device was either never there or totally faulty.
>
>> +	sda_pin = of_get_gpio_flags(np, 0, NULL);
>> +	scl_pin = of_get_gpio_flags(np, 1, NULL);
>> +	if (sda_pin<  0 || scl_pin<  0) {
>> +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
>> +		       np->full_name, sda_pin, scl_pin);
>> +		return -ENXIO;
>> +	}
>
> see same as above.
>
>> +	pdata->sda_pin = sda_pin;
>> +	pdata->scl_pin = scl_pin;
>> +	prop = of_get_property(np, "sda-is-open-drain", NULL);
>> +	if (prop)
>> +		pdata->sda_is_open_drain = 1;
>> +	prop = of_get_property(np, "scl-is-open-drain", NULL);
>> +	if (prop)
>> +		pdata->scl_is_open_drain = 1;
>> +	prop = of_get_property(np, "scl-is-output-only", NULL);
>> +	if (prop)
>> +		pdata->scl_is_output_only = 1;
>> +	prop = of_get_property(np, "speed-hz",&len);
>> +	if (prop&&  len>= sizeof(__be32))
>> +		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
>> +	prop = of_get_property(np, "timeout",&len);
>> +	if (prop&&  len>= sizeof(__be32))
>> +		pdata->timeout = msecs_to_jiffies(be32_to_cpup(prop));
>> +
>> +	return 0;
>> +}
>> +#else
>> +static int i2c_gpio_of_probe(struct platform_device *pdev,
>> +			     struct i2c_gpio_platform_data *pdata)
>> +{
>> +	return -ENXIO;
>> +}
>
> might be valid here, however can we make this NULL if no support?
>

Thanks. How about return NULL if no support, and return pdata if support?

- Thomas

[PATCH v6] i2c-gpio: add devicetree support

[Thomas Chou]

From: Albert Herranz <albert_herranz@yahoo.es>

This patch adds devicetree support to i2c-gpio driver. The allocation
of local data is integrated to a private structure, and use devm_*
helper for easy cleanup.

It is base on an earlier patch for gc-linux from
Albert Herranz <albert_herranz@yahoo.es>.

Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
CC: Albert Herranz <albert_herranz@yahoo.es>
Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>
Acked-by: Grant Likely <grant.likely@secretlab.ca>
---
for v2.6.39
v2 move of nodes probing to a hook, which will be optimized out
     when devicetree is not used.
   use linux/gpio.h.
   move binding doc to i2c/i2c-gpio.txt.
v3 use speed-hz instead of udelay in dts binding.
   condition the devicetree probe.
v4 simplify private allocation.
v5 condition module device table export for of.
v6 change return code if probe fail.

Hi Ben, this driver must get gpio pins from platform data or devicetree,
  so it should fail if there is no platform data and devicetree is not
  configured.

 Documentation/devicetree/bindings/i2c/i2c-gpio.txt |   40 +++++++
 drivers/i2c/busses/i2c-gpio.c                      |  113 ++++++++++++++++----
 2 files changed, 133 insertions(+), 20 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/i2c/i2c-gpio.txt

diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
new file mode 100644
index 0000000..38ef4f2
--- /dev/null
+++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
@@ -0,0 +1,40 @@
+GPIO-based I2C
+
+Required properties:
+- compatible : should be "i2c-gpio".
+- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
+Optional properties:
+- sda-is-open-drain : present if SDA gpio is open-drain.
+- scl-is-open-drain : present if SCL gpio is open-drain.
+- scl-is-output-only : present if the output driver for SCL cannot be
+	turned off. this will prevent clock stretching from working.
+- speed-hz : SCL frequency.
+- timeout : clock stretching timeout in milliseconds.
+
+Example:
+
+gpio0: starlet-gpio@0d8000c0 {
+	compatible = "nintendo,starlet-gpio";
+	reg = <0d8000c0 4>;
+	gpio-controller;
+	#gpio-cells = <2>;
+};
+
+i2c-video {
+	#address-cells = <1>;
+	#size-cells = <0>;
+	compatible = "i2c-gpio";
+
+	gpios = <&gpio0 10 0	/* SDA line */
+		 &gpio0 11 0	/* SCL line */
+		>;
+	sda-is-open-drain;
+	scl-is-open-drain;
+	scl-is-output-only;
+	speed-hz = <250000>;
+
+	audio-video-encoder {
+		compatible = "nintendo,wii-ave-rvl";
+		reg = <70>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index d9aa9a6..feac3e5 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,8 +14,14 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_i2c.h>
 
-#include <asm/gpio.h>
+struct i2c_gpio_private_data {
+	struct i2c_adapter adap;
+	struct i2c_algo_bit_data bit_data;
+	struct i2c_gpio_platform_data pdata;
+};
 
 /* Toggle SDA by changing the direction of the pin */
 static void i2c_gpio_setsda_dir(void *data, int state)
@@ -78,24 +84,80 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
+#ifdef CONFIG_OF
+#include <linux/of_gpio.h>
+
+/* Check if devicetree nodes exist and build platform data */
+static int i2c_gpio_of_probe(struct platform_device *pdev,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	struct device_node *np = pdev->dev.of_node;
+	const __be32 *prop;
+	int sda_pin, scl_pin;
+	int len;
+
+	if (!np || of_gpio_count(np) < 2)
+		return -ENODEV;
+
+	sda_pin = of_get_gpio_flags(np, 0, NULL);
+	scl_pin = of_get_gpio_flags(np, 1, NULL);
+	if (sda_pin < 0 || scl_pin < 0) {
+		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+		       np->full_name, sda_pin, scl_pin);
+		return -ENODEV;
+	}
+
+	pdata->sda_pin = sda_pin;
+	pdata->scl_pin = scl_pin;
+	prop = of_get_property(np, "sda-is-open-drain", NULL);
+	if (prop)
+		pdata->sda_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-open-drain", NULL);
+	if (prop)
+		pdata->scl_is_open_drain = 1;
+	prop = of_get_property(np, "scl-is-output-only", NULL);
+	if (prop)
+		pdata->scl_is_output_only = 1;
+	prop = of_get_property(np, "speed-hz", &len);
+	if (prop && len >= sizeof(__be32))
+		pdata->udelay = DIV_ROUND_UP(1000000, be32_to_cpup(prop) * 2);
+	prop = of_get_property(np, "timeout", &len);
+	if (prop && len >= sizeof(__be32))
+		pdata->timeout = msecs_to_jiffies(be32_to_cpup(prop));
+
+	return 0;
+}
+#else
+static int i2c_gpio_of_probe(struct platform_device *pdev,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	return -ENODEV;
+}
+#endif
+
 static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 {
+	struct i2c_gpio_private_data *priv;
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_algo_bit_data *bit_data;
 	struct i2c_adapter *adap;
 	int ret;
 
-	pdata = pdev->dev.platform_data;
-	if (!pdata)
-		return -ENXIO;
+	priv = devm_kzalloc(&pdev->dev, sizeof(*priv), GFP_KERNEL);
+	if (!priv)
+		return -ENOMEM;
+	adap = &priv->adap;
+	bit_data = &priv->bit_data;
+	pdata = &priv->pdata;
 
-	ret = -ENOMEM;
-	adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
-	if (!adap)
-		goto err_alloc_adap;
-	bit_data = kzalloc(sizeof(struct i2c_algo_bit_data), GFP_KERNEL);
-	if (!bit_data)
-		goto err_alloc_bit_data;
+	if (pdev->dev.platform_data) {
+		*pdata = *(struct i2c_gpio_platform_data *)
+			pdev->dev.platform_data;
+	} else {
+		ret = i2c_gpio_of_probe(pdev, pdata);
+		if (ret)
+			return ret;
+	}
 
 	ret = gpio_request(pdata->sda_pin, "sda");
 	if (ret)
@@ -143,6 +205,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,13 +217,16 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_add_bus;
 
-	platform_set_drvdata(pdev, adap);
+	platform_set_drvdata(pdev, priv);
 
 	dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
 		 pdata->sda_pin, pdata->scl_pin,
 		 pdata->scl_is_output_only
 		 ? ", no clock stretching" : "");
 
+	/* Now register all the child nodes */
+	of_i2c_register_devices(adap);
+
 	return 0;
 
 err_add_bus:
@@ -168,34 +234,41 @@ err_add_bus:
 err_request_scl:
 	gpio_free(pdata->sda_pin);
 err_request_sda:
-	kfree(bit_data);
-err_alloc_bit_data:
-	kfree(adap);
-err_alloc_adap:
 	return ret;
 }
 
 static int __devexit i2c_gpio_remove(struct platform_device *pdev)
 {
+	struct i2c_gpio_private_data *priv;
 	struct i2c_gpio_platform_data *pdata;
 	struct i2c_adapter *adap;
 
-	adap = platform_get_drvdata(pdev);
-	pdata = pdev->dev.platform_data;
+	priv = platform_get_drvdata(pdev);
+	adap = &priv->adap;
+	pdata = &priv->pdata;
 
 	i2c_del_adapter(adap);
 	gpio_free(pdata->scl_pin);
 	gpio_free(pdata->sda_pin);
-	kfree(adap->algo_data);
-	kfree(adap);
 
 	return 0;
 }
 
+#ifdef CONFIG_OF
+static const struct of_device_id i2c_gpio_match[] = {
+	{ .compatible = "i2c-gpio", },
+	{},
+};
+MODULE_DEVICE_TABLE(of, i2c_gpio_match);
+#else /* CONFIG_OF */
+#define i2c_gpio_match NULL
+#endif /* CONFIG_OF */
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table = i2c_gpio_match,
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.4

Re: [PATCH v6] i2c-gpio: add devicetree support

[Grant Likely]

On Thu, Feb 24, 2011 at 12:00:13PM +0800, Thomas Chou wrote:
> From: Albert Herranz <albert_herranz@yahoo.es>
> 
> This patch adds devicetree support to i2c-gpio driver. The allocation
> of local data is integrated to a private structure, and use devm_*
> helper for easy cleanup.
> 
> It is base on an earlier patch for gc-linux from
> Albert Herranz <albert_herranz@yahoo.es>.
> 
> Signed-off-by: Thomas Chou <thomas@wytron.com.tw>
> CC: Albert Herranz <albert_herranz@yahoo.es>
> Acked-by: Haavard Skinnemoen <hskinnemoen@gmail.com>
> Acked-by: Grant Likely <grant.likely@secretlab.ca>
> ---
> diff --git a/Documentation/devicetree/bindings/i2c/i2c-gpio.txt b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> new file mode 100644
> index 0000000..38ef4f2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/i2c/i2c-gpio.txt
> @@ -0,0 +1,40 @@
> +GPIO-based I2C
> +
> +Required properties:
> +- compatible : should be "i2c-gpio".
> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
> +Optional properties:
> +- sda-is-open-drain : present if SDA gpio is open-drain.
> +- scl-is-open-drain : present if SCL gpio is open-drain.
> +- scl-is-output-only : present if the output driver for SCL cannot be
> +	turned off. this will prevent clock stretching from working.
> +- speed-hz : SCL frequency.

Hi Thomas,

One nitpick; I just looked, and other i2c controllers are already
using 'clock-frequency' instead of 'speed-hz' for the speed of the
bus.  I'd like to see this patch use the same terminology.

Otherwise this looks good to me.

Thanks,
g.

Re: [PATCH v6] i2c-gpio: add devicetree support

[Thomas Chou]

Hi Grant,

On 02/25/2011 12:22 AM, Grant Likely wrote:
>> +Required properties:
>> +- compatible : should be "i2c-gpio".
>> +- gpios : should specify GPIOs used for SDA and SCL lines, in that order.
>> +Optional properties:
>> +- sda-is-open-drain : present if SDA gpio is open-drain.
>> +- scl-is-open-drain : present if SCL gpio is open-drain.
>> +- scl-is-output-only : present if the output driver for SCL cannot be
>> +	turned off. this will prevent clock stretching from working.
>> +- speed-hz : SCL frequency.
>
> Hi Thomas,
>
> One nitpick; I just looked, and other i2c controllers are already
> using 'clock-frequency' instead of 'speed-hz' for the speed of the
> bus.  I'd like to see this patch use the same terminology.

I studied 'clock-frequency' usage of existing i2c drivers before working 
on this patch.

i2c-ibm_iic.c, i2c-mpc.c, i2c-ocores.c : use as bus clock frequency 
input to the core.

i2c-cpm.c : use as i2c SCL output frequency.

Most other non-i2c drivers use clock-frequency as bus clock frequency 
input, too.

I think the SCL freq usage in i2c-cpm.c is confusing. So I would suggest 
we borrow 'speed-hz' from spi subsystem, which means the clock freq 
output on the peripheral bus.

Best regards,
Thomas

Re: [PATCH] i2c-gpio: add devicetree support

[Håvard Skinnemoen]

Hi,

On Mon, Jan 31, 2011 at 12:09 AM, Grant Likely
<grant.likely@secretlab.ca> wrote:
> On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen
> <hskinnemoen@gmail.com> wrote:
>> Hi,
>>
>> On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
>>> From: Albert Herranz <albert_herranz@yahoo.es>
>>>
>>> This patch is based on an earlier patch from Albert Herranz,
>>> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
>>> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
>>
>> That commit has a single-line description of which I don't understand
>> a single word (unless "wii" is what I think it is, which seems
>> likely). Could you please explain how that commit relates to this
>> patch?
>
> The URL got wrapped.  Try this one (assuming my mailer doesn't wrap it):
>
> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/9854eb78607c641ab5ae85bcbe3c9d14ac113733

Ok, that seems to be a _bit_ more related, but not that much. I'd
really prefer a patch description which can stand on its own.

>>> The dts binding is modified as Grant suggested. The of probing
>>> is merged inline instead of a separate file. It uses the newer
>>> of gpio probe.
>>
>> It seems like a terrible idea to merge firmware-specific code into the
>> driver. Is there are reason why of-based platforms can't just pass the
>> data they need in pdata like everyone else?
>
> Overall Thomas is doing the right thing here.  The driver data has to
> be decoded *somewhere*, but since that code is definitely
> driver-specific (as opposed to platform, subsystem, or arch specific)
> putting it into the driver is absolutely the right thing to do.  Quite
> a few drivers now do exactly this.

Ok, I'm convinced that this is the right thing to do.

> It is however generally a wise practice to limit the of-support code
> to a hook in the drivers probe hook, and as you point out, care must
> be taken to make sure both CONFIG_OF and !CONFIG_OF kernel builds
> work.
>
>>
>> Not saying that it necessarily _is_ a terrible idea, but I think the
>> reasoning behind it needs to be included in the patch description.
>
> Nah, he doesn't really need to defend this since it is a well
> established pattern.  device tree support is in core code now (see
> of_node an of_match_table in include/linux/device.h), and other
> drivers do exactly this.

Well, perhaps you're right, but I still think the patch description is
a bit on the thin side. In particular, terms like "as Grant suggested"
isn't very helpful for people like me who don't know what you
suggested (even though I'm sure it was a good suggestion).

I think the patch lacks a good description of what's being changed and
why. The references may be nice to have as a supplement to that, but
describing things entirely in terms of some unknown e-mail discussion
that happened earlier is not very helpful for people who want to
figure out what's going on a couple of months or years from now.

Havard

Re: [PATCH] i2c-gpio: add devicetree support

[Grant Likely]

On Mon, Jan 31, 2011 at 02:35:31PM -0800, Håvard Skinnemoen wrote:
> Hi,
> 
> On Mon, Jan 31, 2011 at 12:09 AM, Grant Likely
> <grant.likely@secretlab.ca> wrote:
> > On Sun, Jan 30, 2011 at 8:26 PM, Håvard Skinnemoen
> > <hskinnemoen@gmail.com> wrote:
> >> Hi,
> >>
> >> On Sun, Jan 30, 2011 at 7:56 AM, Thomas Chou <thomas@wytron.com.tw> wrote:
> >>> From: Albert Herranz <albert_herranz@yahoo.es>
> >>>
> >>> This patch is based on an earlier patch from Albert Herranz,
> >>> http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/
> >>> 9854eb78607c641ab5ae85bcbe3c9d14ac113733
> >>
> >> That commit has a single-line description of which I don't understand
> >> a single word (unless "wii" is what I think it is, which seems
> >> likely). Could you please explain how that commit relates to this
> >> patch?
> >
> > The URL got wrapped.  Try this one (assuming my mailer doesn't wrap it):
> >
> > http://git.infradead.org/users/herraa1/gc-linux-2.6.git/commit/9854eb78607c641ab5ae85bcbe3c9d14ac113733
> 
> Ok, that seems to be a _bit_ more related, but not that much. I'd
> really prefer a patch description which can stand on its own.
> 
[...]
>
> >> Not saying that it necessarily _is_ a terrible idea, but I think the
> >> reasoning behind it needs to be included in the patch description.
> >
> > Nah, he doesn't really need to defend this since it is a well
> > established pattern.  device tree support is in core code now (see
> > of_node an of_match_table in include/linux/device.h), and other
> > drivers do exactly this.
> 
> Well, perhaps you're right, but I still think the patch description is
> a bit on the thin side. In particular, terms like "as Grant suggested"
> isn't very helpful for people like me who don't know what you
> suggested (even though I'm sure it was a good suggestion).
> 
> I think the patch lacks a good description of what's being changed and
> why. The references may be nice to have as a supplement to that, but
> describing things entirely in terms of some unknown e-mail discussion
> that happened earlier is not very helpful for people who want to
> figure out what's going on a couple of months or years from now.

No arguments from me on those points.

g.