Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter
On Wed, Mar 23, 2011 at 01:41:53AM -0400, Bernhard Walle wrote: > * Greg KH [2011-03-23 05:50]: > > > > It should, you can talk to the device from userspace using that virtual > > serial port, right? Isn't that what you need to do here? > > > > Why try to create a new type of interface to the device from what it was > > supposed to be by the manufacturer? > > The device is multi-function. It's quite common to use some (extended) > USB-to-serial-converters also for serial busses like I²C or SPI. I know > that from the FT2232C chip. > > Using such a device as normal Linux I²C device makes perfectly sense, > IMO. > Correct. I use and need it as I2C bus controller. FWIW, that is what it is sold for. User space access doesn't help me there. Anyway, the only reason for submitting the driver was that I thought it might be useful for others. Maybe not useful enough, and defintely not worth arguing about it. I'll make it available as stand-alone driver; that should be good enough. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter
* Greg KH [2011-03-23 05:50]: > > It should, you can talk to the device from userspace using that virtual > serial port, right? Isn't that what you need to do here? > > Why try to create a new type of interface to the device from what it was > supposed to be by the manufacturer? The device is multi-function. It's quite common to use some (extended) USB-to-serial-converters also for serial busses like I²C or SPI. I know that from the FT2232C chip. Using such a device as normal Linux I²C device makes perfectly sense, IMO. Regards, Bernhard -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter
On Tue, Mar 22, 2011 at 09:25:07PM -0700, Guenter Roeck wrote: > On Tue, Mar 22, 2011 at 11:45:24PM -0400, Greg KH wrote: > > On Tue, Mar 22, 2011 at 08:43:47PM -0700, Guenter Roeck wrote: > > > This patch adds support for the I2C interface of the Devantech USB-ISS > > > Multifunction adapter. > > > > > > Signed-off-by: Guenter Roeck > > > --- > > > The driver has one problem: It competes with the cdc_acm driver for device > > > access. Copying the usb mailing list in the hope that someone can tell me > > > if there is a way to prevent this from happening. > > > > Why does it "compete"? > > Is it because this driver also exposes a cdc-acm class interface? Why > > I guess so. > > > is it doing that if it doesn't follow that spec? We do already support > > I don't know if it follows the cdc-acm specification or not, though I would > think it does since the cdc-acm driver recognizes it. I do see the > "This device cannot do calls on its own. It is not a modem." message. > > I suspect the device exposes the cdc-acm class interface because it is > a convenient means to make it show up as COM port in Windows. > But I am not associated with the manufacturer, so that is just a wild guess. > > If it does follow the cdc-acm specification, does that help me anything ? It should, you can talk to the device from userspace using that virtual serial port, right? Isn't that what you need to do here? Why try to create a new type of interface to the device from what it was supposed to be by the manufacturer? thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter
On Tue, Mar 22, 2011 at 11:45:24PM -0400, Greg KH wrote: > On Tue, Mar 22, 2011 at 08:43:47PM -0700, Guenter Roeck wrote: > > This patch adds support for the I2C interface of the Devantech USB-ISS > > Multifunction adapter. > > > > Signed-off-by: Guenter Roeck > > --- > > The driver has one problem: It competes with the cdc_acm driver for device > > access. Copying the usb mailing list in the hope that someone can tell me > > if there is a way to prevent this from happening. > > Why does it "compete"? > Is it because this driver also exposes a cdc-acm class interface? Why I guess so. > is it doing that if it doesn't follow that spec? We do already support I don't know if it follows the cdc-acm specification or not, though I would think it does since the cdc-acm driver recognizes it. I do see the "This device cannot do calls on its own. It is not a modem." message. I suspect the device exposes the cdc-acm class interface because it is a convenient means to make it show up as COM port in Windows. But I am not associated with the manufacturer, so that is just a wild guess. If it does follow the cdc-acm specification, does that help me anything ? > a number of "quirks" in the cdc-acm driver, I don't see why we couldn't > add a "blacklist this device" one there as well to help you out here if > it's needed. > Maybe, but only as last resort. Thanks, Guenter -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter
On Tue, Mar 22, 2011 at 08:43:47PM -0700, Guenter Roeck wrote: > This patch adds support for the I2C interface of the Devantech USB-ISS > Multifunction adapter. > > Signed-off-by: Guenter Roeck > --- > The driver has one problem: It competes with the cdc_acm driver for device > access. Copying the usb mailing list in the hope that someone can tell me > if there is a way to prevent this from happening. Why does it "compete"? Is it because this driver also exposes a cdc-acm class interface? Why is it doing that if it doesn't follow that spec? We do already support a number of "quirks" in the cdc-acm driver, I don't see why we couldn't add a "blacklist this device" one there as well to help you out here if it's needed. thanks, greg k-h -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] i2c/busses: Driver for Devantech USB-ISS I2C adapter
This patch adds support for the I2C interface of the Devantech USB-ISS Multifunction adapter. Signed-off-by: Guenter Roeck --- The driver has one problem: It competes with the cdc_acm driver for device access. Copying the usb mailing list in the hope that someone can tell me if there is a way to prevent this from happening. Documentation/i2c/busses/i2c-devantech-iss | 31 ++ drivers/i2c/busses/Kconfig | 10 + drivers/i2c/busses/Makefile|1 + drivers/i2c/busses/i2c-devantech-iss.c | 495 4 files changed, 537 insertions(+), 0 deletions(-) create mode 100644 Documentation/i2c/busses/i2c-devantech-iss create mode 100644 drivers/i2c/busses/i2c-devantech-iss.c diff --git a/Documentation/i2c/busses/i2c-devantech-iss b/Documentation/i2c/busses/i2c-devantech-iss new file mode 100644 index 000..f8199fd --- /dev/null +++ b/Documentation/i2c/busses/i2c-devantech-iss @@ -0,0 +1,31 @@ +Kernel driver i2c-devantech-iss + +Supported adapters: + * Devantech USB-ISS Multifunction USB Communications Module +Documentation: +http://www.robot-electronics.co.uk/htm/usb_iss_tech.htm + +Author: Guenter Roeck + + +Description +--- + +This is the driver for the Devantech USB-ISS Multifunction USB Communications +Module. + +The USB-ISS Multifunction USB Communications Module provides an interface +to I2C, SPI, Serial port, and general purpose Analogue Input or Digital I/O. +The module is powered from USB. + +The driver only supports the I2C interface of USB-ISS. The driver does not use +interrupts. + +Clock stretching is supported for bus frequencies of 100,000 Hz and above. + + +Module parameters +- + +* frequency: I2C bus frequency in Hz + Supported frequencies are 20,000, 50,000, 100,000, 400,000, and 1,000,000 Hz. diff --git a/drivers/i2c/busses/Kconfig b/drivers/i2c/busses/Kconfig index 230601e..0bffacd 100644 --- a/drivers/i2c/busses/Kconfig +++ b/drivers/i2c/busses/Kconfig @@ -676,6 +676,16 @@ config I2C_EG20T comment "External I2C/SMBus adapter drivers" +config I2C_DEVANTECH + tristate "Devantech USB-ISS" + select I2C_SMBUS + help + This supports the I2C interface of the Devantech USB-ISS Multifunction + Communications Module. + + This driver can also be built as a module. If so, the module + will be called i2c-devantech-iss. + config I2C_PARPORT tristate "Parallel port adapter" depends on PARPORT diff --git a/drivers/i2c/busses/Makefile b/drivers/i2c/busses/Makefile index 3878c95..5ecec28 100644 --- a/drivers/i2c/busses/Makefile +++ b/drivers/i2c/busses/Makefile @@ -67,6 +67,7 @@ obj-$(CONFIG_I2C_XILINX) += i2c-xiic.o obj-$(CONFIG_I2C_EG20T) += i2c-eg20t.o # External I2C/SMBus adapter drivers +obj-$(CONFIG_I2C_DEVANTECH)+= i2c-devantech-iss.o obj-$(CONFIG_I2C_PARPORT) += i2c-parport.o obj-$(CONFIG_I2C_PARPORT_LIGHT)+= i2c-parport-light.o obj-$(CONFIG_I2C_TAOS_EVM) += i2c-taos-evm.o diff --git a/drivers/i2c/busses/i2c-devantech-iss.c b/drivers/i2c/busses/i2c-devantech-iss.c new file mode 100644 index 000..b14daa6 --- /dev/null +++ b/drivers/i2c/busses/i2c-devantech-iss.c @@ -0,0 +1,495 @@ +/* + * Driver for the Devantech USB-ISS Multifunction Communications Module + * + * Copyright (c) 2011 Ericsson AB + * + * Derived from: + * i2c-diolan-u2c.c + * Copyright (c) 2010-2011 Ericsson AB + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU General Public License as + * published by the Free Software Foundation, version 2. + */ + +#include +#include +#include +#include +#include +#include +#include + +#define DRIVER_NAME"i2c-devantech-iss" + +#define USB_VENDOR_ID_DEVANTECH0x04d8 +#define USB_DEVICE_ID_ISS 0xffee + +/* commands */ +#define ISS_CMD_I2C_DIRECT 0x57/* Custom I2C sequence */ +#define ISS_CMD_I2C_INTERNAL 0x5a/* internal commands */ + +/* direct commands */ +#define ISS_I2C_START 0x01 +#define ISS_I2C_RESTART0x02 +#define ISS_I2C_STOP 0x03 +#define ISS_I2C_NACK 0x04 +#define ISS_I2C_READ(n)(0x20 + (n) - 1) +#define ISS_I2C_WRITE(n) (0x30 + (n) - 1) + +/* internal commands */ +#define ISS_GET_VERSION0x01 +#define ISS_MODE 0x02 +#define ISS_GET_SERIAL 0x03 + +/* modes */ +#define ISS_MODE_I2C_S_20KHZ 0x20/* I2C, SW (bitbang), 20 kHz clock */ +#define ISS_MODE_I2C_S_50KHZ 0x30/* I2C, SW (bitbang), 50 kHz clock */ +#define ISS_MODE_I2C_S_100KHZ 0x40/* I2C, SW (bitbang), 100 kHz clock */ +#define ISS_MODE_I2C_S_400KHZ 0x50/* I2C, SW (bitbang), 400 kHz clock */ +#define ISS_MODE_I2C_H_100KHZ 0x60/* I2C, HW (PIC), 100 kHz clock */ +#define ISS_MODE_I2C_H_400KHZ 0x70/* I2C, HW (PIC), 400 kHz clock */ +#define ISS_MODE_I2C_H_1000KHZ 0x80/* I2C, HW
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Wed, Mar 23, 2011 at 1:31 AM, Mark Brown
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 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
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Wed, Mar 23, 2011 at 01:20:23AM +0900, Magnus Damm wrote: > On Wed, Mar 23, 2011 at 12:57 AM, Paul Mundt 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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Wed, Mar 23, 2011 at 12:57 AM, Paul Mundt 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 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Wed, Mar 23, 2011 at 12:43:54AM +0900, Magnus Damm wrote: > On Wed, Mar 23, 2011 at 12:32 AM, Paul Mundt 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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Wed, Mar 23, 2011 at 12:32 AM, Paul Mundt 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 wrote: >> > On Tue, Mar 22, 2011 at 02:28:55PM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Wed, Mar 23, 2011 at 12:22:05AM +0900, Magnus Damm wrote: > On Tue, Mar 22, 2011 at 11:33 PM, Paul Mundt wrote: > > On Tue, Mar 22, 2011 at 02:28:55PM +, 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. -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Tue, Mar 22, 2011 at 11:33 PM, Paul Mundt wrote: > On Tue, Mar 22, 2011 at 02:28:55PM +, 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Tue, Mar 22, 2011 at 11:28 PM, 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? 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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
On Tue, Mar 22, 2011 at 02:28:55PM +, 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? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Input: tca6416-keypad: Change to module_init()
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? -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] Input: tca6416-keypad: Change to module_init()
From: 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.
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
---
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.c2011-03-22
22:44:26.0 +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)
{
--
To unsubscribe from this list: send the line "unsubscribe linux-i2c" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
