diff mbox series

[v1,1/2] ARM64: dts: qcom: enable eDP panel support for sc7280

Message ID 1643048114-2996-2-git-send-email-quic_sbillaka@quicinc.com (mailing list archive)
State New, archived
Headers show
Series Add support for the eDP panel on sc7280 CRD | expand

Commit Message

Sankeerth Billakanti (QUIC) Jan. 24, 2022, 6:15 p.m. UTC
Enable the eDP display panel support with backlight on sc7280 platform.

Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
---
 arch/arm64/boot/dts/qcom/sc7280-crd.dts | 127 ++++++++++++++++++++++++++++++++
 1 file changed, 127 insertions(+)

Comments

Doug Anderson Jan. 24, 2022, 7:04 p.m. UTC | #1
Hi,

On Mon, Jan 24, 2022 at 10:15 AM Sankeerth Billakanti
<quic_sbillaka@quicinc.com> wrote:
>
> Enable the eDP display panel support with backlight on sc7280 platform.
>
> Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com>
> ---
>  arch/arm64/boot/dts/qcom/sc7280-crd.dts | 127 ++++++++++++++++++++++++++++++++
>  1 file changed, 127 insertions(+)
>
> diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> index cd2755c..fde6f75 100644
> --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> @@ -64,6 +64,47 @@ ap_ts_pen_1v8: &i2c13 {
>         };
>  };
>
> +&mdss {
> +       status = "okay";
> +};
> +
> +&mdss_mdp {
> +       status = "okay";
> +};

"mdss_mdp" sorts after "mdss_edp",


> +&mdss_edp {
> +       status = "okay";
> +
> +       vdda-1p2-supply = <&vreg_l6b_1p2>;
> +       vdda-0p9-supply = <&vreg_l10c_0p8>;
> +
> +       ports {
> +               port@1 {
> +                       reg = <1>;
> +                       edp_out: endpoint {
> +                               remote-endpoint = <&edp_panel_in>;
> +                       };
> +               };
> +       };

I think part of the above should be in sc7280.dtsi. Basically in
sc7820.dtsi I think you should have:

ports {
  #address-cells = <1>;
  #size-cells = <0>;
  port@0 {
    reg = <0>;
    edp_in: endpoint {
      remote-endpoint = <&dpu_intf5_out>;
    };
  };
  port@1 {
    reg = <1>;
    edp_out: endpoint {
    };
  };
};

...and then the crd dts file just needs:

&edp_out {
  remote-endpoint = <&edp_panel_in>;
};

Right?


> +};
> +
> +&mdss_edp_phy {
> +       status = "okay";
> +
> +       vdda-1p2-supply = <&vreg_l6b_1p2>;
> +       vdda-0p9-supply = <&vreg_l10c_0p8>;
> +};
> +
> +&mdss_dp {
> +       status = "okay";
> +
> +       pinctrl-names = "default";
> +       pinctrl-0 = <&dp_hot_plug_det>;
> +       data-lanes = <0 1>;
> +       vdda-1p2-supply = <&vreg_l6b_1p2>;
> +       vdda-0p9-supply = <&vreg_l1b_0p8>;
> +};
> +
>  &nvme_3v3_regulator {
>         gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>;
>  };
> @@ -72,7 +113,93 @@ ap_ts_pen_1v8: &i2c13 {
>         pins = "gpio51";
>  };
>
> +&pm8350c_pwm{

nit: space after "pwm" and before "{"


> +       status = "okay";

Don't you need:

pinctrl-names = "default";
pinctrl-0 = <&backlight_pwm_default>;


> +};
> +
> +&pm8350c_gpios {
> +       backlight_pwm_default: backlight_pwm_default {

nit: node name should have dashes, not underscores. Also, don't
include the word "default".


> +               pinconf {
> +                       pins = "gpio8";
> +                       function = "func1";
> +                       output-low;
> +                       bias-disable;
> +                       power-source = <0>;
> +                       qcom,drive-strength = <3>;


Instead of 3, should be PMIC_GPIO_STRENGTH_LOW

> +               };

Don't need the "pinconf" subnode.


> +       };
> +};
> +
> +&soc {

Don't need to put the regulators under &soc. They can be top level, right?


> +       backlight_power: backlight_power {

nit: node names should have "-", not "_"


> +               compatible = "regulator-fixed";
> +               regulator-name = "backlight_power";
> +               regulator-always-on;
> +               regulator-boot-on;
> +       };
> +
> +       edp_power: edp_power {

nit: node names should have "-", not "_"


> +               compatible = "regulator-fixed";
> +               regulator-name = "edp_power";
> +
> +               regulator-min-microvolt = <3300000>;
> +               regulator-max-microvolt = <3300000>;
> +
> +               gpio = <&tlmm 80 GPIO_ACTIVE_HIGH>;
> +               enable-active-high;
> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&edp_panel_power_default>;
> +       };
> +
> +       edp_backlight: edp_backlight {

nit: node names should have a "-", not an "_".


> +               compatible = "pwm-backlight";
> +
> +               pwms = <&pm8350c_pwm 3 65535>;
> +               power-supply = <&backlight_power>;
> +               enable-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>;

So wait. Why do you have a "bogus" backlight_power regulator and then
a separate enable-gpio? Why

> +
> +               pinctrl-names = "default";
> +               pinctrl-0 = <&backlight_pwm_default>;

This pinctrl belongs as part of the PWM, not as part of the backlight.

...but where's the pinctrl for pm8350c_gpios #7? That _should_ be here
since that's the backlight enable.


> +       };
> +
> +       edp_panel: edp_panel {
> +               compatible = "sharp_lq140m1jw46";

Device tree compatible strings separate the manufacturer from the
model with ",", not "_".


> +               pinctrl-names = "default";
> +               pinctrl-0 = <&edp_hot_plug_det>;

I think for eDP we probably want the pinctrl to be in the sc7280.dtsi
file. It seems highly likely that people will hook up the eDP HPD pin
to the standard place. If they don't then boards can override it.


> +               power-supply = <&edp_power>;
> +               backlight = <&edp_backlight>;
> +
> +               ports {
> +                       #address-cells = <1>;
> +                       #size-cells = <0>;
> +                       port@0 {
> +                               reg = <0>;
> +                               edp_panel_in: endpoint {
> +                                       remote-endpoint = <&edp_out>;
> +                               };
> +                       };

Like for DP, much of this "ports" stuff should be in sc7280.dtsi.


> +               };
> +       };
> +};
> +
>  &tlmm {
> +       edp_hot_plug_det: edp-hot-plug-det {
> +               pins = "gpio60";
> +               function = "edp_hot";

The "pins" and "function" belong in sc7280.dtsi.


> +               bias-pull-down;

This part belongs in the CRD file.


> +               input-enable;

You don't need "input-enable".


> +       };


> +       edp_panel_power_default: edp_panel_power_default {

Nit: node name shouldn't have underscores.
...and remove the word "default".
Doug Anderson Feb. 2, 2022, 9:26 p.m. UTC | #2
Hi,

On Mon, Jan 24, 2022 at 11:04 AM Doug Anderson <dianders@chromium.org> wrote:
>
> > +&mdss_edp {
> > +       status = "okay";
> > +
> > +       vdda-1p2-supply = <&vreg_l6b_1p2>;
> > +       vdda-0p9-supply = <&vreg_l10c_0p8>;
> > +
> > +       ports {
> > +               port@1 {
> > +                       reg = <1>;
> > +                       edp_out: endpoint {
> > +                               remote-endpoint = <&edp_panel_in>;
> > +                       };
> > +               };
> > +       };
>
> I think part of the above should be in sc7280.dtsi. Basically in
> sc7820.dtsi I think you should have:
>
> ports {
>   #address-cells = <1>;
>   #size-cells = <0>;
>   port@0 {
>     reg = <0>;
>     edp_in: endpoint {
>       remote-endpoint = <&dpu_intf5_out>;
>     };
>   };
>   port@1 {
>     reg = <1>;
>     edp_out: endpoint {
>     };
>   };
> };
>
> ...and then the crd dts file just needs:
>
> &edp_out {
>   remote-endpoint = <&edp_panel_in>;
> };
>
> Right?

I've attempted to do the sc7280 part of this in:

https://lore.kernel.org/r/20220202132301.v3.7.Ic84bb69c45be2fccf50e3bd17b845fe20eec624c@changeid

Assuming folks think that's good then you should probably base your
next version atop that.

-Doug
diff mbox series

Patch

diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
index cd2755c..fde6f75 100644
--- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
+++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
@@ -64,6 +64,47 @@  ap_ts_pen_1v8: &i2c13 {
 	};
 };
 
+&mdss {
+	status = "okay";
+};
+
+&mdss_mdp {
+	status = "okay";
+};
+
+&mdss_edp {
+	status = "okay";
+
+	vdda-1p2-supply = <&vreg_l6b_1p2>;
+	vdda-0p9-supply = <&vreg_l10c_0p8>;
+
+	ports {
+		port@1 {
+			reg = <1>;
+			edp_out: endpoint {
+				remote-endpoint = <&edp_panel_in>;
+			};
+		};
+	};
+};
+
+&mdss_edp_phy {
+	status = "okay";
+
+	vdda-1p2-supply = <&vreg_l6b_1p2>;
+	vdda-0p9-supply = <&vreg_l10c_0p8>;
+};
+
+&mdss_dp {
+	status = "okay";
+
+	pinctrl-names = "default";
+	pinctrl-0 = <&dp_hot_plug_det>;
+	data-lanes = <0 1>;
+	vdda-1p2-supply = <&vreg_l6b_1p2>;
+	vdda-0p9-supply = <&vreg_l1b_0p8>;
+};
+
 &nvme_3v3_regulator {
 	gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>;
 };
@@ -72,7 +113,93 @@  ap_ts_pen_1v8: &i2c13 {
 	pins = "gpio51";
 };
 
+&pm8350c_pwm{
+	status = "okay";
+};
+
+&pm8350c_gpios {
+	backlight_pwm_default: backlight_pwm_default {
+		pinconf {
+			pins = "gpio8";
+			function = "func1";
+			output-low;
+			bias-disable;
+			power-source = <0>;
+			qcom,drive-strength = <3>;
+		};
+	};
+};
+
+&soc {
+	backlight_power: backlight_power {
+		compatible = "regulator-fixed";
+		regulator-name = "backlight_power";
+		regulator-always-on;
+		regulator-boot-on;
+	};
+
+	edp_power: edp_power {
+		compatible = "regulator-fixed";
+		regulator-name = "edp_power";
+
+		regulator-min-microvolt = <3300000>;
+		regulator-max-microvolt = <3300000>;
+
+		gpio = <&tlmm 80 GPIO_ACTIVE_HIGH>;
+		enable-active-high;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&edp_panel_power_default>;
+	};
+
+	edp_backlight: edp_backlight {
+		compatible = "pwm-backlight";
+
+		pwms = <&pm8350c_pwm 3 65535>;
+		power-supply = <&backlight_power>;
+		enable-gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>;
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&backlight_pwm_default>;
+	};
+
+	edp_panel: edp_panel {
+		compatible = "sharp_lq140m1jw46";
+
+		pinctrl-names = "default";
+		pinctrl-0 = <&edp_hot_plug_det>;
+
+		power-supply = <&edp_power>;
+		backlight = <&edp_backlight>;
+
+		ports {
+			#address-cells = <1>;
+			#size-cells = <0>;
+			port@0 {
+				reg = <0>;
+				edp_panel_in: endpoint {
+					remote-endpoint = <&edp_out>;
+				};
+			};
+		};
+	};
+};
+
 &tlmm {
+	edp_hot_plug_det: edp-hot-plug-det {
+		pins = "gpio60";
+		function = "edp_hot";
+		bias-pull-down;
+		input-enable;
+	};
+
+	edp_panel_power_default: edp_panel_power_default {
+		pins = "gpio80";
+		function = "gpio";
+		drive-strength = <2>;
+		output-high;
+	};
+
 	tp_int_odl: tp-int-odl {
 		pins = "gpio7";
 		function = "gpio";