Re: [PATCH 1/6] dt-bindings: edac: altera: Document additional ECC instances

[Niravkumar L Rabara]

On 29/10/2025 2:50 pm, Krzysztof Kozlowski wrote:
> On Tue, Oct 28, 2025 at 05:22:27PM +0800,niravkumarlaxmidas.rabara@xxxxxxxxxx wrote:
> > From: Niravkumar L Rabara<niravkumarlaxmidas.rabara@xxxxxxxxxx>
> >
> > Add support for Secure Device Manager(SDM) QSPI ECC, IO96B memory
> > controller ECC and Configuration RAM(CRAM) Single Event Upset(SEU).
> >
> > Add interrupt-names property and increase interrupts maxItems from 2 to 7
> > to accommodate additional interrupts.
> >
> > Signed-off-by: Niravkumar L Rabara<niravkumarlaxmidas.rabara@xxxxxxxxxx>
> > ---
> >   .../edac/altr,socfpga-ecc-manager.yaml        | 77 ++++++++++++++++++-
> >   1 file changed, 76 insertions(+), 1 deletion(-)
> >
> > diff --git a/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml b/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> > index 3d787dea0f14..5e0c08a15ab9 100644
> > --- a/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> > +++ b/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> > @@ -33,7 +33,13 @@ properties:
> >   
> >     interrupts:
> >       minItems: 1
> > -    maxItems: 2
> > +    maxItems: 7
> No, list the interrupts instead. Your commit msg must clearly explain
> why exception of not-fixed length/entries is justified.
>
> See writing bindings.
>
> > +
> > +  interrupt-names:
> > +    items:
> > +      enum: [global_sbe, global_dbe, io96b0, io96b1, sdm_qspi_sbe, sdm_qspi_dbe, sdm_seu]
> Nope, list the items instead. Please do not come up with some custom
> syntax.
>
> > +    minItems: 1
> > +    maxItems: 7
> >   
> >     interrupt-controller: true
> >   
> > @@ -70,6 +76,41 @@ properties:
> >         - interrupts
> >         - altr,sdr-syscon
> >   
> > +  cram-seu:
> Missing description, so difficult to say what is here.
> Node names should be generic. See also an explanation and list of
> examples (not exhaustive) in DT specification:
> https://devicetree-specification.readthedocs.io/en/latest/chapter2- 
> devicetree-basics.html#generic-names-recommendation
> If you cannot find a name matching your device, please check in kernel
> sources for similar cases or you can grow the spec (via pull request to
> DT spec repo).
>
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      compatible:
> > +        items:
> > +          - const: altr,socfpga-cram-seu
> Why do you need compatible?
>
> > +
> > +      reg:
> > +        maxItems: 1
> So you created child node only for reg? No, fold it into parent.
>
> You also forgot to update the example.
>
> > +
> > +      altr,seu-safe-inject-ce-msb:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: MSB of error injection command for Correctable Error
> > +
> > +      altr,seu-safe-inject-ce-lsb:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: LSB of error injection command for Correctable Error
> > +
> > +      altr,seu-safe-inject-ue-msb:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: MSB of error injection command for Uncorrectable Error
> > +
> > +      altr,seu-safe-inject-ue-lsb:
> > +        $ref: /schemas/types.yaml#/definitions/uint32
> > +        description: LSB of error injection command for Uncorrectable Error
> How are these board-level properties?
>
> > +
> > +    required:
> > +      - compatible
> > +      - altr,seu-safe-inject-ce-msb
> > +      - altr,seu-safe-inject-ce-lsb
> > +      - altr,seu-safe-inject-ue-msb
> > +      - altr,seu-safe-inject-ue-lsb
> > +
> >   patternProperties:
> >     "^ocram-ecc@[a-f0-9]+$":
> >       type: object
> > @@ -191,6 +232,40 @@ patternProperties:
> >         - interrupts
> >         - altr,ecc-parent
> >   
> > +  "^sdm-qspi-ecc@[a-f0-9]+$":
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      compatible:
> > +        items:
> > +          - const: altr,socfpga-sdm-qspi-ecc
> No, drop.
>
> > +
> > +      reg:
> > +        maxItems: 1
> > +
> > +    required:
> > +      - compatible
> > +      - reg
> No point for empty children. One reg is not justification for having a
> child.
>
> > +
> > +  "^io96b[0-9]-ecc@[a-f0-9]+$":
> You need to stop coming with random node names. Nothing explains why you
> need children, why these are not part of parent node.
>
> > +    type: object
> > +    additionalProperties: false
> > +
> > +    properties:
> > +      compatible:
> > +        items:
> > +          - enum:
> > +              - altr,socfpga-io96b0-ecc
> > +              - altr,socfpga-io96b1-ecc
> Plus all your compatibles have WRONG format. See writing bindings and
> numerouse presentations - you always must use SoC specific compatible.
>
> Best regards,
> Krzysztof


Hi Krzysztof,

Thanks for the review and feedback on the patch series.
I’ll go through your comments and address them in the next patch revision.

Thanks,
Nirav