[PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[Wenhua Lin]

Add keypad driver ducumentation.

Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
---
 .../bindings/input/sprd-keypad.yaml           | 76 +++++++++++++++++++
 1 file changed, 76 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/sprd-keypad.yaml

diff --git a/Documentation/devicetree/bindings/input/sprd-keypad.yaml b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
new file mode 100644
index 000000000000..51710e1eb389
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
@@ -0,0 +1,76 @@
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+# Copyright 2023 Unisoc Inc.
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/sprd-keypad.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Device-Tree bindings for GPIO attached keys
+
+maintainers:
+  - Orson Zhai <orsonzhai@gmail.com>
+  - Baolin Wang <baolin.wang7@gmail.com>
+  - Chunyan Zhang <zhang.lyra@gmail.com>
+
+description: |
+    Keypad controller is used to interface a SoC with a matrix-keypad device.
+    The keypad controller supports multiple row and column lines.
+    A key can be placed at each intersection of a unique row and a unique column.
+    The keypad controller can sense a key-press and key-release and report the
+    event using a interrupt to the cpu.
+
+properties:
+    compatible:
+    const: sprd,sc9860-keypad
+
+    reg:
+        maxItems: 1
+
+    interrupts:
+        maxItems: 1
+
+    keypad,num-rows:
+    description: Number of row lines connected to the keypad controller.
+
+    keypad,num-columns:
+    description: Number of column lines connected to the keypad.
+
+    debounce-interval:
+    description:
+        Debouncing interval time in milliseconds. If not specified defaults to 5.
+    $ref: /schemas/types.yaml#/definitions/uint32
+
+    default: 5
+
+    linux,keymap:
+    description: An array of packed 1-cell entries containing the equivalent
+        of row, column and linux key-code. The 32-bit big endian cell is packed.
+
+required:
+        - compatible
+        - reg
+        - keypad,num-rows
+        - keypad,num-columns
+        - linux,keymap
+
+unevaluatedProperties: false
+
+
+examples:
+  - |
+	keypad@40250000 {
+		compatible = "sprd,sc9860-keypad";
+		reg = 	<0x40250000 0x1000>;
+		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
+		clocks = <&aonapb_gate CLK_KPD_EB>,
+			<&aonapb_gate CLK_KPD_RTC_EB>;
+		clock-names = "enable", "rtc";
+		keypad,num-rows= <3>;
+		keypad,num-columns = <3>;
+		debounce-interval = <5>;
+		linux,keymap = < 0x00000001
+				 0x01000002
+				 0x00020003>;
+		status = "okay";
+	};
+...
-- 
2.17.1

Re: [PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[Jonathan Cameron]

On Tue, 8 Aug 2023 15:22:52 +0800
Wenhua Lin <Wenhua.Lin@unisoc.com> wrote:

> Add keypad driver ducumentation.

documentation

Though naming convention for dt-bindings patches does not look like
this. Take a look at what has been accepted as bindings recently.
Also you aren't going to get much review of this without cc'ing the
dt list and maintainers.

> 
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
>  .../bindings/input/sprd-keypad.yaml           | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/sprd-keypad.yaml
> 
> diff --git a/Documentation/devicetree/bindings/input/sprd-keypad.yaml b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
> new file mode 100644
> index 000000000000..51710e1eb389
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2023 Unisoc Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/sprd-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Device-Tree bindings for GPIO attached keys

Seems rather more specific than that!

> +
> +maintainers:
> +  - Orson Zhai <orsonzhai@gmail.com>
> +  - Baolin Wang <baolin.wang7@gmail.com>
> +  - Chunyan Zhang <zhang.lyra@gmail.com>
> +
> +description: |
> +    Keypad controller is used to interface a SoC with a matrix-keypad device.
> +    The keypad controller supports multiple row and column lines.
> +    A key can be placed at each intersection of a unique row and a unique column.
> +    The keypad controller can sense a key-press and key-release and report the
> +    event using a interrupt to the cpu.

CPU

> +
> +properties:
> +    compatible:
> +    const: sprd,sc9860-keypad

Make sure you follow the guidance on test building your binding and example.
See Documentation/devicetree/bindings/writing-bindings.rst and submitting-patches.rst

This has a lot of issues covered by that document.

> +
> +    reg:
> +        maxItems: 1
> +
> +    interrupts:
> +        maxItems: 1
> +
> +    keypad,num-rows:
> +    description: Number of row lines connected to the keypad controller.
> +
> +    keypad,num-columns:
> +    description: Number of column lines connected to the keypad.
> +
> +    debounce-interval:
> +    description:
> +        Debouncing interval time in milliseconds. If not specified defaults to 5.
> +    $ref: /schemas/types.yaml#/definitions/uint32
> +
> +    default: 5
> +
> +    linux,keymap:
> +    description: An array of packed 1-cell entries containing the equivalent
> +        of row, column and linux key-code. The 32-bit big endian cell is packed.
> +
> +required:
> +        - compatible
> +        - reg
> +        - keypad,num-rows
> +        - keypad,num-columns
> +        - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +
> +examples:
> +  - |
> +	keypad@40250000 {
> +		compatible = "sprd,sc9860-keypad";
> +		reg = 	<0x40250000 0x1000>;
> +		interrupts = <GIC_SPI 36 IRQ_TYPE_LEVEL_HIGH>;
> +		clocks = <&aonapb_gate CLK_KPD_EB>,
> +			<&aonapb_gate CLK_KPD_RTC_EB>;
> +		clock-names = "enable", "rtc";
> +		keypad,num-rows= <3>;
> +		keypad,num-columns = <3>;
> +		debounce-interval = <5>;
> +		linux,keymap = < 0x00000001
> +				 0x01000002
> +				 0x00020003>;
> +		status = "okay";

Status shouldn't be in an example.  Formatting wrong.
IF your binding example doesn't build (this won't) then it won't be accepted
by the dt-binding maintainers and their review is needed for it to be
merged.  

Jonathan


> +	};
> +...

Re: [PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[Andy Shevchenko]

On Tue, Aug 08, 2023 at 03:22:52PM +0800, Wenhua Lin wrote:
> Add keypad driver ducumentation.

Why am I Cc'ed to this? I'm not a DT person to review this...

...

> +		reg = 	<0x40250000 0x1000>;

TAB and space mix

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[wenhua lin]

On Tue, Aug 8, 2023 at 9:28 PM Andy Shevchenko
<andriy.shevchenko@linux.intel.com> wrote:
>
> On Tue, Aug 08, 2023 at 03:22:52PM +0800, Wenhua Lin wrote:
> > Add keypad driver ducumentation.
>
> Why am I Cc'ed to this? I'm not a DT person to review this...
>

This is one of our mistakes, thank you very much for your review, we
will correct it in patch v2.

> ...
>
> > +             reg =   <0x40250000 0x1000>;
>
> TAB and space mix

Hi Andy:
We will correct it in patch v2.

Thanks
Wenhua.Lin
>
> --
> With Best Regards,
> Andy Shevchenko
>
>

Re: [PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[Krzysztof Kozlowski]

On 08/08/2023 09:22, Wenhua Lin wrote:
> Add keypad driver ducumentation.

1. Please use scripts/get_maintainers.pl to get a list of necessary
people and lists to CC. It might happen, that command when run on an
older kernel, gives you outdated entries. Therefore please be sure you
base your patches on recent Linux kernel.

You missed at least DT list (maybe more), so this won't be tested by
automated tooling. Performing review on untested code might be a waste
of time, thus I will skip this patch entirely till you follow the
process allowing the patch to be tested.

Please kindly resend and include all necessary To/Cc entries.


2. Please use subject prefixes matching the subsystem. You can get them
for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
directory your patch is touching.


Limited review follows:

> 
> Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> ---
>  .../bindings/input/sprd-keypad.yaml           | 76 +++++++++++++++++++
>  1 file changed, 76 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/sprd-keypad.yaml

Filename matching compatible, so sprd,block-name-etc.yaml

> 
> diff --git a/Documentation/devicetree/bindings/input/sprd-keypad.yaml b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
> new file mode 100644
> index 000000000000..51710e1eb389
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
> @@ -0,0 +1,76 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +# Copyright 2023 Unisoc Inc.
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/sprd-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Device-Tree bindings for GPIO attached keys

This should not work and you should see warnings... which means:

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

so drop "Device-tree bindings".

Title also says nothing about the hardware - it is entirely unrelated.
Describe the hardware.


> +
> +maintainers:
> +  - Orson Zhai <orsonzhai@gmail.com>
> +  - Baolin Wang <baolin.wang7@gmail.com>
> +  - Chunyan Zhang <zhang.lyra@gmail.com>
> +
> +description: |
> +    Keypad controller is used to interface a SoC with a matrix-keypad device.

Mention which SoC.

> +    The keypad controller supports multiple row and column lines.
> +    A key can be placed at each intersection of a unique row and a unique column.
> +    The keypad controller can sense a key-press and key-release and report the
> +    event using a interrupt to the cpu.
> +
> +properties:
> +    compatible:
> +    const: sprd,sc9860-keypad
> +
> +    reg:
> +        maxItems: 1

OK, so this was for 100% not tested and won't work. No need to waste our
time on this. Test before sending:

It does not look like you tested the bindings, at least after quick
look. Please run `make dt_binding_check` (see
Documentation/devicetree/bindings/writing-schema.rst for instructions).
Maybe you need to update your dtschema and yamllint.

Best regards,
Krzysztof

Re: [PATCH 1/2] devicetree: bindings: Add keypad driver ducumentation

[wenhua lin]

On Thu, Aug 10, 2023 at 2:31 AM Krzysztof Kozlowski <krzk@kernel.org> wrote:
>
> On 08/08/2023 09:22, Wenhua Lin wrote:
> > Add keypad driver ducumentation.
>
> 1. Please use scripts/get_maintainers.pl to get a list of necessary
> people and lists to CC. It might happen, that command when run on an
> older kernel, gives you outdated entries. Therefore please be sure you
> base your patches on recent Linux kernel.
>
> You missed at least DT list (maybe more), so this won't be tested by
> automated tooling. Performing review on untested code might be a waste
> of time, thus I will skip this patch entirely till you follow the
> process allowing the patch to be tested.
>
> Please kindly resend and include all necessary To/Cc entries.
>
>
> 2. Please use subject prefixes matching the subsystem. You can get them
> for example with `git log --oneline -- DIRECTORY_OR_FILE` on the
> directory your patch is touching.
>
>
> Limited review follows:
>
> >
> > Signed-off-by: Wenhua Lin <Wenhua.Lin@unisoc.com>
> > ---
> >  .../bindings/input/sprd-keypad.yaml           | 76 +++++++++++++++++++
> >  1 file changed, 76 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/input/sprd-keypad.yaml
>
> Filename matching compatible, so sprd,block-name-etc.yaml
>
> >
> > diff --git a/Documentation/devicetree/bindings/input/sprd-keypad.yaml b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
> > new file mode 100644
> > index 000000000000..51710e1eb389
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/sprd-keypad.yaml
> > @@ -0,0 +1,76 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +# Copyright 2023 Unisoc Inc.
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/sprd-keypad.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Device-Tree bindings for GPIO attached keys
>
> This should not work and you should see warnings... which means:
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
>
> so drop "Device-tree bindings".
>
> Title also says nothing about the hardware - it is entirely unrelated.
> Describe the hardware.
>
>
> > +
> > +maintainers:
> > +  - Orson Zhai <orsonzhai@gmail.com>
> > +  - Baolin Wang <baolin.wang7@gmail.com>
> > +  - Chunyan Zhang <zhang.lyra@gmail.com>
> > +
> > +description: |
> > +    Keypad controller is used to interface a SoC with a matrix-keypad device.
>
> Mention which SoC.
>
> > +    The keypad controller supports multiple row and column lines.
> > +    A key can be placed at each intersection of a unique row and a unique column.
> > +    The keypad controller can sense a key-press and key-release and report the
> > +    event using a interrupt to the cpu.
> > +
> > +properties:
> > +    compatible:
> > +    const: sprd,sc9860-keypad
> > +
> > +    reg:
> > +        maxItems: 1
>
> OK, so this was for 100% not tested and won't work. No need to waste our
> time on this. Test before sending:
>
> It does not look like you tested the bindings, at least after quick
> look. Please run `make dt_binding_check` (see
> Documentation/devicetree/bindings/writing-schema.rst for instructions).
> Maybe you need to update your dtschema and yamllint.
>
> Best regards,
> Krzysztof
>

Hi Krzysztof:
   This is a mistake of ours, sorry for wasting your precious time.

Thanks
Wenhua.Lin