| Message ID | 20220720-mt8183-keypad-v1-2-ef9fc29dbff4@baylibre.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Input: mt6779-keypad - double keys support | expand |
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 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
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 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
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: - |