[PATCH v1 07/12] input: keypad-matrix: introduce polling support
detecting changes in the key press status may not work reliably in
interrupt driven mode (see the documentation part of the change for
details)
add support to poll the matrix in software for reliable operation in the
presence of multi key press events, leave a comment on how sleep and
wakeup could be made to work appropriately in the polling case
Signed-off-by: Gerhard Sittig
---
.../bindings/input/gpio-matrix-keypad.txt | 17
drivers/input/keyboard/matrix_keypad.c | 92 ++--
include/linux/input/matrix_keypad.h|4 +-
3 files changed, 107 insertions(+), 6 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
index e6a01eb..196935b 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
@@ -17,6 +17,20 @@ selected (active) at the same time. Which in turn assumes
that all
column lines can get activated at the same time, and no harm is done to
the hardware when multiple keys are pressed simultaneously.
+The approach to enable all columns at the same time and to determine
+that a key press status change has occured from row pin level changes
+only works reliably for single key presses. Multi key presses where the
+keys share their position on a row line may get deferred or even could
+go unnoticed, pressing and holding one key will shadow events which
+another key on the same row would have generated. When reliable
+detection of key press events is required even in the presence of multi
+key presses, interrupt mode isn't sufficient any longer, and polling
+needs to be used. The polling approach to detecting changes in the key
+press status will periodically activate a single column line and check
+the signals of the row lines. Polling may also be applicable to setups
+where the hardware doesn't support the activation of several columns at
+the same time.
+
Simple keypad matrix layouts just connect GPIO lines to mechanical
switches, with no other active components involved. Although due to the
driver's operation of activating all columns at the same time for most
@@ -60,6 +74,9 @@ Optional Properties:
such that pin levels can settle after
propagating through the matrix and its
associated hardware components
+- col-switch-delay-ms: columns switch interval in milliseconds instead
+ of using interrupts to detect key press changes,
+ enables polling mode when specified
- row-gpios-activelow: row GPIO pins are active low
- col-gpios-activelow: column GPIO pins are active low
- gpio-activelow: row pins as well as column pins are active low
diff --git a/drivers/input/keyboard/matrix_keypad.c
b/drivers/input/keyboard/matrix_keypad.c
index 0b2599d..c2221d1 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -36,10 +36,12 @@ struct matrix_keypad {
uint32_t last_key_state[MATRIX_MAX_COLS];
struct delayed_work work_scan_matrix;
+ struct delayed_work work_switch_column;
spinlock_t lock;
bool scan_pending;
bool stopped;
bool gpio_all_disabled;
+ unsigned int col_to_poll;
};
/*
@@ -152,17 +154,26 @@ static uint32_t sample_rows_for_col(struct matrix_keypad
*keypad, int col)
* to have any subsequent change in the key press status trigger the ISR
*
* a single IRQ line can be used if all involved GPIOs share that IRQ
+ *
+ * in contrast to interrupt driven detection of key press changes, for
+ * polling detection a delayed work routine periodically will switch
+ * columns and upon changes will arrange for the same postprocessing as
+ * an ISR would have done
*/
static void enable_row_irqs(struct matrix_keypad *keypad)
{
const struct matrix_keypad_platform_data *pdata = keypad->pdata;
int i;
+ unsigned long jiffies;
- if (pdata->clustered_irq > 0)
+ if (pdata->clustered_irq > 0) {
enable_irq(pdata->clustered_irq);
- else {
+ } else if (!pdata->col_switch_delay_ms) {
for (i = 0; i < pdata->num_row_gpios; i++)
enable_irq(gpio_to_irq(pdata->row_gpios[i]));
+ } else {
+ jiffies = msecs_to_jiffies(pdata->col_switch_delay_ms);
+ schedule_delayed_work(&keypad->work_switch_column, jiffies);
}
}
@@ -178,11 +189,13 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
const struct matrix_keypad_platform_data *pdata = keypad->pdata;
int i;
- if (pdata->clustered_irq > 0)
+ if (pdata->clustered_irq > 0) {
disable_irq_nosync(pdata->clustered_irq);
- else {
+ } else if (!pdata->col_switch_delay_ms) {
for
Re: [PATCH v1 07/12] input: keypad-matrix: introduce polling support
On 06/21/2013 12:09 PM, Gerhard Sittig wrote: > detecting changes in the key press status may not work reliably in > interrupt driven mode (see the documentation part of the change for > details) > > add support to poll the matrix in software for reliable operation in the > presence of multi key press events, leave a comment on how sleep and > wakeup could be made to work appropriately in the polling case > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt > b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt > +The approach to enable all columns at the same time and to determine > +that a key press status change has occured from row pin level changes > +only works reliably for single key presses. Multi key presses where the > +keys share their position on a row line may get deferred or even could > +go unnoticed, pressing and holding one key will shadow events which > +another key on the same row would have generated. When reliable > +detection of key press events is required even in the presence of multi > +key presses, interrupt mode isn't sufficient any longer, and polling > +needs to be used. The polling approach to detecting changes in the key > +press status will periodically activate a single column line and check > +the signals of the row lines. Polling may also be applicable to setups > +where the hardware doesn't support the activation of several columns at > +the same time. This feels a bit like encoding a driver implementation detail into the DT binding. Instead, couldn't the driver simply: * When no keys are pressed, perhaps after a certain delay/timeout while scanning, enable "interrupt mode". * As soon as an interrupt is seen, switch back to "scanning mode". * Once no keys are pressed again, go back to "interrupt mode". That way, interrupt mode is only used while no key is pressed, and hence there's no possibility of shadowing. This can all be done without any need for DT binding changes. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 07/12] input: keypad-matrix: introduce polling support
On Fri, Jun 21, 2013 at 15:38 -0600, Stephen Warren wrote: > > On 06/21/2013 12:09 PM, Gerhard Sittig wrote: > > detecting changes in the key press status may not work reliably in > > interrupt driven mode (see the documentation part of the change for > > details) > > > > add support to poll the matrix in software for reliable operation in the > > presence of multi key press events, leave a comment on how sleep and > > wakeup could be made to work appropriately in the polling case > > > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt > > b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt > > > +The approach to enable all columns at the same time and to determine > > +that a key press status change has occured from row pin level changes > > +only works reliably for single key presses. Multi key presses where the > > +keys share their position on a row line may get deferred or even could > > +go unnoticed, pressing and holding one key will shadow events which > > +another key on the same row would have generated. When reliable > > +detection of key press events is required even in the presence of multi > > +key presses, interrupt mode isn't sufficient any longer, and polling > > +needs to be used. The polling approach to detecting changes in the key > > +press status will periodically activate a single column line and check > > +the signals of the row lines. Polling may also be applicable to setups > > +where the hardware doesn't support the activation of several columns at > > +the same time. > > This feels a bit like encoding a driver implementation detail into the > DT binding. > > Instead, couldn't the driver simply: > > * When no keys are pressed, perhaps after a certain delay/timeout while > scanning, enable "interrupt mode". > > * As soon as an interrupt is seen, switch back to "scanning mode". > > * Once no keys are pressed again, go back to "interrupt mode". > > That way, interrupt mode is only used while no key is pressed, and hence > there's no possibility of shadowing. > > This can all be done without any need for DT binding changes. That's exactly what the GPIO matrix keypad driver did implement and still does by default after it got extended. There is - the "active scan" step which determines the status of individual keys by querying each matrix line individually (in software) and - the "idle" phase where no software is involved but instead a change is detected by changes in the GPIO pins' level while all columns are enabled (wired-OR if you want) The patch set doesn't introduce that behaviour, but merely describes it in more detail. It doesn't even introduce the interrupt discussion into the binding document in a strict sense, but expands on it in the hope for improved usability of the binding after the motivation became more obvious. What this part of the series does is to introduce polling mode as an alternative to the interrupt driven detection of changes, to improve reliability of change detection in the presence of multi key presses. I suggest to have the "meta-discussions" on which documentation belongs where and on where to put the GPIO polarity and on whether backward compatibility needs to be kept or may be broken, in a single spot, to not have several parallel discussions in multiple subthreads. Is the cover letter or the first patch the most appropriate message to respond to with this though in mind? Or don't you mind if several replies for different parts of the patch set discuss similar "background" aspects of the same series? virtually yours Gerhard Sittig -- DENX Software Engineering GmbH, MD: Wolfgang Denk & Detlev Zundel HRB 165235 Munich, Office: Kirchenstr. 5, D-82194 Groebenzell, Germany Phone: +49-8142-66989-0 Fax: +49-8142-66989-80 Email: off...@denx.de ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 07/12] input: keypad-matrix: introduce polling support
On 06/22/2013 03:50 AM, Gerhard Sittig wrote: ... > The patch set doesn't introduce that behaviour, but merely > describes it in more detail. It doesn't even introduce the > interrupt discussion into the binding document in a strict sense, > but expands on it in the hope for improved usability of the > binding after the motivation became more obvious. > > > What this part of the series does is to introduce polling mode as > an alternative to the interrupt driven detection of changes, to > improve reliability of change detection in the presence of multi > key presses. To me, this sounds more like something for Documentation/input/ rather than DT binding. ... > I suggest to have the "meta-discussions" on which documentation > belongs where and on where to put the GPIO polarity and on > whether backward compatibility needs to be kept or may be broken, > in a single spot, to not have several parallel discussions in > multiple subthreads. > > Is the cover letter or the first patch the most appropriate > message to respond to with this though in mind? Or don't you > mind if several replies for different parts of the patch set > discuss similar "background" aspects of the same series? I don't really have a preference myself; feel free to pick whichever patch or response you want to continue discussing, and reply to that; I'll just reply to whatever sub-thread/... you choose:-) ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
