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

[Frank Wunderlich]

Hi

> Gesendet: Donnerstag, 05. Mai 2022 um 22:29 Uhr
> Von: "Krzysztof Kozlowski" <krzysztof.kozlowski at linaro.org>
> An: "Frank Wunderlich" <linux at fw-web.de>, linux-mediatek at lists.infradead.org
> Cc: "Frank Wunderlich" <frank-w at public-files.de>, "Andrew Lunn" <andrew at lunn.ch>, "Vivien Didelot" <vivien.didelot at gmail.com>, "Florian Fainelli" <f.fainelli at gmail.com>, "Vladimir Oltean" <olteanv at gmail.com>, "David S. Miller" <davem at davemloft.net>, "Jakub Kicinski" <kuba at kernel.org>, "Paolo Abeni" <pabeni at redhat.com>, "Rob Herring" <robh+dt at kernel.org>, "Krzysztof Kozlowski" <krzysztof.kozlowski+dt at linaro.org>, "Matthias Brugger" <matthias.bgg at gmail.com>, "Sean Wang" <sean.wang at mediatek.com>, "Landen Chao" <Landen.Chao at mediatek.com>, "DENG Qingfang" <dqfext at gmail.com>, netdev at vger.kernel.org, devicetree at vger.kernel.org, linux-kernel at vger.kernel.org, linux-arm-kernel at lists.infradead.org, "Greg Ungerer" <gerg at kernel.org>, "René van Dorst" <opensource at vdorst.com>, "Mauro Carvalho Chehab" <mchehab+samsung at kernel.org>
> Betreff: Re: [RFC v2] dt-bindings: net: dsa: convert binding for mediatek switches
>
> On 05/05/2022 17:00, Frank Wunderlich wrote:
> > From: Frank Wunderlich <frank-w at public-files.de>
> > 
> > Convert txt binding to yaml binding for Mediatek switches.
> > 
> > Signed-off-by: Frank Wunderlich <frank-w at public-files.de>
> 
> Thank you for your patch. There is something to discuss/improve.
> 
> > +    const: 1
> > +
> > +  "#size-cells":
> > +    const: 0
> > +
> > +  core-supply:
> > +    description: |
> 
> Drop | everywhere where it is not needed (so in all places, AFAICT)

is it necessary for multiline-descriptions or is indentation enough?
 
> > +      Phandle to the regulator node necessary for the core power.
> > +
> > +  "#gpio-cells":
> > +    description: |
> > +      Must be 2 if gpio-controller is defined.
> 
> Skip description, it's obvious from the GPIO controller schema.

ok

> > +    const: 2
> > +
> > +  gpio-controller:
> > +    type: boolean
> > +    description: |
> > +      if defined, MT7530's LED controller will run on GPIO mode.
> > +
> > +  "#interrupt-cells":
> > +    const: 1
> > +
> > +  interrupt-controller:
> > +    type: boolean
> > +
> > +  interrupts:
> > +    maxItems: 1
> > +
> > +  io-supply:
> > +    description: |
> > +      Phandle to the regulator node necessary for the I/O power.
> > +      See Documentation/devicetree/bindings/regulator/mt6323-regulator.txt
> > +      for details for the regulator setup on these boards.
> > +
> > +  mediatek,mcm:
> > +    type: boolean
> > +    description: |
> > +      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.
> 
> Skip description.
ok 
> > +    maxItems: 1
> > +
> > +  reset-names:
> > +    description: |
> > +      Should be set to "mcm".
> 
> Skip description.
ok
> > +    const: mcm
> > +
> > +  resets:
> > +    description: |
> > +      Phandle pointing to the system reset controller with line index for
> > +      the ethsys.
> > +    maxItems: 1
> > +
> > +required:
> > +  - compatible
> > +  - reg
> > +
> > +allOf:
> > +  - $ref: "dsa.yaml#"
> > +  - if:
> > +      required:
> > +        - mediatek,mcm
> > +    then:
> > +      required:
> > +        - resets
> > +        - reset-names
> > +    else:
> > +      required:
> > +        - reset-gpios
> > +
> > +  - if:
> > +      required:
> > +        - interrupt-controller
> > +    then:
> > +      required:
> > +        - interrupts
> > +
> > +  - if:
> > +      properties:
> > +        compatible:
> > +          items:
> > +            - const: mediatek,mt7530
> > +    then:
> > +      required:
> > +        - core-supply
> > +        - io-supply
> > +
> > +
> > +patternProperties:
> 
> patternProperties go before allOf, just after regular properties.

after required, right?

> > +  "^(ethernet-)?ports$":
> > +    type: object
> 
> Also on this level:
>     unevaluatedProperties: false

this is imho a bit redundant because in dsa.yaml (which is included now after patternProperties)
it is already set on both levels.
Adding it here will fail in examples because of size/address-cells which are already defined in dsa.yaml...
so i need to define them here again.

> > +
> > +    patternProperties:
> > +      "^(ethernet-)?port@[0-9]+$":
> > +        type: object
> > +        description: Ethernet switch ports
> > +
> > +        properties:
> > +          reg:
> > +            description: |
> > +              Port address described must be 6 for CPU port and from 0 to 5 for user ports.
> 
> This looks like not wrapped @80 character.

i fix this

> > +
> > +        unevaluatedProperties: false
> > +
> > +        allOf:
> > +          - $ref: dsa-port.yaml#
> > +          - if:
> > +              properties:
> > +                label:
> > +                  items:
> > +                    - const: cpu
> > +            then:
> > +              required:
> > +                - reg
> > +                - phy-mode
> > +
> > +unevaluatedProperties: false
> 
> Best regards,
> Krzysztof

regards Frank