Re: [PATCH V2 1/2] dt-bindings: PCI: tegra234: Add ECAM support

[Krzysztof Kozlowski <krzysztof.kozlowski@linaro.org>]

On 14/11/2022 15:09, Jon Hunter wrote:
> From: Vidya Sagar <vidyas@nvidia.com>
> 
> Add support for ECAM aperture that is only supported for Tegra234
> devices.
> 
> Co-developed-by: Vidya Sagar <vidyas@nvidia.com>
> Signed-off-by: Vidya Sagar <vidyas@nvidia.com>
> Co-developed-by: Jon Hunter <jonathanh@nvidia.com>
> Signed-off-by: Jon Hunter <jonathanh@nvidia.com>
> ---
> Changes since V1:
> - Restricted the ECAM aperture to only Tegra234 devices that support it.
> 
>  .../bindings/pci/nvidia,tegra194-pcie.yaml    | 76 +++++++++++++++----
>  .../devicetree/bindings/pci/snps,dw-pcie.yaml |  2 +-
>  2 files changed, 62 insertions(+), 16 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
> index 75da3e8eecb9..7ae0f37f5364 100644
> --- a/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
> +++ b/Documentation/devicetree/bindings/pci/nvidia,tegra194-pcie.yaml
> @@ -27,21 +27,12 @@ properties:
>        - nvidia,tegra234-pcie
>  
>    reg:
> -    items:
> -      - description: controller's application logic registers
> -      - description: configuration registers
> -      - description: iATU and DMA registers. This is where the iATU (internal
> -          Address Translation Unit) registers of the PCIe core are made
> -          available for software access.
> -      - description: aperture where the Root Port's own configuration
> -          registers are available.
> +    minItems: 4
> +    maxItems: 5
>  
>    reg-names:
> -    items:
> -      - const: appl
> -      - const: config
> -      - const: atu_dma
> -      - const: dbi
> +    minItems: 4
> +    maxItems: 5
>  
>    interrupts:
>      items:
> @@ -202,6 +193,60 @@ properties:
>  
>  allOf:
>    - $ref: /schemas/pci/snps,dw-pcie.yaml#
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra194-pcie
> +    then:
> +      properties:
> +        reg:
> +          minItems: 4
> +          maxItems: 4

How you wrote it, you do not need min/maxItems here, because you have
items below. However see further comment.

> +          items:
> +            - description: controller's application logic registers
> +            - description: configuration registers
> +            - description: iATU and DMA registers. This is where the iATU (internal
> +                Address Translation Unit) registers of the PCIe core are made
> +                available for software access.
> +            - description: aperture where the Root Port's own configuration
> +                registers are available.
> +        reg-names:
> +          items:
> +            - const: appl
> +            - const: config
> +            - const: atu_dma
> +            - const: dbi
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - nvidia,tegra234-pcie
> +    then:
> +      properties:
> +        reg:
> +          minItems: 5
> +          maxItems: 5

Similar issue.

> +          items:
> +            - description: controller's application logic registers
> +            - description: configuration registers
> +            - description: iATU and DMA registers. This is where the iATU (internal
> +                Address Translation Unit) registers of the PCIe core are made
> +                available for software access.
> +            - description: aperture where the Root Port's own configuration
> +                registers are available.
> +            - description: aperture to access the configuration space through ECAM.

This is unnecessarily duplicated. You can keep the descriptions of items
and reg-names items in top level (with min 4 and max 5) and restrict
maxItems for 194 and minItems for 234 here.


> +        reg-names:
> +          items:
> +            - const: appl
> +            - const: config
> +            - const: atu_dma
> +            - const: dbi
> +            - const: ecam
> +

No need for blank line.
>  
>  unevaluatedProperties: false
>  

Best regards,
Krzysztof