* [PATCH 1/4] i2c/gpio-i2c add: add DT support
@ 2012-02-05 10:38 Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 2/4] ARM: at91s: sam9g20 add i2c " Jean-Christophe PLAGNIOL-VILLARD
` (4 more replies)
0 siblings, 5 replies; 12+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-02-05 10:38 UTC (permalink / raw)
To: linux-arm-kernel
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: linux-i2c at vger.kernel.org
Cc: devicetree-discuss at lists.ozlabs.org
---
.../devicetree/bindings/gpio/gpio_i2c.txt | 33 ++++++++++++
drivers/i2c/busses/i2c-gpio.c | 55 +++++++++++++++++++-
2 files changed, 86 insertions(+), 2 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_i2c.txt
diff --git a/Documentation/devicetree/bindings/gpio/gpio_i2c.txt b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
new file mode 100644
index 0000000..15f288da
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
@@ -0,0 +1,33 @@
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+ - compatible = "gpio-i2c";
+ - gpios: sda and scl gpio
+
+
+Optional properties:
+ - gpio-i2c,sda_is_open_drain: sda as open drain
+ - gpio-i2c,scl_is_open_drain: scl as open drain
+ - gpio-i2c,scl_is_output_only: scl as output only
+ - udelay: half clock cycle time in us (may depend on each platform)
+ - timeout: timeout to get data
+
+
+Example nodes:
+
+i2c-gpio at 0 {
+ compatible = "gpio-i2c";
+ gpios = <&pioA 23 0 /* sda */
+ &pioA 24 0 /* scl */
+ >;
+ gpio-i2c,sda_is_open_drain;
+ gpio-i2c,scl_is_open_drain;
+ udelay = <2>; /* ~100 kHz */
+ #address-cells = <1>;
+ #size-cells = <0>;
+
+ rv3029c2 at 56 {
+ compatible = "rv3029c2";
+ reg = <0x56>;
+ };
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index a651779..6b5d794 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,6 +14,8 @@
#include <linux/module.h>
#include <linux/slab.h>
#include <linux/platform_device.h>
+#include <linux/of_gpio.h>
+#include <linux/of_i2c.h>
#include <asm/gpio.h>
@@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
return gpio_get_value(pdata->scl_pin);
}
+static int of_i2c_gpio_probe(struct device_node *np,
+ struct i2c_gpio_platform_data *pdata)
+{
+ u32 reg;
+
+ pdata->sda_pin = of_get_gpio(np, 0);
+ pdata->scl_pin = of_get_gpio(np, 1);
+
+ if (of_property_read_u32(np, "udelay", ®))
+ pdata->udelay = reg;
+
+ if (of_property_read_u32(np, "timeout", ®))
+ pdata->timeout = reg;
+
+ pdata->sda_is_open_drain =
+ !!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
+ pdata->scl_is_open_drain =
+ !!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
+ pdata->scl_is_output_only =
+ !!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
+
+ return 0;
+}
+
static int __devinit i2c_gpio_probe(struct platform_device *pdev)
{
struct i2c_gpio_platform_data *pdata;
struct i2c_algo_bit_data *bit_data;
struct i2c_adapter *adap;
int ret;
+ int len = sizeof(struct i2c_gpio_platform_data);
- pdata = pdev->dev.platform_data;
+ pdata = kzalloc(len, GFP_KERNEL);
if (!pdata)
- return -ENXIO;
+ return -ENOMEM;
+
+ if (pdev->dev.of_node) {
+ of_i2c_gpio_probe(pdev->dev.of_node, pdata);
+ } else {
+ if (!pdev->dev.platform_data) {
+ ret = -ENXIO;
+ goto err_alloc_adap;
+ }
+ memcpy(pdata, pdev->dev.platform_data, len);
+ }
ret = -ENOMEM;
adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
@@ -143,6 +180,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
adap->algo_data = bit_data;
adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
adap->dev.parent = &pdev->dev;
+ adap->dev.of_node = pdev->dev.of_node;
/*
* If "dev->id" is negative we consider it as zero.
@@ -154,6 +192,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
if (ret)
goto err_add_bus;
+ of_i2c_register_devices(adap);
+
platform_set_drvdata(pdev, adap);
dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
@@ -172,6 +212,7 @@ err_request_sda:
err_alloc_bit_data:
kfree(adap);
err_alloc_adap:
+ kfree(pdata);
return ret;
}
@@ -192,10 +233,20 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
return 0;
}
+#if defined(CONFIG_OF)
+static const struct of_device_id gpio_i2c_dt_ids[] = {
+ { .compatible = "gpio-i2c", },
+ { /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, gpio_i2c_dt_ids);
+#endif
+
static struct platform_driver i2c_gpio_driver = {
.driver = {
.name = "i2c-gpio",
.owner = THIS_MODULE,
+ .of_match_table = of_match_ptr(gpio_i2c_dt_ids),
},
.probe = i2c_gpio_probe,
.remove = __devexit_p(i2c_gpio_remove),
--
1.7.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 2/4] ARM: at91s: sam9g20 add i2c DT support
2012-02-05 10:38 [PATCH 1/4] i2c/gpio-i2c add: add DT support Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-05 10:38 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 3/4] ARM: at91: usb_a9g20 add DT i2c support Jean-Christophe PLAGNIOL-VILLARD
` (3 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-02-05 10:38 UTC (permalink / raw)
To: linux-arm-kernel
for now on use gpio-i2c driver on the same pin as the hardware IP
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: devicetree-discuss at lists.ozlabs.org
---
arch/arm/boot/dts/at91sam9g20.dtsi | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/at91sam9g20.dtsi b/arch/arm/boot/dts/at91sam9g20.dtsi
index f8f4627..d52e827 100644
--- a/arch/arm/boot/dts/at91sam9g20.dtsi
+++ b/arch/arm/boot/dts/at91sam9g20.dtsi
@@ -220,6 +220,19 @@
status = "disabled";
};
+ i2c-gpio at 0 {
+ compatible = "gpio-i2c";
+ gpios = <&pioA 23 0 /* sda */
+ &pioA 24 0 /* scl */
+ >;
+ gpio-i2c,sda_is_open_drain;
+ gpio-i2c,scl_is_open_drain;
+ udelay = <2>; /* ~100 kHz */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
};
};
};
--
1.7.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 3/4] ARM: at91: usb_a9g20 add DT i2c support
2012-02-05 10:38 [PATCH 1/4] i2c/gpio-i2c add: add DT support Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 2/4] ARM: at91s: sam9g20 add i2c " Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-05 10:38 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 4/4] ARM: at91: sam9g45 add i2c DT support Jean-Christophe PLAGNIOL-VILLARD
` (2 subsequent siblings)
4 siblings, 0 replies; 12+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-02-05 10:38 UTC (permalink / raw)
To: linux-arm-kernel
use gpio-i2c and enable rv3029 RTC
enable the in the sam9g20 defconfig
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: devicetree-discuss at lists.ozlabs.org
---
arch/arm/boot/dts/usb_a9g20.dts | 9 +++++++++
arch/arm/configs/at91sam9g20_defconfig | 3 +++
2 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/usb_a9g20.dts b/arch/arm/boot/dts/usb_a9g20.dts
index 11be3e9..9d84374 100644
--- a/arch/arm/boot/dts/usb_a9g20.dts
+++ b/arch/arm/boot/dts/usb_a9g20.dts
@@ -121,6 +121,15 @@
};
+ i2c-gpio at 0 {
+ status = "okay";
+
+ rv3029c2 at 56 {
+ compatible = "rv3029c2";
+ reg = <0x56>;
+ };
+ };
+
};
};
};
diff --git a/arch/arm/configs/at91sam9g20_defconfig b/arch/arm/configs/at91sam9g20_defconfig
index 9123568..994d331 100644
--- a/arch/arm/configs/at91sam9g20_defconfig
+++ b/arch/arm/configs/at91sam9g20_defconfig
@@ -74,6 +74,8 @@ CONFIG_LEGACY_PTY_COUNT=16
CONFIG_SERIAL_ATMEL=y
CONFIG_SERIAL_ATMEL_CONSOLE=y
CONFIG_HW_RANDOM=y
+CONFIG_I2C=y
+CONFIG_I2C_GPIO=y
CONFIG_SPI=y
CONFIG_SPI_ATMEL=y
CONFIG_SPI_SPIDEV=y
@@ -105,6 +107,7 @@ CONFIG_LEDS_TRIGGERS=y
CONFIG_LEDS_TRIGGER_TIMER=y
CONFIG_LEDS_TRIGGER_HEARTBEAT=y
CONFIG_RTC_CLASS=y
+CONFIG_RTC_DRV_RV3029C2=y
CONFIG_RTC_DRV_AT91SAM9=y
CONFIG_EXT2_FS=y
CONFIG_MSDOS_FS=y
--
1.7.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 4/4] ARM: at91: sam9g45 add i2c DT support
2012-02-05 10:38 [PATCH 1/4] i2c/gpio-i2c add: add DT support Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 2/4] ARM: at91s: sam9g20 add i2c " Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 3/4] ARM: at91: usb_a9g20 add DT i2c support Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-05 10:38 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-06 16:09 ` [PATCH 1/4] i2c/gpio-i2c add: add " Mark Brown
2012-02-06 18:38 ` Karol Lewandowski
4 siblings, 0 replies; 12+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-02-05 10:38 UTC (permalink / raw)
To: linux-arm-kernel
for now on use gpio-i2c driver on the same pin as the hardware IP
Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
Cc: devicetree-discuss at lists.ozlabs.org
---
arch/arm/boot/dts/at91sam9g45.dtsi | 13 +++++++++++++
1 files changed, 13 insertions(+), 0 deletions(-)
diff --git a/arch/arm/boot/dts/at91sam9g45.dtsi b/arch/arm/boot/dts/at91sam9g45.dtsi
index a12ebc4..951b2c8 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -211,6 +211,19 @@
status = "disabled";
};
+ i2c-gpio at 0 {
+ compatible = "gpio-i2c";
+ gpios = <&pioA 20 0 /* sda */
+ &pioA 21 0 /* scl */
+ >;
+ gpio-i2c,sda_is_open_drain;
+ gpio-i2c,scl_is_open_drain;
+ udelay = <5>; /* ~100 kHz */
+ #address-cells = <1>;
+ #size-cells = <0>;
+ status = "disabled";
+ };
+
};
};
};
--
1.7.7
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-05 10:38 [PATCH 1/4] i2c/gpio-i2c add: add DT support Jean-Christophe PLAGNIOL-VILLARD
` (2 preceding siblings ...)
2012-02-05 10:38 ` [PATCH 4/4] ARM: at91: sam9g45 add i2c DT support Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-06 16:09 ` Mark Brown
2012-02-07 2:56 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-06 18:38 ` Karol Lewandowski
4 siblings, 1 reply; 12+ messages in thread
From: Mark Brown @ 2012-02-06 16:09 UTC (permalink / raw)
To: linux-arm-kernel
On Sun, Feb 05, 2012 at 11:38:53AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> + - udelay: half clock cycle time in us (may depend on each platform)
> + udelay = <2>; /* ~100 kHz */
Why not specify this in kHz and do the conversion in the driver? It
seems a more intuitive thing to be specifying. I appreciate that the
platform data used udelay but it seems an entirely unintuitive thing
from a user point of view even if it's what the implementation wants.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-05 10:38 [PATCH 1/4] i2c/gpio-i2c add: add DT support Jean-Christophe PLAGNIOL-VILLARD
` (3 preceding siblings ...)
2012-02-06 16:09 ` [PATCH 1/4] i2c/gpio-i2c add: add " Mark Brown
@ 2012-02-06 18:38 ` Karol Lewandowski
2012-02-06 19:15 ` Jean Delvare
` (2 more replies)
4 siblings, 3 replies; 12+ messages in thread
From: Karol Lewandowski @ 2012-02-06 18:38 UTC (permalink / raw)
To: linux-arm-kernel
On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
Hi!
> +Device-Tree bindings for i2c gpio driver
> +
> +Required properties:
> + - compatible = "gpio-i2c";
Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of patch
adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio - could we
stick to that name?
[1] https://lkml.org/lkml/2011/2/23/584
> + - gpios: sda and scl gpio
> +
> +
> +Optional properties:
> + - gpio-i2c,sda_is_open_drain: sda as open drain
> + - gpio-i2c,scl_is_open_drain: scl as open drain
> + - gpio-i2c,scl_is_output_only: scl as output only
Most of DT-properties I've seen used hyphen, not underscore. Could we
stick to that convention?
(Nitpick: I think that "is" in property names is redundant too.)
> + - udelay: half clock cycle time in us (may depend on each platform)
Could we use "clock-frequency" as Grant have suggested during review of
previous patch to i2c-gpio?
https://lkml.org/lkml/2011/2/24/220
> + - timeout: timeout to get data
> +
> +
> +Example nodes:
> +
> +i2c-gpio at 0 {
> + compatible = "gpio-i2c";
> + gpios =<&pioA 23 0 /* sda */
> + &pioA 24 0 /* scl */
> + >;
> + gpio-i2c,sda_is_open_drain;
> + gpio-i2c,scl_is_open_drain;
> + udelay =<2>; /* ~100 kHz */
> + #address-cells =<1>;
> + #size-cells =<0>;
> +
> + rv3029c2 at 56 {
> + compatible = "rv3029c2";
> + reg =<0x56>;
> + };
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index a651779..6b5d794 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,6 +14,8 @@
> #include<linux/module.h>
> #include<linux/slab.h>
> #include<linux/platform_device.h>
> +#include<linux/of_gpio.h>
> +#include<linux/of_i2c.h>
>
> #include<asm/gpio.h>
>
> @@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
> return gpio_get_value(pdata->scl_pin);
> }
>
> +static int of_i2c_gpio_probe(struct device_node *np,
> + struct i2c_gpio_platform_data *pdata)
> +{
> + u32 reg;
> +
if (of_gpio_count(np) < 2)
return -EINVAL;
> + pdata->sda_pin = of_get_gpio(np, 0);
> + pdata->scl_pin = of_get_gpio(np, 1);
if (pdata->sda_pin < 0 || pdata->scl_pin < 0)
return -EINVAL;
> +
> + if (of_property_read_u32(np, "udelay",®))
> + pdata->udelay = reg;
> +
> + if (of_property_read_u32(np, "timeout",®))
> + pdata->timeout = reg;
> +
> + pdata->sda_is_open_drain =
> + !!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
> + pdata->scl_is_open_drain =
> + !!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
> + pdata->scl_is_output_only =
> + !!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
> +
> + return 0;
> +}
> +
> static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> {
> struct i2c_gpio_platform_data *pdata;
> struct i2c_algo_bit_data *bit_data;
> struct i2c_adapter *adap;
> int ret;
> + int len = sizeof(struct i2c_gpio_platform_data);
>
> - pdata = pdev->dev.platform_data;
> + pdata = kzalloc(len, GFP_KERNEL);
Could you also take into account Grant's comment about private/platform
data?
https://lkml.org/lkml/2011/2/3/221
> if (!pdata)
> - return -ENXIO;
> + return -ENOMEM;
> +
> + if (pdev->dev.of_node) {
> + of_i2c_gpio_probe(pdev->dev.of_node, pdata);
Above might fail if configuration is corrupted.
> + } else {
> + if (!pdev->dev.platform_data) {
> + ret = -ENXIO;
> + goto err_alloc_adap;
> + }
> + memcpy(pdata, pdev->dev.platform_data, len);
> + }
>
> ret = -ENOMEM;
> adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> @@ -143,6 +180,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> adap->algo_data = bit_data;
> adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> adap->dev.parent =&pdev->dev;
> + adap->dev.of_node = pdev->dev.of_node;
>
> /*
> * If "dev->id" is negative we consider it as zero.
> @@ -154,6 +192,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> if (ret)
> goto err_add_bus;
>
> + of_i2c_register_devices(adap);
> +
> platform_set_drvdata(pdev, adap);
>
> dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
> @@ -172,6 +212,7 @@ err_request_sda:
> err_alloc_bit_data:
> kfree(adap);
> err_alloc_adap:
> + kfree(pdata);
> return ret;
> }
>
> @@ -192,10 +233,20 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
> return 0;
> }
>
> +#if defined(CONFIG_OF)
> +static const struct of_device_id gpio_i2c_dt_ids[] = {
> + { .compatible = "gpio-i2c", },
There seem to be no good reason to make DT-compatible string different
from driver's name that's already in use:
> static struct platform_driver i2c_gpio_driver = {
> .driver = {
> .name = "i2c-gpio",
Regards,
--
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-06 18:38 ` Karol Lewandowski
@ 2012-02-06 19:15 ` Jean Delvare
2012-02-07 3:25 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-13 23:14 ` Ben Dooks
2 siblings, 0 replies; 12+ messages in thread
From: Jean Delvare @ 2012-02-06 19:15 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, 06 Feb 2012 19:38:29 +0100, Karol Lewandowski wrote:
> On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> Hi!
>
> > +Device-Tree bindings for i2c gpio driver
> > +
> > +Required properties:
> > + - compatible = "gpio-i2c";
>
> Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of patch
> adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio - could we
> stick to that name?
>
> [1] https://lkml.org/lkml/2011/2/23/584
Yes, please. The gpio subsystem normalized its driver names to gpio-*,
so we shouldn't use that anywhere else to avoid confusion.
--
Jean Delvare
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-06 16:09 ` [PATCH 1/4] i2c/gpio-i2c add: add " Mark Brown
@ 2012-02-07 2:56 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-07 11:25 ` Mark Brown
0 siblings, 1 reply; 12+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-02-07 2:56 UTC (permalink / raw)
To: linux-arm-kernel
On 16:09 Mon 06 Feb , Mark Brown wrote:
> On Sun, Feb 05, 2012 at 11:38:53AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> > + - udelay: half clock cycle time in us (may depend on each platform)
>
> > + udelay = <2>; /* ~100 kHz */
>
> Why not specify this in kHz and do the conversion in the driver? It
> seems a more intuitive thing to be specifying. I appreciate that the
> platform data used udelay but it seems an entirely unintuitive thing
> from a user point of view even if it's what the implementation wants.
because it's not accurate and on some platform you need to adapt it so we keep
the udelay
as example due to latency when changing state of the gpio as we may access it
via i2c or internal bus latency
Best Regards,
J.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-06 18:38 ` Karol Lewandowski
2012-02-06 19:15 ` Jean Delvare
@ 2012-02-07 3:25 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-07 15:35 ` Karol Lewandowski
2012-02-13 23:14 ` Ben Dooks
2 siblings, 1 reply; 12+ messages in thread
From: Jean-Christophe PLAGNIOL-VILLARD @ 2012-02-07 3:25 UTC (permalink / raw)
To: linux-arm-kernel
On 19:38 Mon 06 Feb , Karol Lewandowski wrote:
> On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> Hi!
>
> >+Device-Tree bindings for i2c gpio driver
> >+
> >+Required properties:
> >+ - compatible = "gpio-i2c";
>
> Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of
> patch adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio -
> could we stick to that name?
>
> [1] https://lkml.org/lkml/2011/2/23/584
>
> >+ - gpios: sda and scl gpio
> >+
> >+
> >+Optional properties:
> >+ - gpio-i2c,sda_is_open_drain: sda as open drain
> >+ - gpio-i2c,scl_is_open_drain: scl as open drain
> >+ - gpio-i2c,scl_is_output_only: scl as output only
>
> Most of DT-properties I've seen used hyphen, not underscore. Could
> we stick to that convention?
>
> (Nitpick: I think that "is" in property names is redundant too.)
>
> >+ - udelay: half clock cycle time in us (may depend on each platform)
>
> Could we use "clock-frequency" as Grant have suggested during review
> of previous patch to i2c-gpio?
as exaplained no as for gpio the delay is platform dependent
>
> https://lkml.org/lkml/2011/2/24/220
>
> >+ - timeout: timeout to get data
> >+
> >+
> >+Example nodes:
> >+
> >+i2c-gpio at 0 {
> >+ compatible = "gpio-i2c";
> >+ gpios =<&pioA 23 0 /* sda */
> >+ &pioA 24 0 /* scl */
> >+ >;
> >+ gpio-i2c,sda_is_open_drain;
> >+ gpio-i2c,scl_is_open_drain;
> >+ udelay =<2>; /* ~100 kHz */
> >+ #address-cells =<1>;
> >+ #size-cells =<0>;
> >+
> >+ rv3029c2 at 56 {
> >+ compatible = "rv3029c2";
> >+ reg =<0x56>;
> >+ };
> >+};
> >diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> >index a651779..6b5d794 100644
> >--- a/drivers/i2c/busses/i2c-gpio.c
> >+++ b/drivers/i2c/busses/i2c-gpio.c
> >@@ -14,6 +14,8 @@
> > #include<linux/module.h>
> > #include<linux/slab.h>
> > #include<linux/platform_device.h>
> >+#include<linux/of_gpio.h>
> >+#include<linux/of_i2c.h>
> >
> > #include<asm/gpio.h>
> >
> >@@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
> > return gpio_get_value(pdata->scl_pin);
> > }
> >
> >+static int of_i2c_gpio_probe(struct device_node *np,
> >+ struct i2c_gpio_platform_data *pdata)
> >+{
> >+ u32 reg;
> >+
>
> if (of_gpio_count(np) < 2)
> return -EINVAL;
ok
>
> >+ pdata->sda_pin = of_get_gpio(np, 0);
> >+ pdata->scl_pin = of_get_gpio(np, 1);
>
> if (pdata->sda_pin < 0 || pdata->scl_pin < 0)
> return -EINVAL;
>
> >+
> >+ if (of_property_read_u32(np, "udelay",®))
> >+ pdata->udelay = reg;
> >+
> >+ if (of_property_read_u32(np, "timeout",®))
> >+ pdata->timeout = reg;
> >+
> >+ pdata->sda_is_open_drain =
> >+ !!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
> >+ pdata->scl_is_open_drain =
> >+ !!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
> >+ pdata->scl_is_output_only =
> >+ !!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
> >+
> >+ return 0;
> >+}
> >+
> > static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> > {
> > struct i2c_gpio_platform_data *pdata;
> > struct i2c_algo_bit_data *bit_data;
> > struct i2c_adapter *adap;
> > int ret;
> >+ int len = sizeof(struct i2c_gpio_platform_data);
> >
> >- pdata = pdev->dev.platform_data;
> >+ pdata = kzalloc(len, GFP_KERNEL);
>
> Could you also take into account Grant's comment about
> private/platform data?
>
> https://lkml.org/lkml/2011/2/3/221
>
> > if (!pdata)
> >- return -ENXIO;
> >+ return -ENOMEM;
> >+
> >+ if (pdev->dev.of_node) {
> >+ of_i2c_gpio_probe(pdev->dev.of_node, pdata);
>
> Above might fail if configuration is corrupted.
>
> >+ } else {
> >+ if (!pdev->dev.platform_data) {
> >+ ret = -ENXIO;
> >+ goto err_alloc_adap;
> >+ }
> >+ memcpy(pdata, pdev->dev.platform_data, len);
> >+ }
> >
> > ret = -ENOMEM;
> > adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
> >@@ -143,6 +180,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> > adap->algo_data = bit_data;
> > adap->class = I2C_CLASS_HWMON | I2C_CLASS_SPD;
> > adap->dev.parent =&pdev->dev;
> >+ adap->dev.of_node = pdev->dev.of_node;
> >
> > /*
> > * If "dev->id" is negative we consider it as zero.
> >@@ -154,6 +192,8 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
> > if (ret)
> > goto err_add_bus;
> >
> >+ of_i2c_register_devices(adap);
> >+
> > platform_set_drvdata(pdev, adap);
> >
> > dev_info(&pdev->dev, "using pins %u (SDA) and %u (SCL%s)\n",
> >@@ -172,6 +212,7 @@ err_request_sda:
> > err_alloc_bit_data:
> > kfree(adap);
> > err_alloc_adap:
> >+ kfree(pdata);
> > return ret;
> > }
> >
> >@@ -192,10 +233,20 @@ static int __devexit i2c_gpio_remove(struct platform_device *pdev)
> > return 0;
> > }
> >
> >+#if defined(CONFIG_OF)
> >+static const struct of_device_id gpio_i2c_dt_ids[] = {
> >+ { .compatible = "gpio-i2c", },
>
> There seem to be no good reason to make DT-compatible string
> different from driver's name that's already in use:
>
> > static struct platform_driver i2c_gpio_driver = {
> > .driver = {
> > .name = "i2c-gpio",
>
> Regards,
> --
> Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-07 2:56 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-07 11:25 ` Mark Brown
0 siblings, 0 replies; 12+ messages in thread
From: Mark Brown @ 2012-02-07 11:25 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Feb 07, 2012 at 03:56:24AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:09 Mon 06 Feb , Mark Brown wrote:
> > > + - udelay: half clock cycle time in us (may depend on each platform)
> > > + udelay = <2>; /* ~100 kHz */
> > Why not specify this in kHz and do the conversion in the driver? It
> > seems a more intuitive thing to be specifying. I appreciate that the
> > platform data used udelay but it seems an entirely unintuitive thing
> > from a user point of view even if it's what the implementation wants.
> because it's not accurate and on some platform you need to adapt it so we keep
> the udelay
Then you should clarify that in the documentation, it's not the cycle
time but the delay between GPIO operations which isn't quite the same
thing.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20120207/7c9bba3f/attachment.sig>
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-07 3:25 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-07 15:35 ` Karol Lewandowski
0 siblings, 0 replies; 12+ messages in thread
From: Karol Lewandowski @ 2012-02-07 15:35 UTC (permalink / raw)
To: linux-arm-kernel
On 07.02.2012 04:25, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 19:38 Mon 06 Feb , Karol Lewandowski wrote:
>>> + - udelay: half clock cycle time in us (may depend on each platform)
>>
>> Could we use "clock-frequency" as Grant have suggested during review
>> of previous patch to i2c-gpio?
> as exaplained no as for gpio the delay is platform dependent
Following sniplet from i2c-gpio suggests that in some cases both udelay
and timeout can be calculated given presence of other parameters:
static int __devinit i2c_gpio_probe(struct platform_device *pdev)
...
if (pdata->udelay)
bit_data->udelay = pdata->udelay;
else if (pdata->scl_is_output_only)
bit_data->udelay = 50; /* 10 kHz */
else
bit_data->udelay = 5; /* 100 kHz */
if (pdata->timeout)
bit_data->timeout = pdata->timeout;
else
bit_data->timeout = HZ / 10; /* 100 ms */
However, I find it more troubling that driver manually grabs parameters
that are specific to i2c-algo-bit (timeout, udelay).
Thus, I have feeling that it should be generically addressed there. What
do you think?
>>
>> https://lkml.org/lkml/2011/2/24/220
>>
>>> + - timeout: timeout to get data
>>> +
>>> +
>>> +Example nodes:
>>> +
>>> +i2c-gpio at 0 {
>>> + compatible = "gpio-i2c";
>>> + gpios =<&pioA 23 0 /* sda */
>>> + &pioA 24 0 /* scl */
>>> + >;
>>> + gpio-i2c,sda_is_open_drain;
>>> + gpio-i2c,scl_is_open_drain;
>>> + udelay =<2>; /* ~100 kHz */
>>> + #address-cells =<1>;
>>> + #size-cells =<0>;
>>> +
>>> + rv3029c2 at 56 {
>>> + compatible = "rv3029c2";
>>> + reg =<0x56>;
>>> + };
>>> +};
>>> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
>>> index a651779..6b5d794 100644
>>> --- a/drivers/i2c/busses/i2c-gpio.c
>>> +++ b/drivers/i2c/busses/i2c-gpio.c
>>> @@ -14,6 +14,8 @@
>>> #include<linux/module.h>
>>> #include<linux/slab.h>
>>> #include<linux/platform_device.h>
>>> +#include<linux/of_gpio.h>
>>> +#include<linux/of_i2c.h>
>>>
>>> #include<asm/gpio.h>
>>>
>>> @@ -78,16 +80,51 @@ static int i2c_gpio_getscl(void *data)
>>> return gpio_get_value(pdata->scl_pin);
>>> }
>>>
>>> +static int of_i2c_gpio_probe(struct device_node *np,
>>> + struct i2c_gpio_platform_data *pdata)
>>> +{
>>> + u32 reg;
>>> +
>>
>> if (of_gpio_count(np)< 2)
>> return -EINVAL;
> ok
>>
>>> + pdata->sda_pin = of_get_gpio(np, 0);
>>> + pdata->scl_pin = of_get_gpio(np, 1);
>>
>> if (pdata->sda_pin< 0 || pdata->scl_pin< 0)
>> return -EINVAL;
>
>>
>>> +
>>> + if (of_property_read_u32(np, "udelay",®))
>>> + pdata->udelay = reg;
>>> +
>>> + if (of_property_read_u32(np, "timeout",®))
>>> + pdata->timeout = reg;
One more thing missed in original review - of_property_read* rnegative
value on error, so logic has to reversed here:
if (of_property_read_u32(np, "timeout",®) == 0)
or
if (!of_property_read_u32(np, "timeout",®))
As I have been testing your driver on real hardware I've also made
changes (see below) which I have previously written about. If you would
like to see these in git-am-able format please drop me a note.
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index 6b5d794..d22c2c7 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -80,29 +80,45 @@ static int i2c_gpio_getscl(void *data)
return gpio_get_value(pdata->scl_pin);
}
+#ifdef CONFIG_OF
static int of_i2c_gpio_probe(struct device_node *np,
struct i2c_gpio_platform_data *pdata)
{
u32 reg;
+ if (!np)
+ return -EINVAL;
+
+ if (of_gpio_count(np) < 2)
+ return -EINVAL;
+
pdata->sda_pin = of_get_gpio(np, 0);
pdata->scl_pin = of_get_gpio(np, 1);
- if (of_property_read_u32(np, "udelay", ®))
+ if (pdata->sda_pin < 0 || pdata->scl_pin < 0)
+ return -EINVAL;
+
+ if (!of_property_read_u32(np, "udelay", ®))
pdata->udelay = reg;
- if (of_property_read_u32(np, "timeout", ®))
+ if (!of_property_read_u32(np, "timeout", ®))
pdata->timeout = reg;
pdata->sda_is_open_drain =
- !!of_get_property(np, "gpio-i2c,sda_is_open_drain", NULL);
+ !!of_get_property(np, "i2c-gpio,sda-open-drain", NULL);
pdata->scl_is_open_drain =
- !!of_get_property(np, "gpio-i2c,scl_is_open_drain", NULL);
+ !!of_get_property(np, "i2c-gpio,scl-open-drain", NULL);
pdata->scl_is_output_only =
- !!of_get_property(np, "gpio-i2c,scl_is_output_only", NULL);
+ !!of_get_property(np, "i2c-gpio,scl-output-only", NULL);
return 0;
}
+#else
+static int of_i2c_gpio_probe(struct device_node *np,
+ struct i2c_gpio_platform_data *pdata)
+{
+ return -EINVAL
+#endif
static int __devinit i2c_gpio_probe(struct platform_device *pdev)
{
@@ -116,15 +132,11 @@ static int __devinit i2c_gpio_probe(struct
platform_device *pdev)
if (!pdata)
return -ENOMEM;
- if (pdev->dev.of_node) {
- of_i2c_gpio_probe(pdev->dev.of_node, pdata);
- } else {
- if (!pdev->dev.platform_data) {
- ret = -ENXIO;
- goto err_alloc_adap;
- }
+ ret = -ENXIO;
+ if ((ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata)) < 0 &&
!pdev->dev.platform_data)
+ goto err_alloc_adap;
+ else if (pdev->dev.platform_data)
memcpy(pdata, pdev->dev.platform_data, len);
- }
ret = -ENOMEM;
adap = kzalloc(sizeof(struct i2c_adapter), GFP_KERNEL);
@@ -235,7 +247,7 @@ static int __devexit i2c_gpio_remove(struct
platform_device *pdev)
#if defined(CONFIG_OF)
static const struct of_device_id gpio_i2c_dt_ids[] = {
- { .compatible = "gpio-i2c", },
+ { .compatible = "i2c-gpio", },
{ /* sentinel */ }
};
Regards,
--
Karol Lewandowski | Samsung Poland R&D Center | Linux/Platform
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH 1/4] i2c/gpio-i2c add: add DT support
2012-02-06 18:38 ` Karol Lewandowski
2012-02-06 19:15 ` Jean Delvare
2012-02-07 3:25 ` Jean-Christophe PLAGNIOL-VILLARD
@ 2012-02-13 23:14 ` Ben Dooks
2 siblings, 0 replies; 12+ messages in thread
From: Ben Dooks @ 2012-02-13 23:14 UTC (permalink / raw)
To: linux-arm-kernel
On Mon, Feb 06, 2012 at 07:38:29PM +0100, Karol Lewandowski wrote:
> On 05.02.2012 11:38, Jean-Christophe PLAGNIOL-VILLARD wrote:
>
> Hi!
>
> >+Device-Tree bindings for i2c gpio driver
> >+
> >+Required properties:
> >+ - compatible = "gpio-i2c";
>
> Driver name is "i2c-gpio" in file i2c-gpio.c. Previous version of
> patch adding DT-support (prepared by Thomas Chou[1]) used i2c-gpio -
> could we stick to that name?
>
> [1] https://lkml.org/lkml/2011/2/23/584
>
> >+ - gpios: sda and scl gpio
> >+
> >+
> >+Optional properties:
> >+ - gpio-i2c,sda_is_open_drain: sda as open drain
> >+ - gpio-i2c,scl_is_open_drain: scl as open drain
> >+ - gpio-i2c,scl_is_output_only: scl as output only
>
> Most of DT-properties I've seen used hyphen, not underscore. Could
> we stick to that convention?
>
> (Nitpick: I think that "is" in property names is redundant too.)
>
> >+ - udelay: half clock cycle time in us (may depend on each platform)
>
> Could we use "clock-frequency" as Grant have suggested during review
> of previous patch to i2c-gpio?
I'm with Grant on that, it would be nice to have a reasonably sane set of
default i2c dt bindings that everyone uses.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2012-02-13 23:14 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2012-02-05 10:38 [PATCH 1/4] i2c/gpio-i2c add: add DT support Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 2/4] ARM: at91s: sam9g20 add i2c " Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 3/4] ARM: at91: usb_a9g20 add DT i2c support Jean-Christophe PLAGNIOL-VILLARD
2012-02-05 10:38 ` [PATCH 4/4] ARM: at91: sam9g45 add i2c DT support Jean-Christophe PLAGNIOL-VILLARD
2012-02-06 16:09 ` [PATCH 1/4] i2c/gpio-i2c add: add " Mark Brown
2012-02-07 2:56 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-07 11:25 ` Mark Brown
2012-02-06 18:38 ` Karol Lewandowski
2012-02-06 19:15 ` Jean Delvare
2012-02-07 3:25 ` Jean-Christophe PLAGNIOL-VILLARD
2012-02-07 15:35 ` Karol Lewandowski
2012-02-13 23:14 ` Ben Dooks
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).