[PATCH 0/6] Input: matrix-keypad: Various performance improvements

[PATCH 0/6] Input: matrix-keypad: Various performance improvements

[Markus Burri]

This series is needed to avoid key-loss if the GPIOs are connected via a I2C GPIO Mux
that introduces additonal latencies between key change and matrix scan.
 
Patch 1, 2 and 6 in this series introduce a performance boost on its own
and can be merged separately.
 
Patch 3, 4 and 5 depend on each other.

Markus Burri (6):
  Input: matrix_keypad - move gpio-row init to the init part
  Input: matrix_keypad - use fsleep for variable delay duration
  Input: matrix_keypad - add function for reading row state
  dt-bindings: input: matrix_keypad - add settle time after enable all
    columns
  Input: matrix_keypad - add settle time after enable all columns
  Input: matrix_keypad - detect change during scan

 .../bindings/input/gpio-matrix-keypad.txt     |  2 +
 drivers/input/keyboard/matrix_keypad.c        | 44 ++++++++++++++++---
 2 files changed, 39 insertions(+), 7 deletions(-)

-- 
2.39.5

[PATCH 1/6] Input: matrix_keypad - move gpio-row init to the init part

[Markus Burri]

If the matrix keypad is wired over a slow interface
(e.g. a gpio-expansion over i2c), the scan can take a longer time.
Move the initialisation of row gpio from scan to the init function will
reduce the scan time and therefore increase scan performance.

Signed-off-by: Markus Burri <markus.burri@mt.com>
Reviewed-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: HeDong Zhao <hedong.zhao@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 3c38bae..a2ec9f6 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -118,9 +118,6 @@ static void matrix_keypad_scan(struct work_struct *work)
 
 	memset(new_state, 0, sizeof(new_state));
 
-	for (row = 0; row < keypad->num_row_gpios; row++)
-		gpiod_direction_input(keypad->row_gpios[row]);
-
 	/* assert each column and read the row status out */
 	for (col = 0; col < keypad->num_col_gpios; col++) {
 
@@ -326,6 +323,8 @@ static int matrix_keypad_init_gpio(struct platform_device *pdev,
 
 		if (active_low ^ gpiod_is_active_low(keypad->row_gpios[i]))
 			gpiod_toggle_active_low(keypad->row_gpios[i]);
+
+		gpiod_direction_input(keypad->row_gpios[i]);
 	}
 
 	return 0;
-- 
2.39.5

[PATCH 2/6] Input: matrix_keypad - use fsleep for variable delay duration

[Markus Burri]

The delay is retrieved from a device-tree property, so the duration is
variable. fsleep guesses the best delay function based on duration.

Link: https://www.kernel.org/doc/html/latest/timers/timers-howto.html

Signed-off-by: Markus Burri <markus.burri@mt.com>
Reviewed-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: HeDong Zhao <hedong.zhao@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index a2ec9f6..e08d7d5 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -69,7 +69,7 @@ static void activate_col(struct matrix_keypad *keypad, int col, bool on)
 	__activate_col(keypad, col, on);
 
 	if (on && keypad->col_scan_delay_us)
-		udelay(keypad->col_scan_delay_us);
+		fsleep(keypad->col_scan_delay_us);
 }
 
 static void activate_all_cols(struct matrix_keypad *keypad, bool on)
-- 
2.39.5

[PATCH 3/6] Input: matrix_keypad - add function for reading row state

[Markus Burri]

Move the evaluation of a row state into separate function.
It will be also used by a change later in this series.

Signed-off-by: Markus Burri <markus.burri@mt.com>
Reviewed-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: HeDong Zhao <hedong.zhao@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index e08d7d5..c091f90 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -101,6 +101,16 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
 		disable_irq_nosync(keypad->row_irqs[i]);
 }
 
+static uint32_t read_row_state(struct matrix_keypad *keypad)
+{
+	int row;
+	u32 row_state = 0;
+
+	for (row = 0; row < keypad->num_row_gpios; row++)
+		row_state |= row_asserted(keypad, row) ? BIT(row) : 0;
+	return row_state;
+}
+
 /*
  * This gets the keys from keyboard and reports it to input subsystem
  */
@@ -123,9 +133,7 @@ static void matrix_keypad_scan(struct work_struct *work)
 
 		activate_col(keypad, col, true);
 
-		for (row = 0; row < keypad->num_row_gpios; row++)
-			new_state[col] |=
-				row_asserted(keypad, row) ? BIT(row) : 0;
+		new_state[col] = read_row_state(keypad);
 
 		activate_col(keypad, col, false);
 	}
-- 
2.39.5

[PATCH 4/6] dt-bindings: input: matrix_keypad - add settle time after enable all columns

[Markus Burri]

Matrix_keypad with high capacity need a longer settle time after enable
all columns.
Add optional property to specify the settle time

Signed-off-by: Markus Burri <markus.burri@mt.com>
Reviewed-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: HeDong Zhao <hedong.zhao@mt.com>
---
 Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
index 570dc10..cf43fa0 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
@@ -24,6 +24,8 @@ Optional Properties:
 - 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
+- all-cols-on-delay-us:	delay, measured in microseconds, that is needed
+			after activating all column gpios, default is no delay.
 - drive-inactive-cols:	drive inactive columns during scan,
 			default is to turn inactive columns into inputs.
 
-- 
2.39.5

Re: [PATCH 4/6] dt-bindings: input: matrix_keypad - add settle time after enable all columns

[Krzysztof Kozlowski]

On Thu, Oct 31, 2024 at 07:30:02AM +0100, Markus Burri wrote:
> Matrix_keypad with high capacity need a longer settle time after enable
> all columns.
> Add optional property to specify the settle time
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> Reviewed-by: Manuel Traut <manuel.traut@mt.com>
> Tested-by: HeDong Zhao <hedong.zhao@mt.com>

How was a TXT binding tested?

Both tags look fake, where were they given?

> ---
>  Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> index 570dc10..cf43fa0 100644
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> @@ -24,6 +24,8 @@ Optional Properties:
>  - 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
> +- all-cols-on-delay-us:	delay, measured in microseconds, that is needed
> +			after activating all column gpios, default is no delay.

Binding should be converted to DT schema first.

Best regards,
Krzysztof

[PATCH 5/6] Input: matrix_keypad - add settle time after enable all columns

[Markus Burri]

Matrix_keypad with high capacity need a longer settle time after enable
all columns and re-enabling interrupts.
This to give time stable the system and not generate interrupts.

Add a new optional device-tree property to configure the time before
enabling interrupts after disable all columns.
The default is no delay.

Signed-off-by: Markus Burri <markus.burri@mt.com>
Reviewed-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: HeDong Zhao <hedong.zhao@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index c091f90..f0d3004 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -27,6 +27,7 @@ struct matrix_keypad {
 	unsigned int row_shift;
 
 	unsigned int col_scan_delay_us;
+	unsigned int all_cols_on_delay_us;
 	/* key debounce interval in milli-second */
 	unsigned int debounce_ms;
 	bool drive_inactive_cols;
@@ -78,6 +79,9 @@ static void activate_all_cols(struct matrix_keypad *keypad, bool on)
 
 	for (col = 0; col < keypad->num_col_gpios; col++)
 		__activate_col(keypad, col, on);
+
+	if (on && keypad->all_cols_on_delay_us)
+		fsleep(keypad->all_cols_on_delay_us);
 }
 
 static bool row_asserted(struct matrix_keypad *keypad, int row)
@@ -402,6 +406,8 @@ static int matrix_keypad_probe(struct platform_device *pdev)
 				 &keypad->debounce_ms);
 	device_property_read_u32(&pdev->dev, "col-scan-delay-us",
 				 &keypad->col_scan_delay_us);
+	device_property_read_u32(&pdev->dev, "all-cols-on-delay-us",
+				 &keypad->all_cols_on_delay_us);
 
 	err = matrix_keypad_init_gpio(pdev, keypad);
 	if (err)
-- 
2.39.5

[PATCH 6/6] Input: matrix_keypad - detect change during scan

[Markus Burri]

For a setup where the matrix keypad is connected over a slow interface
(e.g. a gpio-expansion over i2c), the scan can take a longer time to read.

Interrupts need to be disabled during scan. And therefore changes in this
period are not detected.
To improve this situation, scan the matrix again if the row state changed
during interrupts disabled.

Signed-off-by: Markus Burri <markus.burri@mt.com>
Reviewed-by: Manuel Traut <manuel.traut@mt.com>
Tested-by: HeDong Zhao <hedong.zhao@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index f0d3004..428b36e 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -126,6 +126,10 @@ static void matrix_keypad_scan(struct work_struct *work)
 	const unsigned short *keycodes = input_dev->keycode;
 	uint32_t new_state[MATRIX_MAX_COLS];
 	int row, col, code;
+	u32 init_row_state, new_row_state;
+
+	/* read initial row state to detect changes between scan */
+	init_row_state = read_row_state(keypad);
 
 	/* de-activate all columns for scanning */
 	activate_all_cols(keypad, false);
@@ -171,6 +175,19 @@ static void matrix_keypad_scan(struct work_struct *work)
 	keypad->scan_pending = false;
 	enable_row_irqs(keypad);
 	spin_unlock_irq(&keypad->lock);
+
+	/* read new row state and detect if value has changed */
+	new_row_state = read_row_state(keypad);
+	if (init_row_state != new_row_state) {
+		spin_lock_irq(&keypad->lock);
+		if (likely(!keypad->scan_pending && !keypad->stopped)) {
+			disable_row_irqs(keypad);
+			keypad->scan_pending = true;
+			schedule_delayed_work(&keypad->work,
+					      msecs_to_jiffies(keypad->debounce_ms));
+		}
+		spin_unlock_irq(&keypad->lock);
+	}
 }
 
 static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
-- 
2.39.5

[PATCH v2 0/7] Input: matrix-keypad: Various performance improvements

[Markus Burri]

This series is needed to avoid key-loss if the GPIOs are connected via a I2C GPIO Mux
that introduces additonal latencies between key change and matrix scan.

This series applies on 6.12-rc6

Changes in V2:
* added patch to convert dt-bindings to YAML
* added missing dt-bindings properties description
* removed [PATCH 1/6] Input: matrix_keypad - move gpio-row init to the init
  it would revert 01c84b03d80aab9f04c4e3e1f9085f4202ff7c29
* removed internally given Tested-by and Reviewed-by tags

V1: https://lore.kernel.org/lkml/20241031063004.69956-1-markus.burri@mt.com/ 

Markus Burri (7):
  Input: matrix_keypad - use fsleep for variable delay duration
  Input: matrix_keypad - add function for reading row state
  dt-bindings: input: matrix_keypad - convert to YAML
  dt-bindings: input: matrix_keypad - add missing property
  dt-bindings: input: matrix_keypad - add settle time after enable all
    columns
  Input: matrix_keypad - add settle time after enable all columns
  Input: matrix_keypad - detect change during scan

 .../bindings/input/gpio-matrix-keypad.txt     |  49 --------
 .../bindings/input/gpio-matrix-keypad.yaml    | 110 ++++++++++++++++++
 drivers/input/keyboard/matrix_keypad.c        |  39 ++++++-
 3 files changed, 145 insertions(+), 53 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
 create mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml

-- 
2.39.5

[PATCH v2 1/7] Input: matrix_keypad - use fsleep for variable delay duration

[Markus Burri]

The delay is retrieved from a device-tree property, so the duration is
variable. fsleep guesses the best delay function based on duration.

Link: https://www.kernel.org/doc/html/latest/timers/timers-howto.html

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 3c38bae..9eb6808 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -69,7 +69,7 @@ static void activate_col(struct matrix_keypad *keypad, int col, bool on)
 	__activate_col(keypad, col, on);
 
 	if (on && keypad->col_scan_delay_us)
-		udelay(keypad->col_scan_delay_us);
+		fsleep(keypad->col_scan_delay_us);
 }
 
 static void activate_all_cols(struct matrix_keypad *keypad, bool on)
-- 
2.39.5

Re: [PATCH v2 1/7] Input: matrix_keypad - use fsleep for variable delay duration

[Dmitry Torokhov]

On Tue, Nov 05, 2024 at 02:03:16PM +0100, Markus Burri wrote:
> The delay is retrieved from a device-tree property, so the duration is
> variable. fsleep guesses the best delay function based on duration.

activate_col() may be called in atomic context, and if fsleep() turns
into usleep_range() or msleep() we are going to have a bad time.

We should either stop using request_any_context_irq() or figure out if
interrupt handler can sleep or not and adjust behavior properly.

> 
> Link: https://www.kernel.org/doc/html/latest/timers/timers-howto.html
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
>  drivers/input/keyboard/matrix_keypad.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index 3c38bae..9eb6808 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -69,7 +69,7 @@ static void activate_col(struct matrix_keypad *keypad, int col, bool on)
>  	__activate_col(keypad, col, on);
>  
>  	if (on && keypad->col_scan_delay_us)
> -		udelay(keypad->col_scan_delay_us);
> +		fsleep(keypad->col_scan_delay_us);
>  }
>  
>  static void activate_all_cols(struct matrix_keypad *keypad, bool on)
> -- 
> 2.39.5
> 

Thanks.

-- 
Dmitry

[PATCH v2 2/7] Input: matrix_keypad - add function for reading row state

[Markus Burri]

Move the evaluation of a row state into separate function.
It will be also used by a change later in this series.

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 9eb6808..2818915 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -101,6 +101,16 @@ static void disable_row_irqs(struct matrix_keypad *keypad)
 		disable_irq_nosync(keypad->row_irqs[i]);
 }
 
+static uint32_t read_row_state(struct matrix_keypad *keypad)
+{
+	int row;
+	u32 row_state = 0;
+
+	for (row = 0; row < keypad->num_row_gpios; row++)
+		row_state |= row_asserted(keypad, row) ? BIT(row) : 0;
+	return row_state;
+}
+
 /*
  * This gets the keys from keyboard and reports it to input subsystem
  */
@@ -126,9 +136,7 @@ static void matrix_keypad_scan(struct work_struct *work)
 
 		activate_col(keypad, col, true);
 
-		for (row = 0; row < keypad->num_row_gpios; row++)
-			new_state[col] |=
-				row_asserted(keypad, row) ? BIT(row) : 0;
+		new_state[col] = read_row_state(keypad);
 
 		activate_col(keypad, col, false);
 	}
-- 
2.39.5

[PATCH v2 3/7] dt-bindings: input: matrix_keypad - convert to YAML

[Markus Burri]

Convert the gpio-matrix-keypad bindings from text to DT schema.

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 .../bindings/input/gpio-matrix-keypad.txt     | 49 ---------
 .../bindings/input/gpio-matrix-keypad.yaml    | 99 +++++++++++++++++++
 2 files changed, 99 insertions(+), 49 deletions(-)
 delete mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
 create mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml

diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
deleted file mode 100644
index 570dc10..0000000
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
+++ /dev/null
@@ -1,49 +0,0 @@
-* GPIO driven matrix keypad device tree bindings
-
-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.
-
-Required Properties:
-- compatible:		Should be "gpio-matrix-keypad"
-- row-gpios:		List of gpios used as row lines. The gpio specifier
-			for this property depends on the gpio controller to
-			which these row lines are connected.
-- col-gpios:		List of gpios used as column lines. The gpio specifier
-			for this property depends on the gpio controller to
-			which these column lines are connected.
-- linux,keymap:		The definition can be found at
-			bindings/input/matrix-keymap.txt
-
-Optional Properties:
-- linux,no-autorepeat:	do no enable autorepeat feature.
-- wakeup-source:	use any event on keypad as wakeup event.
-			(Legacy property supported: "linux,wakeup")
-- 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
-- drive-inactive-cols:	drive inactive columns during scan,
-			default is to turn inactive columns into inputs.
-
-Example:
-	matrix-keypad {
-		compatible = "gpio-matrix-keypad";
-		debounce-delay-ms = <5>;
-		col-scan-delay-us = <2>;
-
-		row-gpios = <&gpio2 25 0
-			     &gpio2 26 0
-			     &gpio2 27 0>;
-
-		col-gpios = <&gpio2 21 0
-			     &gpio2 22 0>;
-
-		linux,keymap = <0x0000008B
-				0x0100009E
-				0x02000069
-				0x0001006A
-				0x0101001C
-				0x0201006C>;
-	};
diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
new file mode 100644
index 0000000..745652b
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -0,0 +1,99 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+
+$id: http://devicetree.org/schemas/input/gpio-matrix-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: GPIO matrix keypad
+
+description: |
+  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.
+
+maintainers:
+  - Marek Vasut <marek.vasut@gmail.com>
+
+properties:
+  compatible:
+    const: gpio-matrix-keypad
+
+  row-gpios:
+    description: |
+      List of GPIOs used as row lines. The gpio specifier
+      for this property depends on the gpio controller to
+      which these row lines are connected.
+
+  col-gpios:
+    description: |
+      List of GPIOs used as column lines. The gpio specifier
+      for this property depends on the gpio controller to
+      which these column lines are connected.
+
+  linux,keymap:
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    description: |
+      An array of packed 1-cell entries containing the equivalent of row,
+      column and linux key-code. The 32-bit big endian cell is packed as:
+          row << 24 | column << 16 | key-code
+
+  linux,no-autorepeat:
+    type: boolean
+    description: Do not enable autorepeat feature.
+    default: false
+
+  wakeup-source:
+    description: |
+      Use any event on keypad as wakeup event.
+      (Legacy property supported: "linux,wakeup")
+    default: false
+
+  debounce-delay-ms:
+    description: Debounce interval in milliseconds.
+    default: 0
+
+  col-scan-delay-us:
+    description: |
+      Delay, measured in microseconds, that is needed
+      before we can scan keypad after activating column gpio.
+    default: 0
+
+  drive-inactive-cols:
+    type: boolean
+    description: |
+      Drive inactive columns during scan,
+      default is to turn inactive columns into inputs.
+    default: false
+
+required:
+  - compatible
+  - row-gpios
+  - col-gpios
+  - linux,keymap
+
+additionalProperties: false
+
+examples:
+  - |
+    matrix-keypad {
+        compatible = "gpio-matrix-keypad";
+        debounce-delay-ms = <5>;
+        col-scan-delay-us = <2>;
+
+        row-gpios = <&gpio2 25 0
+                     &gpio2 26 0
+                     &gpio2 27 0>;
+
+        col-gpios = <&gpio2 21 0
+                     &gpio2 22 0>;
+
+        linux,keymap = <0x0000008B
+                        0x0100009E
+                        0x02000069
+                        0x0001006A
+                        0x0101001C
+                        0x0201006C>;
+    };
-- 
2.39.5

Re: [PATCH v2 3/7] dt-bindings: input: matrix_keypad - convert to YAML

[kernel test robot]

Hi Markus,

kernel test robot noticed the following build warnings:

[auto build test WARNING on dtor-input/for-linus]
[also build test WARNING on robh/for-next linus/master v6.12-rc6 next-20241105]
[cannot apply to dtor-input/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Burri/Input-matrix_keypad-use-fsleep-for-variable-delay-duration/20241105-211348
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
patch link:    https://lore.kernel.org/r/20241105130322.213623-4-markus.burri%40mt.com
patch subject: [PATCH v2 3/7] dt-bindings: input: matrix_keypad - convert to YAML
reproduce: (https://download.01.org/0day-ci/archive/20241106/202411060143.8tNHeidk-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202411060143.8tNHeidk-lkp@intel.com/

All warnings (new ones prefixed by >>):

>> Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
   Warning: Documentation/devicetree/bindings/power/wakeup-source.txt references a file that doesn't exist: Documentation/devicetree/bindings/input/qcom,pm8xxx-keypad.txt
   Warning: Documentation/devicetree/bindings/regulator/siliconmitus,sm5703-regulator.yaml references a file that doesn't exist: Documentation/devicetree/bindings/mfd/siliconmitus,sm5703.yaml
   Warning: Documentation/hwmon/g762.rst references a file that doesn't exist: Documentation/devicetree/bindings/hwmon/g762.txt
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/reserved-memory/qcom
   Warning: MAINTAINERS references a file that doesn't exist: Documentation/devicetree/bindings/display/exynos/

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2 3/7] dt-bindings: input: matrix_keypad - convert to YAML

[Rob Herring]

On Tue, Nov 05, 2024 at 02:03:18PM +0100, Markus Burri wrote:
> Convert the gpio-matrix-keypad bindings from text to DT schema.

Please don't thread new versions to previous thread.

> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
>  .../bindings/input/gpio-matrix-keypad.txt     | 49 ---------
>  .../bindings/input/gpio-matrix-keypad.yaml    | 99 +++++++++++++++++++
>  2 files changed, 99 insertions(+), 49 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
>  create mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> deleted file mode 100644
> index 570dc10..0000000
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> +++ /dev/null
> @@ -1,49 +0,0 @@
> -* GPIO driven matrix keypad device tree bindings
> -
> -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.
> -
> -Required Properties:
> -- compatible:		Should be "gpio-matrix-keypad"
> -- row-gpios:		List of gpios used as row lines. The gpio specifier
> -			for this property depends on the gpio controller to
> -			which these row lines are connected.
> -- col-gpios:		List of gpios used as column lines. The gpio specifier
> -			for this property depends on the gpio controller to
> -			which these column lines are connected.
> -- linux,keymap:		The definition can be found at
> -			bindings/input/matrix-keymap.txt
> -
> -Optional Properties:
> -- linux,no-autorepeat:	do no enable autorepeat feature.
> -- wakeup-source:	use any event on keypad as wakeup event.
> -			(Legacy property supported: "linux,wakeup")
> -- 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
> -- drive-inactive-cols:	drive inactive columns during scan,
> -			default is to turn inactive columns into inputs.
> -
> -Example:
> -	matrix-keypad {
> -		compatible = "gpio-matrix-keypad";
> -		debounce-delay-ms = <5>;
> -		col-scan-delay-us = <2>;
> -
> -		row-gpios = <&gpio2 25 0
> -			     &gpio2 26 0
> -			     &gpio2 27 0>;
> -
> -		col-gpios = <&gpio2 21 0
> -			     &gpio2 22 0>;
> -
> -		linux,keymap = <0x0000008B
> -				0x0100009E
> -				0x02000069
> -				0x0001006A
> -				0x0101001C
> -				0x0201006C>;
> -	};
> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> new file mode 100644
> index 0000000..745652b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -0,0 +1,99 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +
> +$id: http://devicetree.org/schemas/input/gpio-matrix-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: GPIO matrix keypad
> +
> +description: |

Don't need '|' if no formatting.

> +  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.
> +
> +maintainers:
> +  - Marek Vasut <marek.vasut@gmail.com>
> +
> +properties:
> +  compatible:
> +    const: gpio-matrix-keypad
> +
> +  row-gpios:
> +    description: |

Ditto.

> +      List of GPIOs used as row lines. The gpio specifier
> +      for this property depends on the gpio controller to
> +      which these row lines are connected.

Why the odd line length? Wrap at 80 char unless there's some exceptional 
reason to go to 100.

> +
> +  col-gpios:
> +    description: |
> +      List of GPIOs used as column lines. The gpio specifier
> +      for this property depends on the gpio controller to
> +      which these column lines are connected.

Is there some reasonable range of GPIOs? 1xN is in theory possible, but 
surely no one would use N+1 GPIOs for what could be done with N GPIOs? 
So 'minItems: 2'. A max is not so clear. Eventually, I guess you 
couldn't scan them fast enough. 64 or 128 seems like way more than 
anyone would do.

> +
> +  linux,keymap:
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    description: |
> +      An array of packed 1-cell entries containing the equivalent of row,
> +      column and linux key-code. The 32-bit big endian cell is packed as:
> +          row << 24 | column << 16 | key-code

If you are copying another schema, you are doing it wrong...

You need a $ref to matrix-keymap.yaml.

> +
> +  linux,no-autorepeat:
> +    type: boolean
> +    description: Do not enable autorepeat feature.
> +    default: false

Drop 'default'. Doesn't apply to booleans.

> +
> +  wakeup-source:
> +    description: |
> +      Use any event on keypad as wakeup event.
> +      (Legacy property supported: "linux,wakeup")

Not supported if it is not in the schema. I imagine that's been 
deprecated long enough, you can just drop mentioning it.

> +    default: false

Drop.

> +
> +  debounce-delay-ms:
> +    description: Debounce interval in milliseconds.
> +    default: 0
> +
> +  col-scan-delay-us:
> +    description: |
> +      Delay, measured in microseconds, that is needed
> +      before we can scan keypad after activating column gpio.
> +    default: 0
> +
> +  drive-inactive-cols:
> +    type: boolean
> +    description: |
> +      Drive inactive columns during scan,
> +      default is to turn inactive columns into inputs.
> +    default: false

Drop

> +
> +required:
> +  - compatible
> +  - row-gpios
> +  - col-gpios
> +  - linux,keymap
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    matrix-keypad {
> +        compatible = "gpio-matrix-keypad";
> +        debounce-delay-ms = <5>;
> +        col-scan-delay-us = <2>;
> +
> +        row-gpios = <&gpio2 25 0
> +                     &gpio2 26 0
> +                     &gpio2 27 0>;
> +
> +        col-gpios = <&gpio2 21 0
> +                     &gpio2 22 0>;
> +
> +        linux,keymap = <0x0000008B
> +                        0x0100009E
> +                        0x02000069
> +                        0x0001006A
> +                        0x0101001C
> +                        0x0201006C>;
> +    };
> -- 
> 2.39.5
> 

[PATCH v2 4/7] dt-bindings: input: matrix_keypad - add missing property

[Markus Burri]

Add missing property 'gpio-activelow' to DT schema.

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 .../devicetree/bindings/input/gpio-matrix-keypad.yaml        | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
index 745652b..9ea66b3 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -51,6 +51,11 @@ properties:
       (Legacy property supported: "linux,wakeup")
     default: false
 
+  gpio-activelow:
+    type: boolean
+    description: The GPIOs are low active.
+    default: false
+
   debounce-delay-ms:
     description: Debounce interval in milliseconds.
     default: 0
-- 
2.39.5

Re: [PATCH v2 4/7] dt-bindings: input: matrix_keypad - add missing property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

On Tue, Nov 05, 2024 at 02:03:19PM +0100, Markus Burri wrote:
> Add missing property 'gpio-activelow' to DT schema.

What do you mean "missing property"? Why isn't it sufficient to mark the
GPIOs as GPIO_ACTIVE_LOW in the various -gpios properties?

> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
>  .../devicetree/bindings/input/gpio-matrix-keypad.yaml        | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> index 745652b..9ea66b3 100644
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -51,6 +51,11 @@ properties:
>        (Legacy property supported: "linux,wakeup")
>      default: false
>  
> +  gpio-activelow:
> +    type: boolean
> +    description: The GPIOs are low active.
> +    default: false

What you want is a flag, not a boolean here btw. Flags you can check for
the presence of, booleans you cannot.

> +
>    debounce-delay-ms:
>      description: Debounce interval in milliseconds.
>      default: 0
> -- 
> 2.39.5
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v2 4/7] dt-bindings: input: matrix_keypad - add missing property

[Dmitry Torokhov]

Hi Conor,

On Tue, Nov 05, 2024 at 06:22:36PM +0000, Conor Dooley wrote:
> On Tue, Nov 05, 2024 at 02:03:19PM +0100, Markus Burri wrote:
> > Add missing property 'gpio-activelow' to DT schema.
> 
> What do you mean "missing property"? Why isn't it sufficient to mark the
> GPIOs as GPIO_ACTIVE_LOW in the various -gpios properties?

Unfortunately we do have "gpio-activelow" property already used in the
driver since 2012 when DT support was added to the driver. This patch
merely acknowledges that it is there.

If DT maintainers wish to adjust known DTSes and switch to proper
polarity annotation through gpio property I am all for it.

> 
> > 
> > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > ---
> >  .../devicetree/bindings/input/gpio-matrix-keypad.yaml        | 5 +++++
> >  1 file changed, 5 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > index 745652b..9ea66b3 100644
> > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > @@ -51,6 +51,11 @@ properties:
> >        (Legacy property supported: "linux,wakeup")
> >      default: false
> >  
> > +  gpio-activelow:
> > +    type: boolean
> > +    description: The GPIOs are low active.
> > +    default: false
> 
> What you want is a flag, not a boolean here btw. Flags you can check for
> the presence of, booleans you cannot.

The behavior is fixed. If the flag is true GPIO is assumed to be active
low, otherwise (and in the absence of the flag) GPIO is assumed to be
active high.

> 
> > +
> >    debounce-delay-ms:
> >      description: Debounce interval in milliseconds.
> >      default: 0
> > -- 
> > 2.39.5
> > 

Thanks.

-- 
Dmitry

Re: [PATCH v2 4/7] dt-bindings: input: matrix_keypad - add missing property

[Conor Dooley]

[-- Attachment #1: Type: text/plain, Size: 2334 bytes --]

On Tue, Nov 05, 2024 at 11:14:32AM -0800, Dmitry Torokhov wrote:
> Hi Conor,
> 
> On Tue, Nov 05, 2024 at 06:22:36PM +0000, Conor Dooley wrote:
> > On Tue, Nov 05, 2024 at 02:03:19PM +0100, Markus Burri wrote:
> > > Add missing property 'gpio-activelow' to DT schema.
> > 
> > What do you mean "missing property"? Why isn't it sufficient to mark the
> > GPIOs as GPIO_ACTIVE_LOW in the various -gpios properties?
> 
> Unfortunately we do have "gpio-activelow" property already used in the
> driver since 2012 when DT support was added to the driver. This patch
> merely acknowledges that it is there.
> 
> If DT maintainers wish to adjust known DTSes and switch to proper
> polarity annotation through gpio property I am all for it.

If that's the rationale, it should be in the commit message. It's not my
job to figure out people's intentions :)

> > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > > ---
> > >  .../devicetree/bindings/input/gpio-matrix-keypad.yaml        | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > index 745652b..9ea66b3 100644
> > > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > @@ -51,6 +51,11 @@ properties:
> > >        (Legacy property supported: "linux,wakeup")
> > >      default: false
> > >  
> > > +  gpio-activelow:
> > > +    type: boolean
> > > +    description: The GPIOs are low active.
> > > +    default: false
> > 
> > What you want is a flag, not a boolean here btw. Flags you can check for
> > the presence of, booleans you cannot.
> 
> The behavior is fixed.

What do you mean by "fixed"? Corrected, unchangeable or something else?

> If the flag is true GPIO is assumed to be active
> low, otherwise (and in the absence of the flag) GPIO is assumed to be
> active high.

Flags cannot be false only true or absent, what you have described here
is a boolean.

> 
> > 
> > > +
> > >    debounce-delay-ms:
> > >      description: Debounce interval in milliseconds.
> > >      default: 0
> > > -- 
> > > 2.39.5
> > > 
> 
> Thanks.
> 
> -- 
> Dmitry

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

[PATCH v2 5/7] dt-bindings: input: matrix_keypad - add settle time after enable all columns

[Markus Burri]

matrix_keypad with high capacity need a longer settle time after enable
all columns.
Add optional property to specify the settle time

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 .../devicetree/bindings/input/gpio-matrix-keypad.yaml       | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
index 9ea66b3..0ce89a6 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -66,6 +66,12 @@ properties:
       before we can scan keypad after activating column gpio.
     default: 0
 
+  all-cols-on-delay-us:
+    description: |
+      Delay, measured in microseconds, that is needed
+      after activating all column gpios.
+    default: 0
+
   drive-inactive-cols:
     type: boolean
     description: |
-- 
2.39.5

Re: [PATCH v2 5/7] dt-bindings: input: matrix_keypad - add settle time after enable all columns

[Rob Herring]

On Tue, Nov 05, 2024 at 02:03:20PM +0100, Markus Burri wrote:
> matrix_keypad with high capacity need a longer settle time after enable
> all columns.
> Add optional property to specify the settle time
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
>  .../devicetree/bindings/input/gpio-matrix-keypad.yaml       | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> index 9ea66b3..0ce89a6 100644
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -66,6 +66,12 @@ properties:
>        before we can scan keypad after activating column gpio.
>      default: 0
>  
> +  all-cols-on-delay-us:
> +    description: |

Don't need '|'.

> +      Delay, measured in microseconds, that is needed
> +      after activating all column gpios.

Wrap at 80 char.

> +    default: 0
> +
>    drive-inactive-cols:
>      type: boolean
>      description: |
> -- 
> 2.39.5
> 

[PATCH v2 6/7] Input: matrix_keypad - add settle time after enable all columns

[Markus Burri]

Matrix_keypad with high capacity need a longer settle time after enable
all columns and re-enabling interrupts.
This to give time stable the system and not generate interrupts.

Add a new optional device-tree property to configure the time before
enabling interrupts after disable all columns.
The default is no delay.

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 2818915..46873b8 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -27,6 +27,7 @@ struct matrix_keypad {
 	unsigned int row_shift;
 
 	unsigned int col_scan_delay_us;
+	unsigned int all_cols_on_delay_us;
 	/* key debounce interval in milli-second */
 	unsigned int debounce_ms;
 	bool drive_inactive_cols;
@@ -78,6 +79,9 @@ static void activate_all_cols(struct matrix_keypad *keypad, bool on)
 
 	for (col = 0; col < keypad->num_col_gpios; col++)
 		__activate_col(keypad, col, on);
+
+	if (on && keypad->all_cols_on_delay_us)
+		fsleep(keypad->all_cols_on_delay_us);
 }
 
 static bool row_asserted(struct matrix_keypad *keypad, int row)
@@ -403,6 +407,8 @@ static int matrix_keypad_probe(struct platform_device *pdev)
 				 &keypad->debounce_ms);
 	device_property_read_u32(&pdev->dev, "col-scan-delay-us",
 				 &keypad->col_scan_delay_us);
+	device_property_read_u32(&pdev->dev, "all-cols-on-delay-us",
+				 &keypad->all_cols_on_delay_us);
 
 	err = matrix_keypad_init_gpio(pdev, keypad);
 	if (err)
-- 
2.39.5

[PATCH v2 7/7] Input: matrix_keypad - detect change during scan

[Markus Burri]

For a setup where the matrix keypad is connected over a slow interface
(e.g. a gpio-expansion over i2c), the scan can take a longer time to read.

Interrupts need to be disabled during scan. And therefore changes in this
period are not detected.
To improve this situation, scan the matrix again if the row state changed
during interrupts disabled.

Signed-off-by: Markus Burri <markus.burri@mt.com>
---
 drivers/input/keyboard/matrix_keypad.c | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index 46873b8..93c8404 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -126,6 +126,10 @@ static void matrix_keypad_scan(struct work_struct *work)
 	const unsigned short *keycodes = input_dev->keycode;
 	uint32_t new_state[MATRIX_MAX_COLS];
 	int row, col, code;
+	u32 init_row_state, new_row_state;
+
+	/* read initial row state to detect changes between scan */
+	init_row_state = read_row_state(keypad);
 
 	/* de-activate all columns for scanning */
 	activate_all_cols(keypad, false);
@@ -174,6 +178,19 @@ static void matrix_keypad_scan(struct work_struct *work)
 	keypad->scan_pending = false;
 	enable_row_irqs(keypad);
 	spin_unlock_irq(&keypad->lock);
+
+	/* read new row state and detect if value has changed */
+	new_row_state = read_row_state(keypad);
+	if (init_row_state != new_row_state) {
+		spin_lock_irq(&keypad->lock);
+		if (likely(!keypad->scan_pending && !keypad->stopped)) {
+			disable_row_irqs(keypad);
+			keypad->scan_pending = true;
+			schedule_delayed_work(&keypad->work,
+					      msecs_to_jiffies(keypad->debounce_ms));
+		}
+		spin_unlock_irq(&keypad->lock);
+	}
 }
 
 static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
-- 
2.39.5

Re: [PATCH v2 7/7] Input: matrix_keypad - detect change during scan

[Dmitry Torokhov]

On Tue, Nov 05, 2024 at 02:03:22PM +0100, Markus Burri wrote:
> For a setup where the matrix keypad is connected over a slow interface
> (e.g. a gpio-expansion over i2c), the scan can take a longer time to read.
> 
> Interrupts need to be disabled during scan. And therefore changes in this
> period are not detected.
> To improve this situation, scan the matrix again if the row state changed
> during interrupts disabled.

And what if the state changes yet again during the 2nd scan attempt?

> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> ---
>  drivers/input/keyboard/matrix_keypad.c | 17 +++++++++++++++++
>  1 file changed, 17 insertions(+)
> 
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index 46873b8..93c8404 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -126,6 +126,10 @@ static void matrix_keypad_scan(struct work_struct *work)
>  	const unsigned short *keycodes = input_dev->keycode;
>  	uint32_t new_state[MATRIX_MAX_COLS];
>  	int row, col, code;
> +	u32 init_row_state, new_row_state;
> +
> +	/* read initial row state to detect changes between scan */
> +	init_row_state = read_row_state(keypad);
>  
>  	/* de-activate all columns for scanning */
>  	activate_all_cols(keypad, false);
> @@ -174,6 +178,19 @@ static void matrix_keypad_scan(struct work_struct *work)
>  	keypad->scan_pending = false;
>  	enable_row_irqs(keypad);
>  	spin_unlock_irq(&keypad->lock);
> +
> +	/* read new row state and detect if value has changed */
> +	new_row_state = read_row_state(keypad);
> +	if (init_row_state != new_row_state) {
> +		spin_lock_irq(&keypad->lock);
> +		if (likely(!keypad->scan_pending && !keypad->stopped)) {
> +			disable_row_irqs(keypad);
> +			keypad->scan_pending = true;
> +			schedule_delayed_work(&keypad->work,
> +					      msecs_to_jiffies(keypad->debounce_ms));
> +		}
> +		spin_unlock_irq(&keypad->lock);
> +	}
>  }
>  
>  static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
> -- 
> 2.39.5
> 

-- 
Dmitry

Re: [PATCH v2 4/7] dt-bindings: input: matrix_keypad - add missing property

[kernel test robot]

:::::: 
:::::: Manual check reason: "dtcheck: binding changes may go via different trees"
:::::: 

BCC: lkp@intel.com
CC: oe-kbuild-all@lists.linux.dev
In-Reply-To: <20241105130322.213623-5-markus.burri@mt.com>
References: <20241105130322.213623-5-markus.burri@mt.com>
TO: Markus Burri <markus.burri@mt.com>
TO: linux-kernel@vger.kernel.org
CC: Markus Burri <markus.burri@mt.com>
CC: Dmitry Torokhov <dmitry.torokhov@gmail.com>
CC: Rob Herring <robh@kernel.org>
CC: Krzysztof Kozlowski <krzk@kernel.org>
CC: Conor Dooley <conor+dt@kernel.org>
CC: Marek Vasut <marek.vasut@gmail.com>
CC: linux-input@vger.kernel.org
CC: devicetree@vger.kernel.org

Hi Markus,

kernel test robot noticed the following build warnings:

[auto build test WARNING on dtor-input/for-linus]
[also build test WARNING on robh/for-next linus/master v6.12-rc6 next-20241105]
[cannot apply to dtor-input/next]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Markus-Burri/Input-matrix_keypad-use-fsleep-for-variable-delay-duration/20241105-211348
base:   https://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git for-linus
patch link:    https://lore.kernel.org/r/20241105130322.213623-5-markus.burri%40mt.com
patch subject: [PATCH v2 4/7] dt-bindings: input: matrix_keypad - add missing property
:::::: branch date: 9 hours ago
:::::: commit date: 9 hours ago
config: powerpc-randconfig-051-20241106 (https://download.01.org/0day-ci/archive/20241106/202411060511.cI4VWUr3-lkp@intel.com/config)
compiler: powerpc-linux-gcc (GCC) 14.1.0
dtschema version: 2024.10.dev6+g12c3cd5
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20241106/202411060511.cI4VWUr3-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/r/202411060511.cI4VWUr3-lkp@intel.com/

dtcheck warnings: (new ones prefixed by >>)
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/psc@11700: failed to match any schema with compatible: ['fsl,mpc5121-psc-uart', 'fsl,mpc5121-psc']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/psc@11700: failed to match any schema with compatible: ['fsl,mpc5121-psc-uart', 'fsl,mpc5121-psc']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/psc@11800: failed to match any schema with compatible: ['fsl,mpc5121-psc']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/psc@11900: failed to match any schema with compatible: ['fsl,mpc5121-psc']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/psc@11a00: failed to match any schema with compatible: ['fsl,mpc5121-psc']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/psc@11b00: failed to match any schema with compatible: ['fsl,mpc5121-psc']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/pscfifo@11f00: failed to match any schema with compatible: ['fsl,mpc5121-psc-fifo']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/dma@14000: failed to match any schema with compatible: ['fsl,mpc5121-dma']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/axe-base@2000: failed to match any schema with compatible: ['fsl,mpc5121-axe-base']
   arch/powerpc/boot/dts/ac14xx.dtb: /soc@80000000/axe-app: failed to match any schema with compatible: ['fsl,mpc5121-axe-app']
>> arch/powerpc/boot/dts/ac14xx.dtb: matrix_keypad@0: 'col-gpios-binary', 'col-switch-delay-ms' do not match any of the regexes: 'pinctrl-[0-9]+'
   	from schema $id: http://devicetree.org/schemas/input/gpio-matrix-keypad.yaml#
   arch/powerpc/boot/dts/ac14xx.dtb: /pci@80008500: failed to match any schema with compatible: ['fsl,mpc5121-pci']
   arch/powerpc/boot/dts/ac14xx.dtb: leds: 'backlight', 'green', 'red' do not match any of the regexes: '(^led-[0-9a-f]$|led)', 'pinctrl-[0-9]+'
   	from schema $id: http://devicetree.org/schemas/leds/leds-gpio.yaml#

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki