[PATCH] Input: tca6416-keypad: Change to module_init()

[PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

From: Magnus Damm <damm@opensource.se>

The tca6416 driver makes use of the I2C bus for chatting
with the actual hardware device. Without this patch both
the I2C bus driver and the tca6416 driver are initialized
at the subsys_initcall() level. This may lead to problems
with the tca6416 driver being initialized before the I2C
bus driver.

By using module_init() in the tca6416 driver we make sure
the I2C bus driver always is initialized before the keypad
driver.

With this patch applied the boot order becomes:
 - arch_initcall: the ARM architecture ->init_machine()
 - arch_initcall: i2c_register_board_info()
 - arch_initcall: I2C bus device registration
 - subsys_initcall: I2C bus driver probe()
 - module_init: tca6416 driver probe()

Affects the following in-tree platforms:
 - arch/arm/mach-davinci/board-da850-evm.c
 - arch/arm/mach-omap2/board-am3517evm.c
 - arch/arm/mach-shmobile/board-mackerel.c

Signed-off-by: Magnus Damm <damm@opensource.se>
---

 drivers/input/keyboard/tca6416-keypad.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- 0001/drivers/input/keyboard/tca6416-keypad.c
+++ work/drivers/input/keyboard/tca6416-keypad.c	2011-03-22 22:44:26.000000000 +0900
@@ -369,7 +369,7 @@ static int __init tca6416_keypad_init(vo
 	return i2c_add_driver(&tca6416_keypad_driver);
 }
 
-subsys_initcall(tca6416_keypad_init);
+module_init(tca6416_keypad_init);
 
 static void __exit tca6416_keypad_exit(void)
 {

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Mark Brown]

On Tue, Mar 22, 2011 at 11:26:19PM +0900, Magnus Damm wrote:

> The tca6416 driver makes use of the I2C bus for chatting
> with the actual hardware device. Without this patch both
> the I2C bus driver and the tca6416 driver are initialized
> at the subsys_initcall() level. This may lead to problems
> with the tca6416 driver being initialized before the I2C
> bus driver.

While this change seems reasonable I'm curious what the problems caused
by out of order registration are?

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Paul Mundt]

On Tue, Mar 22, 2011 at 02:28:55PM +0000, Mark Brown wrote:
> On Tue, Mar 22, 2011 at 11:26:19PM +0900, Magnus Damm wrote:
> 
> > The tca6416 driver makes use of the I2C bus for chatting
> > with the actual hardware device. Without this patch both
> > the I2C bus driver and the tca6416 driver are initialized
> > at the subsys_initcall() level. This may lead to problems
> > with the tca6416 driver being initialized before the I2C
> > bus driver.
> 
> While this change seems reasonable I'm curious what the problems caused
> by out of order registration are?

I'm also curious as to why link order isn't a sufficient gaurantee like
it is for everyone else?

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Tue, Mar 22, 2011 at 11:28 PM, Mark Brown
<broonie@opensource.wolfsonmicro.com> wrote:
> On Tue, Mar 22, 2011 at 11:26:19PM +0900, Magnus Damm wrote:
>
>> The tca6416 driver makes use of the I2C bus for chatting
>> with the actual hardware device. Without this patch both
>> the I2C bus driver and the tca6416 driver are initialized
>> at the subsys_initcall() level. This may lead to problems
>> with the tca6416 driver being initialized before the I2C
>> bus driver.
>
> While this change seems reasonable I'm curious what the problems caused
> by out of order registration are?

The boot up time is delayed with 5 seconds or so due to a delay that
only happens if subsys_initcall() level is used for both tca6416 and
the I2C driver. By disabling the tca6416 driver in the kernel
configuration the delay goes away. I have not tracked it down further
than using module_init() instead of subsys_initcall() in the tca6416
driver. This because tca6416-keypad.c is the only keyboard driver that
uses subsys_initcall().

If I enable initcall_debug then I get this output:

calling  input_init+0x0/0x114 @ 1
initcall input_init+0x0/0x114 returned 0 after 0 usecs
calling  tca6416_keypad_init+0x0/0x10 @ 1
initcall tca6416_keypad_init+0x0/0x10 returned 0 after 0 usecs
calling  sh_mobile_i2c_adap_init+0x0/0xc @ 1
i2c-sh_mobile i2c-sh_mobile.0: clocks managed by runtime pm
input: tca6408-keys as
/devices/platform/i2c-sh_mobile.0/i2c-0/0-0020/input/input0
[delay]

If my interpretation of the log above is correct then
tca6416_keypad_init() is executed before sh_mobile_i2c_adap_init().

As for link order, i2c is located after input in driver/Makefile. I
believe that matches well with the log above.

Thanks,

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Tue, Mar 22, 2011 at 11:33 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Tue, Mar 22, 2011 at 02:28:55PM +0000, Mark Brown wrote:
>> On Tue, Mar 22, 2011 at 11:26:19PM +0900, Magnus Damm wrote:
>>
>> > The tca6416 driver makes use of the I2C bus for chatting
>> > with the actual hardware device. Without this patch both
>> > the I2C bus driver and the tca6416 driver are initialized
>> > at the subsys_initcall() level. This may lead to problems
>> > with the tca6416 driver being initialized before the I2C
>> > bus driver.
>>
>> While this change seems reasonable I'm curious what the problems caused
>> by out of order registration are?
>
> I'm also curious as to why link order isn't a sufficient gaurantee like
> it is for everyone else?

I believe all other i2c keyboard drivers use module_init().

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Paul Mundt]

On Wed, Mar 23, 2011 at 12:22:05AM +0900, Magnus Damm wrote:
> On Tue, Mar 22, 2011 at 11:33 PM, Paul Mundt <lethal@linux-sh.org> wrote:
> > On Tue, Mar 22, 2011 at 02:28:55PM +0000, Mark Brown wrote:
> >> On Tue, Mar 22, 2011 at 11:26:19PM +0900, Magnus Damm wrote:
> >>
> >> > The tca6416 driver makes use of the I2C bus for chatting
> >> > with the actual hardware device. Without this patch both
> >> > the I2C bus driver and the tca6416 driver are initialized
> >> > at the subsys_initcall() level. This may lead to problems
> >> > with the tca6416 driver being initialized before the I2C
> >> > bus driver.
> >>
> >> While this change seems reasonable I'm curious what the problems caused
> >> by out of order registration are?
> >
> > I'm also curious as to why link order isn't a sufficient gaurantee like
> > it is for everyone else?
> 
> I believe all other i2c keyboard drivers use module_init().
> 
We do not change initcall ordering around unless there is a reason to do
so, as it's assumed that a given initcall has been chosen for a reason.
You have hit upon a bug or at least something timing related causing you
a delay and so have elected to push it down a level. That is of course
fine, but none of that is anywhere in your commit text leaving us to try
and figure out what exactly the point of this exercise is.

Usually "because everyone else is doing it" and another driver is not,
there's a reason for that driver doing things differently. There are
certainly enough cases where initcall and link ordering is strongly
ordered for a reason that cosmetic/janitorial fixes are best rejected out
of hand.

You had a reason, great. Next time put it in your commit text.

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Wed, Mar 23, 2011 at 12:32 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Mar 23, 2011 at 12:22:05AM +0900, Magnus Damm wrote:
>> On Tue, Mar 22, 2011 at 11:33 PM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > On Tue, Mar 22, 2011 at 02:28:55PM +0000, Mark Brown wrote:
>> >> On Tue, Mar 22, 2011 at 11:26:19PM +0900, Magnus Damm wrote:
>> >>
>> >> > The tca6416 driver makes use of the I2C bus for chatting
>> >> > with the actual hardware device. Without this patch both
>> >> > the I2C bus driver and the tca6416 driver are initialized
>> >> > at the subsys_initcall() level. This may lead to problems
>> >> > with the tca6416 driver being initialized before the I2C
>> >> > bus driver.
>> >>
>> >> While this change seems reasonable I'm curious what the problems caused
>> >> by out of order registration are?
>> >
>> > I'm also curious as to why link order isn't a sufficient gaurantee like
>> > it is for everyone else?
>>
>> I believe all other i2c keyboard drivers use module_init().
>>
> We do not change initcall ordering around unless there is a reason to do
> so, as it's assumed that a given initcall has been chosen for a reason.

Yes, obviously this driver is special and all other keypad drivers are wrong.

It would be interesting to hear why subsys_initcall() was put there in
the first place.

> You have hit upon a bug or at least something timing related causing you
> a delay and so have elected to push it down a level. That is of course
> fine, but none of that is anywhere in your commit text leaving us to try
> and figure out what exactly the point of this exercise is.

The keypad driver tries to use the I2C bus before the I2C bus driver
is initialized. Isn't that a pretty good reason to change the initcall
order?

> Usually "because everyone else is doing it" and another driver is not,
> there's a reason for that driver doing things differently. There are
> certainly enough cases where initcall and link ordering is strongly
> ordered for a reason that cosmetic/janitorial fixes are best rejected out
> of hand.

So let's hear what other people have to say about this.

> You had a reason, great. Next time put it in your commit text.

Whatever. Let me know which lines you'd like to add and I'll send a V2.

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Paul Mundt]

On Wed, Mar 23, 2011 at 12:43:54AM +0900, Magnus Damm wrote:
> On Wed, Mar 23, 2011 at 12:32 AM, Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org> wrote:
> > On Wed, Mar 23, 2011 at 12:22:05AM +0900, Magnus Damm wrote:
> >> I believe all other i2c keyboard drivers use module_init().
> >>
> > We do not change initcall ordering around unless there is a reason to do
> > so, as it's assumed that a given initcall has been chosen for a reason.
> 
> Yes, obviously this driver is special and all other keypad drivers are wrong.
> 
I'm not sure why you're purposely trying to be dense. I was explaining
why it's not uncommon to find drivers using subsys_initcall for various
non-obvious reasons and why blindly changing them without valid rationale
generally causes more trouble than it prevents. In the case of a keypad
driver it's unlikely to matter, but someone may still have had a reason
for doing so. Given that your patch is fixing a problem, this is what
should be reflected in your commit text, not some vague hand-waving about
what everyone else is doing or what could hypothetically lead to
problems.

> It would be interesting to hear why subsys_initcall() was put there in
> the first place.
> 
In this case I would suspect general indifference or simply copying other
drivers. I2C is a bit of a tricky case with regards to ordering in
general, but at least input devices are relatively straightforward.

> The keypad driver tries to use the I2C bus before the I2C bus driver
> is initialized. Isn't that a pretty good reason to change the initcall
> order?
> 
Yes, and that part is also not mentioned anywhere in your commit text.
Starting to see a pattern?

> > You had a reason, great. Next time put it in your commit text.
> 
> Whatever. Let me know which lines you'd like to add and I'll send a V2.
> 
I don't think it's too much to ask that you write a commit text that
actually mentions what problem you are experiencing and fixing. I also
don't know why this needs to be pointed out. One shouldn't have to work
for an explanation of what purpose your patch serves when you're the one
trying to get it merged.

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Wed, Mar 23, 2011 at 12:57 AM, Paul Mundt <lethal@linux-sh.org> wrote:
> On Wed, Mar 23, 2011 at 12:43:54AM +0900, Magnus Damm wrote:
>> On Wed, Mar 23, 2011 at 12:32 AM, Paul Mundt <lethal@linux-sh.org> wrote:
>> > On Wed, Mar 23, 2011 at 12:22:05AM +0900, Magnus Damm wrote:
>> >> I believe all other i2c keyboard drivers use module_init().
>> >>
>> > We do not change initcall ordering around unless there is a reason to do
>> > so, as it's assumed that a given initcall has been chosen for a reason.
>>
>> Yes, obviously this driver is special and all other keypad drivers are wrong.
>>
> I'm not sure why you're purposely trying to be dense. I was explaining
> why it's not uncommon to find drivers using subsys_initcall for various
> non-obvious reasons and why blindly changing them without valid rationale
> generally causes more trouble than it prevents. In the case of a keypad
> driver it's unlikely to matter, but someone may still have had a reason
> for doing so. Given that your patch is fixing a problem, this is what
> should be reflected in your commit text, not some vague hand-waving about
> what everyone else is doing or what could hypothetically lead to
> problems.

Sure, including the problem description is of course a good thing.

>> It would be interesting to hear why subsys_initcall() was put there in
>> the first place.
>>
> In this case I would suspect general indifference or simply copying other
> drivers. I2C is a bit of a tricky case with regards to ordering in
> general, but at least input devices are relatively straightforward.

I remember having to move the init order around at least once before
in the case of i2c, so I'm not so surprised when new initcall issues
come up now and then.

Perhaps the original driver author remembers why subsys_initcall() was
put there.

>> The keypad driver tries to use the I2C bus before the I2C bus driver
>> is initialized. Isn't that a pretty good reason to change the initcall
>> order?
>>
> Yes, and that part is also not mentioned anywhere in your commit text.
> Starting to see a pattern?

"This may lead to problems with the tca6416 driver being initialized
before the I2C bus driver."

The "may" above comes from that I don't know the i2c bus driver
initcall time on non-SH-Mobile platforms. So this may trigger on other
platforms, or it may not depending on their cpu/board code and I2c bus
driver.

>> > You had a reason, great. Next time put it in your commit text.
>>
>> Whatever. Let me know which lines you'd like to add and I'll send a V2.
>>
> I don't think it's too much to ask that you write a commit text that
> actually mentions what problem you are experiencing and fixing. I also
> don't know why this needs to be pointed out. One shouldn't have to work
> for an explanation of what purpose your patch serves when you're the one
> trying to get it merged.

I agree that it's not too much to ask for. This patch did however have
20 lines of commit message for a one line change. Obviously the words
were not chosen well enough to please everyone, but compared to most
commit messages I read I believe my description was at least rather
detailed.

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Mark Brown]

On Wed, Mar 23, 2011 at 01:20:23AM +0900, Magnus Damm wrote:
> On Wed, Mar 23, 2011 at 12:57 AM, Paul Mundt <lethal@linux-sh.org> wrote:

> > In this case I would suspect general indifference or simply copying other
> > drivers. I2C is a bit of a tricky case with regards to ordering in
> > general, but at least input devices are relatively straightforward.

> I remember having to move the init order around at least once before
> in the case of i2c, so I'm not so surprised when new initcall issues
> come up now and then.

It's mostly an issue for PMICs (and possibly some other similar things)
so that the regulators are present before their consumers try to start.
I'm not aware of any issues with I2C itself.

> The "may" above comes from that I don't know the i2c bus driver
> initcall time on non-SH-Mobile platforms. So this may trigger on other
> platforms, or it may not depending on their cpu/board code and I2c bus
> driver.

In general embedded platforms register I2C early as things like PMICs
typically hang off them.  Grant was trying to push people to use
deferred registration for this stuff but it didn't happen yet and I'd
personally be more comfortable with more infastructure supporting that.

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Wed, Mar 23, 2011 at 1:31 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Mar 23, 2011 at 01:20:23AM +0900, Magnus Damm wrote:
>> On Wed, Mar 23, 2011 at 12:57 AM, Paul Mundt <lethal-M7jkjyW5wf5g9hUCZPvPmw@public.gmane.org> wrote:
>
>> > In this case I would suspect general indifference or simply copying other
>> > drivers. I2C is a bit of a tricky case with regards to ordering in
>> > general, but at least input devices are relatively straightforward.
>
>> I remember having to move the init order around at least once before
>> in the case of i2c, so I'm not so surprised when new initcall issues
>> come up now and then.
>
> It's mostly an issue for PMICs (and possibly some other similar things)
> so that the regulators are present before their consumers try to start.
> I'm not aware of any issues with I2C itself.

I recall changing my i2c bus driver i2c-sh_mobile.c from module_init()
to subsys_initcall() a few years ago (see
ccb3bc16b4891a82649d4bccbeefe60b1d9a62e2) - this because regular
drivers tend to use module_init() and following the link order is not
enough. I think the patch for tca6416 is more or less the same issue
but on subsys_initcall() level instead.

I'm not sure if this is i2c specific though - in general you probably
need to register the parent bus driver first.

I've seen some SH-Mobile designs with PMICs, and they all use a
dedicated i2c bus.

>> The "may" above comes from that I don't know the i2c bus driver
>> initcall time on non-SH-Mobile platforms. So this may trigger on other
>> platforms, or it may not depending on their cpu/board code and I2c bus
>> driver.
>
> In general embedded platforms register I2C early as things like PMICs
> typically hang off them.  Grant was trying to push people to use
> deferred registration for this stuff but it didn't happen yet and I'd
> personally be more comfortable with more infastructure supporting that.

The dependency tracking is a bit primitive with only initcalls. I
wouldn't mind something like this:

-static int __init tca6416_keypad_init(void)
-{
-	return i2c_add_driver(&tca6416_keypad_driver);
-}
-
-subsys_initcall(tca6416_keypad_init);
-
-static void __exit tca6416_keypad_exit(void)
-{
-	i2c_del_driver(&tca6416_keypad_driver);
-}
-module_exit(tca6416_keypad_exit);
+i2c_driver_module(&tca6416_keypad_driver);

Cheers,

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Mark Brown]

On Wed, Mar 23, 2011 at 01:51:02AM +0900, Magnus Damm wrote:

> I'm not sure if this is i2c specific though - in general you probably
> need to register the parent bus driver first.

This was my original question - I'm not aware of any reason why you
should need to register the driver for the specific bus first, it seems
odd.

> I've seen some SH-Mobile designs with PMICs, and they all use a
> dedicated i2c bus.

It's the most common bus for PMICs but some use SPI for performance
reasons, especially if they integrate other mixed signal functionality
such as touchscreen controllers that might be high volume.

> > In general embedded platforms register I2C early as things like PMICs
> > typically hang off them.  Grant was trying to push people to use
> > deferred registration for this stuff but it didn't happen yet and I'd
> > personally be more comfortable with more infastructure supporting that.

> The dependency tracking is a bit primitive with only initcalls. I
> wouldn't mind something like this:

That's not a general solution as it doesn't cover things like cross
dependencies between devices on the same bus type.  You really want to
be able to say "I depend on this set of other devices" somehow, the
proposal from Grant that I'm talking about was to only register devices
once their dependencies had appeared which solves the issue but is a bit
manual for the board files.

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Wed, Mar 23, 2011 at 1:59 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Wed, Mar 23, 2011 at 01:51:02AM +0900, Magnus Damm wrote:
>
>> I'm not sure if this is i2c specific though - in general you probably
>> need to register the parent bus driver first.
>
> This was my original question - I'm not aware of any reason why you
> should need to register the driver for the specific bus first, it seems
> odd.

Can't see why it wouldn't work - in theory at least. I'll have a look tomorrow.

The use of subsys_initcall() in the keypad driver is also a bit odd IMO.

>> > In general embedded platforms register I2C early as things like PMICs
>> > typically hang off them.  Grant was trying to push people to use
>> > deferred registration for this stuff but it didn't happen yet and I'd
>> > personally be more comfortable with more infastructure supporting that.
>
>> The dependency tracking is a bit primitive with only initcalls. I
>> wouldn't mind something like this:
>
> That's not a general solution as it doesn't cover things like cross
> dependencies between devices on the same bus type.  You really want to
> be able to say "I depend on this set of other devices" somehow, the
> proposal from Grant that I'm talking about was to only register devices
> once their dependencies had appeared which solves the issue but is a bit
> manual for the board files.

Sounds nice. Hopefully that will allow us to use module_init() in
drivers instead of other initcall levels depending on bus type.

Thanks,

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Mark Brown]

On Thu, Mar 24, 2011 at 01:04:31AM +0900, Magnus Damm wrote:

> Sounds nice. Hopefully that will allow us to use module_init() in
> drivers instead of other initcall levels depending on bus type.

None of this is anything to do with the bus type - it's to do with the
function of the devices within the system.  Things that are required for
early init of the kernel get bumped up the list.

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Thu, Mar 24, 2011 at 1:20 AM, Mark Brown
<broonie-yzvPICuk2AATkU/dhu1WVueM+bqZidxxQQ4Iyu8u01E@public.gmane.org> wrote:
> On Thu, Mar 24, 2011 at 01:04:31AM +0900, Magnus Damm wrote:
>
>> Sounds nice. Hopefully that will allow us to use module_init() in
>> drivers instead of other initcall levels depending on bus type.
>
> None of this is anything to do with the bus type - it's to do with the
> function of the devices within the system.  Things that are required for
> early init of the kernel get bumped up the list.

Hm, sorry for being unclear - I should have used the words "other
initcall levels depending on the init order of the parent hardware
bus". I'm referring to the state of things now without any patches
from Grant.

I understand that early init code often are using different initcall
levels to make sure they are getting initialized before other code.
Initcalls can also be used to work around link order issues.

I can't however see the reason why a keypad driver needs to be
initialized early on. I may be wrong, but I thought user space was the
only consumer of input devices?

/ magnus

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Dmitry Torokhov]

On Thu, Mar 24, 2011 at 12:38:21PM +0900, Magnus Damm wrote:
> On Thu, Mar 24, 2011 at 1:20 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
> > On Thu, Mar 24, 2011 at 01:04:31AM +0900, Magnus Damm wrote:
> >
> >> Sounds nice. Hopefully that will allow us to use module_init() in
> >> drivers instead of other initcall levels depending on bus type.
> >
> > None of this is anything to do with the bus type - it's to do with the
> > function of the devices within the system.  Things that are required for
> > early init of the kernel get bumped up the list.
> 
> Hm, sorry for being unclear - I should have used the words "other
> initcall levels depending on the init order of the parent hardware
> bus". I'm referring to the state of things now without any patches
> from Grant.
> 
> I understand that early init code often are using different initcall
> levels to make sure they are getting initialized before other code.
> Initcalls can also be used to work around link order issues.
> 
> I can't however see the reason why a keypad driver needs to be
> initialized early on. I may be wrong, but I thought user space was the
> only consumer of input devices?
> 

SysRq is kind of a consumer, so there sometimes a desire to get keyboard
working as soon as possible. But even SysRq is initialized ad module_init
level, so I tend to agree that tca should simply use module_init.

-- 
Dmitry

Re: [PATCH] Input: tca6416-keypad: Change to module_init()

[Magnus Damm]

On Thu, Mar 24, 2011 at 1:04 AM, Magnus Damm <magnus.damm@gmail.com> wrote:
> On Wed, Mar 23, 2011 at 1:59 AM, Mark Brown
> <broonie@opensource.wolfsonmicro.com> wrote:
>> On Wed, Mar 23, 2011 at 01:51:02AM +0900, Magnus Damm wrote:
>>
>>> I'm not sure if this is i2c specific though - in general you probably
>>> need to register the parent bus driver first.
>>
>> This was my original question - I'm not aware of any reason why you
>> should need to register the driver for the specific bus first, it seems
>> odd.
>
> Can't see why it wouldn't work - in theory at least. I'll have a look tomorrow.

Ok, it seems like the I2C bus registration is working as expected.
After digging into this a bit more I've discovered that the root cause
seems to be related to clocks. On sh7372 the I2C controller and the
serial console share the parent clock, and the setup order is
affecting the serial console.

Without this patch, if subsys_init() is used for tca6416-keypad then
the I2C controller will be used at probe() time from the function
tca6416_setup_registers() - this before the serial console driver has
been initialized properly. The I2C controller used by tca6416 will
stop the parent clock when the I2C bus access is finished. The serial
console driver is driven early using earlyprintk at this time, and the
clock is assumed to be enabled until the real driver is installed at
module_init() time. So with subsys_init() in tca6416 the I2C
controller stops the clock while the earlyprintk code assumes it to be
on. This is causing delays.

If the tca6416 driver is changed to module_init() then the serial
console driver will be initialized before the tca6416 driver due to
the linking order and all is working fine.

As we already knew, the clock framework code in drivers/sh/clk should
be updated to turn off unused clocks. To fix the issue in this mail
thread we should also update the code to force the console clock to be
enabled early on to make sure the serial console clock isn't turned
off by accident.

I propose that we convert the tca6416 driver to module_init() as a
short term fix _and_ at the same time work on improving the clock
framework code.

Can anyone think of a reason why the tca6416 driver shouldn't be
changed to module_init()?

Thanks,

/ magnus