Re: [PATCH v2 01/11] dt-bindings: input: samsung,s3c6410-keypad: introduce compact binding

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Driver fails to probe and skips keymap parsing when using the newly introduced compact device tree binding.
- [Low] Suboptimal workaround for schema validation when importing matrix-keymap.yaml properties.
--

--- Patch [1]: [PATCH v2 01/11] dt-bindings: input: samsung,s3c6410-keypad: introduce compact binding ---
commit b727e1653e0c43f9e2d755869527a44de705430f
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

dt-bindings: input: samsung,s3c6410-keypad: introduce compact binding
    
The binding with a sub-node per each key is very verbose and is hard to
use with static device properties. Allow standard matrix keymap binding
in addition to the verbose one.

> diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> --- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> @@ -37,6 +37,10 @@ properties:
>  
>    wakeup-source: true
>  
> +  keypad,num-columns: true
> +  keypad,num-rows: true
> +  linux,keymap: true
> +

[Severity: Low]
Is it necessary to manually declare these properties?

Since they are already defined in matrix-keymap.yaml, using
unevaluatedProperties: false at the end of the schema instead of
additionalProperties: false should allow them to be evaluated
automatically without explicitly listing them here.

>    linux,input-no-autorepeat:
>      type: boolean
>      description:

[ ... ]

> @@ -81,12 +85,33 @@ patternProperties:
>        - keypad,row
>        - linux,code
>  
> +dependencies:
> +  linux,keymap: [ "keypad,num-columns", "keypad,num-rows" ]
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
> -  - samsung,keypad-num-columns
> -  - samsung,keypad-num-rows
> +
> +allOf:
> +  - $ref: input.yaml#
> +  - $ref: matrix-keymap.yaml#
> +  - if:
> +      required:
> +        - linux,keymap
> +    then:
> +      properties:
> +        samsung,keypad-num-columns: false
> +        samsung,keypad-num-rows: false

[Severity: High]
Will this cause the samsung-keypad driver to fail to probe when using
the new compact binding?

If a device tree drops the legacy properties as mandated here,
samsung_keypad_parse_dt() in drivers/input/keyboard/samsung-keypad.c
will fail to probe the device:

samsung_keypad_parse_dt() {
        ...
        of_property_read_u32(np, "samsung,keypad-num-rows", &num_rows);
        of_property_read_u32(np, "samsung,keypad-num-columns", &num_cols);
        if (!num_rows || !num_cols) {
                dev_err(dev, "number of keypad rows/columns not specified\n");
                return ERR_PTR(-EINVAL);
        }
        ...
}

Additionally, will the driver skip parsing the linux,keymap property
entirely?

The driver currently sizes the keymap based on the child node count, which
is zero for the compact matrix binding:

samsung_keypad_parse_dt() {
        ...
        keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
        ...
        key_count = of_get_child_count(np);
        keymap_data->keymap_size = key_count;
        ...
}

This causes matrix_keypad_build_keymap() to bypass parsing the linux,keymap
property, resulting in an empty keymap.

Does the driver need a corresponding update to support this binding change?

> +      patternProperties:
> +        '^key-[0-9a-z]+$': false
> +    else:
> +      properties:
> +        keypad,num-columns: false
> +        keypad,num-rows: false

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-samsung-kp-v2-0-3c6ed4c9b3b6@gmail.com?part=1