| Message ID | 1644494255-6632-3-git-send-email-quic_sbillaka@quicinc.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add support for the eDP panel on sc7280 CRD | expand |
On Thu 10 Feb 05:57 CST 2022, Sankeerth Billakanti wrote: > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > Changes in v4: > - Create new patch for name changes > - Remove output-low > > Changes in v3: > - Sort the nodes alphabetically > - Use - instead of _ as node names > - Place the backlight and panel nodes under root > - Change the name of edp_out to mdss_edp_out > - Change the names of regulator nodes > - Delete unused properties in the board file > > > Changes in v2: > - Sort node references alphabetically > - Improve readability > - Move the pwm pinctrl to pwm node > - Move the regulators to root > - Define backlight power > - Remove dummy regulator node > - Cleanup pinctrl definitions > > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 120 ++++++++++++++++++++++++++++++++ > 1 file changed, 120 insertions(+) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..6dba5ac 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -21,6 +21,59 @@ > chosen { > stdout-path = "serial0:115200n8"; > }; > + > + backlight_3v3_regulator: backlight-3v3-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "backlight_3v3_regulator"; > + > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&edp_bl_power>; > + }; > + > + 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>; > + }; > + > + edp_backlight: edp-backlight { > + compatible = "pwm-backlight"; > + > + power-supply = <&backlight_3v3_regulator>; > + pwms = <&pm8350c_pwm 3 65535>; > + }; > + > + edp_panel: edp-panel { > + compatible = "sharp,lq140m1jw46"; > + > + power-supply = <&edp_3v3_regulator>; > + backlight = <&edp_backlight>; > + > + ports { > + #address-cells = <1>; > + #size-cells = <0>; > + port@0 { > + reg = <0>; > + edp_panel_in: endpoint { > + remote-endpoint = <&edp_out>; > + }; > + }; > + }; > + }; > }; > > &apps_rsc { > @@ -76,6 +129,44 @@ ap_ts_pen_1v8: &i2c13 { > }; > }; > > +&edp_out { > + remote-endpoint = <&edp_panel_in>; > +}; > + > +&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"; > + > + vdda-1p2-supply = <&vreg_l6b_1p2>; > + vdda-0p9-supply = <&vreg_l10c_0p8>; > + /delete-property/ pinctrl-names; > + /delete-property/ pinctrl-0; If the first device to enable &mdss_edp overwrites pinctrl-{names,0} in &mdss_dp and removes the properties in &mdss_edp, I think that's a sign that they should not be in the .dtsi in the first place. > +}; > + > +&mdss_edp_phy { > + status = "okay"; > + > + vdda-1p2-supply = <&vreg_l6b_1p2>; > + vdda-0p9-supply = <&vreg_l10c_0p8>; > +}; > + > +&mdss_mdp { > + status = "okay"; > +}; > + > &nvme_3v3_regulator { > gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>; > }; > @@ -84,7 +175,36 @@ ap_ts_pen_1v8: &i2c13 { > pins = "gpio51"; > }; > > +&pm8350c_gpios { > + edp_bl_power: edp-bl-power { > + pins = "gpio7"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; > + bias-pull-down; Why do you pull down these two pins? They are both outputs. > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; > + bias-pull-down; > + }; > +}; > + > +&pm8350c_pwm { As stated previously, this will prevent me from merging this patch until the LPG/PWM support has been accepted. As such I would recommend that you drop the backlight parts of this patch until that has landed - so we can merge the rest of this in the meantime. > + status = "okay"; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&edp_bl_pwm>; > +}; > + > &tlmm { > + edp_panel_power: edp-panel-power { > + pins = "gpio80"; > + function = "gpio"; > + bias-pull-down; Same here, why is this pulled down? Thanks, Bjorn > + }; > + > tp_int_odl: tp-int-odl { > pins = "gpio7"; > function = "gpio"; > -- > 2.7.4 >
Hi, On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti <quic_sbillaka@quicinc.com> wrote: > > +&pm8350c_gpios { > + edp_bl_power: edp-bl-power { > + pins = "gpio7"; > + function = "normal"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; As far as I can tell you're lacking: #include <dt-bindings/pinctrl/qcom,pmic-gpio.h> ...which is needed to use PMIC_GPIO_STRENGTH_LOW. I'm curious how you're compiling?
Hi, On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti <quic_sbillaka@quicinc.com> wrote: > > + backlight_3v3_regulator: backlight-3v3-regulator { > + compatible = "regulator-fixed"; > + regulator-name = "backlight_3v3_regulator"; > + > + regulator-min-microvolt = <3300000>; > + regulator-max-microvolt = <3300000>; > + > + gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; > + enable-active-high; > + > + pinctrl-names = "default"; > + pinctrl-0 = <&edp_bl_power>; > + }; So I'm pretty sure that this is wrong and what you had on a previous patch was more correct. Specifically the PMIC's GPIO 7 truly _is_ an enable pin for the backlight. In the schematics I see it's named as "PMIC_EDP_BL_EN" and is essentially the same net as "EDP_BL_EN". This is distinct from the backlight _regulator_ that is named VREG_EDP_BP. I believe the VREG_EDP_BP is essentially sourced directly from PPVAR_SYS. That's how it works on herobrine and I believe that CRD is the same. You currently don't model ppvar_sys, but it's basically just a variable-voltage rail that could be provided somewhat directly from the battery or could be provided from Type C components. I believe that the panel backlight is designed to handle this fairly wide voltage range and it's done this way to get the best efficiency. So personally I'd prefer if you do something like herobrine and model PPVAR_SYS. Then the backlight can use ppvar_sys as its regulator and you can go back to providing this as an "enable" pin for the backlight. I know, technically it doesn't _really_ matter, but it's nice to model it more correctly.
On Thu 17 Feb 17:03 PST 2022, Doug Anderson wrote: > Hi, > > On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti > <quic_sbillaka@quicinc.com> wrote: > > > > + backlight_3v3_regulator: backlight-3v3-regulator { > > + compatible = "regulator-fixed"; > > + regulator-name = "backlight_3v3_regulator"; > > + > > + regulator-min-microvolt = <3300000>; > > + regulator-max-microvolt = <3300000>; > > + > > + gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; > > + enable-active-high; > > + > > + pinctrl-names = "default"; > > + pinctrl-0 = <&edp_bl_power>; > > + }; > > So I'm pretty sure that this is wrong and what you had on a previous > patch was more correct. Specifically the PMIC's GPIO 7 truly _is_ an > enable pin for the backlight. In the schematics I see it's named as > "PMIC_EDP_BL_EN" and is essentially the same net as "EDP_BL_EN". This > is distinct from the backlight _regulator_ that is named VREG_EDP_BP. > I believe the VREG_EDP_BP is essentially sourced directly from > PPVAR_SYS. That's how it works on herobrine and I believe that CRD is > the same. You currently don't model ppvar_sys, but it's basically just > a variable-voltage rail that could be provided somewhat directly from > the battery or could be provided from Type C components. I believe > that the panel backlight is designed to handle this fairly wide > voltage range and it's done this way to get the best efficiency. > > So personally I'd prefer if you do something like herobrine and model > PPVAR_SYS. Then the backlight can use ppvar_sys as its regulator and > you can go back to providing this as an "enable" pin for the > backlight. > > I know, technically it doesn't _really_ matter, but it's nice to model > it more correctly. While I've not seen your schematics, the proposal does look similar to what I have on sc8180x, where there's a power rail, the BL_EN and a pwm signal. If that's the case I think representing BL_EN using the enable-gpios property directly in the pwm-backlight node seems more appropriate (with power-supply being the actual thing that powers the backlight). If however gpio 7 is wired to something like the enable-pin on an actual LDO the proposal here seems reasonable, but it seems unlikely that the output of that would be named "backlight_3v3_regulator"? Regards, Bjorn
Hi, On Thu, Feb 10, 2022 at 4:04 PM Bjorn Andersson <bjorn.andersson@linaro.org> wrote: > > > +&mdss_edp { > > + status = "okay"; > > + > > + vdda-1p2-supply = <&vreg_l6b_1p2>; > > + vdda-0p9-supply = <&vreg_l10c_0p8>; > > + /delete-property/ pinctrl-names; > > + /delete-property/ pinctrl-0; > > If the first device to enable &mdss_edp overwrites pinctrl-{names,0} in > &mdss_dp and removes the properties in &mdss_edp, I think that's a sign > that they should not be in the .dtsi in the first place. Actually, I just looked more carefully here. I think the "/delete-property" for edp_hpd here is just wrong. I'm pretty sure that the HPD signal is hooked up on CRD and we actually need it. If somehow deleting the property helps you then it's probably just hacking around a bug and relying on the panel to be always powered on, or something. I think this gets into some of the stuff in your final patch in this series. I found that, on my hardware, the panel doesn't come up at all with that final patch. When I go back to how things were working in an earlier version of your series, though, I can get things working a little better (though still not perfect). -Doug
diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts index e2efbdd..6dba5ac 100644 --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts @@ -21,6 +21,59 @@ chosen { stdout-path = "serial0:115200n8"; }; + + backlight_3v3_regulator: backlight-3v3-regulator { + compatible = "regulator-fixed"; + regulator-name = "backlight_3v3_regulator"; + + regulator-min-microvolt = <3300000>; + regulator-max-microvolt = <3300000>; + + gpio = <&pm8350c_gpios 7 GPIO_ACTIVE_HIGH>; + enable-active-high; + + pinctrl-names = "default"; + pinctrl-0 = <&edp_bl_power>; + }; + + 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>; + }; + + edp_backlight: edp-backlight { + compatible = "pwm-backlight"; + + power-supply = <&backlight_3v3_regulator>; + pwms = <&pm8350c_pwm 3 65535>; + }; + + edp_panel: edp-panel { + compatible = "sharp,lq140m1jw46"; + + power-supply = <&edp_3v3_regulator>; + backlight = <&edp_backlight>; + + ports { + #address-cells = <1>; + #size-cells = <0>; + port@0 { + reg = <0>; + edp_panel_in: endpoint { + remote-endpoint = <&edp_out>; + }; + }; + }; + }; }; &apps_rsc { @@ -76,6 +129,44 @@ ap_ts_pen_1v8: &i2c13 { }; }; +&edp_out { + remote-endpoint = <&edp_panel_in>; +}; + +&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"; + + vdda-1p2-supply = <&vreg_l6b_1p2>; + vdda-0p9-supply = <&vreg_l10c_0p8>; + /delete-property/ pinctrl-names; + /delete-property/ pinctrl-0; +}; + +&mdss_edp_phy { + status = "okay"; + + vdda-1p2-supply = <&vreg_l6b_1p2>; + vdda-0p9-supply = <&vreg_l10c_0p8>; +}; + +&mdss_mdp { + status = "okay"; +}; + &nvme_3v3_regulator { gpio = <&tlmm 51 GPIO_ACTIVE_HIGH>; }; @@ -84,7 +175,36 @@ ap_ts_pen_1v8: &i2c13 { pins = "gpio51"; }; +&pm8350c_gpios { + edp_bl_power: edp-bl-power { + pins = "gpio7"; + function = "normal"; + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; + bias-pull-down; + }; + + edp_bl_pwm: edp-bl-pwm { + pins = "gpio8"; + function = "func1"; + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; + bias-pull-down; + }; +}; + +&pm8350c_pwm { + status = "okay"; + + pinctrl-names = "default"; + pinctrl-0 = <&edp_bl_pwm>; +}; + &tlmm { + edp_panel_power: edp-panel-power { + pins = "gpio80"; + function = "gpio"; + bias-pull-down; + }; + tp_int_odl: tp-int-odl { pins = "gpio7"; function = "gpio";
Enable the eDP display panel support without HPD on sc7280 platform. Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> --- Changes in v4: - Create new patch for name changes - Remove output-low Changes in v3: - Sort the nodes alphabetically - Use - instead of _ as node names - Place the backlight and panel nodes under root - Change the name of edp_out to mdss_edp_out - Change the names of regulator nodes - Delete unused properties in the board file Changes in v2: - Sort node references alphabetically - Improve readability - Move the pwm pinctrl to pwm node - Move the regulators to root - Define backlight power - Remove dummy regulator node - Cleanup pinctrl definitions arch/arm64/boot/dts/qcom/sc7280-crd.dts | 120 ++++++++++++++++++++++++++++++++ 1 file changed, 120 insertions(+)