Re: [PATCH v2 1/6] dt-bindings: interrupt-controller: Add support for Realtek DHC SoCs

[Krzysztof Kozlowski]

On 17/11/2023 17:27, James Tai wrote:
> Add the YAML documentation for Realtek DHC SoCs.
> 
> CC: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> CC: Marc Zyngier <maz@xxxxxxxxxx>
> CC: Rob Herring <robh+dt@xxxxxxxxxx>
> CC: Krzysztof Kozlowski <krzysztof.kozlowski+dt@xxxxxxxxxx>
> CC: Conor Dooley <conor+dt@xxxxxxxxxx>
> CC: linux-kernel@xxxxxxxxxxxxxxx
> CC: devicetree@xxxxxxxxxxxxxxx
> Signed-off-by: James Tai <james.tai@xxxxxxxxxxx>
> ---
> v1 to v2 change:
> - Tested the bindings using 'make dt_binding_check'

I doubt it.

And bot prooves it.

> - Fixed code style issues

Be specific - what code style issues did you fix?

> 
>  .../realtek,rtd1319-intc.yaml                 | 79 +++++++++++++++++++
>  .../realtek,rtd1319d-intc.yaml                | 79 +++++++++++++++++++
>  .../realtek,rtd1325-intc.yaml                 | 79 +++++++++++++++++++
>  .../realtek,rtd1619b-intc.yaml                | 78 ++++++++++++++++++
>  4 files changed, 315 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319-intc.yaml
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319d-intc.yaml
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1325-intc.yaml
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1619b-intc.yaml

Why do you have four bindings for the same? Please explain me the
differences.

> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319-intc.yaml
> new file mode 100644
> index 000000000000..b88f3ac07cd9
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319-intc.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/realtek,rtd1319-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC RTD1319 Interrupt Controller Device Tree Bindings
> +
> +description:
> +  This interrupt controller is a component of Realtek DHC RTD1319 and
> +  is designed to receive interrupts from peripheral devices.
> +
> +  Each DHC SoC has two sets of interrupt controllers, each capable of
> +  handling up to 32 interrupts.
> +
> +maintainers:
> +  - James Tai <james.tai@xxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  "#interrupt-cells":
> +    const: 1


compatible is first, put the cells next to other interrupt controller
properties.

> +
> +  compatible:
> +    enum:
> +      - realtek,rtd1319-intc-iso
> +      - realtek,rtd1319-intc-misc
> +
> +  "#address-cells":
> +    const: 0
> +
> +  interrupt-controller: true
> +
> +  interrupts-extended:

interrupts instead.

Anyway, you must describe the items. Why this is not fixed but flexible?
Hardware has different number of pins? That's unlikely.

> +    minItems: 1
> +    maxItems: 4
> +
> +  reg:
> +    maxItems: 1
> +
> +required:
> +  - "#interrupt-cells"
> +  - "#address-cells"
> +  - compatible
> +  - interrupt-controller
> +  - interrupts-extended
> +  - reg
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/irq.h>
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +
> +    rtd1319_iso_irq: interrupt-controller@40 {
> +      compatible = "realtek,rtd1319-intc-iso";
> +      reg = <0x00 0x40>;
> +      interrupts-extended = <&gic GIC_SPI 41 IRQ_TYPE_LEVEL_HIGH>,
> +                            <&gic GIC_SPI 0 IRQ_TYPE_LEVEL_HIGH>;
> +      interrupt-controller;
> +      #address-cells = <0>;
> +      #interrupt-cells = <1>;
> +    };
> +
> +    rtd1319_misc_irq: interrupt-controller@80 {
> +      compatible = "realtek,rtd1319-intc-misc";

Drop, one example is enough. This is the same as previous.

> +      reg = <0x00 0x80>;
> +      interrupts-extended = <&gic GIC_SPI 40 IRQ_TYPE_LEVEL_HIGH>,
> +                            <&gic GIC_SPI 89 IRQ_TYPE_LEVEL_HIGH>,
> +                            <&gic GIC_SPI 90 IRQ_TYPE_LEVEL_HIGH>,
> +                            <&gic GIC_SPI 39 IRQ_TYPE_LEVEL_HIGH>;
> +      interrupt-controller;
> +      #address-cells = <0>;
> +      #interrupt-cells = <1>;
> +    };
> +...
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319d-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319d-intc.yaml
> new file mode 100644
> index 000000000000..75aba448baf7
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/realtek,rtd1319d-intc.yaml
> @@ -0,0 +1,79 @@
> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/realtek,rtd1319d-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Realtek DHC RTD1319D Interrupt Controller Device Tree Bindings
> +
> +description:
> +  This interrupt controller is a component of Realtek DHC RTD1319D and
> +  is designed to receive interrupts from peripheral devices.
> +
> +  Each DHC SoC has two sets of interrupt controllers, each capable of
> +  handling up to 32 interrupts.
> +
> +maintainers:
> +  - James Tai <james.tai@xxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  "#interrupt-cells":
> +    const: 1
> +
> +  compatible:
> +    enum:
> +      - realtek,rtd1319d-intc-iso
> +      - realtek,rtd1319d-intc-misc

So this is the same as the other one? Why it cannot be part of that one?
...

> +
> +maintainers:
> +  - James Tai <james.tai@xxxxxxxxxxx>
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#
> +
> +properties:
> +  "#interrupt-cells":
> +    const: 1
> +
> +  compatible:
> +    enum:
> +      - realtek,rtd1325-intc-iso
> +      - realtek,rtd1325-intc-misc

All my comments apply to all your bindings...


Best regards,
Krzysztof