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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
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
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 <[email protected]>
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
From: Fabien Parent <[email protected]>
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 <[email protected]>
Signed-off-by: Mattijs Korpershoek <[email protected]>
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
From: Fabien Parent <[email protected]>
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 <[email protected]>
Signed-off-by: Mattijs Korpershoek <[email protected]>
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
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 <[email protected]>
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
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 <[email protected]>
Reviewed-by: Matthias Brugger <[email protected]>
>
> 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,
>
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/[email protected]
Signed-off-by: Mattijs Korpershoek <[email protected]>
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 <[email protected]>
+S: Supported
+F: Documentation/devicetree/bindings/input/mediatek,mt6779-keypad.yaml
+F: drivers/input/keyboard/mt6779-keypad.c
+
MEDIATEK MDP DRIVER
M: Minghsiu Tsai <[email protected]>
M: Houlong Wei <[email protected]>
--
b4 0.10.0-dev-54fef
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
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 <[email protected]>
>
> 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
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 <[email protected]>
> Reviewed-by: Matthias Brugger <[email protected]>
>
> 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
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 <[email protected]>
>
> 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
Il 20/07/22 16:48, Mattijs Korpershoek ha scritto:
> From: Fabien Parent <[email protected]>
>
> 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 <[email protected]>
> Signed-off-by: Mattijs Korpershoek <[email protected]>
Reviewed-by: AngeloGioacchino Del Regno <[email protected]>
On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski <[email protected]> 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
On 21/07/2022 11:06, Mattijs Korpershoek wrote:
> On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski <[email protected]> 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
On Wed, Jul 20, 2022 at 19:26, Krzysztof Kozlowski <[email protected]> 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 <[email protected]>
>>
>> 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
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
On Thu, Jul 21, 2022 at 11:16, Krzysztof Kozlowski <[email protected]> wrote:
> On 21/07/2022 11:06, Mattijs Korpershoek wrote:
>> On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski <[email protected]> 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
On Thu, Jul 21, 2022 at 10:40, AngeloGioacchino Del Regno <[email protected]> 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 <[email protected]>
>>
>> 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
Il 21/07/22 16:51, Mattijs Korpershoek ha scritto:
> On Thu, Jul 21, 2022 at 10:34, AngeloGioacchino Del Regno <[email protected]> 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 <[email protected]>
>>> Reviewed-by: Matthias Brugger <[email protected]>
>>>
>>> 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
On Thu, Jul 21, 2022 at 15:51, Krzysztof Kozlowski <[email protected]> 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
On Thu, Jul 21, 2022 at 10:34, AngeloGioacchino Del Regno <[email protected]> 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 <[email protected]>
>> Reviewed-by: Matthias Brugger <[email protected]>
>>
>> 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
On Thu, Jul 21, 2022 at 16:55, AngeloGioacchino Del Regno <[email protected]> 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