Re: Aw: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches

From: Krzysztof Kozlowski
Date: Tue May 03 2022 - 10:40:37 EST

On 03/05/2022 16:10, Frank Wunderlich wrote:
> Hi,
>
> thank you for first review.
>
>> Gesendet: Dienstag, 03. Mai 2022 um 14:05 Uhr
>> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski@xxxxxxxxxx>
>> Betreff: Re: [RFC v1] dt-bindings: net: dsa: convert binding for mediatek switches
>>
>> On 02/05/2022 17:32, Frank Wunderlich wrote:
>>> From: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
>>>
>>> Convert txt binding to yaml binding for Mediatek switches.
>>>
>>> Signed-off-by: Frank Wunderlich <frank-w@xxxxxxxxxxxxxxx>
>>> ---
>>> .../devicetree/bindings/net/dsa/mediatek.yaml | 435 ++++++++++++++++++
>>> .../devicetree/bindings/net/dsa/mt7530.txt | 327 -------------
>>> 2 files changed, 435 insertions(+), 327 deletions(-)
>>> create mode 100644 Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>> delete mode 100644 Documentation/devicetree/bindings/net/dsa/mt7530.txt
>>>
>>> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>> new file mode 100644
>>> index 000000000000..c1724809d34e
>>> --- /dev/null
>>> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek.yaml
>>
>> Specific name please, so previous (with vendor prefix) was better:
>> mediatek,mt7530.yaml
>
> ok, named it mediatek only because mt7530 is only one possible chip and driver handles 3 different "variants".
>
>>> @@ -0,0 +1,435 @@
>>> +# SPDX-License-Identifier: (GPL-2.0-only OR BSD-2-Clause)
>>
>> You should CC previous contributors and get their acks on this. You
>> copied here a lot of description.
>
> added 3 Persons that made commits to txt before to let them know about this change
>
> and yes, i tried to define at least the phy-mode requirement as yaml-depency, but failed because i cannot match
> compatible in subnode.

I don't remember such syntax.

(...)

>
>>> if defined, indicates that either MT7530 is the part
>>> + on multi-chip module belong to MT7623A has or the remotely standalone
>>> + chip as the function MT7623N reference board provided for.
>>> +
>>> + reset-gpios:
>>> + description: |
>>> + Should be a gpio specifier for a reset line.
>>> + maxItems: 1
>>> +
>>> + reset-names:
>>> + description: |
>>> + Should be set to "mcm".
>>> + const: mcm
>>> +
>>> + resets:
>>> + description: |
>>> + Phandle pointing to the system reset controller with
>>> + line index for the ethsys.
>>> + maxItems: 1
>>> +
>>> +required:
>>> + - compatible
>>> + - reg
>>
>> What about address/size cells?
>
> you're right even if they are const to a value they need to be set
>
>>> +
>>> +allOf:
>>> + - $ref: "dsa.yaml#"
>>> + - if:
>>> + required:
>>> + - mediatek,mcm
>>
>> Original bindings had this reversed.
>
> i know, but i think it is better readable and i will drop the else-part later.
> Driver supports optional reset ("mediatek,mcm" unset and without reset-gpios)
> as this is needed if there is a shared reset-line for gmac and switch like on R2 Pro.
>
> i left this as separate commit to be posted later to have a nearly 1:1 conversion here.

Ah, I missed that actually your syntax is better. No need to
reverse/negate and the changes do not have to be strict 1:1.

>
>>> + then:
>>> + required:
>>> + - resets
>>> + - reset-names
>>> + else:
>>> + required:
>>> + - reset-gpios
>>> +
>>> + - if:
>>> + required:
>>> + - interrupt-controller
>>> + then:
>>> + required:
>>> + - "#interrupt-cells"
>>
>> This should come from dt schema already...
>
> so i should drop (complete block for interrupt controller)?

The interrupts you need. What I mean, you can skip requirement of cells.

>
>>> + - interrupts
>>> +
>>> + - if:
>>> + properties:
>>> + compatible:
>>> + items:
>>> + - const: mediatek,mt7530
>>> + then:
>>> + required:
>>> + - core-supply
>>> + - io-supply
>>> +
>>> +
>>> +patternProperties:
>>> + "^ports$":
>>
>> It''s not a pattern, so put it under properties, like regular property.
>
> can i then make the subnodes match? so the full block will move above required between "mediatek,mcm" and "reset-gpios"

Yes, subnodes stay with patternProperties.

>
> ports:
> type: object
>
> patternProperties:
> "^port@[0-9]+$":
> type: object
> description: Ethernet switch ports
>
> properties:
> reg:
> description: |
> Port address described must be 5 or 6 for CPU port and from 0 to 5 for user ports.
>
> unevaluatedProperties: false
>
> allOf:
> - $ref: dsa-port.yaml#
> - if:
> ....
>
> basicly this "ports"-property should be required too, right?

Previous binding did not enforce it, I think, but it is reasonable to
require ports.

>
>
>>> + type: object
>>> +
>>> + patternProperties:
>>> + "^port@[0-9]+$":
>>> + type: object
>>> + description: Ethernet switch ports
>>> +
>>> + $ref: dsa-port.yaml#
>>
>> This should go to allOf below.
>
> see above
>
>>> +
>>> + properties:
>>> + reg:
>>> + description: |
>>> + Port address described must be 6 for CPU port and from 0 to 5 for user ports.
>>> +
>>> + unevaluatedProperties: false
>>> +
>>> + allOf:
>>> + - if:
>>> + properties:
>>> + label:
>>> + items:
>>> + - const: cpu
>>> + then:
>>> + required:
>>> + - reg
>>> + - phy-mode
>>> +
>>> +unevaluatedProperties: false
>>> +
>>> +examples:
>>> + - |
>>> + mdio0 {
>>
>> Just mdio
>
> ok
>
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + switch@0 {
>>> + compatible = "mediatek,mt7530";
>>> + #address-cells = <1>;
>>> + #size-cells = <0>;
>>> + reg = <0>;
>>> +
>>> + core-supply = <&mt6323_vpa_reg>;
>>> + io-supply = <&mt6323_vemc3v3_reg>;
>>> + reset-gpios = <&pio 33 0>;
>>
>> Use GPIO flag define/constant.
>
> this example seems to be taken from bpi-r2 (i had taken it from the txt). In dts for this board there are no
> constants too.
>
> i guess
> include/dt-bindings/gpio/gpio.h:14:#define GPIO_ACTIVE_HIGH 0
>
> for 33 there seem no constant..all other references to pio node are with numbers too and there seem no binding
> header defining the gpio pins (only functions in include/dt-bindings/pinctrl/mt7623-pinfunc.h)

ok, then my comment


Best regards,
Krzysztof