Patchwork i2c: sirf: get the i2c pin group by pinctrl api

login
register
mail settings
Submitter Barry Song
Date March 18, 2013, 7:22 a.m.
Message ID <1363591361-5992-1-git-send-email-Barry.Song@csr.com>
Download mbox | patch
Permalink /patch/228381/
State Rejected
Headers show

Comments

Barry Song - March 18, 2013, 7:22 a.m.
From: Barry Song <Baohua.Song@csr.com>

hardcode set i2c pin group to i2c function before, here we
move to use standard pinctrl API to get pins of the group.

Signed-off-by: Barry Song <Baohua.Song@csr.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
 drivers/i2c/busses/i2c-sirf.c |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)
Poddar, Sourav - March 18, 2013, 8:01 a.m.
Hi,
On Monday 18 March 2013 12:52 PM, Barry Song wrote:
> From: Barry Song<Baohua.Song@csr.com>
>
> hardcode set i2c pin group to i2c function before, here we
> move to use standard pinctrl API to get pins of the group.
>
> Signed-off-by: Barry Song<Baohua.Song@csr.com>
> Cc: Linus Walleij<linus.walleij@linaro.org>
> ---
>   drivers/i2c/busses/i2c-sirf.c |    9 +++++++++
>   1 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
> index 5a7ad24..dd4004e 100644
> --- a/drivers/i2c/busses/i2c-sirf.c
> +++ b/drivers/i2c/busses/i2c-sirf.c
> @@ -16,6 +16,7 @@
>   #include<linux/clk.h>
>   #include<linux/err.h>
>   #include<linux/io.h>
> +#include<linux/pinctrl/consumer.h>
>
>   #define SIRFSOC_I2C_CLK_CTRL		0x00
>   #define SIRFSOC_I2C_STATUS		0x0C
> @@ -265,6 +266,7 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
>   	struct i2c_adapter *adap;
>   	struct resource *mem_res;
>   	struct clk *clk;
> +	struct pinctrl *pinctrl;
>   	int bitrate;
>   	int ctrl_speed;
>   	int irq;
> @@ -272,6 +274,12 @@ static int i2c_sirfsoc_probe(struct platform_device *pdev)
>   	int err;
>   	u32 regval;
>
> +	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
> +	if (IS_ERR(pinctrl)) {
> +		err = PTR_ERR(pinctrl);
> +		goto failed_pin;
> +	}
> +
I think,  you should also add an "EPROBE_DEFER" check here ?
>   	clk = clk_get(&pdev->dev, NULL);
>   	if (IS_ERR(clk)) {
>   		err = PTR_ERR(clk);
> @@ -385,6 +393,7 @@ err_clk_en:
>   err_clk_prep:
>   	clk_put(clk);
>   err_get_clk:
> +failed_pin:
>   	return err;
>   }
>

--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Barry Song - March 18, 2013, 8:23 a.m.
2013/3/18 Sourav Poddar <sourav.poddar@ti.com>:
> Hi,
>
> On Monday 18 March 2013 12:52 PM, Barry Song wrote:
>>
>> From: Barry Song<Baohua.Song@csr.com>
>>
>> hardcode set i2c pin group to i2c function before, here we
>> move to use standard pinctrl API to get pins of the group.
>>
>> Signed-off-by: Barry Song<Baohua.Song@csr.com>
>> Cc: Linus Walleij<linus.walleij@linaro.org>
>> ---
>>   drivers/i2c/busses/i2c-sirf.c |    9 +++++++++
>>   1 files changed, 9 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
>> index 5a7ad24..dd4004e 100644
>> --- a/drivers/i2c/busses/i2c-sirf.c
>> +++ b/drivers/i2c/busses/i2c-sirf.c
>> @@ -16,6 +16,7 @@
>>   #include<linux/clk.h>
>>   #include<linux/err.h>
>>   #include<linux/io.h>
>> +#include<linux/pinctrl/consumer.h>
>>
>>   #define SIRFSOC_I2C_CLK_CTRL          0x00
>>   #define SIRFSOC_I2C_STATUS            0x0C
>> @@ -265,6 +266,7 @@ static int i2c_sirfsoc_probe(struct platform_device
>> *pdev)
>>         struct i2c_adapter *adap;
>>         struct resource *mem_res;
>>         struct clk *clk;
>> +       struct pinctrl *pinctrl;
>>         int bitrate;
>>         int ctrl_speed;
>>         int irq;
>> @@ -272,6 +274,12 @@ static int i2c_sirfsoc_probe(struct platform_device
>> *pdev)
>>         int err;
>>         u32 regval;
>>
>> +       pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>> +       if (IS_ERR(pinctrl)) {
>> +               err = PTR_ERR(pinctrl);
>> +               goto failed_pin;
>> +       }
>> +
>
> I think,  you should also add an "EPROBE_DEFER" check here ?

why would the driver require a probe retry since getting the pinctrl
has returned an error? before the driver executes, pinctrl driver has
run earlier and DT has been extended.

all people are using  IS_ERR(pinctrl) to check the ret of
devm_pinctrl_get_select_default.

>
>>         clk = clk_get(&pdev->dev, NULL);
>>         if (IS_ERR(clk)) {
>>                 err = PTR_ERR(clk);
>> @@ -385,6 +393,7 @@ err_clk_en:
>>   err_clk_prep:
>>         clk_put(clk);
>>   err_get_clk:
>> +failed_pin:
>>         return err;
>>   }

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Poddar, Sourav - March 18, 2013, 8:41 a.m.
Hi,
On Monday 18 March 2013 01:53 PM, Barry Song wrote:
> 2013/3/18 Sourav Poddar<sourav.poddar@ti.com>:
>> Hi,
>>
>> On Monday 18 March 2013 12:52 PM, Barry Song wrote:
>>> From: Barry Song<Baohua.Song@csr.com>
>>>
>>> hardcode set i2c pin group to i2c function before, here we
>>> move to use standard pinctrl API to get pins of the group.
>>>
>>> Signed-off-by: Barry Song<Baohua.Song@csr.com>
>>> Cc: Linus Walleij<linus.walleij@linaro.org>
>>> ---
>>>    drivers/i2c/busses/i2c-sirf.c |    9 +++++++++
>>>    1 files changed, 9 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
>>> index 5a7ad24..dd4004e 100644
>>> --- a/drivers/i2c/busses/i2c-sirf.c
>>> +++ b/drivers/i2c/busses/i2c-sirf.c
>>> @@ -16,6 +16,7 @@
>>>    #include<linux/clk.h>
>>>    #include<linux/err.h>
>>>    #include<linux/io.h>
>>> +#include<linux/pinctrl/consumer.h>
>>>
>>>    #define SIRFSOC_I2C_CLK_CTRL          0x00
>>>    #define SIRFSOC_I2C_STATUS            0x0C
>>> @@ -265,6 +266,7 @@ static int i2c_sirfsoc_probe(struct platform_device
>>> *pdev)
>>>          struct i2c_adapter *adap;
>>>          struct resource *mem_res;
>>>          struct clk *clk;
>>> +       struct pinctrl *pinctrl;
>>>          int bitrate;
>>>          int ctrl_speed;
>>>          int irq;
>>> @@ -272,6 +274,12 @@ static int i2c_sirfsoc_probe(struct platform_device
>>> *pdev)
>>>          int err;
>>>          u32 regval;
>>>
>>> +       pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>>> +       if (IS_ERR(pinctrl)) {
>>> +               err = PTR_ERR(pinctrl);
>>> +               goto failed_pin;
>>> +       }
>>> +
>> I think,  you should also add an "EPROBE_DEFER" check here ?
> why would the driver require a probe retry since getting the pinctrl
> has returned an error? before the driver executes, pinctrl driver has
> run earlier and DT has been extended.
>
There might be modules who need some early pinctrl muxing. Some
drivers might also use subsys_initcall instead of module_init. In such 
cases,
"EPROBE_DEFER" might be useful.
> all people are using  IS_ERR(pinctrl) to check the ret of
> devm_pinctrl_get_select_default.
>
Yes, we will keep using this check. DEFER check will be embedded inside 
this.
Something like below, which has been done for omap i2c..

+       dev->pins = devm_pinctrl_get_select_default(&pdev->dev);
+       if (IS_ERR(dev->pins)) {
+               if (PTR_ERR(dev->pins) == -EPROBE_DEFER)
+                       return -EPROBE_DEFER;
+
+               dev_warn(&pdev->dev, "did not get pins for i2c error: 
%li\n",
+                        PTR_ERR(dev->pins));
+               dev->pins = NULL;
+       }
+

>>>          clk = clk_get(&pdev->dev, NULL);
>>>          if (IS_ERR(clk)) {
>>>                  err = PTR_ERR(clk);
>>> @@ -385,6 +393,7 @@ err_clk_en:
>>>    err_clk_prep:
>>>          clk_put(clk);
>>>    err_get_clk:
>>> +failed_pin:
>>>          return err;
>>>    }
> -barry
~Sourav
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Barry Song - March 18, 2013, 8:54 a.m.
2013/3/18 Sourav Poddar <sourav.poddar@ti.com>:
> Hi,
>
> On Monday 18 March 2013 01:53 PM, Barry Song wrote:
>>
>> 2013/3/18 Sourav Poddar<sourav.poddar@ti.com>:
>>>
>>> Hi,
>>>
>>> On Monday 18 March 2013 12:52 PM, Barry Song wrote:
>>>>
>>>> From: Barry Song<Baohua.Song@csr.com>
>>>>
>>>> hardcode set i2c pin group to i2c function before, here we
>>>> move to use standard pinctrl API to get pins of the group.
>>>>
>>>> Signed-off-by: Barry Song<Baohua.Song@csr.com>
>>>> Cc: Linus Walleij<linus.walleij@linaro.org>
>>>> ---
>>>>    drivers/i2c/busses/i2c-sirf.c |    9 +++++++++
>>>>    1 files changed, 9 insertions(+), 0 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/busses/i2c-sirf.c
>>>> b/drivers/i2c/busses/i2c-sirf.c
>>>> index 5a7ad24..dd4004e 100644
>>>> --- a/drivers/i2c/busses/i2c-sirf.c
>>>> +++ b/drivers/i2c/busses/i2c-sirf.c
>>>> @@ -16,6 +16,7 @@
>>>>    #include<linux/clk.h>
>>>>    #include<linux/err.h>
>>>>    #include<linux/io.h>
>>>> +#include<linux/pinctrl/consumer.h>
>>>>
>>>>    #define SIRFSOC_I2C_CLK_CTRL          0x00
>>>>    #define SIRFSOC_I2C_STATUS            0x0C
>>>> @@ -265,6 +266,7 @@ static int i2c_sirfsoc_probe(struct platform_device
>>>> *pdev)
>>>>          struct i2c_adapter *adap;
>>>>          struct resource *mem_res;
>>>>          struct clk *clk;
>>>> +       struct pinctrl *pinctrl;
>>>>          int bitrate;
>>>>          int ctrl_speed;
>>>>          int irq;
>>>> @@ -272,6 +274,12 @@ static int i2c_sirfsoc_probe(struct platform_device
>>>> *pdev)
>>>>          int err;
>>>>          u32 regval;
>>>>
>>>> +       pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
>>>> +       if (IS_ERR(pinctrl)) {
>>>> +               err = PTR_ERR(pinctrl);
>>>> +               goto failed_pin;
>>>> +       }
>>>> +
>>>
>>> I think,  you should also add an "EPROBE_DEFER" check here ?
>>
>> why would the driver require a probe retry since getting the pinctrl
>> has returned an error? before the driver executes, pinctrl driver has
>> run earlier and DT has been extended.
>>
> There might be modules who need some early pinctrl muxing. Some
> drivers might also use subsys_initcall instead of module_init. In such
> cases,
> "EPROBE_DEFER" might be useful.

yes. that is right. here these is no this issue.
in my case, sirf pinctrl is initialized by arch_initcall, i2c probing
is handled by module_init.
that makes i2c is a later user of pinctrl.

>
>> all people are using  IS_ERR(pinctrl) to check the ret of
>> devm_pinctrl_get_select_default.
>>
> Yes, we will keep using this check. DEFER check will be embedded inside
> this.
> Something like below, which has been done for omap i2c..

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij - March 27, 2013, 10:20 a.m.
On Mon, Mar 18, 2013 at 8:22 AM, Barry Song <Barry.Song@csr.com> wrote:

> From: Barry Song <Baohua.Song@csr.com>
>
> hardcode set i2c pin group to i2c function before, here we
> move to use standard pinctrl API to get pins of the group.
>
> Signed-off-by: Barry Song <Baohua.Song@csr.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

NAK.

This is done by the device core as of commit
ab78029ecc347debbd737f06688d788bd9d60c1d
"drivers/pinctrl: grab default handles from device core".

You only need to fetch pinctrl handles in drivers if you're
using anything else than the default state.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Barry Song - March 27, 2013, 10:36 a.m.
2013/3/27 Linus Walleij <linus.walleij@linaro.org>:
> On Mon, Mar 18, 2013 at 8:22 AM, Barry Song <Barry.Song@csr.com> wrote:
>
>> From: Barry Song <Baohua.Song@csr.com>
>>
>> hardcode set i2c pin group to i2c function before, here we
>> move to use standard pinctrl API to get pins of the group.
>>
>> Signed-off-by: Barry Song <Baohua.Song@csr.com>
>> Cc: Linus Walleij <linus.walleij@linaro.org>
>
> NAK.
>
> This is done by the device core as of commit
> ab78029ecc347debbd737f06688d788bd9d60c1d
> "drivers/pinctrl: grab default handles from device core".
>
> You only need to fetch pinctrl handles in drivers if you're
> using anything else than the default state.

well. missed this recent commit.
it should mean we hould drop all devm_pinctrl_get_select_default() if
the driver only takes the default pinctrl?

it looks like there are still a lot of drivers doing that. anyway, i
will drop them in SiRFsoc drivers. and involve you in the coming
patch.

>
> Yours,
> Linus Walleij
>

-barry
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Linus Walleij - March 27, 2013, 3:14 p.m.
On Wed, Mar 27, 2013 at 11:36 AM, Barry Song <21cnbao@gmail.com> wrote:
> 2013/3/27 Linus Walleij <linus.walleij@linaro.org>:

>> You only need to fetch pinctrl handles in drivers if you're
>> using anything else than the default state.
>
> well. missed this recent commit.
> it should mean we hould drop all devm_pinctrl_get_select_default() if
> the driver only takes the default pinctrl?

Yes.

> it looks like there are still a lot of drivers doing that. anyway, i
> will drop them in SiRFsoc drivers. and involve you in the coming
> patch.

I am meaning to fix that up. Just haven't had time to...
Prior to that patch it was the right thing to do.

Yours,
Linus Walleij
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/i2c/busses/i2c-sirf.c b/drivers/i2c/busses/i2c-sirf.c
index 5a7ad24..dd4004e 100644
--- a/drivers/i2c/busses/i2c-sirf.c
+++ b/drivers/i2c/busses/i2c-sirf.c
@@ -16,6 +16,7 @@ 
 #include <linux/clk.h>
 #include <linux/err.h>
 #include <linux/io.h>
+#include <linux/pinctrl/consumer.h>
 
 #define SIRFSOC_I2C_CLK_CTRL		0x00
 #define SIRFSOC_I2C_STATUS		0x0C
@@ -265,6 +266,7 @@  static int i2c_sirfsoc_probe(struct platform_device *pdev)
 	struct i2c_adapter *adap;
 	struct resource *mem_res;
 	struct clk *clk;
+	struct pinctrl *pinctrl;
 	int bitrate;
 	int ctrl_speed;
 	int irq;
@@ -272,6 +274,12 @@  static int i2c_sirfsoc_probe(struct platform_device *pdev)
 	int err;
 	u32 regval;
 
+	pinctrl = devm_pinctrl_get_select_default(&pdev->dev);
+	if (IS_ERR(pinctrl)) {
+		err = PTR_ERR(pinctrl);
+		goto failed_pin;
+	}
+
 	clk = clk_get(&pdev->dev, NULL);
 	if (IS_ERR(clk)) {
 		err = PTR_ERR(clk);
@@ -385,6 +393,7 @@  err_clk_en:
 err_clk_prep:
 	clk_put(clk);
 err_get_clk:
+failed_pin:
 	return err;
 }