Input: tca6416-keypad: Change to module_init()
Subsystems (4)
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
From: Magnus Damm <[email protected]>
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
---
drivers/input/keyboard/tca6416-keypad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/input/keyboard/tca6416-keypad.c +1 -1
@@ -369,7 +369,7 @@ static int __init tca6416_keypad_init(voreturn i2c_add_driver (&tca6416_keypad_driver); } -subsys_initcall (tca6416_keypad_init); +module_init (tca6416_keypad_init); static void __exit tca6416_keypad_exit(void) {
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 6 lines · Magnus Damm
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?
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 11 lines · Mark Brown
Quoted 6 lines · Magnus Damm
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?
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 10 lines · Paul Mundt
Quoted 9 lines · Mark Brown
Quoted 6 lines · Magnus Damm
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.
Quoted 5 lines
Quoted 2 lines
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
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 11 lines · Magnus Damm
Quoted 10 lines · Paul Mundt
Quoted 9 lines · Mark Brown
Quoted 6 lines · Magnus Damm
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.
Quoted 8 lines
Quoted 5 lines
Quoted 2 lines
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.
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 15 lines · Paul Mundt
Quoted 14 lines · Magnus Damm
Quoted 13 lines · Paul Mundt
Quoted 11 lines · Mark Brown
Quoted 6 lines · Magnus Damm
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?
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
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.
Quoted 4 lines
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?
Quoted 5 lines
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.
Quoted 1 line
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
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 9 lines · Magnus Damm
Quoted 5 lines · Paul Mundt
Quoted 2 lines · Magnus Damm
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.
Quoted 3 lines
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.
Quoted 4 lines
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?
Quoted 4 lines
Quoted 1 line
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.
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 10 lines · Paul Mundt
Quoted 9 lines · Magnus Damm
Quoted 5 lines · Paul Mundt
Quoted 2 lines · Magnus Damm
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.
Quoted 9 lines
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_initcallfor 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.
Quoted 6 lines
Quoted 3 lines
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.
Quoted 6 lines
Quoted 4 lines
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.
Quoted 9 lines
Quoted 4 lines
Quoted 1 line
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
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 9 lines · Magnus Damm
Quoted 3 lines · Paul Mundt
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.
Quoted 4 lines
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.
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 7 lines · Mark Brown
Quoted 5 lines · Magnus Damm
Quoted 3 lines · Paul Mundt
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.
Quoted 7 lines
Quoted 3 lines
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.
Quoted 9 lines
Quoted 4 lines
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
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 2 lines · Magnus Damm
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.
Quoted 2 lines
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.
Quoted 7 lines
Quoted 4 lines
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.
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 8 lines · Mark Brown
Quoted 2 lines · Magnus Damm
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.
Quoted 8 lines
Quoted 7 lines
Quoted 4 lines
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:
Quoted 6 lines
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
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 2 lines · Magnus Damm
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.
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 8 lines · Mark Brown
Quoted 2 lines · Magnus Damm
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
Dmitry Torokhov
maintainerPATCH 1/1 Input: tca6416-keypad: Change to module_init()
Quoted 16 lines · Magnus Damm
Quoted 8 lines · Mark Brown
Quoted 2 lines · Magnus Damm
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.
Quoted 8 lines
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.
Signature · 1 line
Dmitry
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
In reply to Magnus DammQuoted 4 lines · Magnus Damm
Quoted 2 lines · Mark Brown
On Wed, Mar 23, 2011 at 01:51:02AM +0900, Magnus Damm wrote:
Quoted 8 lines
Quoted 6 lines
Quoted 2 lines
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
PATCH 1/1 Input: tca6416-keypad: Change to module_init()
In reply to Mark BrownQuoted 11 lines · Mark Brown
Quoted 6 lines · Magnus Damm
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