| Message ID | 1359445134-13323-2-git-send-email-manishv.b@ti.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jan 29, 2013 at 8:38 AM, Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> wrote: > Currently, I2C driver gets probed before pinctrl driver. > To achieve I2C pin muxing via pinctrl driver before I2C > probe get called, register pinctrl driver in arch_initcall. > Also, add module_exit to unregister pinctrl driver. > > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> So your I2C driver is not returning -EPROBE_DEFER if it cannot find its pins? Hm, well I can live with this, if Tony ACKs it. Yours, Linus Walleij
Hi Linus, On Tue, Jan 29, 2013 at 16:29:54, Linus Walleij wrote: > On Tue, Jan 29, 2013 at 8:38 AM, Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> wrote: > > > Currently, I2C driver gets probed before pinctrl driver. > > To achieve I2C pin muxing via pinctrl driver before I2C probe get > > called, register pinctrl driver in arch_initcall. > > Also, add module_exit to unregister pinctrl driver. > > > > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > So your I2C driver is not returning -EPROBE_DEFER if it cannot find its pins? Yes. In Davinci I2C driver, it's not implemented to return -EPROBE_DEFER if it can't find its pins. Please see below discussion for more details: https://patchwork2.kernel.org/patch/2031321/ > > Hm, well I can live with this, if Tony ACKs it. > > Yours, > Linus Walleij > Regards, Manish
On 1/29/2013 4:29 PM, Linus Walleij wrote: > On Tue, Jan 29, 2013 at 8:38 AM, Vishwanathrao Badarkhe, Manish > <manishv.b@ti.com> wrote: > >> Currently, I2C driver gets probed before pinctrl driver. >> To achieve I2C pin muxing via pinctrl driver before I2C >> probe get called, register pinctrl driver in arch_initcall. >> Also, add module_exit to unregister pinctrl driver. >> >> Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > So your I2C driver is not returning -EPROBE_DEFER > if it cannot find its pins? > > Hm, well I can live with this, if Tony ACKs it. FWIW, most of the other pinctrl drivers seem to be using arch_initcall() already. $ grep -r arch_initcall drivers/pinctrl/* drivers/pinctrl/pinctrl-at91.c:arch_initcall(at91_pinctrl_init); drivers/pinctrl/pinctrl-coh901.c:arch_initcall(u300_gpio_init); drivers/pinctrl/pinctrl-imx35.c:arch_initcall(imx35_pinctrl_init); drivers/pinctrl/pinctrl-imx51.c:arch_initcall(imx51_pinctrl_init); drivers/pinctrl/pinctrl-imx53.c:arch_initcall(imx53_pinctrl_init); drivers/pinctrl/pinctrl-imx6q.c:arch_initcall(imx6q_pinctrl_init); drivers/pinctrl/pinctrl-sirf.c:arch_initcall(sirfsoc_pinmux_init); drivers/pinctrl/pinctrl-tegra20.c:arch_initcall(tegra20_pinctrl_init); drivers/pinctrl/pinctrl-tegra30.c:arch_initcall(tegra30_pinctrl_init); drivers/pinctrl/pinctrl-u300.c:arch_initcall(u300_pmx_init); drivers/pinctrl/spear/pinctrl-spear300.c:arch_initcall(spear300_pinctrl_init); drivers/pinctrl/spear/pinctrl-spear320.c:arch_initcall(spear320_pinctrl_init); drivers/pinctrl/spear/pinctrl-spear310.c:arch_initcall(spear310_pinctrl_init); drivers/pinctrl/spear/pinctrl-spear1340.c:arch_initcall(spear1340_pinctrl_init); drivers/pinctrl/spear/pinctrl-spear1310.c:arch_initcall(spear1310_pinctrl_init); Thanks, Sekhar
* Linus Walleij <linus.walleij@linaro.org> [130129 03:03]: > On Tue, Jan 29, 2013 at 8:38 AM, Vishwanathrao Badarkhe, Manish > <manishv.b@ti.com> wrote: > > > Currently, I2C driver gets probed before pinctrl driver. > > To achieve I2C pin muxing via pinctrl driver before I2C > > probe get called, register pinctrl driver in arch_initcall. > > Also, add module_exit to unregister pinctrl driver. > > > > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > So your I2C driver is not returning -EPROBE_DEFER > if it cannot find its pins? > > Hm, well I can live with this, if Tony ACKs it. Hmm pinctrl is before i2c in drivers/Makefile. Making initcalls happen earlier and earlier is usually the wrong way to go. Sounds like there's some other issue here that needs to be fixed instead. Regards, Tony
* Tony Lindgren <tony@atomide.com> [130201 09:12]: > * Linus Walleij <linus.walleij@linaro.org> [130129 03:03]: > > On Tue, Jan 29, 2013 at 8:38 AM, Vishwanathrao Badarkhe, Manish > > <manishv.b@ti.com> wrote: > > > > > Currently, I2C driver gets probed before pinctrl driver. > > > To achieve I2C pin muxing via pinctrl driver before I2C > > > probe get called, register pinctrl driver in arch_initcall. > > > Also, add module_exit to unregister pinctrl driver. > > > > > > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > > > So your I2C driver is not returning -EPROBE_DEFER > > if it cannot find its pins? > > > > Hm, well I can live with this, if Tony ACKs it. > > Hmm pinctrl is before i2c in drivers/Makefile. > Making initcalls happen earlier and earlier is usually the > wrong way to go. Sounds like there's some other issue here > that needs to be fixed instead. Let me guess: The i2c driver is wrongly set to run with arch_initcall? > Regards, > > Tony > _______________________________________________ > devicetree-discuss mailing list > devicetree-discuss@lists.ozlabs.org > https://lists.ozlabs.org/listinfo/devicetree-discuss
On Fri, Feb 01, 2013 at 22:41:24, Tony Lindgren wrote: > * Tony Lindgren <tony@atomide.com> [130201 09:12]: > > * Linus Walleij <linus.walleij@linaro.org> [130129 03:03]: > > > On Tue, Jan 29, 2013 at 8:38 AM, Vishwanathrao Badarkhe, Manish > > > <manishv.b@ti.com> wrote: > > > > > > > Currently, I2C driver gets probed before pinctrl driver. > > > > To achieve I2C pin muxing via pinctrl driver before I2C probe get > > > > called, register pinctrl driver in arch_initcall. > > > > Also, add module_exit to unregister pinctrl driver. > > > > > > > > Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> > > > > > > So your I2C driver is not returning -EPROBE_DEFER if it cannot find > > > its pins? > > > > > > Hm, well I can live with this, if Tony ACKs it. > > > > Hmm pinctrl is before i2c in drivers/Makefile. > > Making initcalls happen earlier and earlier is usually the wrong way > > to go. Sounds like there's some other issue here that needs to be > > fixed instead. > > Let me guess: The i2c driver is wrongly set to run with arch_initcall? > Hi Tony No, Currently i2c driver is set to subsys_initcall. I have seen some problem while using pin control grab functionality. Please see detailed explanation as below. Hi Linus I am using auto grab pin control facility to do pin muxing of I2C0 pins and seen problem as below: Currently, probe of I2C0 driver gets called before pin control driver. Hence, while calling I2C0 probe because of unavailability of pin control node information, its probe get deferred giving following messages: "i2c_davinci i2c_davinci.1: could not find pctldev for node /soc/pinmux@1c14120 /pinmux_i2c0_pins, deferring probe" As I2C0 is in deferred list (as auto grab patch handle this), its probe get called once again, During this second time probe of I2C0, I have observed following crash: "Unable to handle kernel paging request at virtual address fffffe07" As per code analysis of auto grab functionality, I have seen in "pinctrl_bind_pin" function, pin control handle (dpi->p) is returned by "devm_pinctrl_get" function. Pin control handle is assigned with error pointer during 1st time probing of I2C0 (as pin control information is not available at this time). During 2nd time probing (deferred probe) of I2C0, same pin control handle (which was get assigned during 1st probe) is getting used instead of getting updated to correct pin control handle which leads to system crash. I made following changes, in order to update "dip->p" pointer with correct value: - if (!dpi->p) { + if (IS_ERR_OR_NULL(dpi->p)) { dpi->p = devm_pinctrl_get(dev); - if (IS_ERR_OR_NULL(dpi->p)) { - int ret = PTR_ERR(dpi->p); - - dev_dbg(dev, "no pinctrl handle\n"); - /* Only return deferrals */ - if (ret == -EPROBE_DEFER) - return ret; - return 0; - } + ret = PTR_ERR(dpi->p); + dev_dbg(dev, "no pinctrl handle\n"); + /* Only return deferrals */ + if (ret == -EPROBE_DEFER) + return ret; + return 0; Is this intended change? or am I missing something in order to use this auto grab functionality? With the above change, now deferred probing is working fine and there is no need to register pin control driver in arch_initcall. Regards, Manish
On Tue, Feb 05, 2013 at 06:36:34AM +0000, Vishwanathrao Badarkhe, Manish wrote: > I made following changes, in order to update "dip->p" pointer with > correct value: > > - if (!dpi->p) { > + if (IS_ERR_OR_NULL(dpi->p)) { > dpi->p = devm_pinctrl_get(dev); > - if (IS_ERR_OR_NULL(dpi->p)) { > - int ret = PTR_ERR(dpi->p); > - > - dev_dbg(dev, "no pinctrl handle\n"); > - /* Only return deferrals */ > - if (ret == -EPROBE_DEFER) > - return ret; > - return 0; > - } > + ret = PTR_ERR(dpi->p); > + dev_dbg(dev, "no pinctrl handle\n"); > + /* Only return deferrals */ > + if (ret == -EPROBE_DEFER) > + return ret; > + return 0; > > Is this intended change? The above looks totally broken to me. Oh, it's using IS_ERR_OR_NULL(), so it's bound to be broken.
On Tue, Feb 5, 2013 at 7:36 AM, Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> wrote: > I made following changes, in order to update "dip->p" pointer with > correct value: > > - if (!dpi->p) { > + if (IS_ERR_OR_NULL(dpi->p)) { > dpi->p = devm_pinctrl_get(dev); > - if (IS_ERR_OR_NULL(dpi->p)) { > - int ret = PTR_ERR(dpi->p); > - > - dev_dbg(dev, "no pinctrl handle\n"); > - /* Only return deferrals */ > - if (ret == -EPROBE_DEFER) > - return ret; > - return 0; > - } > + ret = PTR_ERR(dpi->p); > + dev_dbg(dev, "no pinctrl handle\n"); > + /* Only return deferrals */ > + if (ret == -EPROBE_DEFER) > + return ret; > + return 0; This is based on some old code that I wrote ages ago. Check the pinctrl tree or linux-next for the latest core pin grabbing code. Use that instead. Yours, Linus Walleij
Hi Linus On Tue, Feb 05, 2013 at 18:30:48, Linus Walleij wrote: > On Tue, Feb 5, 2013 at 7:36 AM, Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> wrote: > > > I made following changes, in order to update "dip->p" pointer with > > correct value: > > > > - if (!dpi->p) { > > + if (IS_ERR_OR_NULL(dpi->p)) { > > dpi->p = devm_pinctrl_get(dev); > > - if (IS_ERR_OR_NULL(dpi->p)) { > > - int ret = PTR_ERR(dpi->p); > > - > > - dev_dbg(dev, "no pinctrl handle\n"); > > - /* Only return deferrals */ > > - if (ret == -EPROBE_DEFER) > > - return ret; > > - return 0; > > - } > > + ret = PTR_ERR(dpi->p); > > + dev_dbg(dev, "no pinctrl handle\n"); > > + /* Only return deferrals */ > > + if (ret == -EPROBE_DEFER) > > + return ret; > > + return 0; > > This is based on some old code that I wrote ages ago. Check the pinctrl tree or linux-next for the latest core pin grabbing code. > Use that instead. Thanks for your reply. I have checked with your latest changes regarding pinctrl in linux-next tree and these changes are working fine for me. Regards Manish Badarkhe
Hi Russell, On Tue, Feb 05, 2013 at 16:57:31, Russell King - ARM Linux wrote: > On Tue, Feb 05, 2013 at 06:36:34AM +0000, Vishwanathrao Badarkhe, Manish wrote: > > I made following changes, in order to update "dip->p" pointer with > > correct value: > > > > - if (!dpi->p) { > > + if (IS_ERR_OR_NULL(dpi->p)) { > > dpi->p = devm_pinctrl_get(dev); > > - if (IS_ERR_OR_NULL(dpi->p)) { > > - int ret = PTR_ERR(dpi->p); > > - > > - dev_dbg(dev, "no pinctrl handle\n"); > > - /* Only return deferrals */ > > - if (ret == -EPROBE_DEFER) > > - return ret; > > - return 0; > > - } > > + ret = PTR_ERR(dpi->p); > > + dev_dbg(dev, "no pinctrl handle\n"); > > + /* Only return deferrals */ > > + if (ret == -EPROBE_DEFER) > > + return ret; > > + return 0; > > > > Is this intended change? > > The above looks totally broken to me. > > Oh, it's using IS_ERR_OR_NULL(), so it's bound to be broken. > I went through discussion in community on usage of "IS_ERR_OR_NULL". https://patchwork.kernel.org/patch/1953271/ Will take care not use this in future. Thanks, Manish
diff --git a/drivers/pinctrl/pinctrl-single.c b/drivers/pinctrl/pinctrl-single.c index f6a360b..3a96390 100644 --- a/drivers/pinctrl/pinctrl-single.c +++ b/drivers/pinctrl/pinctrl-single.c @@ -1089,7 +1089,17 @@ static struct platform_driver pcs_driver = { }, }; -module_platform_driver(pcs_driver); +static int __init pcs_pinctrl_init(void) +{ + return platform_driver_register(&pcs_driver); +} +arch_initcall(pcs_pinctrl_init); + +static void __exit pcs_pinctrl_exit(void) +{ + platform_driver_unregister(&pcs_driver); +} +module_exit(pcs_pinctrl_exit); MODULE_AUTHOR("Tony Lindgren <tony@atomide.com>"); MODULE_DESCRIPTION("One-register-per-pin type device tree based pinctrl driver");
Currently, I2C driver gets probed before pinctrl driver. To achieve I2C pin muxing via pinctrl driver before I2C probe get called, register pinctrl driver in arch_initcall. Also, add module_exit to unregister pinctrl driver. Signed-off-by: Vishwanathrao Badarkhe, Manish <manishv.b@ti.com> --- :100644 100644 f6a360b... 3a96390... M drivers/pinctrl/pinctrl-single.c drivers/pinctrl/pinctrl-single.c | 12 +++++++++++- 1 files changed, 11 insertions(+), 1 deletions(-)