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

["Arınç ÜNAL" <arinc.unal@arinc9.com>]

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ç