diff mbox series

[v2,05/10] dt-bindings: input: adp5588-keys: add bindings

Message ID 20220715125138.378632-6-nuno.sa@analog.com
State Superseded
Headers show
Series adp5588-keys refactor and fw properties support | expand

Commit Message

Nuno Sa July 15, 2022, 12:51 p.m. UTC
Add device tree bindings for the adp5588-keys driver.

Signed-off-by: Nuno Sá <nuno.sa@analog.com>
---
 .../bindings/input/adi,adp5588-keys.yaml      | 110 ++++++++++++++++++
 MAINTAINERS                                   |   1 +
 2 files changed, 111 insertions(+)
 create mode 100644 Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml

Comments

Krzysztof Kozlowski July 18, 2022, 2:27 p.m. UTC | #1
On 15/07/2022 14:51, Nuno Sá wrote:
> Add device tree bindings for the adp5588-keys driver.
> 
> Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> ---
>  .../bindings/input/adi,adp5588-keys.yaml      | 110 ++++++++++++++++++
>  MAINTAINERS                                   |   1 +
>  2 files changed, 111 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml

Thank you for your patch. There is something to discuss/improve.

> 
> diff --git a/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> new file mode 100644
> index 000000000000..0d262b42150a
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> @@ -0,0 +1,110 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/adi,adp5588-keys.yaml#

Is ADP5588 a multi-function device? If not, I propose to drop the "keys"
suffix.

> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Analog Devices ADP5588 Keypad Controller
> +
> +maintainers:
> +  - Nuno Sá <nuno.sa@analog.com>
> +
> +description: |
> +  Analog Devices Mobile I/O Expander and QWERTY Keypad Controller
> +  https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5588.pdf
> +
> +allOf:
> +  - $ref: matrix-keymap.yaml#
> +  - $ref: input.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - adi,adp5588
> +      - adi,adp5587

Bring some order, like alphabetical.

> +
> +  reg:
> +    maxItems: 1
> +
> +  vcc-supply:
> +    description: Supply Voltage Input
> +
> +  reset-gpios:
> +    description:
> +      If specified, it will be asserted during driver probe. As the line is
> +      active low, it should be marked GPIO_ACTIVE_LOW.
> +    maxItems: 1
> +
> +  interrupts:
> +    maxItems: 1
> +
> +  gpio-controller:
> +    description:
> +      This property applies if either keypad,num-rows lower than 8 or
> +      keypad,num-columns lower than 10.
> +
> +  '#gpio-cells':
> +    const: 2
> +
> +  interrupt-controller:
> +    description:
> +      This property applies if either keypad,num-rows lower than 8 or
> +      keypad,num-columns lower than 10.
> +
> +  '#interrupt-cells':
> +    const: 2
> +
> +  adi,unlock-keys:
> +    description:
> +      Specifies a maximum of 2 keys that can be used to unlock the keypad.
> +      If this property is set, the keyboard will be locked and only unlocked
> +      after these keys are pressed. If only one key is set, a double click is
> +      needed to unlock the keypad.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +    minItems: 1
> +    maxItems: 2

What are the values/units? keycodes? If so, maybe "adi,unlock-keycodes"
and add limit on values (like linux,keycodes).

> +
> +required:
> +  - compatible
> +  - reg
> +  - interrupts
> +  - keypad,num-rows
> +  - keypad,num-columns
> +  - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/input/input.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    i2c {
> +          #address-cells = <1>;

Wrong indentation. 4 spaces for DTS example.

> +          #size-cells = <0>;
> +
> +          adp5588@34 {

Node names should be generic.
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Best regards,
Krzysztof
Nuno Sá July 18, 2022, 2:52 p.m. UTC | #2
On Mon, 2022-07-18 at 16:27 +0200, Krzysztof Kozlowski wrote:
> On 15/07/2022 14:51, Nuno Sá wrote:
> > Add device tree bindings for the adp5588-keys driver.
> > 
> > Signed-off-by: Nuno Sá <nuno.sa@analog.com>
> > ---
> >  .../bindings/input/adi,adp5588-keys.yaml      | 110
> > ++++++++++++++++++
> >  MAINTAINERS                                   |   1 +
> >  2 files changed, 111 insertions(+)
> >  create mode 100644
> > Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > 
> > diff --git a/Documentation/devicetree/bindings/input/adi,adp5588-
> > keys.yaml b/Documentation/devicetree/bindings/input/adi,adp5588-
> > keys.yaml
> > new file mode 100644
> > index 000000000000..0d262b42150a
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
> > @@ -0,0 +1,110 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/adi,adp5588-keys.yaml#
> 
> Is ADP5588 a multi-function device? If not, I propose to drop the
> "keys"
> suffix.
> 

I can do that. I just kept the same naming of the c file...

> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADP5588 Keypad Controller
> > +
> > +maintainers:
> > +  - Nuno Sá <nuno.sa@analog.com>
> > +
> > +description: |
> > +  Analog Devices Mobile I/O Expander and QWERTY Keypad Controller
> > + 
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5588.pdf
> > +
> > +allOf:
> > +  - $ref: matrix-keymap.yaml#
> > +  - $ref: input.yaml#
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adp5588
> > +      - adi,adp5587
> 
> Bring some order, like alphabetical.
> 

ack...

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vcc-supply:
> > +    description: Supply Voltage Input
> > +
> > +  reset-gpios:
> > +    description:
> > +      If specified, it will be asserted during driver probe. As
> > the line is
> > +      active low, it should be marked GPIO_ACTIVE_LOW.
> > +    maxItems: 1
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  gpio-controller:
> > +    description:
> > +      This property applies if either keypad,num-rows lower than 8
> > or
> > +      keypad,num-columns lower than 10.
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  interrupt-controller:
> > +    description:
> > +      This property applies if either keypad,num-rows lower than 8
> > or
> > +      keypad,num-columns lower than 10.
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  adi,unlock-keys:
> > +    description:
> > +      Specifies a maximum of 2 keys that can be used to unlock the
> > keypad.
> > +      If this property is set, the keyboard will be locked and
> > only unlocked
> > +      after these keys are pressed. If only one key is set, a
> > double click is
> > +      needed to unlock the keypad.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 2
> 
> What are the values/units? keycodes? If so, maybe "adi,unlock-
> keycodes"
> and add limit on values (like linux,keycodes).
> 

They are just key numbers on the matrix. In the driver there's actually
a check so that the value cannot be >= keypad,num-rows * keypad,num-
columns. Not sure if there's a way to express this but I can surely
improve the description.

(There's also a comment on the driver regarding why I'm doing this
check).

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupts
> > +  - keypad,num-rows
> > +  - keypad,num-columns
> > +  - linux,keymap
> > +
> > +unevaluatedProperties: false
> > +
> > +examples:
> > +  - |
> > +    #include <dt-bindings/interrupt-controller/irq.h>
> > +    #include <dt-bindings/input/input.h>
> > +    #include <dt-bindings/gpio/gpio.h>
> > +    i2c {
> > +          #address-cells = <1>;
> 
> Wrong indentation. 4 spaces for DTS example.
> 

ack

> > +          #size-cells = <0>;
> > +
> > +          adp5588@34 {
> 
> Node names should be generic.
> https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation
> 
> 

ack

- Nuno Sá
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
new file mode 100644
index 000000000000..0d262b42150a
--- /dev/null
+++ b/Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
@@ -0,0 +1,110 @@ 
+# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
+%YAML 1.2
+---
+$id: http://devicetree.org/schemas/input/adi,adp5588-keys.yaml#
+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADP5588 Keypad Controller
+
+maintainers:
+  - Nuno Sá <nuno.sa@analog.com>
+
+description: |
+  Analog Devices Mobile I/O Expander and QWERTY Keypad Controller
+  https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5588.pdf
+
+allOf:
+  - $ref: matrix-keymap.yaml#
+  - $ref: input.yaml#
+
+properties:
+  compatible:
+    enum:
+      - adi,adp5588
+      - adi,adp5587
+
+  reg:
+    maxItems: 1
+
+  vcc-supply:
+    description: Supply Voltage Input
+
+  reset-gpios:
+    description:
+      If specified, it will be asserted during driver probe. As the line is
+      active low, it should be marked GPIO_ACTIVE_LOW.
+    maxItems: 1
+
+  interrupts:
+    maxItems: 1
+
+  gpio-controller:
+    description:
+      This property applies if either keypad,num-rows lower than 8 or
+      keypad,num-columns lower than 10.
+
+  '#gpio-cells':
+    const: 2
+
+  interrupt-controller:
+    description:
+      This property applies if either keypad,num-rows lower than 8 or
+      keypad,num-columns lower than 10.
+
+  '#interrupt-cells':
+    const: 2
+
+  adi,unlock-keys:
+    description:
+      Specifies a maximum of 2 keys that can be used to unlock the keypad.
+      If this property is set, the keyboard will be locked and only unlocked
+      after these keys are pressed. If only one key is set, a double click is
+      needed to unlock the keypad.
+    $ref: /schemas/types.yaml#/definitions/uint32-array
+    minItems: 1
+    maxItems: 2
+
+required:
+  - compatible
+  - reg
+  - interrupts
+  - keypad,num-rows
+  - keypad,num-columns
+  - linux,keymap
+
+unevaluatedProperties: false
+
+examples:
+  - |
+    #include <dt-bindings/interrupt-controller/irq.h>
+    #include <dt-bindings/input/input.h>
+    #include <dt-bindings/gpio/gpio.h>
+    i2c {
+          #address-cells = <1>;
+          #size-cells = <0>;
+
+          adp5588@34 {
+                  compatible = "adi,adp5588-keys";
+                  reg = <0x34>;
+
+                  vcc-supply = <&vcc>;
+                  interrupts = <21 IRQ_TYPE_EDGE_FALLING>;
+                  interrupt-parent = <&gpio>;
+                  reset-gpios = <&gpio 20 GPIO_ACTIVE_LOW>;
+
+                  keypad,num-rows = <1>;
+                  keypad,num-columns = <9>;
+                  linux,keymap = <
+                        MATRIX_KEY(0x00, 0x00, KEY_1)
+                        MATRIX_KEY(0x00, 0x01, KEY_2)
+                        MATRIX_KEY(0x00, 0x02, KEY_3)
+                        MATRIX_KEY(0x00, 0x03, KEY_4)
+                        MATRIX_KEY(0x00, 0x04, KEY_5)
+                        MATRIX_KEY(0x00, 0x05, KEY_6)
+                        MATRIX_KEY(0x00, 0x06, KEY_7)
+                        MATRIX_KEY(0x00, 0x07, KEY_8)
+                        MATRIX_KEY(0x00, 0x08, KEY_9)
+                 >;
+          };
+    };
+...
diff --git a/MAINTAINERS b/MAINTAINERS
index 6d2ef0726291..a654de117d41 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -549,6 +549,7 @@  M:	Michael Hennerich <michael.hennerich@analog.com>
 S:	Supported
 W:	http://wiki.analog.com/ADP5588
 W:	https://ez.analog.com/linux-software-drivers
+F:	Documentation/devicetree/bindings/input/adi,adp5588-keys.yaml
 F:	drivers/input/keyboard/adp5588-keys.c
 
 ADP8860 BACKLIGHT DRIVER (ADP8860/ADP8861/ADP8863)