Re: [PATCH v3 01/12] dt-bindings: net: add STM32MP13 compatible in documentation for stm32

[Krzysztof Kozlowski]

On 28/09/2023 22:39, Conor Dooley wrote:
> Hey,
> 
> On Thu, Sep 28, 2023 at 05:15:01PM +0200, Christophe Roullier wrote:
>> New STM32 SOC have 2 GMACs instances.
>> GMAC IP version is SNPS 4.20.
>>
>> Signed-off-by: Christophe Roullier <christophe.roullier@xxxxxxxxxxx>
>> ---
>>  .../devicetree/bindings/net/stm32-dwmac.yaml  | 78 +++++++++++++++++--
>>  1 file changed, 70 insertions(+), 8 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> index fc8c96b08d7dc..ca976281bfc22 100644
>> --- a/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> +++ b/Documentation/devicetree/bindings/net/stm32-dwmac.yaml
>> @@ -22,18 +22,17 @@ select:
>>          enum:
>>            - st,stm32-dwmac
>>            - st,stm32mp1-dwmac
>> +          - st,stm32mp13-dwmac
>>    required:
>>      - compatible
>>  
>> -allOf:
>> -  - $ref: snps,dwmac.yaml#
>> -
>>  properties:
>>    compatible:
>>      oneOf:
>>        - items:
>>            - enum:
>>                - st,stm32mp1-dwmac
>> +              - st,stm32mp13-dwmac
>>            - const: snps,dwmac-4.20a
>>        - items:
>>            - enum:
>> @@ -74,13 +73,10 @@ properties:
>>  
>>    st,syscon:
>>      $ref: /schemas/types.yaml#/definitions/phandle-array
>> -    items:
>> -      - items:
>> -          - description: phandle to the syscon node which encompases the glue register
>> -          - description: offset of the control register
>>      description:
>>        Should be phandle/offset pair. The phandle to the syscon node which
>> -      encompases the glue register, and the offset of the control register
>> +      encompases the glue register, the offset of the control register and
>> +      the mask to set bitfield in control register
>>  
>>    st,eth-clk-sel:
>>      description:
>> @@ -101,6 +97,38 @@ required:
>>  
>>  unevaluatedProperties: false
>>  
>> +allOf:
>> +  - $ref: snps,dwmac.yaml#
>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - st,stm32mp1-dwmac
>> +              - st,stm32-dwmac
>> +    then:
>> +      properties:
>> +        st,syscon:
>> +          items:
>> +            items:
>> +              - description: phandle to the syscon node which encompases the glue register
>> +              - description: offset of the control register
> 
> These descriptions should, IMO, be moved back out to the st,syscon
> definition. If you put the 3 descriptions there, with "minItems: 2" &
> put "maxItems: 2" and "minItems: 3" in each of the if/then clauses.
> Also, it should be sufficient to simplify to if/then/else.

What's more, same property should not have different types. This syscon
property was way too generic, thus now you have trouble of conflicting
types.

Best regards,
Krzysztof