Re: [PATCH 4/4] dt-bindings: net: dsa: mediatek,mt7530: update json-schema

[Arınç ÜNAL]

On 12.08.2022 16:48, Krzysztof Kozlowski wrote:
> On 12/08/2022 16:41, Arınç ÜNAL wrote:
> > On 12.08.2022 10:01, Krzysztof Kozlowski wrote:
> > > On 12/08/2022 09:57, Krzysztof Kozlowski wrote:
> > > > On 12/08/2022 01:09, Arınç ÜNAL wrote:
> > > > > > > -patternProperties:
> > > > > > > -  "^(ethernet-)?ports$":
> > > > > > > -    type: object
> > > > > >
> > > > > > Actually four patches...
> > > > > >
> > > > > > I don't find this change explained in commit msg. What is more, it looks
> > > > > > incorrect. All properties and patternProperties should be explained in
> > > > > > top-level part.
> > > > > >
> > > > > > Defining such properties (with big piece of YAML) in each if:then: is no
> > > > > > readable.
> > > > >
> > > > > I can't figure out another way. I need to require certain properties for
> > > > > a compatible string AND certain enum/const for certain properties which
> > > > > are inside patternProperties for "^(ethernet-)?port@[0-9]+$" by reading
> > > > > the compatible string.
> > > >
> > > > requiring properties is not equal to defining them and nothing stops you
> > > > from defining all properties top-level and requiring them in
> > > > allOf:if:then:patternProperties.
> > > >
> > > >
> > > > > If I put allOf:if:then under patternProperties, I can't do the latter.
> > > >
> > > > You can.
> >
> > Am I supposed to do something like this:
> >
> > patternProperties:
> >     "^(ethernet-)?ports$":
> >       type: object
> >
> >       patternProperties:
> >         "^(ethernet-)?port@[0-9]+$":
> >           type: object
> >           description: Ethernet switch ports
> >
> >           unevaluatedProperties: false
> >
> >           properties:
> >             reg:
> >               description:
> >                 Port address described must be 5 or 6 for CPU port and
> >                 from 0 to 5 for user ports.
> >
> >           allOf:
> >             - $ref: dsa-port.yaml#
> >             - if:
> >                 properties:
> >                   label:
> >                     items:
> >                       - const: cpu
> >               then:
> >                 allOf:
> >                   - if:
> >                       properties:
>
> Not really, this is absolutely unreadable.
>
> Usually the way it is handled is:
>
> patternProperties:
>     "^(ethernet-)?ports$":
>       type: object
>
>       patternProperties:
>         "^(ethernet-)?port@[0-9]+$":
>           type: object
>           description: Ethernet switch ports
>           unevaluatedProperties: false
>           ... regular stuff follows
>
> allOf:
>   - if:
>       properties:
>         compatible:
>           .....
>     then:
>       patternProperties:
>         "^(ethernet-)?ports$":
>           patternProperties:
>             "^(ethernet-)?port@[0-9]+$":
>               properties:
>                 reg:
>                   const: 5
>
>
> I admit that it is still difficult to parse, which could justify
> splitting to separate schema. Anyway the point of my comment was to
> define all properties in top level, not in allOf.
>
> allOf should be used to constrain these properties.

The problem is:
- only specific values of reg are allowed if label is cpu.
- only specific values of phy-mode are allowed if reg is 5 or 6.

This forces me to define properties under allOf:if:then. Splitting to 
separate schema (per compatible string?) wouldn't help in this case.

I can split patternProperties to two sections, but I can't directly 
define the reg property like you put above.

I can at least split mediatek,mt7531 to a separate schema to have less 
patternProperties on a single binding.

What do you think?

Arınç