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

On Thu, Jul 21, 2022 at 11:16, Krzysztof Kozlowski [off-list ref] wrote:

On 21/07/2022 11:06, Mattijs Korpershoek wrote:

quoted

On Wed, Jul 20, 2022 at 19:14, Krzysztof Kozlowski [off-list ref] wrote:

quoted

On 20/07/2022 16:48, Mattijs Korpershoek wrote:

quoted

writing-bindings.rst states:

quoted

- 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.

quoted

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

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel