[v4,1/8] drm/msm/dp: tie dp_display_irq_handler() with dp driver
Commit Message
Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
which ties irq registration to the DPU device's life cycle, while depending on
resources that are released as the DP device is torn down. Move register DP
driver irq handler at dp_display_probe() to have dp_display_irq_handler()
is tied with DP device.
Changes in v4:
-- delete dp->irq check at dp_display_request_irq()
Changes in v3:
-- move calling dp_display_irq_handler() to probe
Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
---
drivers/gpu/drm/msm/dp/dp_display.c | 29 +++++++++--------------------
drivers/gpu/drm/msm/dp/dp_display.h | 1 -
2 files changed, 9 insertions(+), 21 deletions(-)
Comments
On Wed, 27 Sept 2023 at 23:54, Kuogee Hsieh <quic_khsieh@quicinc.com> wrote:
>
> Currently the dp_display_irq_handler() is executed at msm_dp_modeset_init()
dp_display_request_irq()
> which ties irq registration to the DPU device's life cycle, while depending on
> resources that are released as the DP device is torn down. Move register DP
`registering` or `registration of`
> driver irq handler at dp_display_probe() to have dp_display_irq_handler()
IRQ, s/at/to/
> is tied with DP device.
s/is //
Moreover, your commit does more that you have described in the commit
message. It also e.g. switches to platform_get_irq().
>
> Changes in v4:
> -- delete dp->irq check at dp_display_request_irq()
>
> Changes in v3:
> -- move calling dp_display_irq_handler() to probe
>
> Signed-off-by: Kuogee Hsieh <quic_khsieh@quicinc.com>
> ---
> drivers/gpu/drm/msm/dp/dp_display.c | 29 +++++++++--------------------
> drivers/gpu/drm/msm/dp/dp_display.h | 1 -
> 2 files changed, 9 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> index 76f1395..5645178 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.c
> +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> @@ -1193,30 +1193,21 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
> return ret;
> }
>
> -int dp_display_request_irq(struct msm_dp *dp_display)
> +static int dp_display_request_irq(struct dp_display_private *dp)
> {
> int rc = 0;
> - struct dp_display_private *dp;
> -
> - if (!dp_display) {
> - DRM_ERROR("invalid input\n");
> - return -EINVAL;
> - }
> -
> - dp = container_of(dp_display, struct dp_display_private, dp_display);
> + struct device *dev = &dp->pdev->dev;
>
> - dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> + dp->irq = platform_get_irq(dp->pdev, 0);
> if (!dp->irq) {
> DRM_ERROR("failed to get irq\n");
> return -EINVAL;
> }
>
> - rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
> - dp_display_irq_handler,
> + rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
> IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
> if (rc < 0) {
> - DRM_ERROR("failed to request IRQ%u: %d\n",
> - dp->irq, rc);
> + DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
Please don't mix functional changes with code reformatting.
> return rc;
> }
>
> @@ -1287,6 +1278,10 @@ static int dp_display_probe(struct platform_device *pdev)
>
> platform_set_drvdata(pdev, &dp->dp_display);
>
> + rc = dp_display_request_irq(dp);
> + if (rc)
> + return rc;
Who will perform component teardown for you if the driver just returns
an error here?
> +
> rc = component_add(&pdev->dev, &dp_display_comp_ops);
> if (rc) {
> DRM_ERROR("component add failed, rc=%d\n", rc);
> @@ -1549,12 +1544,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
>
> dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
>
> - ret = dp_display_request_irq(dp_display);
> - if (ret) {
> - DRM_ERROR("request_irq failed, ret=%d\n", ret);
> - return ret;
> - }
> -
> ret = dp_display_get_next_bridge(dp_display);
> if (ret)
> return ret;
> diff --git a/drivers/gpu/drm/msm/dp/dp_display.h b/drivers/gpu/drm/msm/dp/dp_display.h
> index 1e9415a..b3c08de 100644
> --- a/drivers/gpu/drm/msm/dp/dp_display.h
> +++ b/drivers/gpu/drm/msm/dp/dp_display.h
> @@ -35,7 +35,6 @@ struct msm_dp {
> int dp_display_set_plugged_cb(struct msm_dp *dp_display,
> hdmi_codec_plugged_cb fn, struct device *codec_dev);
> int dp_display_get_modes(struct msm_dp *dp_display);
> -int dp_display_request_irq(struct msm_dp *dp_display);
> bool dp_display_check_video_test(struct msm_dp *dp_display);
> int dp_display_get_test_bpp(struct msm_dp *dp_display);
> void dp_display_signal_audio_start(struct msm_dp *dp_display);
> --
> 2.7.4
>
@@ -1193,30 +1193,21 @@ static irqreturn_t dp_display_irq_handler(int irq, void *dev_id)
return ret;
}
-int dp_display_request_irq(struct msm_dp *dp_display)
+static int dp_display_request_irq(struct dp_display_private *dp)
{
int rc = 0;
- struct dp_display_private *dp;
-
- if (!dp_display) {
- DRM_ERROR("invalid input\n");
- return -EINVAL;
- }
-
- dp = container_of(dp_display, struct dp_display_private, dp_display);
+ struct device *dev = &dp->pdev->dev;
- dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
+ dp->irq = platform_get_irq(dp->pdev, 0);
if (!dp->irq) {
DRM_ERROR("failed to get irq\n");
return -EINVAL;
}
- rc = devm_request_irq(dp_display->drm_dev->dev, dp->irq,
- dp_display_irq_handler,
+ rc = devm_request_irq(dev, dp->irq, dp_display_irq_handler,
IRQF_TRIGGER_HIGH, "dp_display_isr", dp);
if (rc < 0) {
- DRM_ERROR("failed to request IRQ%u: %d\n",
- dp->irq, rc);
+ DRM_ERROR("failed to request IRQ%u: %d\n", dp->irq, rc);
return rc;
}
@@ -1287,6 +1278,10 @@ static int dp_display_probe(struct platform_device *pdev)
platform_set_drvdata(pdev, &dp->dp_display);
+ rc = dp_display_request_irq(dp);
+ if (rc)
+ return rc;
+
rc = component_add(&pdev->dev, &dp_display_comp_ops);
if (rc) {
DRM_ERROR("component add failed, rc=%d\n", rc);
@@ -1549,12 +1544,6 @@ int msm_dp_modeset_init(struct msm_dp *dp_display, struct drm_device *dev,
dp_priv = container_of(dp_display, struct dp_display_private, dp_display);
- ret = dp_display_request_irq(dp_display);
- if (ret) {
- DRM_ERROR("request_irq failed, ret=%d\n", ret);
- return ret;
- }
-
ret = dp_display_get_next_bridge(dp_display);
if (ret)
return ret;
@@ -35,7 +35,6 @@ struct msm_dp {
int dp_display_set_plugged_cb(struct msm_dp *dp_display,
hdmi_codec_plugged_cb fn, struct device *codec_dev);
int dp_display_get_modes(struct msm_dp *dp_display);
-int dp_display_request_irq(struct msm_dp *dp_display);
bool dp_display_check_video_test(struct msm_dp *dp_display);
int dp_display_get_test_bpp(struct msm_dp *dp_display);
void dp_display_signal_audio_start(struct msm_dp *dp_display);