linux-kernel - Re: [PATCH 2/2] dt-bindings: serial: renesas,scif: Validate 'interrupts' and 'interrupt-names'

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CA+V-a8v=zMW4oW+fH1nX70fEXP=f9BhWDYgxMy_LpuXvVG5wow@mail.gmail.com>
Date: Thu, 7 Mar 2024 10:03:15 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: Geert Uytterhoeven <geert@...ux-m68k.org>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>, Jiri Slaby <jirislaby@...nel.org>, 
	Rob Herring <robh+dt@...nel.org>, 
	Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>, 
	Magnus Damm <magnus.damm@...il.com>, linux-kernel@...r.kernel.org, 
	linux-serial@...r.kernel.org, devicetree@...r.kernel.org, 
	linux-renesas-soc@...r.kernel.org, 
	Fabrizio Castro <fabrizio.castro.jz@...esas.com>, 
	Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH 2/2] dt-bindings: serial: renesas,scif: Validate
 'interrupts' and 'interrupt-names'

Hi Geert,

Thank you for the review.

On Thu, Mar 7, 2024 at 8:58 AM Geert Uytterhoeven <geert@...ux-m68korg> wrote:
>
> Hi Prabhakar,
>
> On Thu, Mar 7, 2024 at 12:11 AM Prabhakar <prabhakar.csengg@...ilcom> wrote:
> > From: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
> >
> > This commit adds support to validate the 'interrupts' and 'interrupt-names'
> > properties for every supported SoC. This ensures proper handling and
> > configuration of interrupt-related properties across supported platforms.
> >
> > Signed-off-by: Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
>
> Thanks for your patch!
>
> > --- a/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > +++ b/Documentation/devicetree/bindings/serial/renesas,scif.yaml
> > @@ -82,38 +82,6 @@ properties:
> >    reg:
> >      maxItems: 1
> >
> > -  interrupts:
> > -    oneOf:
> > -      - items:
> > -          - description: A combined interrupt
> > -      - items:
> > -          - description: Error interrupt
> > -          - description: Receive buffer full interrupt
> > -          - description: Transmit buffer empty interrupt
> > -          - description: Break interrupt
> > -      - items:
> > -          - description: Error interrupt
> > -          - description: Receive buffer full interrupt
> > -          - description: Transmit buffer empty interrupt
> > -          - description: Break interrupt
> > -          - description: Data Ready interrupt
> > -          - description: Transmit End interrupt
>
> As the above three groups are increasing supersets, you can just use
> a single "items" listing all 6 interrupts, and describe the first one
> as "Error interrupt or single combined interrupt".  After that, the
> SoC-specific logic at the end just needs to specify the appropriate
> minItems/maxItems.
>
Agreed, I will do that.

> > -
> > -  interrupt-names:
> > -    oneOf:
> > -      - items:
> > -          - const: eri
> > -          - const: rxi
> > -          - const: txi
> > -          - const: bri
> > -      - items:
> > -          - const: eri
> > -          - const: rxi
> > -          - const: txi
> > -          - const: bri
> > -          - const: dri
> > -          - const: tei
>
> Likewise, with "interrupt-names: false" below for the ones that don't
> need it.
>
Agreed.

> > -
> >    clocks:
> >      minItems: 1
> >      maxItems: 4
> > @@ -173,6 +141,91 @@ allOf:
> >        required:
> >          - resets
> >
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          contains:
> > +            enum:
> > +              - renesas,scif-r8a7742
> > +              - renesas,scif-r8a7743
> > +              - renesas,scif-r8a7744
> > +              - renesas,scif-r8a7745
> > +              - renesas,scif-r8a77470
> > +              - renesas,scif-r8a774a1
> > +              - renesas,scif-r8a774b1
> > +              - renesas,scif-r8a774c0
> > +              - renesas,scif-r8a774e1
> > +              - renesas,scif-r8a7778
> > +              - renesas,scif-r8a7779
> > +              - renesas,scif-r8a7790
> > +              - renesas,scif-r8a7791
> > +              - renesas,scif-r8a7792
> > +              - renesas,scif-r8a7793
> > +              - renesas,scif-r8a7794
> > +              - renesas,scif-r8a7795
> > +              - renesas,scif-r8a7796
> > +              - renesas,scif-r8a77961
> > +              - renesas,scif-r8a77965
> > +              - renesas,scif-r8a77970
> > +              - renesas,scif-r8a77980
> > +              - renesas,scif-r8a77990
> > +              - renesas,scif-r8a77995
> > +              - renesas,scif-r8a779a0
> > +              - renesas,scif-r8a779f0
> > +              - renesas,scif-r8a779g0
>
> Please simplify using family-specific names:
>   - renesas,rcar-gen1-scif
>   - renesas,rcar-gen2-scif
>   - renesas,rcar-gen3-scif
>   - renesas,rcar-gen4-scif
>
Yep, that makes sense.

Cheers,
Prabhakar