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

[Johan Hovold]

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@xxxxxxxxxx>
> >>> ---
> >>>  .../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