Re: [PATCH v1 4/5] drm/msm/dp: move relevant dp initialization code from bind() to probe()

[Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>]

On 7/17/2023 10:22 AM, Dmitry Baryshkov wrote:
> On 17/07/2023 20:16, Kuogee Hsieh wrote:
> > On 7/10/2023 11:13 AM, Dmitry Baryshkov wrote:
> > > On 10/07/2023 19:57, Kuogee Hsieh wrote:
> > > > On 7/7/2023 5:11 PM, Dmitry Baryshkov wrote:
> > > > > On 08/07/2023 02:52, Kuogee Hsieh wrote:
> > > > > > In preparation of moving edp of_dp_aux_populate_bus() to
> > > > > > dp_display_probe(), move dp_display_request_irq(),
> > > > > > dp->parser->parse() and dp_power_client_init() to dp_display_probe()
> > > > > > too.
> > > > > >
> > > > > > Signed-off-by: Kuogee Hsieh <quic_khsieh@xxxxxxxxxxx>
> > > > > > ---
> > > > > >   drivers/gpu/drm/msm/dp/dp_display.c | 48 
> > > > > > +++++++++++++++++--------------------
> > > > > >   drivers/gpu/drm/msm/dp/dp_display.h |  1 -
> > > > > >   2 files changed, 22 insertions(+), 27 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c 
> > > > > > b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > index 44580c2..185f1eb 100644
> > > > > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > > > > @@ -290,12 +290,6 @@ static int dp_display_bind(struct device 
> > > > > > *dev, struct device *master,
> > > > > >           goto end;
> > > > > >       }
> > > > > >   -    rc = dp_power_client_init(dp->power);
> > > > > > -    if (rc) {
> > > > > > -        DRM_ERROR("Power client create failed\n");
> > > > > > -        goto end;
> > > > > > -    }
> > > > > > -
> > > > > >       rc = dp_register_audio_driver(dev, dp->audio);
> > > > > >       if (rc) {
> > > > > >           DRM_ERROR("Audio registration Dp failed\n");
> > > > > > @@ -752,6 +746,12 @@ static int dp_init_sub_modules(struct 
> > > > > > dp_display_private *dp)
> > > > > >           goto error;
> > > > > >       }
> > > > > >   +    rc = dp->parser->parse(dp->parser);
> > > > > > +    if (rc) {
> > > > > > +        DRM_ERROR("device tree parsing failed\n");
> > > > > > +        goto error;
> > > > > > +    }
> > > > > > +
> > > > > >       dp->catalog = dp_catalog_get(dev, &dp->parser->io);
> > > > > >       if (IS_ERR(dp->catalog)) {
> > > > > >           rc = PTR_ERR(dp->catalog);
> > > > > > @@ -768,6 +768,12 @@ static int dp_init_sub_modules(struct 
> > > > > > dp_display_private *dp)
> > > > > >           goto error;
> > > > > >       }
> > > > > >   +    rc = dp_power_client_init(dp->power);
> > > > > > +    if (rc) {
> > > > > > +        DRM_ERROR("Power client create failed\n");
> > > > > > +        goto error;
> > > > > > +    }
> > > > > > +
> > > > > >       dp->aux = dp_aux_get(dev, dp->catalog, dp->dp_display.is_edp);
> > > > > >       if (IS_ERR(dp->aux)) {
> > > > > >           rc = PTR_ERR(dp->aux);
> > > > > > @@ -1196,26 +1202,20 @@ 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);
> > > > > >       if (!dp->irq) {
> > > > > > -        DRM_ERROR("failed to get irq\n");
> > > > > > -        return -EINVAL;
> > > > > > +        dp->irq = irq_of_parse_and_map(dp->pdev->dev.of_node, 0);
> > > > > > +        if (!dp->irq) {
> > > > > > +            DRM_ERROR("failed to get irq\n");
> > > > > > +            return -EINVAL;
> > > > > > +        }
> > > > > >       }
> > > > >
> > > > > Use platform_get_irq() from probe() function.
> > > > >
> > > > > >   -    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",
> > > > > > @@ -1290,6 +1290,8 @@ static int dp_display_probe(struct 
> > > > > > platform_device *pdev)
> > > > > >         platform_set_drvdata(pdev, &dp->dp_display);
> > > > > >   +    dp_display_request_irq(dp);
> > > > > > +
> > > > >
> > > > > Error checking?
> > > > > Are we completely ready to handle interrupts at this point?
> > > > not until dp_display_host_init() is called which will be called 
> > > > from pm_runtime_resume() later.
> > >
> > > But once you request_irq(), you should be ready for the IRQs to be 
> > > delivered right away.
> >
> > At this point, the DP controller interrupts mask bit is not enabled yet.
> >
> > Therefore interrupts will not happen until dp_bridge_hpd_enable() is 
> > called to initialize dp host controller and then enabled mask bits.
>
> Are AUX and CTRL interrupts also disabled? What about any 
> stray/pending interrupts? Just take it as a rule of thumb. Once 
> request_irq() has been called without the IRQ_NOAUTOEN flag, the 
> driver should be prepared to handle the incoming interrupt requests.

yes, both AUX and CTRL are disabled.

edp population do need irq to handle aux transfer during probe.

it should work by checking core_initialized flag at irq handle to filter 
out stray/pending interrupts.

> > > > > >       rc = component_add(&pdev->dev, &dp_display_comp_ops);
> > > > > >       if (rc) {
> > > > > >           DRM_ERROR("component add failed, rc=%d\n", rc);
> > > > > > @@ -1574,12 +1576,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);