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

[Niravkumar L Rabara <niravkumarlaxmidas.rabara@altera.com>]

On 29/10/2025 2:50 pm, Krzysztof Kozlowski wrote:
> On Tue, Oct 28, 2025 at 05:22:27PM +0800,niravkumarlaxmidas.rabara@altera.com wrote:
>> From: Niravkumar L Rabara<niravkumarlaxmidas.rabara@altera.com>
>>
>> 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@altera.com>
>> ---
>>   .../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