[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <871e9a4c-7a3c-4a24-8829-a079983033da@gmail.com>
Date: Fri, 25 Oct 2024 13:36:48 +0800
From: Ming-Jen Chen <mjchen0829@...il.com>
To: Krzysztof Kozlowski <krzk@...nel.org>
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 2024/10/23 下午 04:40, Krzysztof Kozlowski wrote:
> 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.
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@...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
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@...A0000 {
> Lowercase hex and drop the unused label
I will modify it to: keypad@...a0000 {
>
>> + 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
Powered by blists - more mailing lists