[DPU,v5,1/5] dt-bindings: msm/dp: add bindings of DP/DP-PLL driver for Snapdragon

From: Chandan Uddaraju <chandanu@codeaurora.org>

Add bindings for Snapdragon DisplayPort and
display-port PLL driver.

Changes in V2:
Provide details about sel-gpio

Changes in V4:
Provide details about max dp lanes
Change the commit text

Changes in V5:
Moved dp.txt to yaml file.

Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
Signed-off-by: Vara Reddy <varar@codeaurora.org>
---
 .../devicetree/bindings/display/msm/dp-sc7180.yaml | 325 +++++++++++++++++++++
 .../devicetree/bindings/display/msm/dpu.txt        |  16 +-
 2 files changed, 337 insertions(+), 4 deletions(-)
 create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml

Hi Tanmay


Reviewing the yaml bindings triggered a few comments. See below.

	Sam

On Tue, Mar 31, 2020 at 05:30:27PM -0700, Tanmay Shah wrote:
> From: Chandan Uddaraju <chandanu@codeaurora.org>
> 
> Add bindings for Snapdragon DisplayPort and
> display-port PLL driver.
> 
> Changes in V2:
> Provide details about sel-gpio
> 
> Changes in V4:
> Provide details about max dp lanes
> Change the commit text
> 
> Changes in V5:
> Moved dp.txt to yaml file.
> 
> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
> Signed-off-by: Vara Reddy <varar@codeaurora.org>

As you handle the patch, thus the patch passed throgh you, you are
supposed to sign-off the patch.


The changes to dpu.txt is not explained in the changelog.


> ---
>  .../devicetree/bindings/display/msm/dp-sc7180.yaml | 325 +++++++++++++++++++++
>  .../devicetree/bindings/display/msm/dpu.txt        |  16 +-
>  2 files changed, 337 insertions(+), 4 deletions(-)
>  create mode 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> 
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 0000000..761a01d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> @@ -0,0 +1,325 @@
> +# SPDX-License-Identifier: GPL-2.0-only
For new bindings please use: (GPL-2.0-only OR BSD-2-Clause)


> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Description of Qualcomm Display Port dt properties.
> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  "msm_dp":
The quotes seems not necessary.
This describes the name of the node.
The typical way to identify a node is using a compatible.

So I think that the right solution here is to drop "msm_dp".

> +    type: object
> +    description: |
> +      Node containing Display port register address bases, clocks, power supplies.
> +

And start here.
> +    properties:
> +     compatible:
> +       items:
> +         - const: qcom,dp-display
> +
> +     cell-index:
> +       description: Specifies the controller instance.
> +
> +     reg:
> +       description: Physical base address and length of controller's registers.
This description is generic and can be omitted.
But it would be good with a descrition of the individual registers like
this:

    reg:
      items:
        - description: AHB bla bla
	- description: aux bla bla

> +
> +     reg-names:
> +       description: |
> +         Names for different register regions defined above. The required region
> +         is mentioned below.
> +       items:
> +         - const: dp_ahb
> +         - const: dp_aux
> +         - const: dp_link
> +         - const: dp_p0
> +         - const: dp_phy
> +         - const: dp_ln_tx0
> +         - const: dp_ln_tx1
> +         - const: afprom_physical
> +         - const: dp_pll
> +         - const: usb3_dp_com
> +         - const: hdcp_physical
> +
> +     interrupts:
> +       description: The interrupt signal from the DP block.
> +
> +     clocks:
> +       description: List of clock specifiers for clocks needed by the device.
          items:
	    - description: aux clock bla bla
	    - description: ref clock bla bla


> +
> +     clock-names:
> +       description: |
> +         Device clock names in the same order as mentioned in clocks property.
> +         The required clocks are mentioned below.
> +       items:
> +         - const: core_aux_clk
> +         - const: core_ref_clk_src
> +         - const: core_usb_ref_clk
> +         - const: core_usb_cfg_ahb_clk
> +         - const: core_usb_pipe_clk
> +         - const: ctrl_link_clk
> +         - const: ctrl_link_iface_clk
> +         - const: ctrl_pixel_clk
> +         - const: crypto_clk
> +         - const: pixel_clk_rcg
> +
> +     pll-node:
> +       description: phandle to DP PLL node.
Add type (phandle)

> +
> +     vdda-1p2-supply:
> +       description: phandle to vdda 1.2V regulator node.
> +
> +     vdda-0p9-supply:
> +       description: phandle to vdda 0.9V regulator node.
> +
> +     aux-cfg0-settings:
> +       description: |
> +         Specifies the DP AUX configuration 0 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
Add type, goes for all *-settings


> +
> +     aux-cfg1-settings:
> +       description: |
> +         Specifies the DP AUX configuration 1 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg2-settings:
> +       description: |
> +         Specifies the DP AUX configuration 2 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg3-settings:
> +       description: |
> +         Specifies the DP AUX configuration 3 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg4-settings:
> +       description: |
> +         Specifies the DP AUX configuration 4 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg5-settings:
> +       description: |
> +         Specifies the DP AUX configuration 5 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg6-settings:
> +       description: |
> +         Specifies the DP AUX configuration 6 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg7-settings:
> +       description: |
> +         Specifies the DP AUX configuration 7 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg8-settings:
> +       description: |
> +         Specifies the DP AUX configuration 8 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg9-settings:
> +       description: |
> +         Specifies the DP AUX configuration 9 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     max-pclk-frequency-khz:
> +       description: Maximum displayport pixel clock supported for the chipset.
> +
> +     data-lanes:
> +       description: Maximum number of lanes that can be used for Display port.
> +
> +     usbplug-cc-gpio:
> +       maxItems: 1
> +       description: Specifies the usbplug orientation gpio.
Shall be named -gpios. Goes for all -gpio properties.
maxItems: 1 is good. Keep it.

> +
> +     aux-en-gpio:
> +       maxItems: 1
> +       description: Specifies the aux-channel enable gpio.
> +
> +     aux-sel-gpio:
> +       maxItems: 1
> +       description: Specifies the sux-channel select gpio.
> +
> +     ports:
> +       description: |
> +         Contains display port controller endpoint subnode.
> +         remote-endpoint: |
> +           For port@0, set to phandle of the connected panel/bridge's
> +           input endpoint. For port@1, set to the DPU interface output.
> +           Documentation/devicetree/bindings/graph.txt and
> +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +  "dp_pll":
quotes should not be required here.

I looks like yo try to describe two differents nodes in the same file.
Consider to split in two files.

Some of the comments from above applies here too.

> +     type: object
> +     description: Node contains properties of Display port pll and phy driver.
> +
> +     properties:
> +       compatible:
> +         items:
> +           - const: qcom,dp-pll-10nm
> +
> +       cell-index:
> +         description: Specifies the controller instance.
> +
> +       reg:
> +         description: Physical base address and length of DP phy and pll registers.
> +
> +       reg-names:
> +         description: |
> +           Names for different register regions defined above. The required region
> +           is mentioned below.
> +         items:
> +           - const: pll_base
> +           - const: phy_base
> +           - const: ln_tx0_base
> +           - const: ln_tx1_base
> +           - const: gdsc_base
> +
> +       clocks:
> +         description: List of clock specifiers for clocks needed by the device.
> +
> +       clock-names:
> +         description: |
> +           Device clock names in the same order as mentioned in clocks property.
> +           The required clocks are mentioned below.
> +         items:
> +           - const: iface
> +           - const: ref
> +           - const: cfg_ahb
> +           - const: pipe
> +
> +examples:

4 spaces as indent - good.
You have include files - good.


> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    msm_dp: displayport-controller@ae90000{
> +                cell-index = <0>;
indent only four spaces

> +        compatible = "qcom,dp-display";
> +        reg =   <0 0xae90000 0 0x200>,
Only one space after '=' - rememebr to adjust indent in following lines.
> +                <0 0xae90200 0 0x200>,
> +                <0 0xae90400 0 0xc00>,
> +                <0 0xae91000 0 0x400>,
> +                <0 0x88eaa00 0 0x200>,
> +                <0 0x88ea200 0 0x200>,
> +                <0 0x88ea600 0 0x200>,
> +                <0 0x780000 0 0x6228>,
> +                <0 0x088ea000 0 0x40>,
> +                <0 0x88e8000 0 0x20>,
> +                <0 0x0aee1000 0 0x034>;
> +        reg-names = "dp_ahb", "dp_aux", "dp_link",
> +            "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
> +            "qfprom_physical", "dp_pll",
> +            "usb3_dp_com", "hdcp_physical";

Indent so names in following lines starts where names in previous lines
starts.
Like this:
        reg-names = "dp_ahb", "dp_aux", "dp_link",
                    "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
                    "qfprom_physical", "dp_pll",
                    "usb3_dp_com", "hdcp_physical";


> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
> +            <&rpmhcc RPMH_CXO_CLK>,
Indent so '<' are aligned under each other. Like done above for reg =

> +            <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> +            <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +            <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_CRYPTO_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux_clk", "core_ref_clk_src",
> +            "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
> +            "core_usb_pipe_clk", "ctrl_link_clk",
> +            "ctrl_link_iface_clk", "ctrl_pixel_clk",
> +            "crypto_clk", "pixel_clk_rcg";
Fix indent

> +
> +        pll-node = <&dp_pll>;
> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        aux-cfg0-settings = [20 00];
> +        aux-cfg1-settings = [24 13 23 1d];
> +        aux-cfg2-settings = [28 24];
> +        aux-cfg3-settings = [2c 00];
> +        aux-cfg4-settings = [30 0a];
> +        aux-cfg5-settings = [34 26];
> +        aux-cfg6-settings = [38 0a];
> +        aux-cfg7-settings = [3c 03];
> +        aux-cfg8-settings = [40 bb];
> +        aux-cfg9-settings = [44 03];
> +
> +        max-pclk-frequency-khz = <67500>;
> +        data-lanes = <2>;
> +
> +        aux-en-gpio = <&msmgpio 55 1>;
> +        aux-sel-gpio = <&msmgpio 110 1>;
> +        usbplug-cc-gpio = <&msmgpio 90 1>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                dp_in: endpoint {
> +                    remote-endpoint = <&dpu_intf0_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                dp_out: endpoint {
> +                };
> +            };
> +        };
> +    };
> +
> +    dp_pll: dp-pll@088ea000 {
> +        compatible = "qcom,dp-pll-10nm";
> +        label = "DP PLL";
> +        cell-index = <0>;
> +        #clock-cells = <1>;
> +
> +        reg = <0 0x088ea000 0 0x200>,
> +              <0 0x088eaa00 0 0x200>,
> +              <0 0x088ea200 0 0x200>,
> +              <0 0x088ea600 0 0x200>,
> +              <0 0x08803000 0 0x8>;
> +        reg-names = "pll_base", "phy_base", "ln_tx0_base",
> +            "ln_tx1_base", "gdsc_base";
> +
> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +             <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> +             <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +             <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
> +        clock-names = "iface_clk", "ref_clk",
> +            "cfg_ahb_clk", "pipe_clk";
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26..7e99e45 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -63,8 +63,9 @@ Required properties:
>  	Documentation/devicetree/bindings/graph.txt
>  	Documentation/devicetree/bindings/media/video-interfaces.txt
>  
> -	Port 0 -> DPU_INTF1 (DSI1)
> -	Port 1 -> DPU_INTF2 (DSI2)
> +	Port 0 -> DPU_INTF0 (DP)
> +	Port 1 -> DPU_INTF1 (DSI1)
> +	Port 2 -> DPU_INTF2 (DSI2)
>  
>  Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate assignment
> @@ -125,13 +126,20 @@ Example:
>  
>  				port@0 {
>  					reg = <0>;
> -					dpu_intf1_out: endpoint {
> -						remote-endpoint = <&dsi0_in>;
> +					dpu_intf0_out: endpoint {
> +						remote-endpoint = <&dp_in>;
>  					};
>  				};
>  
>  				port@1 {
>  					reg = <1>;
> +					dpu_intf1_out: endpoint {
> +						remote-endpoint = <&dsi0_in>;
> +					};
> +				};
> +
> +				port@2 {
> +					reg = <2>;
>  					dpu_intf2_out: endpoint {
>  						remote-endpoint = <&dsi1_in>;
>  					};
> -- 
> 1.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Hi Sam,

Thanks for reviews.

The changes were posted by Vara Reddy. Due to some configuration errors,
changes were posted with my E-mail ID. Vara will be addressing comments, and
we will take care of this error with next patchset.

Thanks,
Tanmay

-----Original Message-----
From: Sam Ravnborg <sam@ravnborg.org> 
Sent: Tuesday, March 31, 2020 10:50 PM
To: Tanmay Shah <tanmay@codeaurora.org>
Cc: freedreno@lists.freedesktop.org; linux-arm-msm@vger.kernel.org;
devicetree@vger.kernel.org; seanpaul@chromium.org; swboyd@chromium.org;
abhinavk@codeaurora.org; hoegsberg@google.com;
dri-devel@lists.freedesktop.org; Vara Reddy <varar@codeaurora.org>;
aravindh@codeaurora.org; linux-clk@vger.kernel.org; Chandan Uddaraju
<chandanu@codeaurora.org>
Subject: Re: [DPU PATCH v5 1/5] dt-bindings: msm/dp: add bindings of
DP/DP-PLL driver for Snapdragon

Hi Tanmay


Reviewing the yaml bindings triggered a few comments. See below.

	Sam

On Tue, Mar 31, 2020 at 05:30:27PM -0700, Tanmay Shah wrote:
> From: Chandan Uddaraju <chandanu@codeaurora.org>
> 
> Add bindings for Snapdragon DisplayPort and display-port PLL driver.
> 
> Changes in V2:
> Provide details about sel-gpio
> 
> Changes in V4:
> Provide details about max dp lanes
> Change the commit text
> 
> Changes in V5:
> Moved dp.txt to yaml file.
> 
> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
> Signed-off-by: Vara Reddy <varar@codeaurora.org>

As you handle the patch, thus the patch passed throgh you, you are supposed
to sign-off the patch.


The changes to dpu.txt is not explained in the changelog.


> ---
>  .../devicetree/bindings/display/msm/dp-sc7180.yaml | 325
+++++++++++++++++++++
>  .../devicetree/bindings/display/msm/dpu.txt        |  16 +-
>  2 files changed, 337 insertions(+), 4 deletions(-)  create mode 
> 100644 Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> 
> diff --git 
> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml 
> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 0000000..761a01d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> @@ -0,0 +1,325 @@
> +# SPDX-License-Identifier: GPL-2.0-only
For new bindings please use: (GPL-2.0-only OR BSD-2-Clause)


> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Description of Qualcomm Display Port dt properties.
> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host 
> +controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  "msm_dp":
The quotes seems not necessary.
This describes the name of the node.
The typical way to identify a node is using a compatible.

So I think that the right solution here is to drop "msm_dp".

> +    type: object
> +    description: |
> +      Node containing Display port register address bases, clocks, power
supplies.
> +

And start here.
> +    properties:
> +     compatible:
> +       items:
> +         - const: qcom,dp-display
> +
> +     cell-index:
> +       description: Specifies the controller instance.
> +
> +     reg:
> +       description: Physical base address and length of controller's
registers.
This description is generic and can be omitted.
But it would be good with a descrition of the individual registers like
this:

    reg:
      items:
        - description: AHB bla bla
	- description: aux bla bla

> +
> +     reg-names:
> +       description: |
> +         Names for different register regions defined above. The required
region
> +         is mentioned below.
> +       items:
> +         - const: dp_ahb
> +         - const: dp_aux
> +         - const: dp_link
> +         - const: dp_p0
> +         - const: dp_phy
> +         - const: dp_ln_tx0
> +         - const: dp_ln_tx1
> +         - const: afprom_physical
> +         - const: dp_pll
> +         - const: usb3_dp_com
> +         - const: hdcp_physical
> +
> +     interrupts:
> +       description: The interrupt signal from the DP block.
> +
> +     clocks:
> +       description: List of clock specifiers for clocks needed by the
device.
          items:
	    - description: aux clock bla bla
	    - description: ref clock bla bla


> +
> +     clock-names:
> +       description: |
> +         Device clock names in the same order as mentioned in clocks
property.
> +         The required clocks are mentioned below.
> +       items:
> +         - const: core_aux_clk
> +         - const: core_ref_clk_src
> +         - const: core_usb_ref_clk
> +         - const: core_usb_cfg_ahb_clk
> +         - const: core_usb_pipe_clk
> +         - const: ctrl_link_clk
> +         - const: ctrl_link_iface_clk
> +         - const: ctrl_pixel_clk
> +         - const: crypto_clk
> +         - const: pixel_clk_rcg
> +
> +     pll-node:
> +       description: phandle to DP PLL node.
Add type (phandle)

> +
> +     vdda-1p2-supply:
> +       description: phandle to vdda 1.2V regulator node.
> +
> +     vdda-0p9-supply:
> +       description: phandle to vdda 0.9V regulator node.
> +
> +     aux-cfg0-settings:
> +       description: |
> +         Specifies the DP AUX configuration 0 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
Add type, goes for all *-settings


> +
> +     aux-cfg1-settings:
> +       description: |
> +         Specifies the DP AUX configuration 1 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg2-settings:
> +       description: |
> +         Specifies the DP AUX configuration 2 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg3-settings:
> +       description: |
> +         Specifies the DP AUX configuration 3 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg4-settings:
> +       description: |
> +         Specifies the DP AUX configuration 4 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg5-settings:
> +       description: |
> +         Specifies the DP AUX configuration 5 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg6-settings:
> +       description: |
> +         Specifies the DP AUX configuration 6 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg7-settings:
> +       description: |
> +         Specifies the DP AUX configuration 7 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg8-settings:
> +       description: |
> +         Specifies the DP AUX configuration 8 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg9-settings:
> +       description: |
> +         Specifies the DP AUX configuration 9 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     max-pclk-frequency-khz:
> +       description: Maximum displayport pixel clock supported for the
chipset.
> +
> +     data-lanes:
> +       description: Maximum number of lanes that can be used for Display
port.
> +
> +     usbplug-cc-gpio:
> +       maxItems: 1
> +       description: Specifies the usbplug orientation gpio.
Shall be named -gpios. Goes for all -gpio properties.
maxItems: 1 is good. Keep it.

> +
> +     aux-en-gpio:
> +       maxItems: 1
> +       description: Specifies the aux-channel enable gpio.
> +
> +     aux-sel-gpio:
> +       maxItems: 1
> +       description: Specifies the sux-channel select gpio.
> +
> +     ports:
> +       description: |
> +         Contains display port controller endpoint subnode.
> +         remote-endpoint: |
> +           For port@0, set to phandle of the connected panel/bridge's
> +           input endpoint. For port@1, set to the DPU interface output.
> +           Documentation/devicetree/bindings/graph.txt and
> +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +  "dp_pll":
quotes should not be required here.

I looks like yo try to describe two differents nodes in the same file.
Consider to split in two files.

Some of the comments from above applies here too.

> +     type: object
> +     description: Node contains properties of Display port pll and phy
driver.
> +
> +     properties:
> +       compatible:
> +         items:
> +           - const: qcom,dp-pll-10nm
> +
> +       cell-index:
> +         description: Specifies the controller instance.
> +
> +       reg:
> +         description: Physical base address and length of DP phy and pll
registers.
> +
> +       reg-names:
> +         description: |
> +           Names for different register regions defined above. The
required region
> +           is mentioned below.
> +         items:
> +           - const: pll_base
> +           - const: phy_base
> +           - const: ln_tx0_base
> +           - const: ln_tx1_base
> +           - const: gdsc_base
> +
> +       clocks:
> +         description: List of clock specifiers for clocks needed by the
device.
> +
> +       clock-names:
> +         description: |
> +           Device clock names in the same order as mentioned in clocks
property.
> +           The required clocks are mentioned below.
> +         items:
> +           - const: iface
> +           - const: ref
> +           - const: cfg_ahb
> +           - const: pipe
> +
> +examples:

4 spaces as indent - good.
You have include files - good.


> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    msm_dp: displayport-controller@ae90000{
> +                cell-index = <0>;
indent only four spaces

> +        compatible = "qcom,dp-display";
> +        reg =   <0 0xae90000 0 0x200>,
Only one space after '=' - rememebr to adjust indent in following lines.
> +                <0 0xae90200 0 0x200>,
> +                <0 0xae90400 0 0xc00>,
> +                <0 0xae91000 0 0x400>,
> +                <0 0x88eaa00 0 0x200>,
> +                <0 0x88ea200 0 0x200>,
> +                <0 0x88ea600 0 0x200>,
> +                <0 0x780000 0 0x6228>,
> +                <0 0x088ea000 0 0x40>,
> +                <0 0x88e8000 0 0x20>,
> +                <0 0x0aee1000 0 0x034>;
> +        reg-names = "dp_ahb", "dp_aux", "dp_link",
> +            "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
> +            "qfprom_physical", "dp_pll",
> +            "usb3_dp_com", "hdcp_physical";

Indent so names in following lines starts where names in previous lines
starts.
Like this:
        reg-names = "dp_ahb", "dp_aux", "dp_link",
                    "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
                    "qfprom_physical", "dp_pll",
                    "usb3_dp_com", "hdcp_physical";


> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
> +            <&rpmhcc RPMH_CXO_CLK>,
Indent so '<' are aligned under each other. Like done above for reg =

> +            <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> +            <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +            <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_CRYPTO_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux_clk", "core_ref_clk_src",
> +            "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
> +            "core_usb_pipe_clk", "ctrl_link_clk",
> +            "ctrl_link_iface_clk", "ctrl_pixel_clk",
> +            "crypto_clk", "pixel_clk_rcg";
Fix indent

> +
> +        pll-node = <&dp_pll>;
> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        aux-cfg0-settings = [20 00];
> +        aux-cfg1-settings = [24 13 23 1d];
> +        aux-cfg2-settings = [28 24];
> +        aux-cfg3-settings = [2c 00];
> +        aux-cfg4-settings = [30 0a];
> +        aux-cfg5-settings = [34 26];
> +        aux-cfg6-settings = [38 0a];
> +        aux-cfg7-settings = [3c 03];
> +        aux-cfg8-settings = [40 bb];
> +        aux-cfg9-settings = [44 03];
> +
> +        max-pclk-frequency-khz = <67500>;
> +        data-lanes = <2>;
> +
> +        aux-en-gpio = <&msmgpio 55 1>;
> +        aux-sel-gpio = <&msmgpio 110 1>;
> +        usbplug-cc-gpio = <&msmgpio 90 1>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                dp_in: endpoint {
> +                    remote-endpoint = <&dpu_intf0_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                dp_out: endpoint {
> +                };
> +            };
> +        };
> +    };
> +
> +    dp_pll: dp-pll@088ea000 {
> +        compatible = "qcom,dp-pll-10nm";
> +        label = "DP PLL";
> +        cell-index = <0>;
> +        #clock-cells = <1>;
> +
> +        reg = <0 0x088ea000 0 0x200>,
> +              <0 0x088eaa00 0 0x200>,
> +              <0 0x088ea200 0 0x200>,
> +              <0 0x088ea600 0 0x200>,
> +              <0 0x08803000 0 0x8>;
> +        reg-names = "pll_base", "phy_base", "ln_tx0_base",
> +            "ln_tx1_base", "gdsc_base";
> +
> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +             <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> +             <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +             <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
> +        clock-names = "iface_clk", "ref_clk",
> +            "cfg_ahb_clk", "pipe_clk";
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt 
> b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26..7e99e45 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -63,8 +63,9 @@ Required properties:
>  	Documentation/devicetree/bindings/graph.txt
>  	Documentation/devicetree/bindings/media/video-interfaces.txt
>  
> -	Port 0 -> DPU_INTF1 (DSI1)
> -	Port 1 -> DPU_INTF2 (DSI2)
> +	Port 0 -> DPU_INTF0 (DP)
> +	Port 1 -> DPU_INTF1 (DSI1)
> +	Port 2 -> DPU_INTF2 (DSI2)
>  
>  Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate 
> assignment @@ -125,13 +126,20 @@ Example:
>  
>  				port@0 {
>  					reg = <0>;
> -					dpu_intf1_out: endpoint {
> -						remote-endpoint =
<&dsi0_in>;
> +					dpu_intf0_out: endpoint {
> +						remote-endpoint = <&dp_in>;
>  					};
>  				};
>  
>  				port@1 {
>  					reg = <1>;
> +					dpu_intf1_out: endpoint {
> +						remote-endpoint =
<&dsi0_in>;
> +					};
> +				};
> +
> +				port@2 {
> +					reg = <2>;
>  					dpu_intf2_out: endpoint {
>  						remote-endpoint =
<&dsi1_in>;
>  					};
> --
> 1.9.1
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Quoting Tanmay Shah (2020-03-31 17:30:27)
> diff --git a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> new file mode 100644
> index 0000000..761a01d
> --- /dev/null
> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
> @@ -0,0 +1,325 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +%YAML 1.2
> +---
> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
> +$schema: http://devicetree.org/meta-schemas/core.yaml#
> +
> +title: Description of Qualcomm Display Port dt properties.

This title should be something like

"Qualcomm Display Port Controller"

> +
> +maintainers:
> +  - Chandan Uddaraju <chandanu@codeaurora.org>
> +  - Vara Reddy <varar@codeaurora.org>
> +
> +description: |
> +  Device tree bindings for MSM Display Port which supports DP host controllers
> +  that are compatible with VESA Display Port interface specification.
> +
> +properties:
> +  "msm_dp":
> +    type: object
> +    description: |
> +      Node containing Display port register address bases, clocks, power supplies.
> +
> +    properties:
> +     compatible:
> +       items:
> +         - const: qcom,dp-display
> +
> +     cell-index:
> +       description: Specifies the controller instance.
> +
> +     reg:
> +       description: Physical base address and length of controller's registers.
> +
> +     reg-names:
> +       description: |
> +         Names for different register regions defined above. The required region
> +         is mentioned below.
> +       items:
> +         - const: dp_ahb
> +         - const: dp_aux
> +         - const: dp_link
> +         - const: dp_p0
> +         - const: dp_phy
> +         - const: dp_ln_tx0
> +         - const: dp_ln_tx1
> +         - const: afprom_physical
> +         - const: dp_pll
> +         - const: usb3_dp_com
> +         - const: hdcp_physical
> +
> +     interrupts:
> +       description: The interrupt signal from the DP block.
> +
> +     clocks:
> +       description: List of clock specifiers for clocks needed by the device.
> +
> +     clock-names:
> +       description: |
> +         Device clock names in the same order as mentioned in clocks property.
> +         The required clocks are mentioned below.
> +       items:
> +         - const: core_aux_clk
> +         - const: core_ref_clk_src
> +         - const: core_usb_ref_clk
> +         - const: core_usb_cfg_ahb_clk
> +         - const: core_usb_pipe_clk
> +         - const: ctrl_link_clk
> +         - const: ctrl_link_iface_clk
> +         - const: ctrl_pixel_clk
> +         - const: crypto_clk
> +         - const: pixel_clk_rcg
> +
> +     pll-node:
> +       description: phandle to DP PLL node.
> +
> +     vdda-1p2-supply:
> +       description: phandle to vdda 1.2V regulator node.
> +
> +     vdda-0p9-supply:
> +       description: phandle to vdda 0.9V regulator node.
> +
> +     aux-cfg0-settings:
> +       description: |
> +         Specifies the DP AUX configuration 0 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg1-settings:
> +       description: |
> +         Specifies the DP AUX configuration 1 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg2-settings:
> +       description: |
> +         Specifies the DP AUX configuration 2 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg3-settings:
> +       description: |
> +         Specifies the DP AUX configuration 3 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg4-settings:
> +       description: |
> +         Specifies the DP AUX configuration 4 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg5-settings:
> +       description: |
> +         Specifies the DP AUX configuration 5 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg6-settings:
> +       description: |
> +         Specifies the DP AUX configuration 6 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg7-settings:
> +       description: |
> +         Specifies the DP AUX configuration 7 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg8-settings:
> +       description: |
> +         Specifies the DP AUX configuration 8 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     aux-cfg9-settings:
> +       description: |
> +         Specifies the DP AUX configuration 9 settings.
> +         The first entry in this array corresponds to the register offset
> +         within DP AUX, while the remaining entries indicate the
> +         programmable values.
> +
> +     max-pclk-frequency-khz:
> +       description: Maximum displayport pixel clock supported for the chipset.
> +
> +     data-lanes:
> +       description: Maximum number of lanes that can be used for Display port.

This should be an array of cells, not a single cell indicating the
number of lanes.

> +
> +     usbplug-cc-gpio:
> +       maxItems: 1
> +       description: Specifies the usbplug orientation gpio.
> +
> +     aux-en-gpio:
> +       maxItems: 1
> +       description: Specifies the aux-channel enable gpio.
> +
> +     aux-sel-gpio:
> +       maxItems: 1
> +       description: Specifies the sux-channel select gpio.
> +
> +     ports:
> +       description: |
> +         Contains display port controller endpoint subnode.
> +         remote-endpoint: |
> +           For port@0, set to phandle of the connected panel/bridge's
> +           input endpoint. For port@1, set to the DPU interface output.
> +           Documentation/devicetree/bindings/graph.txt and
> +           Documentation/devicetree/bindings/media/video-interfaces.txt.
> +
> +  "dp_pll":
> +     type: object
> +     description: Node contains properties of Display port pll and phy driver.
> +
> +     properties:
> +       compatible:
> +         items:
> +           - const: qcom,dp-pll-10nm
> +
> +       cell-index:
> +         description: Specifies the controller instance.
> +
> +       reg:
> +         description: Physical base address and length of DP phy and pll registers.
> +
> +       reg-names:
> +         description: |
> +           Names for different register regions defined above. The required region
> +           is mentioned below.
> +         items:
> +           - const: pll_base
> +           - const: phy_base
> +           - const: ln_tx0_base
> +           - const: ln_tx1_base
> +           - const: gdsc_base
> +
> +       clocks:
> +         description: List of clock specifiers for clocks needed by the device.
> +
> +       clock-names:
> +         description: |
> +           Device clock names in the same order as mentioned in clocks property.
> +           The required clocks are mentioned below.
> +         items:
> +           - const: iface
> +           - const: ref
> +           - const: cfg_ahb
> +           - const: pipe
> +
> +examples:
> +  - |
> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
> +    #include <dt-bindings/clock/qcom,rpmh.h>
> +    #include <dt-bindings/gpio/gpio.h>
> +    msm_dp: displayport-controller@ae90000{
> +                cell-index = <0>;
> +        compatible = "qcom,dp-display";
> +        reg =   <0 0xae90000 0 0x200>,
> +                <0 0xae90200 0 0x200>,
> +                <0 0xae90400 0 0xc00>,
> +                <0 0xae91000 0 0x400>,
> +                <0 0x88eaa00 0 0x200>,
> +                <0 0x88ea200 0 0x200>,
> +                <0 0x88ea600 0 0x200>,
> +                <0 0x780000 0 0x6228>,
> +                <0 0x088ea000 0 0x40>,
> +                <0 0x88e8000 0 0x20>,
> +                <0 0x0aee1000 0 0x034>;

This needs to be split up into at least two nodes. Any address above
that starts in 88e needs to be put into a new node underneath the qmp
phy. That is the "DP PHY" that lives in the power domain of the USB+DP
combo phy. The qfprom_physical reg property should be removed from here
and this binding should use the nvmem binding to reach into the qfprom
to read out things (I guess there's some sort of HDCP key in the
qfprom?).

After that I don't know why there are so many different reg properties
for the DP controller here and why it needs to be split up.  It looks
like we should just map the register space from 0xae90000 up to
0xae91400 as one big register region and have the driver figure out how
to operate on top of that. If it changes between SoC versions then we
should have a more specific compatible that tells us what registers are
in what place.

> +        reg-names = "dp_ahb", "dp_aux", "dp_link",
> +            "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
> +            "qfprom_physical", "dp_pll",
> +            "usb3_dp_com", "hdcp_physical";
> +
> +        interrupt-parent = <&display_subsystem>;
> +        interrupts = <12 0>;
> +
> +        clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
> +            <&rpmhcc RPMH_CXO_CLK>,
> +            <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> +            <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +            <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_CRYPTO_CLK>,
> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
> +        clock-names = "core_aux_clk", "core_ref_clk_src",
> +            "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
> +            "core_usb_pipe_clk", "ctrl_link_clk",
> +            "ctrl_link_iface_clk", "ctrl_pixel_clk",
> +            "crypto_clk", "pixel_clk_rcg";
> +
> +        pll-node = <&dp_pll>;

If the DP PLL and DP controller need to be controlled from two software
entities, it may make sense to just combine that DP PLL into the
controller node and have this node be a clk provider. The pll-node
property is pretty ugly and should be removed.

> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
> +
> +        aux-cfg0-settings = [20 00];
> +        aux-cfg1-settings = [24 13 23 1d];
> +        aux-cfg2-settings = [28 24];
> +        aux-cfg3-settings = [2c 00];
> +        aux-cfg4-settings = [30 0a];
> +        aux-cfg5-settings = [34 26];
> +        aux-cfg6-settings = [38 0a];
> +        aux-cfg7-settings = [3c 03];
> +        aux-cfg8-settings = [40 bb];
> +        aux-cfg9-settings = [44 03];

This pile of properties is board specific configuration tuning or
something? Can this go into the driver? Or can it be made more human
readable? I seem to recall that the USB phy had similar properties and
we made them into human readable properties when board integrators
needed to change them. The easiest approach there is to put everything
in the driver for now and then when something has to change for a board
it gets punted out to the DT and that overrides the "default" settings
that are used in the driver.

> +
> +        max-pclk-frequency-khz = <67500>;

What is this? Why isn't this in the driver?

> +        data-lanes = <2>;
> +
> +        aux-en-gpio = <&msmgpio 55 1>;
> +        aux-sel-gpio = <&msmgpio 110 1>;
> +        usbplug-cc-gpio = <&msmgpio 90 1>;
> +
> +        ports {
> +            #address-cells = <1>;
> +            #size-cells = <0>;
> +
> +            port@0 {
> +                reg = <0>;
> +                dp_in: endpoint {
> +                    remote-endpoint = <&dpu_intf0_out>;
> +                };
> +            };
> +
> +            port@1 {
> +                reg = <1>;
> +                dp_out: endpoint {
> +                };
> +            };
> +        };
> +    };
> +
> +    dp_pll: dp-pll@088ea000 {
> +        compatible = "qcom,dp-pll-10nm";
> +        label = "DP PLL";
> +        cell-index = <0>;
> +        #clock-cells = <1>;
> +
> +        reg = <0 0x088ea000 0 0x200>,
> +              <0 0x088eaa00 0 0x200>,
> +              <0 0x088ea200 0 0x200>,
> +              <0 0x088ea600 0 0x200>,
> +              <0 0x08803000 0 0x8>;
> +        reg-names = "pll_base", "phy_base", "ln_tx0_base",
> +            "ln_tx1_base", "gdsc_base";

I guess the DP_PLL lives inside the qmp combo phy? That would match how
the USB phy binding has been done there. This whole node should be
combined with the DP phy node that will be placed as a child of the qmp
phy wrapper (i.e. qcom,sc7180-qmp-usb3-phy compatible node). Might as
well change that compatible from qcom,sc7180-qmp-usb3-phy to be
qcom,sc7180-qmp-usb3-dp-phy too so that it can create the DP phy bits
too.

> +
> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
> +             <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
> +             <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
> +             <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
> +        clock-names = "iface_clk", "ref_clk",
> +            "cfg_ahb_clk", "pipe_clk";
> +    };
> +
> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt b/Documentation/devicetree/bindings/display/msm/dpu.txt
> index 551ae26..7e99e45 100644
> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
> @@ -63,8 +63,9 @@ Required properties:
>         Documentation/devicetree/bindings/graph.txt
>         Documentation/devicetree/bindings/media/video-interfaces.txt
>  
> -       Port 0 -> DPU_INTF1 (DSI1)
> -       Port 1 -> DPU_INTF2 (DSI2)
> +       Port 0 -> DPU_INTF0 (DP)
> +       Port 1 -> DPU_INTF1 (DSI1)
> +       Port 2 -> DPU_INTF2 (DSI2)

DP should come last so that the port mapping doesn't have to change.

>  
>  Optional properties:
>  - assigned-clocks: list of clock specifiers for clocks needing rate assignment

> 
> Hi Tanmay
> 
> 
> Reviewing the yaml bindings triggered a few comments. See below.
> 
> 	Sam
> 

Thanks for reviews Sam. Apologies for delayed response.
We are redesigning our driver according to upstream comments
and had to go through multiple discussions before we post v6.
Please find my comments in bindings according to new design.
> On Tue, Mar 31, 2020 at 05:30:27PM -0700, Tanmay Shah wrote:
>> From: Chandan Uddaraju <chandanu@codeaurora.org>
>> 
>> Add bindings for Snapdragon DisplayPort and
>> display-port PLL driver.
>> 
>> Changes in V2:
>> Provide details about sel-gpio
>> 
>> Changes in V4:
>> Provide details about max dp lanes
>> Change the commit text
>> 
>> Changes in V5:
>> Moved dp.txt to yaml file.
>> 
>> Signed-off-by: Chandan Uddaraju <chandanu@codeaurora.org>
>> Signed-off-by: Vara Reddy <varar@codeaurora.org>
> 
> As you handle the patch, thus the patch passed throgh you, you are
> supposed to sign-off the patch.
> 

Yes sure. Patch-v5 was sent using my email-id due to configuration 
error.
So I wasn't aware. However, I am taking care of Patch-v6 so I will 
sign-off in Patch-v6.
> 
> The changes to dpu.txt is not explained in the changelog.
> 
Explained same in commint message of v6.
> 
>> ---
>>  .../devicetree/bindings/display/msm/dp-sc7180.yaml | 325 
>> +++++++++++++++++++++
>>  .../devicetree/bindings/display/msm/dpu.txt        |  16 +-
>>  2 files changed, 337 insertions(+), 4 deletions(-)
>>  create mode 100644 
>> Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> 
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml 
>> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> new file mode 100644
>> index 0000000..761a01d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> @@ -0,0 +1,325 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
> For new bindings please use: (GPL-2.0-only OR BSD-2-Clause)
> 
Added BSD-2-Clause as License Identifier in v6.
> 
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Description of Qualcomm Display Port dt properties.
>> +
>> +maintainers:
>> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> +  - Vara Reddy <varar@codeaurora.org>
>> +
>> +description: |
>> +  Device tree bindings for MSM Display Port which supports DP host 
>> controllers
>> +  that are compatible with VESA Display Port interface specification.
>> +
>> +properties:
>> +  "msm_dp":
> The quotes seems not necessary.
> This describes the name of the node.
> The typical way to identify a node is using a compatible.
> 
> So I think that the right solution here is to drop "msm_dp".
> 
>> +    type: object
>> +    description: |
>> +      Node containing Display port register address bases, clocks, 
>> power supplies.
>> +
> 
> And start here.

Dropped msm_dp name, type, description and started properties from here
with indentation fixed.

>> +    properties:
>> +     compatible:
>> +       items:
>> +         - const: qcom,dp-display
>> +
>> +     cell-index:
>> +       description: Specifies the controller instance.
>> +
>> +     reg:
>> +       description: Physical base address and length of controller's 
>> registers.
> This description is generic and can be omitted.
Removed this description.
> But it would be good with a descrition of the individual registers like
> this:
> 
>     reg:
>       items:
>         - description: AHB bla bla
> 	- description: aux bla bla

Now DP is accessed as one big register region.
So added its description instead of individual modules.
> 
>> +
>> +     reg-names:
>> +       description: |
>> +         Names for different register regions defined above. The 
>> required region
>> +         is mentioned below.
>> +       items:
>> +         - const: dp_ahb
>> +         - const: dp_aux
>> +         - const: dp_link
>> +         - const: dp_p0
>> +         - const: dp_phy
>> +         - const: dp_ln_tx0
>> +         - const: dp_ln_tx1
>> +         - const: afprom_physical
>> +         - const: dp_pll
>> +         - const: usb3_dp_com
>> +         - const: hdcp_physical
>> +
>> +     interrupts:
>> +       description: The interrupt signal from the DP block.
>> +
>> +     clocks:
>> +       description: List of clock specifiers for clocks needed by the 
>> device.
>           items:
> 	    - description: aux clock bla bla
> 	    - description: ref clock bla bla

Removed all PLL, PHY and DP clocks.
Added description of of DP controller clocks as mentioned.
> 
> 
>> +
>> +     clock-names:
>> +       description: |
>> +         Device clock names in the same order as mentioned in clocks 
>> property.
>> +         The required clocks are mentioned below.
>> +       items:
>> +         - const: core_aux_clk
>> +         - const: core_ref_clk_src
>> +         - const: core_usb_ref_clk
>> +         - const: core_usb_cfg_ahb_clk
>> +         - const: core_usb_pipe_clk
>> +         - const: ctrl_link_clk
>> +         - const: ctrl_link_iface_clk
>> +         - const: ctrl_pixel_clk
>> +         - const: crypto_clk
>> +         - const: pixel_clk_rcg
>> +
>> +     pll-node:
>> +       description: phandle to DP PLL node.
> Add type (phandle)

According to new design, PLL/PHY will be accessed as part of DP driver.
So removed this node so comment Not Applicable now.
> 
>> +
>> +     vdda-1p2-supply:
>> +       description: phandle to vdda 1.2V regulator node.
>> +
>> +     vdda-0p9-supply:
>> +       description: phandle to vdda 0.9V regulator node.
>> +
>> +     aux-cfg0-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 0 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
> Add type, goes for all *-settings

Squashed all aux-cfg[0-9]-settings properties and description into one 
pattern Property.
Also these properties are optional now. Driver will be using default 
configuration
if property is not mentioned in dts.
> 
> 
>> +
>> +     aux-cfg1-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 1 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg2-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 2 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg3-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 3 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg4-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 4 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg5-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 5 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg6-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 6 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg7-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 7 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg8-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 8 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg9-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 9 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     max-pclk-frequency-khz:
>> +       description: Maximum displayport pixel clock supported for the 
>> chipset.
>> +
>> +     data-lanes:
>> +       description: Maximum number of lanes that can be used for 
>> Display port.
>> +
>> +     usbplug-cc-gpio:
>> +       maxItems: 1
>> +       description: Specifies the usbplug orientation gpio.
> Shall be named -gpios. Goes for all -gpio properties.
> maxItems: 1 is good. Keep it.
> 
GPIOs are redundant for now. so removed all gpio properties and
we will be adding them if required in future.
Comment Not Applicable now.

>> +
>> +     aux-en-gpio:
>> +       maxItems: 1
>> +       description: Specifies the aux-channel enable gpio.
>> +
>> +     aux-sel-gpio:
>> +       maxItems: 1
>> +       description: Specifies the sux-channel select gpio.
>> +
>> +     ports:
>> +       description: |
>> +         Contains display port controller endpoint subnode.
>> +         remote-endpoint: |
>> +           For port@0, set to phandle of the connected panel/bridge's
>> +           input endpoint. For port@1, set to the DPU interface 
>> output.
>> +           Documentation/devicetree/bindings/graph.txt and
>> +           
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +  "dp_pll":
> quotes should not be required here.
> 
> I looks like yo try to describe two differents nodes in the same file.
> Consider to split in two files.
> 
> Some of the comments from above applies here too.
> 
Now PLL related code will be part of DP driver,
so removed whole dp_pll node from bindings.
Comment Not Applicable now.
>> +     type: object
>> +     description: Node contains properties of Display port pll and 
>> phy driver.
>> +
>> +     properties:
>> +       compatible:
>> +         items:
>> +           - const: qcom,dp-pll-10nm
>> +
>> +       cell-index:
>> +         description: Specifies the controller instance.
>> +
>> +       reg:
>> +         description: Physical base address and length of DP phy and 
>> pll registers.
>> +
>> +       reg-names:
>> +         description: |
>> +           Names for different register regions defined above. The 
>> required region
>> +           is mentioned below.
>> +         items:
>> +           - const: pll_base
>> +           - const: phy_base
>> +           - const: ln_tx0_base
>> +           - const: ln_tx1_base
>> +           - const: gdsc_base
>> +
>> +       clocks:
>> +         description: List of clock specifiers for clocks needed by 
>> the device.
>> +
>> +       clock-names:
>> +         description: |
>> +           Device clock names in the same order as mentioned in 
>> clocks property.
>> +           The required clocks are mentioned below.
>> +         items:
>> +           - const: iface
>> +           - const: ref
>> +           - const: cfg_ahb
>> +           - const: pipe
>> +
>> +examples:
> 
> 4 spaces as indent - good.
> You have include files - good.
> 
> 
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> +    #include <dt-bindings/clock/qcom,rpmh.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    msm_dp: displayport-controller@ae90000{
>> +                cell-index = <0>;
> indent only four spaces
> 
Removed gpio.h and qcom,rpmh.h as they are redundant now.
Fixed indentation to four space.
>> +        compatible = "qcom,dp-display";
>> +        reg =   <0 0xae90000 0 0x200>,
> Only one space after '=' - rememebr to adjust indent in following 
> lines.
>> +                <0 0xae90200 0 0x200>,
>> +                <0 0xae90400 0 0xc00>,
>> +                <0 0xae91000 0 0x400>,
>> +                <0 0x88eaa00 0 0x200>,
>> +                <0 0x88ea200 0 0x200>,
>> +                <0 0x88ea600 0 0x200>,
>> +                <0 0x780000 0 0x6228>,
>> +                <0 0x088ea000 0 0x40>,
>> +                <0 0x88e8000 0 0x20>,
>> +                <0 0x0aee1000 0 0x034>;
>> +        reg-names = "dp_ahb", "dp_aux", "dp_link",
>> +            "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
>> +            "qfprom_physical", "dp_pll",
>> +            "usb3_dp_com", "hdcp_physical";
> 
> Indent so names in following lines starts where names in previous lines
> starts.
> Like this:
>         reg-names = "dp_ahb", "dp_aux", "dp_link",
>                     "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
>                     "qfprom_physical", "dp_pll",
>                     "usb3_dp_com", "hdcp_physical";
> 
> 
Now we have only one reg-name i.e "dp_controller" as we are accessing DP
as one big register region. so only one reg-name is there. So
indentation won't be required for each name.
>> +
>> +        interrupt-parent = <&display_subsystem>;
>> +        interrupts = <12 0>;
>> +
>> +        clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> +            <&rpmhcc RPMH_CXO_CLK>,
> Indent so '<' are aligned under each other. Like done above for reg =
> 
Fixed indentation for clocks. removed PHY and PLL clocks.
>> +            <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
>> +            <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
>> +            <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_CRYPTO_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +        clock-names = "core_aux_clk", "core_ref_clk_src",
>> +            "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
>> +            "core_usb_pipe_clk", "ctrl_link_clk",
>> +            "ctrl_link_iface_clk", "ctrl_pixel_clk",
>> +            "crypto_clk", "pixel_clk_rcg";
> Fix indent
> 
Fixed indentation for clock-names and started name after = .
Also removed redundant clk suffix from clock names.
>> +
>> +        pll-node = <&dp_pll>;
>> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
>> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
>> +
>> +        aux-cfg0-settings = [20 00];
>> +        aux-cfg1-settings = [24 13 23 1d];
>> +        aux-cfg2-settings = [28 24];
>> +        aux-cfg3-settings = [2c 00];
>> +        aux-cfg4-settings = [30 0a];
>> +        aux-cfg5-settings = [34 26];
>> +        aux-cfg6-settings = [38 0a];
>> +        aux-cfg7-settings = [3c 03];
>> +        aux-cfg8-settings = [40 bb];
>> +        aux-cfg9-settings = [44 03];
>> +
>> +        max-pclk-frequency-khz = <67500>;
>> +        data-lanes = <2>;
>> +
>> +        aux-en-gpio = <&msmgpio 55 1>;
>> +        aux-sel-gpio = <&msmgpio 110 1>;
>> +        usbplug-cc-gpio = <&msmgpio 90 1>;
>> +
>> +        ports {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            port@0 {
>> +                reg = <0>;
>> +                dp_in: endpoint {
>> +                    remote-endpoint = <&dpu_intf0_out>;
>> +                };
>> +            };
>> +
>> +            port@1 {
>> +                reg = <1>;
>> +                dp_out: endpoint {
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +    dp_pll: dp-pll@088ea000 {
>> +        compatible = "qcom,dp-pll-10nm";
>> +        label = "DP PLL";
>> +        cell-index = <0>;
>> +        #clock-cells = <1>;
>> +
>> +        reg = <0 0x088ea000 0 0x200>,
>> +              <0 0x088eaa00 0 0x200>,
>> +              <0 0x088ea200 0 0x200>,
>> +              <0 0x088ea600 0 0x200>,
>> +              <0 0x08803000 0 0x8>;
>> +        reg-names = "pll_base", "phy_base", "ln_tx0_base",
>> +            "ln_tx1_base", "gdsc_base";
>> +
>> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +             <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
>> +             <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
>> +             <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
>> +        clock-names = "iface_clk", "ref_clk",
>> +            "cfg_ahb_clk", "pipe_clk";
>> +    };
>> +
>> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt 
>> b/Documentation/devicetree/bindings/display/msm/dpu.txt
>> index 551ae26..7e99e45 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
>> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
>> @@ -63,8 +63,9 @@ Required properties:
>>  	Documentation/devicetree/bindings/graph.txt
>>  	Documentation/devicetree/bindings/media/video-interfaces.txt
>> 
>> -	Port 0 -> DPU_INTF1 (DSI1)
>> -	Port 1 -> DPU_INTF2 (DSI2)
>> +	Port 0 -> DPU_INTF0 (DP)
>> +	Port 1 -> DPU_INTF1 (DSI1)
>> +	Port 2 -> DPU_INTF2 (DSI2)
>> 
>>  Optional properties:
>>  - assigned-clocks: list of clock specifiers for clocks needing rate 
>> assignment
>> @@ -125,13 +126,20 @@ Example:
>> 
>>  				port@0 {
>>  					reg = <0>;
>> -					dpu_intf1_out: endpoint {
>> -						remote-endpoint = <&dsi0_in>;
>> +					dpu_intf0_out: endpoint {
>> +						remote-endpoint = <&dp_in>;
>>  					};
>>  				};
>> 
>>  				port@1 {
>>  					reg = <1>;
>> +					dpu_intf1_out: endpoint {
>> +						remote-endpoint = <&dsi0_in>;
>> +					};
>> +				};
>> +
>> +				port@2 {
>> +					reg = <2>;
>>  					dpu_intf2_out: endpoint {
>>  						remote-endpoint = <&dsi1_in>;
>>  					};
>> --
>> 1.9.1
>> _______________________________________________
>> dri-devel mailing list
>> dri-devel@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/dri-devel

Thanks for reviews Stephen. Please find my comments according to new 
design.

On 2020-04-23 16:41, Stephen Boyd wrote:
> Quoting Tanmay Shah (2020-03-31 17:30:27)
>> diff --git 
>> a/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> new file mode 100644
>> index 0000000..761a01d
>> --- /dev/null
>> +++ b/Documentation/devicetree/bindings/display/msm/dp-sc7180.yaml
>> @@ -0,0 +1,325 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +%YAML 1.2
>> +---
>> +$id: http://devicetree.org/schemas/display/msm/dp-sc7180.yaml#
>> +$schema: http://devicetree.org/meta-schemas/core.yaml#
>> +
>> +title: Description of Qualcomm Display Port dt properties.
> 
> This title should be something like
> 
> "Qualcomm Display Port Controller"
> 
Changed title as suggested.
>> +
>> +maintainers:
>> +  - Chandan Uddaraju <chandanu@codeaurora.org>
>> +  - Vara Reddy <varar@codeaurora.org>
>> +
>> +description: |
>> +  Device tree bindings for MSM Display Port which supports DP host
>> controllers
>> +  that are compatible with VESA Display Port interface specification.
>> +
>> +properties:
>> +  "msm_dp":
>> +    type: object
>> +    description: |
>> +      Node containing Display port register address bases, clocks, 
>> power
>> supplies.
>> +
>> +    properties:
>> +     compatible:
>> +       items:
>> +         - const: qcom,dp-display
>> +
>> +     cell-index:
>> +       description: Specifies the controller instance.
>> +
>> +     reg:
>> +       description: Physical base address and length of controller's
>> registers.
>> +
>> +     reg-names:
>> +       description: |
>> +         Names for different register regions defined above. The 
>> required
>> region
>> +         is mentioned below.
>> +       items:
>> +         - const: dp_ahb
>> +         - const: dp_aux
>> +         - const: dp_link
>> +         - const: dp_p0
>> +         - const: dp_phy
>> +         - const: dp_ln_tx0
>> +         - const: dp_ln_tx1
>> +         - const: afprom_physical
>> +         - const: dp_pll
>> +         - const: usb3_dp_com
>> +         - const: hdcp_physical
>> +
>> +     interrupts:
>> +       description: The interrupt signal from the DP block.
>> +
>> +     clocks:
>> +       description: List of clock specifiers for clocks needed by the
>> device.
>> +
>> +     clock-names:
>> +       description: |
>> +         Device clock names in the same order as mentioned in clocks
>> property.
>> +         The required clocks are mentioned below.
>> +       items:
>> +         - const: core_aux_clk
>> +         - const: core_ref_clk_src
>> +         - const: core_usb_ref_clk
>> +         - const: core_usb_cfg_ahb_clk
>> +         - const: core_usb_pipe_clk
>> +         - const: ctrl_link_clk
>> +         - const: ctrl_link_iface_clk
>> +         - const: ctrl_pixel_clk
>> +         - const: crypto_clk
>> +         - const: pixel_clk_rcg
>> +
>> +     pll-node:
>> +       description: phandle to DP PLL node.
>> +
>> +     vdda-1p2-supply:
>> +       description: phandle to vdda 1.2V regulator node.
>> +
>> +     vdda-0p9-supply:
>> +       description: phandle to vdda 0.9V regulator node.
>> +
>> +     aux-cfg0-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 0 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg1-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 1 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg2-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 2 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg3-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 3 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg4-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 4 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg5-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 5 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg6-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 6 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg7-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 7 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg8-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 8 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     aux-cfg9-settings:
>> +       description: |
>> +         Specifies the DP AUX configuration 9 settings.
>> +         The first entry in this array corresponds to the register 
>> offset
>> +         within DP AUX, while the remaining entries indicate the
>> +         programmable values.
>> +
>> +     max-pclk-frequency-khz:
>> +       description: Maximum displayport pixel clock supported for the
>> chipset.
>> +
>> +     data-lanes:
>> +       description: Maximum number of lanes that can be used for 
>> Display
>> port.
> 
> This should be an array of cells, not a single cell indicating the
> number of lanes.
> 
Done. Now data-lanes is array of integers and size of array represents 
maximum number of lanes supported.
>> +
>> +     usbplug-cc-gpio:
>> +       maxItems: 1
>> +       description: Specifies the usbplug orientation gpio.
>> +
>> +     aux-en-gpio:
>> +       maxItems: 1
>> +       description: Specifies the aux-channel enable gpio.
>> +
>> +     aux-sel-gpio:
>> +       maxItems: 1
>> +       description: Specifies the sux-channel select gpio.
>> +
>> +     ports:
>> +       description: |
>> +         Contains display port controller endpoint subnode.
>> +         remote-endpoint: |
>> +           For port@0, set to phandle of the connected panel/bridge's
>> +           input endpoint. For port@1, set to the DPU interface 
>> output.
>> +           Documentation/devicetree/bindings/graph.txt and
>> +           
>> Documentation/devicetree/bindings/media/video-interfaces.txt.
>> +
>> +  "dp_pll":
>> +     type: object
>> +     description: Node contains properties of Display port pll and 
>> phy
>> driver.
>> +
>> +     properties:
>> +       compatible:
>> +         items:
>> +           - const: qcom,dp-pll-10nm
>> +
>> +       cell-index:
>> +         description: Specifies the controller instance.
>> +
>> +       reg:
>> +         description: Physical base address and length of DP phy and 
>> pll
>> registers.
>> +
>> +       reg-names:
>> +         description: |
>> +           Names for different register regions defined above. The
>> required region
>> +           is mentioned below.
>> +         items:
>> +           - const: pll_base
>> +           - const: phy_base
>> +           - const: ln_tx0_base
>> +           - const: ln_tx1_base
>> +           - const: gdsc_base
>> +
>> +       clocks:
>> +         description: List of clock specifiers for clocks needed by 
>> the
>> device.
>> +
>> +       clock-names:
>> +         description: |
>> +           Device clock names in the same order as mentioned in 
>> clocks
>> property.
>> +           The required clocks are mentioned below.
>> +         items:
>> +           - const: iface
>> +           - const: ref
>> +           - const: cfg_ahb
>> +           - const: pipe
>> +
>> +examples:
>> +  - |
>> +    #include <dt-bindings/interrupt-controller/arm-gic.h>
>> +    #include <dt-bindings/clock/qcom,dispcc-sdm845.h>
>> +    #include <dt-bindings/clock/qcom,gcc-sdm845.h>
>> +    #include <dt-bindings/clock/qcom,rpmh.h>
>> +    #include <dt-bindings/gpio/gpio.h>
>> +    msm_dp: displayport-controller@ae90000{
>> +                cell-index = <0>;
>> +        compatible = "qcom,dp-display";
>> +        reg =   <0 0xae90000 0 0x200>,
>> +                <0 0xae90200 0 0x200>,
>> +                <0 0xae90400 0 0xc00>,
>> +                <0 0xae91000 0 0x400>,
>> +                <0 0x88eaa00 0 0x200>,
>> +                <0 0x88ea200 0 0x200>,
>> +                <0 0x88ea600 0 0x200>,
>> +                <0 0x780000 0 0x6228>,
>> +                <0 0x088ea000 0 0x40>,
>> +                <0 0x88e8000 0 0x20>,
>> +                <0 0x0aee1000 0 0x034>;
> 
> This needs to be split up into at least two nodes. Any address above
> that starts in 88e needs to be put into a new node underneath the qmp
> phy. That is the "DP PHY" that lives in the power domain of the USB+DP
> combo phy. The qfprom_physical reg property should be removed from here
> and this binding should use the nvmem binding to reach into the qfprom
> to read out things (I guess there's some sort of HDCP key in the
> qfprom?).
> 
> After that I don't know why there are so many different reg properties
> for the DP controller here and why it needs to be split up.  It looks
> like we should just map the register space from 0xae90000 up to
> 0xae91400 as one big register region and have the driver figure out how
> to operate on top of that. If it changes between SoC versions then we
> should have a more specific compatible that tells us what registers are
> in what place.
> 

Done. Only one register region is specified here now in new bindings 
i.e. dp_controller
starting from 0xae90000 upto 0xae91400. Removed rest of the module 
offsets.
Driver will access each module using offset as required. Also PHY and 
USB3 DPCOM
register bases are hard-coded in driver. Removed redundant qfprom and 
hdcp registers from bindings.

>> +        reg-names = "dp_ahb", "dp_aux", "dp_link",
>> +            "dp_p0", "dp_phy", "dp_ln_tx0", "dp_ln_tx1",
>> +            "qfprom_physical", "dp_pll",
>> +            "usb3_dp_com", "hdcp_physical";
>> +
>> +        interrupt-parent = <&display_subsystem>;
>> +        interrupts = <12 0>;
>> +
>> +        clocks = <&dispcc DISP_CC_MDSS_DP_AUX_CLK>,
>> +            <&rpmhcc RPMH_CXO_CLK>,
>> +            <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
>> +            <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
>> +            <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_LINK_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_LINK_INTF_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_CRYPTO_CLK>,
>> +            <&dispcc DISP_CC_MDSS_DP_PIXEL_CLK_SRC>;
>> +        clock-names = "core_aux_clk", "core_ref_clk_src",
>> +            "core_usb_ref_clk", "core_usb_cfg_ahb_clk",
>> +            "core_usb_pipe_clk", "ctrl_link_clk",
>> +            "ctrl_link_iface_clk", "ctrl_pixel_clk",
>> +            "crypto_clk", "pixel_clk_rcg";
>> +
>> +        pll-node = <&dp_pll>;
> 
> If the DP PLL and DP controller need to be controlled from two software
> entities, it may make sense to just combine that DP PLL into the
> controller node and have this node be a clk provider. The pll-node
> property is pretty ugly and should be removed.
> 

Done. Removed PLL as separate node and combined PLL as module of DP 
driver.
Removed pll-node property as well.

>> +        vdda-1p2-supply = <&vreg_l3c_1p2>;
>> +        vdda-0p9-supply = <&vreg_l4a_0p8>;
>> +
>> +        aux-cfg0-settings = [20 00];
>> +        aux-cfg1-settings = [24 13 23 1d];
>> +        aux-cfg2-settings = [28 24];
>> +        aux-cfg3-settings = [2c 00];
>> +        aux-cfg4-settings = [30 0a];
>> +        aux-cfg5-settings = [34 26];
>> +        aux-cfg6-settings = [38 0a];
>> +        aux-cfg7-settings = [3c 03];
>> +        aux-cfg8-settings = [40 bb];
>> +        aux-cfg9-settings = [44 03];
> 
> This pile of properties is board specific configuration tuning or
> something? Can this go into the driver? Or can it be made more human
> readable? I seem to recall that the USB phy had similar properties and
> we made them into human readable properties when board integrators
> needed to change them. The easiest approach there is to put everything
> in the driver for now and then when something has to change for a board
> it gets punted out to the DT and that overrides the "default" settings
> that are used in the driver.
> 
Made aux-cfg[0-9]-settingsproperties optional in bindings.
Added default configurations in driver and using them if these 
properties
are not mentioned in dts.
>> +
>> +        max-pclk-frequency-khz = <67500>;
> 
> What is this? Why isn't this in the driver?
> 

Done. Removed this property from bindings and setting default value in 
driver.

>> +        data-lanes = <2>;
>> +
>> +        aux-en-gpio = <&msmgpio 55 1>;
>> +        aux-sel-gpio = <&msmgpio 110 1>;
>> +        usbplug-cc-gpio = <&msmgpio 90 1>;
>> +
>> +        ports {
>> +            #address-cells = <1>;
>> +            #size-cells = <0>;
>> +
>> +            port@0 {
>> +                reg = <0>;
>> +                dp_in: endpoint {
>> +                    remote-endpoint = <&dpu_intf0_out>;
>> +                };
>> +            };
>> +
>> +            port@1 {
>> +                reg = <1>;
>> +                dp_out: endpoint {
>> +                };
>> +            };
>> +        };
>> +    };
>> +
>> +    dp_pll: dp-pll@088ea000 {
>> +        compatible = "qcom,dp-pll-10nm";
>> +        label = "DP PLL";
>> +        cell-index = <0>;
>> +        #clock-cells = <1>;
>> +
>> +        reg = <0 0x088ea000 0 0x200>,
>> +              <0 0x088eaa00 0 0x200>,
>> +              <0 0x088ea200 0 0x200>,
>> +              <0 0x088ea600 0 0x200>,
>> +              <0 0x08803000 0 0x8>;
>> +        reg-names = "pll_base", "phy_base", "ln_tx0_base",
>> +            "ln_tx1_base", "gdsc_base";
> 
> I guess the DP_PLL lives inside the qmp combo phy? That would match how
> the USB phy binding has been done there. This whole node should be
> combined with the DP phy node that will be placed as a child of the qmp
> phy wrapper (i.e. qcom,sc7180-qmp-usb3-phy compatible node). Might as
> well change that compatible from qcom,sc7180-qmp-usb3-phy to be
> qcom,sc7180-qmp-usb3-dp-phy too so that it can create the DP phy bits
> too.
> 

Done. Removed whole dp_pll node from here and added PLL as module of DP 
driver.
This required hard coding of few register bases in driver for now.

>> +
>> +        clocks = <&dispcc DISP_CC_MDSS_AHB_CLK>,
>> +             <&gcc GCC_USB3_PRIM_CLKREF_CLK>,
>> +             <&gcc GCC_USB_PHY_CFG_AHB2PHY_CLK>,
>> +             <&gcc GCC_USB3_PRIM_PHY_PIPE_CLK>;
>> +        clock-names = "iface_clk", "ref_clk",
>> +            "cfg_ahb_clk", "pipe_clk";
>> +    };
>> +
>> diff --git a/Documentation/devicetree/bindings/display/msm/dpu.txt
>> b/Documentation/devicetree/bindings/display/msm/dpu.txt
>> index 551ae26..7e99e45 100644
>> --- a/Documentation/devicetree/bindings/display/msm/dpu.txt
>> +++ b/Documentation/devicetree/bindings/display/msm/dpu.txt
>> @@ -63,8 +63,9 @@ Required properties:
>>         Documentation/devicetree/bindings/graph.txt
>>         Documentation/devicetree/bindings/media/video-interfaces.txt
>> 
>> -       Port 0 -> DPU_INTF1 (DSI1)
>> -       Port 1 -> DPU_INTF2 (DSI2)
>> +       Port 0 -> DPU_INTF0 (DP)
>> +       Port 1 -> DPU_INTF1 (DSI1)
>> +       Port 2 -> DPU_INTF2 (DSI2)
> 
> DP should come last so that the port mapping doesn't have to change.
> 

Done. Reverted Port mapping to old one i.e. Port0->DSI1, Port1->DSI2 and
Added Port2->DP mapping.

>> 
>>  Optional properties:
>>  - assigned-clocks: list of clock specifiers for clocks needing rate
>> assignment