[PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Mark Brown]

Allow the use of the S3C64xx SPI controller with things like PMICs by
moving the init up to subsys_initcall().

Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
---

Incidentally I don't seem to see anything in the current SPI tree for
-next - should the tree being used be updated or something?

 drivers/spi/spi_s3c64xx.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
index f72e1c0..6e48ea9 100644
--- a/drivers/spi/spi_s3c64xx.c
+++ b/drivers/spi/spi_s3c64xx.c
@@ -1181,7 +1181,7 @@ static int __init s3c64xx_spi_init(void)
 {
 	return platform_driver_probe(&s3c64xx_spi_driver, s3c64xx_spi_probe);
 }
-module_init(s3c64xx_spi_init);
+subsys_initcall(s3c64xx_spi_init);
 
 static void __exit s3c64xx_spi_exit(void)
 {
-- 
1.7.1

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Jassi Brar]

On Tue, Sep 7, 2010 at 7:29 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> Allow the use of the S3C64xx SPI controller with things like PMICs by
> moving the init up to subsys_initcall().
>
> Signed-off-by: Mark Brown <broonie@opensource.wolfsonmicro.com>
> ---
>
> Incidentally I don't seem to see anything in the current SPI tree for
> -next - should the tree being used be updated or something?
>
>  drivers/spi/spi_s3c64xx.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/spi/spi_s3c64xx.c b/drivers/spi/spi_s3c64xx.c
> index f72e1c0..6e48ea9 100644
> --- a/drivers/spi/spi_s3c64xx.c
> +++ b/drivers/spi/spi_s3c64xx.c
> @@ -1181,7 +1181,7 @@ static int __init s3c64xx_spi_init(void)
>  {
>        return platform_driver_probe(&s3c64xx_spi_driver, s3c64xx_spi_probe);
>  }
> -module_init(s3c64xx_spi_init);
> +subsys_initcall(s3c64xx_spi_init);

Couldn't any user ever need to load it as a module?
If no, we might as well drop the s3c64xx_spi_exit and s3c64xx_spi_remove
as well and save space. Rather going a step further, shouldn't then all
spi drivers be that way? Two steps further, why not every 'bus-driver' ?

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Mark Brown]

On Wed, Sep 08, 2010 at 01:55:39PM +0900, Jassi Brar wrote:
> On Tue, Sep 7, 2010 at 7:29 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > Allow the use of the S3C64xx SPI controller with things like PMICs by
> > moving the init up to subsys_initcall().

> Couldn't any user ever need to load it as a module?
> If no, we might as well drop the s3c64xx_spi_exit and s3c64xx_spi_remove

This doesn't prevent building as a module - when built as a module
subsys_initcall() is identical to module_init(), the change will only
affect the order in which things are done when the code is built into
the kernel otherwise it's a noop.

> as well and save space. Rather going a step further, shouldn't then all
> spi drivers be that way? Two steps further, why not every 'bus-driver' ?

Yes, we probably do need to do the same thing for all embedded SPI
controllers (as has already happened with embedded I2C controllers),
though it's less pressing since outside of a few CPUs it's much less
common for things like PMICs to be on SPI than I2C.

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Jassi Brar]

On Wed, Sep 8, 2010 at 6:12 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Wed, Sep 08, 2010 at 01:55:39PM +0900, Jassi Brar wrote:
>> On Tue, Sep 7, 2010 at 7:29 PM, Mark Brown
>> <broonie@opensource.wolfsonmicro.com> wrote:
>> > Allow the use of the S3C64xx SPI controller with things like PMICs by
>> > moving the init up to subsys_initcall().
>
>> Couldn't any user ever need to load it as a module?
>> If no, we might as well drop the s3c64xx_spi_exit and s3c64xx_spi_remove
>
> This doesn't prevent building as a module - when built as a module
> subsys_initcall() is identical to module_init(), the change will only
> affect the order in which things are done when the code is built into
> the kernel otherwise it's a noop.
I didn't know that, thanks for the info.

Acked-by: Jassi Brar <jassi.brar@samsung.com>

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Grant Likely]

On Wed, Sep 08, 2010 at 06:37:59PM +0900, Jassi Brar wrote:
> On Wed, Sep 8, 2010 at 6:12 PM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Wed, Sep 08, 2010 at 01:55:39PM +0900, Jassi Brar wrote:
> >> On Tue, Sep 7, 2010 at 7:29 PM, Mark Brown
> >> <broonie@opensource.wolfsonmicro.com> wrote:
> >> > Allow the use of the S3C64xx SPI controller with things like PMICs by
> >> > moving the init up to subsys_initcall().
> >
> >> Couldn't any user ever need to load it as a module?
> >> If no, we might as well drop the s3c64xx_spi_exit and s3c64xx_spi_remove
> >
> > This doesn't prevent building as a module - when built as a module
> > subsys_initcall() is identical to module_init(), the change will only
> > affect the order in which things are done when the code is built into
> > the kernel otherwise it's a noop.
> I didn't know that, thanks for the info.
> 
> Acked-by: Jassi Brar <jassi.brar@samsung.com>

Applied, thanks.

... but it seems to me that there is a systemic problem in the way the driver model is being used if SPI (and I2C) bus drivers need to be 'special' in this regard.  What are the ordering requirements on things like PMICs?  (My uneducated assumption is that other devices depend on them being enabled before being probed.)  Would it be better to have a mechanism to defer probing on certain devices until other devices are probed?  Then the relationships could be made explicit instead of the rather coarse-grained (and potentially fragile) approach of changing the init order.

g.

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Mark Brown]

On Wed, Sep 08, 2010 at 10:12:45AM -0600, Grant Likely wrote:

[reflowed into 80 columns]

> ... but it seems to me that there is a systemic problem in the way the
> driver model is being used if SPI (and I2C) bus drivers need to be
> 'special' in this regard.  What are the ordering requirements on things
> like PMICs?  (My uneducated assumption is that other devices depend on
> them being enabled before being probed.)  Would it be better to have a

Pretty much this, yes - if you might want to turn on your supplies
when you're probed it's rather helpful if the subsystem needed to
control the regulators is available when you probe.

> mechanism to defer probing on certain devices until other devices are
> probed?  Then the relationships could be made explicit instead of the
> rather coarse-grained (and potentially fragile) approach of changing the
> init order.

That would be much nicer (and this is far from the only case where we
need to deal with it) but it's a substantial change to core kernel
infrastructure so it's being worked around like this.  Given how all the
discussions about sorting and dependencies for suspend and resume went
it might be an uphill struggle to do much more, especially while we are
able to continue to deal with things fairly easily using the current
infrastructure.

Doing dependencies could get pretty complicated, especially once you
handle optional dependencies ("is this missing because it didn't probe
yet or because it's just not there?") so it's not entirely clear to me
that it's worth the hassle.

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Grant Likely]

On Wed, Sep 08, 2010 at 05:22:51PM +0100, Mark Brown wrote:
> On Wed, Sep 08, 2010 at 10:12:45AM -0600, Grant Likely wrote:
> 
> [reflowed into 80 columns]

Sorry about that.  I'm in the process of switching to mutt+vim for my
mailer.  gmail is just not efficient enough when managing patches.

> > ... but it seems to me that there is a systemic problem in the way the
> > driver model is being used if SPI (and I2C) bus drivers need to be
> > 'special' in this regard.  What are the ordering requirements on things
> > like PMICs?  (My uneducated assumption is that other devices depend on
> > them being enabled before being probed.)  Would it be better to have a
> 
> Pretty much this, yes - if you might want to turn on your supplies
> when you're probed it's rather helpful if the subsystem needed to
> control the regulators is available when you probe.
> 
> > mechanism to defer probing on certain devices until other devices are
> > probed?  Then the relationships could be made explicit instead of the
> > rather coarse-grained (and potentially fragile) approach of changing the
> > init order.
> 
> That would be much nicer (and this is far from the only case where we
> need to deal with it) but it's a substantial change to core kernel
> infrastructure so it's being worked around like this.  Given how all the
> discussions about sorting and dependencies for suspend and resume went
> it might be an uphill struggle to do much more, especially while we are
> able to continue to deal with things fairly easily using the current
> infrastructure.
> 
> Doing dependencies could get pretty complicated, especially once you
> handle optional dependencies ("is this missing because it didn't probe
> yet or because it's just not there?") so it's not entirely clear to me
> that it's worth the hassle.

I think it might be doable.  I had a similar problem with Ethernet
MACs and PHYs where the PHY was on a completely separate bus from the
MAC with zero guarantees on probe order.  I had some code that made it
simple to use a bus notifier to defer MAC initialization until the
required phy turned up and was probed.  I eventually abandoned it
because accessing the PHY could be deferred until .ndo_open() time.
However, it would be easy to resurrect, and might be a reasonable
solution.  At the very least it is worth an investigation.

g.

Re: [PATCH] spi/spi_s3c64xx: Move to subsys_initcall()

[Mark Brown]

On Wed, Sep 08, 2010 at 10:44:32AM -0600, Grant Likely wrote:
> On Wed, Sep 08, 2010 at 05:22:51PM +0100, Mark Brown wrote:

> > Doing dependencies could get pretty complicated, especially once you
> > handle optional dependencies ("is this missing because it didn't probe
> > yet or because it's just not there?") so it's not entirely clear to me
> > that it's worth the hassle.

> I think it might be doable.  I had a similar problem with Ethernet
> MACs and PHYs where the PHY was on a completely separate bus from the
> MAC with zero guarantees on probe order.  I had some code that made it
> simple to use a bus notifier to defer MAC initialization until the
> required phy turned up and was probed.  I eventually abandoned it
> because accessing the PHY could be deferred until .ndo_open() time.
> However, it would be easy to resurrect, and might be a reasonable
> solution.  At the very least it is worth an investigation.

If you've got stuff that'd be great - ASoC also has a deferral mechanism
implemented due to this song and dance.  Like I say, the main reason
I've never looked at it myself is that it's never been sufficiently much
of a practical problem for me to justify the effort.