diff mbox

[V2,1/6] pinctrl: pinctrl-single: use arch_initcall and module_exit

Message ID 1359445134-13323-2-git-send-email-manishv.b@ti.com (mailing list archive)
State Not Applicable
Headers show

Commit Message

Vishwanathrao Badarkhe, Manish Jan. 29, 2013, 7:38 a.m. UTC
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(-)

Comments

Linus Walleij Jan. 29, 2013, 10:59 a.m. UTC | #1
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
Vishwanathrao Badarkhe, Manish Jan. 29, 2013, 12:02 p.m. UTC | #2
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
Sekhar Nori Feb. 1, 2013, 7:05 a.m. UTC | #3
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
Tony Lindgren Feb. 1, 2013, 5:09 p.m. UTC | #4
* 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 Feb. 1, 2013, 5:11 p.m. UTC | #5
* 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
Vishwanathrao Badarkhe, Manish Feb. 5, 2013, 6:36 a.m. UTC | #6
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
Russell King - ARM Linux Feb. 5, 2013, 11:27 a.m. UTC | #7
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.
Linus Walleij Feb. 5, 2013, 1 p.m. UTC | #8
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
Vishwanathrao Badarkhe, Manish Feb. 6, 2013, 4:57 a.m. UTC | #9
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
Vishwanathrao Badarkhe, Manish Feb. 6, 2013, 6:04 a.m. UTC | #10
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 mbox

Patch

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");