From: David Jander <hidden> Date: 2011-06-14 09:07:59
This patch series adds support for OF device-tree platform data and non-local
GPIO chips, such as I2C I/O expanders.
This is version 4 of the patches.
Changed in this version:
- Corrected naming of DT properties.
- Fixed a few errors in the way dynamic pdata is handled.
- Added support for device-tree gpios specifier.
David Jander (3):
Input: gpio_keys.c: Simplify platform_device -> device casting
Input: gpio_keys.c: Added support for device-tree platform data
Input: gpio_keys.c: Enable use with non-local GPIO chips.
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++
drivers/input/keyboard/gpio_keys.c | 193 ++++++++++++++++----
2 files changed, 207 insertions(+), 35 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
--
1.7.4.1
From: David Jander <hidden> Date: 2011-06-14 09:07:59
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 <redacted>
---
drivers/input/keyboard/gpio_keys.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)
From: Grant Likely <hidden> Date: 2011-06-16 19:37:45
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 <redacted>
g.
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:
quoted
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.
--
Dmitry
From: Grant Likely <hidden> Date: 2011-06-18 13:18:48
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
--
Dmitry
From: Grant Likely <hidden> Date: 2011-06-18 15:16:49
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:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
quoted
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
From: David Jander <hidden> Date: 2011-06-20 07:48:12
On Sat, 18 Jun 2011 09:16:45 -0600
Grant Likely [off-list ref] wrote:
On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
quoted
On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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!
quoted
quoted
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?
quoted
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,
--
David Jander
Protonic Holland.
On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
On Sat, 18 Jun 2011 09:16:45 -0600
Grant Likely [off-list ref] wrote:
quoted
On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
quoted
On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
quoted
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.
quoted
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!
quoted
quoted
quoted
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?
quoted
quoted
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
From: David Jander <hidden> Date: 2011-06-20 09:32:58
On Mon, 20 Jun 2011 01:45:12 -0700
Dmitry Torokhov [off-list ref] wrote:
On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
quoted
On Sat, 18 Jun 2011 09:16:45 -0600
Grant Likely [off-list ref] wrote:
quoted
On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
quoted
On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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?
quoted
quoted
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.
quoted
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).
quoted
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!
quoted
quoted
quoted
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?
quoted
quoted
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.
quoted
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,
--
David Jander
Protonic Holland.
From: Grant Likely <hidden> Date: 2011-06-20 18:49:30
On Mon, Jun 20, 2011 at 3:33 AM, David Jander [off-list ref] wrote:
On Mon, 20 Jun 2011 01:45:12 -0700
Dmitry Torokhov [off-list ref] wrote:
quoted
On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
quoted
On Sat, 18 Jun 2011 09:16:45 -0600
Grant Likely [off-list ref] wrote:
quoted
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.
quoted
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.
quoted
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.
From: Grant Likely <hidden> Date: 2011-06-20 18:13:58
On Mon, Jun 20, 2011 at 2:45 AM, Dmitry Torokhov
[off-list ref] wrote:
On Mon, Jun 20, 2011 at 09:48:15AM +0200, David Jander wrote:
quoted
On Sat, 18 Jun 2011 09:16:45 -0600
Grant Likely [off-list ref] wrote:
quoted
On Sat, Jun 18, 2011 at 07:51:54AM -0700, Dmitry Torokhov wrote:
quoted
On Sat, Jun 18, 2011 at 07:18:28AM -0600, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov
[off-list ref] wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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
From: Mark Brown <hidden> Date: 2011-06-21 11:46:35
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).
quoted
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.
From: H Hartley Sweeten <hidden> Date: 2011-06-20 17:04:02
On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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
From: Grant Likely <hidden> Date: 2011-06-20 18:20:50
On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten
[off-list ref] wrote:
On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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
From: David Jander <hidden> Date: 2011-06-21 06:55:55
On Mon, 20 Jun 2011 12:20:30 -0600
Grant Likely [off-list ref] wrote:
On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten
[off-list ref] wrote:
quoted
On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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,
--
David Jander
Protonic Holland.
--
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
From: Grant Likely <hidden> Date: 2011-06-21 07:04:26
On Tue, Jun 21, 2011 at 12:55 AM, David Jander [off-list ref] wrote:
On Mon, 20 Jun 2011 12:20:30 -0600
Grant Likely [off-list ref] wrote:
quoted
On Mon, Jun 20, 2011 at 11:03 AM, H Hartley Sweeten
[off-list ref] wrote:
quoted
On Saturday, June 18, 2011 6:18 AM, Grant Likely wrote:
quoted
On Sat, Jun 18, 2011 at 4:17 AM, Dmitry Torokhov wrote:
quoted
On Thu, Jun 16, 2011 at 01:27:32PM -0600, Grant Likely wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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
Hi David,
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
Thanks.
--
Dmitry
From: David Jander <hidden> Date: 2012-03-16 08:34:58
On Fri, 16 Mar 2012 00:20:04 -0700
Dmitry Torokhov [off-list ref] wrote:
Hi David,
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
Sorry, the reason described is apparently not very clear. The real reason seems
to be that I would like this driver to work with I2C GPIO expanders, and its
the GPIO expanders "interrupt controller" which has itself a threaded handler
(due to I2C transfers done in it to ack an IRQ). So this is actually a nested
and threaded interrupt controller (because the IRQ line of the GPIO expander
is connected to a different GPIO acting itself also as interrupt line).
In irq/manage.c, function __setup_irq():
...
/*
* Check whether the interrupt nests into another interrupt
* thread.
*/
nested = irq_settings_is_nested_thread(desc);
if (nested) {
if (!new->thread_fn) {
ret = -EINVAL;
goto out_mput;
}
...
This is were requesting a non-threaded IRQ from this GPIO controller will fail.
I know this is not a trivial setup, but IMHO it is very useful (for
connecting keyboards), and a nice demonstration of the powerful features this
GPIO driver has :-)
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
Not in my case. I just checked again. If I change request_threaded_irq() to
request_irq(), I get this:
...
[ 6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
[ 6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
...
This error -22 (-EINVAL) is returned from __setup_irq() (see above).
BTW: The connections of CPU-GPIO -> IRQ of PCA9539 -> GPIO -> gpio_key is
entirely done in the device tree, which is also sort of cool ;-)
Best regards,
--
David Jander
Protonic Holland.
On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
On Fri, 16 Mar 2012 00:20:04 -0700
Dmitry Torokhov [off-list ref] wrote:
quoted
Hi David,
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
Sorry, the reason described is apparently not very clear. The real reason seems
to be that I would like this driver to work with I2C GPIO expanders, and its
the GPIO expanders "interrupt controller" which has itself a threaded handler
(due to I2C transfers done in it to ack an IRQ). So this is actually a nested
and threaded interrupt controller (because the IRQ line of the GPIO expander
is connected to a different GPIO acting itself also as interrupt line).
In irq/manage.c, function __setup_irq():
...
/*
* Check whether the interrupt nests into another interrupt
* thread.
*/
nested = irq_settings_is_nested_thread(desc);
if (nested) {
if (!new->thread_fn) {
ret = -EINVAL;
goto out_mput;
}
...
This is were requesting a non-threaded IRQ from this GPIO controller will fail.
I know this is not a trivial setup, but IMHO it is very useful (for
connecting keyboards), and a nice demonstration of the powerful features this
GPIO driver has :-)
Thanks for the explanation of your setup.
quoted
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
Not in my case. I just checked again. If I change request_threaded_irq() to
request_irq(), I get this:
...
[ 6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
[ 6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
...
This error -22 (-EINVAL) is returned from __setup_irq() (see above).
But the original code used request_any_context_irq() which should have
taken care of your nested IRQ setup:
int request_any_context_irq(unsigned int irq, irq_handler_t handler,
unsigned long flags, const char *name, void *dev_id)
{
struct irq_desc *desc = irq_to_desc(irq);
int ret;
if (!desc)
return -EINVAL;
if (irq_settings_is_nested_thread(desc)) {
ret = request_threaded_irq(irq, NULL, handler,
flags, name, dev_id);
return !ret ? IRQC_IS_NESTED : ret;
}
ret = request_irq(irq, handler, flags, name, dev_id);
return !ret ? IRQC_IS_HARDIRQ : ret;
}
Thanks.
--
Dmitry
From: David Jander <hidden> Date: 2012-03-16 08:47:56
On Fri, 16 Mar 2012 01:32:01 -0700
Dmitry Torokhov [off-list ref] wrote:
On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
quoted
On Fri, 16 Mar 2012 00:20:04 -0700
Dmitry Torokhov [off-list ref] wrote:
quoted
Hi David,
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
Sorry, the reason described is apparently not very clear. The real reason seems
to be that I would like this driver to work with I2C GPIO expanders, and its
the GPIO expanders "interrupt controller" which has itself a threaded handler
(due to I2C transfers done in it to ack an IRQ). So this is actually a nested
and threaded interrupt controller (because the IRQ line of the GPIO expander
is connected to a different GPIO acting itself also as interrupt line).
In irq/manage.c, function __setup_irq():
...
/*
* Check whether the interrupt nests into another interrupt
* thread.
*/
nested = irq_settings_is_nested_thread(desc);
if (nested) {
if (!new->thread_fn) {
ret = -EINVAL;
goto out_mput;
}
...
This is were requesting a non-threaded IRQ from this GPIO controller will fail.
I know this is not a trivial setup, but IMHO it is very useful (for
connecting keyboards), and a nice demonstration of the powerful features this
GPIO driver has :-)
Thanks for the explanation of your setup.
quoted
quoted
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
Not in my case. I just checked again. If I change request_threaded_irq() to
request_irq(), I get this:
...
[ 6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
[ 6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
...
This error -22 (-EINVAL) is returned from __setup_irq() (see above).
But the original code used request_any_context_irq() which should have
taken care of your nested IRQ setup:
Hmm. You are right. Apparently this change was introduced in 2.6.38, and I
must have missed it. Before 2.6.38, this place called request_irq(), which was
broken for my case.
I just checked, and indeed, using request_any_context_irq() seems to work fine
for me.
Best regards,
--
David Jander
Protonic Holland.
From: Ben Dooks <hidden> Date: 2012-03-16 11:20:13
On Fri, Mar 16, 2012 at 09:48:03AM +0100, David Jander wrote:
On Fri, 16 Mar 2012 01:32:01 -0700
Dmitry Torokhov [off-list ref] wrote:
quoted
On Fri, Mar 16, 2012 at 09:17:01AM +0100, David Jander wrote:
quoted
On Fri, 16 Mar 2012 00:20:04 -0700
Dmitry Torokhov [off-list ref] wrote:
quoted
Hi David,
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
Sorry, the reason described is apparently not very clear. The real reason seems
to be that I would like this driver to work with I2C GPIO expanders, and its
the GPIO expanders "interrupt controller" which has itself a threaded handler
(due to I2C transfers done in it to ack an IRQ). So this is actually a nested
and threaded interrupt controller (because the IRQ line of the GPIO expander
is connected to a different GPIO acting itself also as interrupt line).
In irq/manage.c, function __setup_irq():
...
/*
* Check whether the interrupt nests into another interrupt
* thread.
*/
nested = irq_settings_is_nested_thread(desc);
if (nested) {
if (!new->thread_fn) {
ret = -EINVAL;
goto out_mput;
}
...
This is were requesting a non-threaded IRQ from this GPIO controller will fail.
I know this is not a trivial setup, but IMHO it is very useful (for
connecting keyboards), and a nice demonstration of the powerful features this
GPIO driver has :-)
Thanks for the explanation of your setup.
quoted
quoted
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
Not in my case. I just checked again. If I change request_threaded_irq() to
request_irq(), I get this:
...
[ 6.409810] gpio-keys gpio_keys.0: Unable to claim irq 0; error -22
[ 6.416106] gpio-keys: probe of gpio_keys.0 failed with error -22
...
This error -22 (-EINVAL) is returned from __setup_irq() (see above).
But the original code used request_any_context_irq() which should have
taken care of your nested IRQ setup:
Hmm. You are right. Apparently this change was introduced in 2.6.38, and I
must have missed it. Before 2.6.38, this place called request_irq(), which was
broken for my case.
I just checked, and indeed, using request_any_context_irq() seems to work fine
for me.
I'd say that would be better, seems an un-necesary use of threaded
interrupt work.
--
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
From: Ben Dooks <hidden> Date: 2012-03-16 11:20:15
On Fri, Mar 16, 2012 at 12:20:04AM -0700, Dmitry Torokhov wrote:
Hi David,
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
Why on earth do we need this? this looks like something that is not
necessary and in my view a waste of cpu cycles.
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
See above comment, I'd go with just remove it and unthread.
--
Ben Dooks, ben@fluff.org, http://www.fluff.org/ben/
Large Hadron Colada: A large Pina Colada that makes the universe disappear.
From: David Jander <hidden> Date: 2012-03-16 11:07:56
On Fri, 16 Mar 2012 10:18:15 +0000
Ben Dooks [off-list ref] wrote:
On Fri, Mar 16, 2012 at 12:20:04AM -0700, Dmitry Torokhov wrote:
quoted
Hi David,
On Tue, Jun 14, 2011 at 11:08:11AM +0200, David Jander wrote:
quoted
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.
Don't want to resurrect the whole initcall discussion, but could you
tell me again why the interrup handler needs to be threaded? We do not
access hardware from it, hardware is accessed from workqueue context.
Here is the ISR in its entirety:
static irqreturn_t gpio_keys_isr(int irq, void *dev_id)
{
struct gpio_button_data *bdata = dev_id;
const struct gpio_keys_button *button = bdata->button;
BUG_ON(irq != gpio_to_irq(button->gpio));
Why on earth do we need this? this looks like something that is not
necessary and in my view a waste of cpu cycles.
No idea... catch some weird (hardware-/setup-)bug? Not _that_ many CPU cycles
anyway, plus I am not the author of that line.... maybe ask Uwe Kleine-König
(CC'd)?
quoted
if (bdata->timer_debounce)
mod_timer(&bdata->timer,
jiffies + msecs_to_jiffies(bdata->timer_debounce));
else
schedule_work(&bdata->work);
return IRQ_HANDLED;
}
It looks to me that non-threaded handler would work as well? Or
gpio_to_irq() can sleep with certain chips?
See above comment, I'd go with just remove it and unthread.
Not unthread, but use request_any_context_irq(), please!
Best regards,
--
David Jander
Protonic Holland.
--
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
From: David Jander <hidden> Date: 2011-06-14 09:07:59
This patch factors out the use of struct platform_device *pdev in most
places.
Signed-off-by: David Jander <redacted>
---
drivers/input/keyboard/gpio_keys.c | 46 ++++++++++++++++-------------------
1 files changed, 21 insertions(+), 25 deletions(-)
On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
This patch factors out the use of struct platform_device *pdev in most
places.
Why? We are dealing with a platform device so why would we switch to
generic device?
I also think that we should not be mixing dev_get/set_drvdata() and
<bus>_get/set_drvdata() calls but rather use appropriate bus-specific
version to access data on given layer.
Thanks.
--
Dmitry
From: David Jander <hidden> Date: 2011-06-20 06:52:10
Hi Dmitry,
On Sat, 18 Jun 2011 03:19:25 -0700
Dmitry Torokhov [off-list ref] wrote:
On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
quoted
This patch factors out the use of struct platform_device *pdev in most
places.
Why? We are dealing with a platform device so why would we switch to
generic device?
Actually, when I wrote this patch there still was a difference between
the platform bus and the of_platform bus, and this change was necessary. There
also were ifdefs around platform_driver_register and
of_platform_driver_register. Now it seems this has been merged, and I am
not sure it is necessary anymore, but I still think it simplifies the code
quite a bit. Also, why should the driver be bus-dependent, when it doesn't
even need a real "bus" (it talks to an abstract device through another driver,
potentially connected to any bus), besides due to how linux views devices and
drivers.
I also think that we should not be mixing dev_get/set_drvdata() and
<bus>_get/set_drvdata() calls
AFAICS, we are not mixing.... it is dev_*_drvdata() only.
but rather use appropriate bus-specific version to access data on given
layer.
Doesn't that make the driver much too complex? And why would that be
necessary? The driver isn't bus-specific anymore... except for the binding and
probing part.
Best regards,
--
David Jander
Protonic Holland.
Hi David,
On Mon, Jun 20, 2011 at 08:52:13AM +0200, David Jander wrote:
Hi Dmitry,
On Sat, 18 Jun 2011 03:19:25 -0700
Dmitry Torokhov [off-list ref] wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:09AM +0200, David Jander wrote:
quoted
This patch factors out the use of struct platform_device *pdev in most
places.
Why? We are dealing with a platform device so why would we switch to
generic device?
Actually, when I wrote this patch there still was a difference between
the platform bus and the of_platform bus, and this change was necessary. There
also were ifdefs around platform_driver_register and
of_platform_driver_register. Now it seems this has been merged, and I am
not sure it is necessary anymore, but I still think it simplifies the code
quite a bit. Also, why should the driver be bus-dependent, when it doesn't
even need a real "bus" (it talks to an abstract device through another driver,
potentially connected to any bus), besides due to how linux views devices and
drivers.
While there isn't real hardware bus the driver is fitted into platform
device framework and so we should use platform device API unless there
is compelling reason for using another API.
quoted
I also think that we should not be mixing dev_get/set_drvdata() and
<bus>_get/set_drvdata() calls
AFAICS, we are not mixing.... it is dev_*_drvdata() only.
"Mixing" was probably not the best word. "Using API from a different
layer" would probably be better.
quoted
but rather use appropriate bus-specific version to access data on given
layer.
Doesn't that make the driver much too complex? And why would that be
necessary? The driver isn't bus-specific anymore... except for the binding and
probing part.
Why would it make the driver more complex? Aside from a couple of
PM methods coming from the driver core and therefore operating on
"struct device *" the rest is using platform device directly.
Thanks.
--
Dmitry
From: David Jander <hidden> Date: 2011-06-14 09:08:00
This patch enables fetching configuration data which is normally provided via
platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data struct
is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe function,
to make this possible.
Signed-off-by: David Jander <redacted>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++++
drivers/input/keyboard/gpio_keys.c | 147 ++++++++++++++++++--
2 files changed, 186 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@+Device-Tree bindings for input/gpio_keys.c keyboard driver++Required properties:+ - compatible = "gpio-keys";++Optional properties:+ - autorepeat: Boolean, Enable auto repeat feature of Linux input+ subsystem.++Each button (key) is represented as a sub-node of "gpio-keys":+Subnode properties:++ - reg: GPIO number the key is bound to if linux GPIO numbering is used.+ - gpios: OF devcie-tree gpio specification, can be used alternatively+ if 'reg' is not specified, to use device-tree GPIOs.+ - label: Descriptive name of the key.+ - linux,code: Keycode to emit.++Optional subnode-properties:+ - active-low: Boolean flag to specify active-low GPIO input. Not used+ if device-tree gpios property is used.+ - linux,input-type: Specify event type this button/key generates.+ Default if unspecified is <1> == EV_KEY.+ - debounce-interval: Debouncing interval time in milliseconds.+ Default if unspecified is 5.+ - wakeup: Boolean, button can wake-up the system.++Example nodes:++ gpio_keys {+ compatible = "gpio-keys";+ #address-cells = <1>;+ #size-cells = <0>;+ autorepeat;+ button@20 {+ label = "GPIO Key ESC";+ linux,code = <1>;+ reg = <0x20>;+ key-active-low;+ linux,input-type = <1>;+ };+ button@21 {+ label = "GPIO Key UP";+ linux,code = <103>;+ gpios = <&gpio1 0 1>;+ linux,input-type = <1>;+ };+ ...+
@@ -442,15 +447,124 @@ static void gpio_keys_close(struct input_dev *input)ddata->disable(input->dev.parent);}+/*+*Handlersforalternativesourcesofplatform_data+*/+#ifdef CONFIG_OF+/*+*TranslateOpenFirmwarenodepropertiesintoplatform_data+*/+staticstructgpio_keys_platform_data*+gpio_keys_get_devtree_pdata(structdevice*dev,+structgpio_keys_platform_data*altp)+{+structgpio_keys_platform_data*pdata=altp;+structdevice_node*node,*pp;+inti;+structgpio_keys_button*buttons;+constu32*reg;+intlen;++node=dev->of_node;+if(node==NULL)+returnNULL;++memset(pdata,0,sizeof*pdata);++pdata->rep=!!of_get_property(node,"autorepeat",&len);++/* First count the subnodes */+pdata->nbuttons=0;+pp=NULL;+while((pp=of_get_next_child(node,pp)))+pdata->nbuttons++;++if(pdata->nbuttons==0)+returnNULL;++buttons=kzalloc(pdata->nbuttons*(sizeof*buttons),GFP_KERNEL);+if(!buttons)+returnNULL;++pp=NULL;+i=0;+while((pp=of_get_next_child(node,pp))){+constchar*lbl;+enumof_gpio_flagsflags;++reg=of_get_property(pp,"reg",&len);+if(!reg&&!of_find_property(pp,"gpios",NULL)){+pdata->nbuttons--;+dev_warn(dev,"Found button without reg and without gpios\n");+continue;+}+if(reg){+buttons[i].gpio=be32_to_cpup(reg);+buttons[i].active_low=!!of_get_property(pp,"key-active-low",NULL);+}else{+buttons[i].gpio=of_get_gpio_flags(pp,0,&flags);+buttons[i].active_low=!!(flags&OF_GPIO_ACTIVE_LOW);+}++reg=of_get_property(pp,"linux,code",&len);+if(!reg){+dev_err(dev,"Button without keycode: 0x%x\n",buttons[i].gpio);+gotoout_fail;+}+buttons[i].code=be32_to_cpup(reg);++lbl=of_get_property(pp,"label",&len);+buttons[i].desc=(char*)lbl;++reg=of_get_property(pp,"linux,input-type",&len);+if(reg)+buttons[i].type=be32_to_cpup(reg);+else+buttons[i].type=EV_KEY;++buttons[i].wakeup=!!of_get_property(pp,"wakeup",NULL);++reg=of_get_property(pp,"debounce-interval",&len);+if(reg)+buttons[i].debounce_interval=be32_to_cpup(reg);+else+buttons[i].debounce_interval=5;+i++;+}++pdata->buttons=buttons;++returnpdata;++out_fail:+kfree(buttons);+returnNULL;+}+#else+staticstructgpio_keys_platform_data*+gpio_keys_get_devtree_pdata(structdevice*dev,+structgpio_keys_platform_data*altp)+{+returnNULL;+}+#endif+staticint__devinitgpio_keys_probe(structplatform_device*pdev){structgpio_keys_drvdata*ddata;structdevice*dev=&pdev->dev;structgpio_keys_platform_data*pdata=dev->platform_data;+structgpio_keys_platform_dataalt_pdata;structinput_dev*input;inti,error;intwakeup=0;+if(!pdata){+pdata=gpio_keys_get_devtree_pdata(dev,&alt_pdata);+if(!pdata)+return-ENODEV;+}+ddata=kzalloc(sizeof(structgpio_keys_drvdata)+pdata->nbuttons*sizeof(structgpio_button_data),GFP_KERNEL);
@@ -548,7 +662,6 @@ static int __devinit gpio_keys_probe(struct platform_device *pdev)staticint__devexitgpio_keys_remove(structplatform_device*pdev){structdevice*dev=&pdev->dev;-structgpio_keys_platform_data*pdata=dev->platform_data;structgpio_keys_drvdata*ddata=dev_get_drvdata(dev);structinput_dev*input=ddata->input;inti;
From: Grant Likely <hidden> Date: 2011-06-16 19:26:04
On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
quoted hunk
This patch enables fetching configuration data which is normally provided via
platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data struct
is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe function,
to make this possible.
Signed-off-by: David Jander <redacted>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++++
drivers/input/keyboard/gpio_keys.c | 147 ++++++++++++++++++--
2 files changed, 186 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@+Device-Tree bindings for input/gpio_keys.c keyboard driver++Required properties:+ - compatible = "gpio-keys";++Optional properties:+ - autorepeat: Boolean, Enable auto repeat feature of Linux input+ subsystem.++Each button (key) is represented as a sub-node of "gpio-keys":+Subnode properties:++ - reg: GPIO number the key is bound to if linux GPIO numbering is used.
Wait. That won't work. There is no concept of "linux gpio numbering"
in the device tree data. When using device tree, the gpio numbers
usually get dynamically assigned.
+ - gpios: OF devcie-tree gpio specification, can be used alternatively
+ if 'reg' is not specified, to use device-tree GPIOs.
+ - label: Descriptive name of the key.
+ - linux,code: Keycode to emit.
+
+Optional subnode-properties:
+ - active-low: Boolean flag to specify active-low GPIO input. Not used
+ if device-tree gpios property is used.
+ - linux,input-type: Specify event type this button/key generates.
+ Default if unspecified is <1> == EV_KEY.
+ - debounce-interval: Debouncing interval time in milliseconds.
+ Default if unspecified is 5.
+ - wakeup: Boolean, button can wake-up the system.
"wakeup" is potentially too generic a property name (potential to
conflict with a generic wakeup binding if one ever exists). Just to
be defencive, I'd suggest prefixing these custom gpio keys properties
with something like "gpio-key,".
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
pdata is always the same as altp. You don't need this on the stack,
and the return value should be an error code instead of a pointer
because the pointer is already passed in!
@@ -557,30 +670,42 @@ static int __devexit gpio_keys_remove(struct platform_device *pdev) device_init_wakeup(dev, 0);- for (i = 0; i < pdata->nbuttons; i++) {- int irq = gpio_to_irq(pdata->buttons[i].gpio);+ for (i = 0; i < ddata->n_buttons; i++) {+ int irq = gpio_to_irq(ddata->data[i].button->gpio); free_irq(irq, &ddata->data[i]); if (ddata->data[i].timer_debounce) del_timer_sync(&ddata->data[i].timer); cancel_work_sync(&ddata->data[i].work);- gpio_free(pdata->buttons[i].gpio);+ gpio_free(ddata->data[i].button->gpio); }+ /*+ * If we had no platform_data, we allocated buttons dynamically, and+ * must free them here. ddata->data[0].button is the pointer to the+ * beginning of the allocated array.+ */+ if (!dev->platform_data)+ kfree(ddata->data[0].button);+ input_unregister_device(input); return 0; }+static struct of_device_id gpio_keys_of_match[] = {+ { .compatible = "gpio-keys", },+ {},+}; #ifdef CONFIG_PM static int gpio_keys_suspend(struct device *dev) {- struct gpio_keys_platform_data *pdata = dev->platform_data;+ struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev); int i; if (device_may_wakeup(dev)) {- for (i = 0; i < pdata->nbuttons; i++) {- struct gpio_keys_button *button = &pdata->buttons[i];+ for (i = 0; i < ddata->n_buttons; i++) {+ struct gpio_keys_button *button = ddata->data[i].button; if (button->wakeup) { int irq = gpio_to_irq(button->gpio); enable_irq_wake(irq);
@@ -594,12 +719,11 @@ static int gpio_keys_suspend(struct device *dev) static int gpio_keys_resume(struct device *dev) { struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);- struct gpio_keys_platform_data *pdata = dev->platform_data; int i;- for (i = 0; i < pdata->nbuttons; i++) {+ for (i = 0; i < ddata->n_buttons; i++) {- struct gpio_keys_button *button = &pdata->buttons[i];+ struct gpio_keys_button *button = ddata->data[i].button; if (button->wakeup && device_may_wakeup(dev)) { int irq = gpio_to_irq(button->gpio); disable_irq_wake(irq);
From: David Jander <hidden> Date: 2011-06-17 08:58:24
Hi Grant,
On Thu, 16 Jun 2011 13:25:59 -0600
Grant Likely [off-list ref] wrote:
On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
quoted
This patch enables fetching configuration data which is normally provided
via platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data struct
is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe
function, to make this possible.
Signed-off-by: David Jander <redacted>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++++
drivers/input/keyboard/gpio_keys.c | 147
++++++++++++++++++-- 2 files changed, 186 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@+Device-Tree bindings for input/gpio_keys.c keyboard driver++Required properties:+ - compatible = "gpio-keys";++Optional properties:+ - autorepeat: Boolean, Enable auto repeat feature of Linux input+ subsystem.++Each button (key) is represented as a sub-node of "gpio-keys":+Subnode properties:++ - reg: GPIO number the key is bound to if linux GPIO numbering is
used.
Wait. That won't work. There is no concept of "linux gpio numbering"
in the device tree data. When using device tree, the gpio numbers
usually get dynamically assigned.
Yes I know, but suppose you want to use this driver with a GPIO-driver that
does not have devaice-tree support yet? I need a way of binding the button to
a GPIO pin that does not have a device-tree definition. How should I do this
otherwise?
I am using this driver with a pca963x, as you might have suspected already,
and I just added OF device-tree support to it, so that will work, but
others...?
Before "fixing" pca963x, I used this property and it worked perfectly well, so
please explain what is not supposed to work. Please keep in mind that this
is meant as more of a backwards-compatibility feature. If you think that being
backwards compatible with non-OF GPIO drivers is a bad idea, I'll remove this.
quoted
+ - gpios: OF devcie-tree gpio specification, can be used
alternatively
+ if 'reg' is not specified, to use device-tree GPIOs.
+ - label: Descriptive name of the key.
+ - linux,code: Keycode to emit.
+
+Optional subnode-properties:
+ - active-low: Boolean flag to specify active-low GPIO input. Not
used
+ if device-tree gpios property is used.
+ - linux,input-type: Specify event type this button/key generates.
+ Default if unspecified is <1> == EV_KEY.
+ - debounce-interval: Debouncing interval time in milliseconds.
+ Default if unspecified is 5.
+ - wakeup: Boolean, button can wake-up the system.
"wakeup" is potentially too generic a property name (potential to
conflict with a generic wakeup binding if one ever exists). Just to
be defencive, I'd suggest prefixing these custom gpio keys properties
with something like "gpio-key,".
Ok, "gpio-key,wakeup" it will be then? But isn't this function something
potentially other IO-pins/keys/buttons/interrupts/etc... could have to be able
to wake up the system?
Ooops :-) You see... this patch is rather old already (in my tree). I actually
wrote it in 2010, but I am submitting it now. I guess it should be "2010, 2011"
then?
quoted
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU General Public License version 2 as
* published by the Free Software Foundation.
As mentioned above, this won't work. Linux gpio numbers cannot be
encoded into the DT.
Why not? It worked fine for me before I "fixed" pca963x.
If you have a non-OF GPIO controller, that one will need a numeric range of
GPIO numbers. If that range is fixed, I can perfectly well give this number to
the driver here.... again, this is only used if the GPIO driver does not
have a device-tree node.
@@ -557,30 +670,42 @@ static int __devexit gpio_keys_remove(struct
platform_device *pdev)
device_init_wakeup(dev, 0);
- for (i = 0; i < pdata->nbuttons; i++) {
- int irq = gpio_to_irq(pdata->buttons[i].gpio);
+ for (i = 0; i < ddata->n_buttons; i++) {
+ int irq = gpio_to_irq(ddata->data[i].button->gpio);
free_irq(irq, &ddata->data[i]);
if (ddata->data[i].timer_debounce)
del_timer_sync(&ddata->data[i].timer);
cancel_work_sync(&ddata->data[i].work);
- gpio_free(pdata->buttons[i].gpio);
+ gpio_free(ddata->data[i].button->gpio);
}
+ /*
+ * If we had no platform_data, we allocated buttons dynamically,
and
+ * must free them here. ddata->data[0].button is the pointer to
the
+ * beginning of the allocated array.
+ */
+ if (!dev->platform_data)
+ kfree(ddata->data[0].button);
+
input_unregister_device(input);
return 0;
}
+static struct of_device_id gpio_keys_of_match[] = {
+ { .compatible = "gpio-keys", },
+ {},
+};
#ifdef CONFIG_PM
static int gpio_keys_suspend(struct device *dev)
{
- struct gpio_keys_platform_data *pdata = dev->platform_data;
+ struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);
int i;
if (device_may_wakeup(dev)) {
- for (i = 0; i < pdata->nbuttons; i++) {
- struct gpio_keys_button *button =
&pdata->buttons[i];
+ for (i = 0; i < ddata->n_buttons; i++) {
+ struct gpio_keys_button *button =
ddata->data[i].button; if (button->wakeup) {
int irq = gpio_to_irq(button->gpio);
enable_irq_wake(irq);
@@ -594,12 +719,11 @@ static int gpio_keys_suspend(struct device *dev) static int gpio_keys_resume(struct device *dev) { struct gpio_keys_drvdata *ddata = dev_get_drvdata(dev);- struct gpio_keys_platform_data *pdata = dev->platform_data; int i;- for (i = 0; i < pdata->nbuttons; i++) {+ for (i = 0; i < ddata->n_buttons; i++) {- struct gpio_keys_button *button = &pdata->buttons[i];+ struct gpio_keys_button *button = ddata->data[i].button; if (button->wakeup && device_may_wakeup(dev)) { int irq = gpio_to_irq(button->gpio); disable_irq_wake(irq);
From: Grant Likely <hidden> Date: 2011-06-17 12:54:38
On Fri, Jun 17, 2011 at 2:58 AM, David Jander [off-list ref] wrote:
Hi Grant,
On Thu, 16 Jun 2011 13:25:59 -0600
Grant Likely [off-list ref] wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
quoted
This patch enables fetching configuration data which is normally provided
via platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data struct
is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe
function, to make this possible.
Signed-off-by: David Jander <redacted>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++++
drivers/input/keyboard/gpio_keys.c | 147
++++++++++++++++++-- 2 files changed, 186 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@+Device-Tree bindings for input/gpio_keys.c keyboard driver++Required properties:+ - compatible = "gpio-keys";++Optional properties:+ - autorepeat: Boolean, Enable auto repeat feature of Linux input+ subsystem.++Each button (key) is represented as a sub-node of "gpio-keys":+Subnode properties:++ - reg: GPIO number the key is bound to if linux GPIO numbering is
used.
Wait. That won't work. There is no concept of "linux gpio numbering"
in the device tree data. When using device tree, the gpio numbers
usually get dynamically assigned.
Yes I know, but suppose you want to use this driver with a GPIO-driver that
does not have devaice-tree support yet? I need a way of binding the button to
a GPIO pin that does not have a device-tree definition. How should I do this
otherwise?
I am using this driver with a pca963x, as you might have suspected already,
and I just added OF device-tree support to it, so that will work, but
others...?
The solution is to add OF support to the GPIO driver being used.
Before "fixing" pca963x, I used this property and it worked perfectly well, so
please explain what is not supposed to work. Please keep in mind that this
is meant as more of a backwards-compatibility feature. If you think that being
backwards compatible with non-OF GPIO drivers is a bad idea, I'll remove this.
It is. Something that we've been very careful about is to avoid
encoding Linux-specific implementation details into the device tree
bindings. The implementation details, such as how gpio controllers
are enumerated, are subject to change just in the normal progress of
development. By focusing the DT bindings on HW description, the DT
data is insulated to a degree from kernel churn.
quoted
quoted
+ - wakeup: Boolean, button can wake-up the system.
"wakeup" is potentially too generic a property name (potential to
conflict with a generic wakeup binding if one ever exists). Just to
be defencive, I'd suggest prefixing these custom gpio keys properties
with something like "gpio-key,".
Ok, "gpio-key,wakeup" it will be then? But isn't this function something
potentially other IO-pins/keys/buttons/interrupts/etc... could have to be able
to wake up the system?
Can you foresee how all bindings would potentially use a 'wakeup'
property? It's really hard to define a common binding without first
having several use cases ready to use it. It's better to start being
cautious, and then create a common binding at some point in the
future.
how about:
buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;
Ok, if you prefer this notation.... just an "if...else" in another dress ;-)
Yup, but it's shorter, and I like painting bike sheds.
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 Fri, Jun 17, 2011 at 06:54:17AM -0600, Grant Likely wrote:
On Fri, Jun 17, 2011 at 2:58 AM, David Jander [off-list ref] wrote:
quoted
Hi Grant,
On Thu, 16 Jun 2011 13:25:59 -0600
Grant Likely [off-list ref] wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
quoted
This patch enables fetching configuration data which is normally provided
via platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data struct
is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe
function, to make this possible.
Signed-off-by: David Jander <redacted>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++++
drivers/input/keyboard/gpio_keys.c | 147
++++++++++++++++++-- 2 files changed, 186 insertions(+), 10 deletions(-)
create mode 100644 Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@+Device-Tree bindings for input/gpio_keys.c keyboard driver++Required properties:+ - compatible = "gpio-keys";++Optional properties:+ - autorepeat: Boolean, Enable auto repeat feature of Linux input+ subsystem.++Each button (key) is represented as a sub-node of "gpio-keys":+Subnode properties:++ - reg: GPIO number the key is bound to if linux GPIO numbering is
used.
Wait. That won't work. There is no concept of "linux gpio numbering"
in the device tree data. When using device tree, the gpio numbers
usually get dynamically assigned.
Yes I know, but suppose you want to use this driver with a GPIO-driver that
does not have devaice-tree support yet? I need a way of binding the button to
a GPIO pin that does not have a device-tree definition. How should I do this
otherwise?
I am using this driver with a pca963x, as you might have suspected already,
and I just added OF device-tree support to it, so that will work, but
others...?
The solution is to add OF support to the GPIO driver being used.
quoted
Before "fixing" pca963x, I used this property and it worked perfectly well, so
please explain what is not supposed to work. Please keep in mind that this
is meant as more of a backwards-compatibility feature. If you think that being
backwards compatible with non-OF GPIO drivers is a bad idea, I'll remove this.
It is. Something that we've been very careful about is to avoid
encoding Linux-specific implementation details into the device tree
bindings. The implementation details, such as how gpio controllers
are enumerated, are subject to change just in the normal progress of
development. By focusing the DT bindings on HW description, the DT
data is insulated to a degree from kernel churn.
quoted
quoted
quoted
+ - wakeup: Boolean, button can wake-up the system.
"wakeup" is potentially too generic a property name (potential to
conflict with a generic wakeup binding if one ever exists). Just to
be defencive, I'd suggest prefixing these custom gpio keys properties
with something like "gpio-key,".
Ok, "gpio-key,wakeup" it will be then? But isn't this function something
potentially other IO-pins/keys/buttons/interrupts/etc... could have to be able
to wake up the system?
Can you foresee how all bindings would potentially use a 'wakeup'
property? It's really hard to define a common binding without first
having several use cases ready to use it. It's better to start being
cautious, and then create a common binding at some point in the
future.
how about:
buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;
Ok, if you prefer this notation.... just an "if...else" in another dress ;-)
Yup, but it's shorter, and I like painting bike sheds.
So is there an updated version of this patch coming?
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
From: David Jander <hidden> Date: 2011-06-23 08:55:33
On Thu, 23 Jun 2011 01:24:10 -0700
Dmitry Torokhov [off-list ref] wrote:
On Fri, Jun 17, 2011 at 06:54:17AM -0600, Grant Likely wrote:
quoted
On Fri, Jun 17, 2011 at 2:58 AM, David Jander [off-list ref]
wrote:
quoted
Hi Grant,
On Thu, 16 Jun 2011 13:25:59 -0600
Grant Likely [off-list ref] wrote:
quoted
On Tue, Jun 14, 2011 at 11:08:10AM +0200, David Jander wrote:
quoted
This patch enables fetching configuration data which is normally
provided via platform_data from the device-tree instead.
If the device is configured from device-tree data, the platform_data
struct is not used, and button data needs to be allocated dynamically.
Big part of this patch deals with confining pdata usage to the probe
function, to make this possible.
Signed-off-by: David Jander <redacted>
---
.../devicetree/bindings/gpio/gpio_keys.txt | 49 +++++++
drivers/input/keyboard/gpio_keys.c | 147
++++++++++++++++++-- 2 files changed, 186 insertions(+), 10
deletions(-) create mode 100644
Documentation/devicetree/bindings/gpio/gpio_keys.txt
@@ -0,0 +1,49 @@+Device-Tree bindings for input/gpio_keys.c keyboard driver++Required properties:+ - compatible = "gpio-keys";++Optional properties:+ - autorepeat: Boolean, Enable auto repeat feature of Linux input+ subsystem.++Each button (key) is represented as a sub-node of "gpio-keys":+Subnode properties:++ - reg: GPIO number the key is bound to if linux GPIO numbering is
used.
Wait. That won't work. There is no concept of "linux gpio numbering"
in the device tree data. When using device tree, the gpio numbers
usually get dynamically assigned.
Yes I know, but suppose you want to use this driver with a GPIO-driver
that does not have devaice-tree support yet? I need a way of binding the
button to a GPIO pin that does not have a device-tree definition. How
should I do this otherwise?
I am using this driver with a pca963x, as you might have suspected
already, and I just added OF device-tree support to it, so that will
work, but others...?
The solution is to add OF support to the GPIO driver being used.
quoted
Before "fixing" pca963x, I used this property and it worked perfectly
well, so please explain what is not supposed to work. Please keep in
mind that this is meant as more of a backwards-compatibility feature. If
you think that being backwards compatible with non-OF GPIO drivers is a
bad idea, I'll remove this.
It is. Something that we've been very careful about is to avoid
encoding Linux-specific implementation details into the device tree
bindings. The implementation details, such as how gpio controllers
are enumerated, are subject to change just in the normal progress of
development. By focusing the DT bindings on HW description, the DT
data is insulated to a degree from kernel churn.
quoted
quoted
quoted
+ - wakeup: Boolean, button can wake-up the system.
"wakeup" is potentially too generic a property name (potential to
conflict with a generic wakeup binding if one ever exists). Just to
be defencive, I'd suggest prefixing these custom gpio keys properties
with something like "gpio-key,".
Ok, "gpio-key,wakeup" it will be then? But isn't this function something
potentially other IO-pins/keys/buttons/interrupts/etc... could have to
be able to wake up the system?
Can you foresee how all bindings would potentially use a 'wakeup'
property? It's really hard to define a common binding without first
having several use cases ready to use it. It's better to start being
cautious, and then create a common binding at some point in the
future.
how about:
buttons[i].type = reg ? be32_to_cpup(reg) : EV_KEY;
Ok, if you prefer this notation.... just an "if...else" in another
dress ;-)
Yup, but it's shorter, and I like painting bike sheds.
So is there an updated version of this patch coming?
Yes, I'm preparing v5 right now.
Best regards,
--
David Jander
Protonic Holland.
--
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