[PATCH v2 1/3] arm64: dts: sc7180: Factor out ti-sn65dsi86 support

[PATCH v2 1/3] arm64: dts: sc7180: Factor out ti-sn65dsi86 support

[Philip Chen]

Factor out ti-sn65dsi86 edp bridge as a separate dts fragment.
This helps us introduce the second source edp bridge later.

Reviewed-by: Stephen Boyd <swboyd@chromium.org>
Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v2:
- Move edp_brij_i2c completely out of sc7180-trogdor.dtsi to the
  bridge dts fragment, so that we can cleanly assign different
  edp bridge in every board rev.

 .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  |  1 +
 .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  1 +
 .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  |  1 +
 .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts |  1 +
 .../dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 90 +++++++++++++++++++
 arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  | 86 ------------------
 6 files changed, 94 insertions(+), 86 deletions(-)
 create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
index a758e4d22612..1d13fba3bd2f 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
@@ -11,6 +11,7 @@
 ap_h1_spi: &spi0 {};
 
 #include "sc7180-trogdor.dtsi"
+#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
 
 /* Deleted nodes from trogdor.dtsi */
 
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
index 00535aaa43c9..27b26a782af9 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-lazor.dtsi
@@ -11,6 +11,7 @@
 ap_h1_spi: &spi0 {};
 
 #include "sc7180-trogdor.dtsi"
+#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
 
 &ap_sar_sensor {
 	semtech,cs0-ground;
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
index a246dbd74cc1..e7c7cad14989 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-pompom.dtsi
@@ -11,6 +11,7 @@
 ap_h1_spi: &spi0 {};
 
 #include "sc7180-trogdor.dtsi"
+#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
 
 / {
 	thermal-zones {
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
index 2b522f9e0d8f..457c25499863 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-r1.dts
@@ -13,6 +13,7 @@
 ap_h1_spi: &spi0 {};
 
 #include "sc7180-trogdor.dtsi"
+#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
 
 / {
 	model = "Google Trogdor (rev1+)";
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi
new file mode 100644
index 000000000000..97d5e45abd1d
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi
@@ -0,0 +1,90 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Trogdor dts fragment for the boards with TI sn65dsi86 edp bridge
+ *
+ * Copyright 2021 Google LLC.
+ */
+
+&dsi0_out {
+	remote-endpoint = <&sn65dsi86_in>;
+	data-lanes = <0 1 2 3>;
+};
+
+edp_brij_i2c: &i2c2 {
+	status = "okay";
+	clock-frequency = <400000>;
+
+	sn65dsi86_bridge: bridge@2d {
+		compatible = "ti,sn65dsi86";
+		reg = <0x2d>;
+		pinctrl-names = "default";
+		pinctrl-0 = <&edp_brij_en>, <&edp_brij_irq>;
+		gpio-controller;
+		#gpio-cells = <2>;
+
+		interrupt-parent = <&tlmm>;
+		interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
+
+		enable-gpios = <&tlmm 104 GPIO_ACTIVE_HIGH>;
+
+		vpll-supply = <&pp1800_edp_vpll>;
+		vccio-supply = <&pp1800_brij_vccio>;
+		vcca-supply = <&pp1200_brij>;
+		vcc-supply = <&pp1200_brij>;
+
+		clocks = <&rpmhcc RPMH_LN_BB_CLK3>;
+		clock-names = "refclk";
+
+		no-hpd;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				sn65dsi86_in: endpoint {
+					remote-endpoint = <&dsi0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				sn65dsi86_out: endpoint {
+					data-lanes = <0 1>;
+					remote-endpoint = <&panel_in_edp>;
+				};
+			};
+		};
+
+		aux-bus {
+			panel: panel {
+				/* Compatible will be filled in per-board */
+				power-supply = <&pp3300_dx_edp>;
+				backlight = <&backlight>;
+				hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
+
+				port {
+					panel_in_edp: endpoint {
+						remote-endpoint = <&sn65dsi86_out>;
+					};
+				};
+			};
+		};
+	};
+};
+
+&tlmm {
+	edp_brij_irq: edp-brij-irq {
+		pinmux {
+			pins = "gpio11";
+			function = "gpio";
+		};
+
+		pinconf {
+			pins = "gpio11";
+			drive-strength = <2>;
+			bias-pull-down;
+		};
+	};
+};
diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
index 0f2b3c00e434..702139e89a80 100644
--- a/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi
@@ -602,15 +602,6 @@ &camcc {
 &dsi0 {
 	status = "okay";
 	vdda-supply = <&vdda_mipi_dsi0_1p2>;
-
-	ports {
-		port@1 {
-			endpoint {
-				remote-endpoint = <&sn65dsi86_in>;
-				data-lanes = <0 1 2 3>;
-			};
-		};
-	};
 };
 
 &dsi_phy {
@@ -618,70 +609,6 @@ &dsi_phy {
 	vdds-supply = <&vdda_mipi_dsi0_pll>;
 };
 
-edp_brij_i2c: &i2c2 {
-	status = "okay";
-	clock-frequency = <400000>;
-
-	sn65dsi86_bridge: bridge@2d {
-		compatible = "ti,sn65dsi86";
-		reg = <0x2d>;
-		pinctrl-names = "default";
-		pinctrl-0 = <&edp_brij_en>, <&edp_brij_irq>;
-		gpio-controller;
-		#gpio-cells = <2>;
-
-		interrupt-parent = <&tlmm>;
-		interrupts = <11 IRQ_TYPE_LEVEL_HIGH>;
-
-		enable-gpios = <&tlmm 104 GPIO_ACTIVE_HIGH>;
-
-		vpll-supply = <&pp1800_edp_vpll>;
-		vccio-supply = <&pp1800_brij_vccio>;
-		vcca-supply = <&pp1200_brij>;
-		vcc-supply = <&pp1200_brij>;
-
-		clocks = <&rpmhcc RPMH_LN_BB_CLK3>;
-		clock-names = "refclk";
-
-		no-hpd;
-
-		ports {
-			#address-cells = <1>;
-			#size-cells = <0>;
-
-			port@0 {
-				reg = <0>;
-				sn65dsi86_in: endpoint {
-					remote-endpoint = <&dsi0_out>;
-				};
-			};
-
-			port@1 {
-				reg = <1>;
-				sn65dsi86_out: endpoint {
-					data-lanes = <0 1>;
-					remote-endpoint = <&panel_in_edp>;
-				};
-			};
-		};
-
-		aux-bus {
-			panel: panel {
-				/* Compatible will be filled in per-board */
-				power-supply = <&pp3300_dx_edp>;
-				backlight = <&backlight>;
-				hpd-gpios = <&sn65dsi86_bridge 2 GPIO_ACTIVE_HIGH>;
-
-				port {
-					panel_in_edp: endpoint {
-						remote-endpoint = <&sn65dsi86_out>;
-					};
-				};
-			};
-		};
-	};
-};
-
 ap_sar_sensor_i2c: &i2c5 {
 	clock-frequency = <400000>;
 
@@ -1245,19 +1172,6 @@ pinconf {
 		};
 	};
 
-	edp_brij_irq: edp-brij-irq {
-		pinmux {
-			pins = "gpio11";
-			function = "gpio";
-		};
-
-		pinconf {
-			pins = "gpio11";
-			drive-strength = <2>;
-			bias-pull-down;
-		};
-	};
-
 	en_pp3300_codec: en-pp3300-codec {
 		pinmux {
 			pins = "gpio83";
-- 
2.33.0.685.g46640cef36-goog

[PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Philip Chen]

Add a dts fragment file to support the sc7180 boards with the second
source edp bridge, Parade ps8640.

Signed-off-by: Philip Chen <philipchen@chromium.org>
---

Changes in v2:
- Add the definition of edp_brij_i2c and some other properties to
  ps8640 dts, making it match ti-sn65dsi86 dts better

 .../qcom/sc7180-trogdor-parade-ps8640.dtsi    | 108 ++++++++++++++++++
 1 file changed, 108 insertions(+)
 create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi

diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi
new file mode 100644
index 000000000000..c274ab41bd67
--- /dev/null
+++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi
@@ -0,0 +1,108 @@
+// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
+/*
+ * Google Trogdor dts fragment for the boards with Parade ps8640 edp bridge
+ *
+ * Copyright 2021 Google LLC.
+ */
+
+/ {
+	pp3300_brij_ps8640: pp3300-brij-ps8640 {
+		compatible = "regulator-fixed";
+		status = "okay";
+		regulator-name = "pp3300_brij_ps8640";
+
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+
+		gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
+
+		vin-supply = <&pp3300_a>;
+	};
+};
+
+&dsi0_out {
+	remote-endpoint = <&ps8640_in>;
+};
+
+edp_brij_i2c: &i2c2 {
+	status = "okay";
+	clock-frequency = <400000>;
+
+	ps8640_bridge: edp-bridge@8 {
+		compatible = "parade,ps8640";
+		reg = <0x8>;
+
+		powerdown-gpios = <&tlmm 104 GPIO_ACTIVE_LOW>;
+		reset-gpios = <&tlmm 11 GPIO_ACTIVE_LOW>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&edp_brij_en>, <&edp_brij_ps8640_rst>;
+
+		vdd12-supply = <&pp1200_brij>;
+		vdd33-supply = <&pp3300_brij_ps8640>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+
+			port@0 {
+				reg = <0>;
+				ps8640_in: endpoint {
+					remote-endpoint = <&dsi0_out>;
+				};
+			};
+
+			port@1 {
+				reg = <1>;
+				ps8640_out: endpoint {
+					remote-endpoint = <&panel_in_edp>;
+				};
+			};
+		};
+
+		aux_bus: aux-bus {
+			panel: panel {
+				/* Compatible will be filled in per-board */
+				power-supply = <&pp3300_dx_edp>;
+				backlight = <&backlight>;
+
+				port {
+					panel_in_edp: endpoint {
+						remote-endpoint = <&ps8640_out>;
+					};
+				};
+			};
+		};
+	};
+};
+
+&tlmm {
+	edp_brij_ps8640_rst: edp-brij-ps8640-rst {
+		pinmux {
+			pins = "gpio11";
+			function = "gpio";
+		};
+
+		pinconf {
+			pins = "gpio11";
+			drive-strength = <2>;
+			bias-disable;
+		};
+	};
+
+	en_pp3300_edp_brij_ps8640: en-pp3300-edp-brij-ps8640 {
+		pinmux {
+			pins = "gpio32";
+			function = "gpio";
+		};
+
+		pinconf {
+			pins = "gpio32";
+			drive-strength = <2>;
+			bias-disable;
+		};
+	};
+};
-- 
2.33.0.685.g46640cef36-goog

Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Stephen Boyd]

Quoting Philip Chen (2021-09-29 17:34:58)
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi
> new file mode 100644
> index 000000000000..c274ab41bd67
> --- /dev/null
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-parade-ps8640.dtsi
> @@ -0,0 +1,108 @@
> +// SPDX-License-Identifier: (GPL-2.0+ OR MIT)
> +/*
> + * Google Trogdor dts fragment for the boards with Parade ps8640 edp bridge
> + *
> + * Copyright 2021 Google LLC.
> + */
> +
> +/ {
> +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> +               compatible = "regulator-fixed";
> +               status = "okay";
> +               regulator-name = "pp3300_brij_ps8640";
> +
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +
> +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;

Doesn't this need

		enable-active-high;

?

> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> +
> +               vin-supply = <&pp3300_a>;
> +       };
> +};
> +
> +&dsi0_out {
> +       remote-endpoint = <&ps8640_in>;

Should this also have data-lanes to be "complete"?

> +};
> +
> +edp_brij_i2c: &i2c2 {
> +       status = "okay";
> +       clock-frequency = <400000>;
> +
> +       ps8640_bridge: edp-bridge@8 {

Just bridge@8 to match ti bridge? Also, is the label used? If not
please drop it.

> +               compatible = "parade,ps8640";
> +               reg = <0x8>;
> +
> +               powerdown-gpios = <&tlmm 104 GPIO_ACTIVE_LOW>;
> +               reset-gpios = <&tlmm 11 GPIO_ACTIVE_LOW>;
> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&edp_brij_en>, <&edp_brij_ps8640_rst>;
> +
> +               vdd12-supply = <&pp1200_brij>;
> +               vdd33-supply = <&pp3300_brij_ps8640>;
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +
> +                       port@0 {
> +                               reg = <0>;
> +                               ps8640_in: endpoint {
> +                                       remote-endpoint = <&dsi0_out>;
> +                               };
> +                       };
> +
> +                       port@1 {
> +                               reg = <1>;
> +                               ps8640_out: endpoint {
> +                                       remote-endpoint = <&panel_in_edp>;
> +                               };
> +                       };
> +               };
> +
> +               aux_bus: aux-bus {

Is this label used either?

> +                       panel: panel {
> +                               /* Compatible will be filled in per-board */
> +                               power-supply = <&pp3300_dx_edp>;
> +                               backlight = <&backlight>;
> +
> +                               port {
> +                                       panel_in_edp: endpoint {
> +                                               remote-endpoint = <&ps8640_out>;
> +                                       };
> +                               };
> +                       };
> +               };
> +       };
> +};
> +

Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Doug Anderson]

Hi,

On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
>
> > +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> > +               compatible = "regulator-fixed";
> > +               status = "okay";
> > +               regulator-name = "pp3300_brij_ps8640";
> > +
> > +               regulator-min-microvolt = <3300000>;
> > +               regulator-max-microvolt = <3300000>;
> > +
> > +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
>
> Doesn't this need
>
>                 enable-active-high;

Looks like it. Without that it looks like it assumes active low.


> > +
> > +               pinctrl-names = "default";
> > +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> > +
> > +               vin-supply = <&pp3300_a>;
> > +       };
> > +};
> > +
> > +&dsi0_out {
> > +       remote-endpoint = <&ps8640_in>;
>
> Should this also have data-lanes to be "complete"?

That's still back in the main trogdor.dtsi, isn't it?


> > +edp_brij_i2c: &i2c2 {
> > +       status = "okay";
> > +       clock-frequency = <400000>;
> > +
> > +       ps8640_bridge: edp-bridge@8 {
>
> Just bridge@8 to match ti bridge? Also, is the label used? If not
> please drop it.

I agree with Stephen about it being "bridge@8". Personally I don't
mind labels like this even if they're not used since they don't hurt
and it creates less churn to add them now, but I won't fight hard to
keep them.


> > +               aux_bus: aux-bus {
>
> Is this label used either?

Yeah, I'd get rid of this one since there you didn't add it in the
sn65dsi86 dtsi file and it seems exceedingly unlikely we'd need it for
any reason.

-Doug

Re: [PATCH v2 1/3] arm64: dts: sc7180: Factor out ti-sn65dsi86 support

[Doug Anderson]

Hi,

On Wed, Sep 29, 2021 at 5:35 PM Philip Chen <philipchen@chromium.org> wrote:
>
> Factor out ti-sn65dsi86 edp bridge as a separate dts fragment.
> This helps us introduce the second source edp bridge later.
>
> Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> Signed-off-by: Philip Chen <philipchen@chromium.org>
> ---
>
> Changes in v2:
> - Move edp_brij_i2c completely out of sc7180-trogdor.dtsi to the
>   bridge dts fragment, so that we can cleanly assign different
>   edp bridge in every board rev.
>
>  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  |  1 +
>  .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  1 +
>  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  |  1 +
>  .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts |  1 +
>  .../dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 90 +++++++++++++++++++
>  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  | 86 ------------------
>  6 files changed, 94 insertions(+), 86 deletions(-)
>  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> index a758e4d22612..1d13fba3bd2f 100644
> --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> @@ -11,6 +11,7 @@
>  ap_h1_spi: &spi0 {};
>
>  #include "sc7180-trogdor.dtsi"
> +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"

It looks like you're missing homestar, aren't you? I'd expect that
after applying your change that:

git grep -A1 include.*sc7180-trogdor.dtsi

...should show your new include right after all includes of
sc7180-trogdor.dtsi, but I don't see it for homestar.

Other than that this looks good to me. Feel free to add my Reviewed-by.

Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Philip Chen]

Hi,

On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
> >
> > > +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> > > +               compatible = "regulator-fixed";
> > > +               status = "okay";
> > > +               regulator-name = "pp3300_brij_ps8640";
> > > +
> > > +               regulator-min-microvolt = <3300000>;
> > > +               regulator-max-microvolt = <3300000>;
> > > +
> > > +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> >
> > Doesn't this need
> >
> >                 enable-active-high;
>
> Looks like it. Without that it looks like it assumes active low.
Thanks for catching this.
I'll fix it in v3.

>
>
> > > +
> > > +               pinctrl-names = "default";
> > > +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> > > +
> > > +               vin-supply = <&pp3300_a>;
> > > +       };
> > > +};
> > > +
> > > +&dsi0_out {
> > > +       remote-endpoint = <&ps8640_in>;
> >
> > Should this also have data-lanes to be "complete"?
>
> That's still back in the main trogdor.dtsi, isn't it?
Yes, I think so.
Plus, ti-sn65 dts doesn't define data-lanes for input either.

>
>
> > > +edp_brij_i2c: &i2c2 {
> > > +       status = "okay";
> > > +       clock-frequency = <400000>;
> > > +
> > > +       ps8640_bridge: edp-bridge@8 {
> >
> > Just bridge@8 to match ti bridge? Also, is the label used? If not
> > please drop it.
>
> I agree with Stephen about it being "bridge@8". Personally I don't
> mind labels like this even if they're not used since they don't hurt
> and it creates less churn to add them now, but I won't fight hard to
> keep them.
I will make it "bridge@8" to match ti-sn65 dts.
Meanwhile, can we keep "ps8640_bridge" label to match ti-sn65 dts?

>
>
> > > +               aux_bus: aux-bus {
> >
> > Is this label used either?
>
> Yeah, I'd get rid of this one since there you didn't add it in the
> sn65dsi86 dtsi file and it seems exceedingly unlikely we'd need it for
> any reason.
Sure, I will remove "aux_bus" label in v3.

>
> -Doug

Re: [PATCH v2 1/3] arm64: dts: sc7180: Factor out ti-sn65dsi86 support

[Philip Chen]

Hi

On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Wed, Sep 29, 2021 at 5:35 PM Philip Chen <philipchen@chromium.org> wrote:
> >
> > Factor out ti-sn65dsi86 edp bridge as a separate dts fragment.
> > This helps us introduce the second source edp bridge later.
> >
> > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > ---
> >
> > Changes in v2:
> > - Move edp_brij_i2c completely out of sc7180-trogdor.dtsi to the
> >   bridge dts fragment, so that we can cleanly assign different
> >   edp bridge in every board rev.
> >
> >  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  |  1 +
> >  .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  1 +
> >  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  |  1 +
> >  .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts |  1 +
> >  .../dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 90 +++++++++++++++++++
> >  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  | 86 ------------------
> >  6 files changed, 94 insertions(+), 86 deletions(-)
> >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi
> >
> > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > index a758e4d22612..1d13fba3bd2f 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > @@ -11,6 +11,7 @@
> >  ap_h1_spi: &spi0 {};
> >
> >  #include "sc7180-trogdor.dtsi"
> > +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
>
> It looks like you're missing homestar, aren't you? I'd expect that
> after applying your change that:
>
> git grep -A1 include.*sc7180-trogdor.dtsi
>
> ...should show your new include right after all includes of
> sc7180-trogdor.dtsi, but I don't see it for homestar.

I can't find homestar dts file in my upstream checkout.
But I found: https://patchwork.kernel.org/project/linux-arm-msm/patch/20210909122053.1.Ieafda79b74f74a2b15ed86e181c06a3060706ec5@changeid/
...Is it merged anywhere?

>
> Other than that this looks good to me. Feel free to add my Reviewed-by.

Re: [PATCH v2 1/3] arm64: dts: sc7180: Factor out ti-sn65dsi86 support

[Doug Anderson]

Hi,

On Thu, Oct 7, 2021 at 3:29 PM Philip Chen <philipchen@chromium.org> wrote:
>
> Hi
>
> On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Sep 29, 2021 at 5:35 PM Philip Chen <philipchen@chromium.org> wrote:
> > >
> > > Factor out ti-sn65dsi86 edp bridge as a separate dts fragment.
> > > This helps us introduce the second source edp bridge later.
> > >
> > > Reviewed-by: Stephen Boyd <swboyd@chromium.org>
> > > Signed-off-by: Philip Chen <philipchen@chromium.org>
> > > ---
> > >
> > > Changes in v2:
> > > - Move edp_brij_i2c completely out of sc7180-trogdor.dtsi to the
> > >   bridge dts fragment, so that we can cleanly assign different
> > >   edp bridge in every board rev.
> > >
> > >  .../boot/dts/qcom/sc7180-trogdor-coachz.dtsi  |  1 +
> > >  .../boot/dts/qcom/sc7180-trogdor-lazor.dtsi   |  1 +
> > >  .../boot/dts/qcom/sc7180-trogdor-pompom.dtsi  |  1 +
> > >  .../arm64/boot/dts/qcom/sc7180-trogdor-r1.dts |  1 +
> > >  .../dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi | 90 +++++++++++++++++++
> > >  arch/arm64/boot/dts/qcom/sc7180-trogdor.dtsi  | 86 ------------------
> > >  6 files changed, 94 insertions(+), 86 deletions(-)
> > >  create mode 100644 arch/arm64/boot/dts/qcom/sc7180-trogdor-ti-sn65dsi86.dtsi
> > >
> > > diff --git a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > > index a758e4d22612..1d13fba3bd2f 100644
> > > --- a/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > > +++ b/arch/arm64/boot/dts/qcom/sc7180-trogdor-coachz.dtsi
> > > @@ -11,6 +11,7 @@
> > >  ap_h1_spi: &spi0 {};
> > >
> > >  #include "sc7180-trogdor.dtsi"
> > > +#include "sc7180-trogdor-ti-sn65dsi86.dtsi"
> >
> > It looks like you're missing homestar, aren't you? I'd expect that
> > after applying your change that:
> >
> > git grep -A1 include.*sc7180-trogdor.dtsi
> >
> > ...should show your new include right after all includes of
> > sc7180-trogdor.dtsi, but I don't see it for homestar.
>
> I can't find homestar dts file in my upstream checkout.
> But I found: https://patchwork.kernel.org/project/linux-arm-msm/patch/20210909122053.1.Ieafda79b74f74a2b15ed86e181c06a3060706ec5@changeid/
> ...Is it merged anywhere?

The device trees need to be posted against the Qualcomm tree.

git://git.kernel.org/pub/scm/linux/kernel/git/qcom/linux.git

It should be in the for-next branch there.

-Doug

Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Philip Chen]

Hi

On Thu, Oct 7, 2021 at 11:15 AM Philip Chen <philipchen@chromium.org> wrote:
>
> Hi,
>
> On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson <dianders@chromium.org> wrote:
> >
> > Hi,
> >
> > On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > >
> > > > +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> > > > +               compatible = "regulator-fixed";
> > > > +               status = "okay";
> > > > +               regulator-name = "pp3300_brij_ps8640";
> > > > +
> > > > +               regulator-min-microvolt = <3300000>;
> > > > +               regulator-max-microvolt = <3300000>;
> > > > +
> > > > +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> > >
> > > Doesn't this need
> > >
> > >                 enable-active-high;
> >
> > Looks like it. Without that it looks like it assumes active low.
> Thanks for catching this.
> I'll fix it in v3.
>
> >
> >
> > > > +
> > > > +               pinctrl-names = "default";
> > > > +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> > > > +
> > > > +               vin-supply = <&pp3300_a>;
> > > > +       };
> > > > +};
> > > > +
> > > > +&dsi0_out {
> > > > +       remote-endpoint = <&ps8640_in>;
> > >
> > > Should this also have data-lanes to be "complete"?
> >
> > That's still back in the main trogdor.dtsi, isn't it?
> Yes, I think so.
> Plus, ti-sn65 dts doesn't define data-lanes for input either.
Sorry, I was wrong.
ti-sn65 dts actually defines data-lanes for input.
However, since ps8640 driver doesn't parse input data-lanes for now,
it's not useful to add data-lanes here anyway.

>
> >
> >
> > > > +edp_brij_i2c: &i2c2 {
> > > > +       status = "okay";
> > > > +       clock-frequency = <400000>;
> > > > +
> > > > +       ps8640_bridge: edp-bridge@8 {
> > >
> > > Just bridge@8 to match ti bridge? Also, is the label used? If not
> > > please drop it.
> >
> > I agree with Stephen about it being "bridge@8". Personally I don't
> > mind labels like this even if they're not used since they don't hurt
> > and it creates less churn to add them now, but I won't fight hard to
> > keep them.
> I will make it "bridge@8" to match ti-sn65 dts.
> Meanwhile, can we keep "ps8640_bridge" label to match ti-sn65 dts?
>
> >
> >
> > > > +               aux_bus: aux-bus {
> > >
> > > Is this label used either?
> >
> > Yeah, I'd get rid of this one since there you didn't add it in the
> > sn65dsi86 dtsi file and it seems exceedingly unlikely we'd need it for
> > any reason.
> Sure, I will remove "aux_bus" label in v3.
>
> >
> > -Doug

Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Doug Anderson]

Hi,

On Fri, Oct 8, 2021 at 11:46 AM Philip Chen <philipchen@chromium.org> wrote:
>
> Hi
>
> On Thu, Oct 7, 2021 at 11:15 AM Philip Chen <philipchen@chromium.org> wrote:
> >
> > Hi,
> >
> > On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson <dianders@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > >
> > > > > +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> > > > > +               compatible = "regulator-fixed";
> > > > > +               status = "okay";
> > > > > +               regulator-name = "pp3300_brij_ps8640";
> > > > > +
> > > > > +               regulator-min-microvolt = <3300000>;
> > > > > +               regulator-max-microvolt = <3300000>;
> > > > > +
> > > > > +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> > > >
> > > > Doesn't this need
> > > >
> > > >                 enable-active-high;
> > >
> > > Looks like it. Without that it looks like it assumes active low.
> > Thanks for catching this.
> > I'll fix it in v3.
> >
> > >
> > >
> > > > > +
> > > > > +               pinctrl-names = "default";
> > > > > +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> > > > > +
> > > > > +               vin-supply = <&pp3300_a>;
> > > > > +       };
> > > > > +};
> > > > > +
> > > > > +&dsi0_out {
> > > > > +       remote-endpoint = <&ps8640_in>;
> > > >
> > > > Should this also have data-lanes to be "complete"?
> > >
> > > That's still back in the main trogdor.dtsi, isn't it?
> > Yes, I think so.
> > Plus, ti-sn65 dts doesn't define data-lanes for input either.
> Sorry, I was wrong.
> ti-sn65 dts actually defines data-lanes for input.
> However, since ps8640 driver doesn't parse input data-lanes for now,
> it's not useful to add data-lanes here anyway.

Ah, right. This one _isn't_ in the dtsi. Looking closer, I agree with
you that it's not useful. Specifically it should be noted that, unlike
ti-sn65dsi86, this bridge part looks to only support 2-lanes of DP
traffic. If both of these two lanes are routed to the panel then
there's really nothing to specify--that should be the default
assumption of the driver if/when it ever adds support for data-lanes.

-Doug

Re: [PATCH v2 3/3] arm64: dts: sc7180: Support Parade ps8640 edp bridge

[Philip Chen]

Hi,

On Fri, Oct 8, 2021 at 4:13 PM Doug Anderson <dianders@chromium.org> wrote:
>
> Hi,
>
> On Fri, Oct 8, 2021 at 11:46 AM Philip Chen <philipchen@chromium.org> wrote:
> >
> > Hi
> >
> > On Thu, Oct 7, 2021 at 11:15 AM Philip Chen <philipchen@chromium.org> wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Sep 30, 2021 at 9:22 AM Doug Anderson <dianders@chromium.org> wrote:
> > > >
> > > > Hi,
> > > >
> > > > On Wed, Sep 29, 2021 at 9:02 PM Stephen Boyd <swboyd@chromium.org> wrote:
> > > > >
> > > > > > +       pp3300_brij_ps8640: pp3300-brij-ps8640 {
> > > > > > +               compatible = "regulator-fixed";
> > > > > > +               status = "okay";
> > > > > > +               regulator-name = "pp3300_brij_ps8640";
> > > > > > +
> > > > > > +               regulator-min-microvolt = <3300000>;
> > > > > > +               regulator-max-microvolt = <3300000>;
> > > > > > +
> > > > > > +               gpio = <&tlmm 32 GPIO_ACTIVE_HIGH>;
> > > > >
> > > > > Doesn't this need
> > > > >
> > > > >                 enable-active-high;
> > > >
> > > > Looks like it. Without that it looks like it assumes active low.
> > > Thanks for catching this.
> > > I'll fix it in v3.
> > >
> > > >
> > > >
> > > > > > +
> > > > > > +               pinctrl-names = "default";
> > > > > > +               pinctrl-0 = <&en_pp3300_edp_brij_ps8640>;
> > > > > > +
> > > > > > +               vin-supply = <&pp3300_a>;
> > > > > > +       };
> > > > > > +};
> > > > > > +
> > > > > > +&dsi0_out {
> > > > > > +       remote-endpoint = <&ps8640_in>;
> > > > >
> > > > > Should this also have data-lanes to be "complete"?
> > > >
> > > > That's still back in the main trogdor.dtsi, isn't it?
> > > Yes, I think so.
> > > Plus, ti-sn65 dts doesn't define data-lanes for input either.
> > Sorry, I was wrong.
> > ti-sn65 dts actually defines data-lanes for input.
> > However, since ps8640 driver doesn't parse input data-lanes for now,
> > it's not useful to add data-lanes here anyway.
>
> Ah, right. This one _isn't_ in the dtsi. Looking closer, I agree with
> you that it's not useful. Specifically it should be noted that, unlike
> ti-sn65dsi86, this bridge part looks to only support 2-lanes of DP
> traffic. If both of these two lanes are routed to the panel then
> there's really nothing to specify--that should be the default
> assumption of the driver if/when it ever adds support for data-lanes.

Actually, dsi0_out is the input to the bridge.
So the data lanes here are "MIPI DSI lanes", which is hardcoded to 4,
for both sn65 driver and ps8640 driver, right?

What's different in sn65 driver and ps8640 driver is the output data
lanes for DP:
* sn65 supports 1, 2, or 4 DP lanes. The driver parses DP "data lanes"
from DT and then configures the support accordingly.
* ps8640 supports 1 or 2 DP lanes. As of now, the driver doesn't parse
DP "data lanes" from DT.

As a result:
* Adding input "data lanes" for either sn65 or ps8640 DT is not useful.
* Adding output "data lanes" for sn65 DT is useful, while adding
output "data lanes" for ps8640 DT is not useful.

>
> -Doug