input: keypad-matrix: doc update, hw separation, polling, binary columns

this series addresses one style nit in the matrix-keymap helper (bool vs error code for function calls), and brings the following changes to the matrix-keypad driver
- documentation update (device tree bindings), comments update (driver's program flow as well as data structures), diagnostics rephrased
- more finegrained hardware control (individual polarity for row and for column pins, push-pull operation of column pins)
- clean separation of the physical hardware attachment and the logical keyboard handling (tell GPIO pins and matrix lines apart)
- introduce support for polling vs interrupt driven change detection (more reliable detection of multi key press events)
- optionally reduce timer management overhead by using a range instead of a fixed value for a microseconds delay
- introduce support for keyboard layouts where fewer column gpio pins control more matrix column lines (binary column address encoding)
- adapt a board's device tree to the new features

for unadjusted code and configuration the behaviour remains fully backwards compatible, using any of the new features is completely optional

I'm aware of my introducing many new options, and not removing any of the flexibility to reduce complexity -- but I feel that there's nothing that can get removed without breaking compatibility, while all of the new options and features are desirable or just essential for the end result of binary encoded columns with push/pull requirements for the pins including reliable detection of multi key press events

even if it were not for the binary column addressing, most of the changes are desirable and removing the binary encoding doesn't significantly cut on the complexity

I'd happily trim flexibility and reduce complexity if it turns out that the former support which I did not want to break isn't required any longer (the open collector emulation might be one such candidate)

the series was build tested on PowerPC (mpc512x generic plus matrix-keypad), ARM (omap2plus plus matrix-keypad) and MIPS (bcm47xx plus matrix-keypad) and was run tested on PowerPC (mpc512x generic on an ifm AC14xx board)

test feedback from ARM users (palm tungsten, spitz, corgi with platform data, any recent ARM with matrix-keypad and device tree) is very welcome and greatly appreciated

the series is checkpatch clean in regular mode, and spots one condition in strict mode which I consider acceptable since memset() would be too much for two u32 values and individual assignments appear clumsy to me in this very case

CHECK: multiple assignments should be avoided #114: FILE: drivers/input/keyboard/matrix_keypad.c:671:
+ us[0] = us[1] = 0;

total: 0 errors, 0 warnings, 1 checks, 95 lines checked

Gerhard Sittig (12):
input: matrix-keypad: update devicetree binding doc
input: matrix-keymap: func call coding style nit
input: matrix-keypad: rename variables and funcs
input: matrix-keypad: push/pull, separate polarity
input: matrix-keypad: update comments, diagnostics
input: keypad-matrix: refactor matrix scan logic
input: keypad-matrix: introduce polling support
input: keypad-matrix: tell GPIO pins from matrix lines
input: matrix-keypad: add binary column encoding
input: keypad_matrix: use usleep_range() for scan delay
input: keypad-matrix: AC14xx device tree update
input: matrix-keypad: add diagnostics in probe()

 .../bindings/input/gpio-matrix-keypad.txt          |  138 ++++++-
 arch/arm/mach-davinci/board-tnetv107x-evm.c        |    3 +-
 arch/arm/mach-omap2/board-h4.c                     |    3 +-
 arch/arm/mach-pxa/palmtc.c                         |    3 +-
 arch/mips/jz4740/board-qi_lb60.c                   |    3 +-
 arch/powerpc/boot/dts/ac14xx.dts                   |   13 +-
 drivers/input/keyboard/matrix_keypad.c             |  399 +++++++++++++++++---
 drivers/input/matrix-keymap.c                      |   23 +-
 include/linux/input/matrix_keypad.h                |   44 ++-
 9 files changed, 538 insertions(+), 91 deletions(-)

1.7.10.4

update the device tree binding documentation for the GPIO matrix keypad
driver: mention the driver's selecting all columns at once, reword the delay descriptions, add the missing active low GPIO pin level property

---
 .../bindings/input/gpio-matrix-keypad.txt          |   42 +++++++++++++++++---
 1 file changed, 36 insertions(+), 6 deletions(-)

index ead641c..11d030e 100644
@@ -2,9 +2,30 @@
 
 GPIO driven matrix keypad is used to interface a SoC with a matrix keypad.
 The matrix keypad supports multiple row and column lines, a key can be
-placed at each intersection of a unique row and a unique column. The matrix
-keypad can sense a key-press and key-release by means of GPIO lines and
-report the event using GPIO interrupts to the cpu.
+placed at each intersection of a unique row and a unique column.
+
+Sampling the keypad status is done by individually activating column
+lines (which thus are outputs) and reading back row lines (which are
+inputs).  Each combination of row and column is checked separately to
+determine the status of a single key.
+
+To reduce load on the CPU, changes in the keys' status can get detected
+by means of GPIO interrupts, which get generated upon changes in the pin
+levels.  This approach assumes that any change in the key press status
+will result in a GPIO pin level change interrupt when all columns are
+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.
+
+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
+of the time, the use of current limiting resistors may be desirable, or
+column GPIO lines shall operate in open collector mode.
+
+More involved matrix layouts may include externally connected line
+drivers and might influence signal polarity.  The driver supports low
+active signals, while high active line levels are assumed by default.
 
 Required Properties:
 - compatible:		Should be "gpio-matrix-keypad"

@@ -20,9 +41,18 @@ Required Properties:
 Optional Properties:
 - linux,no-autorepeat:	do no enable autorepeat feature.
 - linux,wakeup:		use any event on keypad as wakeup event.
-- debounce-delay-ms:	debounce interval in milliseconds
-- col-scan-delay-us:	delay, measured in microseconds, that is needed
-			before we can scan keypad after activating column gpio
+- debounce-delay-ms:	debounce interval in milliseconds, which needs
+			to pass between detecting a change in the key
+			press status and sampling the new status, e.g.
+			to support multi key presses without spurious
+			single key events, or to cope with bouncing
+			mechanical key switches
+- col-scan-delay-us:	delay interval in microseconds between selecting
+			a column line and reading back its row status,
+			such that pin levels can settle after
+			propagating through the matrix and its
+			associated hardware components
+- gpio-activelow:	row pins as well as column pins are active low
 
 Example:
 	matrix-keypad {

1.7.10.4

Saying "driver" in a DT binding is a slight red flag. The binding should really purely describe HW, rather than SW's use of it. Here, the use is probably generic enough not to be an issue, although it there's some way to reword it to avoid that word it might be good.

That one change is definitely wrong. Each entry in row-gpios and col-gpios should include GPIO flags (in a format defined by the binding for the DT node providing the GPIO). Those flags include an active-high vs. active-low bit. In Linux, you can use of_get_gpio_flags() to retrieve a standardized representation of the flags.

Let me first thank you for reviewing the set and for providing so much feedback!

Then let me give a little background: The first part of the series with the doc update doesn't introduce anything new into the driver, it just makes the document catch up with what the driver already does.

And the later steps of extending the driver and its getting configured by device tree properties was heavily driven by the desire to keep up strict backwards compatibility. Since breaking a means of input and taking away a UNIX user's keyboard isn't good for your health. :) While I can and do test the setup which the extension is targetting at, I cannot test the other setups due to lack of hardware.

So when in doubt, I went with small and unintrusive steps and lots of remarks such that the default behaviour is strictly identical to before the change yet the desired new behaviour becomes available upon request, and all of it is documented.

I understand your concerns very well, since they crossed my mind too when I wrote that update.

One way of keeping the driver's details away from the device tree doc might be to create another file under Documentation/input/, discussing all of the driver's operation and the assumed or supported hardware setups there, and reducing the device tree binding doc to just "mention the fact" that something is adjustable and which property to use.

This would in addition reduce duplication with other approaches of configuring the driver (there's static platform data as well).

Regarding the layering: I'm aware of the one 'GPIO matrix keypad' driver implemented in drivers/input/keyboard/matrix_keypad.c, which uses a pdata structure which can get filled in by means of static platform data provided with code or by parsing the device tree (the preferred method for future extensions).

Other drivers may implement their own logic to scan matrices and detect changes, and share the pdata structure since they have to track similar conditions as well. That's the only reason why I had to touch other files than just keypad and keymap. But none of them are device tree aware AFAIK.

So yes, from what I can see, the GPIO matrix keypad driver is the only instance which probes the device tree and implements what the binding describes. Which is why I updated the device tree binding's doc to provide motivation why something might need adjustment and what the consequences are of specifying a parameter.

But this only applies if the binding is seen in concert with the Linux driver. When the binding gets used elsewhere in the kernel, or when code outside the kernel implements a binding, then the individual driver's details should be kept out of the binding doc.

Unless all drivers implement similar logic in just individual ways, because they all need to drive a keypad matrix and thus will face similar problems. Oh well, let's see how others feel or think about it ...

Another aspect to keep in mind here is that the current implementation already was layered into "the keypad" which drives the pins and detects presses and may get implemented in several drivers, and "the keymap" which translates key positions to key codes they generate and gets shared among many keypad drivers.

I'm not certain about whether the corgi, spitz, palm, tnetv107x and qi_lb60 implementations just share the pdata layout and implement their own logic, or just fillin the pdata from constants provided with the code yet end up creating the same GPIO matrix keypad driver.

If they brought their own logic, I could not see how they reference the individual settings, and when they don't inspect new settings then they end up with strictly the former behaviour.

If they create the same matrix driver that I extended, and just provide the settings by other means, then they can optionally make use of the new features yet see strictly identical behaviour if they don't adjust what later got introduced.

That's why I wrote about "simple ... layouts", but couldn't tell exactly when it was appropriate to discuss the more advanced setups as well, and in how much depth to do that in the device tree binding.

So is there a better way of putting this? Is the "simple" or the "mechanical" the issue in the description?

All of this (and the other feedback) suggests that I should have another look into the separation of the specific drivers details (under Documentation/input/ and strictly bound to the Linux kernel driver implementation) and DT binding (merely discussing how to specify parameters and not discussing how they affect the driver's operation).

See my introduction above. This isn't a "change", it's just catching up in the documentation and adding what was omitted before.

And I can see another issue here (maybe shadowed by the wording I used, referring to "row pins"): The fact that a pin may be inverted (within the SoC), and the fact that an externally connected component might require low active stimulus over otherwise high active pins/connections, might need to get reflected well. Or am I too picky here?

Are there other cases to learn from, where non-inverted physical pins get attached to components which require inverted logical information? Do we combine the overall polarity within the GPIO pin's abstraction, or do logical drivers above GPIO handle the inversion?

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: office at denx.de

On 06/22/2013 03:23 AM, Gerhard Sittig wrote:
...

...

I think both "simple" and "mechanical" should be removed. My reasoning:

At the level of connection between rows/columns, aren't all matrix keypads set up such that something connects a row to a column to signify a keypress, not just "simple layouts".

Similarly, "mechanical" should be removed since it's not always true, and even if it were, it would be irrelevant to anything outside the keyboard, perhaps aside from the need to debounce.

In fact, thinking about this more, I think all four paragraphs of text that this patch adds would be better in Documentation/input/, as you mentioned elsewhere. When introducing any extra properties, you could mention their potential use by a driver, as explanation for why they're useful, but there's probably no need to describe the complete operation of the driver in the DT binding.

Oh dear, that's quite unfortunate. I see that even though that property wasn't documented in the binding doc, arch/powerpc/boot/dts/ac14xx.dts has still already used it, so we probably can't fix it. I suppose we must simply document it, and ignore the shortcomings of that binding:-(

In general, I would expect the binding to represent the overall effective polarity of the signal that must be programmed into the relevant GPIO controller. SW doesn't really care how/why the signal is inverted, simply that it needs to (or doesn't) invert the signal when it programs the GPIO controller.

So the direction to go is
- move the Linux driver specific discussion to Documentation/input/ including potential hardware setups and the background on the driver's theory of operation
- just concentrate on "adjustables" in the binding document, merely listing the set and their units, while the motivation or background either "is obvious" or can get looked up in the driver's discussion

Reducing the set of options is orthogonal to that. Breaking backwards compatibility by changing the default behaviour after introducing more generic approaches to the specific issue is an option that remains for future changes.

Don't worry about the 'AC14xx' board too long, its using the keypad matrix driver is new in mainline, and still can get adjusted without too much problems before more wide spread use. "Getting it right" is what I'm currently heading for, while nothing is set in stone yet.

The actual issue I see is that
- the device tree binding specs get mapped to platform data
- the platform data is shared among the (device tree capable but as well used without a device tree) keypad matrix driver _and_ the other platform data providing machines (mostly ARM based)

So the drivers' logic referenced the 'active low' flag which the platform data already carried, which the device tree parse routines happen to setup. So when you see a reference to the property in a .dts, it's just "the tip of the ice berg" since there are other ways to communicate or setup that property.

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: office at denx.de

...

Yup, sounds good.

Breaking backwards-compatibility in the DT bindings would be problematic. They're supposed to be an ABI, which if it evolves, does so entirely in a backwards-compatible fashion.

This can usually be achieved by something like:

* If new DT properties present, enable new behaviour.
* If old DT properties present, or lack of new properties, enable old behaviour.

This allows somebody to install a specific DT on a system, then continue booting arbitrary new kernels on it without loss of functionality.

Given the "ABI-ness" of DT, I'm not convinced we don't have to worry about the AC14xx. I think we'll have to continue supporting that property, even if there's a newer/better/more-flexible way of representing the information too.

Don't get me wrong, I wasn't putting this lightly. From first hand experience I can tell that none of those boards in the field use current or even recent mainline kernels. All of them are running an older version which predates the introduction of 'AC14xx' support in kernel.org sources. Extending official support and "doing it the right way" is what I'm currently working on, before those boards may switch to running mainline kernels and compatibility with the first version that was used will become an issue. So to put it into other words:

_If_ the 'AC14xx' board is the _only_ entity which references the property, then there's no problem at all. Incompatible changes for _this_one_board_ are acceptable since at the moment the current implementation is not used in the field.

Of course I agree that breaking any other board isn't acceptable, which is why I followed the approach of strictly keeping backwards compatible behaviour even in those spots where a different approach or a different default would have looked more desireable.

Which is why I'm still reluctant to more intrusively change the general working of the keypad matrix driver in contrast to introducing strictly local changes which only optionally add additional features which explicitly must get enabled before changed behaviour will occur. So I need some more time to think of which parts to change in which ways and how to make sure that nothings breaks ...

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: office at denx.de

make the matrix_keypad_map_key() routine return an error code instead of boolean, as its name suggests an action and not a query

---
 drivers/input/matrix-keymap.c |   19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

index 08b61f5..b7091f2 100644
@@ -27,9 +27,10 @@
 #include <linux/module.h>
 #include <linux/input/matrix_keypad.h>
 
-static bool matrix_keypad_map_key(struct input_dev *input_dev,
-				  unsigned int rows, unsigned int cols,
-				  unsigned int row_shift, unsigned int key)
+/* translates packed row/col/code specs to the corresponding keycode[] item */
+static int matrix_keypad_map_key(struct input_dev *input_dev,
+				 unsigned int rows, unsigned int cols,
+				 unsigned int row_shift, unsigned int key)
 {
 	unsigned short *keymap = input_dev->keycode;
 	unsigned int row = KEY_ROW(key);

@@ -40,13 +41,13 @@ static bool matrix_keypad_map_key(struct input_dev *input_dev,
 		dev_err(input_dev->dev.parent,
 			"%s: invalid keymap entry 0x%x (row: %d, col: %d, rows: %d, cols: %d)\n",
 			__func__, key, row, col, rows, cols);
-		return false;
+		return -ERANGE;
 	}
 
 	keymap[MATRIX_SCAN_CODE(row, col, row_shift)] = code;
 	__set_bit(code, input_dev->keybit);
 
-	return true;
+	return 0;
 }
 
 #ifdef CONFIG_OF

@@ -109,8 +110,8 @@ static int matrix_keypad_parse_of_keymap(const char *propname,
 	for (i = 0; i < size; i++) {
 		unsigned int key = be32_to_cpup(prop + i);
 
-		if (!matrix_keypad_map_key(input_dev, rows, cols,
-					   row_shift, key))
+		if (matrix_keypad_map_key(input_dev, rows, cols,
+					  row_shift, key) != 0)
 			return -EINVAL;
 	}
 

@@ -187,8 +188,8 @@ int matrix_keypad_build_keymap(const struct matrix_keymap_data *keymap_data,
 		for (i = 0; i < keymap_data->keymap_size; i++) {
 			unsigned int key = keymap_data->keymap[i];
 
-			if (!matrix_keypad_map_key(input_dev, rows, cols,
-						   row_shift, key))
+			if (matrix_keypad_map_key(input_dev, rows, cols,
+						  row_shift, key) != 0)
 				return -EINVAL;
 		}
 	} else {

1.7.10.4

Dear Gerhard Sittig,

	return ret;

Will do, queued for v2. Thank you for reviewing this.

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: office at denx.de

Dear Gerhard Sittig,

Wait a little bit for some more feedback, esp. from the marvell guys ;-)

Best regards,
Marek Vasut

mechanical edits before code changes, to reduce diffs later on
- rename the 'work' member of the driver's private data to better reflect that it's the work routine which scans the matrix after a change of status was detected
- rename the __activate_col() routine to better reflect that it takes care of GPIO pins while its caller handles logical matrix lines

---
 drivers/input/keyboard/matrix_keypad.c |   24 ++++++++++++------------
 1 file changed, 12 insertions(+), 12 deletions(-)

index 71d7719..5b5f86d 100644
@@ -35,7 +35,7 @@ struct matrix_keypad {
 	DECLARE_BITMAP(disabled_gpios, MATRIX_MAX_ROWS);
 
 	uint32_t last_key_state[MATRIX_MAX_COLS];
-	struct delayed_work work;
+	struct delayed_work work_scan_matrix;
 	spinlock_t lock;
 	bool scan_pending;
 	bool stopped;

@@ -47,8 +47,8 @@ struct matrix_keypad {
  * minmal side effect when scanning other columns, here it is configured to
  * be input, and it should work on most platforms.
  */
-static void __activate_col(const struct matrix_keypad_platform_data *pdata,
-			   int col, bool on)
+static void __activate_col_pin(const struct matrix_keypad_platform_data *pdata,
+			       int col, bool on)
 {
 	bool level_on = !pdata->active_low;
 

@@ -63,7 +63,7 @@ static void __activate_col(const struct matrix_keypad_platform_data *pdata,
 static void activate_col(const struct matrix_keypad_platform_data *pdata,
 			 int col, bool on)
 {
-	__activate_col(pdata, col, on);
+	__activate_col_pin(pdata, col, on);
 
 	if (on && pdata->col_scan_delay_us)
 		udelay(pdata->col_scan_delay_us);

@@ -75,7 +75,7 @@ static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
 	int col;
 
 	for (col = 0; col < pdata->num_col_gpios; col++)
-		__activate_col(pdata, col, on);
+		__activate_col_pin(pdata, col, on);
 }
 
 static bool row_asserted(const struct matrix_keypad_platform_data *pdata,

@@ -116,8 +116,8 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
  */
 static void matrix_keypad_scan(struct work_struct *work)
 {
-	struct matrix_keypad *keypad =
-		container_of(work, struct matrix_keypad, work.work);
+	struct matrix_keypad *keypad = container_of(work, struct matrix_keypad,
+						    work_scan_matrix.work);
 	struct input_dev *input_dev = keypad->input_dev;
 	const unsigned short *keycodes = input_dev->keycode;
 	const struct matrix_keypad_platform_data *pdata = keypad->pdata;

@@ -189,8 +189,8 @@ static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
 
 	disable_row_irqs(keypad);
 	keypad->scan_pending = true;
-	schedule_delayed_work(&keypad->work,
-		msecs_to_jiffies(keypad->pdata->debounce_ms));
+	schedule_delayed_work(&keypad->work_scan_matrix,
+			      msecs_to_jiffies(keypad->pdata->debounce_ms));
 
 out:
 	spin_unlock_irqrestore(&keypad->lock, flags);

@@ -208,7 +208,7 @@ static int matrix_keypad_start(struct input_dev *dev)
 	 * Schedule an immediate key scan to capture current key state;
 	 * columns will be activated and IRQs be enabled after the scan.
 	 */
-	schedule_delayed_work(&keypad->work, 0);
+	schedule_delayed_work(&keypad->work_scan_matrix, 0);
 
 	return 0;
 }

@@ -219,7 +219,7 @@ static void matrix_keypad_stop(struct input_dev *dev)
 
 	keypad->stopped = true;
 	mb();
-	flush_work(&keypad->work.work);
+	flush_work(&keypad->work_scan_matrix.work);
 	/*
 	 * matrix_keypad_scan() will leave IRQs enabled;
 	 * we should disable them now.

@@ -495,7 +495,7 @@ static int matrix_keypad_probe(struct platform_device *pdev)
 	keypad->pdata = pdata;
 	keypad->row_shift = get_count_order(pdata->num_col_gpios);
 	keypad->stopped = true;
-	INIT_DELAYED_WORK(&keypad->work, matrix_keypad_scan);
+	INIT_DELAYED_WORK(&keypad->work_scan_matrix, matrix_keypad_scan);
 	spin_lock_init(&keypad->lock);
 
 	input_dev->name		= pdev->name;

1.7.10.4

extend the device tree adjustable hardware configuration:
- allow for differing polarity of the row and column GPIO pins
- optionally fully drive column output pins instead of the former unconditional open collector emulation approach

this change introduces individual polarity settings for the row pins as well as the column pins, to achieve this separate members in the platform data bundle get introduced, and all places which fill in the platform data receive an update -- the result is complete backwards compatibility with existing matrix drivers and an improvement for newly written code which needs the individual polarity settings

this change allows optional support to fully drive the column selection pins, but by default sticks with the former behaviour of driving the active level and being passive for the inactive level, so
- unadjusted configurations keep operating the hardware in an identical manner
- adjusted or newly configured hardware will be operated according to the configuration, which now allows for either setups with pull resistors or mere connections between logic

so this change doesn't break existing behaviour, is neutral for unadjusted code or configurations, and is an improvement in those areas where the former implementations was restricted

---
 .../bindings/input/gpio-matrix-keypad.txt          |   18 +++++++++
 arch/arm/mach-davinci/board-tnetv107x-evm.c        |    3 +-
 arch/arm/mach-omap2/board-h4.c                     |    3 +-
 arch/arm/mach-pxa/palmtc.c                         |    3 +-
 arch/mips/jz4740/board-qi_lb60.c                   |    3 +-
 drivers/input/keyboard/matrix_keypad.c             |   39 +++++++++++++++++---
 include/linux/input/matrix_keypad.h                |    9 ++++-
 7 files changed, 66 insertions(+), 12 deletions(-)

index 11d030e..e6a01eb 100644
@@ -27,6 +27,14 @@ More involved matrix layouts may include externally connected line
 drivers and might influence signal polarity.  The driver supports low
 active signals, while high active line levels are assumed by default.
 
+Since output drivers and input drivers may exist independently from each
+other and may or may not invert signals, polarity for row pins and for
+column pins is adjustable individually.  The initial implementation of
+the driver used to emulate some kind of an open collector behaviour in
+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.
+
 Required Properties:
 - compatible:		Should be "gpio-matrix-keypad"
 - row-gpios:		List of gpios used as row lines. The gpio specifier

@@ -52,7 +60,17 @@ Optional Properties:
 			such that pin levels can settle after
 			propagating through the matrix and its
 			associated hardware components
+- 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
+			(provided for backward compatibility, and useful
+			for matrix layouts of identical polarity for
+			rows and columns)
+- col-gpios-pushpull:	fully drive the column selection pins in either
+			direction (high and low signals), the default
+			behaviour is to actively drive low signals and
+			be passive otherwise (emulates an open collector
+			output driver)
 
 Example:
 	matrix-keypad {

index ba79837..d41bd8a 100644
@@ -199,7 +199,8 @@ static struct matrix_keypad_platform_data keypad_config = {
 	.num_row_gpios	= 6,
 	.num_col_gpios	= 5,
 	.debounce_ms	= 0, /* minimum */
-	.active_low	= 0, /* pull up realization */
+	.col_gpios_active_low	= 0, /* pull up realization */
+	.row_gpios_active_low	= 0, /* pull up realization */
 	.no_autorepeat	= 0,
 };
 

index 69c0acf..78c91cd 100644
@@ -97,7 +97,8 @@ static struct matrix_keypad_platform_data board_keypad_platform_data = {
 	.num_row_gpios	= ARRAY_SIZE(board_keypad_row_gpios),
 	.col_gpios	= board_keypad_col_gpios,
 	.num_col_gpios	= ARRAY_SIZE(board_keypad_col_gpios),
-	.active_low	= 1,
+	.col_gpios_active_low	= 1,
+	.row_gpios_active_low	= 1,
 
 	.debounce_ms		= 20,
 	.col_scan_delay_us	= 5,

index 100b176f..4767563 100644
@@ -314,7 +314,8 @@ static struct matrix_keypad_platform_data palmtc_keypad_platform_data = {
 	.num_row_gpios	= ARRAY_SIZE(palmtc_keypad_row_gpios),
 	.col_gpios	= palmtc_keypad_col_gpios,
 	.num_col_gpios	= ARRAY_SIZE(palmtc_keypad_col_gpios),
-	.active_low	= 1,
+	.col_gpios_active_low	= 1,
+	.row_gpios_active_low	= 1,
 
 	.debounce_ms		= 20,
 	.col_scan_delay_us	= 5,

index be2b3de..4235ca2 100644
@@ -250,7 +250,8 @@ static struct matrix_keypad_platform_data qi_lb60_pdata = {
 	.col_scan_delay_us	= 10,
 	.debounce_ms		= 10,
 	.wakeup			= 1,
-	.active_low		= 1,
+	.col_gpios_active_low	= 1,
+	.row_gpios_active_low	= 1,
 };
 
 static struct platform_device qi_lb60_keypad = {

index 5b5f86d..4f22149 100644
@@ -43,20 +43,37 @@ struct matrix_keypad {
 };
 
 /*
+ * former comment before introduction of optional push/pull behaviour:
+ * <cite>
  * NOTE: normally the GPIO has to be put into HiZ when de-activated to cause
  * minmal side effect when scanning other columns, here it is configured to
  * be input, and it should work on most platforms.
+ * </cite>
+ *
+ * the above sounds a lot like emulating open collector (open drain)
+ * behaviour in software, which should only be required if the GPIO pin
+ * lacks such a mode, which should be rare and could be hidden by
+ * transparent support in the GPIO chip driver, or could be handled by
+ * attaching an external transistor to the pin, which is cheap and in
+ * addition avoids damage in the hardware caused by running the wrong
+ * software configuration
+ *
+ * applying the "turn to input" logic unconditionally breaks hardware
+ * setups which have no pull resistors (i.e. where outputs are directly
+ * connected to the matrix or to external matrix line drivers), and
+ * could make input signals float and become unreliable
  */
 static void __activate_col_pin(const struct matrix_keypad_platform_data *pdata,
 			       int col, bool on)
 {
-	bool level_on = !pdata->active_low;
+	bool level_on = !pdata->col_gpios_active_low;
 
 	if (on) {
 		gpio_direction_output(pdata->col_gpios[col], level_on);
 	} else {
 		gpio_set_value_cansleep(pdata->col_gpios[col], !level_on);
-		gpio_direction_input(pdata->col_gpios[col]);
+		if (!pdata->col_gpios_push_pull)
+			gpio_direction_input(pdata->col_gpios[col]);
 	}
 }
 

@@ -82,7 +99,8 @@ static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
 			 int row)
 {
 	return gpio_get_value_cansleep(pdata->row_gpios[row]) ?
-			!pdata->active_low : pdata->active_low;
+			!pdata->row_gpios_active_low :
+			pdata->row_gpios_active_low;
 }
 
 static void enable_row_irqs(struct matrix_keypad *keypad)

@@ -317,7 +335,8 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
 			goto err_free_cols;
 		}
 
-		gpio_direction_output(pdata->col_gpios[i], !pdata->active_low);
+		gpio_direction_output(pdata->col_gpios[i],
+				      !pdata->col_gpios_active_low);
 	}
 
 	for (i = 0; i < pdata->num_row_gpios; i++) {

@@ -427,8 +446,16 @@ matrix_keypad_parse_dt(struct device *dev)
 		pdata->no_autorepeat = true;
 	if (of_get_property(np, "linux,wakeup", NULL))
 		pdata->wakeup = true;
-	if (of_get_property(np, "gpio-activelow", NULL))
-		pdata->active_low = true;
+	if (of_get_property(np, "col-gpios-activelow", NULL))
+		pdata->col_gpios_active_low = true;
+	if (of_get_property(np, "row-gpios-activelow", NULL))
+		pdata->row_gpios_active_low = true;
+	if (of_get_property(np, "gpio-activelow", NULL)) {
+		pdata->row_gpios_active_low = true;
+		pdata->col_gpios_active_low = true;
+	}
+	if (of_get_property(np, "col-gpios-pushpull", NULL))
+		pdata->col_gpios_push_pull = true;
 
 	of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms);
 	of_property_read_u32(np, "col-scan-delay-us",

index 27e06ac..5496a00 100644
@@ -45,7 +45,10 @@ struct matrix_keymap_data {
  * @clustered_irq: may be specified if interrupts of all row/column GPIOs
  *	are bundled to one single irq
  * @clustered_irq_flags: flags that are needed for the clustered irq
- * @active_low: gpio polarity
+ * @row_gpios_active_low: polarity of row gpio pins
+ * @col_gpios_active_low: polarity of column gpio pins
+ * @col_gpios_push_pull: whether column gpio pins emulate open drain
+ *	behaviour or fully drive pin levels to either direction
  * @wakeup: controls whether the device should be set up as wakeup
  *	source
  * @no_autorepeat: disable key autorepeat

@@ -70,7 +73,9 @@ struct matrix_keypad_platform_data {
 	unsigned int	clustered_irq;
 	unsigned int	clustered_irq_flags;
 
-	bool		active_low;
+	bool		row_gpios_active_low;
+	bool		col_gpios_active_low;
+	bool		col_gpios_push_pull;
 	bool		wakeup;
 	bool		no_autorepeat;
 };

1.7.10.4

I see your concerns and understand, but chose the above way to just extend the existing implementation in the least intrusive way without changing its nature. See the reply on the first patch for more details on the motivation.

Of course I agree that pin properties belong to the GPIO layer and that "application drivers" on top should not worry about that. Were I allowed to break backwards compatibility, then I could have done more shuffling. But lacking the capability to test the affected setups puts more burdon on others, which I wanted to avoid.

And then there's the question of whether the "active low" applies to the GPIO pin or to the matrix line (the above property suggests it's the pin, but I'm yet undecided), and whether the subsystems agree on a policy "where to put" the "overall polarity" of such a chain of driver logic and SoC pins and optionally involved inverting drivers and external components which the driver communicates to.

The push-pull vs open-drain should never have made it into the keypad driver, but that fact wasn't introduced but did exist. Even if the kernel had no way to setup OD mode of a pin, the bootloader has, and the kernel just needs to setup "values" just like for any other pin regardless of push-pull or open-drain.

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: office at denx.de

Can't you add this enhancement as follows:

Update the driver to look at the per-pin GPIO flags in all cases. Presumably in any existing cases, those flags all say "active high" anyway, since specifying anything else was useless. In the very unlikely case this isn't true, one could always add a quirk based on the HW-specific compatible value.

If gpio-activelow is set, mark all pins as active-low.

This way, any future extensions (i.e. what this patch implements: just rows active low, just columns active low) can be implemented using purely the standard GPIO flags, but the existing behaviour of the existing gpio-activelow property is maintained.

Maybe I missed something, but
- is "the 'active low' property of a GPIO pin" something platform specific? (maybe ARM inspired, introduced with pinctrl features)
- is it only available for specific GPIO controller drivers ("chips" or "banks"), or is it in some layer on top of gpiolib?
- is it in some other tree or branch than mainline?

Documentation/gpio.txt suggests and eyeballing the gpiolib.c source supports (v3.10-rc7) that the in-kernel API doesn't have an 'active-low' for pins. There is one for the sysfs API and pins that were exported to user space, but not for in-kernel use.

This fits with my observation that application drivers on top of gpiolib often take care of such a property which actually looks like it would belong to the physical attachment.

I understand that a chip's driver will hide when a SoC's pin is inverted, but I cannot see where gpiolib provides a means to hide an externally connected inverting driver.

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: office at denx.de

The way this property is represented is technically defined by each individual GPIO controller's DT binding. However, at least on ARM (and in fact I'd guess everywhere), many/most controllers do this in the same way; by adding an additional cell to #gpio-cells, which holds flags data, with values in that cell containing:

include/linux/of_gpio.h:

include/dt-bindings/gpio/gpio.h:

I'm not sure how new this is; it's possible older bindings don't have this extra cell. A quick grep of arch/*/boot/dts shows that almost all values of #gpio-cells are 2, so likely this is almost everywhere though.

It's been in mainline for quite a while now; a good number of kernel releases.

With the current gpiolib APIs, gpiolib only provides a way for GPIO clients to retrieve the GPIO flags from DT. It's then up to the GPIO client to actually make use of the flag, by inverting the value passed to gpio_set(), or returned from gpio_get().

Flags are retrieved by calling of_get_named_gpio_flags() rather than of_get_named_gpio().

There is a new GPIO API under development which I /think/ will hide this flag inside the GPIO core, so that GPIO clients don't have to care about it, and it all works automatically. I'm not sure what the status of this is though. Search Google for "gpio_get()".

Ah, so it's not a feature of gpiolib in itself, but in parallel to gpiolib.

In addition to the "chip and pin" spec in device tree bindings, one may specify additional flags, of which currently only 'active-low' exists. Thank you for the pointer. Yes I've seen "phandle plus two cell" specs everywhere, but haven't seen any non-zero value so far in the second cell.

Ah, so optionally inverting the level between the logical request in software and controlling the physical pin remains the job of the application driver (here: the input driver that manages the matrix which just happens to be attached to GPIOs), since it's not transparently done within gpiolib or the gpio chip driver.

But what the matrix driver shall do is not apply one 'invert everything' flag, but instead apply a flag that's specific to an individual GPIO pin. And this pin specific flag either gets setup from device tree information (from the flags cell of the GPIO spec), or gets derived from the platform data's 'invert everything' flag that applies to the non-DT case. And thus the individual polarity of rows and columns is a mere "byproduct" of respecting the individual pins' polarity. Nice. :)

Thank you, this pointer is much appreciated.

Oh, I understand that this involves a huge sweep over both layers of GPIO chip drivers as well as GPIO API using drivers. And is completely orthogonal to whether and when more drivers are tought to respect the pin's active-low property.

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: office at denx.de

- add comments about individual routines' purpose and their interaction, pre-conditions and consequences
- mark a few spots which may need some more attention or clarification
- rephrase some diagnostics messages

---
 drivers/input/keyboard/matrix_keypad.c |   81 +++++++++++++++++++++++++++++---
 drivers/input/matrix-keymap.c          |    4 +-
 include/linux/input/matrix_keypad.h    |   17 ++++---
 3 files changed, 89 insertions(+), 13 deletions(-)

index 4f22149..85e16a2 100644
@@ -43,6 +43,10 @@ struct matrix_keypad {
 };
 
 /*
+ * this routine controls a physical column pin which the keypad matrix
+ * is connected to, and takes care of the pin's polarity as well as its
+ * mode of operation (fully driven push/pull, or emulated open drain)
+ *
  * former comment before introduction of optional push/pull behaviour:
  * <cite>
  * NOTE: normally the GPIO has to be put into HiZ when de-activated to cause

@@ -77,6 +81,14 @@ static void __activate_col_pin(const struct matrix_keypad_platform_data *pdata,
 	}
 }
 
+/*
+ * this routine addresses logical columns of the keypad matrix, and
+ * makes sure that the "scan delay" is applied upon activation of the
+ * column (the delay between activating the column and reading rows)
+ *
+ * the caller ensures that this routine need not de-activate other
+ * columns when dealing with the column specified for the invocation
+ */
 static void activate_col(const struct matrix_keypad_platform_data *pdata,
 			 int col, bool on)
 {

@@ -86,6 +98,12 @@ static void activate_col(const struct matrix_keypad_platform_data *pdata,
 		udelay(pdata->col_scan_delay_us);
 }
 
+/*
+ * this routine either de-activates all columns before scanning the
+ * matrix, or re-activates all columns at the same time after the scan
+ * is complete, to make changes in the key press state trigger the
+ * condition to re-scan the matrix
+ */
 static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
 			      bool on)
 {

@@ -95,6 +113,10 @@ static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
 		__activate_col_pin(pdata, col, on);
 }
 
+/*
+ * this routine checks a single row while a specific column is active,
+ * it takes care of the pin's polarity, the pin level had time to settle
+ */
 static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
 			 int row)
 {

@@ -103,6 +125,12 @@ static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
 			pdata->row_gpios_active_low;
 }
 
+/*
+ * this routine enables IRQs after a keypad matrix scan has completed,
+ * 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
+ */
 static void enable_row_irqs(struct matrix_keypad *keypad)
 {
 	const struct matrix_keypad_platform_data *pdata = keypad->pdata;

@@ -116,6 +144,13 @@ static void enable_row_irqs(struct matrix_keypad *keypad)
 	}
 }
 
+/*
+ * this routine disables IRQs for changes in the key press status, which
+ * applies to shutdown or suspend modes, to periods where the keypad
+ * matrix is not used (not opened by any application), as well as to the
+ * interval between the first detected change and scanning the complete
+ * matrix (the debounce interval)
+ */
 static void disable_row_irqs(struct matrix_keypad *keypad)
 {
 	const struct matrix_keypad_platform_data *pdata = keypad->pdata;

@@ -130,7 +165,9 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
 }
 
 /*
- * This gets the keys from keyboard and reports it to input subsystem
+ * this routine scans the keypad matrix and detects changes in the keys'
+ * status against a previously sampled status, from which events for the
+ * input subsystem get derived
  */
 static void matrix_keypad_scan(struct work_struct *work)
 {

@@ -142,12 +179,12 @@ static void matrix_keypad_scan(struct work_struct *work)
 	uint32_t new_state[MATRIX_MAX_COLS];
 	int row, col, code;
 
-	/* de-activate all columns for scanning */
+	/* de-activate all columns before scanning the matrix */
 	activate_all_cols(pdata, false);
 
 	memset(new_state, 0, sizeof(new_state));
 
-	/* assert each column and read the row status out */
+	/* assert each column in turn and read back the row status */
 	for (col = 0; col < pdata->num_col_gpios; col++) {
 
 		activate_col(pdata, col, true);

@@ -159,6 +196,7 @@ static void matrix_keypad_scan(struct work_struct *work)
 		activate_col(pdata, col, false);
 	}
 
+	/* detect changes and derive input events, update the snapshot */
 	for (col = 0; col < pdata->num_col_gpios; col++) {
 		uint32_t bits_changed;
 

@@ -171,6 +209,15 @@ static void matrix_keypad_scan(struct work_struct *work)
 				continue;
 
 			code = MATRIX_SCAN_CODE(row, col, keypad->row_shift);
+			/*
+			 * TODO: the key code matrix may be sparse,
+			 * ignore items in gaps or report changes in all
+			 * positions?  this consideration may especially
+			 * apply where the key code matrix gets setup or
+			 * manipulated from user space, or where the key
+			 * code matrix gets switched (shift or function
+			 * keys, alternate keyboard modes)
+			 */
 			input_event(input_dev, EV_MSC, MSC_SCAN, code);
 			input_report_key(input_dev,
 					 keycodes[code],

@@ -178,9 +225,13 @@ static void matrix_keypad_scan(struct work_struct *work)
 		}
 	}
 	input_sync(input_dev);
-
 	memcpy(keypad->last_key_state, new_state, sizeof(new_state));
 
+	/*
+	 * re-enable all columns after the scan has completed, to have
+	 * changes in their key press status issue interrupts and
+	 * trigger another complete scan of the matrix
+	 */
 	activate_all_cols(pdata, true);
 
 	/* Enable IRQs again */

@@ -190,6 +241,14 @@ static void matrix_keypad_scan(struct work_struct *work)
 	spin_unlock_irq(&keypad->lock);
 }
 
+/*
+ * interrupt service routine, invoked upon the first detected change in
+ * the key press status, initiating a debounce delay, and suppressing
+ * subsequent interrupts until scanning all of the matrix has completed
+ *
+ * copes with interrupts which arrive during the debounce interval or
+ * the actual matrix scan or a shutdown or suspend sequence
+ */
 static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
 {
 	struct matrix_keypad *keypad = id;

@@ -310,6 +369,10 @@ static int matrix_keypad_resume(struct device *dev)
 	if (device_may_wakeup(&pdev->dev))
 		matrix_keypad_disable_wakeup(keypad);
 
+	/*
+	 * TODO: consider whether to only (re-)start the keypad matrix
+	 * driver when it was opened by applications?
+	 */
 	matrix_keypad_start(keypad->input_dev);
 
 	return 0;

@@ -425,7 +488,7 @@ matrix_keypad_parse_dt(struct device *dev)
 	int i, nrow, ncol;
 
 	if (!np) {
-		dev_err(dev, "device lacks DT data\n");
+		dev_err(dev, "device lacks DT data for the keypad config\n");
 		return ERR_PTR(-ENODEV);
 	}
 

@@ -435,6 +498,7 @@ matrix_keypad_parse_dt(struct device *dev)
 		return ERR_PTR(-ENOMEM);
 	}
 
+	/* 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) {

@@ -442,6 +506,7 @@ matrix_keypad_parse_dt(struct device *dev)
 		return ERR_PTR(-EINVAL);
 	}
 
+	/* get input, power, and GPIO pin properties */
 	if (of_get_property(np, "linux,no-autorepeat", NULL))
 		pdata->no_autorepeat = true;
 	if (of_get_property(np, "linux,wakeup", NULL))

@@ -457,10 +522,12 @@ matrix_keypad_parse_dt(struct device *dev)
 	if (of_get_property(np, "col-gpios-pushpull", NULL))
 		pdata->col_gpios_push_pull = true;
 
+	/* get delay and interval timing specs */
 	of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms);
 	of_property_read_u32(np, "col-scan-delay-us",
 						&pdata->col_scan_delay_us);
 
+	/* get the individual row and column GPIO pins */
 	gpios = devm_kzalloc(dev,
 			     sizeof(unsigned int) *
 				(pdata->num_row_gpios + pdata->num_col_gpios),

@@ -486,7 +553,7 @@ matrix_keypad_parse_dt(struct device *dev)
 static inline struct matrix_keypad_platform_data *
 matrix_keypad_parse_dt(struct device *dev)
 {
-	dev_err(dev, "no platform data defined\n");
+	dev_err(dev, "device lacks DT support for the keypad config\n");
 
 	return ERR_PTR(-EINVAL);
 }

@@ -521,6 +588,8 @@ 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);
+
+	/* start in stopped state, open(2) will activate the scan */
 	keypad->stopped = true;
 	INIT_DELAYED_WORK(&keypad->work_scan_matrix, matrix_keypad_scan);
 	spin_lock_init(&keypad->lock);

index b7091f2..c427bf9 100644
@@ -103,7 +103,9 @@ static int matrix_keypad_parse_of_keymap(const char *propname,
 
 	size = proplen / sizeof(u32);
 	if (size > max_keys) {
-		dev_err(dev, "OF: %s size overflow\n", propname);
+		dev_err(dev,
+			"OF: %s size overflow, keymap size %u, max keys %u\n",
+			propname, size, max_keys);
 		return -EINVAL;
 	}
 

index 5496a00..4bbe6b3 100644
@@ -39,9 +39,11 @@ 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
- * @col_scan_delay_us: delay, measured in microseconds, that is
- *	needed before we can keypad after activating column gpio
- * @debounce_ms: debounce interval in milliseconds
+ * @col_scan_delay_us: delay in microseconds, the interval between
+ *	activating a column and reading back row information
+ * @debounce_ms: debounce interval in milliseconds, the interval between
+ *	detecting a change in the key press status and determining the new
+ *	overall keypad matrix status
  * @clustered_irq: may be specified if interrupts of all row/column GPIOs
  *	are bundled to one single irq
  * @clustered_irq_flags: flags that are needed for the clustered irq

@@ -53,26 +55,29 @@ struct matrix_keymap_data {
  *	source
  * @no_autorepeat: disable key autorepeat
  *
- * This structure represents platform-specific data that use used by
+ * This structure represents platform-specific data that is used by
  * matrix_keypad driver to perform proper initialization.
  */
 struct matrix_keypad_platform_data {
+	/* map keys to codes */
 	const struct matrix_keymap_data *keymap_data;
 
+	/* the physical GPIO pin connections */
 	const unsigned int *row_gpios;
 	const unsigned int *col_gpios;
-
 	unsigned int	num_row_gpios;
 	unsigned int	num_col_gpios;
 
+	/* delays and intervals specs */
 	unsigned int	col_scan_delay_us;
 
-	/* key debounce interval in milli-second */
 	unsigned int	debounce_ms;
 
+	/* optionally reduce interrupt mgmt overhead */
 	unsigned int	clustered_irq;
 	unsigned int	clustered_irq_flags;
 
+	/* pin and input system properties */
 	bool		row_gpios_active_low;
 	bool		col_gpios_active_low;
 	bool		col_gpios_push_pull;

1.7.10.4

Dear Gerhard Sittig,

You might want to use kerneldoc for this , see Documentation/kernel-doc-nano- HOWTO.txt

Best regards,
Marek Vasut

- factor out determination of all rows for a column into a separate routine
- only call input_sync() when input events were generated (in the future empty key code positions may get skipped, in the current implementation short bounces may trigger matrix scans while no change is seen)

---
 drivers/input/keyboard/matrix_keypad.c |   43 ++++++++++++++++++++++----------
 1 file changed, 30 insertions(+), 13 deletions(-)

index 85e16a2..0b2599d 100644
@@ -126,6 +126,28 @@ static bool row_asserted(const struct matrix_keypad_platform_data *pdata,
 }
 
 /*
+ * this routine fetches the status of all rows within the specified
+ * column, the column will get selected and deselected before and after
+ * sampling the row status
+ */
+static uint32_t sample_rows_for_col(struct matrix_keypad *keypad, int col)
+{
+	const struct matrix_keypad_platform_data *pdata;
+	uint32_t row_state;
+	int row;
+
+	pdata = keypad->pdata;
+
+	activate_col(pdata, col, true);
+	row_state = 0;
+	for (row = 0; row < pdata->num_row_gpios; row++)
+		row_state |= row_asserted(pdata, row) ? (1 << row) : 0;
+	activate_col(pdata, col, false);
+
+	return row_state;
+}
+
+/*
  * this routine enables IRQs after a keypad matrix scan has completed,
  * to have any subsequent change in the key press status trigger the ISR
  *

@@ -178,25 +200,18 @@ static void matrix_keypad_scan(struct work_struct *work)
 	const struct matrix_keypad_platform_data *pdata = keypad->pdata;
 	uint32_t new_state[MATRIX_MAX_COLS];
 	int row, col, code;
+	int has_events;
 
 	/* de-activate all columns before scanning the matrix */
 	activate_all_cols(pdata, false);
 
-	memset(new_state, 0, sizeof(new_state));
-
 	/* assert each column in turn and read back the row status */
-	for (col = 0; col < pdata->num_col_gpios; col++) {
-
-		activate_col(pdata, col, true);
-
-		for (row = 0; row < pdata->num_row_gpios; row++)
-			new_state[col] |=
-				row_asserted(pdata, row) ? (1 << row) : 0;
-
-		activate_col(pdata, col, false);
-	}
+	memset(new_state, 0, sizeof(new_state));
+	for (col = 0; col < pdata->num_col_gpios; 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++) {
 		uint32_t bits_changed;
 

@@ -222,9 +237,11 @@ static void matrix_keypad_scan(struct work_struct *work)
 			input_report_key(input_dev,
 					 keycodes[code],
 					 new_state[col] & (1 << row));
+			has_events++;
 		}
 	}
-	input_sync(input_dev);
+	if (has_events)
+		input_sync(input_dev);
 	memcpy(keypad->last_key_state, new_state, sizeof(new_state));
 
 	/*

1.7.10.4

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

---
 .../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(-)

index e6a01eb..196935b 100644
@@ -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

index 4bbe6b3..3c8dc39 100644
@@ -41,6 +41,8 @@ struct matrix_keymap_data {
  * @num_col_gpios: actual number of col gpios used by device
  * @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
+ *	colums periodically get checked for changes in key press status
  * @debounce_ms: debounce interval in milliseconds, the interval between
  *	detecting a change in the key press status and determining the new
  *	overall keypad matrix status

@@ -70,7 +72,7 @@ struct matrix_keypad_platform_data {
 
 	/* delays and intervals specs */
 	unsigned int	col_scan_delay_us;
-
+	unsigned int	col_switch_delay_ms;
 	unsigned int	debounce_ms;
 
 	/* optionally reduce interrupt mgmt overhead */

1.7.10.4

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: office at denx.de

On 06/22/2013 03:50 AM, Gerhard Sittig wrote:

...

To me, this sounds more like something for Documentation/input/ rather than DT binding.

...

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:-)

- 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

---
 .../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(-)

index 196935b..f72d262 100644
@@ -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 {

index 3c8dc39..1398d23 100644
@@ -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;

1.7.10.4

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.

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: office at denx.de

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 ]

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 ...

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

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: office at denx.de

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

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).

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: office at denx.de

introduce support to optionally run a binary column address bit pattern on column GPIO pins in contrast to the formerly assumed one-out-of-N approach

---
 .../bindings/input/gpio-matrix-keypad.txt          |   46 ++++++++++++++++++--
 drivers/input/keyboard/matrix_keypad.c             |   39 ++++++++++++++++-
 include/linux/input/matrix_keypad.h                |    3 ++
 3 files changed, 83 insertions(+), 5 deletions(-)

index f72d262..75e9e28 100644
@@ -57,6 +57,18 @@ 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.
 
+The driver assumes that each row and column line of the keypad matrix is
+connected to a specific GPIO pin.  Alternatively column addresses can get
+encoded in binary form to reduce the number of GPIO pins involved, at the
+cost of adding an external decoder which translates the binary pattern of N
+GPIO coloumn pins into one of 2**N column lines of the matrix.  A mere
+circuit of two inverting open collector drivers in series connected to one
+output pin is a special case of this generic decoder approach.  Such a
+decoder may be desirable since it's cheap and protects the SoC from damage
+by unwanted interaction of signals when any combination of multiple keys
+gets pressed, regardless of the software's operating the column GPIO pins.
+The row GPIO pins won't interfere either since they all are inputs.
+
 Required Properties:
 - compatible:		Should be "gpio-matrix-keypad"
 - row-gpios:		List of gpios used as row lines. The gpio specifier

@@ -96,16 +108,21 @@ Optional Properties:
 			behaviour is to actively drive low signals and
 			be passive otherwise (emulates an open collector
 			output driver)
+- col-gpios-binary:	boolean, switches from the 'one GPIO pin for each
+			column' approach to the 'n GPIO pins to address
+			2**n columns' approach (binary encoding of column
+			addresses in contrast to a one-out-of-many pattern
+			for the column GPIO pins)
 - 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
+			column GPIO pins and their optional encoding, useful
+			when not all lines are in use for interconnects
 
-Example:
+Examples:
 	matrix-keypad {
 		compatible = "gpio-matrix-keypad";
 		debounce-delay-ms = <5>;

@@ -125,3 +142,26 @@ Example:
 				0x0101001C
 				0x0201006C>;
 	};
+
+	matrix_keypad {
+		compatible = "gpio-matrix-keypad";
+		debounce-delay-ms = <5>;
+		col-scan-delay-us = <1>;
+		col-switch-delay-ms = <20>;
+
+		col-gpios-binary;
+		col-gpios-pushpull;
+		col-gpios = <&gpio_pic 1 0>;
+
+		row-gpios-activelow;
+		row-gpios = <&gpio_pic 2 0
+			     &gpio_pic 3 0
+			     &gpio_pic 4 0>;
+
+		linux,keymap = <0x0000006e
+				0x01000067
+				0x02000066
+				0x00010069
+				0x0101006c
+				0x0201006a>;
+	}

index 30b7faf..c015f0d 100644
@@ -47,7 +47,9 @@ struct matrix_keypad {
 /*
  * this routine controls a physical column pin which the keypad matrix
  * is connected to, and takes care of the pin's polarity as well as its
- * mode of operation (fully driven push/pull, or emulated open drain)
+ * mode of operation (fully driven push/pull, or emulated open drain),
+ * optional encoding of column address information is taken care of by
+ * the caller
  *
  * former comment before introduction of optional push/pull behaviour:
  * <cite>

@@ -88,13 +90,30 @@ static void __activate_col_pin(const struct matrix_keypad_platform_data *pdata,
  * makes sure that the "scan delay" is applied upon activation of the
  * column (the delay between activating the column and reading rows)
  *
+ * optional encoding of logical column address information and its
+ * mapping to column GPIO pin levels is taken care of as well
+ *
  * the caller ensures that this routine need not de-activate other
  * columns when dealing with the column specified for the invocation
  */
 static void activate_col(const struct matrix_keypad_platform_data *pdata,
 			 int col, bool on)
 {
-	__activate_col_pin(pdata, col, on);
+	int bit, lvl;
+
+	if (!pdata->col_gpios_binary_encoded) {
+		/* one-out-of-N approach, just setup the one pin */
+		__activate_col_pin(pdata, col, on);
+	} else if (on) {
+		/* setup the address bit pattern on all column pins */
+		for (bit = 0; bit < pdata->num_col_gpios; bit++) {
+			lvl = (col & (1 << bit)) ? 1 : 0;
+			__activate_col_pin(pdata, bit, lvl);
+		}
+	} else {
+		/* binary column encoding has no concept of "deselection" */
+		/* EMPTY */
+	}
 
 	if (on && pdata->col_scan_delay_us)
 		udelay(pdata->col_scan_delay_us);

@@ -105,12 +124,20 @@ static void activate_col(const struct matrix_keypad_platform_data *pdata,
  * matrix, or re-activates all columns at the same time after the scan
  * is complete, to make changes in the key press state trigger the
  * condition to re-scan the matrix
+ *
+ * this routine does not apply to the "binary encoded column addresses"
+ * approach, which has no concept of de-selecting columns, and cannot
+ * select multiple columns at the same time; the periodic column switch
+ * work routine takes care of detecting status changes in this mode
  */
 static void activate_all_cols(const struct matrix_keypad_platform_data *pdata,
 			      bool on)
 {
 	int col;
 
+	if (pdata->col_gpios_binary_encoded)
+		return;
+
 	for (col = 0; col < pdata->num_matrix_cols; col++)
 		__activate_col_pin(pdata, col, on);
 }

@@ -623,6 +650,8 @@ matrix_keypad_parse_dt(struct device *dev)
 	}
 	if (of_get_property(np, "col-gpios-pushpull", NULL))
 		pdata->col_gpios_push_pull = true;
+	if (of_get_property(np, "col-gpios-binary", NULL))
+		pdata->col_gpios_binary_encoded = true;
 
 	/* get delay and interval timing specs */
 	of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms);

@@ -630,6 +659,10 @@ matrix_keypad_parse_dt(struct device *dev)
 			     &pdata->col_scan_delay_us);
 	of_property_read_u32(np, "col-switch-delay-ms",
 			     &pdata->col_switch_delay_ms);
+	if (pdata->col_gpios_binary_encoded && !pdata->col_switch_delay_ms) {
+		dev_err(dev, "binary mode needs a column switch delay\n");
+		return ERR_PTR(-EINVAL);
+	}
 
 	/* get the individual row and column GPIO pins */
 	gpios = devm_kzalloc(dev,

@@ -658,6 +691,8 @@ matrix_keypad_parse_dt(struct device *dev)
 	 */
 	pdata->num_matrix_rows = pdata->num_row_gpios;
 	pdata->num_matrix_cols = pdata->num_col_gpios;
+	if (pdata->col_gpios_binary_encoded)
+		pdata->num_matrix_cols = 1 << pdata->num_col_gpios;
 	err = matrix_keypad_parse_of_params(dev,
 					    &pdata->num_matrix_rows,
 					    &pdata->num_matrix_cols);

index 1398d23..82db84a 100644
@@ -55,6 +55,8 @@ struct matrix_keymap_data {
  * @col_gpios_active_low: polarity of column gpio pins
  * @col_gpios_push_pull: whether column gpio pins emulate open drain
  *	behaviour or fully drive pin levels to either direction
+ * @col_gpios_binary_encoded: whether column gpio pins carry 1-of-N or
+ *	binary encoded column address information
  * @wakeup: controls whether the device should be set up as wakeup
  *	source
  * @no_autorepeat: disable key autorepeat

@@ -89,6 +91,7 @@ struct matrix_keypad_platform_data {
 	bool		row_gpios_active_low;
 	bool		col_gpios_active_low;
 	bool		col_gpios_push_pull;
+	bool		col_gpios_binary_encoded;
 	bool		wakeup;
 	bool		no_autorepeat;
 };

1.7.10.4

querying keyboards isn't a time critical task and does not depend on exact timing in the microseconds order -- the timeouts and delays are arbitrary choices or educated guesses at best anyway

so we can be nice and reduce stress on the timer management
- use usleep_range() for the scan delay (a delay in microseconds between activating a column and reading rows)
- allow optional range specs in device tree nodes in addition to the previous simple integer spec of an exact delay
- allow for platform data to optionally specify a delay range

this implementation assumes an exact delay with the first or only integer spec when no upper bound for the range was specified as well as when the range spec is invalid, as well as keeps up the scan delay's being optional -- this results in compatible behaviour to the previous implementation and an improvement on system load when the newly introduced feature gets used

normalization of the range spec (enforcing a valid upper bound against the lower bound, and assuming the exact value when no range was specified) happens during probe in the devide tree case, but cannot be done for the non-DT case because the platform data is read-only during probe -- that's why the routine which enables the columns and optionally applies the delay will enforce the constraints on the range spec -- this is not a problem, and is perfectly acceptable given the low cost of the min/max check and the potential benefit of reduced timer management cost

---
 .../bindings/input/gpio-matrix-keypad.txt          |    7 +++--
 drivers/input/keyboard/matrix_keypad.c             |   29 +++++++++++++++++---
 include/linux/input/matrix_keypad.h                |    5 ++++
 3 files changed, 35 insertions(+), 6 deletions(-)

index 75e9e28..1562681 100644
@@ -93,7 +93,10 @@ Optional Properties:
 			a column line and reading back its row status,
 			such that pin levels can settle after
 			propagating through the matrix and its
-			associated hardware components
+			associated hardware components, can be specified
+			with either one value giving the exact delay, or
+			with two values giving a delay range (allowing
+			for reduced timer management overhead)
 - col-switch-delay-ms:	columns switch interval in milliseconds instead
 			of using interrupts to detect key press changes,
 			enables polling mode when specified

@@ -146,7 +149,7 @@ Examples:
 	matrix_keypad {
 		compatible = "gpio-matrix-keypad";
 		debounce-delay-ms = <5>;
-		col-scan-delay-us = <1>;
+		col-scan-delay-us = <2 10>;
 		col-switch-delay-ms = <20>;
 
 		col-gpios-binary;

index c015f0d..275b157 100644
@@ -100,6 +100,7 @@ static void activate_col(const struct matrix_keypad_platform_data *pdata,
 			 int col, bool on)
 {
 	int bit, lvl;
+	u32 delay_min, delay_max;
 
 	if (!pdata->col_gpios_binary_encoded) {
 		/* one-out-of-N approach, just setup the one pin */

@@ -115,8 +116,19 @@ static void activate_col(const struct matrix_keypad_platform_data *pdata,
 		/* EMPTY */
 	}
 
-	if (on && pdata->col_scan_delay_us)
-		udelay(pdata->col_scan_delay_us);
+	if (on && pdata->col_scan_delay_us) {
+		/*
+		 * the platform data is 'const' aka 'read-only' here, so
+		 * we may not modify it, yet might not have had a chance
+		 * to normalize its data before (we did for the DT case,
+		 * but did not for non-DT cases), which is why we have
+		 * to use local variables here to enforce sane input
+		 * data for the usleep range spec
+		 */
+		delay_min = pdata->col_scan_delay_us;
+		delay_max = max(delay_min, pdata->col_scan_delay_us_max);
+		usleep_range(delay_min, delay_max);
+	}
 }
 
 /*

@@ -609,6 +621,7 @@ matrix_keypad_parse_dt(struct device *dev)
 	unsigned int *gpios;
 	int i;
 	int err;
+	u32 us[2];
 
 	if (!np) {
 		dev_err(dev, "device lacks DT data for the keypad config\n");

@@ -655,8 +668,15 @@ matrix_keypad_parse_dt(struct device *dev)
 
 	/* get delay and interval timing specs */
 	of_property_read_u32(np, "debounce-delay-ms", &pdata->debounce_ms);
-	of_property_read_u32(np, "col-scan-delay-us",
-			     &pdata->col_scan_delay_us);
+	us[0] = us[1] = 0;
+	err = of_property_read_u32_array(np, "col-scan-delay-us", &us[0], 2);
+	if (err == -EOVERFLOW) {
+		err = of_property_read_u32(np, "col-scan-delay-us", &us[0]);
+		us[1] = us[0];
+	}
+	us[1] = max(us[1], us[0]);
+	pdata->col_scan_delay_us = us[0];
+	pdata->col_scan_delay_us_max = us[1];
 	of_property_read_u32(np, "col-switch-delay-ms",
 			     &pdata->col_switch_delay_ms);
 	if (pdata->col_gpios_binary_encoded && !pdata->col_switch_delay_ms) {

@@ -744,6 +764,7 @@ static int matrix_keypad_probe(struct platform_device *pdev)
 	/* start in stopped state, open(2) will activate the scan */
 	keypad->stopped = true;
 	INIT_DELAYED_WORK(&keypad->work_scan_matrix, matrix_keypad_scan);
+	/* we cannot normalize pdata here for the non-DT case, it's read-only */
 	INIT_DELAYED_WORK(&keypad->work_switch_column, matrix_keypad_switch);
 	keypad->col_to_poll = pdata->num_matrix_cols;
 	spin_lock_init(&keypad->lock);

index 82db84a..fa093b2 100644
@@ -43,6 +43,10 @@ struct matrix_keymap_data {
  * @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_scan_delay_us_max: maximum delay in microseconds between
+ *	activating a column and reading back row information,
+ *	defaults to @col_scan_delay_us when not specified, allows
+ *	for reduced timer management overhead
  * @col_switch_delay_ms: delay in milliseconds, the interval with which
  *	colums periodically get checked for changes in key press status
  * @debounce_ms: debounce interval in milliseconds, the interval between

@@ -80,6 +84,7 @@ struct matrix_keypad_platform_data {
 
 	/* delays and intervals specs */
 	unsigned int	col_scan_delay_us;
+	unsigned int	col_scan_delay_us_max;
 	unsigned int	col_switch_delay_ms;
 	unsigned int	debounce_ms;
 

1.7.10.4

Yes, this puts less burdon on the .dts author. The "problem" would be to come up (programmatically, without the user's spec) with an appropriate upper bound.

One might choose "half the col switch delay" when available (in the polling scenario). Or "three times the lower bound". Or an arbitrary upper bound in the 100us order. Or actually with the minimum of all the above. That should keep the absolute minimum (user specified) in the loop, and scan the keys fast enough, yet drastically reduce timer management overhead, and hide all of this from the .dts author.

I will ponder this for a moment ...

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: office at denx.de

Shouldn't this be driven by whatever key repeat delays the user configured in SW; scan only fast enough to implement the correct repeat delay/rate?

- catchup in the 'ifm AC14xx' board's device tree with recently added keypad-matrix driver features: individual pin polarity for rows and columns, push/pull pin operation for columns
- slight re-ordering of keypad-matrix related settings, and tuning of timing specs for more reliable detection of short lived key presses (including a range spec to reduce timer management overhead) as well as a slightly increased overall matrix poll frequency
- fix keymap comments, make the names match the codes

---
 arch/powerpc/boot/dts/ac14xx.dts |   13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

index a27a460..502cc37 100644
@@ -350,13 +350,14 @@
 		matrix_keypad at 0 {
 			compatible = "gpio-matrix-keypad";
 			debounce-delay-ms = <5>;
-			col-scan-delay-us = <1>;
-			gpio-activelow;
-			col-gpios-binary;
-			col-switch-delay-ms = <200>;
+			col-scan-delay-us = <5 100>;
+			col-switch-delay-ms = <15>;
 
+			col-gpios-binary;
+			col-gpios-pushpull;
 			col-gpios = <&gpio_pic 1 0>;	/* pin1 */
 
+			row-gpios-activelow;
 			row-gpios = <&gpio_pic 2 0	/* pin2 */
 				     &gpio_pic 3 0	/* pin3 */
 				     &gpio_pic 4 0>;	/* pin4 */

@@ -365,8 +366,8 @@
 					0x01000067	/* UP */
 					0x02000066	/* FN RIGHT */
 					0x00010069	/* LEFT */
-					0x0101006a	/* DOWN */
-					0x0201006c>;	/* RIGHT */
+					0x0101006a	/* RIGHT */
+					0x0201006c>;	/* DOWN */
 		};
 	};
 

1.7.10.4

optionally dump relevant configuration of pins, matrix lines, and delay/interval times at the very end of the probe routine (development feature, silent by default)

---
 drivers/input/keyboard/matrix_keypad.c |   18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

index 275b157..69b15720 100644
@@ -800,6 +800,24 @@ static int matrix_keypad_probe(struct platform_device *pdev)
 	device_init_wakeup(&pdev->dev, pdata->wakeup);
 	platform_set_drvdata(pdev, keypad);
 
+	dev_dbg(&pdev->dev, "gpios col num[%u] lvl[%s] drv[%s] enc[%s]\n",
+		pdata->num_col_gpios,
+		pdata->col_gpios_active_low ? "low" : "high",
+		pdata->col_gpios_push_pull ? "pp" : "od",
+		pdata->col_gpios_binary_encoded ? "bin" : "1-1");
+	dev_dbg(&pdev->dev, "gpios row num[%u] lvl[%s]\n",
+		pdata->num_row_gpios,
+		pdata->row_gpios_active_low ? "low" : "high");
+	dev_dbg(&pdev->dev, "matrix cols[%u] rows[%u] code shift[%u]\n",
+		pdata->num_matrix_cols,
+		pdata->num_matrix_rows,
+		keypad->row_shift);
+	dev_dbg(&pdev->dev, "times scan[%u-%u] bounce[%u] switch[%u]\n",
+		pdata->col_scan_delay_us,
+		pdata->col_scan_delay_us_max,
+		pdata->debounce_ms,
+		pdata->col_switch_delay_ms);
+
 	return 0;
 
 err_free_gpio:

1.7.10.4

Dear Gerhard Sittig,

This output shall reflect not only what was provided to the driver as input, but as well what the driver made of it (that is, some of the values get derived from others, some underwent normalization).

The terse format mostly is a consequence of the 80 columns line length limit, and the output being for debugging purposes exclusively. But I understand that one needs to know the driver's internal working before one can read this output. And I felt that this is the only situation where one would enable debugging for the driver.

That patch might as well get dropped, I have no strong feelings about it. Actually I put it at the end of the series such that nothing else builds on top of it and that it doesn't change the context of anything else, so that dropping it doesn't matter anyway.

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: office at denx.de