[PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Bo Shen]

When i2c-gpio node number is greater than 1, the second can not sucessfully
regiter

Using alias method to get the pdev->id, or else the second device will use
the same id which will cause fail to register

Signed-off-by: Bo Shen <voice.shen@atmel.com>
---
 Documentation/devicetree/bindings/i2c/gpio-i2c.txt |   46 +++++++++++++++++++-
 drivers/i2c/busses/i2c-gpio.c                      |    1 +
 2 files changed, 46 insertions(+), 1 deletion(-)

diff --git a/Documentation/devicetree/bindings/i2c/gpio-i2c.txt b/Documentation/devicetree/bindings/i2c/gpio-i2c.txt
index 4f8ec94..e76dd00 100644
--- a/Documentation/devicetree/bindings/i2c/gpio-i2c.txt
+++ b/Documentation/devicetree/bindings/i2c/gpio-i2c.txt
@@ -1,4 +1,7 @@
-Device-Tree bindings for i2c gpio driver
+* Device-Tree bindings for i2c gpio driver
+
+Optional alias:
+	- i2c-gpio: used to get id, when more than one i2c-gpio in dts file
 
 Required properties:
 	- compatible = "i2c-gpio";
@@ -30,3 +33,44 @@ i2c@0 {
 		reg = <0x56>;
 	};
 };
+
+
+Example nodes with alias:
+
+aliases {
+	i2c-gpio0 = &i2c0;
+	i2c-gpio1 = &i2c1;
+};
+
+i2c0: i2c@0 {
+	compatible = "i2c-gpio";
+	gpios = <&pioA 23 0 /* sda */
+		 &pioA 24 0 /* scl */
+		>;
+	i2c-gpio,sda-open-drain;
+	i2c-gpio,scl-open-drain;
+	i2c-gpio,delay-us = <2>;	/* ~100 kHz */
+	#address-cells = <1>;
+	#size-cells = <0>;
+
+	rv3029c2@56 {
+		compatible = "rv3029c2";
+		reg = <0x56>;
+	};
+};
+
+i2c1: i2c@0 {
+	compatible = "i2c-gpio";
+	gpios = <&pioA 30 0 /* sda */
+		 &pioA 31 0 /* scl */
+		>;
+	i2c-gpio,sda-open-drain;
+	i2c-gpio,scl-open-drain;
+	i2c-gpio,delay-us = <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 e62d2d9..051fbb3 100644
--- a/drivers/i2c/busses/i2c-gpio.c
+++ b/drivers/i2c/busses/i2c-gpio.c
@@ -136,6 +136,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
 		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
 		if (ret)
 			return ret;
+		pdev->id = of_alias_get_id(pdev->dev.of_node, "i2c-gpio");
 	} else {
 		if (!pdev->dev.platform_data)
 			return -ENXIO;
-- 
1.7.9.5

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Stephen Warren]

On 09/18/2012 03:02 AM, Bo Shen wrote:
> When i2c-gpio node number is greater than 1, the second can not sucessfully
> regiter
> 
> Using alias method to get the pdev->id, or else the second device will use
> the same id which will cause fail to register

pdev->id shouldn't be used at all with device tree. Why does
registration fail without this change?

> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
> index e62d2d9..051fbb3 100644
> --- a/drivers/i2c/busses/i2c-gpio.c
> +++ b/drivers/i2c/busses/i2c-gpio.c
> @@ -136,6 +136,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>  		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
>  		if (ret)
>  			return ret;
> +		pdev->id = of_alias_get_id(pdev->dev.of_node, "i2c-gpio");
>  	} else {
>  		if (!pdev->dev.platform_data)
>  			return -ENXIO;
> 

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Bo Shen]

Hi Stephen Warren,

On 9/18/2012 22:51, Stephen Warren wrote:
> On 09/18/2012 03:02 AM, Bo Shen wrote:
>> When i2c-gpio node number is greater than 1, the second can not sucessfully
>> regiter
>>
>> Using alias method to get the pdev->id, or else the second device will use
>> the same id which will cause fail to register
>
> pdev->id shouldn't be used at all with device tree. Why does
> registration fail without this change?

I add the debug info, and it give the following error without this patch.

--<8----------------------------------
adap->name = i2c-gpio-1
i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
adap->name = i2c-gpio-1
i2c-gpio: probe of i2c.3 failed with error -16
-->8----------------------------------

With this patch, it successfully registered.
--<8----------------------------------
adap->name = i2c-gpio0
i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
adap->name = i2c-gpio1
i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
-->8----------------------------------

>
>> diff --git a/drivers/i2c/busses/i2c-gpio.c b/drivers/i2c/busses/i2c-gpio.c
>> index e62d2d9..051fbb3 100644
>> --- a/drivers/i2c/busses/i2c-gpio.c
>> +++ b/drivers/i2c/busses/i2c-gpio.c
>> @@ -136,6 +136,7 @@ static int __devinit i2c_gpio_probe(struct platform_device *pdev)
>>   		ret = of_i2c_gpio_probe(pdev->dev.of_node, pdata);
>>   		if (ret)
>>   			return ret;
>> +		pdev->id = of_alias_get_id(pdev->dev.of_node, "i2c-gpio");
>>   	} else {
>>   		if (!pdev->dev.platform_data)
>>   			return -ENXIO;
>>
>
>

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Stephen Warren]

On 09/18/2012 07:15 PM, Bo Shen wrote:
> Hi Stephen Warren,
> 
> On 9/18/2012 22:51, Stephen Warren wrote:
>> On 09/18/2012 03:02 AM, Bo Shen wrote:
>>> When i2c-gpio node number is greater than 1, the second can not
>>> sucessfully
>>> regiter
>>>
>>> Using alias method to get the pdev->id, or else the second device
>>> will use
>>> the same id which will cause fail to register
>>
>> pdev->id shouldn't be used at all with device tree. Why does
>> registration fail without this change?
> 
> I add the debug info, and it give the following error without this patch.
> 
> --<8----------------------------------
> adap->name = i2c-gpio-1
> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> adap->name = i2c-gpio-1
> i2c-gpio: probe of i2c.3 failed with error -16
> -->8----------------------------------
> 
> With this patch, it successfully registered.
> --<8----------------------------------
> adap->name = i2c-gpio0
> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> adap->name = i2c-gpio1
> i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
> -->8----------------------------------

Yes, that explains why the registration fails, but not why this patch is
the correct solution to the problem.

The problematic code appears to be:

snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);

Instead, I think that should be something more like:

adap->name = dev_name(&pdev->dev);

or perhaps:

if (pdev->dev.of_node)
  /* named will be based on DT node name */
  adap->name = dev_name(&pdev->dev);
else
  snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);

?

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Jean Delvare]

On Wed, 19 Sep 2012 09:28:04 -0600, Stephen Warren wrote:
> On 09/18/2012 07:15 PM, Bo Shen wrote:
> > I add the debug info, and it give the following error without this patch.
> > 
> > --<8----------------------------------
> > adap->name = i2c-gpio-1
> > i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> > adap->name = i2c-gpio-1
> > i2c-gpio: probe of i2c.3 failed with error -16
> > -->8----------------------------------
> > 
> > With this patch, it successfully registered.
> > --<8----------------------------------
> > adap->name = i2c-gpio0
> > i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> > adap->name = i2c-gpio1
> > i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
> > -->8----------------------------------
> 
> Yes, that explains why the registration fails, but not why this patch is
> the correct solution to the problem.
> 
> The problematic code appears to be:
> 
> snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
> 
> Instead, I think that should be something more like:
> 
> adap->name = dev_name(&pdev->dev);

strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name))

if anything, as adap->name is a buffer, not a pointer.

> or perhaps:
> 
> if (pdev->dev.of_node)
>   /* named will be based on DT node name */
>   adap->name = dev_name(&pdev->dev);
> else
>   snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
> 
> ?

-- 
Jean Delvare

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Bo Shen]

Hi Jean Delvare,

On 9/19/2012 23:54, Jean Delvare wrote:
> On Wed, 19 Sep 2012 09:28:04 -0600, Stephen Warren wrote:
>> On 09/18/2012 07:15 PM, Bo Shen wrote:
>>> I add the debug info, and it give the following error without this patch.
>>>
>>> --<8----------------------------------
>>> adap->name = i2c-gpio-1
>>> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
>>> adap->name = i2c-gpio-1
>>> i2c-gpio: probe of i2c.3 failed with error -16
>>> -->8----------------------------------
>>>
>>> With this patch, it successfully registered.
>>> --<8----------------------------------
>>> adap->name = i2c-gpio0
>>> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
>>> adap->name = i2c-gpio1
>>> i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
>>> -->8----------------------------------
>>
>> Yes, that explains why the registration fails, but not why this patch is
>> the correct solution to the problem.
>>
>> The problematic code appears to be:
>>
>> snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
>>
>> Instead, I think that should be something more like:
>>
>> adap->name = dev_name(&pdev->dev);
>
> strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name))

I am sorry for late. I have tested with DT, it works.

Please send this patch to fix the issue.
Thanks.

> if anything, as adap->name is a buffer, not a pointer.
>
>> or perhaps:
>>
>> if (pdev->dev.of_node)
>>    /* named will be based on DT node name */
>>    adap->name = dev_name(&pdev->dev);
>> else
>>    snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
>>
>> ?
>

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Bo Shen]

Hi Jean Delvare,

On 10/10/2012 10:54, Bo Shen wrote:
>
> Hi Jean Delvare,
>
> On 9/19/2012 23:54, Jean Delvare wrote:
>> On Wed, 19 Sep 2012 09:28:04 -0600, Stephen Warren wrote:
>>> On 09/18/2012 07:15 PM, Bo Shen wrote:
>>>> I add the debug info, and it give the following error without this
>>>> patch.
>>>>
>>>> --<8----------------------------------
>>>> adap->name = i2c-gpio-1
>>>> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
>>>> adap->name = i2c-gpio-1
>>>> i2c-gpio: probe of i2c.3 failed with error -16
>>>> -->8----------------------------------
>>>>
>>>> With this patch, it successfully registered.
>>>> --<8----------------------------------
>>>> adap->name = i2c-gpio0
>>>> i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
>>>> adap->name = i2c-gpio1
>>>> i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
>>>> -->8----------------------------------
>>>
>>> Yes, that explains why the registration fails, but not why this patch is
>>> the correct solution to the problem.
>>>
>>> The problematic code appears to be:
>>>
>>> snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
>>>
>>> Instead, I think that should be something more like:
>>>
>>> adap->name = dev_name(&pdev->dev);
>>
>> strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name))
 >> if anything, as adap->name is a buffer, not a pointer.
>
> I am sorry for late. I have tested with DT, it works.
>
> Please send this patch to fix the issue.
> Thanks.

Ping?

Will you send this patch? Or, I send this patch and add your S.O.B, 
which do you prefer?

BRs,
Bo Shen

>>
>>> or perhaps:
>>>
>>> if (pdev->dev.of_node)
>>>    /* named will be based on DT node name */
>>>    adap->name = dev_name(&pdev->dev);
>>> else
>>>    snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
>>>
>>> ?
>>
>

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Jean-Christophe PLAGNIOL-VILLARD]

On 10:59 Mon 15 Oct     , Bo Shen wrote:
> Hi Jean Delvare,
> 
> On 10/10/2012 10:54, Bo Shen wrote:
> >
> >Hi Jean Delvare,
> >
> >On 9/19/2012 23:54, Jean Delvare wrote:
> >>On Wed, 19 Sep 2012 09:28:04 -0600, Stephen Warren wrote:
> >>>On 09/18/2012 07:15 PM, Bo Shen wrote:
> >>>>I add the debug info, and it give the following error without this
> >>>>patch.
> >>>>
> >>>>--<8----------------------------------
> >>>>adap->name = i2c-gpio-1
> >>>>i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> >>>>adap->name = i2c-gpio-1
> >>>>i2c-gpio: probe of i2c.3 failed with error -16
> >>>>-->8----------------------------------
> >>>>
> >>>>With this patch, it successfully registered.
> >>>>--<8----------------------------------
> >>>>adap->name = i2c-gpio0
> >>>>i2c-gpio i2c.2: using pins 30 (SDA) and 31 (SCL)
> >>>>adap->name = i2c-gpio1
> >>>>i2c-gpio i2c.3: using pins 90 (SDA) and 91 (SCL)
> >>>>-->8----------------------------------
> >>>
> >>>Yes, that explains why the registration fails, but not why this patch is
> >>>the correct solution to the problem.
> >>>
> >>>The problematic code appears to be:
> >>>
> >>>snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
> >>>
> >>>Instead, I think that should be something more like:
> >>>
> >>>adap->name = dev_name(&pdev->dev);
> >>
> >>strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name))
> >> if anything, as adap->name is a buffer, not a pointer.
> >
> >I am sorry for late. I have tested with DT, it works.
> >
> >Please send this patch to fix the issue.
> >Thanks.
> 
> Ping?
> 
> Will you send this patch? Or, I send this patch and add your S.O.B,
> which do you prefer?

do it

Best Regards,
J.

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Jean Delvare]

On Mon, 15 Oct 2012 10:59:19 +0800, Bo Shen wrote:
> On 10/10/2012 10:54, Bo Shen wrote:
> > On 9/19/2012 23:54, Jean Delvare wrote:
> >> On Wed, 19 Sep 2012 09:28:04 -0600, Stephen Warren wrote:
> >>> The problematic code appears to be:
> >>>
> >>> snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
> >>>
> >>> Instead, I think that should be something more like:
> >>>
> >>> adap->name = dev_name(&pdev->dev);
> >>
> >> strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name))
>  >> if anything, as adap->name is a buffer, not a pointer.
> >
> > I am sorry for late. I have tested with DT, it works.
> >
> > Please send this patch to fix the issue.
> > Thanks.
> 
> Ping?
> 
> Will you send this patch?

Which patch, please? On September 18th, you sent a first version.
Stephen Warren asked you for a change, but you never sent version two
of the patch including the requested change. So don't be surprised if
your patch never got applied.

> Or, I send this patch and add your S.O.B, which do you prefer?

I prefer that you send patch v2 and let the interested parties review
and apply it. Do not add S-o-B for anyone else, ever.

Note that i2c-gpio is normally handled by Wolfram Sang, not me.

-- 
Jean Delvare

Re: [PATCH] i2c: i2c-gpio: fix issue when DT node greater than 1

[Bo Shen]

Hi Jean Delvare,

On 10/15/2012 14:49, Jean Delvare wrote:
> On Mon, 15 Oct 2012 10:59:19 +0800, Bo Shen wrote:
>> On 10/10/2012 10:54, Bo Shen wrote:
>>> On 9/19/2012 23:54, Jean Delvare wrote:
>>>> On Wed, 19 Sep 2012 09:28:04 -0600, Stephen Warren wrote:
>>>>> The problematic code appears to be:
>>>>>
>>>>> snprintf(adap->name, sizeof(adap->name), "i2c-gpio%d", pdev->id);
>>>>>
>>>>> Instead, I think that should be something more like:
>>>>>
>>>>> adap->name = dev_name(&pdev->dev);
>>>>
>>>> strlcpy(adap->name, dev_name(&pdev->dev), sizeof(adap->name))
>>   >> if anything, as adap->name is a buffer, not a pointer.
>>>
>>> I am sorry for late. I have tested with DT, it works.
>>>
>>> Please send this patch to fix the issue.
>>> Thanks.
>>
>> Ping?
>>
>> Will you send this patch?
>
> Which patch, please? On September 18th, you sent a first version.
> Stephen Warren asked you for a change, but you never sent version two
> of the patch including the requested change. So don't be surprised if
> your patch never got applied.
>

I am sorry. I take a long vacation, so, I didnot send out version two.

>> Or, I send this patch and add your S.O.B, which do you prefer?
>
> I prefer that you send patch v2 and let the interested parties review
> and apply it. Do not add S-o-B for anyone else, ever.

OK. I will send out v2 as soon as possible.

>
> Note that i2c-gpio is normally handled by Wolfram Sang, not me.

Thanks for your information.

BRs,
Bo Shen