On Mon, 2022-07-18 at 16:27 +0200, Krzysztof Kozlowski wrote:

On 15/07/2022 14:51, Nuno Sá wrote:

quoted

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.

quoted

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...

quoted

+$schema: http://devicetree.org/meta-schemas/core.yaml#
+
+title: Analog Devices ADP5588 Keypad Controller
+
+maintainers:
+  - Nuno Sá [off-list ref]
+
+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...

quoted

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

quoted

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

quoted

+          #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á