[PATCH RESEND] i2c: designware: use module_platform_driver

[PATCH RESEND] i2c: designware: use module_platform_driver

[Zhangfei Gao]

Instead of use platform_driver_probe, use module_platform_driver
To support deferred probing
Also subsys_initcall may too early to auto set pinctl

Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
---
 drivers/i2c/busses/i2c-designware-platdrv.c |   14 ++------------
 1 file changed, 2 insertions(+), 12 deletions(-)

diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
index 4c5fada..36ceebc 100644
--- a/drivers/i2c/busses/i2c-designware-platdrv.c
+++ b/drivers/i2c/busses/i2c-designware-platdrv.c
@@ -236,6 +236,7 @@ static SIMPLE_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, dw_i2c_resume);
 MODULE_ALIAS("platform:i2c_designware");
 
 static struct platform_driver dw_i2c_driver = {
+	.probe		= dw_i2c_probe,
 	.remove		= dw_i2c_remove,
 	.driver		= {
 		.name	= "i2c_designware",
@@ -245,18 +246,7 @@ static struct platform_driver dw_i2c_driver = {
 		.pm	= &dw_i2c_dev_pm_ops,
 	},
 };
-
-static int __init dw_i2c_init_driver(void)
-{
-	return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe);
-}
-subsys_initcall(dw_i2c_init_driver);
-
-static void __exit dw_i2c_exit_driver(void)
-{
-	platform_driver_unregister(&dw_i2c_driver);
-}
-module_exit(dw_i2c_exit_driver);
+module_platform_driver(dw_i2c_driver);
 
 MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
 MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
-- 
1.7.9.5

getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2146 bytes --]

On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote:
> Instead of use platform_driver_probe, use module_platform_driver
> To support deferred probing
> Also subsys_initcall may too early to auto set pinctl
> 
> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>

This patch is tougher than it looks. You need it, because
subsys_initcall may be too early for pinctrl. Other people might be
depending on subsys_initcall to get I2C active before they want to
activate, say, PMICs. So, I fear regressions, since deferred probing
might not be available in the needed places to avoid these regressions.
I am all ears for a nice transition away from subsys_initcall, anyone?

> ---
>  drivers/i2c/busses/i2c-designware-platdrv.c |   14 ++------------
>  1 file changed, 2 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/i2c/busses/i2c-designware-platdrv.c b/drivers/i2c/busses/i2c-designware-platdrv.c
> index 4c5fada..36ceebc 100644
> --- a/drivers/i2c/busses/i2c-designware-platdrv.c
> +++ b/drivers/i2c/busses/i2c-designware-platdrv.c
> @@ -236,6 +236,7 @@ static SIMPLE_DEV_PM_OPS(dw_i2c_dev_pm_ops, dw_i2c_suspend, dw_i2c_resume);
>  MODULE_ALIAS("platform:i2c_designware");
>  
>  static struct platform_driver dw_i2c_driver = {
> +	.probe		= dw_i2c_probe,
>  	.remove		= dw_i2c_remove,
>  	.driver		= {
>  		.name	= "i2c_designware",
> @@ -245,18 +246,7 @@ static struct platform_driver dw_i2c_driver = {
>  		.pm	= &dw_i2c_dev_pm_ops,
>  	},
>  };
> -
> -static int __init dw_i2c_init_driver(void)
> -{
> -	return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe);
> -}
> -subsys_initcall(dw_i2c_init_driver);
> -
> -static void __exit dw_i2c_exit_driver(void)
> -{
> -	platform_driver_unregister(&dw_i2c_driver);
> -}
> -module_exit(dw_i2c_exit_driver);
> +module_platform_driver(dw_i2c_driver);
>  
>  MODULE_AUTHOR("Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>");
>  MODULE_DESCRIPTION("Synopsys DesignWare I2C bus adapter");
> -- 
> 1.7.9.5
> 

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Linus Walleij]

On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote:

>> Instead of use platform_driver_probe, use module_platform_driver
>> To support deferred probing
>> Also subsys_initcall may too early to auto set pinctl
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
>
> This patch is tougher than it looks. You need it, because
> subsys_initcall may be too early for pinctrl.

pinctrl is initialized very early, core_initcall().

This is more a question of individual pin control drivers
and when they probe, and dependencies trying to take
a pinctrl handle before the pin controller is available
will be deferred. Even by those grabbed in the core
by drivers/base/pinctrl.c.

Yours,
Linus Walleij

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[zhangfei gao]

On Thu, Aug 29, 2013 at 4:58 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote:
>
>>> Instead of use platform_driver_probe, use module_platform_driver
>>> To support deferred probing
>>> Also subsys_initcall may too early to auto set pinctl
>>>
>>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>>> Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
>>
>> This patch is tougher than it looks. You need it, because
>> subsys_initcall may be too early for pinctrl.
>
> pinctrl is initialized very early, core_initcall().
>
> This is more a question of individual pin control drivers
> and when they probe, and dependencies trying to take
> a pinctrl handle before the pin controller is available
> will be deferred. Even by those grabbed in the core
> by drivers/base/pinctrl.c.

Thanks Linus.
Your explanation is really make sense.

We use drivers/pinctrl/pinctrl-single.c, if subsys_initcall for
pinctrl-single, no issue at all.

Checked in 3.11-rc4, there is really deferring probe happen.
i2c_designware fcb08000.i2c: could not find pctldev for node
/amba/pinmux@fc803000/i2c0
_pmx_func, deferring probe

However, bus_probe_device failed, since the drv name list does not
have i2c_designware.
deferred_probe_work_func -> bus_probe_device -> device_attach ->
bus_for_each_drv -> __device_attach

It can be solved change
return platform_driver_probe(&dw_i2c_driver, dw_i2c_probe);
to
return platform_driver_register(&dw_i2c_driver);
static struct platform_driver dw_i2c_driver = {
        .probe          = dw_i2c_probe,
~

Dear Wolfram

Thanks for telling me the dependency about subsys_initcall.
Should I resubmit one patch using platform_driver_register while
keeping subsys_initcall?

Besides, also find platform_driver_probe is used in
drivers/i2c/busses/i2c-imx.c and drivers/i2c/busses/i2c-stu300.c.

Thanks

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Linus Walleij]

On Thu, Aug 29, 2013 at 12:55 PM, zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:

> Besides, also find platform_driver_probe is used in
> drivers/i2c/busses/i2c-imx.c and drivers/i2c/busses/i2c-stu300.c.

The platform_driver_probe() is basically a footprint
optimization (more code can be discarded after boot)
and I'm happy to patch it if it disturbs anything, it is
*really* not important for this driver.

Do you guys need a low footprint? Else there is no
use to have platform_driver_probe() in there.

Yours,
Linus Walleij

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Tony Lindgren]

* zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 04:03]:
> On Thu, Aug 29, 2013 at 4:58 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
> > On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
> >> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote:
> >
> >>> Instead of use platform_driver_probe, use module_platform_driver
> >>> To support deferred probing
> >>> Also subsys_initcall may too early to auto set pinctl
> >>>
> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
> >>> Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
> >>
> >> This patch is tougher than it looks. You need it, because
> >> subsys_initcall may be too early for pinctrl.
> >
> > pinctrl is initialized very early, core_initcall().
> >
> > This is more a question of individual pin control drivers
> > and when they probe, and dependencies trying to take
> > a pinctrl handle before the pin controller is available
> > will be deferred. Even by those grabbed in the core
> > by drivers/base/pinctrl.c.
> 
> Thanks Linus.
> Your explanation is really make sense.
> 
> We use drivers/pinctrl/pinctrl-single.c, if subsys_initcall for
> pinctrl-single, no issue at all.

So far we've seen that if you have issues with this, the real
problem is that some other driver is trying to initialize way
too early probably because of legacy reasons that no longer
apply.

FYI, it's best to have all the drivers initialize with just
module_init and make them work as loadable modules because of
the following reasons:

1. You will get real console error messages when something
   goes wrong with no need for debug_ll and earlyprintk

2. By creating loadable driver modules you're already getting
   some protection from spaghetti code as the interfaces
   are defined

3. It will be easier for distros to support various ARM SoCs
   with loadable driver modules

Regards,

Tony

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[zhangfei gao]

On Fri, Aug 30, 2013 at 1:48 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 04:03]:
>> On Thu, Aug 29, 2013 at 4:58 PM, Linus Walleij <linus.walleij-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org> wrote:
>> > On Wed, Aug 28, 2013 at 11:57 AM, Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> wrote:
>> >> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote:
>> >
>> >>> Instead of use platform_driver_probe, use module_platform_driver
>> >>> To support deferred probing
>> >>> Also subsys_initcall may too early to auto set pinctl
>> >>>
>> >>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> >>> Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
>> >>
>> >> This patch is tougher than it looks. You need it, because
>> >> subsys_initcall may be too early for pinctrl.
>> >
>> > pinctrl is initialized very early, core_initcall().
>> >
>> > This is more a question of individual pin control drivers
>> > and when they probe, and dependencies trying to take
>> > a pinctrl handle before the pin controller is available
>> > will be deferred. Even by those grabbed in the core
>> > by drivers/base/pinctrl.c.
>>
>> Thanks Linus.
>> Your explanation is really make sense.
>>
>> We use drivers/pinctrl/pinctrl-single.c, if subsys_initcall for
>> pinctrl-single, no issue at all.
>
> So far we've seen that if you have issues with this, the real
> problem is that some other driver is trying to initialize way
> too early probably because of legacy reasons that no longer
> apply.
>
> FYI, it's best to have all the drivers initialize with just
> module_init and make them work as loadable modules because of
> the following reasons:
>
> 1. You will get real console error messages when something
>    goes wrong with no need for debug_ll and earlyprintk
>
> 2. By creating loadable driver modules you're already getting
>    some protection from spaghetti code as the interfaces
>    are defined
>
> 3. It will be easier for distros to support various ARM SoCs
>    with loadable driver modules
>
> Regards,
>
> Tony

Thanks Tony,

What about concerns from Wolfram:
" Other people might be
depending on subsys_initcall to get I2C active before they want to
activate, say, PMICs. So, I fear regressions, since deferred probing
might not be available in the needed places to avoid these regressions."

Is it too late using module_init for PMIC?

Besides, the deferred probing still there if depend on late registered
pin control driver.

Thanks

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Tony Lindgren]

* zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
> What about concerns from Wolfram:
> " Other people might be
> depending on subsys_initcall to get I2C active before they want to
> activate, say, PMICs. So, I fear regressions, since deferred probing
> might not be available in the needed places to avoid these regressions."

There should not be any reason to get a PMIC activated
early on. The system should be booting already at that point,
and the PMIC related init can be done later on.
 
> Is it too late using module_init for PMIC?

You can probably do it as a fix early on during the -rc
cycle too. Of course it needs to be verified to work first :)
 
> Besides, the deferred probing still there if depend on late registered
> pin control driver.

At least for omaps we have things working just fine with
pinctrl-single and a PMIC on I2C controller.

Regards,

Tony

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[zhangfei gao]

On Fri, Aug 30, 2013 at 4:27 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
> * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
>> What about concerns from Wolfram:
>> " Other people might be
>> depending on subsys_initcall to get I2C active before they want to
>> activate, say, PMICs. So, I fear regressions, since deferred probing
>> might not be available in the needed places to avoid these regressions."
>
> There should not be any reason to get a PMIC activated
> early on. The system should be booting already at that point,
> and the PMIC related init can be done later on.
>
>> Is it too late using module_init for PMIC?
>
> You can probably do it as a fix early on during the -rc
> cycle too. Of course it needs to be verified to work first :)
>

Dear Wolfram

What's your suggestion about this issue.
Use subsys_initcall, deferred probing still exist if base on pin control driver.

Thanks

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[zhangfei gao]

On Thu, Sep 12, 2013 at 9:12 AM, zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> On Fri, Aug 30, 2013 at 4:27 PM, Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> wrote:
>> * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
>>> What about concerns from Wolfram:
>>> " Other people might be
>>> depending on subsys_initcall to get I2C active before they want to
>>> activate, say, PMICs. So, I fear regressions, since deferred probing
>>> might not be available in the needed places to avoid these regressions."
>>
>> There should not be any reason to get a PMIC activated
>> early on. The system should be booting already at that point,
>> and the PMIC related init can be done later on.
>>
>>> Is it too late using module_init for PMIC?
>>
>> You can probably do it as a fix early on during the -rc
>> cycle too. Of course it needs to be verified to work first :)
>>
>
> Dear Wolfram
>
> What's your suggestion about this issue.
> Use subsys_initcall, deferred probing still exist if base on pin control driver.
>
> Thanks

Dear Wolfram

Any plan about the patch?

On one hand, module_X_driver is trend to replace subsys_initcall

Refer from Mark
"We're trying to move away from needing to do this and to using deferred
probing to resolve init ordering issues.  Should we not be able to
convert the drivers to module_X_driver()?"

On the other hand, subsys_initcall still been defered if pin
controller driver been relied on.

Thanks

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 90 bytes --]

> Any plan about the patch?

I am right now working on a patch series dealing with this.


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2054 bytes --]

On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote:
> * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
> > What about concerns from Wolfram:
> > " Other people might be
> > depending on subsys_initcall to get I2C active before they want to
> > activate, say, PMICs. So, I fear regressions, since deferred probing
> > might not be available in the needed places to avoid these regressions."
> 
> There should not be any reason to get a PMIC activated
> early on. The system should be booting already at that point,
> and the PMIC related init can be done later on.

Okay, here is a more concrete example:

Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware
of deferred probing, I wrote the following code to get the GPIOs (which are
optional):

	err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute");
	max9768->mute_gpio = err ?: pdata->mute_gpio;

And later in the process:

        if (gpio_is_valid(max9768->mute_gpio)) {
                ret = snd_soc_add_codec_controls(codec, max9768_mute,
                                ARRAY_SIZE(max9768_mute));
                if (ret)
                        return ret;
        }

So, the mute control will only be added if the gpio_request succeeded. On that
particular board, the mute GPIO was wired to an I2C GPIO controller. If I now
change the I2C (or GPIO) driver from subsys_initcall to module_init, then the
gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute
control will then disappear. Yes, the driver can be fixed easily, yet I fear a
number of regressions like this. Instead of people digging into why things
disappear after a kernel update, I wonder if there is a way to guide users if
this happens. I didn't have time for that, though, sadly. Still, it makes me
wonder how easily we could shift from subsys_initcall to module_init, although
I'd really love to get away from subsys_initcall in device drivers.

Regards,

   Wolfram


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Tony Lindgren]

* Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> [131008 14:01]:
> On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote:
> > * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
> > > What about concerns from Wolfram:
> > > " Other people might be
> > > depending on subsys_initcall to get I2C active before they want to
> > > activate, say, PMICs. So, I fear regressions, since deferred probing
> > > might not be available in the needed places to avoid these regressions."
> > 
> > There should not be any reason to get a PMIC activated
> > early on. The system should be booting already at that point,
> > and the PMIC related init can be done later on.
> 
> Okay, here is a more concrete example:
> 
> Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware
> of deferred probing, I wrote the following code to get the GPIOs (which are
> optional):
> 
> 	err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute");
> 	max9768->mute_gpio = err ?: pdata->mute_gpio;
> 
> And later in the process:
> 
>         if (gpio_is_valid(max9768->mute_gpio)) {
>                 ret = snd_soc_add_codec_controls(codec, max9768_mute,
>                                 ARRAY_SIZE(max9768_mute));
>                 if (ret)
>                         return ret;
>         }
> 
> So, the mute control will only be added if the gpio_request succeeded. On that
> particular board, the mute GPIO was wired to an I2C GPIO controller. If I now
> change the I2C (or GPIO) driver from subsys_initcall to module_init, then the
> gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute
> control will then disappear. Yes, the driver can be fixed easily, yet I fear a
> number of regressions like this. Instead of people digging into why things
> disappear after a kernel update, I wonder if there is a way to guide users if
> this happens. I didn't have time for that, though, sadly. Still, it makes me
> wonder how easily we could shift from subsys_initcall to module_init, although
> I'd really love to get away from subsys_initcall in device drivers.

Well it should be pretty trivial to update drivers to use deferred
probing. Maybe some spatch to check for that in driver probes would
help getting an idea how many might be affected?

Anyways, it should be fixed as otherwise we'll just dig ourselves
deeper into the mess of things not working as loadable modules.

Regards,

Tony

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Tony Lindgren]

* Tony Lindgren <tony-4v6yS6AI5VpBDgjK7y7TUQ@public.gmane.org> [131008 15:19]:
> * Wolfram Sang <wsa-z923LK4zBo2bacvFa/9K2g@public.gmane.org> [131008 14:01]:
> > On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote:
> > > * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
> > > > What about concerns from Wolfram:
> > > > " Other people might be
> > > > depending on subsys_initcall to get I2C active before they want to
> > > > activate, say, PMICs. So, I fear regressions, since deferred probing
> > > > might not be available in the needed places to avoid these regressions."
> > > 
> > > There should not be any reason to get a PMIC activated
> > > early on. The system should be booting already at that point,
> > > and the PMIC related init can be done later on.
> > 
> > Okay, here is a more concrete example:
> > 
> > Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware
> > of deferred probing, I wrote the following code to get the GPIOs (which are
> > optional):
> > 
> > 	err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute");
> > 	max9768->mute_gpio = err ?: pdata->mute_gpio;
> > 
> > And later in the process:
> > 
> >         if (gpio_is_valid(max9768->mute_gpio)) {
> >                 ret = snd_soc_add_codec_controls(codec, max9768_mute,
> >                                 ARRAY_SIZE(max9768_mute));
> >                 if (ret)
> >                         return ret;
> >         }
> > 
> > So, the mute control will only be added if the gpio_request succeeded. On that
> > particular board, the mute GPIO was wired to an I2C GPIO controller. If I now
> > change the I2C (or GPIO) driver from subsys_initcall to module_init, then the
> > gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute
> > control will then disappear. Yes, the driver can be fixed easily, yet I fear a
> > number of regressions like this. Instead of people digging into why things
> > disappear after a kernel update, I wonder if there is a way to guide users if
> > this happens. I didn't have time for that, though, sadly. Still, it makes me
> > wonder how easily we could shift from subsys_initcall to module_init, although
> > I'd really love to get away from subsys_initcall in device drivers.
> 
> Well it should be pretty trivial to update drivers to use deferred
> probing. Maybe some spatch to check for that in driver probes would
> help getting an idea how many might be affected?
> 
> Anyways, it should be fixed as otherwise we'll just dig ourselves
> deeper into the mess of things not working as loadable modules.

BTW, another place where things can go wrong is if there's an irqchip
driver that is being set up at module_init time. If an interrupt client
driver does irq = platform_get_resource(pdev, IORESOURCE_IRQ, 0), the
resources may not have been initialize for the DT case as those are
populated triggered by of_platform_populate().

The fix there is to use irq = irq_of_parse_and_map(pdev->dev.of_node, 0)
instead. Or somehow make of_platform_populate() support -EPROBE_DEFER.

Just FYI,

Tony

Tony

Re: getting rid of subsys_initcall usage? (was: Re: [PATCH RESEND] i2c: designware: use module_platform_driver)

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 589 bytes --]



> Well it should be pretty trivial to update drivers to use deferred
> probing. Maybe some spatch to check for that in driver probes would
> help getting an idea how many might be affected?

That's what I am trying to say. It surely is easy to fix the drivers,
once we know there is something in need of fixing. My question was if
there is common sense to simply risk breaking things and fix them later
(then I'd apply patches switching from subsys_initcall to module_init
right away), or if we can gather ideas how to minimize the impact of
regressions (before applying such patches).


[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: getting rid of subsys_initcall usage?

[zhangfei]

On 10/09/2013 04:53 AM, Wolfram Sang wrote:
> On Fri, Aug 30, 2013 at 01:27:13AM -0700, Tony Lindgren wrote:
>> * zhangfei gao <zhangfei.gao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> [130829 23:36]:
>>> What about concerns from Wolfram:
>>> " Other people might be
>>> depending on subsys_initcall to get I2C active before they want to
>>> activate, say, PMICs. So, I fear regressions, since deferred probing
>>> might not be available in the needed places to avoid these regressions."
>>
>> There should not be any reason to get a PMIC activated
>> early on. The system should be booting already at that point,
>> and the PMIC related init can be done later on.
>
> Okay, here is a more concrete example:
>
> Consider the amplifier driver 'sound/soc/codecs/max9768.c'. Back then, unaware
> of deferred probing, I wrote the following code to get the GPIOs (which are
> optional):
>
> 	err = gpio_request_one(pdata->mute_gpio, GPIOF_INIT_HIGH, "MAX9768 Mute");
> 	max9768->mute_gpio = err ?: pdata->mute_gpio;
>
> And later in the process:
>
>          if (gpio_is_valid(max9768->mute_gpio)) {
>                  ret = snd_soc_add_codec_controls(codec, max9768_mute,
>                                  ARRAY_SIZE(max9768_mute));
>                  if (ret)
>                          return ret;
>          }
>
> So, the mute control will only be added if the gpio_request succeeded. On that
> particular board, the mute GPIO was wired to an I2C GPIO controller. If I now
> change the I2C (or GPIO) driver from subsys_initcall to module_init, then the
> gpio_request in the amplifier driver could hit -EPROBE_DEFER and the mute
> control will then disappear.

However, this may requires I2C, GPIO, pinctrl all use subsys_initcall.
-EPROBE_DEFER still be returned, if pinctrl happen to use 
module_platform_driver, which is most case in drivers/pinctrl/.

Yes, the driver can be fixed easily, yet I fear a
> number of regressions like this. Instead of people digging into why things
> disappear after a kernel update, I wonder if there is a way to guide users if
> this happens. I didn't have time for that, though, sadly. Still, it makes me
> wonder how easily we could shift from subsys_initcall to module_init, although
> I'd really love to get away from subsys_initcall in device drivers.
>

Thanks for clarify the concern.

Re: getting rid of subsys_initcall usage?

[zhangfei]

On 13-08-28 05:57 PM, Wolfram Sang wrote:
> On Tue, Aug 20, 2013 at 04:32:28PM +0800, Zhangfei Gao wrote:
>> Instead of use platform_driver_probe, use module_platform_driver
>> To support deferred probing
>> Also subsys_initcall may too early to auto set pinctl
>>
>> Signed-off-by: Zhangfei Gao <zhangfei.gao-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
>> Acked-by: Baruch Siach <baruch-NswTu9S1W3P6gbPvEgmw2w@public.gmane.org>
>
> This patch is tougher than it looks. You need it, because
> subsys_initcall may be too early for pinctrl. Other people might be
> depending on subsys_initcall to get I2C active before they want to
> activate, say, PMICs. So, I fear regressions, since deferred probing
> might not be available in the needed places to avoid these regressions.
> I am all ears for a nice transition away from subsys_initcall, anyone?

Dear Wolfram,

The deferred probe will be successful if change platform_driver_probe to 
platform_driver_register, to register itself to drv name list.

However, the effect is subsys_initcall is postponed, until pin control 
driver is probed successfully.
If the pin is specific and does not need configure, there is no issue.
There may still have issue if the mulit-function pin need be configured 
via pinctrl, except put pin control driver earlier too.

Thanks