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

[Ming-Jen Chen]

On 2024/10/23 下午 04:40, Krzysztof Kozlowski wrote:
> On Tue, Oct 22, 2024 at 06:31:57AM +0000, mjchen wrote:
> > From: mjchen <mjchen@xxxxxxxxxxx>
> >
> > Add YAML bindings for MA35D1 SoC keypad.
> >
> > Signed-off-by: mjchen <mjchen@xxxxxxxxxxx>
> > ---
> >   .../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.
Sorry, I will remember to run checkpatch.pl before uploading the
subsequent patches and fix all errors and warnings
> > 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.
I will modify it to: nuvoton,ma35d1-keypad.yaml
> > @@ -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?
I will change NVT to Nuvoton
> > +
> > +maintainers:
> > +  - Ming-jen Chen <mjchen0829@xxxxxxxxx>
> > +
> > +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
I will update the definition to specify the debounce period in terms of 
keypad IP clock cycles, as follow:

nuvoton,debounce-period:
    type: integer
    enum: [0, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13]
    description: |
        Key debounce period select, specified in terms of keypad IP 
clock cycles.
        This value corresponds to the register setting for the keypad 
interface.
        The following values indicate the debounce time:
        - 0 = 0 clock cycles (no debounce)
        - 3 = 8 clock cycles
        - 4 = 16 clock cycles
        - 5 = 32 clock cycles
        - 6 = 64 clock cycles
        - 7 = 128 clock cycles
        - 8 = 256 clock cycles
        - 9 = 512 clock cycles
        - 10 = 1024 clock cycles
        - 11 = 2048 clock cycles
        - 12 = 4096 clock cycles
        - 13 = 8192 clock cycles
> > +
> > +  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?
pre-scale:
This value configures the IC register for the row scan cycle 
pre-scaling, with valid values ranging from 1 to 256
per-scalediv:(I will change pre-scalediv to pre-scale-div)
This will describe its role in setting the divisor for the row scan 
cycle pre-scaling, allowing for finer control over the keypad scanning 
frequency

I will change it to the following content:
nuvoton,pre-scale:
    type: uint32
    description: |
        Row Scan Cycle Pre-scale Value, used to pre-scale the row scan 
cycle. The valid range is from 1 to 256.
    minimum: 1
    maximum: 256

nuvoton,pre-scale-div:
    type: uint32
    description: |
        Divider for the pre-scale value, further adjusting the scan 
frequency for the keypad.
    minimum: 1
    maximum: 256
> > +
> > +  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@404A0000 {
> Lowercase hex and drop the unused label
I will modify it to: keypad@404a0000 {
> > +      compatible = "nuvoton,ma35d1-kpi";
> > +      reg = <0x404A0000 0x10000>;
> Lowercase hex
I will modify it to: reg = <0x404a0000 0x10000>
> Best regards,
> Krzysztof
Thank you for your guidance!
I look forward to your further comments.

Best regards,

Ming-Jen Chen