| Message ID | 20220720-mt8183-keypad-v1-0-ef9fc29dbff4@baylibre.com |
|---|---|
| Headers | show |
| Series | Input: mt6779-keypad - double keys support | expand |
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
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
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
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 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
The MediaTek keypad controller has multiple operating modes: * single key detection (currently implemented) * double key detection With double key detection, each (row,column) is a group that can detect two keys in the key matrix. This minimizes the overall pin counts for cost reduction. However, pressing multiple keys in the same group will not be detected properly. On some boards, like mt8183-pumpkin, double key detection is used. Signed-off-by: Mattijs Korpershoek <mkorpershoek@baylibre.com> --- Fabien Parent (2): arm64: dts: mediatek: mt8183: add keyboard node arm64: dts: mediatek: mt8183-pumpkin: add keypad support Mattijs Korpershoek (4): MAINTAINERS: input: add mattijs for mt6779-keypad dt-bindings: mediatek,mt6779-keypad: use unevaluatedProperties dt-bindings: mediatek,mt6779-keypad: add mediatek,double-keys Input: mt6779-keypad - support double keys matrix .../bindings/input/mediatek,mt6779-keypad.yaml | 8 +++++++- MAINTAINERS | 6 ++++++ arch/arm64/boot/dts/mediatek/mt8183-pumpkin.dts | 21 +++++++++++++++++++++ arch/arm64/boot/dts/mediatek/mt8183.dtsi | 9 +++++++++ drivers/input/keyboard/mt6779-keypad.c | 17 +++++++++++++++-- 5 files changed, 58 insertions(+), 3 deletions(-) --- base-commit: 3b87ed7ea4d598c81a03317a92dfbd59102224fd change-id: 20220720-mt8183-keypad-20aa77106ff0 Best regards,