Re: [PATCH 2/2] dt-bindings: pinctrl: Add bindings for Awinic AW9523/AW9523B

[Linus Walleij]

On Mon, Jan 25, 2021 at 7:21 PM AngeloGioacchino Del Regno
<angelogioacchino.delregno@xxxxxxxxxxxxxx> wrote:

> Add bindings for the Awinic AW9523/AW9523B I2C GPIO Expander driver.
>
> Signed-off-by: AngeloGioacchino Del Regno <angelogioacchino.delregno@xxxxxxxxxxxxxx>
(...)
> +  '#interrupt-cells':
> +    description:
> +      Specifies the PIN numbers and Flags, as defined in defined in
> +      include/dt-bindings/interrupt-controller/irq.h
> +    const: 2

Not really. Expand this to at least say in the description that the interrupt
must be IRQ_TYPE_EDGE_BOTH.

Preferably we should enforce it with the schema but I don't know if
that is possible in any easy way.

> +examples:
> +  # Example configuration to drive pins for a keyboard matrix
> +  - |
> +    #include <dt-bindings/gpio/gpio.h>
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +
> +    i2c {
> +        #address-cells = <1>;
> +        #size-cells = <0>;
> +
> +        aw9523: gpio-expander@58 {
> +                compatible = "awinic,aw9523-pinctrl";
> +                reg = <0x58>;
> +                interrupt-parent = <&tlmm>;
> +                interrupts = <50 IRQ_TYPE_EDGE_FALLING>;
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                gpio-ranges = <&tlmm 0 0 16>;
> +                interrupt-controller;
> +                #interrupt-cells = <2>;
> +                reset-gpios = <&tlmm 51 GPIO_ACTIVE_HIGH>;
> +
> +                keyboard-matrix-col-pins {
> +                        pins = "gpio8", "gpio9", "gpio10", "gpio11",
> +                               "gpio12", "gpio13", "gpio14", "gpio15";
> +                        function = "gpio";
> +                        input-disable;
> +                        output-low;
> +                };
> +
> +                keyboard-matrix-row-pins {
> +                        pins = "gpio0", "gpio1", "gpio2", "gpio3",
> +                               "gpio4", "gpio5", "gpio6", "gpio7";
> +                        function = "gpio";
> +                        bias-pull-up;
> +                        drive-open-drain;
> +                        input-enable;
> +                };
> +        };
> +    };

This is fine, but what about adding an example consumer using
<&aw9523 0 IRQ_TYPE_EDGE_BOTH>?

Yours,
Linus Walleij