Re: [PATCH v9 2/2] dt-bindings: msm/dp: Add bindings of MSM DisplayPort controller

[Tanmay Shah <tanmay@codeaurora.org>]

On 2020-12-11 14:36, Stephen Boyd wrote:
> Quoting Krishna Manikandan (2020-12-10 23:09:45)
>> Changes in v10:
>> - Change title of this patch as it does not contain PLL bindings 
>> anymore
>> - Remove redundant properties
>> - Remove use of IRQ flag
>> - Fix ports property
>> 
>> Previous Change log:
>> https://lkml.kernel.org/lkml/1597066683-6044-2-git-send-email-mkrishn@codeaurora.org/
> 
> Why can't that be put in here instead of making me click the link?
> 

Thanks Stephen for reviews.

I just wanted to use space efficiently. But if it helps in reviews,
I will bring all the previous change logs back.

>> diff --git
>> a/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> new file mode 100644
>> index 0000000..bc80632
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-controller.yaml
>> @@ -0,0 +1,120 @@
>> +# SPDX-License-Identifier: (GPL-2.0-only  OR BSD-2-Clause)
> 
> Why two spaces between 'only and 'OR'?
> 
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dp-controller.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: MSM Display Port Controller.
> 
> Drop the full-stop?
> 
>> +
>> +maintainers:
>> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> +  - Vara Reddy <varar@codeaurora.org>
>> +  - Tanmay Shah <tanmay@codeaurora.org>
>> +
>> +description: |
>> +  Device tree bindings for DisplayPort host controller for MSM 
>> targets
>> +  that are compatible with VESA DisplayPort interface specification.
>> +
>> +properties:
>> +  compatible:
>> +    enum:
>> +      - qcom,sc7180-dp
>> +
>> +  reg:
>> +    maxItems: 1
>> +
>> +  interrupts:
>> +    maxItems: 1
>> +
>> +  clocks:
>> +    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 Pixel clock
>> +
>> +  clock-names:
>> +    items:
>> +      - const: core_iface
>> +      - const: core_aux
>> +      - const: ctrl_link
>> +      - const: ctrl_link_iface
>> +      - const: stream_pixel
> 
> There should be a dp-phy too, so 'phy' and 'phy-names' property. I see 
> a
> 'power-domains' property downstream, so please add that too. And also
> '#sound-dai-cells".
> 
>> +
>> +  ports:
>> +    type: object
>> +    description: |
>> +      A ports node with endpoint definitions as defined in
>> +      Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +    properties:
>> +      "#address-cells":
>> +        const: 1
>> +
>> +      "#size-cells":
>> +        const: 0
>> +
>> +      port@0:
>> +        type: object
>> +        description: Input endpoint of the controller.
>> +
>> +      port@1:
>> +        type: object
>> +        description: Output endpoint of the controller.
> 
> This should have a required section too, and state that all the above
> properties are required.
> 
>> +
>> +    additionalProperties: false
>> +
>> +required:
>> +  - compatible
>> +  - reg
>> +  - interrupts
>> +  - clocks
>> +  - clock-names
>> +  - ports
>> +
>> +additionalProperties: false
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,dispcc-sc7180.h>
>> +
>> +    mdss: mdss {
> 
> Can drop the label. Also this node name is bad! Should be
> 'subsystem@<reg>' I guess? And then the reg can just be the first
> address in a reg property that is remapped for the children nodes.
> 

I didn't get this part:
"And then the reg can just be the first address in a reg property that 
is remapped for children nodes."

Does this mean, I should add reg property for mdss node as well? Node 
mdss address is 0x0ae00000. So something like:

reg = <0 0x0ae00000 0 0x1000>;



>> +      #address-cells = <2>;
>> +      #size-cells = <2>;
>> +      interrupt-controller;
>> +
>> +      msm_dp: displayport-controller@ae90000 {
> 
> Can drop the label.
> 
>> +        compatible = "qcom,sc7180-dp";
>> +        reg = <0 0xae90000 0 0x1400>;
>> +        interrupt-parent = <&mdss>;
>> +        interrupts = <12>;
>> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> +                 <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>;
>> +        clock-names = "core_iface", "core_aux",
>> +                      "ctrl_link",
>> +                      "ctrl_link_iface", "stream_pixel";
>> +
>> +        ports {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            port@0 {
>> +                reg = <0>;
>> +                dp_in: endpoint {
> 
> Can drop the label.
> 
>> +                    remote-endpoint = <&dpu_intf0_out>;
>> +                };
>> +            };
>> +
>> +            port@1 {
>> +                reg = <1>;
>> +                dp_out: endpoint {
> 
> Can drop the label and show some remote-endpoint connection?
> 

I am not sure what should I include here as connection. IIRC, Robh 
suggested It should connect to the Type-C connector through some sort of 
mixing node. However, I see only typec connector node at: 
Documentation/devicetree/bindings/chrome/google,cros-ec-typec.yaml. 
Should we use that directly or should we use ec node?

Can we do something like:

remote-endpoint = <&typec> or <&ec>

Rest of the comments looks good and I will address them in next patch.

Thanks.

>> +                };
>> +            };
>> +        };
>> +      };
>> +    };
>> +...