| Message ID | 1308042491-20203-4-git-send-email-david@protonic.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > Use a threaded interrupt handler in order to permit the handler to use > a GPIO driver that causes things like I2C transactions being done inside > the handler context. > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure > all needed GPIO drivers have been loaded if the drivers are built into the > kernel. ...which is a horrid hack, but until device dependencies can be described, it isn't one that can be solved easily. This patch looks okay to me. Acked-by: Grant Likely <grant.likely@secretlab.ca> g. > > Signed-off-by: David Jander <david@protonic.nl> > --- > drivers/input/keyboard/gpio_keys.c | 8 ++++---- > 1 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c > index 78aeeaa..d179861 100644 > --- a/drivers/input/keyboard/gpio_keys.c > +++ b/drivers/input/keyboard/gpio_keys.c > @@ -3,7 +3,7 @@ > * > * Copyright 2005 Phil Blundell > * > - * Added OF support: > + * Added OF support and enabled use with I2C GPIO expanders: > * Copyright 2010 David Jander <david@protonic.nl> > * > * This program is free software; you can redistribute it and/or modify > @@ -417,7 +417,7 @@ static int __devinit gpio_keys_setup_key(struct device *dev, > if (!button->can_disable) > irqflags |= IRQF_SHARED; > > - error = request_any_context_irq(irq, gpio_keys_isr, irqflags, desc, bdata); > + error = request_threaded_irq(irq, NULL, gpio_keys_isr, irqflags, desc, bdata); > if (error < 0) { > dev_err(dev, "Unable to claim irq %d; error %d\n", > irq, error); > @@ -767,10 +767,10 @@ static void __exit gpio_keys_exit(void) > platform_driver_unregister(&gpio_keys_device_driver); > } > > -module_init(gpio_keys_init); > +late_initcall(gpio_keys_init); > module_exit(gpio_keys_exit); > > MODULE_LICENSE("GPL"); > MODULE_AUTHOR("Phil Blundell <pb@handhelds.org>"); > -MODULE_DESCRIPTION("Keyboard driver for CPU GPIOs"); > +MODULE_DESCRIPTION("Keyboard driver for GPIOs"); > MODULE_ALIAS("platform:gpio-keys"); > -- > 1.7.4.1 > -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > > Use a threaded interrupt handler in order to permit the handler to use > > a GPIO driver that causes things like I2C transactions being done inside > > the handler context. > > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure > > all needed GPIO drivers have been loaded if the drivers are built into the > > kernel. > > ...which is a horrid hack, but until device dependencies can be > described, it isn't one that can be solved easily. > I really do not want to apply this... Currently the order of initialization does not matter since nothing actually happens until corresponding device appears on the bus. Does the OF code creates devices before all resources are ready? Thanks.
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: >> > Use a threaded interrupt handler in order to permit the handler to use >> > a GPIO driver that causes things like I2C transactions being done inside >> > the handler context. >> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure >> > all needed GPIO drivers have been loaded if the drivers are built into the >> > kernel. >> >> ...which is a horrid hack, but until device dependencies can be >> described, it isn't one that can be solved easily. >> > > I really do not want to apply this... Currently the order of > initialization does not matter since nothing actually happens until > corresponding device appears on the bus. Does the OF code creates > devices before all resources are ready? It's not an OF problem. The problem is that all the platform_devices typically get registered all at once at machine_init time (on arm), and if the gpio expander isn't a platform_device, (like an i2c gpio expander which would end up being a child of a platform_device), then it won't be ready. The real problem is that we have no mechanism for holding off or deferring a driver probe if it depends on an asynchronous resource. g. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote: > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > >> > Use a threaded interrupt handler in order to permit the handler to use > >> > a GPIO driver that causes things like I2C transactions being done inside > >> > the handler context. > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure > >> > all needed GPIO drivers have been loaded if the drivers are built into the > >> > kernel. > >> > >> ...which is a horrid hack, but until device dependencies can be > >> described, it isn't one that can be solved easily. > >> > > > > I really do not want to apply this... Currently the order of > > initialization does not matter since nothing actually happens until > > corresponding device appears on the bus. Does the OF code creates > > devices before all resources are ready? > > It's not an OF problem. The problem is that all the platform_devices > typically get registered all at once at machine_init time (on arm), > and if the gpio expander isn't a platform_device, (like an i2c gpio > expander which would end up being a child of a platform_device), then > it won't be ready. Ah, I see. But that can be handled in board code that should ensure that it registers devices in correct order. > The real problem is that we have no mechanism for > holding off or deferring a driver probe if it depends on an > asynchronous resource. The mechanism we do have - we should not be creating the device for the driver to bind to unless all resources that are needed by that device are ready. Just shuffling the initcall order is not maintanable. Next there will be GPIO expander that is for some reason registered as late_initcall and we'll be back to square one. I am going to take the threaded IRQ bit but will drop the initcall bit from the patch. Thanks.
On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote: > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote: > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov > > <dmitry.torokhov@gmail.com> wrote: > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > > >> > Use a threaded interrupt handler in order to permit the handler to use > > >> > a GPIO driver that causes things like I2C transactions being done inside > > >> > the handler context. > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to make sure > > >> > all needed GPIO drivers have been loaded if the drivers are built into the > > >> > kernel. > > >> > > >> ...which is a horrid hack, but until device dependencies can be > > >> described, it isn't one that can be solved easily. > > >> > > > > > > I really do not want to apply this... Currently the order of > > > initialization does not matter since nothing actually happens until > > > corresponding device appears on the bus. Does the OF code creates > > > devices before all resources are ready? > > > > It's not an OF problem. The problem is that all the platform_devices > > typically get registered all at once at machine_init time (on arm), > > and if the gpio expander isn't a platform_device, (like an i2c gpio > > expander which would end up being a child of a platform_device), then > > it won't be ready. > > Ah, I see. But that can be handled in board code that should ensure that > it registers devices in correct order. Unfortunately, handling it in board code doesn't really work either. It just shuffles the complexity to the board code to implement some kind of deferred mechanism for registering devices, and it has to take into account that it may be a long time before the device actually appears, such as when the driver is configured as a module. I completely agree that shuffling initcall order isn't maintainable though. A related concern is that changing the device registration order, or the initcall order, does absolutely nothing to tell runtime PM about the dependencies between devices. For instance, how does runtime PM know when it is safe to PM a gpio controller, when it has no reference to devices depending on it, like gpio-keys? (although gpio-keys isn't a great example because it doesn't really have any runtime PM states). I think part of the solution is to give drivers the option of returning a 'defer' code at probe time if it cannot obtain all it's resources, and have the driver core re-probe it when more devices become available, but I haven't had time to prototype it yet. g. > > > The real problem is that we have no mechanism for > > holding off or deferring a driver probe if it depends on an > > asynchronous resource. > > The mechanism we do have - we should not be creating the device for the > driver to bind to unless all resources that are needed by that device > are ready. > > Just shuffling the initcall order is not maintanable. Next there will be > GPIO expander that is for some reason registered as late_initcall and > we'll be back to square one. I am going to take the threaded IRQ bit but > will drop the initcall bit from the patch. > > Thanks. > > -- > Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, 18 Jun 2011 09:16:45 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote: > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote: > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov > > > <dmitry.torokhov@gmail.com> wrote: > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > > > >> > Use a threaded interrupt handler in order to permit the handler to > > > >> > use a GPIO driver that causes things like I2C transactions being > > > >> > done inside the handler context. > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to > > > >> > make sure all needed GPIO drivers have been loaded if the drivers > > > >> > are built into the kernel. > > > >> > > > >> ...which is a horrid hack, but until device dependencies can be > > > >> described, it isn't one that can be solved easily. > > > >> > > > > > > > > I really do not want to apply this... Currently the order of > > > > initialization does not matter since nothing actually happens until > > > > corresponding device appears on the bus. Does the OF code creates > > > > devices before all resources are ready? > > > > > > It's not an OF problem. The problem is that all the platform_devices > > > typically get registered all at once at machine_init time (on arm), > > > and if the gpio expander isn't a platform_device, (like an i2c gpio > > > expander which would end up being a child of a platform_device), then > > > it won't be ready. > > > > Ah, I see. But that can be handled in board code that should ensure that > > it registers devices in correct order. > > Unfortunately, handling it in board code doesn't really work either. > It just shuffles the complexity to the board code to implement some > kind of deferred mechanism for registering devices, and it has to take > into account that it may be a long time before the device actually > appears, such as when the driver is configured as a module. Besides... we don't want anymore board-code, do we? I mean, if a board can use a generic board configuration and specify all it needs in the device-tree, why should something as trivial as connecting a gpio_keys device to a I2C GPIO expander force us to do special board setup all of a sudden? IMHO specifying I2C-gpios to be used for gpio_keys should "just work", even if declared in a device-tree. > I completely agree that shuffling initcall order isn't maintainable > though. I also agree, and if there is a better solution to make this work without additional board-support code, please tell me. I just think that this patch makes the already cool gpio_keys driver quite a bit more awesome. IMO, being able to just hook it all up in the device-tree is just fantastic, and we should make it possible. > A related concern is that changing the device registration order, or > the initcall order, does absolutely nothing to tell runtime PM about > the dependencies between devices. For instance, how does runtime PM > know when it is safe to PM a gpio controller, when it has no reference > to devices depending on it, like gpio-keys? (although gpio-keys isn't > a great example because it doesn't really have any runtime PM states). > > I think part of the solution is to give drivers the option of > returning a 'defer' code at probe time if it cannot obtain all it's > resources, and have the driver core re-probe it when more devices > become available, but I haven't had time to prototype it yet. Sounds interesting. So the probe function could return some sort of -ENOTYET or -EAGAIN and have it called again later? But, does that mean that we really need to miss this use-case until something like this gets approved and merged? Can't we just declare this late_initcall for now and fix it later? Please! > > > The real problem is that we have no mechanism for > > > holding off or deferring a driver probe if it depends on an > > > asynchronous resource. > > > > The mechanism we do have - we should not be creating the device for the > > driver to bind to unless all resources that are needed by that device > > are ready. How would we do that in a device-tree? > > Just shuffling the initcall order is not maintanable. Next there will be > > GPIO expander that is for some reason registered as late_initcall and > > we'll be back to square one. I am going to take the threaded IRQ bit but > > will drop the initcall bit from the patch. That would destroy the whole purpose of this patch. Do you mean to say, what I want to do has no acceptable implementation? That would be a pity, since IMHO it is a very cool feature, and quite trivial to implement this way. Our boards do not need any board setup code. Actually just adding one line of code in arch/powerpc/platforms/512x/mpc5121_generic.c or arch/powerpc/platforms/52xx/mpc5200_simple.c is enough to support any of our boards that need this driver... the rest is done in the device-tree. Don't you think this is worth that little bit of (temporary) ugliness? Best regards,
On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote: > On Sat, 18 Jun 2011 09:16:45 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > > > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote: > > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote: > > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > > > > >> > Use a threaded interrupt handler in order to permit the handler to > > > > >> > use a GPIO driver that causes things like I2C transactions being > > > > >> > done inside the handler context. > > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to > > > > >> > make sure all needed GPIO drivers have been loaded if the drivers > > > > >> > are built into the kernel. > > > > >> > > > > >> ...which is a horrid hack, but until device dependencies can be > > > > >> described, it isn't one that can be solved easily. > > > > >> > > > > > > > > > > I really do not want to apply this... Currently the order of > > > > > initialization does not matter since nothing actually happens until > > > > > corresponding device appears on the bus. Does the OF code creates > > > > > devices before all resources are ready? > > > > > > > > It's not an OF problem. The problem is that all the platform_devices > > > > typically get registered all at once at machine_init time (on arm), > > > > and if the gpio expander isn't a platform_device, (like an i2c gpio > > > > expander which would end up being a child of a platform_device), then > > > > it won't be ready. > > > > > > Ah, I see. But that can be handled in board code that should ensure that > > > it registers devices in correct order. > > > > Unfortunately, handling it in board code doesn't really work either. > > It just shuffles the complexity to the board code to implement some > > kind of deferred mechanism for registering devices, and it has to take > > into account that it may be a long time before the device actually > > appears, such as when the driver is configured as a module. > > Besides... we don't want anymore board-code, do we? I mean, if a board can use > a generic board configuration and specify all it needs in the device-tree, why > should something as trivial as connecting a gpio_keys device to a I2C GPIO > expander force us to do special board setup all of a sudden? > IMHO specifying I2C-gpios to be used for gpio_keys should "just work", even if > declared in a device-tree. This is a laudable goal, but then device-tree needs to be able to express device dependencies better. Until then board-specific code is needed to register devices in proper order. > > > I completely agree that shuffling initcall order isn't maintainable > > though. > > I also agree, and if there is a better solution to make this work without > additional board-support code, please tell me. > I just think that this patch makes the already cool gpio_keys driver quite a > bit more awesome. IMO, being able to just hook it all up in the device-tree is > just fantastic, and we should make it possible. > > > A related concern is that changing the device registration order, or > > the initcall order, does absolutely nothing to tell runtime PM about > > the dependencies between devices. For instance, how does runtime PM > > know when it is safe to PM a gpio controller, when it has no reference > > to devices depending on it, like gpio-keys? (although gpio-keys isn't > > a great example because it doesn't really have any runtime PM states). > > > > I think part of the solution is to give drivers the option of > > returning a 'defer' code at probe time if it cannot obtain all it's > > resources, and have the driver core re-probe it when more devices > > become available, but I haven't had time to prototype it yet. > > Sounds interesting. So the probe function could return some sort of -ENOTYET > or -EAGAIN and have it called again later? How about we do not register device until all resources are ready? This is pretty simple concept - do not create an object until it is usable. Then nobody needs to bother with -EAGAIN or -ENOTYET or any other similar garbage. > > But, does that mean that we really need to miss this use-case until something > like this gets approved and merged? Can't we just declare this late_initcall > for now and fix it later? Please! > > > > > The real problem is that we have no mechanism for > > > > holding off or deferring a driver probe if it depends on an > > > > asynchronous resource. > > > > > > The mechanism we do have - we should not be creating the device for the > > > driver to bind to unless all resources that are needed by that device > > > are ready. > > How would we do that in a device-tree? > > > > Just shuffling the initcall order is not maintanable. Next there will be > > > GPIO expander that is for some reason registered as late_initcall and > > > we'll be back to square one. I am going to take the threaded IRQ bit but > > > will drop the initcall bit from the patch. > > That would destroy the whole purpose of this patch. No, it is still useful as it will allow using the driver with GPIOs accessed over a slow bus. > Do you mean to say, what I > want to do has no acceptable implementation? That would be a pity, since IMHO > it is a very cool feature, and quite trivial to implement this way. > Our boards do not need any board setup code. Actually just adding one > line of code in arch/powerpc/platforms/512x/mpc5121_generic.c or > arch/powerpc/platforms/52xx/mpc5200_simple.c is enough to support any of our > boards that need this driver... the rest is done in the device-tree. Don't you > think this is worth that little bit of (temporary) ugliness? Turning the question around, can you add secondary device tree traversal for gpio_keys to your board code and keep the ugliness there until device tree can better express dependencies between resources? Thanks.
On Mon, 20 Jun 2011 01:45:12 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote: > > On Sat, 18 Jun 2011 09:16:45 -0600 > > Grant Likely <grant.likely@secretlab.ca> wrote: > > > > > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote: > > > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote: > > > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov > > > > > <dmitry.torokhov@gmail.com> wrote: > > > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > > > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > > > > > >> > Use a threaded interrupt handler in order to permit the handler > > > > > >> > to use a GPIO driver that causes things like I2C transactions > > > > > >> > being done inside the handler context. > > > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to > > > > > >> > make sure all needed GPIO drivers have been loaded if the > > > > > >> > drivers are built into the kernel. > > > > > >> > > > > > >> ...which is a horrid hack, but until device dependencies can be > > > > > >> described, it isn't one that can be solved easily. > > > > > >> > > > > > > > > > > > > I really do not want to apply this... Currently the order of > > > > > > initialization does not matter since nothing actually happens until > > > > > > corresponding device appears on the bus. Does the OF code creates > > > > > > devices before all resources are ready? > > > > > > > > > > It's not an OF problem. The problem is that all the platform_devices > > > > > typically get registered all at once at machine_init time (on arm), > > > > > and if the gpio expander isn't a platform_device, (like an i2c gpio > > > > > expander which would end up being a child of a platform_device), then > > > > > it won't be ready. > > > > > > > > Ah, I see. But that can be handled in board code that should ensure > > > > that it registers devices in correct order. > > > > > > Unfortunately, handling it in board code doesn't really work either. > > > It just shuffles the complexity to the board code to implement some > > > kind of deferred mechanism for registering devices, and it has to take > > > into account that it may be a long time before the device actually > > > appears, such as when the driver is configured as a module. > > > > Besides... we don't want anymore board-code, do we? I mean, if a board can > > use a generic board configuration and specify all it needs in the > > device-tree, why should something as trivial as connecting a gpio_keys > > device to a I2C GPIO expander force us to do special board setup all of a > > sudden? IMHO specifying I2C-gpios to be used for gpio_keys should "just > > work", even if declared in a device-tree. > > This is a laudable goal, but then device-tree needs to be able to > express device dependencies better. Until then board-specific code is > needed to register devices in proper order. Hmmm, I am not an expert in OF/DT stuff, but I think that while it would theoretically be possible to add extra properties to the tree, that are handled by the of_platform code, I am not sure if that is an option, since that would be pretty much linux-specific, and could never work on another OS. Grant? > > > I completely agree that shuffling initcall order isn't maintainable > > > though. > > > > I also agree, and if there is a better solution to make this work without > > additional board-support code, please tell me. > > I just think that this patch makes the already cool gpio_keys driver quite > > a bit more awesome. IMO, being able to just hook it all up in the > > device-tree is just fantastic, and we should make it possible. > > > > > A related concern is that changing the device registration order, or > > > the initcall order, does absolutely nothing to tell runtime PM about > > > the dependencies between devices. For instance, how does runtime PM > > > know when it is safe to PM a gpio controller, when it has no reference > > > to devices depending on it, like gpio-keys? (although gpio-keys isn't > > > a great example because it doesn't really have any runtime PM states). > > > > > > I think part of the solution is to give drivers the option of > > > returning a 'defer' code at probe time if it cannot obtain all it's > > > resources, and have the driver core re-probe it when more devices > > > become available, but I haven't had time to prototype it yet. > > > > Sounds interesting. So the probe function could return some sort of > > -ENOTYET or -EAGAIN and have it called again later? > > How about we do not register device until all resources are ready? This > is pretty simple concept - do not create an object until it is usable. Then > nobody needs to bother with -EAGAIN or -ENOTYET or any other similar > garbage. I agree, but DT doesn't permit that (yet). > > But, does that mean that we really need to miss this use-case until > > something like this gets approved and merged? Can't we just declare this > > late_initcall for now and fix it later? Please! > > > > > > > The real problem is that we have no mechanism for > > > > > holding off or deferring a driver probe if it depends on an > > > > > asynchronous resource. > > > > > > > > The mechanism we do have - we should not be creating the device for the > > > > driver to bind to unless all resources that are needed by that device > > > > are ready. > > > > How would we do that in a device-tree? > > > > > > Just shuffling the initcall order is not maintanable. Next there will > > > > be GPIO expander that is for some reason registered as late_initcall > > > > and we'll be back to square one. I am going to take the threaded IRQ > > > > bit but will drop the initcall bit from the patch. > > > > That would destroy the whole purpose of this patch. > > No, it is still useful as it will allow using the driver with GPIOs > accessed over a slow bus. Ok, that's true. Problem is that such slow busses (usually I2C or SPI) most probably have this dependency problem also, so it is a general problem that needs a solution. > > Do you mean to say, what I > > want to do has no acceptable implementation? That would be a pity, since > > IMHO it is a very cool feature, and quite trivial to implement this way. > > Our boards do not need any board setup code. Actually just adding one > > line of code in arch/powerpc/platforms/512x/mpc5121_generic.c or > > arch/powerpc/platforms/52xx/mpc5200_simple.c is enough to support any of > > our boards that need this driver... the rest is done in the device-tree. > > Don't you think this is worth that little bit of (temporary) ugliness? > > Turning the question around, can you add secondary device tree traversal > for gpio_keys to your board code and keep the ugliness there until > device tree can better express dependencies between resources? What do you think, Grant? Would it be possible/acceptable to add some special property to devices that could make them be added in a second round by of_platform code (until there are _real_ dependencies)? Or could the of_platform code be smart and just notice that gpio_keys needs "gpios" (or other resources for that matter) that are depending on another node in the tree, and make sure it gets probed before adding this one? I just don't want to give up on that feature now... besides, now that ARM will hopefully also adopt OF/DT, more and more drivers will need to be adapted, and this problem will repeat more sooner than later I guess. Thanks a lot. Best regards,
On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote: > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote: >> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: >>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: >>>> Use a threaded interrupt handler in order to permit the handler to use >>>> a GPIO driver that causes things like I2C transactions being done inside >>>> the handler context. >>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make sure >>>> all needed GPIO drivers have been loaded if the drivers are built into the >>>> kernel. >>> >>> ...which is a horrid hack, but until device dependencies can be >>> described, it isn't one that can be solved easily. >>> >> >> I really do not want to apply this... Currently the order of >> initialization does not matter since nothing actually happens until >> corresponding device appears on the bus. Does the OF code creates >> devices before all resources are ready? > > It's not an OF problem. The problem is that all the platform_devices > typically get registered all at once at machine_init time (on arm), > and if the gpio expander isn't a platform_device, (like an i2c gpio > expander which would end up being a child of a platform_device), then > it won't be ready. The real problem is that we have no mechanism for > holding off or deferring a driver probe if it depends on an > asynchronous resource. To avoid the registration order issue, isn't the proper fix to use a "setup" method in the gpio expander driver? The gpio-pca953x.c driver has this and I use it to hook the gpio_keys driver to those gpios. The registration order ends up being: i2c-gpio as a platform_device in the machine init code pca9539 as part of the i2c_board_info for the i2c-gpio device gpio-keys as a platform_device via the .setup callback of pca9539 Regards, Hartley-- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 20, 2011 at 2:45 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote: >> On Sat, 18 Jun 2011 09:16:45 -0600 >> Grant Likely <grant.likely@secretlab.ca> wrote: >> >> > On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote: >> > > On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote: >> > > > On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov >> > > > <dmitry.torokhov@gmail.com> wrote: >> > > > > On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: >> > > > >> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: >> > > > >> > Use a threaded interrupt handler in order to permit the handler to >> > > > >> > use a GPIO driver that causes things like I2C transactions being >> > > > >> > done inside the handler context. >> > > > >> > Also, gpio_keys_init needs to be declared as a late_initcall, to >> > > > >> > make sure all needed GPIO drivers have been loaded if the drivers >> > > > >> > are built into the kernel. >> > > > >> >> > > > >> ...which is a horrid hack, but until device dependencies can be >> > > > >> described, it isn't one that can be solved easily. >> > > > >> >> > > > > >> > > > > I really do not want to apply this... Currently the order of >> > > > > initialization does not matter since nothing actually happens until >> > > > > corresponding device appears on the bus. Does the OF code creates >> > > > > devices before all resources are ready? >> > > > >> > > > It's not an OF problem. The problem is that all the platform_devices >> > > > typically get registered all at once at machine_init time (on arm), >> > > > and if the gpio expander isn't a platform_device, (like an i2c gpio >> > > > expander which would end up being a child of a platform_device), then >> > > > it won't be ready. >> > > >> > > Ah, I see. But that can be handled in board code that should ensure that >> > > it registers devices in correct order. >> > >> > Unfortunately, handling it in board code doesn't really work either. >> > It just shuffles the complexity to the board code to implement some >> > kind of deferred mechanism for registering devices, and it has to take >> > into account that it may be a long time before the device actually >> > appears, such as when the driver is configured as a module. >> >> Besides... we don't want anymore board-code, do we? I mean, if a board can use >> a generic board configuration and specify all it needs in the device-tree, why >> should something as trivial as connecting a gpio_keys device to a I2C GPIO >> expander force us to do special board setup all of a sudden? >> IMHO specifying I2C-gpios to be used for gpio_keys should "just work", even if >> declared in a device-tree. > > This is a laudable goal, but then device-tree needs to be able to > express device dependencies better. Until then board-specific code is > needed to register devices in proper order. Do: $ git grep _initcall drivers/gpio and $ git grep module_init drivers/gpio I curse and hold my nose every time I have to apply one of those initcall patches, but I have to anyway since there isn't even a good way in board-specific code to control the device registration order. Everything gets registered at machine_init time, and the /order/ that things get registered has barely any effect. It all ends up hanging on the initcall order. The only way for board code to have a meaningful impact on initialization order is to wait for a driver to get probed on a prerequisite device, probably by using a notifier, and then register the device at that point. As far as I can tell, no board code does that. As ugly as fiddling with initcall levels is, it has been sufficient up to this point for existing (non-DT) board ports. g. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten <hartleys@visionengravers.com> wrote: > On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote: >> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote: >>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: >>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: >>>>> Use a threaded interrupt handler in order to permit the handler to use >>>>> a GPIO driver that causes things like I2C transactions being done inside >>>>> the handler context. >>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make sure >>>>> all needed GPIO drivers have been loaded if the drivers are built into the >>>>> kernel. >>>> >>>> ...which is a horrid hack, but until device dependencies can be >>>> described, it isn't one that can be solved easily. >>>> >>> >>> I really do not want to apply this... Currently the order of >>> initialization does not matter since nothing actually happens until >>> corresponding device appears on the bus. Does the OF code creates >>> devices before all resources are ready? >> >> It's not an OF problem. The problem is that all the platform_devices >> typically get registered all at once at machine_init time (on arm), >> and if the gpio expander isn't a platform_device, (like an i2c gpio >> expander which would end up being a child of a platform_device), then >> it won't be ready. The real problem is that we have no mechanism for >> holding off or deferring a driver probe if it depends on an >> asynchronous resource. > > To avoid the registration order issue, isn't the proper fix to use a "setup" > method in the gpio expander driver? The gpio-pca953x.c driver has this and > I use it to hook the gpio_keys driver to those gpios. The registration order > ends up being: > > i2c-gpio as a platform_device in the machine init code > pca9539 as part of the i2c_board_info for the i2c-gpio device > gpio-keys as a platform_device via the .setup callback of pca9539 Blech! That approach fell out of the ugly tree and hit every branch on the way down! Points for creativity though. :-) Essentially that ends up being a driver-specific notifier implementation. Yes, that works, but to me it just highlights a deficiency in the Linux device model. g. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 20, 2011 at 3:33 AM, David Jander <david.jander@protonic.nl> wrote: > On Mon, 20 Jun 2011 01:45:12 -0700 > Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > >> On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote: >> > On Sat, 18 Jun 2011 09:16:45 -0600 >> > Grant Likely <grant.likely@secretlab.ca> wrote: >> > > Unfortunately, handling it in board code doesn't really work either. >> > > It just shuffles the complexity to the board code to implement some >> > > kind of deferred mechanism for registering devices, and it has to take >> > > into account that it may be a long time before the device actually >> > > appears, such as when the driver is configured as a module. >> > >> > Besides... we don't want anymore board-code, do we? I mean, if a board can >> > use a generic board configuration and specify all it needs in the >> > device-tree, why should something as trivial as connecting a gpio_keys >> > device to a I2C GPIO expander force us to do special board setup all of a >> > sudden? IMHO specifying I2C-gpios to be used for gpio_keys should "just >> > work", even if declared in a device-tree. No, of course we don't. It's a big problem. The "just work" test oversimplifies what is going on here. Yes, you need gpio-keys working, and yes changing the initcall solves the problem for you, and it may very well be expedient to apply the change for the short term, but no it doesn't solve the underlying problem. While it makes it "just work" for you, there is the potential that it will make it "just not work" for somebody else. >> >> This is a laudable goal, but then device-tree needs to be able to >> express device dependencies better. Until then board-specific code is >> needed to register devices in proper order. > > Hmmm, I am not an expert in OF/DT stuff, but I think that while it would > theoretically be possible to add extra properties to the tree, that are > handled by the of_platform code, I am not sure if that is an option, since > that would be pretty much linux-specific, and could never work on another OS. > Grant? We /could/, but I don't think that it is a good idea. Dependencies between devices are already expressed by the device tree in the domain-specific properties. A gpio property expresses which gpio controller it depends on. Similarly an interrupt-parent property expresses which interrupt controller it depends on. Similarly with phy-device for PHYs, and other bindings for i2s links, clock connections, etc. What is not defined, is any kind of global "depends-on" node that states dependencies on other nodes. I don't think that having a depends-on property that duplicates already present information is a good idea; particularly when having inaccurate data in the property is very likely to go unnoticed because it may not break anything. My opinion is that making decisions about dependencies, and telling the core kernel about those dependencies, really should be in the domain of the driver. The driver has all the information about what a device needs to operate correctly, and it is the only place that will be able to describe constraints on other devices to the runtime PM infrastructure. Plus, most dependencies aren't necessarily on devices, but rather on the interfaces that a device driver advertises. For instance, a single device may present multiple interfaces (say, gpio controller with irq function), but depending on the configuration, the driver may not register one or the other. It's not the actual device that a driver like gpio-keys depends on, but rather whether or not the driver registers the gpio interface when it initialized the device. Again, this is a core infrastructure deficiency. The problem is just as much present when not using the DT, but it does present differently. >> How about we do not register device until all resources are ready? This >> is pretty simple concept - do not create an object until it is usable. Then >> nobody needs to bother with -EAGAIN or -ENOTYET or any other similar >> garbage. > > I agree, but DT doesn't permit that (yet). I don't. The systems we're working with are sufficiently complex now that I don't think that solution works in the long term. Take a look at the work the ALSA folks needed to do to get the audio complex wired up. Three or more devices get registered, a DAI driver, a codec driver, and a sound device driver, all of which can get registered in any order. The sound driver waits for the other devices to show up, and then does the work to attach them all together. I think this is a very credible approach, and I intend to dig into generalizing it. -EAGAIN might not be the right mechanism, but I do think it is entirely appropriate for drivers to be able to defer initialization when waiting for asynchronous events. > What do you think, Grant? Would it be possible/acceptable to add some special > property to devices that could make them be added in a second round by > of_platform code (until there are _real_ dependencies)? As discussed above, I don't think this is a good idea. > Or could the of_platform code be smart and just notice that gpio_keys needs > "gpios" (or other resources for that matter) that are depending on another > node in the tree, and make sure it gets probed before adding this one? I'm not going to say no; but I'd like to see a prototype what it would look like before I say yes. If it can be done relatively cleanly, then I'll be okay with it. I suspect that it will end up being crazy complex to use global code for tracking dependencies in this manor. g. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 20 Jun 2011 12:20:30 -0600 Grant Likely <grant.likely@secretlab.ca> wrote: > On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten > <hartleys@visionengravers.com> wrote: > > On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote: > >> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote: > >>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: > >>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: > >>>>> Use a threaded interrupt handler in order to permit the handler to use > >>>>> a GPIO driver that causes things like I2C transactions being done > >>>>> inside the handler context. > >>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make > >>>>> sure all needed GPIO drivers have been loaded if the drivers are built > >>>>> into the kernel. > >>>> > >>>> ...which is a horrid hack, but until device dependencies can be > >>>> described, it isn't one that can be solved easily. > >>>> > >>> > >>> I really do not want to apply this... Currently the order of > >>> initialization does not matter since nothing actually happens until > >>> corresponding device appears on the bus. Does the OF code creates > >>> devices before all resources are ready? > >> > >> It's not an OF problem. The problem is that all the platform_devices > >> typically get registered all at once at machine_init time (on arm), > >> and if the gpio expander isn't a platform_device, (like an i2c gpio > >> expander which would end up being a child of a platform_device), then > >> it won't be ready. The real problem is that we have no mechanism for > >> holding off or deferring a driver probe if it depends on an > >> asynchronous resource. > > > > To avoid the registration order issue, isn't the proper fix to use a > > "setup" method in the gpio expander driver? The gpio-pca953x.c driver has > > this and I use it to hook the gpio_keys driver to those gpios. The > > registration order ends up being: > > > > i2c-gpio as a platform_device in the machine init code > > pca9539 as part of the i2c_board_info for the i2c-gpio device > > gpio-keys as a platform_device via the .setup callback of pca9539 > > Blech! That approach fell out of the ugly tree and hit every branch > on the way down! Points for creativity though. :-) Essentially that > ends up being a driver-specific notifier implementation. > > Yes, that works, but to me it just highlights a deficiency in the > Linux device model. Especially, how would I specify this setup handler in a device-tree? The DT compiler still has no support for something like embedded DT-script ;-) Still, an interesting (albeit smelly) idea I hadn't thought of before. Best regards,
On Tue, Jun 21, 2011 at 12:55 AM, David Jander <david.jander@protonic.nl> wrote: > On Mon, 20 Jun 2011 12:20:30 -0600 > Grant Likely <grant.likely@secretlab.ca> wrote: > >> On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten >> <hartleys@visionengravers.com> wrote: >> > On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote: >> >> On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote: >> >>> On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote: >> >>>> On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote: >> >>>>> Use a threaded interrupt handler in order to permit the handler to use >> >>>>> a GPIO driver that causes things like I2C transactions being done >> >>>>> inside the handler context. >> >>>>> Also, gpio_keys_init needs to be declared as a late_initcall, to make >> >>>>> sure all needed GPIO drivers have been loaded if the drivers are built >> >>>>> into the kernel. >> >>>> >> >>>> ...which is a horrid hack, but until device dependencies can be >> >>>> described, it isn't one that can be solved easily. >> >>>> >> >>> >> >>> I really do not want to apply this... Currently the order of >> >>> initialization does not matter since nothing actually happens until >> >>> corresponding device appears on the bus. Does the OF code creates >> >>> devices before all resources are ready? >> >> >> >> It's not an OF problem. The problem is that all the platform_devices >> >> typically get registered all at once at machine_init time (on arm), >> >> and if the gpio expander isn't a platform_device, (like an i2c gpio >> >> expander which would end up being a child of a platform_device), then >> >> it won't be ready. The real problem is that we have no mechanism for >> >> holding off or deferring a driver probe if it depends on an >> >> asynchronous resource. >> > >> > To avoid the registration order issue, isn't the proper fix to use a >> > "setup" method in the gpio expander driver? The gpio-pca953x.c driver has >> > this and I use it to hook the gpio_keys driver to those gpios. The >> > registration order ends up being: >> > >> > i2c-gpio as a platform_device in the machine init code >> > pca9539 as part of the i2c_board_info for the i2c-gpio device >> > gpio-keys as a platform_device via the .setup callback of pca9539 >> >> Blech! That approach fell out of the ugly tree and hit every branch >> on the way down! Points for creativity though. :-) Essentially that >> ends up being a driver-specific notifier implementation. >> >> Yes, that works, but to me it just highlights a deficiency in the >> Linux device model. > > Especially, how would I specify this setup handler in a device-tree? You wouldn't. Notifiers (and the above poor-man's notifier) only works for specific cases in traditional board support code. There isn't a good pattern for using it with DT data. You'd have to make of_platform_populate() parse every node for any properties containing a phandle it recognizes (assuming you can know whether the resource is actually needed for the device to operate) and keep track of which devices should not be registered because other devices haven't been bound to drivers yet. Then you need to have a notifier that looks for those resources showing up, and registers the deferred device. > The DT > compiler still has no support for something like embedded DT-script ;-) We're not going there. g. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote: > This is a laudable goal, but then device-tree needs to be able to > express device dependencies better. Until then board-specific code is > needed to register devices in proper order. Like Grant says this really isn't terribly sustainable - it's not just the device registration you need to sort out, it's also the registration of the drivers so things actually get bound and handing of any delays in the process of getting things to appear. It's not trivial to get this right in the general case and it's not reasonable to expect individual boards to open code things, we really do need core code to figure things out in some fashion (ideally data rather than retry driven). > > Sounds interesting. So the probe function could return some sort of -ENOTYET > > or -EAGAIN and have it called again later? > How about we do not register device until all resources are ready? This > is pretty simple concept - do not create an object until it is usable. Then > nobody needs to bother with -EAGAIN or -ENOTYET or any other similar > garbage. As soon as you let the user build drivers modular this goes out of the window. All the faff with initcall ordering that we do at the minute is essentially trying to implement this mechanism. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 21 Jun 2011 06:34:48 -0700 Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > On Jun 21, 2011 3:46 PM, "Mark Brown" <broonie@opensource.wolfsonmicro.com> > wrote: > > > > On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote: > > > > > This is a laudable goal, but then device-tree needs to be able to > > > express device dependencies better. Until then board-specific code is > > > needed to register devices in proper order. > > > > Like Grant says this really isn't terribly sustainable - it's not just > > the device registration you need to sort out, it's also the registration > > of the drivers so things actually get bound and handing of any delays in > > the process of getting things to appear. > > If devices are registered only when they are fully usable then driver > registration does not matter. > > > It's not trivial to get this > > right in the general case and it's not reasonable to expect individual > > boards to open code things, > > Board code has the ultimate knowledge about connected devices though. Ok, let me try this essay: Board code, if there is any, or device-tree, or any other source of setup information knows best what needs to be initialized or bound when and in which order. If there is board code, one could solve this by embedding the logic in synchronous (initcall-based) or asynchronous (thread) board setup code. It is done on ARM that way, and IMHO it stinks, and AFAICS even Linus would probably agree (see http://thread.gmane.org/gmane.linux.ports.arm.kernel/113483/focus=113895 ). But we already have one example of non-code based setup information sources, which is the device-tree. A flexible system should not lock out the possibility that there are other such sources which favor generic code and dis-favor specific board setup code (reinventing wheels btw). So I guess everyone agrees that some core-infrastructure is missing to solve this problem "correctly". Since requiring board-specific code is not desirable due to the reasons stated above, what should we do in the meantime? IMHO, the late_initcall thing is both simple and should increase chances of success by a reasonable amount, while waiting for the correct solution to be implemented: An interface in the device/driver core infrastructure to specify device-dependencies in a generic and flexible way, so it can be sourced from a device-tree, board setup code (if you must) or any other source for that matter. At that moment, it is a matter of grepping for late_initcall and reverting these changes, if needed. Also, something like a keyboard driver (the part that generates input events, gpio_keys.c in this case), hardly could be a prerequisite for anything else, since it is clearly at the end of the driver food-chain. So what could possibly break by this change? Best regards,
On Tue, Jun 21, 2011 at 06:34:48AM -0700, Dmitry Torokhov wrote: > On Jun 21, 2011 3:46 PM, "Mark Brown" <broonie@opensource.wolfsonmicro.com> > > On Mon, Jun 20, 2011 at 01:45:12AM -0700, Dmitry Torokhov wrote: > > Like Grant says this really isn't terribly sustainable - it's not just > > the device registration you need to sort out, it's also the registration > > of the drivers so things actually get bound and handing of any delays in > > the process of getting things to appear. > If devices are registered only when they are fully usable then driver > registration does not matter. Right, but this is something that it's not reasonable to implement in board code - if nothing else implementing it in board code would mean we'd got lots of repitition of common patterns. > > It's not trivial to get this > > right in the general case and it's not reasonable to expect individual > > boards to open code things, > Board code has the ultimate knowledge about connected devices though. Absolutely, board code or data should provide the information about how things are wired up. It's the acting on it bit that's the issue. > > > How about we do not register device until all resources are ready? This > > > is pretty simple concept - do not create an object until it is usable. > Then > > > nobody needs to bother with -EAGAIN or -ENOTYET or any other similar > > > garbage. > > As soon as you let the user build drivers modular this goes out of the > > window. > Why is that? If device is registered only when it is ready to be bound to > then it does not matter when the driver is registered and whether it is > built into the kernel or as a module. Originally you were talking about registration ordering - solving the module load issues also requires dynamic delays and rollbacks when things get unregisterd, something that goes well beyond simple ordering of the registrations. > > All the faff with initcall ordering that we do at the minute is > > essentially trying to implement this mechanism. > No, what you are doing is creating devices before they are usable and > postponing the driver registration in hopes that devices will be ready by > that time. Right, which is controlling the ordering of registration so that things generally work out OK as described above. Nobody's arguing that we don't want to solve this in a better way, we're just saying that actually doing that requires improvements in both core infrastructure and the data we've got available to the infrastructure so there's no reasonable solutions that we can deploy which are better than the initcall ordering stuff we're doing at the minute. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote: > On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote: > > Right, but this is something that it's not reasonable to implement in > > board code - if nothing else implementing it in board code would mean > > we'd got lots of repitition of common patterns. > I agree here. I just disagree that we should be implementing this in > driver core by having special -EAGAIN handling. Having a common > library-like code (probably tied to device-tree) that handles device > dependencies would be great. Ah, that's more OK then. I'm not entirely sure about the -EAGAIN proposal but it does seem to have some advantages in terms of deployment. > Ah, OK, so we basically in agreement here with the exception that I do > not want the band-aid to hit mainline since it takes the heat off people > who need inter-device dependency to actually work. > Can the initcall stuff be kept out of mainline? I'd expect The init order stuff is in mainline already, you're far too late to the party here. > there exist board-specific trees where such patches could be kept? Or > maybe interested parties could create board-crap tree to store patches > like this one? Keeping things in board trees is exactly the sort of thing we want to avoid people doing. That just means people do all sorts of stuff that wouldn't be acceptable upstream, either out of ignorance or through knowing that only their systems have to work with what they're doing, and just don't bother working upstream at all half the time making life miserable for pretty much everyone. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 22 Jun 2011 00:02:42 +0100 Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote: > > On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote: > > > > Right, but this is something that it's not reasonable to implement in > > > board code - if nothing else implementing it in board code would mean > > > we'd got lots of repitition of common patterns. > > > I agree here. I just disagree that we should be implementing this in > > driver core by having special -EAGAIN handling. Having a common > > library-like code (probably tied to device-tree) that handles device > > dependencies would be great. > > Ah, that's more OK then. I'm not entirely sure about the -EAGAIN > proposal but it does seem to have some advantages in terms of > deployment. > > > Ah, OK, so we basically in agreement here with the exception that I do > > not want the band-aid to hit mainline since it takes the heat off people > > who need inter-device dependency to actually work. > > > Can the initcall stuff be kept out of mainline? I'd expect > > The init order stuff is in mainline already, you're far too late to the > party here. > > > there exist board-specific trees where such patches could be kept? Or > > maybe interested parties could create board-crap tree to store patches > > like this one? > > Keeping things in board trees is exactly the sort of thing we want to > avoid people doing. That just means people do all sorts of stuff that > wouldn't be acceptable upstream, either out of ignorance or through > knowing that only their systems have to work with what they're doing, > and just don't bother working upstream at all half the time making life > miserable for pretty much everyone. Looks like we all agree then? Dmitry, would you consider the late_initcall() part of the hack now (temporarily)? Best regards,
On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote: > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote: > > On Tue, Jun 21, 2011 at 06:27:45PM +0100, Mark Brown wrote: > > > > Right, but this is something that it's not reasonable to implement in > > > board code - if nothing else implementing it in board code would mean > > > we'd got lots of repitition of common patterns. > > > I agree here. I just disagree that we should be implementing this in > > driver core by having special -EAGAIN handling. Having a common > > library-like code (probably tied to device-tree) that handles device > > dependencies would be great. > > Ah, that's more OK then. I'm not entirely sure about the -EAGAIN > proposal but it does seem to have some advantages in terms of > deployment. > > > Ah, OK, so we basically in agreement here with the exception that I do > > not want the band-aid to hit mainline since it takes the heat off people > > who need inter-device dependency to actually work. > > > Can the initcall stuff be kept out of mainline? I'd expect > > The init order stuff is in mainline already, you're far too late to the > party here. For some drivers it might be already in mainline, it does not matter that we should continue adding more. > > > there exist board-specific trees where such patches could be kept? Or > > maybe interested parties could create board-crap tree to store patches > > like this one? > > Keeping things in board trees is exactly the sort of thing we want to > avoid people doing. That just means people do all sorts of stuff that > wouldn't be acceptable upstream, either out of ignorance or through > knowing that only their systems have to work with what they're doing, > and just don't bother working upstream at all half the time making life > miserable for pretty much everyone. So you are saying that we should accept such crap directly into mainline? Again, it looks like we agree that shuffling initcalls is not proper solution for this problem nor it is maintainable, so it is exactly the kind of patches that should be kept in the board trees and out of mainline.
On Wed, Jun 22, 2011 at 12:00:52AM -0700, Dmitry Torokhov wrote: > On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote: > > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote: > > > Can the initcall stuff be kept out of mainline? I'd expect > > The init order stuff is in mainline already, you're far too late to the > > party here. > For some drivers it might be already in mainline, it does not matter > that we should continue adding more. It's not just a few drivers, there's entire subsystems that are doing this. > > Keeping things in board trees is exactly the sort of thing we want to > > avoid people doing. That just means people do all sorts of stuff that > > wouldn't be acceptable upstream, either out of ignorance or through > > knowing that only their systems have to work with what they're doing, > > and just don't bother working upstream at all half the time making life > > miserable for pretty much everyone. > So you are saying that we should accept such crap directly into > mainline? Pretty much, yes. In code terms it's not really invasive and it doesn't have any real impact on other systems so it's the sort of thing we can carry without too much pain. Pragmatically it's not unreasonable. > Again, it looks like we agree that shuffling initcalls is not proper > solution for this problem nor it is maintainable, so it is exactly the > kind of patches that should be kept in the board trees and out of > mainline. On the other hand if we're telling people that they can't run their system usefully from mainline (in some cases we can't even boot) then we're sending a bad message about the usefulness of mainline and we're encouraging a space where non-mainline code is acceptable. The situation here is similar to what we used to have with interrupt controllers on slow buses - we spent a while working with open coded non-genirq implementations confined to particular drivers before genirq was able to support this sort of hardware because there wasn't a clear route to getting that done in a reasonable timeframe. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 22, 2011 at 5:38 AM, Mark Brown <broonie@opensource.wolfsonmicro.com> wrote: > On Wed, Jun 22, 2011 at 12:00:52AM -0700, Dmitry Torokhov wrote: >> On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote: >> > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote: > >> > > Can the initcall stuff be kept out of mainline? I'd expect > >> > The init order stuff is in mainline already, you're far too late to the >> > party here. > >> For some drivers it might be already in mainline, it does not matter >> that we should continue adding more. > > It's not just a few drivers, there's entire subsystems that are doing > this. > >> > Keeping things in board trees is exactly the sort of thing we want to >> > avoid people doing. That just means people do all sorts of stuff that >> > wouldn't be acceptable upstream, either out of ignorance or through >> > knowing that only their systems have to work with what they're doing, >> > and just don't bother working upstream at all half the time making life >> > miserable for pretty much everyone. > >> So you are saying that we should accept such crap directly into >> mainline? > > Pretty much, yes. In code terms it's not really invasive and it doesn't > have any real impact on other systems so it's the sort of thing we can > carry without too much pain. Pragmatically it's not unreasonable. +1 g. -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Jun 22, 2011 at 08:58:45AM -0600, Grant Likely wrote: > On Wed, Jun 22, 2011 at 5:38 AM, Mark Brown > <broonie@opensource.wolfsonmicro.com> wrote: > > On Wed, Jun 22, 2011 at 12:00:52AM -0700, Dmitry Torokhov wrote: > >> On Wed, Jun 22, 2011 at 12:02:42AM +0100, Mark Brown wrote: > >> > On Tue, Jun 21, 2011 at 01:48:05PM -0700, Dmitry Torokhov wrote: > > > >> > > Can the initcall stuff be kept out of mainline? I'd expect > > > >> > The init order stuff is in mainline already, you're far too late to the > >> > party here. > > > >> For some drivers it might be already in mainline, it does not matter > >> that we should continue adding more. > > > > It's not just a few drivers, there's entire subsystems that are doing > > this. > > > >> > Keeping things in board trees is exactly the sort of thing we want to > >> > avoid people doing. That just means people do all sorts of stuff that > >> > wouldn't be acceptable upstream, either out of ignorance or through > >> > knowing that only their systems have to work with what they're doing, > >> > and just don't bother working upstream at all half the time making life > >> > miserable for pretty much everyone. > > > >> So you are saying that we should accept such crap directly into > >> mainline? > > > > Pretty much, yes. In code terms it's not really invasive and it doesn't > > have any real impact on other systems so it's the sort of thing we can > > carry without too much pain. Pragmatically it's not unreasonable. > > +1 > OK, you wore me out. I'll apply the initcall change...
diff --git a/drivers/input/keyboard/gpio_keys.c b/drivers/input/keyboard/gpio_keys.c index 78aeeaa..d179861 100644 --- a/drivers/input/keyboard/gpio_keys.c +++ b/drivers/input/keyboard/gpio_keys.c @@ -3,7 +3,7 @@ * * Copyright 2005 Phil Blundell * - * Added OF support: + * Added OF support and enabled use with I2C GPIO expanders: * Copyright 2010 David Jander <david@protonic.nl> * * This program is free software; you can redistribute it and/or modify @@ -417,7 +417,7 @@ static int __devinit gpio_keys_setup_key(struct device *dev, if (!button->can_disable) irqflags |= IRQF_SHARED; - error = request_any_context_irq(irq, gpio_keys_isr, irqflags, desc, bdata); + error = request_threaded_irq(irq, NULL, gpio_keys_isr, irqflags, desc, bdata); if (error < 0) { dev_err(dev, "Unable to claim irq %d; error %d\n", irq, error); @@ -767,10 +767,10 @@ static void __exit gpio_keys_exit(void) platform_driver_unregister(&gpio_keys_device_driver); } -module_init(gpio_keys_init); +late_initcall(gpio_keys_init); module_exit(gpio_keys_exit); MODULE_LICENSE("GPL"); MODULE_AUTHOR("Phil Blundell <pb@handhelds.org>"); -MODULE_DESCRIPTION("Keyboard driver for CPU GPIOs"); +MODULE_DESCRIPTION("Keyboard driver for GPIOs"); MODULE_ALIAS("platform:gpio-keys");
Use a threaded interrupt handler in order to permit the handler to use a GPIO driver that causes things like I2C transactions being done inside the handler context. Also, gpio_keys_init needs to be declared as a late_initcall, to make sure all needed GPIO drivers have been loaded if the drivers are built into the kernel. Signed-off-by: David Jander <david@protonic.nl> --- drivers/input/keyboard/gpio_keys.c | 8 ++++---- 1 files changed, 4 insertions(+), 4 deletions(-)