Re: [PATCH v11 04/15] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller

[Serge Semin <fancer.lancer@xxxxxxxxx>]

On Sat, Mar 11, 2023 at 04:44:34PM -0800, Brad Larson wrote:
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@xxxxxxx>
> Reviewed-by: Krzysztof Kozlowski <krzysztof.kozlowski@xxxxxxxxxx>
> ---
> 
> v10 changes:
> - Move definition of amd,pensando-elba-syscon into properties
>   with a better description
> - Add amd,pensando-elba-syscon: false for non elba designs
> 
> v9 changes:
> - Define property amd,pensando-elba-syscon
> - Move compatible amd,pensando-elba-spi ahead of baikal,bt1-ssi
> 
> ---
>  .../bindings/spi/snps,dw-apb-ssi.yaml         | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index a132b5fc56e0..2383d6497b1e 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,17 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            const: amd,pensando-elba-spi
> +    then:
> +      required:
> +        - amd,pensando-elba-syscon
> +    else:
> +      properties:
> +        amd,pensando-elba-syscon: false
>  
>  properties:
>    compatible:
> @@ -63,6 +74,8 @@ properties:
>          const: intel,keembay-ssi
>        - description: Intel Thunder Bay SPI Controller
>          const: intel,thunderbay-ssi
> +      - description: AMD Pensando Elba SoC SPI Controller
> +        const: amd,pensando-elba-spi
>        - description: Baikal-T1 SPI Controller
>          const: baikal,bt1-ssi
>        - description: Baikal-T1 System Boot SPI Controller
> @@ -136,6 +149,12 @@ properties:
>        of the designware controller, and the upper limit is also subject to
>        controller configuration.
>  
> +  amd,pensando-elba-syscon:
> +    $ref: /schemas/types.yaml#/definitions/phandle-array

> +    description: |
                    ^
     +--------------+
This + modifier is redundant.

> +      Block address to control SPI chip-selects.  The Elba SoC
> +      does not use ssi.                          ^
                                                    |
1. Drop one of the whitespaces ---------------------+
2. The description is misleading. SSI means "Synchronous Serial
Interface" which basically means SPI. If you meant SS (slave-select)
signals then Elba SoC do use them. What would sound correctly here is
that Elba SoC system controller provides an interface to override the
native DWC SSI CS control.

Please fix the notes above. Then feel free to add
Reviewed-by: Serge Semin <fancer.lancer@xxxxxxxxx>

-Serge(y)

> +
>  patternProperties:
>    "^.*@[0-9a-f]+$":
>      type: object
> -- 
> 2.17.1
>