[PATCH] arm64: dts: sdm845: Add display nodes to MTP dts

[Doug Anderson]

Hi,

On Fri, Nov 2, 2018 at 12:42 PM Jeykumar Sankaran <jsanka at codeaurora.org> wrote:
>
> Add mdss, dsi, dsi_phy, dsi pinctrl  and truly nt35597 panel nodes to
> sdm845 MTP board dtsi.
>
> Signed-off-by: Jeykumar Sankaran <jsanka at codeaurora.org>
> ---
>  arch/arm64/boot/dts/qcom/sdm845-mtp.dts | 124 ++++++++++++++++++++++++++++++++
>  1 file changed, 124 insertions(+)

A few nits below about trying to keep the sort ordering right in this
file.  I'm not an expert on device tree properties for display, but
other than these nits things here look good to me.

NOTE also that I'd probably put all 3 of your recent patches in a
3-part series since they all depend on each other, don't they?  AKA
I'd to see next time:

[PATCH v4 1/3] arm64: dts: qcom: sdm845: Add dpu to sdm845 dts file
[PATCH v4 2/3] arm64: dts: sdm845: Add dsi pinctrl nodes
[PATCH v4 3/3] arm64: dts: sdm845: Add display nodes to MTP dts

...you can call them all v4 just to make it easier to track...


> diff --git a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> index 6d651f3..6e98ae8 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> +++ b/arch/arm64/boot/dts/qcom/sdm845-mtp.dts
> @@ -8,6 +8,7 @@
>  /dts-v1/;
>
>  #include "sdm845.dtsi"
> +#include <dt-bindings/gpio/gpio.h>

Generally includes should have < includes above " includes.

...also: please rebase atop Andy's tree.  Then you'll have the line:

#include <dt-bindings/regulator/qcom,rpmh-regulator.h>

..."gpio" is alphabetically before "regulator" so your include should
be _above_ that one.

>
>  / {
>         model = "Qualcomm Technologies, Inc. SDM845 MTP";
> @@ -22,6 +23,113 @@
>         };
>  };
>
> +&dsi0 {
> +       status = "okay";
> +       qcom,dual-dsi-mode;
> +       qcom,master-dsi;
> +       qcom,sync-dual-dsi;
> +
> +       vdda-supply = <&vdda_mipi_dsi0_1p2>;
> +
> +       panel at 0 {
> +               compatible = "truly,nt35597-2K-display";
> +               reg = <0>;
> +
> +               vdda-supply = <&vreg_l14a_1p88>;
> +               vdispp-supply = <&lab_regulator>;
> +               vdispn-supply = <&ibb_regulator>;
> +
> +               pinctrl-names = "default", "suspend";
> +               pinctrl-0 = <&dpu_dsi_active>;
> +               pinctrl-1 = <&dpu_dsi_suspend>;
> +
> +               reset-gpios = <&tlmm 6 GPIO_ACTIVE_HIGH>;
> +               mode-gpios = <&tlmm 52 GPIO_ACTIVE_HIGH>;
> +
> +               display-timings {
> +                       timing0: timing-0 {
> +                               /* originally
> +                                * 268316160 Mhz,
> +                                * but value below fits
> +                                * better w/ downstream
> +                                */
> +                               clock-frequency = <268316138>;
> +                               hactive = <1440>;
> +                               vactive = <2560>;
> +                               hfront-porch = <200>;
> +                               hback-porch = <64>;
> +                               hsync-len = <32>;
> +                               vfront-porch = <8>;
> +                               vback-porch = <7>;
> +                               vsync-len = <1>;
> +                       };
> +               };
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       port at 0 {
> +                               reg = <0>;
> +                               panel0_in: endpoint {
> +                                       remote-endpoint = <&dsi0_out>;
> +                               };
> +                       };
> +
> +                       port at 1 {
> +                               reg = <1>;
> +                               panel1_in: endpoint {
> +                                       remote-endpoint = <&dsi1_out>;
> +                               };
> +                       };
> +               };
> +       };
> +
> +       ports {
> +               port at 1 {
> +                       endpoint {
> +                               remote-endpoint = <&panel0_in>;
> +                               data-lanes = <0 1 2 3>;
> +                       };
> +               };
> +       };
> +};
> +
> +&dsi0_phy {
> +       status = "okay";
> +       vdds-supply = <&vdda_mipi_dsi0_pll>;
> +};
> +
> +&dsi1 {
> +       status = "okay";
> +
> +       qcom,dual-dsi-mode;
> +       qcom,sync-dual-dsi;
> +
> +       vdda-supply = <&vdda_mipi_dsi1_1p2>;
> +
> +       ports {
> +               port at 1 {
> +                       endpoint {
> +                               remote-endpoint = <&panel1_in>;
> +                               data-lanes = <0 1 2 3>;
> +                       };
> +               };
> +       };
> +};
> +
> +&dsi1_phy {
> +       status = "okay";
> +       vdds-supply = <&vdda_mipi_dsi1_pll>;
> +};
> +
> +&mdss {
> +       status = "okay";
> +};
> +
> +&mdss_mdp {
> +       status = "okay";
> +};

Please keep these nodes sorted alphabetically even if it means you
don't keep all the display nodes together.  Thus "mdss" and "mdss_mdp"
should be below "i2c10" and above "qupv3_id_1".


> +
>  &i2c10 {
>         status = "okay";
>         clock-frequency = <400000>;
> @@ -58,3 +166,19 @@
>                 bias-pull-up;
>         };
>  };
> +
> +&dpu_dsi_active {
> +       pinconf {
> +               pins = "gpio6", "gpio52";
> +               drive-strength = <8>;
> +               bias-disable;
> +       };
> +};
> +
> +&dpu_dsi_suspend {
> +       pinconf {
> +               pins = "gpio6", "gpio52";
> +               drive-strength = <2>;
> +               bias-pull-down;
> +       };
> +};

These two things should sorted alphabetically in the "PINCTRL -
additions to nodes defined in sdm845.dtsi" section.  So please place
them both above the "qup_i2c10_default" node.