[PATCH 0/2] Add support for nuvoton ma35d1 keypad controller

[PATCH 0/2] Add support for nuvoton ma35d1 keypad controller

[mjchen]

From: mjchen <mjchen@nuvoton.com>

This patch series adds keypad driver for the nuvoton ma35 ARMv8 SoC.
It includes DT binding documentation and the ma35d1 keypad driver.

mjchen (2):
  dt-bindings: input: Add Nuvoton MA35D1 keypad
  input: keypad: add new keypad driver for MA35D1

 .../bindings/input/nvt,ma35d1-keypad.yaml     |  88 +++++
 drivers/input/keyboard/Kconfig                |  10 +
 drivers/input/keyboard/Makefile               |   1 +
 drivers/input/keyboard/ma35d1_keypad.c        | 312 ++++++++++++++++++
 4 files changed, 411 insertions(+)
 create mode 100755 Documentation/devicetree/bindings/input/nvt,ma35d1-keypad.yaml
 create mode 100644 drivers/input/keyboard/ma35d1_keypad.c

-- 
2.17.1

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

[mjchen]

From: mjchen <mjchen@nuvoton.com>

Add YAML bindings for MA35D1 SoC keypad.

Signed-off-by: mjchen <mjchen@nuvoton.com>
---
 .../bindings/input/nvt,ma35d1-keypad.yaml     | 88 +++++++++++++++++++
 1 file changed, 88 insertions(+)
 create mode 100755 Documentation/devicetree/bindings/input/nvt,ma35d1-keypad.yaml

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
@@ -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
+
+maintainers:
+  - Ming-jen Chen <mjchen0829@gmail.com>
+
+allOf:
+  - $ref: /schemas/input/matrix-keymap.yaml#
+
+properties:
+  compatible:
+    const: nuvoton,ma35d1-kpi
+
+  debounce-period:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: |
+      key debounce period select
+      0  = 0 clock
+      1  = 0 clock
+      2  = 0 clock
+      3  = 8 clocks
+      4  = 16 clocks
+      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
+
+  per-scale:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Row Scan Cycle Pre-scale Value (1 to 256).
+
+  per-scalediv:
+    $ref: /schemas/types.yaml#/definitions/uint32
+    description: Per-scale divider (1 to 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 {
+      compatible = "nuvoton,ma35d1-kpi";
+      reg = <0x404A0000 0x10000>;
+      interrupts = <79>;
+      clocks = <&clk>;
+      keypad,num-rows = <2>;
+      keypad,num-columns = <2>;
+
+      linux,keymap = <
+         MATRIX_KEY(0,  0, KEY_ENTER)
+         MATRIX_KEY(0,  1, KEY_ENTER)
+         MATRIX_KEY(1,  0, KEY_SPACE)
+         MATRIX_KEY(1,  1, KEY_Z)
+     >;
+
+     debounce-period = <1>;
+     per-scale = <1>;
+     per-scalediv = <24>;
+    };
-- 
2.17.1

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

[Krzysztof Kozlowski]

On Tue, Oct 22, 2024 at 06:31:57AM +0000, mjchen wrote:
> From: mjchen <mjchen@nuvoton.com>
> 
> Add YAML bindings for MA35D1 SoC keypad.
> 
> Signed-off-by: mjchen <mjchen@nuvoton.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@gmail.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@404A0000 {

Lowercase hex and drop the unused label

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

Lowercase hex

Best regards,
Krzysztof

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@nuvoton.com>
>>
>> Add YAML bindings for MA35D1 SoC keypad.
>>
>> Signed-off-by: mjchen <mjchen@nuvoton.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@gmail.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@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

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

[Krzysztof Kozlowski]

On 25/10/2024 07:36, Ming-Jen Chen wrote:
>>
>>> +      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

No. 0, 8, 16, 32 , 64 etc.

>>
>>
>>> +
>>> +  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)

Please look for matching existing properties first.

> 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


Best regards,
Krzysztof

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

[Ming-Jen Chen]

On 2024/10/25 下午 07:42, Krzysztof Kozlowski wrote:
> On 25/10/2024 07:36, Ming-Jen Chen wrote:
>>>> +      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
> No. 0, 8, 16, 32 , 64 etc.

I will change it to the following content:

nuvoton,debounce-period:
   type:  integer
   enum:  [0,8,16,32,64,128,256,512,1024,2048,4096,8192]
   description:  | Key debounce period select, specified in terms of keypad IP clock 
cycles. Valid values include 0 (no debounce) and specific clock cycle 
values: 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, and 8192.

>>>
>>>> +
>>>> +  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)
> Please look for matching existing properties first.

I will change it to the following content:

nuvoton,scan-time:
   type:  uint32
   description:  | Set the scan time for each key, in IP clock cycles. The valid range is 
from 1 to 256.    minimum:  1
   maximum:  256

nuvoton,scan-time-div:
   type:  uint32
   description:  | Divider for the scan-time, further adjusting the scan frequency for 
the keypad. The valid range is from 1 to 256.    minimum:  1
   maximum:  256

>> 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
>
> Best regards,
> Krzysztof

Best regards,

Ming-Jen Chen

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

[Krzysztof Kozlowski]

On 28/10/2024 02:15, Ming-Jen Chen wrote:
> 
> On 2024/10/25 下午 07:42, Krzysztof Kozlowski wrote:
>> On 25/10/2024 07:36, Ming-Jen Chen wrote:
>>>>> +      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
>> No. 0, 8, 16, 32 , 64 etc.
> 
> I will change it to the following content:
> 
> nuvoton,debounce-period:
>    type:  integer
>    enum:  [0,8,16,32,64,128,256,512,1024,2048,4096,8192]
>    description:  | Key debounce period select, specified in terms of keypad IP clock 
> cycles. Valid values include 0 (no debounce) and specific clock cycle 
> values: 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, and 8192.
> 
>>>>
>>>>> +
>>>>> +  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)
>> Please look for matching existing properties first.
> 
> I will change it to the following content:
> 
> nuvoton,scan-time:

Why? What about my request?


Best regards,
Krzysztof

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

[Ming-Jen Chen]

On 2024/10/28 下午 03:04, Krzysztof Kozlowski wrote:

> On 28/10/2024 02:15, Ming-Jen Chen wrote:
>> On 2024/10/25 下午 07:42, Krzysztof Kozlowski wrote:
>>> On 25/10/2024 07:36, Ming-Jen Chen wrote:
>>>>>> +      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
>>> No. 0, 8, 16, 32 , 64 etc.
>> I will change it to the following content:
>>
>> nuvoton,debounce-period:
>>     type:  integer
>>     enum:  [0,8,16,32,64,128,256,512,1024,2048,4096,8192]
>>     description:  | Key debounce period select, specified in terms of keypad IP clock
>> cycles. Valid values include 0 (no debounce) and specific clock cycle
>> values: 8, 16, 32, 64, 128, 256, 512, 1024, 2048, 4096, and 8192.
>>
>>>>>> +
>>>>>> +  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)
>>> Please look for matching existing properties first.
>> I will change it to the following content:
>>
>> nuvoton,scan-time:
> Why? What about my request?

I utilized|grep|  to search for relevant properties in the|input/|  folder using keywords such as|scan|,|time|,|period|,|freq|, and|interval|.
While I found some similar properties, I did not locate any that completely meet my requirements.

For example, I found|"scanning_period"|, which is described as "Time between scans. Each step is 1024 us. Valid 1-256."
I would like to confirm if you are suggesting that I use|scanning_period|  and explain my specific use case in the description,
for example:

nuvoton,scanning-period:
     type:  uint32
     description:  | Set the scan time for each key, specified in terms of keypad IP clock 
cycles. The valid range is from 1 to 256.      minimum:  1
     maximum:  256 Could you please confirm if this approach aligns with your suggestion,
  or if you have any other recommended existing properties?

Thank you for your assistance!

>
>
> Best regards,
> Krzysztof

Best regards,
Ming-Jen Chen

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

[Krzysztof Kozlowski]

On 29/10/2024 03:00, Ming-Jen Chen wrote:
>>>>>>> +
>>>>>>> +  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)
>>>> Please look for matching existing properties first.
>>> I will change it to the following content:
>>>
>>> nuvoton,scan-time:
>> Why? What about my request?
> 
> I utilized|grep|  to search for relevant properties in the|input/|  folder using keywords such as|scan|,|time|,|period|,|freq|, and|interval|.
> While I found some similar properties, I did not locate any that completely meet my requirements.
> 
> For example, I found|"scanning_period"|, which is described as "Time between scans. Each step is 1024 us. Valid 1-256."
> I would like to confirm if you are suggesting that I use|scanning_period|  and explain my specific use case in the description,
> for example:

Description of these properties did not tell me much about their purpose
and underlying hardware, so I don't know which fits here. It looks like
you want to configure clock... but then wording confuses me -
"per-scale". What is "per"? Isn't it usually "pre"?

So in general I don't know what to recommend you because your patch is
really unclear.

Please also wrap emails according to mailing lists standards. And use
proper line separation of sentences. It's really hard to understand your
email.

> 
> nuvoton,scanning-period:
>      type:  uint32
>      description:  | Set the scan time for each key, specified in terms of keypad IP clock 
> cycles. The valid range is from 1 to 256.      minimum:  1
>      maximum:  256 Could you please confirm if this approach aligns with your suggestion,
>   or if you have any other recommended existing properties?

Why this would be board dependent?

Best regards,
Krzysztof

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

[Ming-Jen Chen]

On 2024/10/29 下午 09:19, Krzysztof Kozlowski wrote:
> On 29/10/2024 03:00, Ming-Jen Chen wrote:
>>>>>>>> +
>>>>>>>> +  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)
>>>>> Please look for matching existing properties first.
>>>> I will change it to the following content:
>>>>
>>>> nuvoton,scan-time:
>>> Why? What about my request?
>> I utilized|grep|  to search for relevant properties in the|input/|  folder using keywords such as|scan|,|time|,|period|,|freq|, and|interval|.
>> While I found some similar properties, I did not locate any that completely meet my requirements.
>>
>> For example, I found|"scanning_period"|, which is described as "Time between scans. Each step is 1024 us. Valid 1-256."
>> I would like to confirm if you are suggesting that I use|scanning_period|  and explain my specific use case in the description,
>> for example:
> Description of these properties did not tell me much about their purpose
> and underlying hardware, so I don't know which fits here. It looks like
> you want to configure clock... but then wording confuses me -
> "per-scale". What is "per"? Isn't it usually "pre"?
>
> So in general I don't know what to recommend you because your patch is
> really unclear.
>
> Please also wrap emails according to mailing lists standards. And use
> proper line separation of sentences. It's really hard to understand your
> email.

I apologize for any confusion caused by my previous responses regarding 
this issue.
It seems that our discussion has reached a bit of a bottleneck.

I have a suggestion that I hope you might agree with: I would like to 
upload version 2 of the code.
In this version, I will rewrite the properties, although it may not 
resolve their underlying issues.
I will also continue to keep our current discussion ongoing in version 2.

Thank you for your understanding, and I look forward to your thoughts on 
this approach


>
>> nuvoton,scanning-period:
>>       type:  uint32
>>       description:  | Set the scan time for each key, specified in terms of keypad IP clock
>> cycles. The valid range is from 1 to 256.      minimum:  1
>>       maximum:  256 Could you please confirm if this approach aligns with your suggestion,
>>    or if you have any other recommended existing properties?
> Why this would be board dependent?
>
> Best regards,
> Krzysztof
>

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

[Krzysztof Kozlowski]

On 30/10/2024 02:46, Ming-Jen Chen wrote:
>> Description of these properties did not tell me much about their purpose
>> and underlying hardware, so I don't know which fits here. It looks like
>> you want to configure clock... but then wording confuses me -
>> "per-scale". What is "per"? Isn't it usually "pre"?
>>
>> So in general I don't know what to recommend you because your patch is
>> really unclear.
>>
>> Please also wrap emails according to mailing lists standards. And use
>> proper line separation of sentences. It's really hard to understand your
>> email.
> 
> I apologize for any confusion caused by my previous responses regarding 
> this issue.
> It seems that our discussion has reached a bit of a bottleneck.
> 
> I have a suggestion that I hope you might agree with: I would like to 
> upload version 2 of the code.
> In this version, I will rewrite the properties, although it may not 
> resolve their underlying issues.
> I will also continue to keep our current discussion ongoing in version 2.
> 
> Thank you for your understanding, and I look forward to your thoughts on 
> this approach

Sure, let's try that.

Best regards,
Krzysztof

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

[Krzysztof Kozlowski]

On 22/10/2024 08:31, mjchen wrote:
> From: mjchen <mjchen@nuvoton.com>
> 
> Add YAML bindings for MA35D1 SoC keypad.
> 
> Signed-off-by: mjchen <mjchen@nuvoton.com>

Don't use your login name as name, but use your known identity or full
legal name.

Best regards,
Krzysztof

[PATCH 2/2] input: keypad: add new keypad driver for MA35D1

[mjchen]

From: mjchen <mjchen@nuvoton.com>

Adds a new keypad driver for the MA35D1 platform.
The driver supports key scanning and interrupt handling.

Signed-off-by: mjchen <mjchen@nuvoton.com>
---
 drivers/input/keyboard/Kconfig         |  10 +
 drivers/input/keyboard/Makefile        |   1 +
 drivers/input/keyboard/ma35d1_keypad.c | 312 +++++++++++++++++++++++++
 3 files changed, 323 insertions(+)
 create mode 100644 drivers/input/keyboard/ma35d1_keypad.c

diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
index 721ab69e84ac..ce9bd5cc13a1 100644
--- a/drivers/input/keyboard/Kconfig
+++ b/drivers/input/keyboard/Kconfig
@@ -797,4 +797,14 @@ config KEYBOARD_CYPRESS_SF
 	  To compile this driver as a module, choose M here: the
 	  module will be called cypress-sf.
 
+config KEYBOARD_MA35D1
+	tristate "Nuvoton MA35D1 keypad driver"
+	depends on ARCH_MA35
+	select INPUT_MATRIXKMAP
+	help
+	  Say Y here if you want to use Nuvoton MA35D1 keypad.
+
+	  To compile this driver as a module, choose M here: the
+	  module will be called ma35d1-keypad.
+
 endif
diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
index 1e0721c30709..9b858cdd1b6b 100644
--- a/drivers/input/keyboard/Makefile
+++ b/drivers/input/keyboard/Makefile
@@ -70,3 +70,4 @@ obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
 obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)	+= tm2-touchkey.o
 obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
 obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
+obj-$(CONFIG_KEYBOARD_MA35D1)		+= ma35d1_keypad.o
diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
new file mode 100644
index 000000000000..20b5b1b91127
--- /dev/null
+++ b/drivers/input/keyboard/ma35d1_keypad.c
@@ -0,0 +1,312 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ *  MA35D1 keypad driver
+ *  Copyright (C) 2024 Nuvoton Technology Corp.
+ */
+
+#include <linux/interrupt.h>
+#include <linux/input.h>
+#include <linux/platform_device.h>
+#include <linux/input/matrix_keypad.h>
+#include <linux/clk.h>
+#include <linux/of.h>
+
+/* Keypad Interface Registers */
+#define KPI_CONF		0x00
+#define KPI_3KCONF		0x04
+#define KPI_STATUS		0x08
+#define KPI_RSTC		0x0C
+#define KPI_KEST		0x10
+#define KPI_KPE0		0x18
+#define KPI_KPE1		0x1C
+#define KPI_KRE0		0x20
+#define KPI_KRE1		0x24
+#define KPI_PRESCALDIV	0x28
+
+/* KPI_CONF - Keypad Configuration Register */
+#define KROW		GENMASK(30, 28) /* Keypad Matrix ROW number */
+#define KCOL		GENMASK(26, 24) /* Keypad Matrix COL Number */
+#define DB_CLKSEL	GENMASK(19, 16) /* De-bounce sampling cycle selection */
+#define PRESCALE	GENMASK(15, 8)  /* Row Scan Cycle Pre-scale Value */
+#define WAKEUP		BIT(5) /* Lower Power Wakeup Enable */
+#define INTEN		BIT(3) /* Key Interrupt Enable Control */
+#define RKINTEN		BIT(2) /* Release Key Interrupt Enable */
+#define PKINTEN		BIT(1) /* Press Key Interrupt Enable Control */
+#define ENKP		BIT(0) /* Keypad Scan Enable */
+
+/* KPI_STATUS - Keypad Status Register */
+#define PKEY_INT	BIT(4) /* Press key interrupt */
+#define RKEY_INT	BIT(3) /* Release key interrupt */
+#define KEY_INT		BIT(2) /* Key Interrupt */
+#define RST_3KEY	BIT(1) /* 3-Keys Reset Flag */
+#define PDWAKE		BIT(0) /* Power Down Wakeup Flag */
+
+#define DEFAULT_DEBOUNCE		1
+#define DEFAULT_PRE_SCALE		1
+#define DEFAULT_PRE_SCALEDIV	32
+
+struct ma35d1_keypad {
+	struct clk *clk;
+	struct input_dev *input_dev;
+	void __iomem *mmio_base;
+	int irq;
+	unsigned int kpi_row;
+	unsigned int kpi_col;
+	unsigned int debounce_val;
+	unsigned int pre_scale;
+	unsigned int pre_scale_divider;
+};
+
+static void ma35d1_keypad_scan_matrix(struct ma35d1_keypad *keypad,	unsigned int status)
+{
+	struct input_dev *input_dev = keypad->input_dev;
+	unsigned int i, j;
+	unsigned int row_add = 0;
+	unsigned int code;
+	unsigned int key;
+	unsigned int press_key;
+	unsigned long KeyEvent[4];
+	unsigned int row_shift = get_count_order(keypad->kpi_col);
+	unsigned short *keymap = input_dev->keycode;
+
+	/* Read key event status */
+	KeyEvent[0] = readl(keypad->mmio_base + KPI_KPE0);
+	KeyEvent[1] = readl(keypad->mmio_base + KPI_KPE1);
+	KeyEvent[2] = readl(keypad->mmio_base + KPI_KRE0);
+	KeyEvent[3] = readl(keypad->mmio_base + KPI_KRE1);
+
+	/* Clear key event status */
+	writel(KeyEvent[0], (keypad->mmio_base + KPI_KPE0));
+	writel(KeyEvent[1], (keypad->mmio_base + KPI_KPE1));
+	writel(KeyEvent[2], (keypad->mmio_base + KPI_KRE0));
+	writel(KeyEvent[3], (keypad->mmio_base + KPI_KRE1));
+
+	for (j = 0; j < 4; j++) {
+		if (KeyEvent[j] != 0) {
+			row_add = (j % 2) ? 4 : 0;
+			press_key = (j < 2) ? 1 : 0;
+
+			for (i = 0; i < 32; i++) {
+				if (KeyEvent[j] & (1<<i)) {
+					code = MATRIX_SCAN_CODE(((i/8) + row_add), (i % 8), row_shift);
+					key = keymap[code];
+
+					input_event(input_dev, EV_MSC, MSC_SCAN, code);
+					input_report_key(input_dev, key, press_key);
+				}
+			}
+		}
+	}
+
+	input_sync(input_dev);
+}
+
+static irqreturn_t ma35d1_keypad_interrupt(int irq, void *dev_id)
+{
+	struct ma35d1_keypad *keypad = dev_id;
+	unsigned int  kstatus;
+
+	kstatus = readl(keypad->mmio_base + KPI_STATUS);
+
+	if (kstatus & (PKEY_INT|RKEY_INT)) {
+		ma35d1_keypad_scan_matrix(keypad, kstatus);
+	} else {
+		if (kstatus & PDWAKE)
+			writel(PDWAKE, (keypad->mmio_base + KPI_STATUS));
+	}
+
+	return IRQ_HANDLED;
+}
+
+static int ma35d1_keypad_open(struct input_dev *dev)
+{
+	struct ma35d1_keypad *keypad = input_get_drvdata(dev);
+	unsigned int val, config;
+
+	val = RKINTEN | PKINTEN | INTEN | ENKP;
+	val |= FIELD_PREP(KCOL, (keypad->kpi_col - 1)) | FIELD_PREP(KROW, (keypad->kpi_row - 1));
+
+	if (keypad->debounce_val > 0)
+		config = FIELD_PREP(PRESCALE, (keypad->pre_scale - 1)) |
+				 FIELD_PREP(DB_CLKSEL, keypad->debounce_val);
+	else
+		config = FIELD_PREP(PRESCALE, (keypad->pre_scale - 1));
+
+	val |= config;
+
+	writel(val, keypad->mmio_base + KPI_CONF);
+	writel((keypad->pre_scale_divider - 1),	keypad->mmio_base + KPI_PRESCALDIV);
+
+	return 0;
+}
+
+static void ma35d1_keypad_close(struct input_dev *dev)
+{
+	struct ma35d1_keypad *keypad = input_get_drvdata(dev);
+
+	clk_disable(keypad->clk);
+}
+
+static int ma35d1_keypad_probe(struct platform_device *pdev)
+{
+	struct ma35d1_keypad *keypad;
+	struct input_dev *input_dev;
+	struct resource *res;
+	int error = 0;
+
+	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
+	if (!keypad)
+		return -ENOMEM;
+
+
+	input_dev = input_allocate_device();
+	if (!input_dev) {
+		dev_err(&pdev->dev, "failed to allocate input device\n");
+		return -ENOMEM;
+	}
+
+	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+	if (!res) {
+		dev_err(&pdev->dev, "failed to get I/O memory\n");
+		error = -ENXIO;
+		goto failed_free_input;
+	}
+
+	keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res);
+	if (IS_ERR(keypad->mmio_base)) {
+		dev_err(&pdev->dev, "failed to remap I/O memory\n");
+		return PTR_ERR(keypad->mmio_base);
+	}
+
+	keypad->irq = platform_get_irq(pdev, 0);
+	if (keypad->irq < 0) {
+		dev_err(&pdev->dev, "failed to get IRQ\n");
+		return keypad->irq;
+	}
+
+	keypad->clk = devm_clk_get(&pdev->dev, NULL);
+	if (IS_ERR(keypad->clk)) {
+		dev_err(&pdev->dev, "failed to get core clk: %ld\n", PTR_ERR(keypad->clk));
+		return PTR_ERR(keypad->clk);
+	}
+
+	error = matrix_keypad_parse_properties(&pdev->dev,
+										   &(keypad->kpi_row),
+										   &(keypad->kpi_col));
+	if (error) {
+		dev_err(&pdev->dev, "failed to parse kp params\n");
+		return error;
+	}
+
+	error = matrix_keypad_build_keymap(NULL, NULL,
+									   keypad->kpi_row,
+									   keypad->kpi_col,
+									   NULL, input_dev);
+	if (error) {
+		dev_err(&pdev->dev, "failed to build keymap\n");
+		return error;
+	}
+
+	keypad->input_dev = input_dev;
+	input_dev->name = pdev->name;
+	input_dev->id.bustype = BUS_HOST;
+	input_dev->open = ma35d1_keypad_open;
+	input_dev->close = ma35d1_keypad_close;
+	input_dev->dev.parent = &pdev->dev;
+
+	if (of_property_read_u32(pdev->dev.of_node, "debounce-period", &(keypad->debounce_val)))
+		keypad->debounce_val = DEFAULT_DEBOUNCE;
+
+	if (of_property_read_u32(pdev->dev.of_node, "per-scale", &(keypad->pre_scale)))
+		keypad->pre_scale = DEFAULT_PRE_SCALE;
+
+	if (of_property_read_u32(pdev->dev.of_node, "per-scalediv", &(keypad->pre_scale_divider)))
+		keypad->pre_scale_divider = DEFAULT_PRE_SCALEDIV;
+
+	__set_bit(EV_REP, input_dev->evbit);
+	input_set_drvdata(input_dev, keypad);
+	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
+
+	error = input_register_device(input_dev);
+	if (error) {
+		dev_err(&pdev->dev, "failed to register input device\n");
+		goto failed_free_input;
+	}
+
+	error = devm_request_irq(&pdev->dev, keypad->irq,
+							 ma35d1_keypad_interrupt,
+							 IRQF_NO_SUSPEND, pdev->name, keypad);
+	if (error) {
+		dev_err(&pdev->dev, "failed to request IRQ\n");
+		goto failed_unregister_input;
+	}
+
+	platform_set_drvdata(pdev, keypad);
+	device_init_wakeup(&pdev->dev, 1);
+	clk_prepare_enable(keypad->clk);
+
+	return 0;
+
+failed_unregister_input:
+	input_unregister_device(input_dev);
+failed_free_input:
+	input_free_device(input_dev);
+	return error;
+}
+
+static void ma35d1_keypad_remove(struct platform_device *pdev)
+{
+	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
+
+	input_unregister_device(keypad->input_dev);
+	clk_disable_unprepare(keypad->clk);
+}
+
+static int ma35d1_keypad_suspend(struct platform_device *pdev,
+									pm_message_t state)
+{
+	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev)) {
+		writel(readl(keypad->mmio_base + KPI_CONF) | WAKEUP, keypad->mmio_base + KPI_CONF);
+		enable_irq_wake(keypad->irq);
+	}
+
+	return 0;
+}
+
+static int ma35d1_keypad_resume(struct platform_device *pdev)
+{
+	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
+
+	if (device_may_wakeup(&pdev->dev)) {
+		writel(readl(keypad->mmio_base + KPI_CONF) & ~(WAKEUP),
+						keypad->mmio_base + KPI_CONF);
+		disable_irq_wake(keypad->irq);
+	}
+
+	return 0;
+}
+
+static const struct of_device_id ma35d1_kpi_of_match[] = {
+	{ .compatible = "nuvoton,ma35d1-kpi"},
+	{},
+};
+MODULE_DEVICE_TABLE(of, ma35d1_kpi_of_match);
+
+static struct platform_driver ma35d1_keypad_driver = {
+	.probe		= ma35d1_keypad_probe,
+	.remove		= ma35d1_keypad_remove,
+	.suspend	= ma35d1_keypad_suspend,
+	.resume		= ma35d1_keypad_resume,
+	.driver		= {
+		.name	= "ma35d1-kpi",
+		.of_match_table = of_match_ptr(ma35d1_kpi_of_match),
+	},
+};
+module_platform_driver(ma35d1_keypad_driver);
+
+MODULE_AUTHOR("Ming-Jen Chen");
+MODULE_DESCRIPTION("MA35D1 Keypad Driver");
+MODULE_LICENSE("GPL");
+
-- 
2.17.1

Re: [PATCH 2/2] input: keypad: add new keypad driver for MA35D1

[Krzysztof Kozlowski]

On Tue, Oct 22, 2024 at 06:31:58AM +0000, mjchen wrote:
> From: mjchen <mjchen@nuvoton.com>
> 
> Adds a new keypad driver for the MA35D1 platform.
> The driver supports key scanning and interrupt handling.
> 
> Signed-off-by: mjchen <mjchen@nuvoton.com>
> ---
>  drivers/input/keyboard/Kconfig         |  10 +
>  drivers/input/keyboard/Makefile        |   1 +
>  drivers/input/keyboard/ma35d1_keypad.c | 312 +++++++++++++++++++++++++
>  3 files changed, 323 insertions(+)
>  create mode 100644 drivers/input/keyboard/ma35d1_keypad.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 721ab69e84ac..ce9bd5cc13a1 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -797,4 +797,14 @@ config KEYBOARD_CYPRESS_SF
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cypress-sf.
>  
> +config KEYBOARD_MA35D1
> +	tristate "Nuvoton MA35D1 keypad driver"
> +	depends on ARCH_MA35

|| COMPILE_TEST

> +	select INPUT_MATRIXKMAP
> +	help
> +	  Say Y here if you want to use Nuvoton MA35D1 keypad.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ma35d1-keypad.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 1e0721c30709..9b858cdd1b6b 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -70,3 +70,4 @@ obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
>  obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)	+= tm2-touchkey.o
>  obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
> +obj-$(CONFIG_KEYBOARD_MA35D1)		+= ma35d1_keypad.o
> diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
> new file mode 100644
> index 000000000000..20b5b1b91127
> --- /dev/null
> +++ b/drivers/input/keyboard/ma35d1_keypad.c
> @@ -0,0 +1,312 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  MA35D1 keypad driver
> + *  Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +
> +/* Keypad Interface Registers */
> +#define KPI_CONF		0x00
> +#define KPI_3KCONF		0x04
> +#define KPI_STATUS		0x08
> +#define KPI_RSTC		0x0C
> +#define KPI_KEST		0x10
> +#define KPI_KPE0		0x18
> +#define KPI_KPE1		0x1C
> +#define KPI_KRE0		0x20
> +#define KPI_KRE1		0x24
> +#define KPI_PRESCALDIV	0x28
> +
> +/* KPI_CONF - Keypad Configuration Register */
> +#define KROW		GENMASK(30, 28) /* Keypad Matrix ROW number */
> +#define KCOL		GENMASK(26, 24) /* Keypad Matrix COL Number */
> +#define DB_CLKSEL	GENMASK(19, 16) /* De-bounce sampling cycle selection */
> +#define PRESCALE	GENMASK(15, 8)  /* Row Scan Cycle Pre-scale Value */
> +#define WAKEUP		BIT(5) /* Lower Power Wakeup Enable */
> +#define INTEN		BIT(3) /* Key Interrupt Enable Control */
> +#define RKINTEN		BIT(2) /* Release Key Interrupt Enable */
> +#define PKINTEN		BIT(1) /* Press Key Interrupt Enable Control */
> +#define ENKP		BIT(0) /* Keypad Scan Enable */
> +
> +/* KPI_STATUS - Keypad Status Register */
> +#define PKEY_INT	BIT(4) /* Press key interrupt */
> +#define RKEY_INT	BIT(3) /* Release key interrupt */
> +#define KEY_INT		BIT(2) /* Key Interrupt */
> +#define RST_3KEY	BIT(1) /* 3-Keys Reset Flag */
> +#define PDWAKE		BIT(0) /* Power Down Wakeup Flag */
> +
> +#define DEFAULT_DEBOUNCE		1
> +#define DEFAULT_PRE_SCALE		1
> +#define DEFAULT_PRE_SCALEDIV	32
> +
> +struct ma35d1_keypad {
> +	struct clk *clk;
> +	struct input_dev *input_dev;
> +	void __iomem *mmio_base;
> +	int irq;
> +	unsigned int kpi_row;
> +	unsigned int kpi_col;
> +	unsigned int debounce_val;
> +	unsigned int pre_scale;
> +	unsigned int pre_scale_divider;
> +};
> +
> +static void ma35d1_keypad_scan_matrix(struct ma35d1_keypad *keypad,	unsigned int status)
> +{
> +	struct input_dev *input_dev = keypad->input_dev;
> +	unsigned int i, j;
> +	unsigned int row_add = 0;
> +	unsigned int code;
> +	unsigned int key;
> +	unsigned int press_key;
> +	unsigned long KeyEvent[4];

No Windows or C++ code, please.

> +	unsigned int row_shift = get_count_order(keypad->kpi_col);
> +	unsigned short *keymap = input_dev->keycode;
> +

...

> +static int ma35d1_keypad_probe(struct platform_device *pdev)
> +{
> +	struct ma35d1_keypad *keypad;
> +	struct input_dev *input_dev;
> +	struct resource *res;
> +	int error = 0;
> +
> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> +	if (!keypad)
> +		return -ENOMEM;
> +
> +
> +	input_dev = input_allocate_device();
> +	if (!input_dev) {
> +		dev_err(&pdev->dev, "failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get I/O memory\n");
> +		error = -ENXIO;
> +		goto failed_free_input;
> +	}
> +
> +	keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(keypad->mmio_base)) {
> +		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> +		return PTR_ERR(keypad->mmio_base);
> +	}
> +
> +	keypad->irq = platform_get_irq(pdev, 0);
> +	if (keypad->irq < 0) {
> +		dev_err(&pdev->dev, "failed to get IRQ\n");
> +		return keypad->irq;
> +	}
> +
> +	keypad->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(keypad->clk)) {
> +		dev_err(&pdev->dev, "failed to get core clk: %ld\n", PTR_ERR(keypad->clk));

Syntax is: return dev_err_probe, except that your error handling code is a mess.
Earlier you have gotos, now return. Organize it nicely, so this will
follow some logical concept.

> +		return PTR_ERR(keypad->clk);
> +	}
> +
> +	error = matrix_keypad_parse_properties(&pdev->dev,
> +										   &(keypad->kpi_row),
> +										   &(keypad->kpi_col));

How did you aligned it?

> +	if (error) {
> +		dev_err(&pdev->dev, "failed to parse kp params\n");
> +		return error;
> +	}
> +
> +	error = matrix_keypad_build_keymap(NULL, NULL,
> +									   keypad->kpi_row,
> +									   keypad->kpi_col,
> +									   NULL, input_dev);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to build keymap\n");
> +		return error;
> +	}
> +
> +	keypad->input_dev = input_dev;
> +	input_dev->name = pdev->name;
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->open = ma35d1_keypad_open;
> +	input_dev->close = ma35d1_keypad_close;
> +	input_dev->dev.parent = &pdev->dev;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "debounce-period", &(keypad->debounce_val)))
> +		keypad->debounce_val = DEFAULT_DEBOUNCE;

This is not used anywhere. Drop dead code.

> +
> +	if (of_property_read_u32(pdev->dev.of_node, "per-scale", &(keypad->pre_scale)))
> +		keypad->pre_scale = DEFAULT_PRE_SCALE;

Not better...

> +
> +	if (of_property_read_u32(pdev->dev.of_node, "per-scalediv", &(keypad->pre_scale_divider)))
> +		keypad->pre_scale_divider = DEFAULT_PRE_SCALEDIV;

Still not better...

So there are defaults? Why these are required by bindings? Why bindings do not say
defaults?

> +
> +	__set_bit(EV_REP, input_dev->evbit);
> +	input_set_drvdata(input_dev, keypad);
> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		goto failed_free_input;
> +	}
> +
> +	error = devm_request_irq(&pdev->dev, keypad->irq,
> +							 ma35d1_keypad_interrupt,
> +							 IRQF_NO_SUSPEND, pdev->name, keypad);

Totally mesed alignment.

> +	if (error) {
> +		dev_err(&pdev->dev, "failed to request IRQ\n");
> +		goto failed_unregister_input;
> +	}
> +
> +	platform_set_drvdata(pdev, keypad);
> +	device_init_wakeup(&pdev->dev, 1);
> +	clk_prepare_enable(keypad->clk);
> +
> +	return 0;
> +
> +failed_unregister_input:
> +	input_unregister_device(input_dev);
> +failed_free_input:
> +	input_free_device(input_dev);
> +	return error;
> +}
> +
> +static void ma35d1_keypad_remove(struct platform_device *pdev)
> +{
> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	input_unregister_device(keypad->input_dev);
> +	clk_disable_unprepare(keypad->clk);

Why aren't you using devm_clk_get_enabled()?

> +}
> +
> +static int ma35d1_keypad_suspend(struct platform_device *pdev,
> +									pm_message_t state)
> +{
> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		writel(readl(keypad->mmio_base + KPI_CONF) | WAKEUP, keypad->mmio_base + KPI_CONF);
> +		enable_irq_wake(keypad->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static int ma35d1_keypad_resume(struct platform_device *pdev)
> +{
> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		writel(readl(keypad->mmio_base + KPI_CONF) & ~(WAKEUP),
> +						keypad->mmio_base + KPI_CONF);
> +		disable_irq_wake(keypad->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ma35d1_kpi_of_match[] = {
> +	{ .compatible = "nuvoton,ma35d1-kpi"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ma35d1_kpi_of_match);
> +
> +static struct platform_driver ma35d1_keypad_driver = {
> +	.probe		= ma35d1_keypad_probe,
> +	.remove		= ma35d1_keypad_remove,
> +	.suspend	= ma35d1_keypad_suspend,
> +	.resume		= ma35d1_keypad_resume,
> +	.driver		= {
> +		.name	= "ma35d1-kpi",
> +		.of_match_table = of_match_ptr(ma35d1_kpi_of_match),

Drop of_match_ptr(), you have warnings here.

Best regards,
Krzysztof

Re: [PATCH 2/2] input: keypad: add new keypad driver for MA35D1

[Ming-Jen Chen]

On 2024/10/23 下午 04:45, Krzysztof Kozlowski wrote:
> On Tue, Oct 22, 2024 at 06:31:58AM +0000, mjchen wrote:
>> From: mjchen <mjchen@nuvoton.com>
>>
>> Adds a new keypad driver for the MA35D1 platform.
>> The driver supports key scanning and interrupt handling.
>>
>> Signed-off-by: mjchen <mjchen@nuvoton.com>
>> ---
>>   drivers/input/keyboard/Kconfig         |  10 +
>>   drivers/input/keyboard/Makefile        |   1 +
>>   drivers/input/keyboard/ma35d1_keypad.c | 312 +++++++++++++++++++++++++
>>   3 files changed, 323 insertions(+)
>>   create mode 100644 drivers/input/keyboard/ma35d1_keypad.c
>>
>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>> index 721ab69e84ac..ce9bd5cc13a1 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -797,4 +797,14 @@ config KEYBOARD_CYPRESS_SF
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called cypress-sf.
>>   
>> +config KEYBOARD_MA35D1
>> +	tristate "Nuvoton MA35D1 keypad driver"
>> +	depends on ARCH_MA35
> || COMPILE_TEST


I will modify to: depends on ARCH_MA35 || COMPILE_TEST

>
>> +	select INPUT_MATRIXKMAP
>> +	help
>> +	  Say Y here if you want to use Nuvoton MA35D1 keypad.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called ma35d1-keypad.
>> +
>>   endif
>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>> index 1e0721c30709..9b858cdd1b6b 100644
>> --- a/drivers/input/keyboard/Makefile
>> +++ b/drivers/input/keyboard/Makefile
>> @@ -70,3 +70,4 @@ obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
>>   obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)	+= tm2-touchkey.o
>>   obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
>>   obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
>> +obj-$(CONFIG_KEYBOARD_MA35D1)		+= ma35d1_keypad.o
>> diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
>> new file mode 100644
>> index 000000000000..20b5b1b91127
>> --- /dev/null
>> +++ b/drivers/input/keyboard/ma35d1_keypad.c
>> @@ -0,0 +1,312 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + *  MA35D1 keypad driver
>> + *  Copyright (C) 2024 Nuvoton Technology Corp.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/input.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/input/matrix_keypad.h>
>> +#include <linux/clk.h>
>> +#include <linux/of.h>
>> +
>> +/* Keypad Interface Registers */
>> +#define KPI_CONF		0x00
>> +#define KPI_3KCONF		0x04
>> +#define KPI_STATUS		0x08
>> +#define KPI_RSTC		0x0C
>> +#define KPI_KEST		0x10
>> +#define KPI_KPE0		0x18
>> +#define KPI_KPE1		0x1C
>> +#define KPI_KRE0		0x20
>> +#define KPI_KRE1		0x24
>> +#define KPI_PRESCALDIV	0x28
>> +
>> +/* KPI_CONF - Keypad Configuration Register */
>> +#define KROW		GENMASK(30, 28) /* Keypad Matrix ROW number */
>> +#define KCOL		GENMASK(26, 24) /* Keypad Matrix COL Number */
>> +#define DB_CLKSEL	GENMASK(19, 16) /* De-bounce sampling cycle selection */
>> +#define PRESCALE	GENMASK(15, 8)  /* Row Scan Cycle Pre-scale Value */
>> +#define WAKEUP		BIT(5) /* Lower Power Wakeup Enable */
>> +#define INTEN		BIT(3) /* Key Interrupt Enable Control */
>> +#define RKINTEN		BIT(2) /* Release Key Interrupt Enable */
>> +#define PKINTEN		BIT(1) /* Press Key Interrupt Enable Control */
>> +#define ENKP		BIT(0) /* Keypad Scan Enable */
>> +
>> +/* KPI_STATUS - Keypad Status Register */
>> +#define PKEY_INT	BIT(4) /* Press key interrupt */
>> +#define RKEY_INT	BIT(3) /* Release key interrupt */
>> +#define KEY_INT		BIT(2) /* Key Interrupt */
>> +#define RST_3KEY	BIT(1) /* 3-Keys Reset Flag */
>> +#define PDWAKE		BIT(0) /* Power Down Wakeup Flag */
>> +
>> +#define DEFAULT_DEBOUNCE		1
>> +#define DEFAULT_PRE_SCALE		1
>> +#define DEFAULT_PRE_SCALEDIV	32
>> +
>> +struct ma35d1_keypad {
>> +	struct clk *clk;
>> +	struct input_dev *input_dev;
>> +	void __iomem *mmio_base;
>> +	int irq;
>> +	unsigned int kpi_row;
>> +	unsigned int kpi_col;
>> +	unsigned int debounce_val;
>> +	unsigned int pre_scale;
>> +	unsigned int pre_scale_divider;
>> +};
>> +
>> +static void ma35d1_keypad_scan_matrix(struct ma35d1_keypad *keypad,	unsigned int status)
>> +{
>> +	struct input_dev *input_dev = keypad->input_dev;
>> +	unsigned int i, j;
>> +	unsigned int row_add = 0;
>> +	unsigned int code;
>> +	unsigned int key;
>> +	unsigned int press_key;
>> +	unsigned long KeyEvent[4];
> No Windows or C++ code, please.


I will modify to:unsigned long key_event[4];

>
>> +	unsigned int row_shift = get_count_order(keypad->kpi_col);
>> +	unsigned short *keymap = input_dev->keycode;
>> +
> ...
>
>> +static int ma35d1_keypad_probe(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_keypad *keypad;
>> +	struct input_dev *input_dev;
>> +	struct resource *res;
>> +	int error = 0;
>> +
>> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
>> +	if (!keypad)
>> +		return -ENOMEM;
>> +
>> +
>> +	input_dev = input_allocate_device();
>> +	if (!input_dev) {
>> +		dev_err(&pdev->dev, "failed to allocate input device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "failed to get I/O memory\n");
>> +		error = -ENXIO;
>> +		goto failed_free_input;
>> +	}
>> +
>> +	keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(keypad->mmio_base)) {
>> +		dev_err(&pdev->dev, "failed to remap I/O memory\n");
>> +		return PTR_ERR(keypad->mmio_base);
>> +	}
>> +
>> +	keypad->irq = platform_get_irq(pdev, 0);
>> +	if (keypad->irq < 0) {
>> +		dev_err(&pdev->dev, "failed to get IRQ\n");
>> +		return keypad->irq;
>> +	}
>> +
>> +	keypad->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(keypad->clk)) {
>> +		dev_err(&pdev->dev, "failed to get core clk: %ld\n", PTR_ERR(keypad->clk));
> Syntax is: return dev_err_probe, except that your error handling code is a mess.
> Earlier you have gotos, now return. Organize it nicely, so this will
> follow some logical concept.


I will refactor the error handling in my driver to consistently use either the return dev_err_probe() statement
or the goto method, ensuring a clearer and more logical flow in the code.

>
>> +		return PTR_ERR(keypad->clk);
>> +	}
>> +
>> +	error = matrix_keypad_parse_properties(&pdev->dev,
>> +										   &(keypad->kpi_row),
>> +										   &(keypad->kpi_col));
> How did you aligned it?
>
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to parse kp params\n");
>> +		return error;
>> +	}
>> +
>> +	error = matrix_keypad_build_keymap(NULL, NULL,
>> +									   keypad->kpi_row,
>> +									   keypad->kpi_col,
>> +									   NULL, input_dev);
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to build keymap\n");
>> +		return error;
>> +	}
>> +
>> +	keypad->input_dev = input_dev;
>> +	input_dev->name = pdev->name;
>> +	input_dev->id.bustype = BUS_HOST;
>> +	input_dev->open = ma35d1_keypad_open;
>> +	input_dev->close = ma35d1_keypad_close;
>> +	input_dev->dev.parent = &pdev->dev;
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "debounce-period", &(keypad->debounce_val)))
>> +		keypad->debounce_val = DEFAULT_DEBOUNCE;
> This is not used anywhere. Drop dead code.


In|ma35d1_keypad_open()|, I used|keypad->debounce_val|  to set the debounce length.

>
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "per-scale", &(keypad->pre_scale)))
>> +		keypad->pre_scale = DEFAULT_PRE_SCALE;
> Not better...
>
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "per-scalediv", &(keypad->pre_scale_divider)))
>> +		keypad->pre_scale_divider = DEFAULT_PRE_SCALEDIV;
> Still not better...
>
> So there are defaults? Why these are required by bindings? Why bindings do not say
> defaults?


I will remove the default values for pre-scale and pre-scalediv.

>
>> +
>> +	__set_bit(EV_REP, input_dev->evbit);
>> +	input_set_drvdata(input_dev, keypad);
>> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
>> +
>> +	error = input_register_device(input_dev);
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to register input device\n");
>> +		goto failed_free_input;
>> +	}
>> +
>> +	error = devm_request_irq(&pdev->dev, keypad->irq,
>> +							 ma35d1_keypad_interrupt,
>> +							 IRQF_NO_SUSPEND, pdev->name, keypad);
> Totally mesed alignment.


I will review the editor settings and make the necessary adjustments to resolve this problem.

>
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to request IRQ\n");
>> +		goto failed_unregister_input;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, keypad);
>> +	device_init_wakeup(&pdev->dev, 1);
>> +	clk_prepare_enable(keypad->clk);
>> +
>> +	return 0;
>> +
>> +failed_unregister_input:
>> +	input_unregister_device(input_dev);
>> +failed_free_input:
>> +	input_free_device(input_dev);
>> +	return error;
>> +}
>> +
>> +static void ma35d1_keypad_remove(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	input_unregister_device(keypad->input_dev);
>> +	clk_disable_unprepare(keypad->clk);
> Why aren't you using devm_clk_get_enabled()?


I will update the code to utilize devm_clk_get_enabled() and remove the clk_disable_unprepare(keypad->clk)

>
>> +}
>> +
>> +static int ma35d1_keypad_suspend(struct platform_device *pdev,
>> +									pm_message_t state)
>> +{
>> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	if (device_may_wakeup(&pdev->dev)) {
>> +		writel(readl(keypad->mmio_base + KPI_CONF) | WAKEUP, keypad->mmio_base + KPI_CONF);
>> +		enable_irq_wake(keypad->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_keypad_resume(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	if (device_may_wakeup(&pdev->dev)) {
>> +		writel(readl(keypad->mmio_base + KPI_CONF) & ~(WAKEUP),
>> +						keypad->mmio_base + KPI_CONF);
>> +		disable_irq_wake(keypad->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id ma35d1_kpi_of_match[] = {
>> +	{ .compatible = "nuvoton,ma35d1-kpi"},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35d1_kpi_of_match);
>> +
>> +static struct platform_driver ma35d1_keypad_driver = {
>> +	.probe		= ma35d1_keypad_probe,
>> +	.remove		= ma35d1_keypad_remove,
>> +	.suspend	= ma35d1_keypad_suspend,
>> +	.resume		= ma35d1_keypad_resume,
>> +	.driver		= {
>> +		.name	= "ma35d1-kpi",
>> +		.of_match_table = of_match_ptr(ma35d1_kpi_of_match),
> Drop of_match_ptr(), you have warnings here.


I will remove this line of code.

>
> Best regards,
> Krzysztof

Best regards,
Ming-Jen Chen

Re: [PATCH 2/2] input: keypad: add new keypad driver for MA35D1

[Dmitry Torokhov]

Hi,

On Tue, Oct 22, 2024 at 06:31:58AM +0000, mjchen wrote:
> From: mjchen <mjchen@nuvoton.com>
> 
> Adds a new keypad driver for the MA35D1 platform.
> The driver supports key scanning and interrupt handling.
> 
> Signed-off-by: mjchen <mjchen@nuvoton.com>
> ---
>  drivers/input/keyboard/Kconfig         |  10 +
>  drivers/input/keyboard/Makefile        |   1 +
>  drivers/input/keyboard/ma35d1_keypad.c | 312 +++++++++++++++++++++++++
>  3 files changed, 323 insertions(+)
>  create mode 100644 drivers/input/keyboard/ma35d1_keypad.c
> 
> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
> index 721ab69e84ac..ce9bd5cc13a1 100644
> --- a/drivers/input/keyboard/Kconfig
> +++ b/drivers/input/keyboard/Kconfig
> @@ -797,4 +797,14 @@ config KEYBOARD_CYPRESS_SF
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called cypress-sf.
>  
> +config KEYBOARD_MA35D1
> +	tristate "Nuvoton MA35D1 keypad driver"
> +	depends on ARCH_MA35
> +	select INPUT_MATRIXKMAP
> +	help
> +	  Say Y here if you want to use Nuvoton MA35D1 keypad.
> +
> +	  To compile this driver as a module, choose M here: the
> +	  module will be called ma35d1-keypad.
> +
>  endif
> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
> index 1e0721c30709..9b858cdd1b6b 100644
> --- a/drivers/input/keyboard/Makefile
> +++ b/drivers/input/keyboard/Makefile
> @@ -70,3 +70,4 @@ obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
>  obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)	+= tm2-touchkey.o
>  obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
>  obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
> +obj-$(CONFIG_KEYBOARD_MA35D1)		+= ma35d1_keypad.o
> diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
> new file mode 100644
> index 000000000000..20b5b1b91127
> --- /dev/null
> +++ b/drivers/input/keyboard/ma35d1_keypad.c
> @@ -0,0 +1,312 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +/*
> + *  MA35D1 keypad driver
> + *  Copyright (C) 2024 Nuvoton Technology Corp.
> + */
> +
> +#include <linux/interrupt.h>
> +#include <linux/input.h>
> +#include <linux/platform_device.h>
> +#include <linux/input/matrix_keypad.h>
> +#include <linux/clk.h>
> +#include <linux/of.h>
> +
> +/* Keypad Interface Registers */
> +#define KPI_CONF		0x00
> +#define KPI_3KCONF		0x04
> +#define KPI_STATUS		0x08
> +#define KPI_RSTC		0x0C
> +#define KPI_KEST		0x10
> +#define KPI_KPE0		0x18
> +#define KPI_KPE1		0x1C
> +#define KPI_KRE0		0x20
> +#define KPI_KRE1		0x24
> +#define KPI_PRESCALDIV	0x28
> +
> +/* KPI_CONF - Keypad Configuration Register */
> +#define KROW		GENMASK(30, 28) /* Keypad Matrix ROW number */
> +#define KCOL		GENMASK(26, 24) /* Keypad Matrix COL Number */
> +#define DB_CLKSEL	GENMASK(19, 16) /* De-bounce sampling cycle selection */
> +#define PRESCALE	GENMASK(15, 8)  /* Row Scan Cycle Pre-scale Value */
> +#define WAKEUP		BIT(5) /* Lower Power Wakeup Enable */
> +#define INTEN		BIT(3) /* Key Interrupt Enable Control */
> +#define RKINTEN		BIT(2) /* Release Key Interrupt Enable */
> +#define PKINTEN		BIT(1) /* Press Key Interrupt Enable Control */
> +#define ENKP		BIT(0) /* Keypad Scan Enable */
> +
> +/* KPI_STATUS - Keypad Status Register */
> +#define PKEY_INT	BIT(4) /* Press key interrupt */
> +#define RKEY_INT	BIT(3) /* Release key interrupt */
> +#define KEY_INT		BIT(2) /* Key Interrupt */
> +#define RST_3KEY	BIT(1) /* 3-Keys Reset Flag */
> +#define PDWAKE		BIT(0) /* Power Down Wakeup Flag */
> +
> +#define DEFAULT_DEBOUNCE		1
> +#define DEFAULT_PRE_SCALE		1
> +#define DEFAULT_PRE_SCALEDIV	32
> +
> +struct ma35d1_keypad {
> +	struct clk *clk;
> +	struct input_dev *input_dev;
> +	void __iomem *mmio_base;
> +	int irq;
> +	unsigned int kpi_row;
> +	unsigned int kpi_col;
> +	unsigned int debounce_val;
> +	unsigned int pre_scale;
> +	unsigned int pre_scale_divider;
> +};
> +
> +static void ma35d1_keypad_scan_matrix(struct ma35d1_keypad *keypad,	unsigned int status)
> +{
> +	struct input_dev *input_dev = keypad->input_dev;
> +	unsigned int i, j;
> +	unsigned int row_add = 0;
> +	unsigned int code;
> +	unsigned int key;
> +	unsigned int press_key;
> +	unsigned long KeyEvent[4];

No camel-casing please.

> +	unsigned int row_shift = get_count_order(keypad->kpi_col);
> +	unsigned short *keymap = input_dev->keycode;
> +
> +	/* Read key event status */
> +	KeyEvent[0] = readl(keypad->mmio_base + KPI_KPE0);
> +	KeyEvent[1] = readl(keypad->mmio_base + KPI_KPE1);
> +	KeyEvent[2] = readl(keypad->mmio_base + KPI_KRE0);
> +	KeyEvent[3] = readl(keypad->mmio_base + KPI_KRE1);
> +
> +	/* Clear key event status */
> +	writel(KeyEvent[0], (keypad->mmio_base + KPI_KPE0));
> +	writel(KeyEvent[1], (keypad->mmio_base + KPI_KPE1));
> +	writel(KeyEvent[2], (keypad->mmio_base + KPI_KRE0));
> +	writel(KeyEvent[3], (keypad->mmio_base + KPI_KRE1));
> +
> +	for (j = 0; j < 4; j++) {
> +		if (KeyEvent[j] != 0) {
> +			row_add = (j % 2) ? 4 : 0;
> +			press_key = (j < 2) ? 1 : 0;

So you have first 64 bits to indicate pressed keys, followed by 64 bits
of released keys, right?

I wonder if you could declare 2 bitmaps of 64 bits and then used
for_each_set_bit() for each of them.

> +
> +			for (i = 0; i < 32; i++) {
> +				if (KeyEvent[j] & (1<<i)) {
> +					code = MATRIX_SCAN_CODE(((i/8) + row_add), (i % 8), row_shift);
> +					key = keymap[code];
> +
> +					input_event(input_dev, EV_MSC, MSC_SCAN, code);
> +					input_report_key(input_dev, key, press_key);
> +				}
> +			}
> +		}
> +	}
> +
> +	input_sync(input_dev);
> +}
> +
> +static irqreturn_t ma35d1_keypad_interrupt(int irq, void *dev_id)
> +{
> +	struct ma35d1_keypad *keypad = dev_id;
> +	unsigned int  kstatus;
> +
> +	kstatus = readl(keypad->mmio_base + KPI_STATUS);
> +
> +	if (kstatus & (PKEY_INT|RKEY_INT)) {
> +		ma35d1_keypad_scan_matrix(keypad, kstatus);
> +	} else {

Is this really "else"? Can it be that all 3 bits will be set?

> +		if (kstatus & PDWAKE)
> +			writel(PDWAKE, (keypad->mmio_base + KPI_STATUS));
> +	}
> +
> +	return IRQ_HANDLED;
> +}
> +
> +static int ma35d1_keypad_open(struct input_dev *dev)
> +{
> +	struct ma35d1_keypad *keypad = input_get_drvdata(dev);
> +	unsigned int val, config;
> +
> +	val = RKINTEN | PKINTEN | INTEN | ENKP;
> +	val |= FIELD_PREP(KCOL, (keypad->kpi_col - 1)) | FIELD_PREP(KROW, (keypad->kpi_row - 1));
> +
> +	if (keypad->debounce_val > 0)
> +		config = FIELD_PREP(PRESCALE, (keypad->pre_scale - 1)) |
> +				 FIELD_PREP(DB_CLKSEL, keypad->debounce_val);
> +	else
> +		config = FIELD_PREP(PRESCALE, (keypad->pre_scale - 1));
> +
> +	val |= config;
> +
> +	writel(val, keypad->mmio_base + KPI_CONF);
> +	writel((keypad->pre_scale_divider - 1),	keypad->mmio_base + KPI_PRESCALDIV);
> +
> +	return 0;
> +}
> +
> +static void ma35d1_keypad_close(struct input_dev *dev)
> +{
> +	struct ma35d1_keypad *keypad = input_get_drvdata(dev);
> +
> +	clk_disable(keypad->clk);

This is broken. What will happen if you open and close the device twice?
If you disable the clock in close() you need to enable it in open().

> +}
> +
> +static int ma35d1_keypad_probe(struct platform_device *pdev)
> +{
> +	struct ma35d1_keypad *keypad;
> +	struct input_dev *input_dev;
> +	struct resource *res;
> +	int error = 0;
> +
> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
> +	if (!keypad)
> +		return -ENOMEM;
> +
> +
> +	input_dev = input_allocate_device();

devm_input_allocate_device().

> +	if (!input_dev) {
> +		dev_err(&pdev->dev, "failed to allocate input device\n");
> +		return -ENOMEM;
> +	}
> +
> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> +	if (!res) {
> +		dev_err(&pdev->dev, "failed to get I/O memory\n");
> +		error = -ENXIO;
> +		goto failed_free_input;
> +	}
> +
> +	keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res);
> +	if (IS_ERR(keypad->mmio_base)) {
> +		dev_err(&pdev->dev, "failed to remap I/O memory\n");
> +		return PTR_ERR(keypad->mmio_base);

Leaking input device (but OK if you switch to devm for it).

> +	}
> +
> +	keypad->irq = platform_get_irq(pdev, 0);
> +	if (keypad->irq < 0) {
> +		dev_err(&pdev->dev, "failed to get IRQ\n");
> +		return keypad->irq;
> +	}
> +
> +	keypad->clk = devm_clk_get(&pdev->dev, NULL);
> +	if (IS_ERR(keypad->clk)) {
> +		dev_err(&pdev->dev, "failed to get core clk: %ld\n", PTR_ERR(keypad->clk));
> +		return PTR_ERR(keypad->clk);
> +	}
> +
> +	error = matrix_keypad_parse_properties(&pdev->dev,
> +										   &(keypad->kpi_row),
> +										   &(keypad->kpi_col));

What tab stop are you using?

> +	if (error) {
> +		dev_err(&pdev->dev, "failed to parse kp params\n");
> +		return error;
> +	}
> +
> +	error = matrix_keypad_build_keymap(NULL, NULL,
> +									   keypad->kpi_row,
> +									   keypad->kpi_col,
> +									   NULL, input_dev);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to build keymap\n");
> +		return error;
> +	}
> +
> +	keypad->input_dev = input_dev;
> +	input_dev->name = pdev->name;
> +	input_dev->id.bustype = BUS_HOST;
> +	input_dev->open = ma35d1_keypad_open;
> +	input_dev->close = ma35d1_keypad_close;
> +	input_dev->dev.parent = &pdev->dev;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "debounce-period", &(keypad->debounce_val)))
> +		keypad->debounce_val = DEFAULT_DEBOUNCE;

Please use generic device property API (device_property_read_u32() and
others).

> +
> +	if (of_property_read_u32(pdev->dev.of_node, "per-scale", &(keypad->pre_scale)))
> +		keypad->pre_scale = DEFAULT_PRE_SCALE;
> +
> +	if (of_property_read_u32(pdev->dev.of_node, "per-scalediv", &(keypad->pre_scale_divider)))
> +		keypad->pre_scale_divider = DEFAULT_PRE_SCALEDIV;
> +
> +	__set_bit(EV_REP, input_dev->evbit);
> +	input_set_drvdata(input_dev, keypad);
> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
> +
> +	error = input_register_device(input_dev);
> +	if (error) {
> +		dev_err(&pdev->dev, "failed to register input device\n");
> +		goto failed_free_input;
> +	}
> +
> +	error = devm_request_irq(&pdev->dev, keypad->irq,
> +							 ma35d1_keypad_interrupt,
> +							 IRQF_NO_SUSPEND, pdev->name, keypad);

Why IRQF_NO_SUSPEND?

> +	if (error) {
> +		dev_err(&pdev->dev, "failed to request IRQ\n");
> +		goto failed_unregister_input;
> +	}
> +
> +	platform_set_drvdata(pdev, keypad);
> +	device_init_wakeup(&pdev->dev, 1);
> +	clk_prepare_enable(keypad->clk);

Too late preparing clock here.

> +
> +	return 0;
> +
> +failed_unregister_input:
> +	input_unregister_device(input_dev);
> +failed_free_input:
> +	input_free_device(input_dev);

Do not mix up devm and non-devm resources.

> +	return error;
> +}
> +
> +static void ma35d1_keypad_remove(struct platform_device *pdev)
> +{
> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	input_unregister_device(keypad->input_dev);
> +	clk_disable_unprepare(keypad->clk);
> +}
> +
> +static int ma35d1_keypad_suspend(struct platform_device *pdev,
> +									pm_message_t state)

Oof, this is so old style. Use DEFINE_SIMPLE_DEV_PM_OPS().

> +{
> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		writel(readl(keypad->mmio_base + KPI_CONF) | WAKEUP, keypad->mmio_base + KPI_CONF);
> +		enable_irq_wake(keypad->irq);

Can you mark the interrupt as a wakeup interrupt in probe by calling
dev_pm_set_wake_irq()? Then you do not need to call enable_irq_wake()
here.

> +	}
> +
> +	return 0;
> +}
> +
> +static int ma35d1_keypad_resume(struct platform_device *pdev)
> +{
> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
> +
> +	if (device_may_wakeup(&pdev->dev)) {
> +		writel(readl(keypad->mmio_base + KPI_CONF) & ~(WAKEUP),
> +						keypad->mmio_base + KPI_CONF);
> +		disable_irq_wake(keypad->irq);
> +	}
> +
> +	return 0;
> +}
> +
> +static const struct of_device_id ma35d1_kpi_of_match[] = {
> +	{ .compatible = "nuvoton,ma35d1-kpi"},
> +	{},
> +};
> +MODULE_DEVICE_TABLE(of, ma35d1_kpi_of_match);
> +
> +static struct platform_driver ma35d1_keypad_driver = {
> +	.probe		= ma35d1_keypad_probe,
> +	.remove		= ma35d1_keypad_remove,
> +	.suspend	= ma35d1_keypad_suspend,
> +	.resume		= ma35d1_keypad_resume,
> +	.driver		= {
> +		.name	= "ma35d1-kpi",
> +		.of_match_table = of_match_ptr(ma35d1_kpi_of_match),
> +	},
> +};
> +module_platform_driver(ma35d1_keypad_driver);
> +
> +MODULE_AUTHOR("Ming-Jen Chen");
> +MODULE_DESCRIPTION("MA35D1 Keypad Driver");
> +MODULE_LICENSE("GPL");
> +
> -- 
> 2.17.1
> 

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] input: keypad: add new keypad driver for MA35D1

[Ming-Jen Chen]

On 2024/10/24 上午 05:20, Dmitry Torokhov wrote:

> Hi,
>
> On Tue, Oct 22, 2024 at 06:31:58AM +0000, mjchen wrote:
>> From: mjchen <mjchen@nuvoton.com>
>>
>> Adds a new keypad driver for the MA35D1 platform.
>> The driver supports key scanning and interrupt handling.
>>
>> Signed-off-by: mjchen <mjchen@nuvoton.com>
>> ---
>>   drivers/input/keyboard/Kconfig         |  10 +
>>   drivers/input/keyboard/Makefile        |   1 +
>>   drivers/input/keyboard/ma35d1_keypad.c | 312 +++++++++++++++++++++++++
>>   3 files changed, 323 insertions(+)
>>   create mode 100644 drivers/input/keyboard/ma35d1_keypad.c
>>
>> diff --git a/drivers/input/keyboard/Kconfig b/drivers/input/keyboard/Kconfig
>> index 721ab69e84ac..ce9bd5cc13a1 100644
>> --- a/drivers/input/keyboard/Kconfig
>> +++ b/drivers/input/keyboard/Kconfig
>> @@ -797,4 +797,14 @@ config KEYBOARD_CYPRESS_SF
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called cypress-sf.
>>   
>> +config KEYBOARD_MA35D1
>> +	tristate "Nuvoton MA35D1 keypad driver"
>> +	depends on ARCH_MA35
>> +	select INPUT_MATRIXKMAP
>> +	help
>> +	  Say Y here if you want to use Nuvoton MA35D1 keypad.
>> +
>> +	  To compile this driver as a module, choose M here: the
>> +	  module will be called ma35d1-keypad.
>> +
>>   endif
>> diff --git a/drivers/input/keyboard/Makefile b/drivers/input/keyboard/Makefile
>> index 1e0721c30709..9b858cdd1b6b 100644
>> --- a/drivers/input/keyboard/Makefile
>> +++ b/drivers/input/keyboard/Makefile
>> @@ -70,3 +70,4 @@ obj-$(CONFIG_KEYBOARD_TEGRA)		+= tegra-kbc.o
>>   obj-$(CONFIG_KEYBOARD_TM2_TOUCHKEY)	+= tm2-touchkey.o
>>   obj-$(CONFIG_KEYBOARD_TWL4030)		+= twl4030_keypad.o
>>   obj-$(CONFIG_KEYBOARD_XTKBD)		+= xtkbd.o
>> +obj-$(CONFIG_KEYBOARD_MA35D1)		+= ma35d1_keypad.o
>> diff --git a/drivers/input/keyboard/ma35d1_keypad.c b/drivers/input/keyboard/ma35d1_keypad.c
>> new file mode 100644
>> index 000000000000..20b5b1b91127
>> --- /dev/null
>> +++ b/drivers/input/keyboard/ma35d1_keypad.c
>> @@ -0,0 +1,312 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +/*
>> + *  MA35D1 keypad driver
>> + *  Copyright (C) 2024 Nuvoton Technology Corp.
>> + */
>> +
>> +#include <linux/interrupt.h>
>> +#include <linux/input.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/input/matrix_keypad.h>
>> +#include <linux/clk.h>
>> +#include <linux/of.h>
>> +
>> +/* Keypad Interface Registers */
>> +#define KPI_CONF		0x00
>> +#define KPI_3KCONF		0x04
>> +#define KPI_STATUS		0x08
>> +#define KPI_RSTC		0x0C
>> +#define KPI_KEST		0x10
>> +#define KPI_KPE0		0x18
>> +#define KPI_KPE1		0x1C
>> +#define KPI_KRE0		0x20
>> +#define KPI_KRE1		0x24
>> +#define KPI_PRESCALDIV	0x28
>> +
>> +/* KPI_CONF - Keypad Configuration Register */
>> +#define KROW		GENMASK(30, 28) /* Keypad Matrix ROW number */
>> +#define KCOL		GENMASK(26, 24) /* Keypad Matrix COL Number */
>> +#define DB_CLKSEL	GENMASK(19, 16) /* De-bounce sampling cycle selection */
>> +#define PRESCALE	GENMASK(15, 8)  /* Row Scan Cycle Pre-scale Value */
>> +#define WAKEUP		BIT(5) /* Lower Power Wakeup Enable */
>> +#define INTEN		BIT(3) /* Key Interrupt Enable Control */
>> +#define RKINTEN		BIT(2) /* Release Key Interrupt Enable */
>> +#define PKINTEN		BIT(1) /* Press Key Interrupt Enable Control */
>> +#define ENKP		BIT(0) /* Keypad Scan Enable */
>> +
>> +/* KPI_STATUS - Keypad Status Register */
>> +#define PKEY_INT	BIT(4) /* Press key interrupt */
>> +#define RKEY_INT	BIT(3) /* Release key interrupt */
>> +#define KEY_INT		BIT(2) /* Key Interrupt */
>> +#define RST_3KEY	BIT(1) /* 3-Keys Reset Flag */
>> +#define PDWAKE		BIT(0) /* Power Down Wakeup Flag */
>> +
>> +#define DEFAULT_DEBOUNCE		1
>> +#define DEFAULT_PRE_SCALE		1
>> +#define DEFAULT_PRE_SCALEDIV	32
>> +
>> +struct ma35d1_keypad {
>> +	struct clk *clk;
>> +	struct input_dev *input_dev;
>> +	void __iomem *mmio_base;
>> +	int irq;
>> +	unsigned int kpi_row;
>> +	unsigned int kpi_col;
>> +	unsigned int debounce_val;
>> +	unsigned int pre_scale;
>> +	unsigned int pre_scale_divider;
>> +};
>> +
>> +static void ma35d1_keypad_scan_matrix(struct ma35d1_keypad *keypad,	unsigned int status)
>> +{
>> +	struct input_dev *input_dev = keypad->input_dev;
>> +	unsigned int i, j;
>> +	unsigned int row_add = 0;
>> +	unsigned int code;
>> +	unsigned int key;
>> +	unsigned int press_key;
>> +	unsigned long KeyEvent[4];
> No camel-casing please.

I will modify to:unsigned long key_event[4];

>
>> +	unsigned int row_shift = get_count_order(keypad->kpi_col);
>> +	unsigned short *keymap = input_dev->keycode;
>> +
>> +	/* Read key event status */
>> +	KeyEvent[0] = readl(keypad->mmio_base + KPI_KPE0);
>> +	KeyEvent[1] = readl(keypad->mmio_base + KPI_KPE1);
>> +	KeyEvent[2] = readl(keypad->mmio_base + KPI_KRE0);
>> +	KeyEvent[3] = readl(keypad->mmio_base + KPI_KRE1);
>> +
>> +	/* Clear key event status */
>> +	writel(KeyEvent[0], (keypad->mmio_base + KPI_KPE0));
>> +	writel(KeyEvent[1], (keypad->mmio_base + KPI_KPE1));
>> +	writel(KeyEvent[2], (keypad->mmio_base + KPI_KRE0));
>> +	writel(KeyEvent[3], (keypad->mmio_base + KPI_KRE1));
>> +
>> +	for (j = 0; j < 4; j++) {
>> +		if (KeyEvent[j] != 0) {
>> +			row_add = (j % 2) ? 4 : 0;
>> +			press_key = (j < 2) ? 1 : 0;
> So you have first 64 bits to indicate pressed keys, followed by 64 bits
> of released keys, right?
>
> I wonder if you could declare 2 bitmaps of 64 bits and then used
> for_each_set_bit() for each of them.

I will modify to:

	pressed_keys  = KeyEvent[0] | KeyEvent[1] << 32;
	released_keys = KeyEvent[2] | KeyEvent[3] << 32;

	// Process pressed keys
	for_each_set_bit(index, &pressed_keys, KEY_EVENT_BITS) {
		code = MATRIX_SCAN_CODE(index/8, (i % 8), row_shift);
		key = keymap[code];

		input_event(input_dev, EV_MSC, MSC_SCAN, code);
		input_report_key(input_dev, key, 1);
	}

	// Process released keys
	for_each_set_bit(index, &released_keys, KEY_EVENT_BITS) {
		code = MATRIX_SCAN_CODE(index/8, (i % 8), row_shift);
		key = keymap[code];

		input_event(input_dev, EV_MSC, MSC_SCAN, code);
		input_report_key(input_dev, key, 0);
	}

>
>> +
>> +			for (i = 0; i < 32; i++) {
>> +				if (KeyEvent[j] & (1<<i)) {
>> +					code = MATRIX_SCAN_CODE(((i/8) + row_add), (i % 8), row_shift);
>> +					key = keymap[code];
>> +
>> +					input_event(input_dev, EV_MSC, MSC_SCAN, code);
>> +					input_report_key(input_dev, key, press_key);
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	input_sync(input_dev);
>> +}
>> +
>> +static irqreturn_t ma35d1_keypad_interrupt(int irq, void *dev_id)
>> +{
>> +	struct ma35d1_keypad *keypad = dev_id;
>> +	unsigned int  kstatus;
>> +
>> +	kstatus = readl(keypad->mmio_base + KPI_STATUS);
>> +
>> +	if (kstatus & (PKEY_INT|RKEY_INT)) {
>> +		ma35d1_keypad_scan_matrix(keypad, kstatus);
>> +	} else {
> Is this really "else"? Can it be that all 3 bits will be set?

There are indeed four interrupt bits on the hardware, one of which is the "Three Keys Reset" interrupt.
However, my driver does not enable the Three Keys Reset functionality,
so this interrupt will not occur in practice.
Therefore, the actual interrupts that will be triggered are only|PKEY_INT, RKEY_INT and PDWAKE. |

>
>> +		if (kstatus & PDWAKE)
>> +			writel(PDWAKE, (keypad->mmio_base + KPI_STATUS));
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>> +
>> +static int ma35d1_keypad_open(struct input_dev *dev)
>> +{
>> +	struct ma35d1_keypad *keypad = input_get_drvdata(dev);
>> +	unsigned int val, config;
>> +
>> +	val = RKINTEN | PKINTEN | INTEN | ENKP;
>> +	val |= FIELD_PREP(KCOL, (keypad->kpi_col - 1)) | FIELD_PREP(KROW, (keypad->kpi_row - 1));
>> +
>> +	if (keypad->debounce_val > 0)
>> +		config = FIELD_PREP(PRESCALE, (keypad->pre_scale - 1)) |
>> +				 FIELD_PREP(DB_CLKSEL, keypad->debounce_val);
>> +	else
>> +		config = FIELD_PREP(PRESCALE, (keypad->pre_scale - 1));
>> +
>> +	val |= config;
>> +
>> +	writel(val, keypad->mmio_base + KPI_CONF);
>> +	writel((keypad->pre_scale_divider - 1),	keypad->mmio_base + KPI_PRESCALDIV);
>> +
>> +	return 0;
>> +}
>> +
>> +static void ma35d1_keypad_close(struct input_dev *dev)
>> +{
>> +	struct ma35d1_keypad *keypad = input_get_drvdata(dev);
>> +
>> +	clk_disable(keypad->clk);
> This is broken. What will happen if you open and close the device twice?
> If you disable the clock in close() you need to enable it in open().

I will modify the code to ensure that the clock is enabled in the open function and disabled in the close function,
allowing for proper handling when the device is opened and closed multiple times.

>> +}
>> +
>> +static int ma35d1_keypad_probe(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_keypad *keypad;
>> +	struct input_dev *input_dev;
>> +	struct resource *res;
>> +	int error = 0;
>> +
>> +	keypad = devm_kzalloc(&pdev->dev, sizeof(*keypad), GFP_KERNEL);
>> +	if (!keypad)
>> +		return -ENOMEM;
>> +
>> +
>> +	input_dev = input_allocate_device();
> devm_input_allocate_device().

I will modify to:  input_dev = devm_input_allocate_device();

>
>> +	if (!input_dev) {
>> +		dev_err(&pdev->dev, "failed to allocate input device\n");
>> +		return -ENOMEM;
>> +	}
>> +
>> +	res = platform_get_resource(pdev, IORESOURCE_MEM, 0);
>> +	if (!res) {
>> +		dev_err(&pdev->dev, "failed to get I/O memory\n");
>> +		error = -ENXIO;
>> +		goto failed_free_input;
>> +	}
>> +
>> +	keypad->mmio_base = devm_ioremap_resource(&pdev->dev, res);
>> +	if (IS_ERR(keypad->mmio_base)) {
>> +		dev_err(&pdev->dev, "failed to remap I/O memory\n");
>> +		return PTR_ERR(keypad->mmio_base);
> Leaking input device (but OK if you switch to devm for it).

I switch to devm.

>
>> +	}
>> +
>> +	keypad->irq = platform_get_irq(pdev, 0);
>> +	if (keypad->irq < 0) {
>> +		dev_err(&pdev->dev, "failed to get IRQ\n");
>> +		return keypad->irq;
>> +	}
>> +
>> +	keypad->clk = devm_clk_get(&pdev->dev, NULL);
>> +	if (IS_ERR(keypad->clk)) {
>> +		dev_err(&pdev->dev, "failed to get core clk: %ld\n", PTR_ERR(keypad->clk));
>> +		return PTR_ERR(keypad->clk);
>> +	}
>> +
>> +	error = matrix_keypad_parse_properties(&pdev->dev,
>> +										   &(keypad->kpi_row),
>> +										   &(keypad->kpi_col));
> What tab stop are you using?

Sorry, I'll check my editor settings.

>
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to parse kp params\n");
>> +		return error;
>> +	}
>> +
>> +	error = matrix_keypad_build_keymap(NULL, NULL,
>> +									   keypad->kpi_row,
>> +									   keypad->kpi_col,
>> +									   NULL, input_dev);
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to build keymap\n");
>> +		return error;
>> +	}
>> +
>> +	keypad->input_dev = input_dev;
>> +	input_dev->name = pdev->name;
>> +	input_dev->id.bustype = BUS_HOST;
>> +	input_dev->open = ma35d1_keypad_open;
>> +	input_dev->close = ma35d1_keypad_close;
>> +	input_dev->dev.parent = &pdev->dev;
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "debounce-period", &(keypad->debounce_val)))
>> +		keypad->debounce_val = DEFAULT_DEBOUNCE;
> Please use generic device property API (device_property_read_u32() and
> others).

I will use device_property_read_u32()

>
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "per-scale", &(keypad->pre_scale)))
>> +		keypad->pre_scale = DEFAULT_PRE_SCALE;
>> +
>> +	if (of_property_read_u32(pdev->dev.of_node, "per-scalediv", &(keypad->pre_scale_divider)))
>> +		keypad->pre_scale_divider = DEFAULT_PRE_SCALEDIV;
>> +
>> +	__set_bit(EV_REP, input_dev->evbit);
>> +	input_set_drvdata(input_dev, keypad);
>> +	input_set_capability(input_dev, EV_MSC, MSC_SCAN);
>> +
>> +	error = input_register_device(input_dev);
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to register input device\n");
>> +		goto failed_free_input;
>> +	}
>> +
>> +	error = devm_request_irq(&pdev->dev, keypad->irq,
>> +							 ma35d1_keypad_interrupt,
>> +							 IRQF_NO_SUSPEND, pdev->name, keypad);
> Why IRQF_NO_SUSPEND?

I use this flag to ensure that the keypad’s interrupt remains enabled during system suspend,
allowing it to wake up the system when needed.
Without IRQF_NO_SUSPEND, the interrupt would be disabled,
preventing the keypad from functioning as a wake-up source.

>
>> +	if (error) {
>> +		dev_err(&pdev->dev, "failed to request IRQ\n");
>> +		goto failed_unregister_input;
>> +	}
>> +
>> +	platform_set_drvdata(pdev, keypad);
>> +	device_init_wakeup(&pdev->dev, 1);
>> +	clk_prepare_enable(keypad->clk);
> Too late preparing clock here.

I will move the|clk_prepare_enable()|  function to be called immediately after|devm_clk_get() |

>
>> +
>> +	return 0;
>> +
>> +failed_unregister_input:
>> +	input_unregister_device(input_dev);
>> +failed_free_input:
>> +	input_free_device(input_dev);
> Do not mix up devm and non-devm resources.

I will use devm.

>
>> +	return error;
>> +}
>> +
>> +static void ma35d1_keypad_remove(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	input_unregister_device(keypad->input_dev);
>> +	clk_disable_unprepare(keypad->clk);
>> +}
>> +
>> +static int ma35d1_keypad_suspend(struct platform_device *pdev,
>> +									pm_message_t state)
> Oof, this is so old style. Use DEFINE_SIMPLE_DEV_PM_OPS().

I will modify to:
static DEFINE_SIMPLE_DEV_PM_OPS(ma35d1_pm_ops, ma35d1_keypad_suspend, ma35d1_keypad_resume);

static struct platform_driver ma35d1_keypad_driver = {
	.probe		= ma35d1_keypad_probe,
	.remove		= ma35d1_keypad_remove,
	.driver		= {
		.name	= "ma35d1-kpi",
		.pm	= pm_sleep_ptr(&ma35d1_pm_ops),
	},
};

>
>> +{
>> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	if (device_may_wakeup(&pdev->dev)) {
>> +		writel(readl(keypad->mmio_base + KPI_CONF) | WAKEUP, keypad->mmio_base + KPI_CONF);
>> +		enable_irq_wake(keypad->irq);
> Can you mark the interrupt as a wakeup interrupt in probe by calling
> dev_pm_set_wake_irq()? Then you do not need to call enable_irq_wake()
> here.

Okay, I will use|dev_pm_set_wake_irq()|  in the probe and remove|enable_irq_wake()|

>
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static int ma35d1_keypad_resume(struct platform_device *pdev)
>> +{
>> +	struct ma35d1_keypad *keypad = platform_get_drvdata(pdev);
>> +
>> +	if (device_may_wakeup(&pdev->dev)) {
>> +		writel(readl(keypad->mmio_base + KPI_CONF) & ~(WAKEUP),
>> +						keypad->mmio_base + KPI_CONF);
>> +		disable_irq_wake(keypad->irq);
>> +	}
>> +
>> +	return 0;
>> +}
>> +
>> +static const struct of_device_id ma35d1_kpi_of_match[] = {
>> +	{ .compatible = "nuvoton,ma35d1-kpi"},
>> +	{},
>> +};
>> +MODULE_DEVICE_TABLE(of, ma35d1_kpi_of_match);
>> +
>> +static struct platform_driver ma35d1_keypad_driver = {
>> +	.probe		= ma35d1_keypad_probe,
>> +	.remove		= ma35d1_keypad_remove,
>> +	.suspend	= ma35d1_keypad_suspend,
>> +	.resume		= ma35d1_keypad_resume,
>> +	.driver		= {
>> +		.name	= "ma35d1-kpi",
>> +		.of_match_table = of_match_ptr(ma35d1_kpi_of_match),
>> +	},
>> +};
>> +module_platform_driver(ma35d1_keypad_driver);
>> +
>> +MODULE_AUTHOR("Ming-Jen Chen");
>> +MODULE_DESCRIPTION("MA35D1 Keypad Driver");
>> +MODULE_LICENSE("GPL");
>> +
>> -- 
>> 2.17.1
>>
> Thanks.
>