diff mbox series

[09/14] dt-bindings: input: samsung,s3c6410-keypad: introduce compact binding

Message ID 20240819045813.2154642-10-dmitry.torokhov@gmail.com
State New
Headers show
Series Remove support for platform data from samsung keypad | expand

Commit Message

Dmitry Torokhov Aug. 19, 2024, 4:58 a.m. UTC
The binding with a sub-node per each key is very verbose and is hard to
use with static device properties. Allow standard matrix keymap binding
in addition to the verbose one.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 .../input/samsung,s3c6410-keypad.yaml         | 57 ++++++++++++++++++-
 1 file changed, 54 insertions(+), 3 deletions(-)

Comments

Krzysztof Kozlowski Aug. 19, 2024, 1:02 p.m. UTC | #1
On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote:
> The binding with a sub-node per each key is very verbose and is hard to
> use with static device properties. Allow standard matrix keymap binding
> in addition to the verbose one.
> 
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>  .../input/samsung,s3c6410-keypad.yaml         | 57 ++++++++++++++++++-
>  1 file changed, 54 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> index a53569aa0ee7..28a318a0ff7e 100644
> --- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> +++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> @@ -16,6 +16,10 @@ description:
>  maintainers:
>    - Krzysztof Kozlowski <krzk@kernel.org>
>  
> +allOf:
> +  - $ref: input.yaml#
> +  - $ref: matrix-keymap.yaml#
> +
>  properties:
>    compatible:
>      enum:
> @@ -37,6 +41,10 @@ properties:
>  
>    wakeup-source: true
>  
> +  keypad,num-columns: true
> +  keypad,num-rows: true
> +  linux,keymap: true
> +
>    linux,input-no-autorepeat:
>      type: boolean
>      description:
> @@ -81,12 +89,33 @@ patternProperties:
>        - keypad,row
>        - linux,code
>  
> +dependencies:
> +  linux,keymap:
> +    required:

Why "required" keyword? The dependencies should have just list of
properties. See example-schema.

> +      - keypad,num-columns
> +      - keypad,num-rows
> +
>  required:
>    - compatible
>    - reg
>    - interrupts
> -  - samsung,keypad-num-columns
> -  - samsung,keypad-num-rows
> +
> +if:

put allOf: here and this within allOf, so you the "if" could grow in the
future.

Best regards,
Krzysztof
Dmitry Torokhov Aug. 19, 2024, 3:49 p.m. UTC | #2
On Mon, Aug 19, 2024 at 03:02:07PM +0200, Krzysztof Kozlowski wrote:
> On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote:
> > The binding with a sub-node per each key is very verbose and is hard to
> > use with static device properties. Allow standard matrix keymap binding
> > in addition to the verbose one.
> > 
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> >  .../input/samsung,s3c6410-keypad.yaml         | 57 ++++++++++++++++++-
> >  1 file changed, 54 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> > index a53569aa0ee7..28a318a0ff7e 100644
> > --- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> > +++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
> > @@ -16,6 +16,10 @@ description:
> >  maintainers:
> >    - Krzysztof Kozlowski <krzk@kernel.org>
> >  
> > +allOf:
> > +  - $ref: input.yaml#
> > +  - $ref: matrix-keymap.yaml#
> > +
> >  properties:
> >    compatible:
> >      enum:
> > @@ -37,6 +41,10 @@ properties:
> >  
> >    wakeup-source: true
> >  
> > +  keypad,num-columns: true
> > +  keypad,num-rows: true
> > +  linux,keymap: true
> > +
> >    linux,input-no-autorepeat:
> >      type: boolean
> >      description:
> > @@ -81,12 +89,33 @@ patternProperties:
> >        - keypad,row
> >        - linux,code
> >  
> > +dependencies:
> > +  linux,keymap:
> > +    required:
> 
> Why "required" keyword? The dependencies should have just list of
> properties. See example-schema.

OK, changed this to

dependencies:
  linux,keymap: [ "keypad,num-columns", "keypad,num-rows" ]
  
> 
> > +      - keypad,num-columns
> > +      - keypad,num-rows
> > +
> >  required:
> >    - compatible
> >    - reg
> >    - interrupts
> > -  - samsung,keypad-num-columns
> > -  - samsung,keypad-num-rows
> > +
> > +if:
> 
> put allOf: here and this within allOf, so you the "if" could grow in the
> future.

Hmm, there is already "allOf" at the beginning of the file, so adding
another one results in complaints about duplicate "allOf". I can move it
all to the top, like this:

allOf:
  - $ref: input.yaml#
  - $ref: matrix-keymap.yaml#
  - if:
      required:
        - linux,keymap
    then:
      properties:
        samsung,keypad-num-columns: false
        samsung,keypad-num-rows: false
      patternProperties:
        '^key-[0-9a-z]+$': false
    else:
      properties:
        keypad,num-columns: false
        keypad,num-rows: false
      required:
        - samsung,keypad-num-columns
        - samsung,keypad-num-rows

Is this OK? I don't quite like that "tweaks" are listed before main
body of properties.

Thanks.
Conor Dooley Aug. 19, 2024, 4:48 p.m. UTC | #3
On Mon, Aug 19, 2024 at 08:49:10AM -0700, Dmitry Torokhov wrote:
> On Mon, Aug 19, 2024 at 03:02:07PM +0200, Krzysztof Kozlowski wrote:
> > On Sun, Aug 18, 2024 at 09:58:06PM -0700, Dmitry Torokhov wrote:

> > 
> > > +      - keypad,num-columns
> > > +      - keypad,num-rows
> > > +
> > >  required:
> > >    - compatible
> > >    - reg
> > >    - interrupts
> > > -  - samsung,keypad-num-columns
> > > -  - samsung,keypad-num-rows
> > > +
> > > +if:
> > 
> > put allOf: here and this within allOf, so you the "if" could grow in the
> > future.
> 
> Hmm, there is already "allOf" at the beginning of the file, so adding
> another one results in complaints about duplicate "allOf". I can move it
> all to the top, like this:
> 
> allOf:
>   - $ref: input.yaml#
>   - $ref: matrix-keymap.yaml#
>   - if:
>       required:
>         - linux,keymap
>     then:
>       properties:
>         samsung,keypad-num-columns: false
>         samsung,keypad-num-rows: false
>       patternProperties:
>         '^key-[0-9a-z]+$': false
>     else:
>       properties:
>         keypad,num-columns: false
>         keypad,num-rows: false
>       required:
>         - samsung,keypad-num-columns
>         - samsung,keypad-num-rows
> 
> Is this OK? I don't quite like that "tweaks" are listed before main
> body of properties.

The normal thing to do is to put the allOf at the end, not the start, in
cases like this, for the reason you mention.
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
index a53569aa0ee7..28a318a0ff7e 100644
--- a/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
+++ b/Documentation/devicetree/bindings/input/samsung,s3c6410-keypad.yaml
@@ -16,6 +16,10 @@  description:
 maintainers:
   - Krzysztof Kozlowski <krzk@kernel.org>
 
+allOf:
+  - $ref: input.yaml#
+  - $ref: matrix-keymap.yaml#
+
 properties:
   compatible:
     enum:
@@ -37,6 +41,10 @@  properties:
 
   wakeup-source: true
 
+  keypad,num-columns: true
+  keypad,num-rows: true
+  linux,keymap: true
+
   linux,input-no-autorepeat:
     type: boolean
     description:
@@ -81,12 +89,33 @@  patternProperties:
       - keypad,row
       - linux,code
 
+dependencies:
+  linux,keymap:
+    required:
+      - keypad,num-columns
+      - keypad,num-rows
+
 required:
   - compatible
   - reg
   - interrupts
-  - samsung,keypad-num-columns
-  - samsung,keypad-num-rows
+
+if:
+  required:
+    - linux,keymap
+then:
+  properties:
+    samsung,keypad-num-columns: false
+    samsung,keypad-num-rows: false
+  patternProperties:
+    '^key-[0-9a-z]+$': false
+else:
+  properties:
+    keypad,num-columns: false
+    keypad,num-rows: false
+  required:
+    - samsung,keypad-num-columns
+    - samsung,keypad-num-rows
 
 additionalProperties: false
 
@@ -94,8 +123,9 @@  examples:
   - |
     #include <dt-bindings/clock/exynos4.h>
     #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/input/input.h>
 
-    keypad@100a0000 {
+    keypad1@100a0000 {
         compatible = "samsung,s5pv210-keypad";
         reg = <0x100a0000 0x100>;
         interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
@@ -119,3 +149,24 @@  examples:
             linux,code = <3>;
         };
     };
+  - |
+    #include <dt-bindings/clock/exynos4.h>
+    #include <dt-bindings/interrupt-controller/arm-gic.h>
+    #include <dt-bindings/input/input.h>
+
+    keypad2@100a0000 {
+        compatible = "samsung,s5pv210-keypad";
+        reg = <0x100a0000 0x100>;
+        interrupts = <GIC_SPI 109 IRQ_TYPE_LEVEL_HIGH>;
+        clocks = <&clock CLK_KEYIF>;
+        clock-names = "keypad";
+
+        keypad,num-rows = <2>;
+        keypad,num-columns = <8>;
+        linux,keymap = <
+          MATRIX_KEY(0, 3, 2)
+          MATRIX_KEY(0, 4, 3)
+        >;
+        linux,input-no-autorepeat;
+        wakeup-source;
+    };