| Message ID | 1644396932-17932-1-git-send-email-quic_sbillaka@quicinc.com |
|---|---|
| Headers | show |
| Series | Add support for the eDP panel on sc7280 CRD | expand |
Quoting Sankeerth Billakanti (2022-02-09 00:55:30) > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > 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 | 122 ++++++++++++++++++++++++++++++++ > arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +- > 2 files changed, 123 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..2a490a0 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>; Where is edp_out now? > + }; > + }; > + }; > + }; > }; > > &apps_rsc { > @@ -76,6 +129,44 @@ 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"; > + > + vdda-1p2-supply = <&vreg_l6b_1p2>; > + vdda-0p9-supply = <&vreg_l10c_0p8>; > + /delete-property/ pinctrl-names; > + /delete-property/ pinctrl-0; > +}; > + > +&mdss_edp_out { > + remote-endpoint = <&edp_panel_in>; > +}; > + > +&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,38 @@ 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-disable; > + output-low; Is the 'output-low' necessary? > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; > + bias-disable; > + output-low; Same question. > + }; > +}; > + > +&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"; > 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 { }; > }; It feels like this should be a different patch for this phandle rename.
Quoting Sankeerth Billakanti (2022-02-09 00:55:31) > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- Can you share the edid-decode for this panel here under the cut "---"? It would help to see the timings and make sure everything matches.
Quoting Sankeerth Billakanti (2022-02-09 00:55:32) > 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 v3: > None > > drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++ > drivers/gpu/drm/msm/dp/dp_drm.c | 54 +++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/msm/dp/dp_parser.h | 3 +++ > 3 files changed, 60 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 7cc4d21..410fda4 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1513,6 +1513,10 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > return -EINVAL; > } > > + /* handle eDP on */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > @@ -1577,6 +1581,10 @@ int msm_dp_display_disable(struct msm_dp *dp, struct drm_encoder *encoder) > > dp_display = container_of(dp, struct dp_display_private, dp_display); > > + /* handle edp off */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_unplug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c b/drivers/gpu/drm/msm/dp/dp_drm.c > index d4d360d..12fa8c1 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -39,6 +39,10 @@ static enum drm_connector_status dp_connector_detect(struct drm_connector *conn, > > dp = to_dp_connector(conn)->dp_display; > > + /* eDP is always connected */ > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + return connector_status_connected; Why not implement different connector ops for eDP and then not implement this function at all in that case? > + > DRM_DEBUG_DP("is_connected = %s\n", > (dp->is_connected) ? "true" : "false"); > > @@ -123,6 +127,35 @@ static enum drm_mode_status dp_connector_mode_valid( > return dp_display_validate_mode(dp_disp, mode->clock); > } > > +static int edp_connector_get_modes(struct drm_connector *connector) > +{ > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + 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) > +{ > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + dp = to_dp_connector(connector)->dp_display; > + > + if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ) > + return MODE_BAD; Why not 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, > @@ -137,6 +170,12 @@ static const struct drm_connector_helper_funcs dp_connector_helper_funcs = { > .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, > + Why the extra newline? > +}; > + > /* connector initialization */ > struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > { > @@ -160,12 +199,17 @@ struct drm_connector *dp_drm_connector_init(struct msm_dp *dp_display) > if (ret) > return ERR_PTR(ret); > > - drm_connector_helper_add(connector, &dp_connector_helper_funcs); > + if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) { > + drm_connector_helper_add(connector, > + &edp_connector_helper_funcs); > + } else { > + drm_connector_helper_add(connector, &dp_connector_helper_funcs); > > - /* > - * 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); >
Hi Stephen, Will implement all the suggested changes. Thank you, Sankeerth -----Original Message----- From: Stephen Boyd <swboyd@chromium.org> Sent: Thursday, February 10, 2022 6:45 AM To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; agross@kernel.org; airlied@linux.ie; bjorn.andersson@linaro.org; daniel@ffwll.ch; devicetree@vger.kernel.org; dianders@chromium.org; dri-devel@lists.freedesktop.org; freedreno@lists.freedesktop.org; krzysztof.kozlowski@canonical.com; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; robdclark@gmail.com; robh+dt@kernel.org; sam@ravnborg.org; seanpaul@chromium.org; thierry.reding@gmail.com Cc: quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; quic_mkrishn <quic_mkrishn@quicinc.com>; quic_vproddut <quic_vproddut@quicinc.com> Subject: Re: [PATCH v3 2/4] arm64: dts: qcom: sc7280: Add support for eDP panel on CRD Quoting Sankeerth Billakanti (2022-02-09 00:55:30) > Enable the eDP display panel support without HPD on sc7280 platform. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- > > 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 | 122 ++++++++++++++++++++++++++++++++ > arch/arm64/boot/dts/qcom/sc7280.dtsi | 2 +- > 2 files changed, 123 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/boot/dts/qcom/sc7280-crd.dts > b/arch/arm64/boot/dts/qcom/sc7280-crd.dts > index e2efbdd..2a490a0 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>; Where is edp_out now? > + }; > + }; > + }; > + }; > }; > > &apps_rsc { > @@ -76,6 +129,44 @@ 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"; > + > + vdda-1p2-supply = <&vreg_l6b_1p2>; > + vdda-0p9-supply = <&vreg_l10c_0p8>; > + /delete-property/ pinctrl-names; > + /delete-property/ pinctrl-0; > +}; > + > +&mdss_edp_out { > + remote-endpoint = <&edp_panel_in>; }; > + > +&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,38 @@ > 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-disable; > + output-low; Is the 'output-low' necessary? > + }; > + > + edp_bl_pwm: edp-bl-pwm { > + pins = "gpio8"; > + function = "func1"; > + qcom,drive-strength = <PMIC_GPIO_STRENGTH_LOW>; > + bias-disable; > + output-low; Same question. > + }; > +}; > + > +&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"; > 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 > + { }; > }; It feels like this should be a different patch for this phandle rename.
Hi Stephen, Thank you for the review. I will share the new patch. -----Original Message----- From: Stephen Boyd <swboyd@chromium.org> Sent: Thursday, February 10, 2022 6:47 AM To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; agross@kernel.org; airlied@linux.ie; bjorn.andersson@linaro.org; daniel@ffwll.ch; devicetree@vger.kernel.org; dianders@chromium.org; dri-devel@lists.freedesktop.org; freedreno@lists.freedesktop.org; krzysztof.kozlowski@canonical.com; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; robdclark@gmail.com; robh+dt@kernel.org; sam@ravnborg.org; seanpaul@chromium.org; thierry.reding@gmail.com Cc: quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; quic_mkrishn <quic_mkrishn@quicinc.com>; quic_vproddut <quic_vproddut@quicinc.com> Subject: Re: [PATCH v3 3/4] drm/panel-edp: Add eDP sharp panel support Quoting Sankeerth Billakanti (2022-02-09 00:55:31) > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> > --- Can you share the edid-decode for this panel here under the cut "---"? It would help to see the timings and make sure everything matches.
Hi Stephen, Will make the changes. -----Original Message----- From: Stephen Boyd <swboyd@chromium.org> Sent: Thursday, February 10, 2022 6:52 AM To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; agross@kernel.org; airlied@linux.ie; bjorn.andersson@linaro.org; daniel@ffwll.ch; devicetree@vger.kernel.org; dianders@chromium.org; dri-devel@lists.freedesktop.org; freedreno@lists.freedesktop.org; krzysztof.kozlowski@canonical.com; linux-arm-msm@vger.kernel.org; linux-kernel@vger.kernel.org; robdclark@gmail.com; robh+dt@kernel.org; sam@ravnborg.org; seanpaul@chromium.org; thierry.reding@gmail.com Cc: quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; quic_mkrishn <quic_mkrishn@quicinc.com>; quic_vproddut <quic_vproddut@quicinc.com>; dmitry.baryshkov@linaro.org Subject: Re: [PATCH v3 4/4] drm/msm/dp: Add driver support to utilize drm panel Quoting Sankeerth Billakanti (2022-02-09 00:55:32) > 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 v3: > None > > drivers/gpu/drm/msm/dp/dp_display.c | 8 ++++++ > drivers/gpu/drm/msm/dp/dp_drm.c | 54 +++++++++++++++++++++++++++++++++---- > drivers/gpu/drm/msm/dp/dp_parser.h | 3 +++ > 3 files changed, 60 insertions(+), 5 deletions(-) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c > b/drivers/gpu/drm/msm/dp/dp_display.c > index 7cc4d21..410fda4 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -1513,6 +1513,10 @@ int msm_dp_display_enable(struct msm_dp *dp, struct drm_encoder *encoder) > return -EINVAL; > } > > + /* handle eDP on */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_plug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > @@ -1577,6 +1581,10 @@ int msm_dp_display_disable(struct msm_dp *dp, > struct drm_encoder *encoder) > > dp_display = container_of(dp, struct dp_display_private, > dp_display); > > + /* handle edp off */ This comment is obvious. Please remove. > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + dp_hpd_unplug_handle(dp_display, 0); > + > mutex_lock(&dp_display->event_mutex); > > /* stop sentinel checking */ > diff --git a/drivers/gpu/drm/msm/dp/dp_drm.c > b/drivers/gpu/drm/msm/dp/dp_drm.c index d4d360d..12fa8c1 100644 > --- a/drivers/gpu/drm/msm/dp/dp_drm.c > +++ b/drivers/gpu/drm/msm/dp/dp_drm.c > @@ -39,6 +39,10 @@ static enum drm_connector_status > dp_connector_detect(struct drm_connector *conn, > > dp = to_dp_connector(conn)->dp_display; > > + /* eDP is always connected */ > + if (dp->connector_type == DRM_MODE_CONNECTOR_eDP) > + return connector_status_connected; Why not implement different connector ops for eDP and then not implement this function at all in that case? > + > DRM_DEBUG_DP("is_connected = %s\n", > (dp->is_connected) ? "true" : "false"); > > @@ -123,6 +127,35 @@ static enum drm_mode_status dp_connector_mode_valid( > return dp_display_validate_mode(dp_disp, mode->clock); } > > +static int edp_connector_get_modes(struct drm_connector *connector) { > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + 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) { > + struct msm_dp *dp; > + > + if (!connector) Is this check really necessary? Why doesn't drm do it in higher layers? > + return 0; > + > + dp = to_dp_connector(connector)->dp_display; > + > + if (mode->clock > EDP_MAX_PIXEL_CLK_KHZ) > + return MODE_BAD; Why not 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, > @@ -137,6 +170,12 @@ static const struct drm_connector_helper_funcs dp_connector_helper_funcs = { > .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, > + Why the extra newline? > +}; > + > /* connector initialization */ > struct drm_connector *dp_drm_connector_init(struct msm_dp > *dp_display) { @@ -160,12 +199,17 @@ struct drm_connector > *dp_drm_connector_init(struct msm_dp *dp_display) > if (ret) > return ERR_PTR(ret); > > - drm_connector_helper_add(connector, &dp_connector_helper_funcs); > + if (dp_display->connector_type == DRM_MODE_CONNECTOR_eDP) { > + drm_connector_helper_add(connector, > + &edp_connector_helper_funcs); > + } else { > + drm_connector_helper_add(connector, > + &dp_connector_helper_funcs); > > - /* > - * 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); >
On 09/02/2022 11:55, Sankeerth Billakanti wrote: > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> Please excuse my ignorance, is there any reason, why we can't use generic panel-edp here? > --- > > Changes in v3: > None > > drivers/gpu/drm/panel/panel-edp.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/panel/panel-edp.c b/drivers/gpu/drm/panel/panel-edp.c > index a394a15..5d13ccc 100644 > --- a/drivers/gpu/drm/panel/panel-edp.c > +++ b/drivers/gpu/drm/panel/panel-edp.c > @@ -1605,6 +1605,34 @@ static const struct panel_desc sharp_lq123p1jx31 = { > }, > }; > > +static const struct drm_display_mode sharp_lq140m1jw46_mode = { > + .clock = 144370, > + .hdisplay = 1920, > + .hsync_start = 1920 + 48, > + .hsync_end = 1920 + 48 + 32, > + .htotal = 1920 + 48 + 32 + 80, > + .vdisplay = 1080, > + .vsync_start = 1080 + 3, > + .vsync_end = 1080 + 3 + 5, > + .vtotal = 1080 + 3 + 5 + 69, > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC, > +}; > + > +static const struct panel_desc sharp_lq140m1jw46 = { > + .modes = &sharp_lq140m1jw46_mode, > + .num_modes = 1, > + .bpc = 8, > + .size = { > + .width = 309, > + .height = 174, > + }, > + .delay = { > + .hpd_absent = 80, > + .enable = 50, > + .unprepare = 500, > + }, > +}; > + > static const struct drm_display_mode starry_kr122ea0sra_mode = { > .clock = 147000, > .hdisplay = 1920, > @@ -1719,6 +1747,9 @@ static const struct of_device_id platform_of_match[] = { > .compatible = "sharp,lq123p1jx31", > .data = &sharp_lq123p1jx31, > }, { > + .compatible = "sharp,lq140m1jw46", > + .data = &sharp_lq140m1jw46, > + }, { > .compatible = "starry,kr122ea0sra", > .data = &starry_kr122ea0sra, > }, {
Hi Dmitry, As discussed over a separate email, we will be exploring using the generic panel edp and aux bus after implementing the basic PSR feature. We are using a non-generic compatible string now because we enabled PSR with this. The changes from aux-bus and generic-edp may be intrusive for the dp driver and hence we want to explore it as a separate task/feature after the basic PSR feature. Thank you, Sankeerth -----Original Message----- From: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> Sent: Thursday, February 10, 2022 5:55 PM To: Sankeerth Billakanti (QUIC) <quic_sbillaka@quicinc.com>; dri-devel@lists.freedesktop.org; linux-arm-msm@vger.kernel.org; freedreno@lists.freedesktop.org; linux-kernel@vger.kernel.org; devicetree@vger.kernel.org; agross@kernel.org; bjorn.andersson@linaro.org; robh+dt@kernel.org; robdclark@gmail.com; seanpaul@chromium.org; swboyd@chromium.org; dianders@chromium.org; krzysztof.kozlowski@canonical.com; thierry.reding@gmail.com; sam@ravnborg.org; airlied@linux.ie; daniel@ffwll.ch Cc: quic_kalyant <quic_kalyant@quicinc.com>; Abhinav Kumar (QUIC) <quic_abhinavk@quicinc.com>; Kuogee Hsieh (QUIC) <quic_khsieh@quicinc.com>; quic_mkrishn <quic_mkrishn@quicinc.com>; quic_vproddut <quic_vproddut@quicinc.com> Subject: Re: [PATCH v3 3/4] drm/panel-edp: Add eDP sharp panel support WARNING: This email originated from outside of Qualcomm. Please be wary of any links or attachments, and do not enable macros. On 09/02/2022 11:55, Sankeerth Billakanti wrote: > Add support for the 14" sharp,lq140m1jw46 eDP panel. > > Signed-off-by: Sankeerth Billakanti <quic_sbillaka@quicinc.com> Please excuse my ignorance, is there any reason, why we can't use generic panel-edp here? > --- > > Changes in v3: > None > > drivers/gpu/drm/panel/panel-edp.c | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/gpu/drm/panel/panel-edp.c > b/drivers/gpu/drm/panel/panel-edp.c > index a394a15..5d13ccc 100644 > --- a/drivers/gpu/drm/panel/panel-edp.c > +++ b/drivers/gpu/drm/panel/panel-edp.c > @@ -1605,6 +1605,34 @@ static const struct panel_desc sharp_lq123p1jx31 = { > }, > }; > > +static const struct drm_display_mode sharp_lq140m1jw46_mode = { > + .clock = 144370, > + .hdisplay = 1920, > + .hsync_start = 1920 + 48, > + .hsync_end = 1920 + 48 + 32, > + .htotal = 1920 + 48 + 32 + 80, > + .vdisplay = 1080, > + .vsync_start = 1080 + 3, > + .vsync_end = 1080 + 3 + 5, > + .vtotal = 1080 + 3 + 5 + 69, > + .flags = DRM_MODE_FLAG_NVSYNC | DRM_MODE_FLAG_NHSYNC, }; > + > +static const struct panel_desc sharp_lq140m1jw46 = { > + .modes = &sharp_lq140m1jw46_mode, > + .num_modes = 1, > + .bpc = 8, > + .size = { > + .width = 309, > + .height = 174, > + }, > + .delay = { > + .hpd_absent = 80, > + .enable = 50, > + .unprepare = 500, > + }, > +}; > + > static const struct drm_display_mode starry_kr122ea0sra_mode = { > .clock = 147000, > .hdisplay = 1920, > @@ -1719,6 +1747,9 @@ static const struct of_device_id platform_of_match[] = { > .compatible = "sharp,lq123p1jx31", > .data = &sharp_lq123p1jx31, > }, { > + .compatible = "sharp,lq140m1jw46", > + .data = &sharp_lq140m1jw46, > + }, { > .compatible = "starry,kr122ea0sra", > .data = &starry_kr122ea0sra, > }, { -- With best wishes Dmitry