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

[Krzysztof Kozlowski]

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