Re: [PATCHv3 2/4] dt-bindings: edac: altera: agilex5: document new edac support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issues to consider:
- [Medium] dt-bindings: edac: altera: fixed-order interrupt-names tuple
  with minItems 1
- [Medium] dt-bindings: edac: altera: incompatible a10 fallback
  compatible for agilex5
- [Low] dt-bindings: edac: altera: unwieldy per-compatible conditionals
  in schema
--

commit 8ef209bb93609f50bb0952f3479bdf8adcfdc2d5
Author: Dinh Nguyen <dinguyen@kernel.org>

dt-bindings: edac: altera: agilex5: document new edac support

This commit documents the Agilex5 ECC manager compatible string and its
seven named interrupts in the Altera SoCFPGA ECC manager DT binding.

> diff --git a/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml b/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> index 136e8fccd429a..14e09fc86055f 100644
> --- a/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> +++ b/Documentation/devicetree/bindings/edac/altr,socfpga-ecc-manager.yaml
> @@ -20,7 +20,9 @@ properties:
>    compatible:
>      oneOf:
>        - items:
> -          - const: altr,socfpga-s10-ecc-manager
> +          - enum:
> +              - altr,socfpga-s10-ecc-manager
> +              - altr,socfpga-agilex5-ecc-manager
>            - const: altr,socfpga-a10-ecc-manager
>        - const: altr,socfpga-a10-ecc-manager
>        - const: altr,socfpga-ecc-manager

[Severity: Medium]
Is altr,socfpga-a10-ecc-manager a valid fallback compatible for
altr,socfpga-agilex5-ecc-manager?

On a kernel without dedicated Agilex5 support, matching
altr,socfpga-a10-ecc-manager binds altr_edac_a10_driver with is_s10 set
to false.  altr_edac_a10_probe() and altr_edac_a10_device_add() then
treat the device as a 32-bit Arria10 ECC manager, writing Arria10 System
Manager register offsets (A10_SYSMGR_ECC_INTMASK_SET_OFST), registering
platform_get_irq(pdev, 1) as a chained IRQ handler, and requiring two
interrupts on every child peripheral ECC node.

> @@ -33,7 +35,25 @@ properties:
>  
>    interrupts:
>      minItems: 1
> -    maxItems: 2
> +    items:
> +      - description: Global single-bit error
> +      - description: Global double-bit error
> +      - description: IO96B0 DRAM controller error
> +      - description: IO96B1 DRAM controller error
> +      - description: SDM QSPI single-bit error
> +      - description: SDM QSPI double-bit error
> +      - description: SDM single-event
> +
> +  interrupt-names:
> +    minItems: 1
> +    items:
> +      - const: global_sbe
> +      - const: global_dbe
> +      - const: io96b0
> +      - const: io96b1
> +      - const: sdm_qspi_sbe
> +      - const: sdm_qspi_dbe
> +      - const: sdm_seu

[Severity: Medium]
Does defining interrupt-names as a fixed-order tuple of const values
with minItems: 1 match how Agilex5 nodes specify interrupts?

In JSON Schema tuple validation, an array of length N (with N < 7) must
match the first N entries in exact order, so dtbs_check will reject any
Agilex5 node that omits an earlier interrupt (such as global_dbe or
io96b1) while specifying a later one (such as io96b0 or sdm_seu).  Or, if
all seven GIC interrupts are always wired in the Agilex5 SoC device tree,
should minItems be 7?

[ ... ]

> @@ -235,28 +255,50 @@ patternProperties:
>        - interrupts
>        - altr,ecc-parent
>  
> -if:
> -  properties:
> -    compatible:
> -      contains:
> -        const: altr,socfpga-ecc-manager
> -then:
> -  required:
> -    - compatible
> -    - "#address-cells"
> -    - "#size-cells"
> -    - ranges
> -
> -else:
> -  required:
> -    - compatible
> -    - "#address-cells"
> -    - "#size-cells"
> -    - interrupts
> -    - interrupt-controller
> -    - "#interrupt-cells"
> -    - ranges
> -    - altr,sysmgr-syscon
> +allOf:
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: altr,socfpga-ecc-manager
> +    then:
> +      required:
> +        - compatible
> +        - "#address-cells"
> +        - "#size-cells"
> +        - ranges
> +
> +    else:
> +      required:
> +        - compatible
> +        - "#address-cells"
> +        - "#size-cells"
> +        - interrupts
> +        - interrupt-controller
> +        - "#interrupt-cells"
> +        - ranges
> +        - altr,sysmgr-syscon
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: altr,socfpga-agilex5-ecc-manager
> +    then:
> +      properties:
> +        interrupts:
> +          minItems: 1
> +          maxItems: 7
> +        interrupt-names:
> +          minItems: 1
> +          maxItems: 7
> +      required:
> +        - interrupt-names
> +    else:
> +      properties:
> +        interrupts:
> +          maxItems: 2
> +        interrupt-names: false

[Severity: Low]
This isn't a bug, but this binding already spans several SoCFPGA
generations with different property requirements, and defining seven
Agilex5-specific interrupts at the top level only to restrict older
variants back in a second if/then/else block makes the schema hard to
follow (with minItems: 1 and maxItems: 7 in the Agilex5 branch also
duplicating the top-level 7-item lists).  Would it be cleaner to split
Agilex5 into a separate schema document instead of adding more
per-compatible conditionals here?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112215342.2476801-2-dinguyen@kernel.org?part=1