Re: [PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
On Fri, Jun 28, 2013 at 11:25 -0700, Dmitry Torokhov wrote: On Fri, Jun 28, 2013 at 08:35:42AM -0600, Stephen Warren wrote: On 06/28/2013 01:52 AM, Gerhard Sittig wrote: [ late reply, just catching up with the backlog ] On Mon, Jun 24, 2013 at 17:26 -0600, Stephen Warren wrote: [ ... sparse matrices, not all columns/rows populated ... ] On some Tegra boards, there end up being rather tri-angular keymaps, where key positions (0, 0), (0, 1), (0, 2), (1, 1), (1, 2), (2, 2) end up being used. In this scenario, detailed investigation of the keymap would reveal: * Only scan columns 0..2 * For column 0, scan rows 0..2 * For column 1, scan rows 1..2 * For columm 2, scan row 2. That's another question I had. So far I was concerned with just polling or scanning the relevant columns, while all the rows for a given column were queried (as the driver always used to do). Now you raise the question of whether rows should get queried as well depending on whether a key may sit there. It wasn't the exact question I raised, but I added a comment to the spot where input subsystem events get generated: Is the driver expected to emit events for matrix positions where no key map entry exists? I would assume there is no need to, but I don't know for sure. Perhaps Dmitry can answer that? It really depends whether the driver can absolutely be sure that the key is not there or if it might be. Because keymaps are configurable from userspace the driver should not make this decision based on keymap itself. When you scan a matrix and come upon the pressed key condition, you supposed to emit EV_MSC/MSC_SCAN, followed by appropriate EV_KEY/KEY_*. Normally the keys that aren't there generate KEY_RESERVED events that are simply dropped by input core (cause it is easier to implement). MSC_SCAN events, however, reach the userspace intact. OK, so I understand that - filtering events based on the keymap entry being present would be inappropriate, the driver MUST emit key press events even if no key code is mapped to that position (especially since keymap entries may get modified at runtime) - it's perfectly appropriate for a driver to assume that _any_ intersection in the matrix _may_ carry a key, and scan for changes on that position, to derive input events (this is what the current implementation of the matrix driver does) - it's an _option_ whether the theoretically possible set of key positions further gets reduced to some really physically present and thus to get scanned only set, which currently isn't done and isn't mandatory, as in theory absent keys cannot lead to changes and thus should never result in input events So optionally reducing the set of to get scanned positions is some kind of micro-optimization (depending on the cost of GPIO access, or in the polling scenario depending on how many columns are empty). My patch did not introduce the filter with new keywords, it just made the existing driver use an existing DT parse routine which scans for already documented properties of the keymap helper. This simple approach assumes that the populated set always is in the range from 0 .. N-1, sparse layouts beyond that simple approach are possible and get processed correctly in the driver, but cannot get described in the device tree. Which still results in non-optimized but correct behaviour. Finer grained control beyond what my patch addresses would be possible but remain an option for later improvement (if desire is big enough to describe non-square layouts or sparse layouts where the gaps are in any arbitrary position). Hope this helps. Yes, your response answers a question that I raised elsewhere in the form of a TODO comment, that now has become obsolete. I will reword it to not raise the question, but to mention that emitting input events for key positions even in the absence of a key code is not just desirable but actually required. Thank you! 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 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
On 06/28/2013 01:52 AM, Gerhard Sittig wrote: [ late reply, just catching up with the backlog ] On Mon, Jun 24, 2013 at 17:26 -0600, Stephen Warren wrote: [ ... sparse matrices, not all columns/rows populated ... ] On some Tegra boards, there end up being rather tri-angular keymaps, where key positions (0, 0), (0, 1), (0, 2), (1, 1), (1, 2), (2, 2) end up being used. In this scenario, detailed investigation of the keymap would reveal: * Only scan columns 0..2 * For column 0, scan rows 0..2 * For column 1, scan rows 1..2 * For columm 2, scan row 2. That's another question I had. So far I was concerned with just polling or scanning the relevant columns, while all the rows for a given column were queried (as the driver always used to do). Now you raise the question of whether rows should get queried as well depending on whether a key may sit there. It wasn't the exact question I raised, but I added a comment to the spot where input subsystem events get generated: Is the driver expected to emit events for matrix positions where no key map entry exists? I would assume there is no need to, but I don't know for sure. Perhaps Dmitry can answer that? ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
On Fri, Jun 28, 2013 at 08:35:42AM -0600, Stephen Warren wrote: On 06/28/2013 01:52 AM, Gerhard Sittig wrote: [ late reply, just catching up with the backlog ] On Mon, Jun 24, 2013 at 17:26 -0600, Stephen Warren wrote: [ ... sparse matrices, not all columns/rows populated ... ] On some Tegra boards, there end up being rather tri-angular keymaps, where key positions (0, 0), (0, 1), (0, 2), (1, 1), (1, 2), (2, 2) end up being used. In this scenario, detailed investigation of the keymap would reveal: * Only scan columns 0..2 * For column 0, scan rows 0..2 * For column 1, scan rows 1..2 * For columm 2, scan row 2. That's another question I had. So far I was concerned with just polling or scanning the relevant columns, while all the rows for a given column were queried (as the driver always used to do). Now you raise the question of whether rows should get queried as well depending on whether a key may sit there. It wasn't the exact question I raised, but I added a comment to the spot where input subsystem events get generated: Is the driver expected to emit events for matrix positions where no key map entry exists? I would assume there is no need to, but I don't know for sure. Perhaps Dmitry can answer that? It really depends whether the driver can absolutely be sure that the key is not there or if it might be. Because keymaps are configurable from userspace the driver should not make this decision based on keymap itself. When you scan a matrix and come upon the pressed key condition, you supposed to emit EV_MSC/MSC_SCAN, followed by appropriate EV_KEY/KEY_*. Normally the keys that aren't there generate KEY_RESERVED events that are simply dropped by input core (cause it is easier to implement). MSC_SCAN events, however, reach the userspace intact. Hope this helps. -- Dmitry ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
On 06/22/2013 04:00 AM, Gerhard Sittig wrote: On Fri, Jun 21, 2013 at 15:41 -0600, Stephen Warren wrote: On 06/21/2013 12:09 PM, Gerhard Sittig wrote: diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt +The driver assumes that all interconnections of the matrix can potentially +contain a button, and will submit scan and key code events to the input +subsystem. By default the keypad matrix dimenstions are automatically +derived from the GPIO pin specifications. Optionally device tree +information can override the keypad matrix dimension data, e.g. when not +all of the potentially available physical connections are used to create +the logical keypad matrix. Ignoring the binary encoding in the next patch, why would someone ever define more row GPIOs that there are rows (or similarly for columns)? On its own, I don't think this patch is needed. Well, the keypad's property (remember the layering between keypad and keymap) has already been there, I just made the matrix keypad driver actually use the keymap's DT parse call. Regarding the usefulness of the patch in the absence of binary encodings which only later get introduced: I can't tell how much of a stretch it's going to get perceived as, but I suggested this: A .dtsi may specify the GPIO pins for a keypad attachment (say, the SoC's or module's potential to drive a matrix, the physical perspective), while boards' .dts files may specify the keymap and its specific layout (the logical matrix description). In this case, I'd say that the row-/column-GPIOs should only be specified by the .dts/.dtsi file for the HW that actually commits the GPIOs to that purpose. So in your example, the .dtsi file shouldn't specify which GPIOs to use, since the SoC (or base-board) doesn't define that; only the HW module which actually contains the keypad does, and hence only the .dts file for that piece of HW should specify the GPIOs. I suppose that approach doesn't handle a board with a generic keypad controller socket though; the user could plug in anything they want, and wouldn't want to have to re-write the board .dts file just to specify their keymap. Looking at this from the approach of the keymap data: Why does the DT need to say these columns are used or these rows are used? That data is already available from a simple search of the keymap for the various row-/column-IDs. Let the driver or keymap parser calculate the set of valid rows/columns when the keymap is installed. With this approach, you could even get far more optimal. On some Tegra boards, there end up being rather tri-angular keymaps, where key positions (0, 0), (0, 1), (0, 2), (1, 1), (1, 2), (2, 2) end up being used. In this scenario, detailed investigation of the keymap would reveal: * Only scan columns 0..2 * For column 0, scan rows 0..2 * For column 1, scan rows 1..2 * For columm 2, scan row 2. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
Re: [PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
On Fri, Jun 21, 2013 at 15:41 -0600, Stephen Warren wrote: On 06/21/2013 12:09 PM, Gerhard Sittig wrote: diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt +The driver assumes that all interconnections of the matrix can potentially +contain a button, and will submit scan and key code events to the input +subsystem. By default the keypad matrix dimenstions are automatically +derived from the GPIO pin specifications. Optionally device tree +information can override the keypad matrix dimension data, e.g. when not +all of the potentially available physical connections are used to create +the logical keypad matrix. Ignoring the binary encoding in the next patch, why would someone ever define more row GPIOs that there are rows (or similarly for columns)? On its own, I don't think this patch is needed. Well, the keypad's property (remember the layering between keypad and keymap) has already been there, I just made the matrix keypad driver actually use the keymap's DT parse call. Regarding the usefulness of the patch in the absence of binary encodings which only later get introduced: I can't tell how much of a stretch it's going to get perceived as, but I suggested this: A .dtsi may specify the GPIO pins for a keypad attachment (say, the SoC's or module's potential to drive a matrix, the physical perspective), while boards' .dts files may specify the keymap and its specific layout (the logical matrix description). Not populating logical lines of the matrix will hardly influence the scan time as status changes were detected, and it won't generate key events where interconnects are missing. But it might be desirable to not iterate over empty lines when polling is used to detect changes. Polling was introduced in an earlier part of the series, and allows for reliable detection of multi key press events. So sparse matrices are useful without binary encodings as well. Now, if you add binary encoding, I can see that you might have say 3 row GPIOs but only say 6 rows even though there are 8 combinations of row GPIO values. If that is the situation this patch is intended to cover, the changes here should be introduced as part of, and only applicable to, the binary encoding patch instead. I feel that although binary encoding was what I needed in the end, all the other steps taken to get there (except for the last one with the encoding) are of benefit for others as well. 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
[PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
- cleanly tell physical GPIO connections from logical matrix lines
- allow device tree specs to override matrix dimension (reduce
the size when not all of the available intersections are used)
in the current implementation both aspects of key code mapping
and keypad matrix scanning are handled in separate components,
the logical layout of the matrix need not be identical to what
the physical attachment allows for or would suggest
device tree setups allow to share the hardware controller's GPIO
attachment description with M x N intersections, yet individual
boards may use m x n matrix layouts with m = M and n = N
the separation of the physical attachment from the logical
operation further allows for the future introduction of optional
encodings or mappings, and lifts the current limitation (removes
the coded assumption) that there is exactly one pin for each line
Signed-off-by: Gerhard Sittig g...@denx.de
---
.../bindings/input/gpio-matrix-keypad.txt | 16 +++
drivers/input/keyboard/matrix_keypad.c | 50 ++--
include/linux/input/matrix_keypad.h|6 +++
3 files changed, 57 insertions(+), 15 deletions(-)
diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
index 196935b..f72d262 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
@@ -49,6 +49,14 @@ software, which makes signals float in the absence of pull
up resistors.
To fully drive output signals in either direction, enable push/pull mode
for column pins. This option is off by default for compatibility.
+The driver assumes that all interconnections of the matrix can potentially
+contain a button, and will submit scan and key code events to the input
+subsystem. By default the keypad matrix dimenstions are automatically
+derived from the GPIO pin specifications. Optionally device tree
+information can override the keypad matrix dimension data, e.g. when not
+all of the potentially available physical connections are used to create
+the logical keypad matrix.
+
Required Properties:
- compatible: Should be gpio-matrix-keypad
- row-gpios: List of gpios used as row lines. The gpio specifier
@@ -88,6 +96,14 @@ Optional Properties:
behaviour is to actively drive low signals and
be passive otherwise (emulates an open collector
output driver)
+- keypad,num-rows: number of rows in the keypad matrix, overrides the
+ value which gets derived from the number of row
+ GPIO pins, useful when not all lines are in use for
+ interconnects
+- keypad,num-columns: number of columns in the keypad matrix, overrides
+ the value which gets derived from the number of
+ column GPIO pins, useful when not all lines are in
+ use for interconnects
Example:
matrix-keypad {
diff --git a/drivers/input/keyboard/matrix_keypad.c
b/drivers/input/keyboard/matrix_keypad.c
index c2221d1..30b7faf 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -111,7 +111,7 @@ static void activate_all_cols(const struct
matrix_keypad_platform_data *pdata,
{
int col;
- for (col = 0; col pdata-num_col_gpios; col++)
+ for (col = 0; col pdata-num_matrix_cols; col++)
__activate_col_pin(pdata, col, on);
}
@@ -142,7 +142,7 @@ static uint32_t sample_rows_for_col(struct matrix_keypad
*keypad, int col)
activate_col(pdata, col, true);
row_state = 0;
- for (row = 0; row pdata-num_row_gpios; row++)
+ for (row = 0; row pdata-num_matrix_rows; row++)
row_state |= row_asserted(pdata, row) ? (1 row) : 0;
activate_col(pdata, col, false);
@@ -220,19 +220,19 @@ static void matrix_keypad_scan(struct work_struct *work)
/* assert each column in turn and read back the row status */
memset(new_state, 0, sizeof(new_state));
- for (col = 0; col pdata-num_col_gpios; col++)
+ for (col = 0; col pdata-num_matrix_cols; col++)
new_state[col] = sample_rows_for_col(keypad, col);
/* detect changes and derive input events, update the snapshot */
has_events = 0;
- for (col = 0; col pdata-num_col_gpios; col++) {
+ for (col = 0; col pdata-num_matrix_cols; col++) {
uint32_t bits_changed;
bits_changed = keypad-last_key_state[col] ^ new_state[col];
if (bits_changed == 0)
continue;
- for (row = 0; row pdata-num_row_gpios; row++) {
+ for (row = 0; row pdata-num_matrix_rows; row++) {
if ((bits_changed (1
Re: [PATCH v1 08/12] input: keypad-matrix: tell GPIO pins from matrix lines
On 06/21/2013 12:09 PM, Gerhard Sittig wrote: diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt +The driver assumes that all interconnections of the matrix can potentially +contain a button, and will submit scan and key code events to the input +subsystem. By default the keypad matrix dimenstions are automatically +derived from the GPIO pin specifications. Optionally device tree +information can override the keypad matrix dimension data, e.g. when not +all of the potentially available physical connections are used to create +the logical keypad matrix. Ignoring the binary encoding in the next patch, why would someone ever define more row GPIOs that there are rows (or similarly for columns)? On its own, I don't think this patch is needed. Now, if you add binary encoding, I can see that you might have say 3 row GPIOs but only say 6 rows even though there are 8 combinations of row GPIO values. If that is the situation this patch is intended to cover, the changes here should be introduced as part of, and only applicable to, the binary encoding patch instead. ___ devicetree-discuss mailing list devicetree-discuss@lists.ozlabs.org https://lists.ozlabs.org/listinfo/devicetree-discuss
