i2c/gpio: add DT support
Subsystems (6)
PATCH v2 1/4 i2c/gpio: add DT support
---
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
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +32 -0
@@ -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 at 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 at 56 { + compatible = "rv3029c2"; + reg = <0x56>; + }; +};
drivers/i2c/busses/i2c-gpio.c +75 -20
154 lines not on this page · raw message
Signature · 1 line
1.7.7
PATCH v2 2/4 ARM: at91: sam9g20 add i2c DT support
for now on use i2c-gpio driver on the same pin as the hardware IP
---
v2:
update DT bindings
Best Regards,
J.
arch/arm/boot/dts/at91sam9g20.dtsi | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
arch/arm/boot/dts/at91sam9g20.dtsi +13 -0
@@ -220,6 +220,19 @@ status = "disabled"; }; + i2c-gpio at 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"; + }; + }; }; };
Signature · 1 line
1.7.7
PATCH 3/4 ARM: at91: usb_a9g20 add DT i2c support
In reply to Jean-Christophe PLAGNIOL-VILLARDuse i2c-gpio and enable rv3029 RTC
enable the in the sam9g20 defconfig
--- arch/arm/boot/dts/usb_a9g20.dts | 9 +++++++++ arch/arm/configs/at91sam9g20_defconfig | 3 +++ 2 files changed, 12 insertions(+), 0 deletions(-)
arch/arm/boot/dts/usb_a9g20.dts +9 -0
@@ -121,6 +121,15 @@ }; + i2c-gpio at 0 { + status = "okay"; + + rv3029c2 at 56 { + compatible = "rv3029c2"; + reg = <0x56>; + }; + }; + }; }; };
arch/arm/configs/at91sam9g20_defconfig +3 -0
@@ -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
Signature · 1 line
1.7.7
PATCH v2 4/4 ARM: at91: sam9g45 add i2c DT support
In reply to Jean-Christophe PLAGNIOL-VILLARDfor now on use i2c-gpio driver on the same pin as the hardware IP
---
v2:
update DT bindings
Best Regards,
J.
arch/arm/boot/dts/at91sam9g45.dtsi | 13 +++++++++++++ 1 files changed, 13 insertions(+), 0 deletions(-)
arch/arm/boot/dts/at91sam9g45.dtsi +13 -0
@@ -211,6 +211,19 @@ status = "disabled"; }; + i2c-gpio at 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"; + }; + }; }; };
Signature · 1 line
1.7.7
PATCH v2 1/4 i2c/gpio: add DT support
In reply to Jean-Christophe PLAGNIOL-VILLARDOn 09.02.2012 03:25, Jean-Christophe PLAGNIOL-VILLARD wrote:
Hi!
Quoted 29 lines
Cc Nicolas Ferre <[email protected]>Cc linux-i2c-u79uwXL29TY76Z2rM5mHXA at public.gmane.orgCc devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ at 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.txtDocumentation/devicetree/bindings/gpio/gpio_i2c.txt new file +5
-0@@ -0,0 +1,32 @@ +Device-Tree bindings for i2c gpio driver + +Required properties: + - compatible = "i2c-gpio"; + - gpios: sda and scl gpio
Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 13
Quoted 10 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +8
-0+ + +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:
Quoted 59 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +19
-0+ +Example nodes: + +i2c-gpio at 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 at 56 { + compatible = "rv3029c2"; + reg = <0x56>; + }; +};drivers/i2c/busses/i2c-gpio.c +23
-1@@ -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 ;
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 112
Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +7
-0+ } + +if (of_property_read_u32 (np,"udelay" , ®)) + pdata->udelay = reg; + +if (of_property_read_u32 (np,"timeout" , ®)) + 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
Quoted 116 lines
drivers/i2c/busses/i2c-gpio.c +27
-10+ + 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)); + }drivers/i2c/busses/i2c-gpio.c +18
-9ret =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),
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 48 lines · Karol Lewandowski
On 09.02.2012 03:25, Jean-Christophe PLAGNIOL-VILLARD wrote:
Hi!
Quoted 29 lines
Cc Nicolas Ferre <[email protected]>Cc linux-i2c-u79uwXL29TY76Z2rM5mHXA at public.gmane.orgCc devicetree-discuss-uLR06cmDAlY/bJ5BZ2RsiQ at 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.txtDocumentation/devicetree/bindings/gpio/gpio_i2c.txt new file +5
-0@@ -0,0 +1,32 @@ +Device-Tree bindings for i2c gpio driver + +Required properties: + - compatible = "i2c-gpio"; + - gpios: sda and scl gpioDocumentation/devicetree/bindings/gpio/gpio_i2c.txt line 13
Quoted 10 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +8
-0+ + +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).
Quoted 5 lines
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:
timeout are usualy in ms I don't really see the need of -ms or so
Best Regards,
J.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · Karol Lewandowski
Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 13
Quoted 2 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2
-0+ - 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).
Quoted 6 lines
Quoted 5 lines
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:
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 9 lines · Russell King - ARM Linux
Quoted 8 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 7 lines · Karol Lewandowski
Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 13
Quoted 2 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2
-0+ - 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).
Quoted 8 lines
Quoted 6 lines
Quoted 5 lines
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:
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 13
Quoted 2 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 5 lines · Russell King - ARM Linux
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.
Signature · 1 line
Jean Delvare
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 7 lines · Jean Delvare
Quoted 5 lines · Russell King - ARM Linux
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.
Quoted 6 lines
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'.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 22 lines · Russell King - ARM Linux
Quoted 7 lines · Jean Delvare
Quoted 5 lines · Russell King - ARM Linux
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.
Quoted 6 lines
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.
Quoted 8 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 39 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 22 lines · Russell King - ARM Linux
Quoted 7 lines · Jean Delvare
Quoted 5 lines · Russell King - ARM Linux
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.
Quoted 6 lines
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.
Quoted 8 lines
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)
Quoted 6 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
In reply to Russell King - ARM LinuxQuoted 33 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Documentation/devicetree/bindings/gpio/gpio_i2c.txt line 13
Quoted 2 lines
Documentation/devicetree/bindings/gpio/gpio_i2c.txt new file +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 4 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 34 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 33 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Review
Quoted 2 lines
(quoted diff) +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 5 lines
Quoted 4 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Review
Quoted 2 lines
(quoted diff) +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 53 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Review
Quoted 2 lines
(quoted diff) +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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:
Quoted 6 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Review
Quoted 2 lines
(quoted diff) +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat 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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 77 lines · Russell King - ARM Linux
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Review
Quoted 2 lines
(quoted diff) +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat all in this reply so the NACK is no revelentLET 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.
Quoted 6 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 83 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 82 lines · Russell King - ARM Linux
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Review
Quoted 2 lines
(quoted diff) +2
-0+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat all in this reply so the NACK is no revelentLET 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?
Quoted 7 lines
Quoted 1 line
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.
Quoted 1 line
- 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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 92 lines · Russell King - ARM Linux
Quoted 83 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 82 lines · Russell King - ARM Linux
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
Quoted 2 lines
+ - 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:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat all in this reply so the NACK is no revelentLET 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?
Quoted 7 lines
Quoted 1 line
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
Quoted 8 lines
THIS IS IN MILLISECONDS.
Quoted 1 line
- 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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 102 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 92 lines · Russell King - ARM Linux
Quoted 83 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 82 lines · Russell King - ARM Linux
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 11 lines · Karol Lewandowski
+ - 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:Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat all in this reply so the NACK is no revelentLET 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?
Quoted 7 lines
Quoted 1 line
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-milisecondQuoted 8 lines
THIS IS IN MILLISECONDS.
Quoted 1 line
- 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.
Quoted 6 lines
I said
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 103 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 102 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 100 lines · Russell King - ARM Linux
Quoted 83 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 82 lines · Russell King - ARM Linux
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · Russell King - ARM Linux
Quoted 12 lines · Jean-Christophe PLAGNIOL-VILLARD
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:Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
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 soWhich 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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat all in this reply so the NACK is no revelentLET 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?
Quoted 7 lines
Quoted 1 line
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-milisecondTHIS IS IN MILLISECONDS.
Quoted 1 line
- 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.
Quoted 8 lines
Quoted 6 lines
I said
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
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.
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 110 lines · Russell King - ARM Linux
Quoted 109 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 108 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 92 lines · Russell King - ARM Linux
Quoted 83 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 82 lines · Russell King - ARM Linux
Quoted 56 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 55 lines · Russell King - ARM Linux
Quoted 38 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 37 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 36 lines · Russell King - ARM Linux
Quoted 14 lines · Jean-Christophe PLAGNIOL-VILLARD
Quoted 13 lines · 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:Quoted 8 lines
Quoted 4 lines
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 soWhich 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.
Quoted 8 lines
Quoted 2 lines
Quoted 1 line
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.
Quoted 7 lines
Quoted 4 lines
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_Sat all in this reply so the NACK is no revelentLET 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?
Quoted 7 lines
Quoted 1 line
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-milisecondQuoted 8 lines
THIS IS IN MILLISECONDS.
Quoted 1 line
- 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
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
Quoted 1 line
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
Quoted 7 lines
Quoted 2 lines
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.
PATCH v2 1/4 i2c/gpio: add DT support
In reply to Jean-Christophe PLAGNIOL-VILLARDQuoted 77 lines · Jean-Christophe PLAGNIOL-VILLARD
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>Cc Nicolas Ferre <[email protected]>Cc linux-i2c at vger.kernel.orgCc devicetree-discuss at 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.txtDocumentation/devicetree/bindings/gpio/gpio_i2c.txt new file +32
-0@@ -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 at 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 at 56 { + compatible = "rv3029c2"; + reg = <0x56>; + }; +};drivers/i2c/busses/i2c-gpio.c +8
-1@@ -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)
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 94
Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +7
-0@@ -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.
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 106
Quoted 12 lines
drivers/i2c/busses/i2c-gpio.c +12
-0+ + 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" , ®)) + 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.
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 116
Quoted 9 lines
drivers/i2c/busses/i2c-gpio.c +9
-0+if (of_property_read_u32 (np,"timeout" , ®)) + 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.
Quoted 26 lines
drivers/i2c/busses/i2c-gpio.c +10
-8+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 );
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 143
Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +8
-2-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.
drivers/i2c/busses/i2c-gpio.c static int __devinit i2c_gpio_probe(struct platform_device *pdev) · line 192
Quoted 9 lines
drivers/i2c/busses/i2c-gpio.c +2
-0+ } 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?
Quoted 70 lines
drivers/i2c/busses/i2c-gpio.c +17
-9/* * 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),Signature · 6 lines
1.7.7 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo at vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
PATCH v2 1/4 i2c/gpio: add DT support
Quoted 96 lines · Ben Dooks
Quoted 77 lines · Jean-Christophe PLAGNIOL-VILLARD
Signed-off-by Jean-Christophe PLAGNIOL-VILLARD <[email protected]>Cc Nicolas Ferre <[email protected]>Cc linux-i2c at vger.kernel.orgCc devicetree-discuss at 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.txtDocumentation/devicetree/bindings/gpio/gpio_i2c.txt new file +32
-0@@ -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 at 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 at 56 { + compatible = "rv3029c2"; + reg = <0x56>; + }; +};drivers/i2c/busses/i2c-gpio.c +8
-1@@ -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)drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 94
Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +7
-0@@ -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
-EINVALas it is not a valid configuration.
Quoted 16 lines
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 106
Quoted 12 lines
drivers/i2c/busses/i2c-gpio.c +12
-0+ + 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" , ®)) + 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).
Quoted 13 lines
drivers/i2c/busses/i2c-gpio.c static int i2c_gpio_getscl(void *data) · line 116
Quoted 10 lines
drivers/i2c/busses/i2c-gpio.c +9
-0+if (of_property_read_u32 (np,"timeout" , ®)) + 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 findof_property_read_bool()ininclude/linux/*?
because it's an other patch
Quoted 4 lines
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.