| Message ID | 20220730142627.29028-5-arinc.unal@arinc9.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | completely rework mediatek,mt7530 binding | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On 30/07/2022 16:26, Arınç ÜNAL wrote: > Update the json-schema for compatible devices. > > - Define acceptable phy-mode values for CPU port of each compatible device. > - Remove requiring the "reg" property since the referred dsa-port.yaml > already does that. > - Require mediatek,mcm for the described MT7621 SoCs as the compatible > string is only used for MT7530 which is a part of the multi-chip module. 3 separate patches. > > Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> > --- > .../bindings/net/dsa/mediatek,mt7530.yaml | 220 +++++++++++++++--- > 1 file changed, 191 insertions(+), 29 deletions(-) > > diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > index a88e650e910b..a37a14fba9f6 100644 > --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml > @@ -135,35 +135,6 @@ properties: > the ethsys. > maxItems: 1 > > -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. > - > - patternProperties: Best regards, Krzysztof
On 2.08.2022 11:46, Krzysztof Kozlowski wrote: > On 30/07/2022 16:26, Arınç ÜNAL wrote: >> Update the json-schema for compatible devices. >> >> - Define acceptable phy-mode values for CPU port of each compatible device. >> - Remove requiring the "reg" property since the referred dsa-port.yaml >> already does that. >> - Require mediatek,mcm for the described MT7621 SoCs as the compatible >> string is only used for MT7530 which is a part of the multi-chip module. > > 3 separate patches. Roger. > >> >> Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> >> --- >> .../bindings/net/dsa/mediatek,mt7530.yaml | 220 +++++++++++++++--- >> 1 file changed, 191 insertions(+), 29 deletions(-) >> >> diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >> index a88e650e910b..a37a14fba9f6 100644 >> --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >> +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml >> @@ -135,35 +135,6 @@ properties: >> the ethsys. >> maxItems: 1 >> >> -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. If I put allOf:if:then under patternProperties, I can't do the latter. Other than readability to human eyes, binding check works as intended, in case there's no other way to do it. Arınç
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. > > Other than readability to human eyes, binding check works as intended, > in case there's no other way to do it. I don't see the problem in doing it and readability is one of main factors of code admission to Linux kernel. Best regards, Krzysztof
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. > >> >> Other than readability to human eyes, binding check works as intended, >> in case there's no other way to do it. > > I don't see the problem in doing it and readability is one of main > factors of code admission to Linux kernel. One more thought - if your schema around allOf:if:then grows too much, it is actually a sign that it might benefit from splitting. Either into two separate schemas or into common+two separate. Best regards, Krzysztof
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: compatible: items: - const: mediatek,mt7530 - const: mediatek,mt7621 then: allOf: - if: properties: reg: const: 5 then: properties: phy-mode: enum: - gmii - mii - rgmii - if: properties: reg: const: 6 then: properties: phy-mode: enum: - rgmii - trgmii - if: properties: compatible: items: - const: mediatek,mt7531 then: allOf: - if: properties: reg: const: 5 then: properties: phy-mode: enum: - 1000base-x - 2500base-x - rgmii - sgmii - if: properties: reg: const: 6 then: properties: phy-mode: enum: - 1000base-x - 2500base-x - sgmii properties: reg: enum: - 5 - 6 required: - phy-mode >> >>> >>> Other than readability to human eyes, binding check works as intended, >>> in case there's no other way to do it. >> >> I don't see the problem in doing it and readability is one of main >> factors of code admission to Linux kernel. > > One more thought - if your schema around allOf:if:then grows too much, > it is actually a sign that it might benefit from splitting. Either into > two separate schemas or into common+two separate. > > Best regards, > Krzysztof Arınç
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. Best regards, Krzysztof
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ç
On 12/08/2022 17:06, Arınç ÜNAL wrote: > > > 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. 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
diff --git a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml index a88e650e910b..a37a14fba9f6 100644 --- a/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml +++ b/Documentation/devicetree/bindings/net/dsa/mediatek,mt7530.yaml @@ -135,35 +135,6 @@ properties: the ethsys. maxItems: 1 -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: - required: - - reg - - phy-mode - required: - compatible - reg @@ -187,10 +158,201 @@ allOf: items: - const: mediatek,mt7530 then: + 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: + reg: + const: 5 + then: + properties: + phy-mode: + enum: + - gmii + - mii + - rgmii + + - if: + properties: + reg: + const: 6 + then: + properties: + phy-mode: + enum: + - rgmii + - trgmii + + properties: + reg: + enum: + - 5 + - 6 + + required: + - phy-mode + required: - core-supply - io-supply + - if: + properties: + compatible: + items: + - const: mediatek,mt7531 + then: + 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: + reg: + const: 5 + then: + properties: + phy-mode: + enum: + - 1000base-x + - 2500base-x + - rgmii + - sgmii + + - if: + properties: + reg: + const: 6 + then: + properties: + phy-mode: + enum: + - 1000base-x + - 2500base-x + - sgmii + + properties: + reg: + enum: + - 5 + - 6 + + required: + - phy-mode + + - if: + properties: + compatible: + items: + - const: mediatek,mt7621 + then: + 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: + reg: + const: 5 + then: + properties: + phy-mode: + enum: + - gmii + - mii + - rgmii + + - if: + properties: + reg: + const: 6 + then: + properties: + phy-mode: + enum: + - rgmii + - trgmii + + properties: + reg: + enum: + - 5 + - 6 + + required: + - phy-mode + + required: + - mediatek,mcm + unevaluatedProperties: false examples:
Update the json-schema for compatible devices. - Define acceptable phy-mode values for CPU port of each compatible device. - Remove requiring the "reg" property since the referred dsa-port.yaml already does that. - Require mediatek,mcm for the described MT7621 SoCs as the compatible string is only used for MT7530 which is a part of the multi-chip module. Signed-off-by: Arınç ÜNAL <arinc.unal@arinc9.com> --- .../bindings/net/dsa/mediatek,mt7530.yaml | 220 +++++++++++++++--- 1 file changed, 191 insertions(+), 29 deletions(-)