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@xxxxxxxxxxxxxx/
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@xxxxxxxxxxxxxx>
+ - Vara Reddy <varar@xxxxxxxxxxxxxx>
+ - Tanmay Shah <tanmay@xxxxxxxxxxxxxx>
+
+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.
+ };
+ };
+ };
+ };
+ };
+...
