diff mbox series

[v6,04/17] dt-bindings: spi: dw: Add AMD Pensando Elba SoC SPI Controller bindings

Message ID 20220820195750.70861-5-brad@pensando.io
State New
Headers show
Series Support AMD Pensando Elba SoC | expand

Commit Message

Brad Larson Aug. 20, 2022, 7:57 p.m. UTC 
From: Brad Larson <blarson@amd.com>

The AMD Pensando Elba SoC has integrated the DW APB SPI Controller

Signed-off-by: Brad Larson <blarson@amd.com>
---
 .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
 1 file changed, 11 insertions(+)

Comments

Serge Semin Aug. 21, 2022, 5:49 p.m. UTC | #1 
On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index 37c3c272407d..403d6416f7ac 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,15 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amd,pensando-elba-spi
> +    then:
> +      required:
> +        - amd,pensando-elba-syscon

Please add the "amd,pensando-elba-syscon" property definition as I
asked here:
https://lore.kernel.org/lkml/20220704131810.kabkuy6e4qmhfm3n@mobilestation/

-Sergey

>  
>  properties:
>    compatible:
> @@ -75,6 +84,8 @@ properties:
>                - renesas,r9a06g032-spi # RZ/N1D
>                - renesas,r9a06g033-spi # RZ/N1S
>            - const: renesas,rzn1-spi   # RZ/N1
> +      - description: AMD Pensando Elba SoC SPI Controller
> +        const: amd,pensando-elba-spi
>  
>    reg:
>      minItems: 1
> -- 
> 2.17.1
>
Krzysztof Kozlowski Aug. 22, 2022, 6:19 p.m. UTC | #2 
On 20/08/2022 22:57, Brad Larson wrote:
> From: Brad Larson <blarson@amd.com>
> 
> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
> 
> Signed-off-by: Brad Larson <blarson@amd.com>
> ---
>  .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> index 37c3c272407d..403d6416f7ac 100644
> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
> @@ -37,6 +37,15 @@ allOf:
>      else:
>        required:
>          - interrupts
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - amd,pensando-elba-spi
> +    then:
> +      required:
> +        - amd,pensando-elba-syscon

There is no such property. You cannot make it required without first
defining it.

>  
>  properties:
>    compatible:
> @@ -75,6 +84,8 @@ properties:
>                - renesas,r9a06g032-spi # RZ/N1D
>                - renesas,r9a06g033-spi # RZ/N1S
>            - const: renesas,rzn1-spi   # RZ/N1
> +      - description: AMD Pensando Elba SoC SPI Controller
> +        const: amd,pensando-elba-spi

Don't add stuff at the end, but in some logical (usually alphabetical)
place. The order is already broken as everyone likes to add stuff in
conflict-style, so just add it before baikal, for example.


Best regards,
Krzysztof
Larson, Bradley Aug. 31, 2022, 6:28 p.m. UTC | #3 
On 8/21/22 10:49 AM, Serge Semin wrote:
> On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
>> From: Brad Larson <blarson@amd.com>
>>
>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>>   .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> index 37c3c272407d..403d6416f7ac 100644
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -37,6 +37,15 @@ allOf:
>>       else:
>>         required:
>>           - interrupts
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - amd,pensando-elba-spi
>> +    then:
>> +      required:
>> +        - amd,pensando-elba-syscon
> Please add the "amd,pensando-elba-syscon" property definition as I
> asked here:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7Cbradley.larson%40amd.com%7C1c4f822c81424048873508da839d90fc%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637967010019245894%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=xl9OU9P9QK3wLHc25hQZK393ylULd41qc4HB2Zt%2F0BQ%3D&amp;reserved=0

Proposing this addition:

--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -148,6 +148,15 @@ 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"
+    maxItems: 1
+    description:
+      A phandle to syscon used to access the spi chip-select override 
register.
+    items:
+      - items:
+        - description: phandle to the syscon node
+
  patternProperties:
    "^.*@[0-9a-f]+$":
      type: object

Regards,
Brad
Larson, Bradley Aug. 31, 2022, 6:45 p.m. UTC | #4 
On 8/22/22 11:19 AM, Krzysztof Kozlowski wrote:
> On 20/08/2022 22:57, Brad Larson wrote:
>> From: Brad Larson <blarson@amd.com>
>>
>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>
>> Signed-off-by: Brad Larson <blarson@amd.com>
>> ---
>>   .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>>   1 file changed, 11 insertions(+)
>>
>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> index 37c3c272407d..403d6416f7ac 100644
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -37,6 +37,15 @@ allOf:
>>       else:
>>         required:
>>           - interrupts
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - amd,pensando-elba-spi
>> +    then:
>> +      required:
>> +        - amd,pensando-elba-syscon
> There is no such property. You cannot make it required without first
> defining it.

Added the definition of 'amd,pensando-elba-syscon' to snps,dw-apb-ssi.yaml

>>   properties:
>>     compatible:
>> @@ -75,6 +84,8 @@ properties:
>>                 - renesas,r9a06g032-spi # RZ/N1D
>>                 - renesas,r9a06g033-spi # RZ/N1S
>>             - const: renesas,rzn1-spi   # RZ/N1
>> +      - description: AMD Pensando Elba SoC SPI Controller
>> +        const: amd,pensando-elba-spi
> Don't add stuff at the end, but in some logical (usually alphabetical)
> place. The order is already broken as everyone likes to add stuff in
> conflict-style, so just add it before baikal, for example.

Yes, tried to follow existing style.  Will add it before baikal.

Regards,
Brad
Larson, Bradley Sept. 14, 2022, 6:47 p.m. UTC | #5 
On 9/11/22 11:34 AM, Serge Semin wrote:
> On Wed, Aug 31, 2022 at 06:28:46PM +0000, Larson, Bradley wrote:
>> On 8/21/22 10:49 AM, Serge Semin wrote:
>>> On Sat, Aug 20, 2022 at 12:57:37PM -0700, Brad Larson wrote:
>>>> From: Brad Larson <blarson@amd.com>
>>>>
>>>> The AMD Pensando Elba SoC has integrated the DW APB SPI Controller
>>>>
>>>> Signed-off-by: Brad Larson <blarson@amd.com>
>>>> ---
>>>>    .../devicetree/bindings/spi/snps,dw-apb-ssi.yaml      | 11 +++++++++++
>>>>    1 file changed, 11 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> index 37c3c272407d..403d6416f7ac 100644
>>>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>>>> @@ -37,6 +37,15 @@ allOf:
>>>>        else:
>>>>          required:
>>>>            - interrupts
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - amd,pensando-elba-spi
>>>> +    then:
>>>> +      required:
>>>> +        - amd,pensando-elba-syscon
>>> Please add the "amd,pensando-elba-syscon" property definition as I
>>> asked here:
>>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7CBradley.Larson%40amd.com%7C5f61c8f8130c47fa814208da94243e17%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637985180686357705%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uzcSBjuxs1JZFXiRGFVLGojAsipJACXEGskYdGmr7qA%3D&amp;reserved=0
>> Proposing this addition:
>>
>> --- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> +++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
>> @@ -148,6 +148,15 @@ 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"
>> +    maxItems: 1
>> +    description:
>> +      A phandle to syscon used to access the spi chip-select override
>> register.
>> +    items:
>> +      - items:
>> +        - description: phandle to the syscon node
>> +
> No. What Krzysztof and I asked was to add the property definition
> into the allOf: [ if ...,  ] statement. Please read more carefully my
> last comment:
> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Flkml%2F20220704131810.kabkuy6e4qmhfm3n%40mobilestation%2F&amp;data=05%7C01%7CBradley.Larson%40amd.com%7C5f61c8f8130c47fa814208da94243e17%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637985180686357705%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000%7C%7C%7C&amp;sdata=uzcSBjuxs1JZFXiRGFVLGojAsipJACXEGskYdGmr7qA%3D&amp;reserved=0
> The definition is supposed to look like this:
>
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            const: amd,pensando-elba-spi
>> +    then:
>    +      properties:
>    +        amd,pensando-elba-syscon
>    +          $ref: /schemas/types.yaml#/definitions/phandle
>    +          description: AMD Pensando Elba SoC system controller
>> +      required:
>> +        - amd,pensando-elba-syscon
> * Please also note that I've replaced "enum:" with "const:" in the if
> statement above.
>
> The difference with what you suggested is that my version is
> applicable for the Pensando ELBA SPI controller only, while your
> update will cause applying the "amd,pensando-elba-syscon" property
> constraints for all DW SSI controllers which isn't what we would want.


Yes, I see by moving this property into the allOf its only applicable 
for compatible "amd,pensando-elba-spi". Also changing "enum:" to 
"const:" as shown.  Yes on the DT-bindings check. Rob Herring's bot 
indicated an error but I had none in checking V6 patchset.  I did a 
dtschema update and it went from dtschema 2022.3.2 to dtschema-2022.8.3 
and will see if that is the reason.

Regards,
Brad
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
index 37c3c272407d..403d6416f7ac 100644
--- a/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
+++ b/Documentation/devicetree/bindings/spi/snps,dw-apb-ssi.yaml
@@ -37,6 +37,15 @@  allOf:
     else:
       required:
         - interrupts
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - amd,pensando-elba-spi
+    then:
+      required:
+        - amd,pensando-elba-syscon
 
 properties:
   compatible:
@@ -75,6 +84,8 @@  properties:
               - renesas,r9a06g032-spi # RZ/N1D
               - renesas,r9a06g033-spi # RZ/N1S
           - const: renesas,rzn1-spi   # RZ/N1
+      - description: AMD Pensando Elba SoC SPI Controller
+        const: amd,pensando-elba-spi
 
   reg:
     minItems: 1