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:   Thu, 27 Aug 2020 00:42:20 +0300
From:   Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
To:     Rob Herring <robh@kernel.org>
Cc:     Thomas Gleixner <tglx@linutronix.de>,
        Jason Cooper <jason@lakedaemon.net>,
        Marc Zyngier <maz@kernel.org>,
        Andreas =?iso-8859-1?Q?F=E4rber?= <afaerber@suse.de>,
        Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>,
        linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
        linux-arm-kernel@lists.infradead.org,
        linux-actions@lists.infradead.org
Subject: Re: [PATCH v5 1/3] dt-bindings: interrupt-controller: Add Actions
 SIRQ controller binding
Message-ID: <20200826214220.GA2444747@BV030612LT>
References: <cover.1597852360.git.cristian.ciocaltea@gmail.com>
 <6bd99d4a7e50904b57bb3ad050725fbb418874b7.1597852360.git.cristian.ciocaltea@gmail.com>
 <20200825220913.GA1423455@bogus>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200825220913.GA1423455@bogus>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Hi Rob,

Thanks for the review!

On Tue, Aug 25, 2020 at 04:09:13PM -0600, Rob Herring wrote:
> On Wed, Aug 19, 2020 at 07:37:56PM +0300, Cristian Ciocaltea wrote:
> > Actions Semi Owl SoCs SIRQ interrupt controller is found in S500, S700
> > and S900 SoCs and provides support for handling up to 3 external
> > interrupt lines.
> > 
> > Signed-off-by: Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > ---
> > Changes in v5:
> >  - Updated controller description statements both in the commit message
> >    and the binding doc
> > 
> >  .../actions,owl-sirq.yaml                     | 68 +++++++++++++++++++
> >  1 file changed, 68 insertions(+)
> >  create mode 100644 Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > 
> > diff --git a/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > new file mode 100644
> > index 000000000000..cf9b7a514e4e
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/interrupt-controller/actions,owl-sirq.yaml
> > @@ -0,0 +1,68 @@
> > +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
> > +%YAML 1.2
> > +---
> > +$id: http://devicetree.org/schemas/interrupt-controller/actions,owl-sirq.yaml#
> > +$schema: http://devicetree.org/meta-schemas/core.yaml#
> > +
> > +title: Actions Semi Owl SoCs SIRQ interrupt controller
> > +
> > +maintainers:
> > +  - Manivannan Sadhasivam <manivannan.sadhasivam@linaro.org>
> > +  - Cristian Ciocaltea <cristian.ciocaltea@gmail.com>
> > +
> > +description: |
> > +  This interrupt controller is found in the Actions Semi Owl SoCs (S500, S700
> > +  and S900) and provides support for handling up to 3 external interrupt lines.
> > +
> > +properties:
> > +  compatible:
> > +    oneOf:
> > +      - items:
> > +        - enum:
> > +          - actions,s500-sirq
> > +          - actions,s700-sirq
> > +          - actions,s900-sirq
> > +        - const: actions,owl-sirq
> > +      - const: actions,owl-sirq
> 
> This should be dropped. You should always have the SoC specific 
> compatible.

Sure, I will get rid of the 'owl-sirq' compatible.

> > +
> > +  reg:
> > +    maxItems: 1
> > +
> > +  interrupt-controller: true
> > +
> > +  '#interrupt-cells':
> > +    const: 2
> > +    description:
> > +      The first cell is the input IRQ number, between 0 and 2, while the second
> > +      cell is the trigger type as defined in interrupt.txt in this directory.
> > +
> > +  'actions,ext-interrupts':
> > +    description: |
> > +      Contains the GIC SPI IRQ numbers mapped to the external interrupt
> > +      lines. They shall be specified sequentially from output 0 to 2.
> > +    $ref: /schemas/types.yaml#/definitions/uint32-array
> > +    minItems: 3
> > +    maxItems: 3
> 
> Can't you use 'interrupts' here?

This was actually my initial idea, but it might confuse the users since
this is not following the parent controller IRQ specs, i.e. the trigger
type is set internally by the SIRQ driver, it's not taken from DT.

Please see the DTS sample bellow where both devices are on the same
level and have GIC as interrupt parent. The 'interrupts' property
in the sirq node looks incomplete now. That is why I decided to use
a custom name for it, although I'm not sure it's the most relevant one,
I am open to any other suggestion.

i2c0: i2c@b0170000 {
  [...]
  interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
  [...]
};

sirq: interrupt-controller@b01b0200 {
  [...]
  interrupt-controller;
  #interrupt-cells = <2>;
  interrupts = <13>, /* SIRQ0 */
               <14>, /* SIRQ1 */
               <15>; /* SIRQ2 */
};

Regards,
Cristi

> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +  - interrupt-controller
> > +  - '#interrupt-cells'
> > +  - 'actions,ext-interrupts'
> > +
> > +additionalProperties: false
> > +
> > +examples:
> > +  - |
> > +    sirq: interrupt-controller@b01b0200 {
> > +      compatible = "actions,s500-sirq", "actions,owl-sirq";
> > +      reg = <0xb01b0200 0x4>;
> > +      interrupt-controller;
> > +      #interrupt-cells = <2>;
> > +      actions,ext-interrupts = <13>, /* SIRQ0 */
> > +                               <14>, /* SIRQ1 */
> > +                               <15>; /* SIRQ2 */
> > +    };
> > +
> > +...
> > -- 
> > 2.28.0
> >