| Message ID | 1363591361-5992-1-git-send-email-Barry.Song@csr.com |
|---|---|
| State | Rejected |
| Headers | show |
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
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
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
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
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
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
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
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; }