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

[PATCH v3 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 V3:
* fix 'references a file that doesn't exist' in wakeup-source.txt
* remove copied and depricated 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

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


-- 
2.39.5

[PATCH v3 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 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 v3 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 v3 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    | 86 +++++++++++++++++++
 .../bindings/power/wakeup-source.txt          |  2 +-
 3 files changed, 87 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..75975a1
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -0,0 +1,86 @@
+# 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.
+
+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/input/matrix-keymap.yaml#/properties/linux,keymap
+
+  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 v3 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          | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
index 75975a1..b10da65 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -38,6 +38,9 @@ properties:
     type: boolean
     description: Do not enable autorepeat feature.
 
+  gpio-activelow:
+    type: boolean
+    description: The GPIOs are low active.
 
   debounce-delay-ms:
     description: Debounce interval in milliseconds.
-- 
2.39.5

[PATCH v3 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 b10da65..f527f33 100644
--- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
@@ -52,6 +52,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 v3 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 v3 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 | 17 +++++++++++++++++
 1 file changed, 17 insertions(+)

diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c
index fdb3499..ddf2672 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,19 @@ 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) {
+		scoped_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 v3 3/7] dt-bindings: input: matrix_keypad - convert to YAML

[Rob Herring]

On Tue, Jan 07, 2025 at 02:56:55PM +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    | 86 +++++++++++++++++++
>  .../bindings/power/wakeup-source.txt          |  2 +-
>  3 files changed, 87 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..75975a1
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -0,0 +1,86 @@
> +# 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.
> +
> +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/input/matrix-keymap.yaml#/properties/linux,keymap

We generally don't reference individual properties. Instead, at the 
top-level you need:

allOf:
  - $ref: matrix-keymap.yaml#

And then here just: "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 v3 4/7] dt-bindings: input: matrix_keypad - add missing property

[Rob Herring]

On Tue, Jan 07, 2025 at 02:56:56PM +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          | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> index 75975a1..b10da65 100644
> --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> @@ -38,6 +38,9 @@ properties:
>      type: boolean
>      description: Do not enable autorepeat feature.
>  
> +  gpio-activelow:
> +    type: boolean
> +    description: The GPIOs are low active.

Ideally this should be defined correctly in the gpio properties. The 
problem is that does a 0 flag value mean active high or I forgot to 
define it. There's a similar issue in spi-controller.yaml. I *think* the 
problem is better here since this is an active low boolean rather than 
an active high boolean.

Of the users in the kernel tree, only 
arch/arm/boot/dts/ti/omap/am335x-guardian.dts got this right.

So I think we should mark this deprecated and put a note to use GPIO 
flags instead.

Rob

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

[Rob Herring (Arm)]

On Tue, 07 Jan 2025 14:56:57 +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(+)
> 

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

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

[Thomas Weißschuh]

On 2025-01-07 14:56:53+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.
> 
> Link: https://www.kernel.org/doc/html/latest/timers/timers-howto.html

timers-howto.rst was removed in commit 1f455f601e20
("timers/Documentation: Cleanup delay/sleep documentation").

Also inside the kernel tree refer to documentation files by their location
inside the tree instead of the website.

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

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

[Krzysztof Kozlowski]

On Tue, Jan 07, 2025 at 02:56:56PM +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.

Wasn't this added for ACPI or some platform-data users? It is not really
a correct reason to document something post-factum just because review
was skipped...

Best regards,
Krzysztof

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

[Dmitry Torokhov]

On Tue, Jan 07, 2025 at 01:27:01PM -0600, Rob Herring wrote:
> On Tue, Jan 07, 2025 at 02:56:56PM +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          | 3 +++
> >  1 file changed, 3 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > index 75975a1..b10da65 100644
> > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > @@ -38,6 +38,9 @@ properties:
> >      type: boolean
> >      description: Do not enable autorepeat feature.
> >  
> > +  gpio-activelow:
> > +    type: boolean
> > +    description: The GPIOs are low active.
> 
> Ideally this should be defined correctly in the gpio properties. The 
> problem is that does a 0 flag value mean active high or I forgot to 
> define it. There's a similar issue in spi-controller.yaml. I *think* the 
> problem is better here since this is an active low boolean rather than 
> an active high boolean.
> 
> Of the users in the kernel tree, only 
> arch/arm/boot/dts/ti/omap/am335x-guardian.dts got this right.
> 
> So I think we should mark this deprecated and put a note to use GPIO 
> flags instead.

So is the proposal to force GPIO as active low if the property is
present and leave as is if it is missing? Because current driver
behavior is to force GPIOs as active high if the property is missing.

Also, what is the benefit from having property marked as deprecated vs
not documenting it in hopes that DTSes will fail validation and be
fixed?

Thanks.

-- 
Dmitry

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

[Markus Burri]

On Wed, Jan 08, 2025 at 10:04:24PM -0800, Dmitry Torokhov wrote:
> On Tue, Jan 07, 2025 at 01:27:01PM -0600, Rob Herring wrote:
> > On Tue, Jan 07, 2025 at 02:56:56PM +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          | 3 +++
> > >  1 file changed, 3 insertions(+)
> > > 
> > > diff --git a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > index 75975a1..b10da65 100644
> > > --- a/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > +++ b/Documentation/devicetree/bindings/input/gpio-matrix-keypad.yaml
> > > @@ -38,6 +38,9 @@ properties:
> > >      type: boolean
> > >      description: Do not enable autorepeat feature.
> > >  
> > > +  gpio-activelow:
> > > +    type: boolean
> > > +    description: The GPIOs are low active.
> > 
> > Ideally this should be defined correctly in the gpio properties. The 
> > problem is that does a 0 flag value mean active high or I forgot to 
> > define it. There's a similar issue in spi-controller.yaml. I *think* the 
> > problem is better here since this is an active low boolean rather than 
> > an active high boolean.
> > 
> > Of the users in the kernel tree, only 
> > arch/arm/boot/dts/ti/omap/am335x-guardian.dts got this right.
> > 
> > So I think we should mark this deprecated and put a note to use GPIO 
> > flags instead.
> 
> So is the proposal to force GPIO as active low if the property is
> present and leave as is if it is missing? Because current driver
> behavior is to force GPIOs as active high if the property is missing.
> 

I do not touch the current implementation.
Currently if the property is set the GPIO's are toggled to active low or if
the property is missing to active high.

> Also, what is the benefit from having property marked as deprecated vs
> not documenting it in hopes that DTSes will fail validation and be
> fixed?

Good question?
The dt schema checker will complain since it is used in some dtb's
I do not like to see warnings

> 
> Thanks.
> 
> -- 
> Dmitry