| Message ID | 20220705094239.17174-18-johan+linaro@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | phy: qcom,qmp: fix dt-bindings and deprecate lane suffix | expand |
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
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. > > + 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 Johan
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. Best regards, Krzysztof
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
On Tue, 05 Jul 2022 11:42:13 +0200, 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(-) > My bot found errors running 'make DT_CHECKER_FLAGS=-m dt_binding_check' on your patch (DT_CHECKER_FLAGS is new in v5.13): yamllint warnings/errors: dtschema/dtc warnings/errors: /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml: allOf:3:else:patternProperties:^phy@[0-9a-f]+$:properties:reg: {'minItems': 3, 'maxItems': 4, 'items': [{'description': 'TX'}, {'description': 'RX'}, {'description': 'PCS'}, {'description': 'PCS_MISC'}]} should not be valid under {'required': ['maxItems']} hint: "maxItems" is not needed with an "items" list from schema $id: http://devicetree.org/meta-schemas/items.yaml# /builds/robherring/linux-dt-review/Documentation/devicetree/bindings/phy/qcom,qmp-pcie-phy.yaml: ignoring, error in schema: allOf: 3: else: patternProperties: ^phy@[0-9a-f]+$: properties: reg doc reference errors (make refcheckdocs): See https://patchwork.ozlabs.org/patch/ This check can fail if there are any dependencies. The base for a patch series is generally the most recent rc1. If you already ran 'make dt_binding_check' and didn't see the above error(s), then make sure 'yamllint' is installed and dt-schema is up to date: pip3 install dtschema --upgrade Please check and re-submit.
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
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 --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
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(-)