Re: [PATCH 05/13] dt-bindings: input: Document adp5589 and similar devices

["Nuno Sá" <noname.nuno@gmail.com>]

On Wed, 2024-10-02 at 15:58 -0500, Rob Herring wrote:
> On Tue, Oct 01, 2024 at 03:41:36PM +0200, Nuno Sa wrote:
> > Add device tree bindings for the adp5589 keypad (and similar devices). The
> > ADP5585 family has small differences like the lack of the unlock
> > function and less pins (cols x rows) for the keymap.
> > 
> > As there's no MAINTAINERS entry for these devices, add one. Also to note
> > that these devices were removed from trivial-devices.yaml.
> > 
> > Signed-off-by: Nuno Sa <nuno.sa@analog.com>
> > ---
> >  .../devicetree/bindings/input/adi,adp5589.yaml     | 310
> > +++++++++++++++++++++
> >  .../devicetree/bindings/trivial-devices.yaml       |   6 -
> >  MAINTAINERS                                        |   9 +
> >  3 files changed, 319 insertions(+), 6 deletions(-)
> > 
> > diff --git a/Documentation/devicetree/bindings/input/adi,adp5589.yaml
> > b/Documentation/devicetree/bindings/input/adi,adp5589.yaml
> > new file mode 100644
> > index
> > 0000000000000000000000000000000000000000..bdbc79758a0390952c8363ec28f48057da
> > b929a9
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/input/adi,adp5589.yaml
> > @@ -0,0 +1,310 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/input/adi,adp5589.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Analog Devices ADP5589 and similar Keypad Controllers
> > +
> > +maintainers:
> > +  - Nuno Sa <nuno.sa@analog.com>
> > +  - Michael Hennerich <michael.hennerich@analog.com>
> > +
> > +description: |
> > +  Analog Devices Mobile I/O Expander and QWERTY Keypad Controllers
> > +   -
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5589.pdf
> > +   -
> > https://www.analog.com/media/en/technical-documentation/data-sheets/ADP5585.pdf
> > +
> > +properties:
> > +  compatible:
> > +    enum:
> > +      - adi,adp5589
> > +      - adi,adp5585
> > +      - adi,adp5585-2
> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  vcc-supply: true
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  gpio-controller:
> > +    description:
> > +      This property applies if there are pins not used in the keypad.
> > +
> > +  '#gpio-cells':
> > +    const: 2
> > +
> > +  interrupt-controller:
> > +    description:
> > +      This property applies if there are pins not used in the keypad.
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +
> > +  adi,cols-mask:
> > +    description:
> > +      Defines the number of pins (columns) being used ad part of the
> > keymap. As
> > +      the device is fully configurable and we can have holes in the columns
> > +      being used, this is given as mask.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0x1
> > +    maximum: 0x3f
> > +
> > +  adi,rows-mask:
> > +    description:
> > +      Defines the number of pins (rows) being used ad part of the keymap.
> > As
> > +      the device is fully configurable and we can have holes in the rows
> > being
> > +      used, this is given as mask.
> > +    $ref: /schemas/types.yaml#/definitions/uint32
> > +    minimum: 0x1
> > +    maximum: 0xff
> > +
> > +  adi,key-poll-ms:
> > +    description: Configure time between consecutive scan cycles.
> > +    enum: [10, 20, 30, 40]
> > +    default: 10
> > +
> > +  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. The value 127 serves as a wildcard
> > which
> > +      means any key can be used for unlocking.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 2
> 
> You probably don't allow repeated key values? If so, then you can add 
> 'uniqueItems: true' to enforce that.
> 

Oh nice. Indeed repeated keys make no sense...

> > +    items:
> > +      anyOf:
> > +        - minimum: 1
> > +          maximum: 88
> > +        - minimum: 97
> > +          maximum: 115
> > +        - const: 127
> > +
> > +  adi,unlock-trigger-sec:
> > +    description:
> > +      Defines the time in which the second unlock event must occur after
> > the
> > +      first unlock event has occurred.
> > +    maximum: 7
> > +    default: 0
> > +
> > +  adi,reset1-keys:
> > +    description:
> > +      Defines the trigger events (key presses) that can generate reset
> > +      conditions one the reset1 block.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 3
> > +    items:
> > +      anyOf:
> > +        - minimum: 1
> > +          maximum: 88
> > +        - minimum: 97
> > +          maximum: 115
> > +
> > +  adi,reset2-keys:
> > +    description:
> > +      Defines the trigger events (key presses) that can generate reset
> > +      conditions one the reset2 block.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 1
> > +    maxItems: 2
> > +    items:
> > +      anyOf:
> > +        - minimum: 1
> > +          maximum: 88
> > +        - minimum: 97
> > +          maximum: 115
> > +
> > +  adi,reset1-active-high:
> > +    description: Sets the reset1 signal as active high.
> > +    type: boolean
> > +
> > +  adi,reset2-active-high:
> > +    description: Sets the reset2 signal as active high.
> > +    type: boolean
> > +
> > +  adi,rst-passtrough-enable:
> 
> passthrough
> 
> > +    description: Allows the RST pin to override (OR with) the reset1
> > signal.
> > +    type: boolean
> > +
> > +  adi,reset-trigger-ms:
> > +    description:
> > +      Defines the length of time that the reset events must be active
> > before a
> > +      reset signal is generated. All events must be active at the same time
> > for
> > +      the same duration.
> > +    enum: [0, 1000, 1500, 2000, 2500, 3000, 3500, 4000]
> > +    default: 0
> > +
> > +  adi,reset-pulse-width-us:
> > +    description: Defines the pulse width of the reset signals.
> > +    enum: [500, 1000, 2000, 10000]
> > +    default: 500
> > +
> > +  '#address-cells':
> > +    const: 1
> > +
> > +  '#size-cells':
> > +    const: 0
> > +
> > +patternProperties:
> > +  "^gpio@":
> 
> 'gpio' for node name is for gpio-controllers which this is not.

Well this part is also a gpio-controllers but given your comment below, I'll
replace the child nodes for an array.
> 
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      reg:
> > +        description: The GPIO number being configured.
> > +        maximum: 18
> > +
> > +      adi,pull-up-ohms:
> > +        description: The pullup resistor to be used.
> > +        enum: [100000, 300000]
> > +        default: 300000
> 
> Key mode doesn't have a pull-up?
> 

Fair question... I'm taking the same approach as another similar part I
refactored a while ago. Which is, the pin bias is configured through the GPIO
subsystem when the pins are used as GPIOs. Seems to me the more sensible usecase
but truth to be said, there's nothing on the datasheet enforcing that...

Anyways, moving this into the array will "drop" this restriction at least in the
bindings.

- Nuno Sá
>