Re: [PATCH v4 3/3] Input: gpio_keys.c: Enable use with non-local GPIO chips.

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

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.

-- 
Dmitry