Re: [PATCH v1 1/2] dt-bindings: input: Add Nuvoton MA35D1 keypad controller

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Low] dt-bindings: input: ma35d1-keypad: redefine autorepeat instead
  of input.yaml#
- [Low] dt-bindings: input: ma35d1-keypad: omit resets property for
  MA35D1_RESET_KPI
- [Low] dt-bindings: input: ma35d1-keypad: bad clock/IRQ cells and align
  in example
--

commit 6eade3d3eb06d707c8d9f0ddfa65827ec7270074
Author: Feng-Yi Hsueh <fyhsueh4linux@gmail.com>

dt-bindings: input: Add Nuvoton MA35D1 keypad controller

This commit adds the Device Tree binding schema for the Nuvoton MA35D1
matrix keypad controller.

> diff --git a/Documentation/devicetree/bindings/input/nuvoton,ma35d1-keypad.yaml b/Documentation/devicetree/bindings/input/nuvoton,ma35d1-keypad.yaml
> new file mode 100644
> index 0000000000000..be678d0b38b98
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/nuvoton,ma35d1-keypad.yaml
> @@ -0,0 +1,73 @@
[ ... ]
> +allOf:
> +  - $ref: /schemas/input/matrix-keymap.yaml#
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-keypad
> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1

[Severity: Low]
Should properties also document resets (with maxItems: 1)?

include/dt-bindings/reset/nuvoton,ma35d1-reset.h defines MA35D1_RESET_KPI for
this controller, and drivers/reset/reset-ma35d1.c implements it in
ma35d1_reset_map.  Because this schema sets unevaluatedProperties: false, any
board or SoC devicetree node specifying resets = <&sys MA35D1_RESET_KPI> will
fail dtbs_check validation.

> +
> +  keypad,num-rows:
> +    minimum: 2
> +    maximum: 8
> +
> +  keypad,num-columns:
> +    minimum: 1
> +    maximum: 8
> +
> +  autorepeat:
> +    type: boolean
> +    description: Enable key autorepeat events.

[Severity: Low]
Should this schema reference input.yaml# in allOf instead of redefining
autorepeat locally?

Documentation/devicetree/bindings/input/input.yaml already defines autorepeat
with its type and description, so referencing input.yaml# in allOf (and at
most listing autorepeat: true here) avoids duplicating the common property
definition.

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - keypad,num-rows
> +  - keypad,num-columns
> +  - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/input/input.h>
> +
> +    keypad@404a0000 {
> +        compatible = "nuvoton,ma35d1-keypad";
> +        reg = <0x404a0000 0x10000>;
> +        interrupts = <79>;
> +        clocks = <&clk>;
> +
> +        keypad,num-rows = <4>;
> +        keypad,num-columns = <1>;
> +        linux,keymap = <
> +            MATRIX_KEY(0, 0, KEY_1)
> +            MATRIX_KEY(1, 0, KEY_2)
> +            MATRIX_KEY(2, 0, KEY_3)
> +            MATRIX_KEY(3, 0, KEY_4)
> +        >;
> +    };

[Severity: Low]
Should the example include the GIC interrupt specifier and the KPI_GATE clock
cell, and align the linux,keymap entries?

On MA35D1, the interrupt controller uses three GIC cells (such as
<GIC_SPI 79 IRQ_TYPE_LEVEL_HIGH>) and &clk takes a clock specifier cell
(<&clk KPI_GATE> from dt-bindings/clock/nuvoton,ma35d1-clk.h).  Also, the
multi-line linux,keymap entries are indented by four extra spaces rather than
aligned with the opening bracket, which triggers continuation-alignment
warnings from scripts/dtc/dt-check-style --mode=strict.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261110111419.89535-1-fyhsueh4linux@gmail.com?part=1