* [PATCH v1 1/6] MAINTAINERS: input: add mattijs for mt6779-keypad
2022-07-20 14:48 [PATCH v1 0/6] Input: mt6779-keypad - double keys support Mattijs Korpershoek
@ 2022-07-20 14:48 ` Mattijs Korpershoek
2022-07-20 14:48 ` [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties Mattijs Korpershoek
` (4 subsequent siblings)
5 siblings, 0 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-20 14:48 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Mattijs Korpershoek,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
2022-07-20 14:48 [PATCH v1 0/6] Input: mt6779-keypad - double keys support Mattijs Korpershoek
2022-07-20 14:48 ` [PATCH v1 1/6] MAINTAINERS: input: add mattijs for mt6779-keypad Mattijs Korpershoek
@ 2022-07-20 14:48 ` Mattijs Korpershoek
2022-07-20 17:14 ` Krzysztof Kozlowski
2022-07-20 14:48 ` [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys Mattijs Korpershoek
` (3 subsequent siblings)
5 siblings, 1 reply; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-20 14:48 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Mattijs Korpershoek,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
2022-07-20 14:48 ` [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties Mattijs Korpershoek
@ 2022-07-20 17:14 ` Krzysztof Kozlowski
2022-07-21 9:06 ` Mattijs Korpershoek
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-20 17:14 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
2022-07-20 17:14 ` Krzysztof Kozlowski
@ 2022-07-21 9:06 ` Mattijs Korpershoek
2022-07-21 9:16 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-21 9:06 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
2022-07-21 9:06 ` Mattijs Korpershoek
@ 2022-07-21 9:16 ` Krzysztof Kozlowski
2022-07-21 13:11 ` Mattijs Korpershoek
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-21 9:16 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties
2022-07-21 9:16 ` Krzysztof Kozlowski
@ 2022-07-21 13:11 ` Mattijs Korpershoek
0 siblings, 0 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-21 13:11 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-20 14:48 [PATCH v1 0/6] Input: mt6779-keypad - double keys support Mattijs Korpershoek
2022-07-20 14:48 ` [PATCH v1 1/6] MAINTAINERS: input: add mattijs for mt6779-keypad Mattijs Korpershoek
2022-07-20 14:48 ` [PATCH v1 2/6] dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties Mattijs Korpershoek
@ 2022-07-20 14:48 ` Mattijs Korpershoek
2022-07-20 17:26 ` Krzysztof Kozlowski
2022-07-21 8:40 ` AngeloGioacchino Del Regno
2022-07-20 14:48 ` [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix Mattijs Korpershoek
` (2 subsequent siblings)
5 siblings, 2 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-20 14:48 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Mattijs Korpershoek,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-20 14:48 ` [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys Mattijs Korpershoek
@ 2022-07-20 17:26 ` Krzysztof Kozlowski
2022-07-21 13:32 ` Mattijs Korpershoek
2022-07-21 8:40 ` AngeloGioacchino Del Regno
1 sibling, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-20 17:26 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-20 17:26 ` Krzysztof Kozlowski
@ 2022-07-21 13:32 ` Mattijs Korpershoek
2022-07-21 13:51 ` Krzysztof Kozlowski
0 siblings, 1 reply; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-21 13:32 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-21 13:32 ` Mattijs Korpershoek
@ 2022-07-21 13:51 ` Krzysztof Kozlowski
2022-07-21 14:44 ` Mattijs Korpershoek
0 siblings, 1 reply; 23+ messages in thread
From: Krzysztof Kozlowski @ 2022-07-21 13:51 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-21 13:51 ` Krzysztof Kozlowski
@ 2022-07-21 14:44 ` Mattijs Korpershoek
0 siblings, 0 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-21 14:44 UTC (permalink / raw)
To: Krzysztof Kozlowski, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-20 14:48 ` [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys Mattijs Korpershoek
2022-07-20 17:26 ` Krzysztof Kozlowski
@ 2022-07-21 8:40 ` AngeloGioacchino Del Regno
2022-07-21 13:34 ` Mattijs Korpershoek
1 sibling, 1 reply; 23+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-21 8:40 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys
2022-07-21 8:40 ` AngeloGioacchino Del Regno
@ 2022-07-21 13:34 ` Mattijs Korpershoek
0 siblings, 0 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-21 13:34 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix
2022-07-20 14:48 [PATCH v1 0/6] Input: mt6779-keypad - double keys support Mattijs Korpershoek
` (2 preceding siblings ...)
2022-07-20 14:48 ` [PATCH v1 3/6] dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys Mattijs Korpershoek
@ 2022-07-20 14:48 ` Mattijs Korpershoek
2022-07-20 14:53 ` Matthias Brugger
2022-07-21 8:34 ` AngeloGioacchino Del Regno
2022-07-20 14:48 ` [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node Mattijs Korpershoek
2022-07-20 14:48 ` [PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support Mattijs Korpershoek
5 siblings, 2 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-20 14:48 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Mattijs Korpershoek,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix
2022-07-20 14:48 ` [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix Mattijs Korpershoek
@ 2022-07-20 14:53 ` Matthias Brugger
2022-07-21 8:34 ` AngeloGioacchino Del Regno
1 sibling, 0 replies; 23+ messages in thread
From: Matthias Brugger @ 2022-07-20 14:53 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski, Dmitry Torokhov
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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,
>
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix
2022-07-20 14:48 ` [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix Mattijs Korpershoek
2022-07-20 14:53 ` Matthias Brugger
@ 2022-07-21 8:34 ` AngeloGioacchino Del Regno
2022-07-21 14:51 ` Mattijs Korpershoek
1 sibling, 1 reply; 23+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-21 8:34 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix
2022-07-21 8:34 ` AngeloGioacchino Del Regno
@ 2022-07-21 14:51 ` Mattijs Korpershoek
2022-07-21 14:55 ` AngeloGioacchino Del Regno
0 siblings, 1 reply; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-21 14:51 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix
2022-07-21 14:51 ` Mattijs Korpershoek
@ 2022-07-21 14:55 ` AngeloGioacchino Del Regno
2022-07-26 9:52 ` Mattijs Korpershoek
0 siblings, 1 reply; 23+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-21 14:55 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix
2022-07-21 14:55 ` AngeloGioacchino Del Regno
@ 2022-07-26 9:52 ` Mattijs Korpershoek
0 siblings, 0 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-26 9:52 UTC (permalink / raw)
To: AngeloGioacchino Del Regno, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node
2022-07-20 14:48 [PATCH v1 0/6] Input: mt6779-keypad - double keys support Mattijs Korpershoek
` (3 preceding siblings ...)
2022-07-20 14:48 ` [PATCH v1 4/6] Input: mt6779-keypad - support double keys matrix Mattijs Korpershoek
@ 2022-07-20 14:48 ` Mattijs Korpershoek
2022-07-21 8:41 ` AngeloGioacchino Del Regno
2022-07-20 14:48 ` [PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support Mattijs Korpershoek
5 siblings, 1 reply; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-20 14:48 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Mattijs Korpershoek,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node
2022-07-20 14:48 ` [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node Mattijs Korpershoek
@ 2022-07-21 8:41 ` AngeloGioacchino Del Regno
0 siblings, 0 replies; 23+ messages in thread
From: AngeloGioacchino Del Regno @ 2022-07-21 8:41 UTC (permalink / raw)
To: Mattijs Korpershoek, Rob Herring, Krzysztof Kozlowski,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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>
^ permalink raw reply [flat|nested] 23+ messages in thread
* [PATCH v1 6/6] arm64: dts: mediatek: mt8183-pumpkin: add keypad support
2022-07-20 14:48 [PATCH v1 0/6] Input: mt6779-keypad - double keys support Mattijs Korpershoek
` (4 preceding siblings ...)
2022-07-20 14:48 ` [PATCH v1 5/6] arm64: dts: mediatek: mt8183: add keyboard node Mattijs Korpershoek
@ 2022-07-20 14:48 ` Mattijs Korpershoek
5 siblings, 0 replies; 23+ messages in thread
From: Mattijs Korpershoek @ 2022-07-20 14:48 UTC (permalink / raw)
To: Rob Herring, Krzysztof Kozlowski, Mattijs Korpershoek,
Dmitry Torokhov, Matthias Brugger
Cc: linux-kernel, linux-input, Fabien Parent, devicetree,
linux-mediatek, Fabien Parent, linux-arm-kernel
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
^ permalink raw reply related [flat|nested] 23+ messages in thread