[PATCH v1 0/6] Input: mt6779-keypad - double keys support

[PATCH v1 0/6] Input: mt6779-keypad - double keys support

[Mattijs Korpershoek]

The MediaTek keypad controller has multiple operating modes:
* single key detection (currently implemented)
* double key detection

With double key detection, each (row,column) is a group that can detect
two keys in the key matrix.
This minimizes the overall pin counts for cost reduction.
However, pressing multiple keys in the same group will not be
detected properly.

On some boards, like mt8183-pumpkin, double key detection is used.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

---
Fabien Parent (2):
      arm64: dts: mediatek: mt8183: add keyboard node
      arm64: dts: mediatek: mt8183-pumpkin: add keypad support

Mattijs Korpershoek (4):
      MAINTAINERS: input: add mattijs for mt6779-keypad
      dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
      dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
      Input: mt6779-keypad - support double keys matrix

 .../bindings/input/mediatek,mt6779-keypad.yaml      |  8 +++++++-
 MAINTAINERS                                         |  6 ++++++
 arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts     | 21 +++++++++++++++++++++
 arch/arm64/boot/dts/mediatek/mt8183.dtsi            |  9 +++++++++
 drivers/input/keyboard/mt6779-keypad.c              | 17 +++++++++++++++--
 5 files changed, 58 insertions(+), 3 deletions(-)
---
base-commit: 3b87ed7ea4d598c81a03317a92dfbd59102224fd
change-id: 20220720-mt8183-keypad-20aa77106ff0

Best regards,
-- 
Mattijs Korpershoek <mkorpershoek@baylibre.com>

[PATCH v1 1/6] MAINTAINERS: input: add mattijs for mt6779-keypad

[Mattijs Korpershoek]

As stated in [1]:
Fengping has no longer interest and time to maintain this driver so he
agreed to transfer maintainership over to me.

Add a dedicated maintainer entry as well for the driver to make sure
that I can help with patch reviews.

[1] https://lore.kernel.org/r/20220421140255.2781505-1-mkorpershoek@baylibre.com
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

diff --git a/MAINTAINERS b/MAINTAINERS
index 264e7a72afd6..f7a0bae74dc8 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -12771,6 +12771,12 @@ S:	Supported
 F:	Documentation/devicetree/bindings/media/mediatek-jpeg-*.yaml
 F:	drivers/media/platform/mediatek/jpeg/
 
+MEDIATEK KEYPAD DRIVER
+M:	Mattijs Korpershoek <mkorpershoek@baylibre.com>
+S:	Supported
+F:	Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
+F:	drivers/input/keyboard/mt6779-keypad.c
+
 MEDIATEK MDP DRIVER
 M:	Minghsiu Tsai <minghsiu.tsai@mediatek.com>
 M:	Houlong Wei <houlong.wei@mediatek.com>

-- 
b4 0.10.0-dev-54fef

[PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties

[Mattijs Korpershoek]

writing-bindings.rst states:
> - If schema includes other schema (e.g. /schemas/i2c/i2c-controller.yaml) use
>   "unevaluatedProperties:false". In other cases, usually use
>   "additionalProperties:false".

mt6779-keypad includes matrix-keymap.yaml so replace additionalProperties:false
by unevaluatedProperties:false.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
index 03ebd2665d07..ca8ae40a73f7 100644
--- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
@@ -56,7 +56,7 @@ required:
   - clocks
   - clock-names
 
-additionalProperties: false
+unevaluatedProperties: false
 
 examples:
   - |

-- 
b4 0.10.0-dev-54fef

[PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[Mattijs Korpershoek]

MediaTek keypad has 2 modes of detecting key events:
- single key: each (row, column) can detect one key
- double key: each (row, column) is a group of 2 keys

Currently, only single key detection is supported (by default)
Add an optional property, mediatek,double-keys to support double
key detection.

Double key support exists to minimize cost, since it reduces the number
of pins required for physical keys.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
index ca8ae40a73f7..03c9555849e5 100644
--- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
@@ -49,6 +49,12 @@ properties:
     maximum: 256
     default: 16
 
+  mediatek,double-keys:
+    description: |
+      use double key matrix instead of single key
+      when set, each (row,column) is a group that can detect 2 keys
+    type: boolean
+
 required:
   - compatible
   - reg

-- 
b4 0.10.0-dev-54fef

[PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix

[Mattijs Korpershoek]

MediaTek keypad has 2 modes of detecting key events:
- single key: each (row, column) can detect one key
- double key: each (row, column) is a group of 2 keys

Double key support exists to minimize cost, since it reduces the number
of pins required for physical keys.

Double key is configured by setting BIT(0) of the KP_SEL register.

Enable double key matrix support based on the mediatek,double-keys
device tree property.

Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
index bf447bf598fb..9a5dbd415dac 100644
--- a/drivers/input/keyboard/mt6779-keypad.c
+++ b/drivers/input/keyboard/mt6779-keypad.c
@@ -18,6 +18,7 @@
 #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
 #define MTK_KPD_DEBOUNCE_MAX_MS	256
 #define MTK_KPD_SEL		0x0020
+#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
 #define MTK_KPD_SEL_COL	GENMASK(15, 10)
 #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
 #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
@@ -31,6 +32,7 @@ struct mt6779_keypad {
 	struct clk *clk;
 	u32 n_rows;
 	u32 n_cols;
+	bool double_keys;
 	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
 };
 
@@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
 			continue;
 
 		key = bit_nr / 32 * 16 + bit_nr % 32;
-		row = key / 9;
-		col = key % 9;
+		if (keypad->double_keys) {
+			row = key / 13;
+			col = (key % 13) / 2;
+		} else {
+			row = key / 9;
+			col = key % 9;
+		}
 
 		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
 		/* 1: not pressed, 0: pressed */
@@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
 
 	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
 
+	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
+
 	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
 		keypad->n_rows, keypad->n_cols, debounce);
 
@@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
 	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
 		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
 
+	if (keypad->double_keys)
+		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
+				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
+
 	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_ROW,
 			   MTK_KPD_SEL_ROWMASK(keypad->n_rows));
 	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_COL,

-- 
b4 0.10.0-dev-54fef

[PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node

[Mattijs Korpershoek]

From: Fabien Parent <fparent@baylibre.com>

MT8183 has an on-SoC keyboard controller commonly used for volume
up/down buttons.

List it in the SoC dts so that boards can enable/use it.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

diff --git a/arch/arm64/boot/dts/mediatek/mt8183.dtsi b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
index 9d32871973a2..9d8fdebaabe3 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183.dtsi
+++ b/arch/arm64/boot/dts/mediatek/mt8183.dtsi
@@ -943,6 +943,15 @@ pwrap: pwrap@1000d000 {
 			clock-names = "spi", "wrap";
 		};
 
+		keyboard: keyboard@10010000 {
+			compatible = "mediatek,mt6779-keypad";
+			reg = <0 0x10010000 0 0x1000>;
+			interrupts = <GIC_SPI 186 IRQ_TYPE_EDGE_FALLING>;
+			clocks = <&clk26m>;
+			clock-names = "kpd";
+			status = "disabled";
+		};
+
 		scp: scp@10500000 {
 			compatible = "mediatek,mt8183-scp";
 			reg = <0 0x10500000 0 0x80000>,

-- 
b4 0.10.0-dev-54fef

[PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support

[Mattijs Korpershoek]

From: Fabien Parent <fparent@baylibre.com>

Add device-tree bindings for the keypad driver on the MT8183 Pumpkin
board.

The MT8183 Pumpkin board has 2 buttons connected using: KPROW0,
KPROW1 and KPCOL0.

Signed-off-by: Fabien Parent <fparent@baylibre.com>
Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

diff --git a/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts b/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts
index 530e0c9ce0c9..add697c94b05 100644
--- a/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts
+++ b/arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts
@@ -7,6 +7,7 @@
 /dts-v1/;
 
 #include <dt-bindings/gpio/gpio.h>
+#include <dt-bindings/input/input.h>
 #include "mt8183.dtsi"
 #include "mt6358.dtsi"
 
@@ -122,6 +123,18 @@ &i2c6 {
 	clock-frequency = <100000>;
 };
 
+&keyboard {
+	pinctrl-names = "default";
+	pinctrl-0 = <&keyboard_pins>;
+	status = "okay";
+	linux,keymap = <MATRIX_KEY(0x00, 0x00, KEY_VOLUMEDOWN)
+			MATRIX_KEY(0x01, 0x00, KEY_VOLUMEUP)>;
+	keypad,num-rows = <2>;
+	keypad,num-columns = <1>;
+	debounce-delay-ms = <32>;
+	mediatek,double-keys;
+};
+
 &mmc0 {
 	status = "okay";
 	pinctrl-names = "default", "state_uhs";
@@ -226,6 +239,14 @@ pins_cmd_dat {
 		};
 	};
 
+	keyboard_pins: keyboard {
+		pins_keyboard {
+			pinmux = <PINMUX_GPIO91__FUNC_KPROW1>,
+				 <PINMUX_GPIO92__FUNC_KPROW0>,
+				 <PINMUX_GPIO93__FUNC_KPCOL0>;
+		};
+	};
+
 	mmc0_pins_default: mmc0-pins-default {
 		pins_cmd_dat {
 			pinmux = <PINMUX_GPIO123__FUNC_MSDC0_DAT0>,

-- 
b4 0.10.0-dev-54fef

Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix

[Matthias Brugger]

On 20/07/2022 16:48, Mattijs Korpershoek wrote:
> MediaTek keypad has 2 modes of detecting key events:
> - single key: each (row, column) can detect one key
> - double key: each (row, column) is a group of 2 keys
> 
> Double key support exists to minimize cost, since it reduces the number
> of pins required for physical keys.
> 
> Double key is configured by setting BIT(0) of the KP_SEL register.
> 
> Enable double key matrix support based on the mediatek,double-keys
> device tree property.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>

> 
> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
> index bf447bf598fb..9a5dbd415dac 100644
> --- a/drivers/input/keyboard/mt6779-keypad.c
> +++ b/drivers/input/keyboard/mt6779-keypad.c
> @@ -18,6 +18,7 @@
>   #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>   #define MTK_KPD_DEBOUNCE_MAX_MS	256
>   #define MTK_KPD_SEL		0x0020
> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>   #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>   #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>   #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>   	struct clk *clk;
>   	u32 n_rows;
>   	u32 n_cols;
> +	bool double_keys;
>   	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>   };
>   
> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>   			continue;
>   
>   		key = bit_nr / 32 * 16 + bit_nr % 32;
> -		row = key / 9;
> -		col = key % 9;
> +		if (keypad->double_keys) {
> +			row = key / 13;
> +			col = (key % 13) / 2;
> +		} else {
> +			row = key / 9;
> +			col = key % 9;
> +		}
>   
>   		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>   		/* 1: not pressed, 0: pressed */
> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   
>   	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>   
> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
> +
>   	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>   		keypad->n_rows, keypad->n_cols, debounce);
>   
> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>   		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>   
> +	if (keypad->double_keys)
> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
> +
>   	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_ROW,
>   			   MTK_KPD_SEL_ROWMASK(keypad->n_rows));
>   	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_COL,
> 

Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties

[Krzysztof Kozlowski]

On 20/07/2022 16:48, Mattijs Korpershoek wrote:
> writing-bindings.rst states:
>> - If schema includes other schema (e.g. /schemas/i2c/i2c-controller.yaml) use
>>   "unevaluatedProperties:false". In other cases, usually use
>>   "additionalProperties:false".
> 
> mt6779-keypad includes matrix-keymap.yaml so replace additionalProperties:false
> by unevaluatedProperties:false.

This is not sufficient explanation. You now allow all properties from
matrix-keymap.yaml, which might be desired or might be not (e.g. they
are not valid for this device). Please investigate it and mention the
outcome.

Best regards,
Krzysztof

Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[Krzysztof Kozlowski]

On 20/07/2022 16:48, Mattijs Korpershoek wrote:
> MediaTek keypad has 2 modes of detecting key events:
> - single key: each (row, column) can detect one key
> - double key: each (row, column) is a group of 2 keys
> 
> Currently, only single key detection is supported (by default)
> Add an optional property, mediatek,double-keys to support double
> key detection.

You focus here on driver implementation and behavior, but should rather
focus on hardware, like - in such setup two keys are physically wired to
one (row,column) pin.

> 
> Double key support exists to minimize cost, since it reduces the number
> of pins required for physical keys.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> 
> diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
> index ca8ae40a73f7..03c9555849e5 100644
> --- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
> @@ -49,6 +49,12 @@ properties:
>      maximum: 256
>      default: 16
>  
> +  mediatek,double-keys:

Do you think there could be another MT keypad version with triple-keys?

> +    description: |
> +      use double key matrix instead of single key
> +      when set, each (row,column) is a group that can detect 2 keys
> +    type: boolean
> +
>  required:
>    - compatible
>    - reg
> 


Best regards,
Krzysztof

Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix

[AngeloGioacchino Del Regno]

Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
> MediaTek keypad has 2 modes of detecting key events:
> - single key: each (row, column) can detect one key
> - double key: each (row, column) is a group of 2 keys
> 
> Double key support exists to minimize cost, since it reduces the number
> of pins required for physical keys.
> 
> Double key is configured by setting BIT(0) of the KP_SEL register.
> 
> Enable double key matrix support based on the mediatek,double-keys
> device tree property.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
> 
> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
> index bf447bf598fb..9a5dbd415dac 100644
> --- a/drivers/input/keyboard/mt6779-keypad.c
> +++ b/drivers/input/keyboard/mt6779-keypad.c
> @@ -18,6 +18,7 @@
>   #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>   #define MTK_KPD_DEBOUNCE_MAX_MS	256
>   #define MTK_KPD_SEL		0x0020
> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>   #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>   #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>   #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>   	struct clk *clk;
>   	u32 n_rows;
>   	u32 n_cols;
> +	bool double_keys;
>   	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>   };
>   
> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>   			continue;
>   
>   		key = bit_nr / 32 * 16 + bit_nr % 32;
> -		row = key / 9;
> -		col = key % 9;
> +		if (keypad->double_keys) {
> +			row = key / 13;
> +			col = (key % 13) / 2;
> +		} else {
> +			row = key / 9;
> +			col = key % 9;
> +		}

I don't fully like this if branch permanently evaluating true or false, as no
runtime can actually change this result...

In practice, it's fine, but I was wondering if anyone would disagree with the
following proposal...

struct mt6779_keypad {
	.......
	void (*calc_row_col)(unsigned int *row, unsigned int *col);
};

In mt6779_keypad_irq_handler:

	key = bit_nr / 32 * 16 + bit_nr % 32;
	keypad->calc_row_col(&row, &col);

and below...

>   
>   		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>   		/* 1: not pressed, 0: pressed */
> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   
>   	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>   
> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
> +
>   	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>   		keypad->n_rows, keypad->n_cols, debounce);
>   
> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>   	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>   		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>   
> +	if (keypad->double_keys)

		keypad->calc_row_col = mt6779_keypad_calc_row_col_double_kp;

> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
> +

	} else {
		keypad->calc_row_col = mt6779_keypad_calc_row_col_single_kp;
	}

>   	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_ROW,
>   			   MTK_KPD_SEL_ROWMASK(keypad->n_rows));
>   	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_COL,

what do you think?

Cheers,
Angelo

Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[AngeloGioacchino Del Regno]

Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
> MediaTek keypad has 2 modes of detecting key events:
> - single key: each (row, column) can detect one key
> - double key: each (row, column) is a group of 2 keys
> 
> Currently, only single key detection is supported (by default)
> Add an optional property, mediatek,double-keys to support double
> key detection.
> 
> Double key support exists to minimize cost, since it reduces the number
> of pins required for physical keys.
> 
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
> 
> diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
> index ca8ae40a73f7..03c9555849e5 100644
> --- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
> @@ -49,6 +49,12 @@ properties:
>       maximum: 256
>       default: 16
>   
> +  mediatek,double-keys:
> +    description: |
> +      use double key matrix instead of single key
> +      when set, each (row,column) is a group that can detect 2 keys

We can make it shorter and (imo) easier to understand, like:

   mediatek,double-keys:

     description: Each (row, column) group has two keys

...also because, if we say that the group "can detect" two keys, it may be
creating a misunderstandment such as "if I press one key, it gives me two
different input events for two different keys.", which is something that
wouldn't make a lot of sense, would it? :-)

> +    type: boolean
> +
>   required:
>     - compatible
>     - reg

Re: [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node

[AngeloGioacchino Del Regno]

Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
> From: Fabien Parent <fparent@baylibre.com>
> 
> MT8183 has an on-SoC keyboard controller commonly used for volume
> up/down buttons.
> 
> List it in the SoC dts so that boards can enable/use it.
> 
> Signed-off-by: Fabien Parent <fparent@baylibre.com>
> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>

Reviewed-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com>

Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties

[Mattijs Korpershoek]

On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 20/07/2022 16:48, Mattijs Korpershoek wrote:
>> writing-bindings.rst states:
>>> - If schema includes other schema (e.g. /schemas/i2c/i2c-controller.yaml) use
>>>   "unevaluatedProperties:false". In other cases, usually use
>>>   "additionalProperties:false".
>> 
>> mt6779-keypad includes matrix-keymap.yaml so replace additionalProperties:false
>> by unevaluatedProperties:false.
>
> This is not sufficient explanation. You now allow all properties from
> matrix-keymap.yaml, which might be desired or might be not (e.g. they
> are not valid for this device). Please investigate it and mention the
> outcome.

Hi Krzysztof,

Thank you for your prompt review.

In mt6779_keypad_pdrv_probe(), we call
* matrix_keypad_parse_properties() which requires keypad,num-rows and keypad,num-cols.
* matrix_keypad_build_keymap() which uses linux,keymap

Therefore, all properties from matrix-keymap.yaml are
required by the mt6779-keypad driver.

In v2, I will add the above justification and also add all 3 properties
in the "required" list.

Initially, I did not do this because from a dts/code perspective it seemed
interesting to split out SoC specific keyboard node vs board specific key configuration:
* [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node # SoC specific
* [PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support # board specific

What would be the recommend approach for above?
I see at least 2:
* "move the whole keyboard node into the board file (mt8183-pumpkin.dts)" even if it generates
  duplication between boards using the same SoC.
* "add a "dummy keymap,row,cols" properties in the soc node which can be overriden in board file.
  For example, use rows and cols = 0 which would have the driver early exit.

Thanks,
Mattijs

>
> Best regards,
> Krzysztof

Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties

[Krzysztof Kozlowski]

On 21/07/2022 11:06, Mattijs Korpershoek wrote:
> On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
> 
>> On 20/07/2022 16:48, Mattijs Korpershoek wrote:
>>> writing-bindings.rst states:
>>>> - If schema includes other schema (e.g. /schemas/i2c/i2c-controller.yaml) use
>>>>   "unevaluatedProperties:false". In other cases, usually use
>>>>   "additionalProperties:false".
>>>
>>> mt6779-keypad includes matrix-keymap.yaml so replace additionalProperties:false
>>> by unevaluatedProperties:false.
>>
>> This is not sufficient explanation. You now allow all properties from
>> matrix-keymap.yaml, which might be desired or might be not (e.g. they
>> are not valid for this device). Please investigate it and mention the
>> outcome.
> 
> Hi Krzysztof,
> 
> Thank you for your prompt review.
> 
> In mt6779_keypad_pdrv_probe(), we call
> * matrix_keypad_parse_properties() which requires keypad,num-rows and keypad,num-cols.
> * matrix_keypad_build_keymap() which uses linux,keymap
> 
> Therefore, all properties from matrix-keymap.yaml are
> required by the mt6779-keypad 
Better to mention the device, not driver.

> 
> In v2, I will add the above justification and also add all 3 properties
> in the "required" list.
> 
> Initially, I did not do this because from a dts/code perspective it seemed
> interesting to split out SoC specific keyboard node vs board specific key configuration:
> * [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node # SoC specific
> * [PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support # board specific
> 
> What would be the recommend approach for above?
> I see at least 2:
> * "move the whole keyboard node into the board file (mt8183-pumpkin.dts)" even if it generates
>   duplication between boards using the same SoC.
> * "add a "dummy keymap,row,cols" properties in the soc node which can be overriden in board file.
>   For example, use rows and cols = 0 which would have the driver early exit.
> 
SoC DTSI should have only SoC properties. The keyboard module is part of
SoC. The keys and how it is wired to them - not.

Best regards,
Krzysztof

Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties

[Mattijs Korpershoek]

On Thu, Jul 21, 2022 at 11:16, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 21/07/2022 11:06, Mattijs Korpershoek wrote:
>> On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:
>> 
>>> On 20/07/2022 16:48, Mattijs Korpershoek wrote:
>>>> writing-bindings.rst states:
>>>>> - If schema includes other schema (e.g. /schemas/i2c/i2c-controller.yaml) use
>>>>>   "unevaluatedProperties:false". In other cases, usually use
>>>>>   "additionalProperties:false".
>>>>
>>>> mt6779-keypad includes matrix-keymap.yaml so replace additionalProperties:false
>>>> by unevaluatedProperties:false.
>>>
>>> This is not sufficient explanation. You now allow all properties from
>>> matrix-keymap.yaml, which might be desired or might be not (e.g. they
>>> are not valid for this device). Please investigate it and mention the
>>> outcome.
>> 
>> Hi Krzysztof,
>> 
>> Thank you for your prompt review.
>> 
>> In mt6779_keypad_pdrv_probe(), we call
>> * matrix_keypad_parse_properties() which requires keypad,num-rows and keypad,num-cols.
>> * matrix_keypad_build_keymap() which uses linux,keymap
>> 
>> Therefore, all properties from matrix-keymap.yaml are
>> required by the mt6779-keypad 
> Better to mention the device, not driver.

I mixed up driver versus device (hardware). Sorry about that.

For successful key detection, the hardware (called MediaTek keypad) 
requires that we program rows/columns via the KP_SEL register.
So num-rows and num-cols are valid properties for this device.

The MediaTek keypad has a set of bits representing keys, from KEY0 to KEY77. 
These keys are organized in a 8x8 hardware matrix.
Therefore, linux,keymap is also a valid property for this device.
>
>> 
>> In v2, I will add the above justification and also add all 3 properties
>> in the "required" list.
>> 
>> Initially, I did not do this because from a dts/code perspective it seemed
>> interesting to split out SoC specific keyboard node vs board specific key configuration:
>> * [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node # SoC specific
>> * [PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support # board specific
>> 
>> What would be the recommend approach for above?
>> I see at least 2:
>> * "move the whole keyboard node into the board file (mt8183-pumpkin.dts)" even if it generates
>>   duplication between boards using the same SoC.
>> * "add a "dummy keymap,row,cols" properties in the soc node which can be overriden in board file.
>>   For example, use rows and cols = 0 which would have the driver early exit.
>> 
> SoC DTSI should have only SoC properties. The keyboard module is part of
> SoC. The keys and how it is wired to them - not.

Indeed. So the split I send in v1 is "valid", from a device(hardware)
point of view.
In that case i'll not make the properties from matrix-keymap.yaml
*required* in v2.

Thanks again for your feedback.

Mattijs

>
> Best regards,
> Krzysztof

Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[Mattijs Korpershoek]

On Wed, Jul 20, 2022 at 19:26, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 20/07/2022 16:48, Mattijs Korpershoek wrote:
>> MediaTek keypad has 2 modes of detecting key events:
>> - single key: each (row, column) can detect one key
>> - double key: each (row, column) is a group of 2 keys
>> 
>> Currently, only single key detection is supported (by default)
>> Add an optional property, mediatek,double-keys to support double
>> key detection.
>
> You focus here on driver implementation and behavior, but should rather
> focus on hardware, like - in such setup two keys are physically wired to
> one (row,column) pin.

Understood. Will reword in v2 to reflect that this is hardware
description, not a software feature.

>
>> 
>> Double key support exists to minimize cost, since it reduces the number
>> of pins required for physical keys.
>> 
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> 
>> diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>> index ca8ae40a73f7..03c9555849e5 100644
>> --- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>> +++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>> @@ -49,6 +49,12 @@ properties:
>>      maximum: 256
>>      default: 16
>>  
>> +  mediatek,double-keys:
>
> Do you think there could be another MT keypad version with triple-keys?

Of all the SoC's i've worked on (MT8167, MT8183, MT8365, MT8195) I've
never seen a "triple-keys" keypad.

>
>> +    description: |
>> +      use double key matrix instead of single key
>> +      when set, each (row,column) is a group that can detect 2 keys
>> +    type: boolean
>> +
>>  required:
>>    - compatible
>>    - reg
>> 
>
>
> Best regards,
> Krzysztof

Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[Mattijs Korpershoek]

On Thu, Jul 21, 2022 at 10:40, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
>> MediaTek keypad has 2 modes of detecting key events:
>> - single key: each (row, column) can detect one key
>> - double key: each (row, column) is a group of 2 keys
>> 
>> Currently, only single key detection is supported (by default)
>> Add an optional property, mediatek,double-keys to support double
>> key detection.
>> 
>> Double key support exists to minimize cost, since it reduces the number
>> of pins required for physical keys.
>> 
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> 
>> diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>> index ca8ae40a73f7..03c9555849e5 100644
>> --- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>> +++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>> @@ -49,6 +49,12 @@ properties:
>>       maximum: 256
>>       default: 16
>>   
>> +  mediatek,double-keys:
>> +    description: |
>> +      use double key matrix instead of single key
>> +      when set, each (row,column) is a group that can detect 2 keys
>
> We can make it shorter and (imo) easier to understand, like:
>
>    mediatek,double-keys:
>
>      description: Each (row, column) group has two keys
>
> ...also because, if we say that the group "can detect" two keys, it may be
> creating a misunderstandment such as "if I press one key, it gives me two
> different input events for two different keys.", which is something that
> wouldn't make a lot of sense, would it? :-)

Hi AngeloGioacchino,

Thank you for the suggestion. I like your description better as well :)
Will use it in v2.

>
>> +    type: boolean
>> +
>>   required:
>>     - compatible
>>     - reg

Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[Krzysztof Kozlowski]

On 21/07/2022 15:32, Mattijs Korpershoek wrote:
>>> diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>>> index ca8ae40a73f7..03c9555849e5 100644
>>> --- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>>> +++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>>> @@ -49,6 +49,12 @@ properties:
>>>      maximum: 256
>>>      default: 16
>>>  
>>> +  mediatek,double-keys:
>>
>> Do you think there could be another MT keypad version with triple-keys?
> 
> Of all the SoC's i've worked on (MT8167, MT8183, MT8365, MT8195) I've
> never seen a "triple-keys" keypad.

OK, but the binding you create now would be poor if MT comes with such
tripe-key feature later...


Best regards,
Krzysztof

Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys

[Mattijs Korpershoek]

On Thu, Jul 21, 2022 at 15:51, Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org> wrote:

> On 21/07/2022 15:32, Mattijs Korpershoek wrote:
>>>> diff --git a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>>>> index ca8ae40a73f7..03c9555849e5 100644
>>>> --- a/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>>>> +++ b/Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
>>>> @@ -49,6 +49,12 @@ properties:
>>>>      maximum: 256
>>>>      default: 16
>>>>  
>>>> +  mediatek,double-keys:
>>>
>>> Do you think there could be another MT keypad version with triple-keys?
>> 
>> Of all the SoC's i've worked on (MT8167, MT8183, MT8365, MT8195) I've
>> never seen a "triple-keys" keypad.
>
> OK, but the binding you create now would be poor if MT comes with such
> tripe-key feature later...

ACK, I'll send a v2 to transform this into a uint32 property named
mediatek,keys-per-group

Thanks,
Mattijs

>
>
> Best regards,
> Krzysztof

Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix

[Mattijs Korpershoek]

On Thu, Jul 21, 2022 at 10:34, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:

> Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
>> MediaTek keypad has 2 modes of detecting key events:
>> - single key: each (row, column) can detect one key
>> - double key: each (row, column) is a group of 2 keys
>> 
>> Double key support exists to minimize cost, since it reduces the number
>> of pins required for physical keys.
>> 
>> Double key is configured by setting BIT(0) of the KP_SEL register.
>> 
>> Enable double key matrix support based on the mediatek,double-keys
>> device tree property.
>> 
>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>> 
>> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
>> index bf447bf598fb..9a5dbd415dac 100644
>> --- a/drivers/input/keyboard/mt6779-keypad.c
>> +++ b/drivers/input/keyboard/mt6779-keypad.c
>> @@ -18,6 +18,7 @@
>>   #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>>   #define MTK_KPD_DEBOUNCE_MAX_MS	256
>>   #define MTK_KPD_SEL		0x0020
>> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>>   #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>>   #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>>   #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
>> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>>   	struct clk *clk;
>>   	u32 n_rows;
>>   	u32 n_cols;
>> +	bool double_keys;
>>   	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>>   };
>>   
>> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>>   			continue;
>>   
>>   		key = bit_nr / 32 * 16 + bit_nr % 32;
>> -		row = key / 9;
>> -		col = key % 9;
>> +		if (keypad->double_keys) {
>> +			row = key / 13;
>> +			col = (key % 13) / 2;
>> +		} else {
>> +			row = key / 9;
>> +			col = key % 9;
>> +		}
>
> I don't fully like this if branch permanently evaluating true or false, as no
> runtime can actually change this result...
>
> In practice, it's fine, but I was wondering if anyone would disagree with the
> following proposal...
>
> struct mt6779_keypad {
> 	.......
> 	void (*calc_row_col)(unsigned int *row, unsigned int *col);
> };
>
> In mt6779_keypad_irq_handler:
>
> 	key = bit_nr / 32 * 16 + bit_nr % 32;
> 	keypad->calc_row_col(&row, &col);
>
> and below...
>
>>   
>>   		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>>   		/* 1: not pressed, 0: pressed */
>> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>   
>>   	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>>   
>> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
>> +
>>   	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>>   		keypad->n_rows, keypad->n_cols, debounce);
>>   
>> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>   	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>>   		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>>   
>> +	if (keypad->double_keys)
>
> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_double_kp;
>
>> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
>> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
>> +
>
> 	} else {
> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_single_kp;
> 	}
>
>>   	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_ROW,
>>   			   MTK_KPD_SEL_ROWMASK(keypad->n_rows));
>>   	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_COL,
>
> what do you think?

Hi Angelo,

Thank you for your detailed suggestion. I like it and since I have to
resend a v2 anyways, I will consider implementing it.
On the other hand, I'm a little reluctant because it means that I'll
have to remove Matthias's reviewed-by :(

>
> Cheers,
> Angelo

Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix

[AngeloGioacchino Del Regno]

Il 21/07/22 16:51, Mattijs Korpershoek ha scritto:
> On Thu, Jul 21, 2022 at 10:34, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
> 
>> Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
>>> MediaTek keypad has 2 modes of detecting key events:
>>> - single key: each (row, column) can detect one key
>>> - double key: each (row, column) is a group of 2 keys
>>>
>>> Double key support exists to minimize cost, since it reduces the number
>>> of pins required for physical keys.
>>>
>>> Double key is configured by setting BIT(0) of the KP_SEL register.
>>>
>>> Enable double key matrix support based on the mediatek,double-keys
>>> device tree property.
>>>
>>> Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com>
>>> Reviewed-by: Matthias Brugger <matthias.bgg@gmail.com>
>>>
>>> diff --git a/drivers/input/keyboard/mt6779-keypad.c b/drivers/input/keyboard/mt6779-keypad.c
>>> index bf447bf598fb..9a5dbd415dac 100644
>>> --- a/drivers/input/keyboard/mt6779-keypad.c
>>> +++ b/drivers/input/keyboard/mt6779-keypad.c
>>> @@ -18,6 +18,7 @@
>>>    #define MTK_KPD_DEBOUNCE_MASK	GENMASK(13, 0)
>>>    #define MTK_KPD_DEBOUNCE_MAX_MS	256
>>>    #define MTK_KPD_SEL		0x0020
>>> +#define MTK_KPD_SEL_DOUBLE_KP_MODE	BIT(0)
>>>    #define MTK_KPD_SEL_COL	GENMASK(15, 10)
>>>    #define MTK_KPD_SEL_ROW	GENMASK(9, 4)
>>>    #define MTK_KPD_SEL_COLMASK(c)	GENMASK((c) + 9, 10)
>>> @@ -31,6 +32,7 @@ struct mt6779_keypad {
>>>    	struct clk *clk;
>>>    	u32 n_rows;
>>>    	u32 n_cols;
>>> +	bool double_keys;
>>>    	DECLARE_BITMAP(keymap_state, MTK_KPD_NUM_BITS);
>>>    };
>>>    
>>> @@ -67,8 +69,13 @@ static irqreturn_t mt6779_keypad_irq_handler(int irq, void *dev_id)
>>>    			continue;
>>>    
>>>    		key = bit_nr / 32 * 16 + bit_nr % 32;
>>> -		row = key / 9;
>>> -		col = key % 9;
>>> +		if (keypad->double_keys) {
>>> +			row = key / 13;
>>> +			col = (key % 13) / 2;
>>> +		} else {
>>> +			row = key / 9;
>>> +			col = key % 9;
>>> +		}
>>
>> I don't fully like this if branch permanently evaluating true or false, as no
>> runtime can actually change this result...
>>
>> In practice, it's fine, but I was wondering if anyone would disagree with the
>> following proposal...
>>
>> struct mt6779_keypad {
>> 	.......
>> 	void (*calc_row_col)(unsigned int *row, unsigned int *col);
>> };
>>
>> In mt6779_keypad_irq_handler:
>>
>> 	key = bit_nr / 32 * 16 + bit_nr % 32;
>> 	keypad->calc_row_col(&row, &col);
>>
>> and below...
>>
>>>    
>>>    		scancode = MATRIX_SCAN_CODE(row, col, row_shift);
>>>    		/* 1: not pressed, 0: pressed */
>>> @@ -150,6 +157,8 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>>    
>>>    	wakeup = device_property_read_bool(&pdev->dev, "wakeup-source");
>>>    
>>> +	keypad->double_keys = device_property_read_bool(&pdev->dev, "mediatek,double-keys");
>>> +
>>>    	dev_dbg(&pdev->dev, "n_row=%d n_col=%d debounce=%d\n",
>>>    		keypad->n_rows, keypad->n_cols, debounce);
>>>    
>>> @@ -166,6 +175,10 @@ static int mt6779_keypad_pdrv_probe(struct platform_device *pdev)
>>>    	regmap_write(keypad->regmap, MTK_KPD_DEBOUNCE,
>>>    		     (debounce * (1 << 5)) & MTK_KPD_DEBOUNCE_MASK);
>>>    
>>> +	if (keypad->double_keys)
>>
>> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_double_kp;
>>
>>> +		regmap_update_bits(keypad->regmap, MTK_KPD_SEL,
>>> +				   MTK_KPD_SEL_DOUBLE_KP_MODE, MTK_KPD_SEL_DOUBLE_KP_MODE);
>>> +
>>
>> 	} else {
>> 		keypad->calc_row_col = mt6779_keypad_calc_row_col_single_kp;
>> 	}
>>
>>>    	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_ROW,
>>>    			   MTK_KPD_SEL_ROWMASK(keypad->n_rows));
>>>    	regmap_update_bits(keypad->regmap, MTK_KPD_SEL, MTK_KPD_SEL_COL,
>>
>> what do you think?
> 
> Hi Angelo,
> 
> Thank you for your detailed suggestion. I like it and since I have to
> resend a v2 anyways, I will consider implementing it.
> On the other hand, I'm a little reluctant because it means that I'll
> have to remove Matthias's reviewed-by :(
> 

Yes, you will have to. In that case:

Matthias, any considerations about this idea? :)))

>>
>> Cheers,
>> Angelo

Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix

[Mattijs Korpershoek]

On Thu, Jul 21, 2022 at 16:55, AngeloGioacchino Del Regno <angelogioacchino.delregno@collabora.com> wrote:
[...]

> Il 21/07/22 16:51, Mattijs Korpershoek ha scritto:
>> 
>> Hi Angelo,
>> 
>> Thank you for your detailed suggestion. I like it and since I have to
>> resend a v2 anyways, I will consider implementing it.
>> On the other hand, I'm a little reluctant because it means that I'll
>> have to remove Matthias's reviewed-by :(
>> 
>
> Yes, you will have to. In that case:
>
> Matthias, any considerations about this idea? :)))

Since the binding document changed, I have to rework this patch anyways.
So I will drop Matthias's reviewed-by in v2.

>
>>>
>>> Cheers,
>>> Angelo