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

[PATCH v5 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 additional latencies between key change and
matrix scan.

This series applies on 6.13-rc6

Changes in V5:
* formatting and typo

Changes in V4:
* deprecate gpio-activelow in gpio-matrix-keypad.yaml
* change reference to linux,keymap in gpio-matrix-keypad.yaml

Changes in V3:
* fix 'references a file that doesn't exist' in wakeup-source.txt
* remove copied and deprecated properties from gpio-matrix-keypad.yaml
* extend patch description to clarify the changes
* rebase to 6.13-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

[V4] https://lore.kernel.org/lkml/20250108134507.257268-1-markus.burri@mt.com/
[V3] https://lore.kernel.org/lkml/20250107135659.185293-1-markus.burri@mt.com/
[V2] https://lore.kernel.org/lkml/20241105130322.213623-1-markus.burri@mt.com/
[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    | 100 ++++++++++++++++++
 .../bindings/power/wakeup-source.txt          |   2 +-
 drivers/input/keyboard/matrix_keypad.c        |  38 ++++++-
 4 files changed, 135 insertions(+), 54 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 v5 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.

see Documentation/timers/delay_sleep_functions.rst

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 2a3b3bf..5571d2e 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -68,7 +68,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 v5 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 5571d2e..90148d3 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -100,6 +100,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
  */
@@ -125,9 +135,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 v5 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    | 88 +++++++++++++++++++
 .../bindings/power/wakeup-source.txt          |  2 +-
 3 files changed, 89 insertions(+), 50 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..0f348b9
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -0,0 +1,88 @@
+# 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
+
+maintainers:
+  - Marek Vasut <marek.vasut@gmail.com>
+
+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.
+
+allOf:
+  - $ref: /schemas/input/matrix-keymap.yaml#
+
+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: true
+
+  linux,no-autorepeat:
+    type: boolean
+    description: Do not enable autorepeat feature.
+
+
+  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.
+
+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>;
+    };
diff --git a/Documentation/devicetree/bindings/power/wakeup-source.txt b/Documentation/devicetree/bindings/power/wakeup-source.txt
index 27f1797..66bb016 100644
--- a/Documentation/devicetree/bindings/power/wakeup-source.txt
+++ b/Documentation/devicetree/bindings/power/wakeup-source.txt
@@ -23,7 +23,7 @@ List of legacy properties and respective binding document
 
 1. "gpio-key,wakeup"		Documentation/devicetree/bindings/input/gpio-keys{,-polled}.txt
 2. "has-tpo"			Documentation/devicetree/bindings/rtc/rtc-opal.txt
-3. "linux,wakeup"		Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
+3. "linux,wakeup"		Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
 				Documentation/devicetree/bindings/mfd/tc3589x.txt
 				Documentation/devicetree/bindings/input/touchscreen/ti,ads7843.yaml
 4. "linux,keypad-wakeup"	Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
-- 
2.39.5

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

[Markus Burri]

The property is implemented in the driver but not described in dt-bindings.
Add missing property 'gpio-activelow' to DT schema.

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 0f348b9..9c91224 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -40,6 +40,12 @@ properties:
     type: boolean
     description: Do not enable autorepeat feature.
 
+  gpio-activelow:
+    type: boolean
+    deprecated: true
+    description:
+      The GPIOs are low active (deprecated).
+      Use the GPIO-flags in gpio controller instead of this property.
 
   debounce-delay-ms:
     description: Debounce interval in milliseconds.
-- 
2.39.5

[PATCH v5 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>
Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

---
 .../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 9c91224..dc08d39 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -57,6 +57,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

[PATCH v5 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 90148d3..fdb3499 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -26,6 +26,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;
@@ -77,6 +78,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)
@@ -400,6 +404,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 v5 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.
The rescan is repeated until no change is detected anymore.

Signed-off-by: Markus Burri <markus.burri@mt.com>

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

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index fdb3499..e50a6fe 100644
--- a/drivers/input/keyboard/matrix_keypad.c
+++ b/drivers/input/keyboard/matrix_keypad.c
@@ -125,6 +125,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);
@@ -173,6 +177,18 @@ static void matrix_keypad_scan(struct work_struct *work)
 		keypad->scan_pending = false;
 		enable_row_irqs(keypad);
 	}
+
+	/* read new row state and detect if value has changed */
+	new_row_state = read_row_state(keypad);
+	if (init_row_state != new_row_state) {
+		guard(spinlock_irq)(&keypad->lock);
+		if (unlikely(keypad->scan_pending || keypad->stopped))
+			return;
+		disable_row_irqs(keypad);
+		keypad->scan_pending = true;
+		schedule_delayed_work(&keypad->work,
+				      msecs_to_jiffies(keypad->debounce_ms));
+	}
 }
 
 static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
-- 
2.39.5

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

[Rob Herring (Arm)]

On Fri, 10 Jan 2025 06:49:02 +0100, Markus Burri wrote:
> 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    | 88 +++++++++++++++++++
>  .../bindings/power/wakeup-source.txt          |  2 +-
>  3 files changed, 89 insertions(+), 50 deletions(-)
>  delete mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
>  create mode 100644 Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

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

[Rob Herring (Arm)]

On Fri, 10 Jan 2025 06:49:03 +0100, Markus Burri wrote:
> The property is implemented in the driver but not described in dt-bindings.
> Add missing property 'gpio-activelow' to DT schema.
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> 
> ---
>  .../devicetree/bindings/input/gpio-matrix-keypad.yaml       | 6 ++++++
>  1 file changed, 6 insertions(+)
> 

Reviewed-by: Rob Herring (Arm) <robh@kernel.org>

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

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:00AM +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.
> 
> see Documentation/timers/delay_sleep_functions.rst
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>

Reviewed-by: Manuel Traut <manuel.traut@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 2a3b3bf..5571d2e 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -68,7 +68,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 v5 2/7] Input: matrix_keypad - add function for reading row state

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:01AM +0100, Markus Burri wrote:
> 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>

> 
> ---
>  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 5571d2e..90148d3 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -100,6 +100,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;

uint32_t row_state = 0;

would look more consistent.

> +
> +	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
>   */
> @@ -125,9 +135,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
> 

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

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:02AM +0100, Markus Burri wrote:
> Convert the gpio-matrix-keypad bindings from text to DT schema.
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>

Reviewed-by: Manuel Traut <manuel.traut@mt.com>

> ---
>  .../bindings/input/gpio-matrix-keypad.txt     | 49 -----------
>  .../bindings/input/gpio-matrix-keypad.yaml    | 88 +++++++++++++++++++
>  .../bindings/power/wakeup-source.txt          |  2 +-
>  3 files changed, 89 insertions(+), 50 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..0f348b9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -0,0 +1,88 @@
> +# 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
> +
> +maintainers:
> +  - Marek Vasut <marek.vasut@gmail.com>
> +
> +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.
> +
> +allOf:
> +  - $ref: /schemas/input/matrix-keymap.yaml#
> +
> +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: true
> +
> +  linux,no-autorepeat:
> +    type: boolean
> +    description: Do not enable autorepeat feature.
> +
> +
> +  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.
> +
> +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>;
> +    };
> diff --git a/Documentation/devicetree/bindings/power/wakeup-source.txt b/Documentation/devicetree/bindings/power/wakeup-source.txt
> index 27f1797..66bb016 100644
> --- a/Documentation/devicetree/bindings/power/wakeup-source.txt
> +++ b/Documentation/devicetree/bindings/power/wakeup-source.txt
> @@ -23,7 +23,7 @@ List of legacy properties and respective binding document
>  
>  1. "gpio-key,wakeup"		Documentation/devicetree/bindings/input/gpio-keys{,-polled}.txt
>  2. "has-tpo"			Documentation/devicetree/bindings/rtc/rtc-opal.txt
> -3. "linux,wakeup"		Documentation/devicetree/bindings/input/gpio-matrix-keypad.txt
> +3. "linux,wakeup"		Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
>  				Documentation/devicetree/bindings/mfd/tc3589x.txt
>  				Documentation/devicetree/bindings/input/touchscreen/ti,ads7843.yaml
>  4. "linux,keypad-wakeup"	Documentation/devicetree/bindings/input/qcom,pm8921-keypad.yaml
> -- 
> 2.39.5
> 

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

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:03AM +0100, Markus Burri wrote:
> The property is implemented in the driver but not described in dt-bindings.
> Add missing property 'gpio-activelow' to DT schema.
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>

Reviewed-by: Manuel Traut <manuel.traut@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 0f348b9..9c91224 100644
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -40,6 +40,12 @@ properties:
>      type: boolean
>      description: Do not enable autorepeat feature.
>  
> +  gpio-activelow:
> +    type: boolean
> +    deprecated: true
> +    description:
> +      The GPIOs are low active (deprecated).
> +      Use the GPIO-flags in gpio controller instead of this property.
>  
>    debounce-delay-ms:
>      description: Debounce interval in milliseconds.
> -- 
> 2.39.5
> 

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

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:04AM +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: Rob Herring (Arm) <robh@kernel.org>

Reviewed-by: Manuel Traut <manuel.traut@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 9c91224..dc08d39 100644
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -57,6 +57,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 v5 6/7] Input: matrix_keypad - add settle time after enable all columns

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:05AM +0100, Markus Burri wrote:
> Matrix_keypad with high capacity need a longer settle time after enable

matrix keypads with a 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.
It avoids spurios interrupts by giving the system time to stable.
 
> Add a new optional device-tree property to configure the time before
> enabling interrupts after disable all columns.
> The default is no delay.

The delay for re-enabling the interrupts can be configured by a
device-tree property. The default is no delay.

> Signed-off-by: Markus Burri <markus.burri@mt.com>

Reviewed-by: Manuel Traut <manuel.traut@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 90148d3..fdb3499 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -26,6 +26,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;
> @@ -77,6 +78,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)
> @@ -400,6 +404,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
> 

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

[Manuel Traut]

On Fri, Jan 10, 2025 at 06:49:06AM +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.
> The rescan is repeated until no change is detected anymore.

This is a quirk for a bad hardware design. For 'good' hardware it adds
an additional read_row_state for no need. For even slower connected
GPIOs this will also not help much. However it is obvious that it will
be an improvement for some designs. 

Dmitry, would it make sense to make this configurable?

> Signed-off-by: Markus Burri <markus.burri@mt.com>
> 
> ---
>  drivers/input/keyboard/matrix_keypad.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
> index fdb3499..e50a6fe 100644
> --- a/drivers/input/keyboard/matrix_keypad.c
> +++ b/drivers/input/keyboard/matrix_keypad.c
> @@ -125,6 +125,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);
> @@ -173,6 +177,18 @@ static void matrix_keypad_scan(struct work_struct *work)
>  		keypad->scan_pending = false;
>  		enable_row_irqs(keypad);
>  	}
> +
> +	/* read new row state and detect if value has changed */
> +	new_row_state = read_row_state(keypad);
> +	if (init_row_state != new_row_state) {
> +		guard(spinlock_irq)(&keypad->lock);
> +		if (unlikely(keypad->scan_pending || keypad->stopped))
> +			return;
> +		disable_row_irqs(keypad);
> +		keypad->scan_pending = true;
> +		schedule_delayed_work(&keypad->work,
> +				      msecs_to_jiffies(keypad->debounce_ms));
> +	}
>  }
>  
>  static irqreturn_t matrix_keypad_interrupt(int irq, void *id)
> -- 
> 2.39.5
> 

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

[Dmitry Torokhov]

On Wed, Feb 19, 2025 at 05:34:49PM +0100, Manuel Traut wrote:
> On Fri, Jan 10, 2025 at 06:49:00AM +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.
> > 
> > see Documentation/timers/delay_sleep_functions.rst
> > 
> > Signed-off-by: Markus Burri <markus.burri@mt.com>
> 
> Reviewed-by: Manuel Traut <manuel.traut@mt.com> 

As I mentioned in other review activate_col() may be called in atomic
context where we can not sleep:

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

Unfortunately this was completely ignored.

> 
> > ---
> >  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 2a3b3bf..5571d2e 100644
> > --- a/drivers/input/keyboard/matrix_keypad.c
> > +++ b/drivers/input/keyboard/matrix_keypad.c
> > @@ -68,7 +68,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

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

[Dmitry Torokhov]

On Wed, Feb 19, 2025 at 05:42:00PM +0100, Manuel Traut wrote:
> On Fri, Jan 10, 2025 at 06:49:03AM +0100, Markus Burri wrote:
> > The property is implemented in the driver but not described in dt-bindings.
> > Add missing property 'gpio-activelow' to DT schema.
> > 
> > Signed-off-by: Markus Burri <markus.burri@mt.com>
> 
> Reviewed-by: Manuel Traut <manuel.traut@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 0f348b9..9c91224 100644
> > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > @@ -40,6 +40,12 @@ properties:
> >      type: boolean
> >      description: Do not enable autorepeat feature.
> >  
> > +  gpio-activelow:
> > +    type: boolean
> > +    deprecated: true
> > +    description:
> > +      The GPIOs are low active (deprecated).
> > +      Use the GPIO-flags in gpio controller instead of this property.

No, we unfortunately can not rely on GPIO-flags. This is not how driver
works: current driver behavior is to force GPIOs as active high if the
property is missing and ignore polarity specified in GPIO property.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Mon, Feb 24, 2025 at 10:40:55PM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 19, 2025 at 05:34:49PM +0100, Manuel Traut wrote:
> > On Fri, Jan 10, 2025 at 06:49:00AM +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.
> > > 
> > > see Documentation/timers/delay_sleep_functions.rst
> > > 
> > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > 
> > Reviewed-by: Manuel Traut <manuel.traut@mt.com> 
> 
> As I mentioned in other review activate_col() may be called in atomic
> context where we can not sleep:
> 
> "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."
> 
> Unfortunately this was completely ignored.

My apologies, it looks like it only is called from work handler, so my
comment was wrong.

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Wed, Feb 19, 2025 at 05:56:10PM +0100, Manuel Traut wrote:
> On Fri, Jan 10, 2025 at 06:49:06AM +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.
> > The rescan is repeated until no change is detected anymore.
> 
> This is a quirk for a bad hardware design. For 'good' hardware it adds
> an additional read_row_state for no need. For even slower connected
> GPIOs this will also not help much. However it is obvious that it will
> be an improvement for some designs. 
> 
> Dmitry, would it make sense to make this configurable?

What if we do not disable interrupts after the first one, but record
the last interrupt time and rescan if it arrived after work handler
started executing?

Thanks.

-- 
Dmitry

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

[Markus Burri]

On Mon, Feb 24, 2025 at 10:55:36PM -0800, Dmitry Torokhov wrote:
> On Mon, Feb 24, 2025 at 10:40:55PM -0800, Dmitry Torokhov wrote:
> > On Wed, Feb 19, 2025 at 05:34:49PM +0100, Manuel Traut wrote:
> > > On Fri, Jan 10, 2025 at 06:49:00AM +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.
> > > > 
> > > > see Documentation/timers/delay_sleep_functions.rst
> > > > 
> > > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > > 
> > > Reviewed-by: Manuel Traut <manuel.traut@mt.com> 
> > 
> > As I mentioned in other review activate_col() may be called in atomic
> > context where we can not sleep:
> > 
> > "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."
> > 
> > Unfortunately this was completely ignored.
> 
> My apologies, it looks like it only is called from work handler, so my
> comment was wrong.
> 
> Thanks.

Yes sorry to ignore, it is only called in work handler
> 
> -- 
> Dmitry

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

[Markus Burri]

On Mon, Feb 24, 2025 at 10:58:27PM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 19, 2025 at 05:56:10PM +0100, Manuel Traut wrote:
> > On Fri, Jan 10, 2025 at 06:49:06AM +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.
> > > The rescan is repeated until no change is detected anymore.
> > 
> > This is a quirk for a bad hardware design. For 'good' hardware it adds
> > an additional read_row_state for no need. For even slower connected
> > GPIOs this will also not help much. However it is obvious that it will
> > be an improvement for some designs. 
> > 
> > Dmitry, would it make sense to make this configurable?
> 
> What if we do not disable interrupts after the first one, but record
> the last interrupt time and rescan if it arrived after work handler
> started executing?
> 
> Thanks.

I was also thinking about that.
If we do not disable interrupts we will get a lot of interrupts during scan.
The scanning process itself generate interrupts because of selecting the columns
and read row state. Therefore after scan we will not know if the interrupts are
caused by scanning or a change.
  
> 
> -- 
> Dmitry

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

[Markus Burri]

On Mon, Feb 24, 2025 at 10:46:09PM -0800, Dmitry Torokhov wrote:
> On Wed, Feb 19, 2025 at 05:42:00PM +0100, Manuel Traut wrote:
> > On Fri, Jan 10, 2025 at 06:49:03AM +0100, Markus Burri wrote:
> > > The property is implemented in the driver but not described in dt-bindings.
> > > Add missing property 'gpio-activelow' to DT schema.
> > > 
> > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > 
> > Reviewed-by: Manuel Traut <manuel.traut@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 0f348b9..9c91224 100644
> > > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > @@ -40,6 +40,12 @@ properties:
> > >      type: boolean
> > >      description: Do not enable autorepeat feature.
> > >  
> > > +  gpio-activelow:
> > > +    type: boolean
> > > +    deprecated: true
> > > +    description:
> > > +      The GPIOs are low active (deprecated).
> > > +      Use the GPIO-flags in gpio controller instead of this property.
> 
> No, we unfortunately can not rely on GPIO-flags. This is not how driver
> works: current driver behavior is to force GPIOs as active high if the
> property is missing and ignore polarity specified in GPIO property.

So you mean this property is still valid?
Current implementation toggle the GPIO flags to low active if the property and the GPIO flags are different.
So in case of missing property (false) the settings from GPIO flags will be used.
In case of gpio-activelow (true) GPIO flags are set to low active.

So I will remove the deprecated flag and change the description to:
"Force GPIO flags to active low. Use GPIO flags if property is missing."
OK?

> 
> Thanks.
> 
> -- 
> Dmitry

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

[Dmitry Torokhov]

On Tue, Feb 25, 2025 at 11:16:07AM +0100, Markus Burri wrote:
> On Mon, Feb 24, 2025 at 10:46:09PM -0800, Dmitry Torokhov wrote:
> > On Wed, Feb 19, 2025 at 05:42:00PM +0100, Manuel Traut wrote:
> > > On Fri, Jan 10, 2025 at 06:49:03AM +0100, Markus Burri wrote:
> > > > The property is implemented in the driver but not described in dt-bindings.
> > > > Add missing property 'gpio-activelow' to DT schema.
> > > > 
> > > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > > 
> > > Reviewed-by: Manuel Traut <manuel.traut@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 0f348b9..9c91224 100644
> > > > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > > @@ -40,6 +40,12 @@ properties:
> > > >      type: boolean
> > > >      description: Do not enable autorepeat feature.
> > > >  
> > > > +  gpio-activelow:
> > > > +    type: boolean
> > > > +    deprecated: true
> > > > +    description:
> > > > +      The GPIOs are low active (deprecated).
> > > > +      Use the GPIO-flags in gpio controller instead of this property.
> > 
> > No, we unfortunately can not rely on GPIO-flags. This is not how driver
> > works: current driver behavior is to force GPIOs as active high if the
> > property is missing and ignore polarity specified in GPIO property.
> 
> So you mean this property is still valid?
> Current implementation toggle the GPIO flags to low active if the property and the GPIO flags are different.
> So in case of missing property (false) the settings from GPIO flags will be used.
> In case of gpio-activelow (true) GPIO flags are set to low active.

The driver does:

	active_low = device_property_read_bool(&pdev->dev, "gpio-activelow");

	...

		if (active_low ^ gpiod_is_active_low(keypad->col_gpios[i]))
			gpiod_toggle_active_low(keypad->col_gpios[i]);

f the property is present active_low is true. If it is absent active_low
is false. And the code below, if GPIO polarity setting from DT disagree
with active_low value it will flip GPIO polarity.

You probably think that gpiod_toggle_active_low() sets line as active
low, when in fact it flips to opposite setting:

void gpiod_toggle_active_low(struct gpio_desc *desc)
{
	VALIDATE_DESC_VOID(desc);
	change_bit(FLAG_ACTIVE_LOW, &desc->flags);

	^^^ flip, not set or clear.

	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
}

> 
> So I will remove the deprecated flag and change the description to:
> "Force GPIO flags to active low. Use GPIO flags if property is missing."
> OK?

No. It should say:

"Force GPIO polarity to active low. In the absence of this property
GPIOs are treated as active high."

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Tue, Feb 25, 2025 at 09:51:22AM +0100, Markus Burri wrote:
> On Mon, Feb 24, 2025 at 10:58:27PM -0800, Dmitry Torokhov wrote:
> > On Wed, Feb 19, 2025 at 05:56:10PM +0100, Manuel Traut wrote:
> > > On Fri, Jan 10, 2025 at 06:49:06AM +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.
> > > > The rescan is repeated until no change is detected anymore.
> > > 
> > > This is a quirk for a bad hardware design. For 'good' hardware it adds
> > > an additional read_row_state for no need. For even slower connected
> > > GPIOs this will also not help much. However it is obvious that it will
> > > be an improvement for some designs. 
> > > 
> > > Dmitry, would it make sense to make this configurable?
> > 
> > What if we do not disable interrupts after the first one, but record
> > the last interrupt time and rescan if it arrived after work handler
> > started executing?
> > 
> > Thanks.
> 
> I was also thinking about that.
> If we do not disable interrupts we will get a lot of interrupts during scan.
> The scanning process itself generate interrupts because of selecting the columns
> and read row state. Therefore after scan we will not know if the interrupts are
> caused by scanning or a change.

OK, then maybe we should keep re-submitting the work until we get to
stable state? My objection is repeating the scan once does not really
solve the issue....

Thanks.

-- 
Dmitry

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

[Dmitry Torokhov]

On Fri, Jan 10, 2025 at 06:49:00AM +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.
> 
> see Documentation/timers/delay_sleep_functions.rst
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> 

Applied, thank you.

-- 
Dmitry

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

[Dmitry Torokhov]

On Fri, Jan 10, 2025 at 06:49:02AM +0100, Markus Burri wrote:
> Convert the gpio-matrix-keypad bindings from text to DT schema.
> 
> Signed-off-by: Markus Burri <markus.burri@mt.com>
> 

Applied, thank you.

-- 
Dmitry

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

[Dmitry Torokhov]

On Fri, Jan 10, 2025 at 06:49:04AM +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: Rob Herring (Arm) <robh@kernel.org>
> 

Applied, thank you.

-- 
Dmitry

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

[Dmitry Torokhov]

On Fri, Jan 10, 2025 at 06:49:05AM +0100, Markus Burri wrote:
> 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>
> 

Applied, thank you.

-- 
Dmitry

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

[Markus Burri]

On Tue, Feb 25, 2025 at 09:46:24AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 25, 2025 at 09:51:22AM +0100, Markus Burri wrote:
> > On Mon, Feb 24, 2025 at 10:58:27PM -0800, Dmitry Torokhov wrote:
> > > On Wed, Feb 19, 2025 at 05:56:10PM +0100, Manuel Traut wrote:
> > > > On Fri, Jan 10, 2025 at 06:49:06AM +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.
> > > > > The rescan is repeated until no change is detected anymore.
> > > > 
> > > > This is a quirk for a bad hardware design. For 'good' hardware it adds
> > > > an additional read_row_state for no need. For even slower connected
> > > > GPIOs this will also not help much. However it is obvious that it will
> > > > be an improvement for some designs. 
> > > > 
> > > > Dmitry, would it make sense to make this configurable?
> > > 
> > > What if we do not disable interrupts after the first one, but record
> > > the last interrupt time and rescan if it arrived after work handler
> > > started executing?
> > > 
> > > Thanks.
> > 
> > I was also thinking about that.
> > If we do not disable interrupts we will get a lot of interrupts during scan.
> > The scanning process itself generate interrupts because of selecting the columns
> > and read row state. Therefore after scan we will not know if the interrupts are
> > caused by scanning or a change.
> 
> OK, then maybe we should keep re-submitting the work until we get to
> stable state? My objection is repeating the scan once does not really
> solve the issue....

Thanks for feedback.
Yes, this is what the patch does. All changes before read the initial state are
handled in the current scan. Changes during scan are detected by the compare at
the end and trigger a re-scan. Since this is done every time at the end of the
scan function, it is done until we have a stable state and not only once. 

> 
> Thanks.
> 
> -- 
> Dmitry

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

[Markus Burri]

On Tue, Feb 25, 2025 at 09:43:21AM -0800, Dmitry Torokhov wrote:
> On Tue, Feb 25, 2025 at 11:16:07AM +0100, Markus Burri wrote:
> > On Mon, Feb 24, 2025 at 10:46:09PM -0800, Dmitry Torokhov wrote:
> > > On Wed, Feb 19, 2025 at 05:42:00PM +0100, Manuel Traut wrote:
> > > > On Fri, Jan 10, 2025 at 06:49:03AM +0100, Markus Burri wrote:
> > > > > The property is implemented in the driver but not described in dt-bindings.
> > > > > Add missing property 'gpio-activelow' to DT schema.
> > > > > 
> > > > > Signed-off-by: Markus Burri <markus.burri@mt.com>
> > > > 
> > > > Reviewed-by: Manuel Traut <manuel.traut@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 0f348b9..9c91224 100644
> > > > > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > > > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > > > @@ -40,6 +40,12 @@ properties:
> > > > >      type: boolean
> > > > >      description: Do not enable autorepeat feature.
> > > > >  
> > > > > +  gpio-activelow:
> > > > > +    type: boolean
> > > > > +    deprecated: true
> > > > > +    description:
> > > > > +      The GPIOs are low active (deprecated).
> > > > > +      Use the GPIO-flags in gpio controller instead of this property.
> > > 
> > > No, we unfortunately can not rely on GPIO-flags. This is not how driver
> > > works: current driver behavior is to force GPIOs as active high if the
> > > property is missing and ignore polarity specified in GPIO property.
> > 
> > So you mean this property is still valid?
> > Current implementation toggle the GPIO flags to low active if the property and the GPIO flags are different.
> > So in case of missing property (false) the settings from GPIO flags will be used.
> > In case of gpio-activelow (true) GPIO flags are set to low active.
> 
> The driver does:
> 
> 	active_low = device_property_read_bool(&pdev->dev, "gpio-activelow");
> 
> 	...
> 
> 		if (active_low ^ gpiod_is_active_low(keypad->col_gpios[i]))
> 			gpiod_toggle_active_low(keypad->col_gpios[i]);
> 
> f the property is present active_low is true. If it is absent active_low
> is false. And the code below, if GPIO polarity setting from DT disagree
> with active_low value it will flip GPIO polarity.
> 
> You probably think that gpiod_toggle_active_low() sets line as active
> low, when in fact it flips to opposite setting:

Yes exactly my fault. Thanks for the hint!
> 
> void gpiod_toggle_active_low(struct gpio_desc *desc)
> {
> 	VALIDATE_DESC_VOID(desc);
> 	change_bit(FLAG_ACTIVE_LOW, &desc->flags);
> 
> 	^^^ flip, not set or clear.
> 
> 	gpiod_line_state_notify(desc, GPIO_V2_LINE_CHANGED_CONFIG);
> }
> 
> > 
> > So I will remove the deprecated flag and change the description to:
> > "Force GPIO flags to active low. Use GPIO flags if property is missing."
> > OK?
> 
> No. It should say:
> 
> "Force GPIO polarity to active low. In the absence of this property
> GPIOs are treated as active high."
I will send a new version of the patch

> 
> Thanks.
> 
> -- 
> Dmitry