Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 24 Apr 2023 11:32:03 -0500
From:   Rob Herring <robh@kernel.org>
To:     Nikita Shubin <nikita.shubin@maquefel.me>
Cc:     Arnd Bergmann <arnd@kernel.org>, Linus Walleij <linusw@kernel.org>,
        Alexander Sverdlin <alexander.sverdlin@gmail.com>,
        Linus Walleij <linus.walleij@linaro.org>,
        Bartosz Golaszewski <brgl@bgdev.pl>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        linux-gpio@vger.kernel.org, devicetree@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH 32/43] dt-bindings: gpio: Add DT bindings ep93xx gpio
Message-ID: <20230424163203.GK2701399-robh@kernel.org>
References: <20230424123522.18302-1-nikita.shubin@maquefel.me>
 <20230424123522.18302-33-nikita.shubin@maquefel.me>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230424123522.18302-33-nikita.shubin@maquefel.me>
Precedence: bulk

On Mon, Apr 24, 2023 at 03:34:48PM +0300, Nikita Shubin wrote:
> Add YAML bindings for ep93xx SoC.
> 
> Signed-off-by: Nikita Shubin <nikita.shubin@maquefel.me>
> ---
>  .../devicetree/bindings/gpio/gpio-ep93xx.yaml | 161 ++++++++++++++++++
>  1 file changed, 161 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml
> 
> diff --git a/Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml b/Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml
> new file mode 100644
> index 000000000000..4cf03c325d39
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/gpio/gpio-ep93xx.yaml
> @@ -0,0 +1,161 @@
> +# SPDX-License-Identifier: (GPL-2.0 OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/gpio/gpio-ep93xx.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: EP93xx GPIO controller
> +
> +maintainers:
> +  - Linus Walleij <linus.walleij@linaro.org>
> +  - Bartosz Golaszewski <brgl@bgdev.pl>
> +  - Nikita Shubin <nikita.shubin@maquefel.me>
> +
> +properties:
> +  compatible:
> +    const: cirrus,ep9301-gpio
> +
> +  chip-label:
> +    maxItems: 1
> +    description: human readable name.

Why do you need this? It's not standard and I don't see other GPIO 
controllers needing it.


> +
> +  reg:
> +    minItems: 2
> +    items:
> +      - description: data register
> +      - description: direction register
> +      - description: interrupt registers base
> +
> +  reg-names:
> +    minItems: 2
> +    items:
> +      - const: data
> +      - const: dir
> +      - const: intr
> +
> +  gpio-controller: true
> +
> +  gpio-ranges: true
> +
> +  "#gpio-cells":
> +    const: 2
> +
> +  interrupt-controller: true
> +
> +  "#interrupt-cells":
> +    const: 2
> +
> +  interrupts:
> +    items:
> +      - const: 27

The value of the interrupt cells depends on the parent which is outside 
the scope of this binding. Just 'maxItems: 1' is sufficient.

> +
> +  interrupts-extended:

'interrupts' covers interrupts-extended. So 'interrupts' should be:

interrupts:
  oneOf:
    - maxItems: 1
    - description: port F has dedicated irq line for each gpio line
      maxItems: 8

> +    minItems: 8
> +    maxItems: 8
> +    description: port F has dedicated irq line for each gpio line.
> +
> +required:
> +  - compatible
> +  - reg
> +  - gpio-controller
> +  - "#gpio-cells"
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +        gpio0: gpio@80840000 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "A";
> +                reg = <0x80840000 0x04>,
> +                      <0x80840010 0x04>,
> +                      <0x80840090 0x1c>;
> +                reg-names = "data", "dir", "intr";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                interrupt-controller;
> +                interrupt-parent = <&vic1>;
> +                interrupts = <27>;
> +        };
> +
> +        gpio1: gpio@80840004 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "B";
> +                reg = <0x80840004 0x04>,
> +                      <0x80840014 0x04>,
> +                      <0x808400ac 0x1c>;
> +                reg-names = "data", "dir", "intr";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                interrupt-controller;
> +                interrupt-parent = <&vic1>;
> +                interrupts = <27>;
> +        };
> +
> +        gpio2: gpio@80840008 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "C";
> +                reg = <0x80840008 0x04>,
> +                      <0x80840018 0x04>;
> +                reg-names = "data", "dir";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +        };
> +
> +        gpio3: gpio@8084000c {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "D";
> +                reg = <0x8084000c 0x04>,
> +                      <0x8084001c 0x04>;
> +                reg-names = "data", "dir";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +        };
> +
> +        gpio4: gpio@80840020 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "E";
> +                reg = <0x80840020 0x04>,
> +                      <0x80840024 0x04>;
> +                reg-names = "data", "dir";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +        };
> +
> +        gpio5: gpio@80840030 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "F";
> +                reg = <0x80840030 0x04>,
> +                      <0x80840034 0x04>,
> +                      <0x8084004c 0x1c>;
> +                reg-names = "data", "dir", "intr";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +                interrupt-controller;
> +                interrupts-extended = <&vic0 19>, <&vic0 20>,
> +                                      <&vic0 21>, <&vic0 22>,
> +                                      <&vic1 15>, <&vic1 16>,
> +                                      <&vic1 17>, <&vic1 18>;
> +        };
> +
> +        gpio6: gpio@80840038 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "G";
> +                reg = <0x80840038 0x04>,
> +                      <0x8084003c 0x04>;
> +                reg-names = "data", "dir";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +        };
> +
> +        gpio7: gpio@80840040 {
> +                compatible = "cirrus,ep9301-gpio";
> +                chip-label = "H";
> +                reg = <0x80840040 0x04>,
> +                      <0x80840044 0x04>;
> +                reg-names = "data", "dir";
> +                gpio-controller;
> +                #gpio-cells = <2>;
> +        };
> +
> +...
> -- 
> 2.39.2
>