| Message ID | 1644494255-6632-1-git-send-email-quic_sbillaka@quicinc.com |
|---|---|
| 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 >
On Thu 10 Feb 05:57 CST 2022, Sankeerth Billakanti wrote: > Rename the edp_out label in the sc7280 platform to mdss_edp_out. Next week, or in the next product, it might not be obvious why we did this change. So please continue this sentence with something like "so that the nodes are grouped together when sorted in the dts". > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > arch/arm64/boot/dts/qcom/sc7280-crd.dts | 10 +++++----- > arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +- > 2 files changed, 6 insertions(+), 6 deletions(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index 6dba5ac..af40e14 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > +++ b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > @@ -69,7 +69,7 @@ > port@0 { > reg = <0>; > edp_panel_in: endpoint { > - remote-endpoint = <&edp_out>; > + remote-endpoint = <&mdss_edp_out>; > }; > }; > }; > @@ -129,10 +129,6 @@ ap_ts_pen_1v8: &i2c13 { > }; > }; > > -&edp_out { You just added this node in patch 2 and now you change it immediately. If you reorder the two patches the history will be cleaner. Thanks, Bjorn > - remote-endpoint = <&edp_panel_in>; > -}; > - > &mdss { > status = "okay"; > }; > @@ -156,6 +152,10 @@ ap_ts_pen_1v8: &i2c13 { > /delete-property/ pinctrl-0; > }; > > +&mdss_edp_out { > + remote-endpoint = <&edp_panel_in>; > +}; > + > &mdss_edp_phy { > status = "okay"; > > diff --git a/arch/arm64/boot/dts/qcom/sc7280.dtsi b/arch/arm64/boot/dts/qcom/sc7280.dtsi > index 3572399..eca403a 100644 > --- a/arch/arm64/boot/dts/qcom/sc7280.dtsi > +++ b/arch/arm64/boot/dts/qcom/sc7280.dtsi > @@ -3066,7 +3066,7 @@ > > port@1 { > reg = <1>; > - edp_out: endpoint { }; > + mdss_edp_out: endpoint { }; > }; > }; > > -- > 2.7.4 >
Hi, On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti <quic_sbillaka@quicinc.com> wrote: > > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > 00 ff ff ff ff ff ff 00 4d 10 23 15 00 00 00 00 > 35 1e 01 04 a5 1f 11 78 07 de 50 a3 54 4c 99 26 > 0f 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > 01 01 01 01 01 01 5a 87 80 a0 70 38 4d 40 30 20 > 35 00 35 ae 10 00 00 18 65 38 80 a0 70 38 4d 40 > 30 20 35 00 35 ae 10 00 00 18 00 00 00 fd 00 30 > 90 a7 a7 23 01 00 00 00 00 00 00 00 00 00 00 fc > 00 4c 51 31 34 30 4d 31 4a 57 34 39 0a 20 00 77 > > ---------------- > > Block 0, Base EDID: > EDID Structure Version & Revision: 1.4 > Vendor & Product Identification: > Manufacturer: SHP > Model: 5411 > Made in: week 53 of 2020 > Basic Display Parameters & Features: > Digital display > Bits per primary color channel: 8 > DisplayPort interface > Maximum image size: 31 cm x 17 cm > Gamma: 2.20 > Supported color formats: RGB 4:4:4 > Default (sRGB) color space is primary color space > First detailed timing includes the native pixel format and preferred refresh rate > Display is continuous frequency > Color Characteristics: > Red : 0.6396, 0.3291 > Green: 0.2998, 0.5996 > Blue : 0.1494, 0.0595 > White: 0.3125, 0.3281 > Established Timings I & II: none > Standard Timings: none > Detailed Timing Descriptors: > DTD 1: 1920x1080 143.981 Hz 16:9 166.587 kHz 346.500 MHz (309 mm x 174 mm) > Hfront 48 Hsync 32 Hback 80 Hpol N > Vfront 3 Vsync 5 Vback 69 Vpol N > DTD 2: 1920x1080 59.990 Hz 16:9 69.409 kHz 144.370 MHz (309 mm x 174 mm) > Hfront 48 Hsync 32 Hback 80 Hpol N > Vfront 3 Vsync 5 Vback 69 Vpol N > Display Range Limits: > Monitor ranges (Bare Limits): 48-144 Hz V, 167-167 kHz H, max dotclock 350 MHz > Display Product Name: 'LQ140M1JW49' > Checksum: 0x77 > > Changes in v4: > -Add all modes from EDID > -Provide EDID blob > > Changes in v3: > None > > drivers/gpu/drm/panel/panel-edp.c | 44 +++++++++++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) We want to be moving to the generic edp-panel but even if we move to edp-panel there's no harm in supporting things the old way, especially as people are transitioning. Reviewed-by: Douglas Anderson <dianders@chromium.org>
Hi, On Wed, Feb 16, 2022 at 11:29 AM Doug Anderson <dianders@chromium.org> wrote: > > Hi, > > On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti > <quic_sbillaka@quicinc.com> wrote: > > > > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > > --- > > 00 ff ff ff ff ff ff 00 4d 10 23 15 00 00 00 00 > > 35 1e 01 04 a5 1f 11 78 07 de 50 a3 54 4c 99 26 > > 0f 50 54 00 00 00 01 01 01 01 01 01 01 01 01 01 > > 01 01 01 01 01 01 5a 87 80 a0 70 38 4d 40 30 20 > > 35 00 35 ae 10 00 00 18 65 38 80 a0 70 38 4d 40 > > 30 20 35 00 35 ae 10 00 00 18 00 00 00 fd 00 30 > > 90 a7 a7 23 01 00 00 00 00 00 00 00 00 00 00 fc > > 00 4c 51 31 34 30 4d 31 4a 57 34 39 0a 20 00 77 > > > > ---------------- > > > > Block 0, Base EDID: > > EDID Structure Version & Revision: 1.4 > > Vendor & Product Identification: > > Manufacturer: SHP > > Model: 5411 > > Made in: week 53 of 2020 > > Basic Display Parameters & Features: > > Digital display > > Bits per primary color channel: 8 > > DisplayPort interface > > Maximum image size: 31 cm x 17 cm > > Gamma: 2.20 > > Supported color formats: RGB 4:4:4 > > Default (sRGB) color space is primary color space > > First detailed timing includes the native pixel format and preferred refresh rate > > Display is continuous frequency > > Color Characteristics: > > Red : 0.6396, 0.3291 > > Green: 0.2998, 0.5996 > > Blue : 0.1494, 0.0595 > > White: 0.3125, 0.3281 > > Established Timings I & II: none > > Standard Timings: none > > Detailed Timing Descriptors: > > DTD 1: 1920x1080 143.981 Hz 16:9 166.587 kHz 346.500 MHz (309 mm x 174 mm) > > Hfront 48 Hsync 32 Hback 80 Hpol N > > Vfront 3 Vsync 5 Vback 69 Vpol N > > DTD 2: 1920x1080 59.990 Hz 16:9 69.409 kHz 144.370 MHz (309 mm x 174 mm) > > Hfront 48 Hsync 32 Hback 80 Hpol N > > Vfront 3 Vsync 5 Vback 69 Vpol N > > Display Range Limits: > > Monitor ranges (Bare Limits): 48-144 Hz V, 167-167 kHz H, max dotclock 350 MHz > > Display Product Name: 'LQ140M1JW49' > > Checksum: 0x77 > > > > Changes in v4: > > -Add all modes from EDID > > -Provide EDID blob > > > > Changes in v3: > > None > > > > drivers/gpu/drm/panel/panel-edp.c | 44 +++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 44 insertions(+) > > We want to be moving to the generic edp-panel but even if we move to > edp-panel there's no harm in supporting things the old way, especially > as people are transitioning. > > Reviewed-by: Douglas Anderson <dianders@chromium.org> ...and pushed to drm-misc-next: a874aba8bbc5 drm/panel-edp: Add eDP sharp panel support So v5 shouldn't include this patch. -Doug
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
Hi, On Thu, Feb 10, 2022 at 3:58 AM Sankeerth Billakanti <quic_sbillaka@quicinc.com> wrote: > > Add support in the DP driver to utilize the custom eDP panels > from drm/panels. > > An eDP panel is always connected to the platform. So, the eDP > connector can be reported as always connected. The display mode > will be sourced from the panel. The panel mode will be set after > the link training is completed. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > Changes in v4: > - Remove obvious comments > - Define separate connector_ops for eDP > - Remove unnecessary checks > > Changes in v3: > None > > drivers/gpu/drm/msm/dp/dp_display.c | 6 ++++ > drivers/gpu/drm/msm/dp/dp_drm.c | 62 +++++++++++++++++++++++++++++++------ > drivers/gpu/drm/msm/dp/dp_parser.h | 3 ++ > 3 files changed, 61 insertions(+), 10 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 7cc4d21..5d314e6 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1513,6 +1513,9 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > return -EINVAL; > } > > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); I'm really not so sure here. You're just totally ignoring the HPD signal here which isn't right at all. The HPD signal is important for knowing if an edp panel is ready yet so you can't just ignore it. The only way this could work is if something else turns the panel on w/ plenty of time before your code runs so it has had time to get ready... It feels like we just need to work to get this all plumbed up properly with the right power sequencing. That'll also allow us to enable the generic edp-panel stuff... > +static int edp_connector_get_modes(struct drm_connector *connector) > +{ > + struct msm_dp *dp; > + > + dp = to_dp_connector(connector)->dp_display; > + > + return drm_bridge_get_modes(dp->panel_bridge, connector); > +} > + > +static enum drm_mode_status edp_connector_mode_valid( > + struct drm_connector *connector, > + struct drm_display_mode *mode) > +{ > + if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ) > + return MODE_CLOCK_HIGH; > + > + return MODE_OK; > +} > + > static const struct drm_connector_funcs dp_connector_funcs = { > .detect = dp_connector_detect, > .fill_modes = drm_helper_probe_single_connector_modes, > @@ -132,11 +151,24 @@ static const struct drm_connector_funcs dp_connector_funcs = { > .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > }; > > +static const struct drm_connector_funcs edp_connector_funcs = { > + .fill_modes = drm_helper_probe_single_connector_modes, > + .destroy = drm_connector_cleanup, > + .reset = drm_atomic_helper_connector_reset, > + .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state, > + .atomic_destroy_state = drm_atomic_helper_connector_destroy_state, > +}; > + > static const struct drm_connector_helper_funcs dp_connector_helper_funcs = { > .get_modes = dp_connector_get_modes, > .mode_valid = dp_connector_mode_valid, > }; > > +static const struct drm_connector_helper_funcs edp_connector_helper_funcs = { > + .get_modes = edp_connector_get_modes, > + .mode_valid = edp_connector_mode_valid, > +}; > + > /* connector initialization */ > struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > { > @@ -154,18 +186,28 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > > connector = &dp_connector->base; > > - ret = drm_connector_init(dp_display->drm_dev, connector, > - &dp_connector_funcs, > - dp_display->connector_type); > - if (ret) > - return ERR_PTR(ret); > + if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) { > + ret = drm_connector_init(dp_display->drm_dev, connector, > + &edp_connector_funcs, DRM_MODE_CONNECTOR_eDP); > + if (ret) > + return ERR_PTR(ret); > + > + drm_connector_helper_add(connector, > + &edp_connector_helper_funcs); > + } else { > + ret = drm_connector_init(dp_display->drm_dev, connector, > + &dp_connector_funcs, > + DRM_MODE_CONNECTOR_DisplayPort); > + if (ret) > + return ERR_PTR(ret); > > - drm_connector_helper_add(connector, &dp_connector_helper_funcs); > + drm_connector_helper_add(connector, &dp_connector_helper_funcs); This is probably not the correct way to do this. Drivers like this should really be moving _away_ from creating their own connectors. The idea is that you should just be creating bridges and then someone creates a "bridge connector" that implements the connector functions atop the bridge. This is what Dmitry is working on [1]. Speaking of which, he really ought to be CCed on all your patches. > - /* > - * Enable HPD to let hpd event is handled when cable is connected. > - */ > - connector->polled = DRM_CONNECTOR_POLL_HPD; > + /* > + * Enable HPD to let hpd event is handled when cable is connected. > + */ > + connector->polled = DRM_CONNECTOR_POLL_HPD; > + } > > drm_connector_attach_encoder(connector, dp_display->encoder); > > diff --git a/drivers/gpu/drm/msm/dp/dp_parser.h b/drivers/gpu/drm/msm/dp/dp_parser.h > index 3172da0..58c4f27 100644 > --- a/drivers/gpu/drm/msm/dp/dp_parser.h > +++ b/drivers/gpu/drm/msm/dp/dp_parser.h > @@ -17,6 +17,9 @@ > #define DP_MAX_PIXEL_CLK_KHZ 675000 > #define DP_MAX_NUM_DP_LANES 4 > > +/* Maximum validated clock */ > +#define EDP_MAX_PIXEL_CLK_KHZ 285550 As discussed out-of-band, this isn't my favorite define. The datasheet for sc7280, which is what you're testing on / targeting, claims to support higher rates. Other users of this driver also ought to support higher rates. It might be OK short term, but it's definitely not a good long term solution. [1] https://lore.kernel.org/all/20220211224006.1797846-5-dmitry.baryshkov@linaro.org/