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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <b88512ff-d062-276c-981d-98ec65cb8527@linaro.org>
Date:   Tue, 30 Aug 2022 22:19:09 +0300
From:   Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>
To:     Maxime Chevallier <maxime.chevallier@...tlin.com>
Cc:     davem@...emloft.net, Rob Herring <robh+dt@...nel.org>,
        netdev@...r.kernel.org, linux-kernel@...r.kernel.org,
        thomas.petazzoni@...tlin.com, Andrew Lunn <andrew@...n.ch>,
        Jakub Kicinski <kuba@...nel.org>,
        Eric Dumazet <edumazet@...gle.com>,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        Russell King <linux@...linux.org.uk>,
        linux-arm-kernel@...ts.infradead.org, devicetree@...r.kernel.org
Subject: Re: [PATCH net-next v2 1/5] dt-bindings: net: Convert Altera TSE
 bindings to yaml

On 30/08/2022 22:16, Maxime Chevallier wrote:
>>> Signed-off-by: Maxime Chevallier <maxime.chevallier@...tlin.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@...tlin.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