| Message ID | 1371838198-7327-9-git-send-email-gsi@denx.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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
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.
[ 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 ... ] > > 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. I agree that this reduces pain for .dts authors, which is a very good thing. They need to provide the key map anyway, and the set of interconnections in the matrix can thus get determined programmatically. Data overhead should be acceptable given the maximum matrix dimensions of 32x32. I'll look into this ... > 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? OTOH was my further reasoning that in theory the keymap shall reflect the hardware implementation, so users should not be able to press keys which don't have a corresponding key map entry. And switching or modifying key maps from software shall cope with the fact when "raw keys" get reported which don't carry a code. Enter the above row/column filter based on the keymap: Only controlling those pins where interconnections have a keymap entry (an operation that's cheap) reduces access to GPIOs (which may or may not be expensive in "abolute" terms, but certainly is more expensive than a keymap check), and eliminiates the issue of emitting events which lack codes. Sounds like the appropriate solution to the problem. virtually yours Gerhard Sittig
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?
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.
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
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 << row)) == 0) continue; @@ -329,7 +329,7 @@ static void matrix_keypad_switch(struct work_struct *work) * compile time constants) */ keypad->col_to_poll++; - if (keypad->col_to_poll >= pdata->num_col_gpios) + if (keypad->col_to_poll >= pdata->num_matrix_cols) keypad->col_to_poll = 0; col = keypad->col_to_poll; @@ -580,7 +580,8 @@ matrix_keypad_parse_dt(struct device *dev) struct matrix_keypad_platform_data *pdata; struct device_node *np = dev->of_node; unsigned int *gpios; - int i, nrow, ncol; + int i; + int err; if (!np) { dev_err(dev, "device lacks DT data for the keypad config\n"); @@ -594,12 +595,18 @@ matrix_keypad_parse_dt(struct device *dev) } /* get the pin count for rows and columns */ - pdata->num_row_gpios = nrow = of_gpio_named_count(np, "row-gpios"); - pdata->num_col_gpios = ncol = of_gpio_named_count(np, "col-gpios"); - if (nrow <= 0 || ncol <= 0) { - dev_err(dev, "number of keypad rows/columns not specified\n"); + i = of_gpio_named_count(np, "row-gpios"); + if (i <= 0) { + dev_err(dev, "row GPIO pin specs for keypad missing\n"); + return ERR_PTR(-EINVAL); + } + pdata->num_row_gpios = i; + i = of_gpio_named_count(np, "col-gpios"); + if (i <= 0) { + dev_err(dev, "column GPIO pin specs for keypad missing\n"); return ERR_PTR(-EINVAL); } + pdata->num_col_gpios = i; /* get input, power, and GPIO pin properties */ if (of_get_property(np, "linux,no-autorepeat", NULL)) @@ -644,6 +651,19 @@ matrix_keypad_parse_dt(struct device *dev) pdata->row_gpios = gpios; pdata->col_gpios = &gpios[pdata->num_row_gpios]; + /* + * auto determine the number of keypad matrix rows and columns + * from the number of GPIO pins, optionally allow device tree + * information to override these values + */ + pdata->num_matrix_rows = pdata->num_row_gpios; + pdata->num_matrix_cols = pdata->num_col_gpios; + err = matrix_keypad_parse_of_params(dev, + &pdata->num_matrix_rows, + &pdata->num_matrix_cols); + if (err) + return ERR_PTR(err); + return pdata; } #else @@ -684,13 +704,13 @@ static int matrix_keypad_probe(struct platform_device *pdev) keypad->input_dev = input_dev; keypad->pdata = pdata; - keypad->row_shift = get_count_order(pdata->num_col_gpios); + keypad->row_shift = get_count_order(pdata->num_matrix_cols); /* start in stopped state, open(2) will activate the scan */ keypad->stopped = true; INIT_DELAYED_WORK(&keypad->work_scan_matrix, matrix_keypad_scan); INIT_DELAYED_WORK(&keypad->work_switch_column, matrix_keypad_switch); - keypad->col_to_poll = pdata->num_col_gpios; + keypad->col_to_poll = pdata->num_matrix_cols; spin_lock_init(&keypad->lock); input_dev->name = pdev->name; @@ -700,8 +720,8 @@ static int matrix_keypad_probe(struct platform_device *pdev) input_dev->close = matrix_keypad_stop; err = matrix_keypad_build_keymap(pdata->keymap_data, NULL, - pdata->num_row_gpios, - pdata->num_col_gpios, + pdata->num_matrix_rows, + pdata->num_matrix_cols, NULL, input_dev); if (err) { dev_err(&pdev->dev, "failed to build keymap\n"); diff --git a/include/linux/input/matrix_keypad.h b/include/linux/input/matrix_keypad.h index 3c8dc39..1398d23 100644 --- a/include/linux/input/matrix_keypad.h +++ b/include/linux/input/matrix_keypad.h @@ -39,6 +39,8 @@ struct matrix_keymap_data { * @col_gpios: pointer to array of gpio numbers reporesenting colums * @num_row_gpios: actual number of row gpios used by device * @num_col_gpios: actual number of col gpios used by device + * @num_matrix_rows: number of logical row lines in the matrix + * @num_matrix_cols: number of logical column lines in the matrix * @col_scan_delay_us: delay in microseconds, the interval between * activating a column and reading back row information * @col_switch_delay_ms: delay in milliseconds, the interval with which @@ -70,6 +72,10 @@ struct matrix_keypad_platform_data { unsigned int num_row_gpios; unsigned int num_col_gpios; + /* the logical matrix row/column lines */ + unsigned int num_matrix_rows; + unsigned int num_matrix_cols; + /* delays and intervals specs */ unsigned int col_scan_delay_us; unsigned int col_switch_delay_ms;
- 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 <gsi@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(-)