| Message ID | 1347958949-5598-1-git-send-email-voice.shen@atmel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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; >
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; >> > >
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); ?
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); > > ?
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); >> >> ? >
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); >>> >>> ? >> >
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.
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.
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
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;
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(-)