Re: [PATCH 2/8] dt-bindings: Add MAX7360 subdevices

[Krzysztof Kozlowski <krzk@kernel.org>]

On 19/12/2024 17:21, Mathieu Dubois-Briand wrote:
> ---
>  .../devicetree/bindings/gpio/max7360-gpio.yaml     | 96 ++++++++++++++++++++++
>  .../devicetree/bindings/input/max7360-keypad.yaml  | 67 +++++++++++++++
>  .../devicetree/bindings/input/max7360-rotary.yaml  | 52 ++++++++++++
>  .../devicetree/bindings/pwm/max7360-pwm.yaml       | 35 ++++++++
>  4 files changed, 250 insertions(+)


I don't understand how this patchset was split. MFD binding cannot be
empty and cannot be before child devices.

All filenames are wrong here: use compatibles.


> 
> diff --git a/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> new file mode 100644
> index 000000000000..3c006dc0380b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/max7360-gpio.yaml
> @@ -0,0 +1,96 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/max7360-gpio.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 GPIO controller
> +
> +maintainers:
> +  - Kamel Bouhara <kamel.bouhara@bootlin.com>
> +  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> +
> +description: |
> +  Maxim MAX7360 GPIO controller, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-gpio
> +      - maxim,max7360-gpo

Why? What are the differences?

> +
> +  gpio-controller: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  ngpios:
> +    minimum: 0
> +    maximum: 8

Why this is flexible?

> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.

Drop

> +    maxItems: 1
> +
> +  constant-current-disable:

You always need vendor prefix.

> +    $ref: /schemas/types.yaml#/definitions/uint32
> +    description: Bit field, each bit disables constant-current output of the
> +                 associated GPIO.

Oddly aligned.

Missing constraints.

> +
> +
> +required:
> +  - compatible
> +  - gpio-controller
> +  - ngpios
> +
> +if:
> +  properties:
> +    compatible:
> +      contains:
> +        enum:
> +          - maxim,max7360-gpio
> +then:
> +  required:
> +    - interrupt-controller
> +    - interrupts
> +else:
> +  properties:
> +    interrupt-controller: false
> +    interrupts: false
> +    constant-current-disable: false
> +
> +    ngpios:
> +      maximum: 6
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    max7360_gpo: max7360_gpo {

Plaese follow DTS coding style.

Keep only one, complete example.

> +            compatible = "maxim,max7360-gpo";
> +            gpio-controller;
> +            #gpio-cells = <0x2>;
> +            ngpios = <4>;

Odd indentation. Your MFD patch had very different one.

> +    };
> +
> +    max7360_gpio: max7360_gpio {
> +            compatible = "maxim,max7360-gpio";
> +
> +            gpio-controller;
> +            #gpio-cells = <0x2>;
> +            ngpios = <8>;
> +            constant-current-disable = <0x06>;
> +
> +            interrupt-controller;
> +            #interrupt-cells = <0x2>;
> +            interrupt-parent = <&gpio1>;
> +            interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> +    };
> diff --git a/Documentation/devicetree/bindings/input/max7360-keypad.yaml b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
> new file mode 100644
> index 000000000000..8bc3c841465b
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/max7360-keypad.yaml
> @@ -0,0 +1,67 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/max7360-keypad.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 Keypad Controller
> +
> +maintainers:
> +  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> +
> +description: |
> +  Maxim MAX7360 Keypad Controller, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +allOf:
> +  - $ref: matrix-keymap.yaml#
> +  - $ref: input.yaml#
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-keypad
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.


Really? Each separate device has its own interrupt line? How is it
possible if diagram here:
https://www.analog.com/en/products/max7360.html

has only one interrupt?

Fold the binding into the parent node.


> +    maxItems: 1
> +
> +  debounce-delay-ms:
> +    description: Debounce delay in ms
> +    minimum: 9
> +    maximum: 40
> +    default: 9
> +
> +  linux,input-no-autorepeat:
> +    description: If present, the keys will not autorepeat when pressed
> +
> +required:
> +  - compatible
> +  - interrupts
> +  - linux,keymap
> +
> +unevaluatedProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/input/input.h>
> +
> +    max7360_keypad {

Node names should be generic. See also an explanation and list of
examples (not exhaustive) in DT specification:
https://devicetree-specification.readthedocs.io/en/latest/chapter2-devicetree-basics.html#generic-names-recommendation


Please read and follow DTS coding style.

> +      compatible = "maxim,max7360-keypad";
> +
> +      keypad,num-rows = <8>;
> +      keypad,num-columns = <4>;
> +
> +      linux,keymap = <
> +        MATRIX_KEY(0x00, 0x00, KEY_F5)
> +        MATRIX_KEY(0x01, 0x00, KEY_F4)
> +        MATRIX_KEY(0x02, 0x01, KEY_F6)
> +        >;
> +
> +      interrupt-parent = <&gpio1>;
> +      interrupts = <23 IRQ_TYPE_LEVEL_LOW>;
> +
> +      debounce-delay-ms = <10>;
> +      linux,input-no-autorepeat;
> +    };
> diff --git a/Documentation/devicetree/bindings/input/max7360-rotary.yaml b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
> new file mode 100644
> index 000000000000..19afa8344249
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/input/max7360-rotary.yaml
> @@ -0,0 +1,52 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/input/max7360-rotary.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Maxim MAX7360 Rotary Encoder
> +
> +maintainers:
> +  - Mathieu Dubois-Briand <mathieu.dubois-briand@bootlin.com>
> +
> +description: |
> +  Maxim MAX7360 Rotary Encoder, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-rotary
> +
> +  interrupts:
> +    description: The interrupt line the device is connected to.
> +    maxItems: 1
> +
> +  debounce-delay-ms:
> +    description: Debounce delay in ms
> +    minimum: 0
> +    maximum: 15
> +    default: 0
> +
> +  linux,axis:
> +    description: The input subsystem axis to map to this rotary encoder.
> +

Fold into parent node.

> +description: |
> +  Maxim MAX7360 PWM controller, in MAX7360 MFD
> +  https://www.analog.com/en/products/max7360.html
> +
> +properties:
> +  compatible:
> +    enum:
> +      - maxim,max7360-pwm
> +
> +  "#pwm-cells":
> +    const: 2
> +
> +


Fold into parent.



Best regards,
Krzysztof