Thread (17 messages) 17 messages, 2 authors, 2022-08-19

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: 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.

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help