2011-02-03 02:26:18

by Thomas Chou

[permalink] [raw]
Subject: [PATCH] i2c-gpio: add devicetree support

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 <[email protected]>
Signed-off-by: Thomas Chou <[email protected]>
---
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


2011-02-03 04:24:09

by Håvard Skinnemoen

[permalink] [raw]
Subject: Re: [PATCH] i2c-gpio: add devicetree support

On Wed, Feb 2, 2011 at 6:26 PM, Thomas Chou <[email protected]> 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 <[email protected]>
> Signed-off-by: Thomas Chou <[email protected]>

Acked-by: Haavard Skinnemoen <[email protected]>

2011-02-03 18:07:13

by Grant Likely

[permalink] [raw]
Subject: Re: [PATCH] i2c-gpio: add devicetree support

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 <[email protected]>
> Signed-off-by: Thomas Chou <[email protected]>
> ---
> 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
>

2011-02-10 02:26:53

by Thomas Chou

[permalink] [raw]
Subject: [PATCH v4] i2c-gpio: add devicetree support

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 <[email protected]>.

Signed-off-by: Thomas Chou <[email protected]>
Acked-by: Haavard Skinnemoen <[email protected]>
---
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

2011-02-14 02:28:49

by Thomas Chou

[permalink] [raw]
Subject: [PATCH v5] i2c-gpio: add devicetree support

From: Albert Herranz <[email protected]>

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 <[email protected]>.

Signed-off-by: Thomas Chou <[email protected]>
CC: Albert Herranz <[email protected]>
Acked-by: Haavard Skinnemoen <[email protected]>
---
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

2011-02-16 05:50:04

by Grant Likely

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

On Mon, Feb 14, 2011 at 10:30:10AM +0800, Thomas Chou wrote:
> From: Albert Herranz <[email protected]>
>
> 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 <[email protected]>.
>
> Signed-off-by: Thomas Chou <[email protected]>
> CC: Albert Herranz <[email protected]>
> Acked-by: Haavard Skinnemoen <[email protected]>

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 <[email protected]>

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
>

2011-02-16 06:10:06

by Thomas Chou

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

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<[email protected]>
>>
>> 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<[email protected]>.
>>
>> Signed-off-by: Thomas Chou<[email protected]>
>> CC: Albert Herranz<[email protected]>
>> Acked-by: Haavard Skinnemoen<[email protected]>
>
> 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<[email protected]>
>
> 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

2011-02-23 01:12:41

by Ben Dooks

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

On Mon, Feb 14, 2011 at 10:30:10AM +0800, Thomas Chou wrote:
> From: Albert Herranz <[email protected]>
>
> 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 <[email protected]>.
>
> Signed-off-by: Thomas Chou <[email protected]>
> CC: Albert Herranz <[email protected]>
> Acked-by: Haavard Skinnemoen <[email protected]>
> ---
> 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 [email protected]
> More majordomo info at http://vger.kernel.org/majordomo-info.html

--
Ben Dooks, [email protected], http://www.fluff.org/ben/

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

2011-02-23 13:18:20

by Thomas Chou

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

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<[email protected]>
>>
>> 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<[email protected]>.
>>
>> Signed-off-by: Thomas Chou<[email protected]>
>> CC: Albert Herranz<[email protected]>
>> Acked-by: Haavard Skinnemoen<[email protected]>
>> ---
>> 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

2011-02-24 04:00:23

by Thomas Chou

[permalink] [raw]
Subject: [PATCH v6] i2c-gpio: add devicetree support

From: Albert Herranz <[email protected]>

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 <[email protected]>.

Signed-off-by: Thomas Chou <[email protected]>
CC: Albert Herranz <[email protected]>
Acked-by: Haavard Skinnemoen <[email protected]>
Acked-by: Grant Likely <[email protected]>
---
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

2011-02-24 16:22:15

by Grant Likely

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

On Thu, Feb 24, 2011 at 12:00:13PM +0800, Thomas Chou wrote:
> From: Albert Herranz <[email protected]>
>
> 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 <[email protected]>.
>
> Signed-off-by: Thomas Chou <[email protected]>
> CC: Albert Herranz <[email protected]>
> Acked-by: Haavard Skinnemoen <[email protected]>
> Acked-by: Grant Likely <[email protected]>
> ---
> 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.

2011-02-25 02:05:08

by Thomas Chou

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

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