Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Tue, 25 Aug 2020 15:48:42 -0600
From:   Rob Herring <robh@kernel.org>
To:     Mark-PK Tsai <mark-pk.tsai@mediatek.com>
Cc:     maz@kernel.org, tglx@linutronix.de, jason@lakedaemon.net,
        matthias.bgg@gmail.com, linux-kernel@vger.kernel.org,
        devicetree@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-mediatek@lists.infradead.org, yj.chiang@mediatek.com,
        alix.wu@mediatek.com, daniel@0x0f.com
Subject: Re: [PATCH 2/2] dt-bindings: interrupt-controller: Add MStar
 interrupt controller
Message-ID: <20200825214842.GA1367012@bogus>
References: <20200819034231.20726-1-mark-pk.tsai@mediatek.com>
 <20200819034231.20726-3-mark-pk.tsai@mediatek.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200819034231.20726-3-mark-pk.tsai@mediatek.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Aug 19, 2020 at 11:42:31AM +0800, Mark-PK Tsai wrote:
> Add binding for MStar interrupt controller.
> 
> Signed-off-by: Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> ---
>  .../interrupt-controller/mstar,mst-intc.yaml  | 82 +++++++++++++++++++
>  1 file changed, 82 insertions(+)
>  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> 
> diff --git a/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> new file mode 100644
> index 000000000000..6e383315e529
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/interrupt-controller/mstar,mst-intc.yaml
> @@ -0,0 +1,82 @@
> +# SPDX-License-Identifier: GPL-2.0

Dual license new bindings.

(GPL-2.0-only OR BSD-2-Clause)

> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/interrupt-controller/mstar,mst-intc.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: MStar Interrupt Controller
> +
> +maintainers:
> +  - Mark-PK Tsai <mark-pk.tsai@mediatek.com>
> +
> +description: |+
> +  MStar, SigmaStar and Mediatek DTV SoCs contain multiple legacy
> +  interrupt controllers that routes interrupts to the GIC.
> +
> +  The HW block exposes a number of interrupt controllers, each
> +  can support up to 64 interrupts.
> +
> +allOf:
> +  - $ref: /schemas/interrupt-controller.yaml#

Drop this. It is applied based on node name matching already.

> +
> +properties:
> +  compatible:
> +    items:
> +      - const: mstar,mst-intc
> +      - enum:
> +          - mediatek,mt58xx-intc

Normally, the 1st entry would be enum as that's where you'd add new 
compatibles (as the fallback is constant). But if you don't forsee any 
additions, just make both 'const'

> +
> +  interrupt-controller: true
> +
> +  "#address-cells":
> +    enum: [ 0, 1, 2 ]

This would normally be 0 in an interrupt controller. It's only relevant 
if you have an 'interrupt-map' which this is the parent for.

> +  "#size-cells":
> +    enum: [ 1, 2 ]

And this should be dropped.

> +
> +  "#interrupt-cells":
> +    const: 3
> +    description: |
> +      Use the same format as specified by GIC in arm,gic.yaml.

That's odd. You have the same SPI and PPI stuff?

> +
> +  reg:
> +    description: |
> +      Physical base address of the mstar interrupt controller
> +      registers and length of memory mapped region.

Drop this. That's every 'reg' property.

> +    minItems: 1

maxItems is more logical.

> +
> +  mstar,irqs-map-range:
> +    description: |
> +      The range of parent interrupt controller's interrupt lines
> +      that are hardwired to mstar interrupt controller.

Is this <start size> or <start end>?

Really, this should just use 'interrupts' even though that's a bit 
verbose. Or be implied by the compatible string. What's the maximum 
number of parent interrupts?

In any case, we really need to stop having vendor specific properties 
for this.

> +    $ref: /schemas/types.yaml#/definitions/uint32-matrix
> +    items:
> +      minItems: 2
> +      maxItems: 2
> +
> +  mstar,intc-no-eoi:
> +    description: |

Don't need '|' if there's no formatting.

> +      Mark this controller has no End Of Interrupt(EOI) implementation.
> +      This is a empty, boolean property.

You can drop this line. The schema says this.

> +    type: boolean
> +
> +required:
> +  - compatible
> +  - reg
> +  - mstar,irqs-map-range
> +
> +additionalProperties: false
> +
> +examples:
> +  - |
> +    mst_intc0: interrupt-controller@1f2032d0 {
> +      compatible = "mstar,mst-intc", "mediatek,mt58xx-intc";
> +      interrupt-controller;
> +      #interrupt-cells = <3>;
> +      #address-cells = <1>;
> +      #size-cells = <1>;
> +      interrupt-parent = <&gic>;
> +      reg = <0x1f2032d0 0x30>;
> +      mstar,irqs-map-range = <0 63>;

Is 0 a PPI or SPI? This property is making some assumption and wouldn't 
be able to support both types or another parent interrupt controller.

> +    };
> +...
> -- 
> 2.18.0