> -----Original Message-----
> From: Stephen Boyd <swboyd@xxxxxxxxxxxx>
> Sent: Friday, March 18, 2022 2:53 AM
> To: Sankeerth Billakanti (QUIC) <quic_sbillaka@xxxxxxxxxxx>;
> devicetree@xxxxxxxxxxxxxxx; dri-devel@xxxxxxxxxxxxxxxxxxxxx;
> freedreno@xxxxxxxxxxxxxxxxxxxxx; linux-arm-msm@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Cc: robdclark@xxxxxxxxx; seanpaul@xxxxxxxxxxxx; quic_kalyant
> <quic_kalyant@xxxxxxxxxxx>; Abhinav Kumar (QUIC)
> <quic_abhinavk@xxxxxxxxxxx>; dianders@xxxxxxxxxxxx; Kuogee Hsieh
> (QUIC) <quic_khsieh@xxxxxxxxxxx>; agross@xxxxxxxxxx;
> bjorn.andersson@xxxxxxxxxx; robh+dt@xxxxxxxxxx; krzk+dt@xxxxxxxxxx;
> sean@xxxxxxxxxx; airlied@xxxxxxxx; daniel@xxxxxxxx;
> thierry.reding@xxxxxxxxx; sam@xxxxxxxxxxxx;
> dmitry.baryshkov@xxxxxxxxxx; quic_vproddut <quic_vproddut@xxxxxxxxxxx>
> Subject: Re: [PATCH v5 2/9] arm64: dts: qcom: sc7280: Add support for eDP
> panel on CRD
>
> Quoting Sankeerth Billakanti (2022-03-16 10:35:47)
> > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > index e2efbdd..2df654e 100644
> > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts
> > @@ -7,6 +7,7 @@
> >
> > /dts-v1/;
> >
> > +#include <dt-bindings/pinctrl/qcom,pmic-gpio.h>
> > #include "sc7280-idp.dtsi"
> > #include "sc7280-idp-ec-h1.dtsi"
> >
> > @@ -21,6 +22,27 @@
> > chosen {
> > stdout-path = "serial0:115200n8";
> > };
> > +
> > + edp_3v3_regulator: edp-3v3-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "edp_3v3_regulator";
> > +
> > + 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>;
> > + };
> > +
> > + vreg_edp_bp: vreg-edp-bp-regulator {
> > + compatible = "regulator-fixed";
> > + regulator-name = "vreg_edp_bp";
> > + regulator-always-on;
> > + regulator-boot-on;
> > + };
> > };
> >
> > &apps_rsc {
> > @@ -76,6 +98,58 @@ ap_ts_pen_1v8: &i2c13 {
> > };
> > };
> >
> > +&mdss {
> > + status = "okay";
> > +};
> > +
> > +&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>; };
> > +
> > +&mdss_edp {
> > + status = "okay";
> > +
> > + data-lanes = <0 1 2 3>;
>
> Is this property necessary? It looks like the default.
>
Will remove it
> > + vdda-1p2-supply = <&vreg_l6b_1p2>;
> > + vdda-0p9-supply = <&vreg_l10c_0p8>;
> > +
> > + aux-bus {
>
> Can this move to sc7280.dtsi and get a phandle?
>
Okay, I will move this to sc7280.dtsi like below.
Shall I define the required properties under &mdss_edp_panel node in the sc7280-crd3.dts?
--- a/arch/arm64/boot/dts/qcom/sc7280.dtsi
+++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi
@@ -3283,6 +3283,18 @@
status = "disabled";
+ aux-bus {
+ mdss_edp_panel: panel {
+ compatible = "edp-panel";
+
+ port {
+ mdss_edp_panel_in: endpoint {
+ remote-endpoint = <&mdss_edp_out>;
+ };
+ };
+ };
+ };
+
ports {
#address-cells = <1>;
#size-cells = <0>;
@@ -3296,7 +3308,9 @@
port@1 {
reg = <1>;
- mdss_edp_out: endpoint { };
+ mdss_edp_out: endpoint {
+ remote-endpoint = <&mdss_edp_panel_in>;
+ };
};
};
> > + edp_panel: edp-panel {
>
> I'd prefer
>
> edp_panel: panel {
>
> because there's only one panel node at this level.
>
Okay. I will change it.
> > + compatible = "edp-panel";
> > +
> > + power-supply = <&edp_3v3_regulator>;
>
> This is board specific, but I thought it was on the qcard so we should move
> this to sc7280-qcard.dtsi?
>
Qcard is used only for herobrine as of now according to the code. We defined this only for CRD. We will discuss this internally to understand the plan ahead.
> > + ports {
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > + port@0 {
> > + reg = <0>;
> > + edp_panel_in: endpoint {
>
> This can be shortened to
>
> port {
> edp_panel_in: endpoint {
>
> according to panel-edp.yaml
>
Okay. I will do it
> > + remote-endpoint = <&mdss_edp_out>;
> > + };
> > + };
> > + };
> > + };
> > + };
> > +};
> > +
> > +&mdss_edp_out {
> > + remote-endpoint = <&edp_panel_in>; };
> > +
> > +&mdss_edp_phy {
> > + status = "okay";
> > +};
> > +
> > +&mdss_mdp {
> > + status = "okay";
> > +};
> > +
> > &nvme_3v3_regulator {
> > gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>; }; @@ -84,7 +158,26 @@
> > ap_ts_pen_1v8: &i2c13 {
> > pins = "gpio51";
> > };
> >
> > +&pm8350c_gpios {
> > + edp_bl_power: edp-bl-power {
>
> Is this used in a later patch?
>
Yes, will move it to that patch.
> > + pins = "gpio7";
> > + function = "normal";
> > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
> > + };
> > +
> > + edp_bl_pwm: edp-bl-pwm {
>
> Is this used in a later patch? Can it be moved to the patch that uses it?
>
Yes, will move it to that patch. We split the patch to exclude the dependent pwm nodes so that Bjorn can merge this patch. But we will move all the related nodes to the next patch
> > + pins = "gpio8";
> > + function = "func1";
> > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>;
> > + };
> > +};
> > +
> > &tlmm {
> > + edp_panel_power: edp-panel-power {
> > + pins = "gpio80";
> > + function = "gpio";
>
> function of gpio is unnecessary. Where is the bias and drive-strength
> settings?
>
Will add it
> > + };
> > +
> > tp_int_odl: tp-int-odl {
> > pins = "gpio7";
> > function = "gpio";
> > --
> > 2.7.4
> >
