Re: [PATCH 3/4] dt-bindings: display/msm: add stream 1 pixel clock binding

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

Hi Krzysztof

On 12/3/2024 12:04 AM, Krzysztof Kozlowski wrote:
> On 03/12/2024 04:31, Abhinav Kumar wrote:
>> On some chipsets the display port controller can support more
> 
> Which chipsets?
> 

 From the current list of chipsets which support DP, the following can 
support more than one stream.

qcom,sa8775p-dp
qcom,sc7280-dp
qcom,sc8180x-dp
qcom,sc8280xp-dp
qcom,sm8350-dp
qcom,sm8650-dp
qcom,sm8550-dp
qcom,sm8450-dp
qcom,sm8250-dp
qcom,sm8150-dp

So do you also want all of these to be added in the same if block as
qcom,sa8775p-dp?

>> than one pixel stream (multi-stream transport). To support MST
>> on such chipsets, add the binding for stream 1 pixel clock for
>> display port controller. Since this mode is not supported on all
>> chipsets, add exception rules and min/max items to clearly mark
>> which chipsets support only SST mode (single stream) and which ones
>> support MST.
>>
>> Signed-off-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
>> ---
>>   .../bindings/display/msm/dp-controller.yaml        | 32 ++++++++++++++++++++++
>>   .../bindings/display/msm/qcom,sa8775p-mdss.yaml    |  9 ++++--
>>   2 files changed, 38 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> index 9fe2bf0484d8..650d19e58277 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -50,30 +50,38 @@ properties:
>>       maxItems: 1
>>   
>>     clocks:
>> +    minItems: 5
>>       items:
>>         - description: AHB clock to enable register access
>>         - description: Display Port AUX clock
>>         - description: Display Port Link clock
>>         - description: Link interface clock between DP and PHY
>>         - description: Display Port stream 0 Pixel clock
>> +      - description: Display Port stream 1 Pixel clock
>>   
>>     clock-names:
>> +    minItems: 5
>>       items:
>>         - const: core_iface
>>         - const: core_aux
>>         - const: ctrl_link
>>         - const: ctrl_link_iface
>>         - const: stream_pixel
>> +      - const: stream_1_pixel
>>   
>>     assigned-clocks:
>> +    minItems: 2
>>       items:
>>         - description: link clock source
>>         - description: stream 0 pixel clock source
>> +      - description: stream 1 pixel clock source
>>   
>>     assigned-clock-parents:
>> +    minItems: 2
>>       items:
>>         - description: Link clock PLL output provided by PHY block
>>         - description: Stream 0 pixel clock PLL output provided by PHY block
>> +      - description: Stream 1 pixel clock PLL output provided by PHY block
>>   
>>     phys:
>>       maxItems: 1
>> @@ -175,6 +183,30 @@ allOf:
>>         required:
>>           - "#sound-dai-cells"
>>   
> 
> Missing if: narrowing this to 5 items for other devices.
> 

OR would an else be better?

+    else:
+      properties:
+        clocks:
+          maxItems: 5
+        clock-names:
+          items:
+            - const: core_iface
+            - const: core_aux
+            - const: ctrl_link
+            - const: ctrl_link_iface
+            - const: stream_pixel

>> +  - if:
>> +      properties:
>> +        compatible:
>> +          contains:
>> +            enum:
>> +              - qcom,sa8775p-dp
>> +
>> +    then:
>> +      properties:
>> +        clocks:
> 
> Missing minItems, otherwise it is pointless.
> 

I thought that since I have already specified the minItems as 5
in the clocks in the section above, I need to specify only the maxItems 
here?

    clocks:
+    minItems: 5
      items:
        - description: AHB clock to enable register access
        - description: Display Port AUX clock
        - description: Display Port Link clock
        - description: Link interface clock between DP and PHY
        - description: Display Port stream 0 Pixel clock
+      - description: Display Port stream 1 Pixel clock


>> +          maxItems: 6
>> +        clock-names:
>> +          items:
>> +            - const: core_iface
>> +            - const: core_aux
>> +            - const: ctrl_link
>> +            - const: ctrl_link_iface
>> +            - const: stream_pixel
>> +            - const: stream_1_pixel
>> +        assigned-clocks:
>> +          maxItems: 3
> 
> Missing minItems... or just drop, it's not accurate or not even correct.
> I can assign 4 clocks, why not? Or rather: why do you stop users from
> assigning 4 clocks?
> 

Sure, I can drop this.

> 
>> +        assigned-clock-parents:
>> +          maxItems: 3
>> +
>>   additionalProperties: false
> 
> 
> 
> Best regards,
> Krzysztof