[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <YgWoAwdH/AqJUshh@builder.lan>
Date: Thu, 10 Feb 2022 18:04:19 -0600
From: Bjorn Andersson <bjorn.andersson@...aro.org>
To: Sankeerth Billakanti <quic_sbillaka@...cinc.com>
Cc: dri-devel@...ts.freedesktop.org, linux-arm-msm@...r.kernel.org,
freedreno@...ts.freedesktop.org, linux-kernel@...r.kernel.org,
devicetree@...r.kernel.org, agross@...nel.org, robh+dt@...nel.org,
robdclark@...il.com, seanpaul@...omium.org, swboyd@...omium.org,
dianders@...omium.org, krzysztof.kozlowski@...onical.com,
thierry.reding@...il.com, sam@...nborg.org, airlied@...ux.ie,
daniel@...ll.ch, quic_kalyant@...cinc.com,
quic_abhinavk@...cinc.com, quic_khsieh@...cinc.com,
quic_mkrishn@...cinc.com, quic_vproddut@...cinc.com
Subject: Re: [PATCH v4 2/5] arm64: dts: qcom: sc7280: Add support for eDP
panel on CRD
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@...cinc.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
>
Powered by blists - more mailing lists