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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <csbechg6iarxx52z2gqidszhvgjdvaraoumpfcsozelhuuhmtb@ec7es3txuzxc>
Date: Wed, 23 Oct 2024 10:40:41 +0200
From: Krzysztof Kozlowski <krzk@...nel.org>
To: mjchen <mjchen0829@...il.com>
Cc: linux-kernel@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-input@...r.kernel.org, linux-arm-kernel@...ts.infradead.org, mjchen@...oton.com, 
	peng.fan@....com, sudeep.holla@....com, arnd@...db.de, conor+dt@...nel.org, 
	krzk+dt@...nel.org, robh@...nel.org, dmitry.torokhov@...il.com
Subject: Re: [PATCH 1/2] dt-bindings: input: Add Nuvoton MA35D1 keypad

On Tue, Oct 22, 2024 at 06:31:57AM +0000, mjchen wrote:
> From: mjchen <mjchen@...oton.com>
> 
> Add YAML bindings for MA35D1 SoC keypad.
> 
> Signed-off-by: mjchen <mjchen@...oton.com>
> ---
>  .../bindings/input/nvt,ma35d1-keypad.yaml     | 88 +++++++++++++++++++
>  1 file changed, 88 insertions(+)
>  create mode 100755 Documentation/devicetree/bindings/input/nvt,ma35d1-keypad.yaml
> 

Please run scripts/checkpatch.pl and fix reported warnings. Then please
run 'scripts/checkpatch.pl --strict' and (probably) fix more warnings.
Some warnings can be ignored, especially from --strict run, but the code
here looks like it needs a fix. Feel free to get in touch if the warning
is not clear.

> diff --git a/Documentation/devicetree/bindings/input/nvt,ma35d1-keypad.yaml b/Documentation/devicetree/bindings/input/nvt,ma35d1-keypad.yaml
> new file mode 100755
> index 000000000000..3d9fc26cc132
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/nvt,ma35d1-keypad.yaml

Filename based on compatible. There is no nvt prefix. Entire filename is
somehowdifferent than compatible.

> @@ -0,0 +1,88 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/nvt,ma35d1-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: NVT MA35D1 Keypad

NVT? Nuvoton?

> +
> +maintainers:
> +  - Ming-jen Chen <mjchen0829@...il.com>
> +
> +allOf:
> +  - $ref: /schemas/input/matrix-keymap.yaml#
> +
> +properties:
> +  compatible:
> +    const: nuvoton,ma35d1-kpi
> +
> +  debounce-period:
> +    $ref: /schemas/types.yaml#/definitions/uint32

Missing vendor prefix... or why are you not using existing properties?

> +    description: |
> +      key debounce period select

select? or clock cycles? I don't understand this. Say something useful
here.


> +      0  = 0 clock
> +      1  = 0 clock
> +      2  = 0 clock

Heh? So this is just 0

> +      3  = 8 clocks

This is 8

> +      4  = 16 clocks

16, not 4

> +      5  = 32 clocks
> +      6  = 64 clocks
> +      7  = 128 clocks
> +      8  = 256 clocks
> +      9  = 512 clocks
> +      10 = 1024 clocks
> +      11 = 2048 clocks
> +      12 = 4096 clocks
> +      13 = 8192 clocks

Use proper enum


> +
> +  per-scale:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Row Scan Cycle Pre-scale Value (1 to 256).

Missing constraints

> +
> +  per-scalediv:
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Per-scale divider (1 to 256).

Missing constraints

Both properties are unexpected... aren't you duplicating existing
properties?

> +
> +  reg:
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  clocks:
> +    maxItems: 1
> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - clocks
> +  - linux,keymap
> +  - debounce-period
> +  - per-scale
> +  - per-scalediv
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/input/input.h>
> +    keypad: keypad@...A0000 {

Lowercase hex and drop the unused label

> +      compatible = "nuvoton,ma35d1-kpi";
> +      reg = <0x404A0000 0x10000>;

Lowercase hex

Best regards,
Krzysztof