diff mbox series

[17/43] dt-bindings: phy: qcom,qmp-pcie: add missing child node schema

Message ID 20220705094239.17174-18-johan+linaro@kernel.org
State New
Headers show
Series phy: qcom,qmp: fix dt-bindings and deprecate lane suffix | expand

Commit Message

Johan Hovold July 5, 2022, 9:42 a.m. UTC
Add the missing the description of the PHY-provider child node which was
ignored when converting to DT schema.

Also fix up the incorrect description that claimed that one child node
per lane was required.

Fixes: ccf51c1cedfd ("dt-bindings: phy: qcom,qmp: Convert QMP PHY bindings to yaml")
Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
---
 .../bindings/phy/qcom,qmp-pcie-phy.yaml       | 88 ++++++++++++++++++-
 1 file changed, 85 insertions(+), 3 deletions(-)

Comments

Krzysztof Kozlowski July 5, 2022, 10:18 a.m. UTC | #1
On 05/07/2022 11:42, Johan Hovold wrote:
> Add the missing the description of the PHY-provider child node which was
> ignored when converting to DT schema.
> 
> Also fix up the incorrect description that claimed that one child node
> per lane was required.
> 
> Fixes: ccf51c1cedfd ("dt-bindings: phy: qcom,qmp: Convert QMP PHY bindings to yaml")
> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> ---
>  .../bindings/phy/qcom,qmp-pcie-phy.yaml       | 88 ++++++++++++++++++-
>  1 file changed, 85 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> index ff1577f68a00..5a1ebf874559 100644
> --- a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> +++ b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> @@ -69,9 +69,37 @@ properties:
>  patternProperties:
>    "^phy@[0-9a-f]+$":
>      type: object
> -    description:
> -      Each device node of QMP PHY is required to have as many child nodes as
> -      the number of lanes the PHY has.
> +    description: Single PHY-provider child node.
> +    properties:
> +      reg:
> +        minItems: 3
> +        maxItems: 6
> +
> +      clocks:
> +        items:
> +          - description: PIPE clock
> +
> +      clock-names:
> +        items:
> +          - const: pipe0
> +
> +      "#clock-cells":
> +        const: 0
> +
> +      clock-output-names: true
> +
> +      "#phy-cells":
> +        const: 0
> +
> +    required:
> +      - reg
> +      - clocks
> +      - clock-names
> +      - "#clock-cells"
> +      - clock-output-names
> +      - "#phy-cells"
> +
> +    additionalProperties: false
>  
>  required:
>    - compatible
> @@ -180,3 +208,57 @@ allOf:
>        required:
>          - vdda-phy-supply
>          - vdda-pll-supply
> +
> +  - if:
> +      properties:
> +        compatible:
> +          contains:
> +            enum:
> +              - qcom,sm8250-qmp-gen3x2-pcie-phy
> +              - qcom,sm8250-qmp-modem-pcie-phy
> +              - qcom,sm8450-qmp-gen4x2-pcie-phy
> +    then:
> +      patternProperties:
> +        "^phy@[0-9a-f]+$":
> +          properties:
> +            reg:
> +              items:
> +                - description: TX lane 1
> +                - description: RX lane 1
> +                - description: PCS
> +                - description: TX lane 2
> +                - description: RX lane 2
> +                - description: PCS_MISC
> +    else:
> +      patternProperties:
> +        "^phy@[0-9a-f]+$":
> +          properties:
> +            reg:
> +              minItems: 3
> +              maxItems: 4
> +              items:
> +                - description: TX
> +                - description: RX
> +                - description: PCS
> +                - description: PCS_MISC
> +      if:

Do not include if within other if. Just split the entire section to its
own if:.


> +        properties:
> +          compatible:
> +            contains:
> +              enum:
> +                - qcom,ipq6018-qmp-pcie-phy
> +                - qcom,ipq8074-qmp-pcie-phy
> +                - qcom,msm8998-qmp-pcie-phy
> +                - qcom,sdm845-qhp-pcie-phy
> +      then:
> +        patternProperties:
> +          "^phy@[0-9a-f]+$":
> +            properties:
> +              reg:
> +                maxItems: 3
> +      else:
> +        patternProperties:
> +          "^phy@[0-9a-f]+$":
> +            properties:
> +              reg:
> +                minItems: 4


Best regards,
Krzysztof
Johan Hovold July 5, 2022, 12:11 p.m. UTC | #2
On Tue, Jul 05, 2022 at 01:56:32PM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2022 13:51, Johan Hovold wrote:
> > On Tue, Jul 05, 2022 at 12:18:37PM +0200, Krzysztof Kozlowski wrote:
> >> On 05/07/2022 11:42, Johan Hovold wrote:
> >>> Add the missing the description of the PHY-provider child node which was
> >>> ignored when converting to DT schema.
> >>>
> >>> Also fix up the incorrect description that claimed that one child node
> >>> per lane was required.
> >>>
> >>> Fixes: ccf51c1cedfd ("dt-bindings: phy: qcom,qmp: Convert QMP PHY bindings to yaml")
> >>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
> >>> ---
> >>>  .../bindings/phy/qcom,qmp-pcie-phy.yaml       | 88 ++++++++++++++++++-
> >>>  1 file changed, 85 insertions(+), 3 deletions(-)
> >>>
> >>> diff --git a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> >>> index ff1577f68a00..5a1ebf874559 100644
> >>> --- a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> >>> +++ b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
> >>> @@ -69,9 +69,37 @@ properties:
> > 
> >>> +  - if:
> >>> +      properties:
> >>> +        compatible:
> >>> +          contains:
> >>> +            enum:
> >>> +              - qcom,sm8250-qmp-gen3x2-pcie-phy
> >>> +              - qcom,sm8250-qmp-modem-pcie-phy
> >>> +              - qcom,sm8450-qmp-gen4x2-pcie-phy
> >>> +    then:
> >>> +      patternProperties:
> >>> +        "^phy@[0-9a-f]+$":
> >>> +          properties:
> >>> +            reg:
> >>> +              items:
> >>> +                - description: TX lane 1
> >>> +                - description: RX lane 1
> >>> +                - description: PCS
> >>> +                - description: TX lane 2
> >>> +                - description: RX lane 2
> >>> +                - description: PCS_MISC
> >>> +    else:
> >>> +      patternProperties:
> >>> +        "^phy@[0-9a-f]+$":
> >>> +          properties:
> >>> +            reg:
> >>> +              minItems: 3
> >>> +              maxItems: 4
> >>> +              items:
> >>> +                - description: TX
> >>> +                - description: RX
> >>> +                - description: PCS
> >>> +                - description: PCS_MISC
> >>> +      if:
> >>
> >> Do not include if within other if. Just split the entire section to its
> >> own if:.
> > 
> > That sounds like it would just obfuscate the logic. The else clause
> > specified 3-4 registers and the nested if determines which compatibles
> > use which by further narrowing the range.
> > 
> > If you move it out to the else: this would be really hard understand and
> > verify.
> 
> Every bindings are expected to do that way and most of them are doing
> it: define broad constraints in properties:, then define strict
> constraints per each variant. Easy to follow code. This binding is not
> particularly special to make it different than other ones. Doing
> semi-strict constraints in if: and then additional constrain in nested
> if: is not easy to understand and verify.

Ok, so you want to flatten this by repeating also the register
descriptions?

That wouldn't hurt readability as much, but doing so would be more error
prone as it's easy to miss adding a new compatible in every group of
conditionals and there's no else clause to catch the mistake.

Right know the logic is

	if dual-lane
		items = 6
	else
		items = 3 or 4
		if single-lane-exception
			items = 3
		else
			items = 4

Flattening this gives

	if dual-lane
		items = 6
	if single-lane-normal
		items = 4
	if single-lane-exception
		items = 3

Which means that every compatible must now be listed in one of the
conditionals.

Fine with me, but please confirm that I understood you correctly.

Johan
Krzysztof Kozlowski July 5, 2022, 6:21 p.m. UTC | #3
On 05/07/2022 14:11, Johan Hovold wrote:
> On Tue, Jul 05, 2022 at 01:56:32PM +0200, Krzysztof Kozlowski wrote:
>> On 05/07/2022 13:51, Johan Hovold wrote:
>>> On Tue, Jul 05, 2022 at 12:18:37PM +0200, Krzysztof Kozlowski wrote:
>>>> On 05/07/2022 11:42, Johan Hovold wrote:
>>>>> Add the missing the description of the PHY-provider child node which was
>>>>> ignored when converting to DT schema.
>>>>>
>>>>> Also fix up the incorrect description that claimed that one child node
>>>>> per lane was required.
>>>>>
>>>>> Fixes: ccf51c1cedfd ("dt-bindings: phy: qcom,qmp: Convert QMP PHY bindings to yaml")
>>>>> Signed-off-by: Johan Hovold <johan+linaro@kernel.org>
>>>>> ---
>>>>>  .../bindings/phy/qcom,qmp-pcie-phy.yaml       | 88 ++++++++++++++++++-
>>>>>  1 file changed, 85 insertions(+), 3 deletions(-)
>>>>>
>>>>> diff --git a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
>>>>> index ff1577f68a00..5a1ebf874559 100644
>>>>> --- a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
>>>>> +++ b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
>>>>> @@ -69,9 +69,37 @@ properties:
>>>
>>>>> +  - if:
>>>>> +      properties:
>>>>> +        compatible:
>>>>> +          contains:
>>>>> +            enum:
>>>>> +              - qcom,sm8250-qmp-gen3x2-pcie-phy
>>>>> +              - qcom,sm8250-qmp-modem-pcie-phy
>>>>> +              - qcom,sm8450-qmp-gen4x2-pcie-phy
>>>>> +    then:
>>>>> +      patternProperties:
>>>>> +        "^phy@[0-9a-f]+$":
>>>>> +          properties:
>>>>> +            reg:
>>>>> +              items:
>>>>> +                - description: TX lane 1
>>>>> +                - description: RX lane 1
>>>>> +                - description: PCS
>>>>> +                - description: TX lane 2
>>>>> +                - description: RX lane 2
>>>>> +                - description: PCS_MISC
>>>>> +    else:
>>>>> +      patternProperties:
>>>>> +        "^phy@[0-9a-f]+$":
>>>>> +          properties:
>>>>> +            reg:
>>>>> +              minItems: 3
>>>>> +              maxItems: 4
>>>>> +              items:
>>>>> +                - description: TX
>>>>> +                - description: RX
>>>>> +                - description: PCS
>>>>> +                - description: PCS_MISC
>>>>> +      if:
>>>>
>>>> Do not include if within other if. Just split the entire section to its
>>>> own if:.
>>>
>>> That sounds like it would just obfuscate the logic. The else clause
>>> specified 3-4 registers and the nested if determines which compatibles
>>> use which by further narrowing the range.
>>>
>>> If you move it out to the else: this would be really hard understand and
>>> verify.
>>
>> Every bindings are expected to do that way and most of them are doing
>> it: define broad constraints in properties:, then define strict
>> constraints per each variant. Easy to follow code. This binding is not
>> particularly special to make it different than other ones. Doing
>> semi-strict constraints in if: and then additional constrain in nested
>> if: is not easy to understand and verify.
> 
> Ok, so you want to flatten this by repeating also the register
> descriptions?
> 
> That wouldn't hurt readability as much, but doing so would be more error
> prone as it's easy to miss adding a new compatible in every group of
> conditionals and there's no else clause to catch the mistake.
> 
> Right know the logic is
> 
> 	if dual-lane
> 		items = 6
> 	else
> 		items = 3 or 4
> 		if single-lane-exception
> 			items = 3
> 		else
> 			items = 4
> 
> Flattening this gives
> 
> 	if dual-lane
> 		items = 6
> 	if single-lane-normal
> 		items = 4
> 	if single-lane-exception
> 		items = 3
> 
> Which means that every compatible must now be listed in one of the
> conditionals.

Yes, because it's explicit and easy to read. Handling compatibles in
'else' makes it opposite - one cannot use grep and cannot easily find
what is actually covered by maxItems:4 (you need to check all 7
compatibles to find what is not covered here).

> 
> Fine with me, but please confirm that I understood you correctly.

You have already flattened if-if-if for clocks and resets, so this
should follow similar approach. I don't think it could be squashed with
that previous if-if-if, though.

Best regards,
Krzysztof
Johan Hovold July 6, 2022, 6:06 a.m. UTC | #4
On Tue, Jul 05, 2022 at 08:21:12PM +0200, Krzysztof Kozlowski wrote:
> On 05/07/2022 14:11, Johan Hovold wrote:

> > Ok, so you want to flatten this by repeating also the register
> > descriptions?
> > 
> > That wouldn't hurt readability as much, but doing so would be more error
> > prone as it's easy to miss adding a new compatible in every group of
> > conditionals and there's no else clause to catch the mistake.
> > 
> > Right know the logic is
> > 
> > 	if dual-lane
> > 		items = 6
> > 	else
> > 		items = 3 or 4
> > 		if single-lane-exception
> > 			items = 3
> > 		else
> > 			items = 4
> > 
> > Flattening this gives
> > 
> > 	if dual-lane
> > 		items = 6
> > 	if single-lane-normal
> > 		items = 4
> > 	if single-lane-exception
> > 		items = 3
> > 
> > Which means that every compatible must now be listed in one of the
> > conditionals.
> 
> Yes, because it's explicit and easy to read. Handling compatibles in
> 'else' makes it opposite - one cannot use grep and cannot easily find
> what is actually covered by maxItems:4 (you need to check all 7
> compatibles to find what is not covered here).

I'll go with that then.

Johan
diff mbox series

Patch

diff --git a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
index ff1577f68a00..5a1ebf874559 100644
--- a/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
+++ b/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml
@@ -69,9 +69,37 @@  properties:
 patternProperties:
   "^phy@[0-9a-f]+$":
     type: object
-    description:
-      Each device node of QMP PHY is required to have as many child nodes as
-      the number of lanes the PHY has.
+    description: Single PHY-provider child node.
+    properties:
+      reg:
+        minItems: 3
+        maxItems: 6
+
+      clocks:
+        items:
+          - description: PIPE clock
+
+      clock-names:
+        items:
+          - const: pipe0
+
+      "#clock-cells":
+        const: 0
+
+      clock-output-names: true
+
+      "#phy-cells":
+        const: 0
+
+    required:
+      - reg
+      - clocks
+      - clock-names
+      - "#clock-cells"
+      - clock-output-names
+      - "#phy-cells"
+
+    additionalProperties: false
 
 required:
   - compatible
@@ -180,3 +208,57 @@  allOf:
       required:
         - vdda-phy-supply
         - vdda-pll-supply
+
+  - if:
+      properties:
+        compatible:
+          contains:
+            enum:
+              - qcom,sm8250-qmp-gen3x2-pcie-phy
+              - qcom,sm8250-qmp-modem-pcie-phy
+              - qcom,sm8450-qmp-gen4x2-pcie-phy
+    then:
+      patternProperties:
+        "^phy@[0-9a-f]+$":
+          properties:
+            reg:
+              items:
+                - description: TX lane 1
+                - description: RX lane 1
+                - description: PCS
+                - description: TX lane 2
+                - description: RX lane 2
+                - description: PCS_MISC
+    else:
+      patternProperties:
+        "^phy@[0-9a-f]+$":
+          properties:
+            reg:
+              minItems: 3
+              maxItems: 4
+              items:
+                - description: TX
+                - description: RX
+                - description: PCS
+                - description: PCS_MISC
+      if:
+        properties:
+          compatible:
+            contains:
+              enum:
+                - qcom,ipq6018-qmp-pcie-phy
+                - qcom,ipq8074-qmp-pcie-phy
+                - qcom,msm8998-qmp-pcie-phy
+                - qcom,sdm845-qhp-pcie-phy
+      then:
+        patternProperties:
+          "^phy@[0-9a-f]+$":
+            properties:
+              reg:
+                maxItems: 3
+      else:
+        patternProperties:
+          "^phy@[0-9a-f]+$":
+            properties:
+              reg:
+                minItems: 4