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

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

[Jean-Christophe PLAGNIOL-VILLARD]

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
---
v2:

	use devm
	update DT bindings to i2c-gpio and sda-open-drain...
	update against "of: introduce helper to manage boolean"

Best Regards,
J.
 .../devicetree/bindings/gpio/gpio_i2c.txt          |   32 +++++++
 drivers/i2c/busses/i2c-gpio.c                      |   95 +++++++++++++++----
 2 files changed, 107 insertions(+), 20 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..9710ed2
--- /dev/null
+++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
@@ -0,0 +1,32 @@
+Device-Tree bindings for i2c gpio driver
+
+Required properties:
+	- compatible = "i2c-gpio";
+	- gpios: sda and scl gpio
+
+
+Optional properties:
+	- i2c-gpio,sda-open-drain: sda as open drain
+	- i2c-gpio,scl-open-drain: scl as open drain
+	- i2c-gpio,scl-output-only: scl as output only
+	- udelay: delay between GPIO operations (may depend on each platform)
+	- timeout: timeout to get data (ms)
+
+Example nodes:
+
+i2c-gpio@0 {
+	compatible = "i2c-gpio";
+	gpios = <&pioA 23 0 /* sda */
+		 &pioA 24 0 /* scl */
+		>;
+	i2c-gpio,sda-open-drain;
+	i2c-gpio,scl-open-drain;
+	udelay = <2>;		/* ~100 kHz */
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	rv3029c2@56 {
+		compatible = "rv3029c2";
+		reg = <0x56>;
+	};
+};
diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
index a651779..d7032c1 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -14,8 +14,15 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/platform_device.h>
+#include <linux/gpio.h>
+#include <linux/of_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 +85,63 @@ static int i2c_gpio_getscl(void *data)
 	return gpio_get_value(pdata->scl_pin);
 }
 
+static int __devinit of_i2c_gpio_probe(struct device_node *np,
+			     struct i2c_gpio_platform_data *pdata)
+{
+	u32 reg;
+
+	if (of_gpio_count(np) < 2)
+		return -ENODEV;
+
+	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) {
+		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
+		       np->full_name, pdata->sda_pin, pdata->scl_pin);
+		return -ENODEV;
+	}
+
+	if (of_property_read_u32(np, "udelay", &reg))
+		pdata->udelay = reg;
+
+	if (of_property_read_u32(np, "timeout", &reg))
+		pdata->timeout = msecs_to_jiffies(reg);
+
+	pdata->sda_is_open_drain =
+		!!of_property_read_bool(np, "i2c-gpio,sda-open-drain");
+	pdata->scl_is_open_drain =
+		!!of_property_read_bool(np, "i2c-gpio,scl-open-drain");
+	pdata->scl_is_output_only =
+		!!of_property_read_bool(np, "i2c-gpio,scl-output-only");
+
+	return 0;
+}
+
 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.of_node) {
+		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
+		if (ret)
+			return ret;
+	} else {
+		if (!pdev->dev.platform_data)
+			return -ENXIO;
+		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
+	}
 
 	ret = gpio_request(pdata->sda_pin, "sda");
 	if (ret)
@@ -143,6 +189,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,7 +201,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 	if (ret)
 		goto err_add_bus;
 
-	platform_set_drvdata(pdev, adap);
+	of_i2c_register_devices(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,
@@ -168,34 +217,40 @@ 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;
 }
 
+#if defined(CONFIG_OF)
+static const struct of_device_id i2c_gpio_dt_ids[] = {
+	{ .compatible = "i2c-gpio", },
+	{ /* sentinel */ }
+};
+
+MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
+#endif
+
 static struct platform_driver i2c_gpio_driver = {
 	.driver		= {
 		.name	= "i2c-gpio",
 		.owner	= THIS_MODULE,
+		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
 	},
 	.probe		= i2c_gpio_probe,
 	.remove		= __devexit_p(i2c_gpio_remove),
-- 
1.7.7

[PATCH 2/4 v2] ARM: at91: sam9g20 add i2c DT support

[Jean-Christophe PLAGNIOL-VILLARD]

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

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
---
v2:

	update DT bindings

Best Regards,
J.
 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..81f8615 100644
--- a/arch/arm/boot/dts/at91sam9g20.dtsi
+++ b/arch/arm/boot/dts/at91sam9g20.dtsi
@@ -220,6 +220,19 @@
 				status = "disabled";
 			};
 
+			i2c-gpio@0 {
+				compatible = "i2c-gpio";
+				gpios = <&pioA 23 0 /* sda */
+					 &pioA 24 0 /* scl */
+					>;
+				i2c-gpio,sda-open-drain;
+				i2c-gpio,scl-open-drain;
+				udelay = <2>;		/* ~100 kHz */
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+
 		};
 	};
 };
-- 
1.7.7

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

[Jean-Christophe PLAGNIOL-VILLARD]

use i2c-gpio and enable rv3029 RTC

enable the in the sam9g20 defconfig

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.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@0 {
+				status = "okay";
+
+				rv3029c2@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

[PATCH 4/4 v2] ARM: at91: sam9g45 add i2c DT support

[Jean-Christophe PLAGNIOL-VILLARD]

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

Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
---
v2:

	update DT bindings

Best Regards,
J.
 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..9f266ee 100644
--- a/arch/arm/boot/dts/at91sam9g45.dtsi
+++ b/arch/arm/boot/dts/at91sam9g45.dtsi
@@ -211,6 +211,19 @@
 				status = "disabled";
 			};
 
+			i2c-gpio@0 {
+				compatible = "i2c-gpio";
+				gpios = <&pioA 20 0 /* sda */
+					 &pioA 21 0 /* scl */
+					>;
+				i2c-gpio,sda-open-drain;
+				i2c-gpio,scl-open-drain;
+				udelay = <5>;		/* ~100 kHz */
+				#address-cells = <1>;
+				#size-cells = <0>;
+				status = "disabled";
+			};
+
 		};
 	};
 };
-- 
1.7.7

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

[Karol Lewandowski]

On 09.02.2012 03:25, Jean-Christophe PLAGNIOL-VILLARD wrote:

Hi!

> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ-XMD5yJDbdMReXY1tMh2IBg@public.gmane.org
> ---
> v2:
> 
> 	use devm
> 	update DT bindings to i2c-gpio and sda-open-drain...
> 	update against "of: introduce helper to manage boolean"
> 
> Best Regards,
> J.
>  .../devicetree/bindings/gpio/gpio_i2c.txt          |   32 +++++++
>  drivers/i2c/busses/i2c-gpio.c                      |   95 +++++++++++++++----
>  2 files changed, 107 insertions(+), 20 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..9710ed2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
> @@ -0,0 +1,32 @@
> +Device-Tree bindings for i2c gpio driver
> +
> +Required properties:
> +	- compatible = "i2c-gpio";
> +	- gpios: sda and scl gpio
> +
> +
> +Optional properties:
> +	- i2c-gpio,sda-open-drain: sda as open drain
> +	- i2c-gpio,scl-open-drain: scl as open drain
> +	- i2c-gpio,scl-output-only: scl as output only


> +	- udelay: delay between GPIO operations (may depend on each platform)
> +	- timeout: timeout to get data (ms)


If these are really needed then I would prefer to have these fully
qualified (with unit type "-ms/-millisecs" appended).

Regulator framework, with its "-microvolt/-microamp", serve here as
prime example of being quite descriptive (one doesn't neet to look up
the docs). Please see:

  http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637

> +
> +Example nodes:
> +
> +i2c-gpio@0 {
> +	compatible = "i2c-gpio";
> +	gpios = <&pioA 23 0 /* sda */
> +		 &pioA 24 0 /* scl */
> +		>;
> +	i2c-gpio,sda-open-drain;
> +	i2c-gpio,scl-open-drain;
> +	udelay = <2>;		/* ~100 kHz */
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	rv3029c2@56 {
> +		compatible = "rv3029c2";
> +		reg = <0x56>;
> +	};
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index a651779..d7032c1 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,8 +14,15 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of_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 +85,63 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> +static int __devinit of_i2c_gpio_probe(struct device_node *np,
> +			     struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
> +	if (of_gpio_count(np) < 2)
> +		return -ENODEV;
> +
> +	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) {
> +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> +		       np->full_name, pdata->sda_pin, pdata->scl_pin);
> +		return -ENODEV;
> +	}
> +



> +	if (of_property_read_u32(np, "udelay", &reg))
> +		pdata->udelay = reg;
> +
> +	if (of_property_read_u32(np, "timeout", &reg))
> +		pdata->timeout = msecs_to_jiffies(reg);


I don't know if you have received my previous mail[1] but as I said
there - of_property_read_* returns non-zero (< 0) value on _error_.
Thus, logic of above two statements has to be reversed.

[1] http://permalink.gmane.org/gmane.linux.drivers.i2c/10009

Thanks

> +
> +	pdata->sda_is_open_drain =
> +		!!of_property_read_bool(np, "i2c-gpio,sda-open-drain");
> +	pdata->scl_is_open_drain =
> +		!!of_property_read_bool(np, "i2c-gpio,scl-open-drain");
> +	pdata->scl_is_output_only =
> +		!!of_property_read_bool(np, "i2c-gpio,scl-output-only");
> +
> +	return 0;
> +}
> +
>  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.of_node) {
> +		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (!pdev->dev.platform_data)
> +			return -ENXIO;
> +		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));
> +	}
>  
>  	ret = gpio_request(pdata->sda_pin, "sda");
>  	if (ret)
> @@ -143,6 +189,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,7 +201,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_add_bus;
>  
> -	platform_set_drvdata(pdev, adap);
> +	of_i2c_register_devices(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,
> @@ -168,34 +217,40 @@ 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;
>  }
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id i2c_gpio_dt_ids[] = {
> +	{ .compatible = "i2c-gpio", },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
> +#endif
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= __devexit_p(i2c_gpio_remove),

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> On 09.02.2012 03:25, Jean-Christophe PLAGNIOL-VILLARD wrote:
> 
> Hi!
> 
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> > Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> > Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> > Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> > ---
> > v2:
> > 
> > 	use devm
> > 	update DT bindings to i2c-gpio and sda-open-drain...
> > 	update against "of: introduce helper to manage boolean"
> > 
> > Best Regards,
> > J.
> >  .../devicetree/bindings/gpio/gpio_i2c.txt          |   32 +++++++
> >  drivers/i2c/busses/i2c-gpio.c                      |   95 +++++++++++++++----
> >  2 files changed, 107 insertions(+), 20 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..9710ed2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
> > @@ -0,0 +1,32 @@
> > +Device-Tree bindings for i2c gpio driver
> > +
> > +Required properties:
> > +	- compatible = "i2c-gpio";
> > +	- gpios: sda and scl gpio
> > +
> > +
> > +Optional properties:
> > +	- i2c-gpio,sda-open-drain: sda as open drain
> > +	- i2c-gpio,scl-open-drain: scl as open drain
> > +	- i2c-gpio,scl-output-only: scl as output only
> 
> 
> > +	- udelay: delay between GPIO operations (may depend on each platform)
> > +	- timeout: timeout to get data (ms)
> 
> 
> If these are really needed then I would prefer to have these fully
> qualified (with unit type "-ms/-millisecs" appended).
> 
> Regulator framework, with its "-microvolt/-microamp", serve here as
> prime example of being quite descriptive (one doesn't neet to look up
> the docs). Please see:
> 
>   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
timeout are usualy in ms I don't really see the need of -ms or so

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > +	- timeout: timeout to get data (ms)
> > 
> > 
> > If these are really needed then I would prefer to have these fully
> > qualified (with unit type "-ms/-millisecs" appended).
> > 
> > Regulator framework, with its "-microvolt/-microamp", serve here as
> > prime example of being quite descriptive (one doesn't neet to look up
> > the docs). Please see:
> > 
> >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> timeout are usualy in ms I don't really see the need of -ms or so

Which is obviously total crap for udelay, which would be in _micro_seconds.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > +	- timeout: timeout to get data (ms)
> > > 
> > > 
> > > If these are really needed then I would prefer to have these fully
> > > qualified (with unit type "-ms/-millisecs" appended).
> > > 
> > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > prime example of being quite descriptive (one doesn't neet to look up
> > > the docs). Please see:
> > > 
> > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > timeout are usualy in ms I don't really see the need of -ms or so
> 
> Which is obviously total crap for udelay, which would be in _micro_seconds.
agreed but here on i2c gpio I never see timetout as udelay so I don't see
the mandatory to force the name in the binding

futhermore it's maybe linux specific

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > +	- timeout: timeout to get data (ms)
> > > > 
> > > > 
> > > > If these are really needed then I would prefer to have these fully
> > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > 
> > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > the docs). Please see:
> > > > 
> > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > timeout are usualy in ms I don't really see the need of -ms or so
> > 
> > Which is obviously total crap for udelay, which would be in _micro_seconds.
> agreed but here on i2c gpio I never see timetout as udelay so I don't see
> the mandatory to force the name in the binding
> 
> futhermore it's maybe linux specific

Stop grabbing at straws.  There's nothing linux specific about the units
of specification.

What is linux specific is specifying the _delay_ rather than specifying
the bus frequency.  So as soon as you're trying to justify not adding
the units because they may be linux specific, you've already lost that
argument by using a delay rather than a bus frequency.  You can't have
it both ways.

Moreover, mixing microseconds and milliseconds in the properties for a
device is absolutely insane.

So, which ever way, your patch as it currently stands is wrong and broken.

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

[Jean Delvare]

On Mon, 20 Feb 2012 12:50:54 +0000, Russell King - ARM Linux wrote:
> What is linux specific is specifying the _delay_ rather than specifying
> the bus frequency.  So as soon as you're trying to justify not adding
> the units because they may be linux specific, you've already lost that
> argument by using a delay rather than a bus frequency.  You can't have
> it both ways.

While I am not much into DT and did not follow this thread too
carefully... I seem to understand that the dispute is mainly on
frequency vs. udelay specification for the bus speed, Jean-Christophe
arguing that hardware-specific delays are added when changing e.g. a
GPIO pin output value and thus the frequency can't be guaranteed. Do I
get this right?

If so I would suggest that you specify neither the frequency nor
udelay, but the _maximum_ frequency. Where maximum frequency = 10^6 /
(2 * udelay) Hz. Would this make everybody happy?

Note that I2C allows clock stretching by the slaves so even without the
extra delay incurred by hardware state switching, the bus operating
frequency is already a maximum rather than a guaranteed valued.

-- 
Jean Delvare

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 02:35:57PM +0100, Jean Delvare wrote:
> On Mon, 20 Feb 2012 12:50:54 +0000, Russell King - ARM Linux wrote:
> > What is linux specific is specifying the _delay_ rather than specifying
> > the bus frequency.  So as soon as you're trying to justify not adding
> > the units because they may be linux specific, you've already lost that
> > argument by using a delay rather than a bus frequency.  You can't have
> > it both ways.
> 
> While I am not much into DT and did not follow this thread too
> carefully... I seem to understand that the dispute is mainly on
> frequency vs. udelay specification for the bus speed, Jean-Christophe
> arguing that hardware-specific delays are added when changing e.g. a
> GPIO pin output value and thus the frequency can't be guaranteed. Do I
> get this right?

This sub-thread is more about the units of the properties rather
than the properties themselves.

What's being proposed is to have two properties, one named 'udelay'
which takes microseconds, and one named 'timeout' which takes
milliseconds.

I'm saying that's a completely absurd proposal, as the proposal is
for two opaque numeric properties with different units.  At least
make the units the same, or as Karol said, incorporate the units
into the property names.

At least we can then create new properties in the future of we need
to change the units, rather than thinking up a different name for
'timeout'.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:51 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 02:35:57PM +0100, Jean Delvare wrote:
> > On Mon, 20 Feb 2012 12:50:54 +0000, Russell King - ARM Linux wrote:
> > > What is linux specific is specifying the _delay_ rather than specifying
> > > the bus frequency.  So as soon as you're trying to justify not adding
> > > the units because they may be linux specific, you've already lost that
> > > argument by using a delay rather than a bus frequency.  You can't have
> > > it both ways.
> > 
> > While I am not much into DT and did not follow this thread too
> > carefully... I seem to understand that the dispute is mainly on
> > frequency vs. udelay specification for the bus speed, Jean-Christophe
> > arguing that hardware-specific delays are added when changing e.g. a
> > GPIO pin output value and thus the frequency can't be guaranteed. Do I
> > get this right?
> 
> This sub-thread is more about the units of the properties rather
> than the properties themselves.
> 
> What's being proposed is to have two properties, one named 'udelay'
> which takes microseconds, and one named 'timeout' which takes
> milliseconds.
> 
> I'm saying that's a completely absurd proposal, as the proposal is
> for two opaque numeric properties with different units.  At least
> make the units the same, or as Karol said, incorporate the units
> into the property names.
> 
> At least we can then create new properties in the future of we need
> to change the units, rather than thinking up a different name for
> 'timeout'.

please read the binding

we have 2 properties

 - udelay: delay between GPIO operations (may depend on each platform)
 - timeout: timeout to get data (ms)

 please do not mixed them together

udelay is related to bus frequency

timeout is implelentation detail, that allow to parameter the timeout og i2c
bit algo when reading the scl on slow device

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 03:51:37PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 13:51 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > On Mon, Feb 20, 2012 at 02:35:57PM +0100, Jean Delvare wrote:
> > > On Mon, 20 Feb 2012 12:50:54 +0000, Russell King - ARM Linux wrote:
> > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > the units because they may be linux specific, you've already lost that
> > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > it both ways.
> > > 
> > > While I am not much into DT and did not follow this thread too
> > > carefully... I seem to understand that the dispute is mainly on
> > > frequency vs. udelay specification for the bus speed, Jean-Christophe
> > > arguing that hardware-specific delays are added when changing e.g. a
> > > GPIO pin output value and thus the frequency can't be guaranteed. Do I
> > > get this right?
> > 
> > This sub-thread is more about the units of the properties rather
> > than the properties themselves.
> > 
> > What's being proposed is to have two properties, one named 'udelay'
> > which takes microseconds, and one named 'timeout' which takes
> > milliseconds.
> > 
> > I'm saying that's a completely absurd proposal, as the proposal is
> > for two opaque numeric properties with different units.  At least
> > make the units the same, or as Karol said, incorporate the units
> > into the property names.
> > 
> > At least we can then create new properties in the future of we need
> > to change the units, rather than thinking up a different name for
> > 'timeout'.
> 
> please read the binding
> 
> we have 2 properties
> 
>  - udelay: delay between GPIO operations (may depend on each platform)
>  - timeout: timeout to get data (ms)
> 
>  please do not mixed them together
> 
> udelay is related to bus frequency
> 
> timeout is implelentation detail, that allow to parameter the timeout og i2c
> bit algo when reading the scl on slow device

FOR FUCKING SAKE.  MILLISECONDS FOR SOME STUFF VS MICROSECONDS FOR OTHER
STUFF IS BAD NEWS.  FIX THIS AND I WILL WITHDRAW MY NACK.  CONTINUE BEING
OBSTRUCTIVE OVER THIS AND MY NACK STANDS.

I HOPE USING BIG LETTERS HELPS TO GET MY POINT THROUGH.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > +	- timeout: timeout to get data (ms)
> > > > > 
> > > > > 
> > > > > If these are really needed then I would prefer to have these fully
> > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > 
> > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > the docs). Please see:
> > > > > 
> > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > 
> > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > the mandatory to force the name in the binding
> > 
> > futhermore it's maybe linux specific
> 
> Stop grabbing at straws.  There's nothing linux specific about the units
> of specification.
> 
> What is linux specific is specifying the _delay_ rather than specifying
> the bus frequency.  So as soon as you're trying to justify not adding
> the units because they may be linux specific, you've already lost that
> argument by using a delay rather than a bus frequency.  You can't have
> it both ways.
> 
> Moreover, mixing microseconds and milliseconds in the properties for a
> device is absolutely insane.
> 
> So, which ever way, your patch as it currently stands is wrong and broken.
 no what I said is the binding timeout is maybe linux specific for i2c gpio

Best Regards,
J.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > 
> > > > > > 
> > > > > > If these are really needed then I would prefer to have these fully
> > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > 
> > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > the docs). Please see:
> > > > > > 
> > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > 
> > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > the mandatory to force the name in the binding
> > > 
> > > futhermore it's maybe linux specific
> > 
> > Stop grabbing at straws.  There's nothing linux specific about the units
> > of specification.
> > 
> > What is linux specific is specifying the _delay_ rather than specifying
> > the bus frequency.  So as soon as you're trying to justify not adding
> > the units because they may be linux specific, you've already lost that
> > argument by using a delay rather than a bus frequency.  You can't have
> > it both ways.
> > 
> > Moreover, mixing microseconds and milliseconds in the properties for a
> > device is absolutely insane.
> > 
> > So, which ever way, your patch as it currently stands is wrong and broken.
>  no what I said is the binding timeout is maybe linux specific for i2c gpio

I do not argue about that here we do not even disucss about the bus frequency
but the specific bining to the i2c algo bit for it's internal timeout

the timeout is used to do not end in an infinite loop while ready the scl on
slow device

that's why I said it's maybe linux specific

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > 
> > > > > > > 
> > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > 
> > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > the docs). Please see:
> > > > > > > 
> > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > 
> > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > the mandatory to force the name in the binding
> > > > 
> > > > futhermore it's maybe linux specific
> > > 
> > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > of specification.
> > > 
> > > What is linux specific is specifying the _delay_ rather than specifying
> > > the bus frequency.  So as soon as you're trying to justify not adding
> > > the units because they may be linux specific, you've already lost that
> > > argument by using a delay rather than a bus frequency.  You can't have
> > > it both ways.
> > > 
> > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > device is absolutely insane.
> > > 
> > > So, which ever way, your patch as it currently stands is wrong and broken.
> >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> 
> I do not argue about that here we do not even disucss about the bus frequency
> but the specific bining to the i2c algo bit for it's internal timeout
> 
> the timeout is used to do not end in an infinite loop while ready the scl on
> slow device

The patch is still wrong and broken.

As you're not listening to me at all, I've lost patience, so I'm just going
to say this:

Explicit NAK on this patch.

When you feel like you can _constructively_ _consider_ the point that both
Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
to continue this discussion.  If not, don't waste your time writing another
email.  I hope that's plain.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > 
> > > > > > > > 
> > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > 
> > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > the docs). Please see:
> > > > > > > > 
> > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > 
> > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > the mandatory to force the name in the binding
> > > > > 
> > > > > futhermore it's maybe linux specific
> > > > 
> > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > of specification.
> > > > 
> > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > the units because they may be linux specific, you've already lost that
> > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > it both ways.
> > > > 
> > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > device is absolutely insane.
> > > > 
> > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > 
> > I do not argue about that here we do not even disucss about the bus frequency
> > but the specific bining to the i2c algo bit for it's internal timeout
> > 
> > the timeout is used to do not end in an infinite loop while ready the scl on
> > slow device
> 
> The patch is still wrong and broken.
> 
> As you're not listening to me at all, I've lost patience, so I'm just going
> to say this:
> 
> Explicit NAK on this patch.
> 
> When you feel like you can _constructively_ _consider_ the point that both
> Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> to continue this discussion.  If not, don't waste your time writing another
> email.  I hope that's plain.
I do not discuss about the U_N_I_T_S at all in this reply
so the NACK is no revelent

I just said here the bind for the "timeout" is maybe linux specific which means
need to be prefixed by "linux,"

so you discuss about timing again when I explain the binding named "timeout"
meaming

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > 
> > > > > > > > > 
> > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > 
> > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > the docs). Please see:
> > > > > > > > > 
> > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > 
> > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > the mandatory to force the name in the binding
> > > > > > 
> > > > > > futhermore it's maybe linux specific
> > > > > 
> > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > of specification.
> > > > > 
> > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > the units because they may be linux specific, you've already lost that
> > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > it both ways.
> > > > > 
> > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > device is absolutely insane.
> > > > > 
> > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > 
> > > I do not argue about that here we do not even disucss about the bus frequency
> > > but the specific bining to the i2c algo bit for it's internal timeout
> > > 
> > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > slow device
> > 
> > The patch is still wrong and broken.
> > 
> > As you're not listening to me at all, I've lost patience, so I'm just going
> > to say this:
> > 
> > Explicit NAK on this patch.
> > 
> > When you feel like you can _constructively_ _consider_ the point that both
> > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > to continue this discussion.  If not, don't waste your time writing another
> > email.  I hope that's plain.
>
> I do not discuss about the U_N_I_T_S at all in this reply
> so the NACK is no revelent

LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
TIMEOUT.

I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.

I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.

GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
OR DELAYS ARE APPROPRIATE IN THIS CASE.

ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
*U*N*I*T*S*.

HAVE YOU GOT THE FUCKING MESSAGE YET?

SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:00 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > > 
> > > > > > > > > > 
> > > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > > 
> > > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > > the docs). Please see:
> > > > > > > > > > 
> > > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > > 
> > > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > > the mandatory to force the name in the binding
> > > > > > > 
> > > > > > > futhermore it's maybe linux specific
> > > > > > 
> > > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > > of specification.
> > > > > > 
> > > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > > the units because they may be linux specific, you've already lost that
> > > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > > it both ways.
> > > > > > 
> > > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > > device is absolutely insane.
> > > > > > 
> > > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > > 
> > > > I do not argue about that here we do not even disucss about the bus frequency
> > > > but the specific bining to the i2c algo bit for it's internal timeout
> > > > 
> > > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > > slow device
> > > 
> > > The patch is still wrong and broken.
> > > 
> > > As you're not listening to me at all, I've lost patience, so I'm just going
> > > to say this:
> > > 
> > > Explicit NAK on this patch.
> > > 
> > > When you feel like you can _constructively_ _consider_ the point that both
> > > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > > to continue this discussion.  If not, don't waste your time writing another
> > > email.  I hope that's plain.
> >
> > I do not discuss about the U_N_I_T_S at all in this reply
> > so the NACK is no revelent
> 
> LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
> MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
> TIMEOUT.
> 
> I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
> 
> I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
> 
> GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
> NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
> OR DELAYS ARE APPROPRIATE IN THIS CASE.
> 
> ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
> *U*N*I*T*S*.
> 
> HAVE YOU GOT THE FUCKING MESSAGE YET?
> 
> SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.

I just said we have 2 properties

- timeout which is expressed in jiffies (today in C) which is at my sense a linux specific
  propertie as it's representing a timeout of the i2c bit algo
  and here I don't see the mandatory to name it timeout-ms or timeout-milisecond

- udelay which is the delay between GPIO operations

Nothing more

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 04:08:10PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 15:00 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > > > 
> > > > > > > > > > > 
> > > > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > > > 
> > > > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > > > the docs). Please see:
> > > > > > > > > > > 
> > > > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > > > 
> > > > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > > > the mandatory to force the name in the binding
> > > > > > > > 
> > > > > > > > futhermore it's maybe linux specific
> > > > > > > 
> > > > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > > > of specification.
> > > > > > > 
> > > > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > > > the units because they may be linux specific, you've already lost that
> > > > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > > > it both ways.
> > > > > > > 
> > > > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > > > device is absolutely insane.
> > > > > > > 
> > > > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > > > 
> > > > > I do not argue about that here we do not even disucss about the bus frequency
> > > > > but the specific bining to the i2c algo bit for it's internal timeout
> > > > > 
> > > > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > > > slow device
> > > > 
> > > > The patch is still wrong and broken.
> > > > 
> > > > As you're not listening to me at all, I've lost patience, so I'm just going
> > > > to say this:
> > > > 
> > > > Explicit NAK on this patch.
> > > > 
> > > > When you feel like you can _constructively_ _consider_ the point that both
> > > > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > > > to continue this discussion.  If not, don't waste your time writing another
> > > > email.  I hope that's plain.
> > >
> > > I do not discuss about the U_N_I_T_S at all in this reply
> > > so the NACK is no revelent
> > 
> > LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
> > MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
> > TIMEOUT.
> > 
> > I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
> > 
> > I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
> > 
> > GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
> > NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
> > OR DELAYS ARE APPROPRIATE IN THIS CASE.
> > 
> > ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
> > *U*N*I*T*S*.
> > 
> > HAVE YOU GOT THE FUCKING MESSAGE YET?
> > 
> > SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
> 
> I just said we have 2 properties
> 
> - timeout which is expressed in jiffies (today in C) which is at my sense a linux specific
>   propertie as it's representing a timeout of the i2c bit algo
>   and here I don't see the mandatory to name it timeout-ms or timeout-milisecond

THIS IS IN MILLISECONDS.

> - udelay which is the delay between GPIO operations

THIS IS IN MICROSECONDS.

TWO DIFFERENT UNITS FOR TWO DIFFERENT PROPERTIES FOR THE SAME DEVICE.
CONFUSING.  NACK STANDS.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 15:27 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 04:08:10PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 15:00 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > > > > 
> > > > > > > > > > > > 
> > > > > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > > > > 
> > > > > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > > > > the docs). Please see:
> > > > > > > > > > > > 
> > > > > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > > > > 
> > > > > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > > > > the mandatory to force the name in the binding
> > > > > > > > > 
> > > > > > > > > futhermore it's maybe linux specific
> > > > > > > > 
> > > > > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > > > > of specification.
> > > > > > > > 
> > > > > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > > > > the units because they may be linux specific, you've already lost that
> > > > > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > > > > it both ways.
> > > > > > > > 
> > > > > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > > > > device is absolutely insane.
> > > > > > > > 
> > > > > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > > > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > > > > 
> > > > > > I do not argue about that here we do not even disucss about the bus frequency
> > > > > > but the specific bining to the i2c algo bit for it's internal timeout
> > > > > > 
> > > > > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > > > > slow device
> > > > > 
> > > > > The patch is still wrong and broken.
> > > > > 
> > > > > As you're not listening to me at all, I've lost patience, so I'm just going
> > > > > to say this:
> > > > > 
> > > > > Explicit NAK on this patch.
> > > > > 
> > > > > When you feel like you can _constructively_ _consider_ the point that both
> > > > > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > > > > to continue this discussion.  If not, don't waste your time writing another
> > > > > email.  I hope that's plain.
> > > >
> > > > I do not discuss about the U_N_I_T_S at all in this reply
> > > > so the NACK is no revelent
> > > 
> > > LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
> > > MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
> > > TIMEOUT.
> > > 
> > > I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
> > > 
> > > I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
> > > 
> > > GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
> > > NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
> > > OR DELAYS ARE APPROPRIATE IN THIS CASE.
> > > 
> > > ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
> > > *U*N*I*T*S*.
> > > 
> > > HAVE YOU GOT THE FUCKING MESSAGE YET?
> > > 
> > > SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
> > 
> > I just said we have 2 properties
> > 
> > - timeout which is expressed in jiffies (today in C) which is at my sense a linux specific
> >   propertie as it's representing a timeout of the i2c bit algo
> >   and here I don't see the mandatory to name it timeout-ms or timeout-milisecond
> 
> THIS IS IN MILLISECONDS.
> 
> > - udelay which is the delay between GPIO operations
> 
> THIS IS IN MICROSECONDS.
> 
> TWO DIFFERENT UNITS FOR TWO DIFFERENT PROPERTIES FOR THE SAME DEVICE.
> CONFUSING.  NACK STANDS.

I said 

> > > > > > I do not argue about that 

after I just discuss about the fact taht "timeout" is maybe linux
implementation specic and maybe need "linux," prefix that's all

Best Regards,
J.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:30 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 15:27 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > On Mon, Feb 20, 2012 at 04:08:10PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 15:00 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > > > > > 
> > > > > > > > > > > > > 
> > > > > > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > > > > > the docs). Please see:
> > > > > > > > > > > > > 
> > > > > > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > > > > > 
> > > > > > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > > > > > the mandatory to force the name in the binding
> > > > > > > > > > 
> > > > > > > > > > futhermore it's maybe linux specific
> > > > > > > > > 
> > > > > > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > > > > > of specification.
> > > > > > > > > 
> > > > > > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > > > > > the units because they may be linux specific, you've already lost that
> > > > > > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > > > > > it both ways.
> > > > > > > > > 
> > > > > > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > > > > > device is absolutely insane.
> > > > > > > > > 
> > > > > > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > > > > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > > > > > 
> > > > > > > I do not argue about that here we do not even disucss about the bus frequency
> > > > > > > but the specific bining to the i2c algo bit for it's internal timeout
> > > > > > > 
> > > > > > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > > > > > slow device
> > > > > > 
> > > > > > The patch is still wrong and broken.
> > > > > > 
> > > > > > As you're not listening to me at all, I've lost patience, so I'm just going
> > > > > > to say this:
> > > > > > 
> > > > > > Explicit NAK on this patch.
> > > > > > 
> > > > > > When you feel like you can _constructively_ _consider_ the point that both
> > > > > > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > > > > > to continue this discussion.  If not, don't waste your time writing another
> > > > > > email.  I hope that's plain.
> > > > >
> > > > > I do not discuss about the U_N_I_T_S at all in this reply
> > > > > so the NACK is no revelent
> > > > 
> > > > LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
> > > > MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
> > > > TIMEOUT.
> > > > 
> > > > I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
> > > > 
> > > > I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
> > > > 
> > > > GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
> > > > NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
> > > > OR DELAYS ARE APPROPRIATE IN THIS CASE.
> > > > 
> > > > ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
> > > > *U*N*I*T*S*.
> > > > 
> > > > HAVE YOU GOT THE FUCKING MESSAGE YET?
> > > > 
> > > > SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
> > > 
> > > I just said we have 2 properties
> > > 
> > > - timeout which is expressed in jiffies (today in C) which is at my sense a linux specific
> > >   propertie as it's representing a timeout of the i2c bit algo
> > >   and here I don't see the mandatory to name it timeout-ms or timeout-milisecond
> > 
> > THIS IS IN MILLISECONDS.
> > 
> > > - udelay which is the delay between GPIO operations
> > 
> > THIS IS IN MICROSECONDS.
> > 
> > TWO DIFFERENT UNITS FOR TWO DIFFERENT PROPERTIES FOR THE SAME DEVICE.
> > CONFUSING.  NACK STANDS.
> 
> I said 
> 
> > > > > > > I do not argue about that 
> 
> after I just discuss about the fact taht "timeout" is maybe linux
> implementation specic and maybe need "linux," prefix that's all
can I have the NACK removed because I sis not agrued on the UNIT I add more
information about the fact that the property may be linux specific

Best Regards,
J.

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

[Russell King - ARM Linux]

On Mon, Feb 20, 2012 at 04:46:13PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 16:30 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 15:27 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > On Mon, Feb 20, 2012 at 04:08:10PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > On 15:00 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > > > > > > the docs). Please see:
> > > > > > > > > > > > > > 
> > > > > > > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > > > > > > 
> > > > > > > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > > > > > > the mandatory to force the name in the binding
> > > > > > > > > > > 
> > > > > > > > > > > futhermore it's maybe linux specific
> > > > > > > > > > 
> > > > > > > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > > > > > > of specification.
> > > > > > > > > > 
> > > > > > > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > > > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > > > > > > the units because they may be linux specific, you've already lost that
> > > > > > > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > > > > > > it both ways.
> > > > > > > > > > 
> > > > > > > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > > > > > > device is absolutely insane.
> > > > > > > > > > 
> > > > > > > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > > > > > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > > > > > > 
> > > > > > > > I do not argue about that here we do not even disucss about the bus frequency
> > > > > > > > but the specific bining to the i2c algo bit for it's internal timeout
> > > > > > > > 
> > > > > > > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > > > > > > slow device
> > > > > > > 
> > > > > > > The patch is still wrong and broken.
> > > > > > > 
> > > > > > > As you're not listening to me at all, I've lost patience, so I'm just going
> > > > > > > to say this:
> > > > > > > 
> > > > > > > Explicit NAK on this patch.
> > > > > > > 
> > > > > > > When you feel like you can _constructively_ _consider_ the point that both
> > > > > > > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > > > > > > to continue this discussion.  If not, don't waste your time writing another
> > > > > > > email.  I hope that's plain.
> > > > > >
> > > > > > I do not discuss about the U_N_I_T_S at all in this reply
> > > > > > so the NACK is no revelent
> > > > > 
> > > > > LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
> > > > > MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
> > > > > TIMEOUT.
> > > > > 
> > > > > I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
> > > > > 
> > > > > I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
> > > > > 
> > > > > GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
> > > > > NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
> > > > > OR DELAYS ARE APPROPRIATE IN THIS CASE.
> > > > > 
> > > > > ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
> > > > > *U*N*I*T*S*.
> > > > > 
> > > > > HAVE YOU GOT THE FUCKING MESSAGE YET?
> > > > > 
> > > > > SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
> > > > 
> > > > I just said we have 2 properties
> > > > 
> > > > - timeout which is expressed in jiffies (today in C) which is at my sense a linux specific
> > > >   propertie as it's representing a timeout of the i2c bit algo
> > > >   and here I don't see the mandatory to name it timeout-ms or timeout-milisecond
> > > 
> > > THIS IS IN MILLISECONDS.
> > > 
> > > > - udelay which is the delay between GPIO operations
> > > 
> > > THIS IS IN MICROSECONDS.
> > > 
> > > TWO DIFFERENT UNITS FOR TWO DIFFERENT PROPERTIES FOR THE SAME DEVICE.
> > > CONFUSING.  NACK STANDS.
> > 
> > I said 
> > 
> > > > > > > > I do not argue about that 
> > 
> > after I just discuss about the fact taht "timeout" is maybe linux
> > implementation specic and maybe need "linux," prefix that's all
> can I have the NACK removed because I sis not agrued on the UNIT I add more
> information about the fact that the property may be linux specific

There is nothing more to add to this thread.  You have all the
information you require to have me remove the NACK.  I will not repeat
it yet again.  As your patch currently stands it is not acceptable to
me.

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 16:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> On Mon, Feb 20, 2012 at 04:46:13PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > On 16:30 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > On 15:27 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > On Mon, Feb 20, 2012 at 04:08:10PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > On 15:00 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > On Mon, Feb 20, 2012 at 03:46:35PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > On 13:58 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > On Mon, Feb 20, 2012 at 02:46:34PM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > On 14:37 Mon 20 Feb     , Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > On 12:50 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > > > On Mon, Feb 20, 2012 at 11:22:31AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > > > On 10:08 Mon 20 Feb     , Russell King - ARM Linux wrote:
> > > > > > > > > > > > > On Mon, Feb 20, 2012 at 10:58:13AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > > > > > > > > > > > > > On 18:17 Mon 13 Feb     , Karol Lewandowski wrote:
> > > > > > > > > > > > > > > > +	- udelay: delay between GPIO operations (may depend on each platform)
> > > > > > > > > > > > > > > > +	- timeout: timeout to get data (ms)
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > If these are really needed then I would prefer to have these fully
> > > > > > > > > > > > > > > qualified (with unit type "-ms/-millisecs" appended).
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > > Regulator framework, with its "-microvolt/-microamp", serve here as
> > > > > > > > > > > > > > > prime example of being quite descriptive (one doesn't neet to look up
> > > > > > > > > > > > > > > the docs). Please see:
> > > > > > > > > > > > > > > 
> > > > > > > > > > > > > > >   http://permalink.gmane.org/gmane.linux.ports.arm.omap/67637
> > > > > > > > > > > > > > timeout are usualy in ms I don't really see the need of -ms or so
> > > > > > > > > > > > > 
> > > > > > > > > > > > > Which is obviously total crap for udelay, which would be in _micro_seconds.
> > > > > > > > > > > > agreed but here on i2c gpio I never see timetout as udelay so I don't see
> > > > > > > > > > > > the mandatory to force the name in the binding
> > > > > > > > > > > > 
> > > > > > > > > > > > futhermore it's maybe linux specific
> > > > > > > > > > > 
> > > > > > > > > > > Stop grabbing at straws.  There's nothing linux specific about the units
> > > > > > > > > > > of specification.
> > > > > > > > > > > 
> > > > > > > > > > > What is linux specific is specifying the _delay_ rather than specifying
> > > > > > > > > > > the bus frequency.  So as soon as you're trying to justify not adding
> > > > > > > > > > > the units because they may be linux specific, you've already lost that
> > > > > > > > > > > argument by using a delay rather than a bus frequency.  You can't have
> > > > > > > > > > > it both ways.
> > > > > > > > > > > 
> > > > > > > > > > > Moreover, mixing microseconds and milliseconds in the properties for a
> > > > > > > > > > > device is absolutely insane.
> > > > > > > > > > > 
> > > > > > > > > > > So, which ever way, your patch as it currently stands is wrong and broken.
> > > > > > > > > >  no what I said is the binding timeout is maybe linux specific for i2c gpio
> > > > > > > > > 
> > > > > > > > > I do not argue about that here we do not even disucss about the bus frequency
> > > > > > > > > but the specific bining to the i2c algo bit for it's internal timeout
> > > > > > > > > 
> > > > > > > > > the timeout is used to do not end in an infinite loop while ready the scl on
> > > > > > > > > slow device
> > > > > > > > 
> > > > > > > > The patch is still wrong and broken.
> > > > > > > > 
> > > > > > > > As you're not listening to me at all, I've lost patience, so I'm just going
> > > > > > > > to say this:
> > > > > > > > 
> > > > > > > > Explicit NAK on this patch.
> > > > > > > > 
> > > > > > > > When you feel like you can _constructively_ _consider_ the point that both
> > > > > > > > Karol and myself have raised with respect to the _U_N_I_T_S_ then feel free
> > > > > > > > to continue this discussion.  If not, don't waste your time writing another
> > > > > > > > email.  I hope that's plain.
> > > > > > >
> > > > > > > I do not discuss about the U_N_I_T_S at all in this reply
> > > > > > > so the NACK is no revelent
> > > > > > 
> > > > > > LET ME PUT IT IN BIG LETTERS FOR YOU.  I AM DISCUSSING THE UNITS ISSUE IN
> > > > > > MY EMAILS.  YOU KEEP BRINGING UP THE LINUX SPECIFIC CRAP ABOUT UDELAY OR
> > > > > > TIMEOUT.
> > > > > > 
> > > > > > I AM TALKING ABOUT UNITS.  MICROSECONDS.  MILLISECONDS.
> > > > > > 
> > > > > > I HAVE BEEN TALKING ABOUT UNITS ON THIS THREAD ALL DAY SO FAR.
> > > > > > 
> > > > > > GET IT THROUGH YOUR BIG HEAD THAT I AM DISCUSSING ABOUT THE UNITS.  I AM
> > > > > > NOT DISCUSSING, AND HAVE NOT BEEN DISCUSSING ABOUT WHETHER BUS FREQUENCY
> > > > > > OR DELAYS ARE APPROPRIATE IN THIS CASE.
> > > > > > 
> > > > > > ALL THAT I AM DISCUSSING IS ABOUT THE UNITS.  *T*H*E* *S*O*D*D*I*N*G*
> > > > > > *U*N*I*T*S*.
> > > > > > 
> > > > > > HAVE YOU GOT THE FUCKING MESSAGE YET?
> > > > > > 
> > > > > > SO, THE NACK STANDS UNTIL YOU START REPLYING TO THE POINT I AM RAISING.
> > > > > 
> > > > > I just said we have 2 properties
> > > > > 
> > > > > - timeout which is expressed in jiffies (today in C) which is at my sense a linux specific
> > > > >   propertie as it's representing a timeout of the i2c bit algo
> > > > >   and here I don't see the mandatory to name it timeout-ms or timeout-milisecond
> > > > 
> > > > THIS IS IN MILLISECONDS.
> > > > 
> > > > > - udelay which is the delay between GPIO operations
> > > > 
> > > > THIS IS IN MICROSECONDS.
> > > > 
> > > > TWO DIFFERENT UNITS FOR TWO DIFFERENT PROPERTIES FOR THE SAME DEVICE.
> > > > CONFUSING.  NACK STANDS.
> > > 
> > > I said 
> > > 
> > > > > > > > > I do not argue about that 
> > > 
> > > after I just discuss about the fact taht "timeout" is maybe linux
> > > implementation specic and maybe need "linux," prefix that's all
> > can I have the NACK removed because I sis not agrued on the UNIT I add more
> > information about the fact that the property may be linux specific
> 
> There is nothing more to add to this thread.  You have all the
> information you require to have me remove the NACK.  I will not repeat
> it yet again.  As your patch currently stands it is not acceptable to
> me.
I said already yes for the change so can I've the Acked-by

for this
- udelay: delay between GPIO operations (may depend on each platform)

- i2c-algo-bit,timeout-milliseconds: timeout to get data (ms)

Best Regards,
J.

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

[Ben Dooks]

On Thu, Feb 09, 2012 at 03:25:05AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj-sclMFOaUSTBWk0Htik3J/w@public.gmane.org>
> Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
> Cc: linux-i2c-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> Cc: devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
> ---
> v2:
> 
> 	use devm
> 	update DT bindings to i2c-gpio and sda-open-drain...
> 	update against "of: introduce helper to manage boolean"
> 
> Best Regards,
> J.
>  .../devicetree/bindings/gpio/gpio_i2c.txt          |   32 +++++++
>  drivers/i2c/busses/i2c-gpio.c                      |   95 +++++++++++++++----
>  2 files changed, 107 insertions(+), 20 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..9710ed2
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
> @@ -0,0 +1,32 @@
> +Device-Tree bindings for i2c gpio driver
> +
> +Required properties:
> +	- compatible = "i2c-gpio";
> +	- gpios: sda and scl gpio
> +
> +
> +Optional properties:
> +	- i2c-gpio,sda-open-drain: sda as open drain
> +	- i2c-gpio,scl-open-drain: scl as open drain
> +	- i2c-gpio,scl-output-only: scl as output only
> +	- udelay: delay between GPIO operations (may depend on each platform)
> +	- timeout: timeout to get data (ms)
> +
> +Example nodes:
> +
> +i2c-gpio@0 {
> +	compatible = "i2c-gpio";
> +	gpios = <&pioA 23 0 /* sda */
> +		 &pioA 24 0 /* scl */
> +		>;
> +	i2c-gpio,sda-open-drain;
> +	i2c-gpio,scl-open-drain;
> +	udelay = <2>;		/* ~100 kHz */
> +	#address-cells = <1>;
> +	#size-cells = <0>;
> +
> +	rv3029c2@56 {
> +		compatible = "rv3029c2";
> +		reg = <0x56>;
> +	};
> +};
> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index a651779..d7032c1 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -14,8 +14,15 @@
>  #include <linux/module.h>
>  #include <linux/slab.h>
>  #include <linux/platform_device.h>
> +#include <linux/gpio.h>
> +#include <linux/of_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 +85,63 @@ static int i2c_gpio_getscl(void *data)
>  	return gpio_get_value(pdata->scl_pin);
>  }
>  
> +static int __devinit of_i2c_gpio_probe(struct device_node *np,
> +			     struct i2c_gpio_platform_data *pdata)
> +{
> +	u32 reg;
> +
> +	if (of_gpio_count(np) < 2)
> +		return -ENODEV;

Hmm, there's no error print and unless updated recently I think the
driver core will not produce any warnings if ->probe returns an
-ENODEV. I would prefer to see this printing a warning as for the
case below if an GPIO fails to be found.

Maybe return -EINVAL as it is not a valid configuration.

> +
> +	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) {
> +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> +		       np->full_name, pdata->sda_pin, pdata->scl_pin);
> +		return -ENODEV;
> +	}
> +
> +	if (of_property_read_u32(np, "udelay", &reg))
> +		pdata->udelay = reg;

This sounds like a "linux" specific property. I would think about
giving it a name like frequency, or ask Grant if we have a base set
of properties that a i2c controller should think about implementing.

> +	if (of_property_read_u32(np, "timeout", &reg))
> +		pdata->timeout = msecs_to_jiffies(reg);
> +
> +	pdata->sda_is_open_drain =
> +		!!of_property_read_bool(np, "i2c-gpio,sda-open-drain");
> +	pdata->scl_is_open_drain =
> +		!!of_property_read_bool(np, "i2c-gpio,scl-open-drain");
> +	pdata->scl_is_output_only =
> +		!!of_property_read_bool(np, "i2c-gpio,scl-output-only");

I hate !!, why does of_property_read_bool() not return a bool? Also,
why can't I find of_property_read_bool() in include/linux/* ?

Also, does of_property_read_bool() consitently return a useful value if
the property does not exist, or is not readable? I am most concerned
with the behaviour if one of these properties has un-parsable contents.

> +	return 0;
> +}
> +
>  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.of_node) {
> +		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
> +		if (ret)
> +			return ret;
> +	} else {
> +		if (!pdev->dev.platform_data)
> +			return -ENXIO;
> +		memcpy(pdata, pdev->dev.platform_data, sizeof(*pdata));

I'd moan about the error return, but that's the original driver for you.



> +	}
>  
>  	ret = gpio_request(pdata->sda_pin, "sda");
>  	if (ret)
> @@ -143,6 +189,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;

Do we need to keep this around, surely we can get back to the of_node from
what we already have in adap->dev.parent?

>  
>  	/*
>  	 * If "dev->id" is negative we consider it as zero.
> @@ -154,7 +201,9 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  	if (ret)
>  		goto err_add_bus;
>  
> -	platform_set_drvdata(pdev, adap);
> +	of_i2c_register_devices(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,
> @@ -168,34 +217,40 @@ 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;
>  }
>  
> +#if defined(CONFIG_OF)
> +static const struct of_device_id i2c_gpio_dt_ids[] = {
> +	{ .compatible = "i2c-gpio", },
> +	{ /* sentinel */ }
> +};
> +
> +MODULE_DEVICE_TABLE(of, i2c_gpio_dt_ids);
> +#endif
> +
>  static struct platform_driver i2c_gpio_driver = {
>  	.driver		= {
>  		.name	= "i2c-gpio",
>  		.owner	= THIS_MODULE,
> +		.of_match_table	= of_match_ptr(i2c_gpio_dt_ids),
>  	},
>  	.probe		= i2c_gpio_probe,
>  	.remove		= __devexit_p(i2c_gpio_remove),
> -- 
> 1.7.7
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Jean-Christophe PLAGNIOL-VILLARD]

On 23:09 Mon 13 Feb     , Ben Dooks wrote:
> On Thu, Feb 09, 2012 at 03:25:05AM +0100, Jean-Christophe PLAGNIOL-VILLARD wrote:
> > Signed-off-by: Jean-Christophe PLAGNIOL-VILLARD <plagnioj@jcrosoft.com>
> > Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
> > Cc: linux-i2c@vger.kernel.org
> > Cc: devicetree-discuss@lists.ozlabs.org
> > ---
> > v2:
> > 
> > 	use devm
> > 	update DT bindings to i2c-gpio and sda-open-drain...
> > 	update against "of: introduce helper to manage boolean"
> > 
> > Best Regards,
> > J.
> >  .../devicetree/bindings/gpio/gpio_i2c.txt          |   32 +++++++
> >  drivers/i2c/busses/i2c-gpio.c                      |   95 +++++++++++++++----
> >  2 files changed, 107 insertions(+), 20 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..9710ed2
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/gpio/gpio_i2c.txt
> > @@ -0,0 +1,32 @@
> > +Device-Tree bindings for i2c gpio driver
> > +
> > +Required properties:
> > +	- compatible = "i2c-gpio";
> > +	- gpios: sda and scl gpio
> > +
> > +
> > +Optional properties:
> > +	- i2c-gpio,sda-open-drain: sda as open drain
> > +	- i2c-gpio,scl-open-drain: scl as open drain
> > +	- i2c-gpio,scl-output-only: scl as output only
> > +	- udelay: delay between GPIO operations (may depend on each platform)
> > +	- timeout: timeout to get data (ms)
> > +
> > +Example nodes:
> > +
> > +i2c-gpio@0 {
> > +	compatible = "i2c-gpio";
> > +	gpios = <&pioA 23 0 /* sda */
> > +		 &pioA 24 0 /* scl */
> > +		>;
> > +	i2c-gpio,sda-open-drain;
> > +	i2c-gpio,scl-open-drain;
> > +	udelay = <2>;		/* ~100 kHz */
> > +	#address-cells = <1>;
> > +	#size-cells = <0>;
> > +
> > +	rv3029c2@56 {
> > +		compatible = "rv3029c2";
> > +		reg = <0x56>;
> > +	};
> > +};
> > diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> > index a651779..d7032c1 100644
> > --- a/drivers/i2c/busses/i2c-gpio.c
> > +++ b/drivers/i2c/busses/i2c-gpio.c
> > @@ -14,8 +14,15 @@
> >  #include <linux/module.h>
> >  #include <linux/slab.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/gpio.h>
> > +#include <linux/of_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 +85,63 @@ static int i2c_gpio_getscl(void *data)
> >  	return gpio_get_value(pdata->scl_pin);
> >  }
> >  
> > +static int __devinit of_i2c_gpio_probe(struct device_node *np,
> > +			     struct i2c_gpio_platform_data *pdata)
> > +{
> > +	u32 reg;
> > +
> > +	if (of_gpio_count(np) < 2)
> > +		return -ENODEV;
> 
> Hmm, there's no error print and unless updated recently I think the
> driver core will not produce any warnings if ->probe returns an
> -ENODEV. I would prefer to see this printing a warning as for the
> case below if an GPIO fails to be found.
> 
> Maybe return -EINVAL as it is not a valid configuration.
> 
> > +
> > +	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) {
> > +		pr_err("%s: invalid GPIO pins, sda=%d/scl=%d\n",
> > +		       np->full_name, pdata->sda_pin, pdata->scl_pin);
> > +		return -ENODEV;
> > +	}
> > +
> > +	if (of_property_read_u32(np, "udelay", &reg))
> > +		pdata->udelay = reg;
> 
> This sounds like a "linux" specific property. I would think about
> giving it a name like frequency, or ask Grant if we have a base set
> of properties that a i2c controller should think about implementing.
as example in the doc it's the delay between each gpio operations and it's
platform specific. put frequency will assume you specify the frequency of the
bus you want but we can not calculate here (platform specific).
> 
> > +	if (of_property_read_u32(np, "timeout", &reg))
> > +		pdata->timeout = msecs_to_jiffies(reg);
> > +
> > +	pdata->sda_is_open_drain =
> > +		!!of_property_read_bool(np, "i2c-gpio,sda-open-drain");
> > +	pdata->scl_is_open_drain =
> > +		!!of_property_read_bool(np, "i2c-gpio,scl-open-drain");
> > +	pdata->scl_is_output_only =
> > +		!!of_property_read_bool(np, "i2c-gpio,scl-output-only");
> 
> I hate !!, why does of_property_read_bool() not return a bool? Also,
> why can't I find of_property_read_bool() in include/linux/* ?
because it's an other patch
> 
> Also, does of_property_read_bool() consitently return a useful value if
> the property does not exist, or is not readable? I am most concerned
> with the behaviour if one of these properties has un-parsable contents.
it's use read_u32

and return false if read_u32 return -EINVAL

Best Regards,
J.