Re: [PATCH v1 1/7] dt-bindings: PCI: qcom: Add sm8350 to bindings

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

On 01/11/2022 20:22, Rob Herring wrote:
> On Mon, Oct 31, 2022 at 4:47 PM Dmitry Baryshkov
> <dmitry.baryshkov@linaro.org> wrote:
>>
>> On Tue, 1 Nov 2022 at 00:40, Rob Herring <robh@kernel.org> wrote:
>>>
>>> On Sun, Oct 30, 2022 at 12:13:06AM +0300, Dmitry Baryshkov wrote:
>>>> Add bindings for two PCIe hosts on SM8350 platform. The only difference
>>>> between them is in the aggre0 clock, which warrants the oneOf clause for
>>>> the clocks properties.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>   .../devicetree/bindings/pci/qcom,pcie.yaml    | 54 +++++++++++++++++++
>>>>   1 file changed, 54 insertions(+)
>>>>
>>>> diff --git a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>> index 54f07852d279..55bf5958ef79 100644
>>>> --- a/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>> +++ b/Documentation/devicetree/bindings/pci/qcom,pcie.yaml
>>>> @@ -32,6 +32,7 @@ properties:
>>>>         - qcom,pcie-sdm845
>>>>         - qcom,pcie-sm8150
>>>>         - qcom,pcie-sm8250
>>>> +      - qcom,pcie-sm8350
>>>>         - qcom,pcie-sm8450-pcie0
>>>>         - qcom,pcie-sm8450-pcie1
>>>>         - qcom,pcie-ipq6018
>>>> @@ -185,6 +186,7 @@ allOf:
>>>>                 - qcom,pcie-sc8180x
>>>>                 - qcom,pcie-sc8280xp
>>>>                 - qcom,pcie-sm8250
>>>> +              - qcom,pcie-sm8350
>>>>                 - qcom,pcie-sm8450-pcie0
>>>>                 - qcom,pcie-sm8450-pcie1
>>>>       then:
>>>> @@ -540,6 +542,57 @@ allOf:
>>>>             items:
>>>>               - const: pci # PCIe core reset
>>>>
>>>> +  - if:
>>>> +      properties:
>>>> +        compatible:
>>>> +          contains:
>>>> +            enum:
>>>> +              - qcom,pcie-sm8350
>>>> +    then:
>>>> +      oneOf:
>>>> +          # Unfortunately the "optional" ref clock is used in the middle of the list
>>>> +        - properties:
>>>> +            clocks:
>>>> +              maxItems: 13
>>>> +            clock-names:
>>>> +              items:
>>>> +                - const: pipe # PIPE clock
>>>> +                - const: pipe_mux # PIPE MUX
>>>> +                - const: phy_pipe # PIPE output clock
>>>> +                - const: ref # REFERENCE clock
>>>> +                - const: aux # Auxiliary clock
>>>> +                - const: cfg # Configuration clock
>>>> +                - const: bus_master # Master AXI clock
>>>> +                - const: bus_slave # Slave AXI clock
>>>> +                - const: slave_q2a # Slave Q2A clock
>>>> +                - const: tbu # PCIe TBU clock
>>>> +                - const: ddrss_sf_tbu # PCIe SF TBU clock
>>>> +                - const: aggre0 # Aggre NoC PCIe0 AXI clock
>>>
>>> 'enum: [ aggre0, aggre1 ]' and 'minItems: 12' would eliminate the 2nd
>>> case. There's a implicit requirement that string names are unique (by
>>> default).
>>
>> Wouldn't it also allow a single 'aggre0' string?
> 
> No, because it's only for the 12th entry in the list.

If I got your suggestion right, it would be:
clock-names:
   minItems: 12
   items:
     ..... 11 names
     - enum: [ aggre0, aggre1 ]
     - const: aggre1

Having 11 clocks + aggre0 would pass this schema (incorrectly) because 
there will be no duplicate to fail the check.

We have two cases here:
  - 11 common clocks + aggre0 + aggre1
  - 11 common clocks + aggre1

I think I'll keep the oneOf in v2. Please tell me if I got your 
suggestion incorrectly or if there is any other way to express my case.

-- 
With best wishes
Dmitry