[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Barry Song]

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;
+	}
+
 	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;
 }
 
-- 
1.7.5.4



Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
More information can be found at www.csr.com. Follow CSR on Twitter at http://twitter.com/CSR_PLC and read our blog at www.csr.com/blog

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Sourav Poddar]

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;
>   }
>

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Barry Song]

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

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Sourav Poddar]

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

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Barry Song]

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

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Linus Walleij]

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

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Barry Song]

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

[PATCH] i2c: sirf: get the i2c pin group by pinctrl api

[Linus Walleij]

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