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

[Nuno Sá <noname.nuno@xxxxxxxxx>]

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@xxxxxxxxxx>
> > ---
> >  .../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@xxxxxxxxxx>
> > +
> > +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á