Re: [PATCH 4/4] dt-bindings: net: dsa: mediatek,mt7530: update json-schema
From: Krzysztof Kozlowski <hidden>
Date: 2022-08-19 12:41:07
Also in:
linux-arm-kernel, linux-devicetree, linux-mediatek, lkml
On 12/08/2022 17:06, Arınç ÜNAL wrote:
On 12.08.2022 16:48, Krzysztof Kozlowski wrote:quoted
On 12/08/2022 16:41, Arınç ÜNAL wrote:quoted
On 12.08.2022 10:01, Krzysztof Kozlowski wrote:quoted
On 12/08/2022 09:57, Krzysztof Kozlowski wrote:quoted
On 12/08/2022 01:09, Arınç ÜNAL wrote:quoted
quoted
quoted
-patternProperties: - "^(ethernet-)?ports$": - type: objectActually 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.quoted
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.
None of the reasons above force you to define properties in some allOf:if:then subblock. These force you to constrain the properties in allOf:if:then, but not define.
Splitting to separate schema (per compatible string?) wouldn't help in this case.
True.
I can split patternProperties to two sections, but I can't directly define the reg property like you put above.
Of course you can and original bindings were doing it. Let me ask specific questions (yes, no): 1. Are ethernet-ports and ethernet-port present in each variant? 2. Is dsa-port.yaml applicable to each variant? (looks like that - three compatibles, three all:if:then) 3. If reg appearing in each variant? 4. If above is true, if reg is maximum one item in each variant? Looking at your patch, I think answer is 4x yes, which means you can define them in one place and constrain in allOf:if:then, just like all other schemas, because this one is not different. Best regards, Krzysztof