Re: [PATCH v4] Input: matrix-keypad - Add device tree support

On 11/09/2012 04:07 AM, AnilKumar, Chimata wrote:

On Thu, Nov 08, 2012 at 22:25:33, Stephen Warren wrote:

quoted

On 11/07/2012 02:32 AM, AnilKumar Ch wrote:

quoted

Add device tree support to matrix keypad driver and usage details
are added to device tree documentation. Driver was tested on AM335x
EVM.

quoted

diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt

quoted

+Optional Properties:

quoted

+- clustered-irq:	have clustered irq number, that is needed if the irq
+			is a combined irq source for the whole matrix keypad.
+			This is useful if rows and columns of the keypad are
+			connected to a GPIO expander.
+- clustered-irq-flags:	clustered irq flags to specify the interrupt line
+			behavior among IRQF_TRIGGER_*

I still don't understand why there's a need for a clustered-irq-flags
property; if those flags are the flags for an interrupt, why aren't the
flags part of the clustered-irq interrupt specifier, just like any other
interrupt in DT?

Exactly, I agree with you on this.

Honestly, I added clustered_xxx properties to DT considering the 
platform_data currently implemented in driver. And I do not have
hardware to  validate clustered_xxx execution flow.

I looked at the commit which adds support for clustered_irq,

Commit Description:
 
"This one adds support of a combined irq source for the whole matrix
keypad. This can be useful if all rows and columns of the keypad are
e.g. connected to a GPIO expander, which only has one interrupt line
for all events on every single GPIO."


So I believe this was meant for matrix keypad interfaced over I2C
expander, But I am not sure how it is being used.

The hardware is connected in any of the following methods, the driver
should supports the same.

i) gpio interrupt can be used as keypad interrupt, connection is
like this (MPU<===>IOEXP)
            |_______| (gpio line)

Here I expect driver should have gpio_to_irq implementation, but
its missing from driver. So platform code must be doing it, but
surprisingly I couldn't able to find any platform which uses this.

ii) i2c interrupt can be used as keypad interrupt

Here driver is not using threaded irq.

As a reference I looked tca6416-keypad.c driver and it has right implementation,

if (pdata->irq_is_gpio)
	chip->irqnum = gpio_to_irq(client->irq);
else
	chip->irqnum = client->irq;


As per my understanding matrix-keypad driver has to change
accordingly to tca6416-keypad.c driver to handle clustered-irq.

That sounds correct.

So I left with below options,

1. Lets only support normal GPIO based matrix key-pad, without
clustered_irq support in DT.

2. Cleanup the driver to remove clustered_irq all together,
considering the fact that we do not have any platform in kernel
to use it. And once we get known platform in the future, we can
again add it.

I vote for option-1, what do you think?

I think that's fine.

The only issue is that it means the DT binding will not be complete the
first time around, and will probably need to be extended in the future.
I know that some people will want to see the binding be complete right
from the very start, to make sure the binding has been fully thought
through. However, it seems clear that the binding can be extended in a
backwards-compatible fashion to support clustered IRQs later, so I don't
think there's any issue here myself.