Re: [PATCH net-next v2 1/5] dt-bindings: net: Convert Altera TSE bindings to yaml

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

On 30/08/2022 22:16, Maxime Chevallier wrote:
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@bootlin.com>  
>>
>> Rebase your changes on some decent kernel and use
>> get_maintainers.pl...
> 
> I'm rebased against net-next, so I don't understand how I'm supposed to
> do for this series, should I sent binding patches separately and based
> on another tree ?
> 
> I'll cc you next time, sorry about that.

net-next is correct, I just assumed it is some older tree since you did
not Cc me.

> 
>>> ---
>>> V1->V2:
>>>  - Removed unnedded maxItems
>>>  - Added missing minItems
>>>  - Fixed typos in some properties names
>>>  - Fixed the mdio subnode definition
>>>
>>>  .../devicetree/bindings/net/altera_tse.txt    | 113 -------------
>>>  .../devicetree/bindings/net/altr,tse.yaml     | 156
>>> ++++++++++++++++++ 2 files changed, 156 insertions(+), 113
>>> deletions(-) delete mode 100644
>>> Documentation/devicetree/bindings/net/altera_tse.txt create mode
>>> 100644 Documentation/devicetree/bindings/net/altr,tse.yaml 
>>
>> (...)
>>
>>> diff --git a/Documentation/devicetree/bindings/net/altr,tse.yaml
>>> b/Documentation/devicetree/bindings/net/altr,tse.yaml new file mode
>>> 100644 index 000000000000..1676e13b8c64
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/altr,tse.yaml
>>> @@ -0,0 +1,156 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>> +%YAML 1.2
>>> +---
>>> +$id: http://devicetree.org/schemas/net/altr,tse.yaml#
>>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>>> +
>>> +title: Altera Triple Speed Ethernet MAC driver (TSE)
>>> +
>>> +maintainers:
>>> +  - Maxime Chevallier <maxime.chevallier@bootlin.com>
>>> +
>>> +allOf:  
>>
>> Put allOf below "required".
> 
> Ack
> 
>>> +  - $ref: "ethernet-controller.yaml#"
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - altr,tse-1.0
>>> +              - ALTR,tse-1.0
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 4
>>> +        reg-names:
>>> +          items:
>>> +            - const: control_port
>>> +            - const: rx_csr
>>> +            - const: tx_csr
>>> +            - const: s1
>>> +
>>> +  - if:
>>> +      properties:
>>> +        compatible:
>>> +          contains:
>>> +            enum:
>>> +              - altr,tse-msgdma-1.0
>>> +    then:
>>> +      properties:
>>> +        reg:
>>> +          minItems: 6
>>> +        reg-names:
>>> +          minItems: 6  
>>
>> No need for minItems.
> 
> Ok I'll remove it
> 
>>> +          items:
>>> +            - const: control_port
>>> +            - const: rx_csr
>>> +            - const: rx_desc
>>> +            - const: rx_resp
>>> +            - const: tx_csr
>>> +            - const: tx_desc
>>> +
>>> +properties:
>>> +  compatible:
>>> +    enum:
>>> +      - altr,tse-1.0
>>> +      - ALTR,tse-1.0  
>>
>> This is deprecated compatible. You need oneOf and then deprecated:
>> true.
> 
> Ok thanks for the tip
> 
>>> +      - altr,tse-msgdma-1.0
>>> +
>>> +  reg:
>>> +    minItems: 4
>>> +    maxItems: 6
>>> +
>>> +  reg-names:
>>> +    minItems: 4
>>> +    items:
>>> +      - const: control_port
>>> +      - const: rx_csr
>>> +      - const: rx_desc
>>> +      - const: rx_resp
>>> +      - const: tx_csr
>>> +      - const: tx_desc
>>> +      - const: s1  
>>
>> This is messed up. You allow only 6 items maximum, but list 7. It
>> contradicts your other variants in allOf:if:then.
> 
> I'll remove that part then, apparently it's not needed at all if the
> allOf:if:then cover all cases.

Right. The typical pattern is like clocks/clock-names here:

https://elixir.bootlin.com/linux/v5.19-rc6/source/Documentation/devicetree/bindings/clock/samsung,exynos7-clock.yaml#L57

Best regards,
Krzysztof