Re: [PATCH v2 02/13] drm/msm/hdmi: switch to devm_drm_bridge_add()

[Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>]

On 09/10/2023 22:19, Abhinav Kumar wrote:
> On 10/9/2023 11:10 AM, Dmitry Baryshkov wrote:
> > Make MSM HDMI driver use devm_drm_bridge_add() instead of plain
> > drm_bridge_add(). As the driver doesn't require any additional cleanup,
> > stop adding created bridge to the priv->bridges array.
> >
> > Reviewed-by: Rob Clark <robdclark@xxxxxxxxx>
> > Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@xxxxxxxxxx>
> > ---
> >   drivers/gpu/drm/msm/hdmi/hdmi.c        | 22 +++++--------------
> >   drivers/gpu/drm/msm/hdmi/hdmi.h        |  5 ++---
> >   drivers/gpu/drm/msm/hdmi/hdmi_bridge.c | 30 ++++++++------------------
> >   drivers/gpu/drm/msm/hdmi/hdmi_hpd.c    |  3 +--
> >   4 files changed, 17 insertions(+), 43 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.c 
> > b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > index b6bcb9f675fe..c8ebd75176bb 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.c
> > @@ -160,24 +160,16 @@ static int msm_hdmi_init(struct hdmi *hdmi)
> >   int msm_hdmi_modeset_init(struct hdmi *hdmi,
> >           struct drm_device *dev, struct drm_encoder *encoder)
> >   {
> > -    struct msm_drm_private *priv = dev->dev_private;
> >       int ret;
> > -    if (priv->num_bridges == ARRAY_SIZE(priv->bridges)) {
> > -        DRM_DEV_ERROR(dev->dev, "too many bridges\n");
> > -        return -ENOSPC;
> > -    }
> > -
> >       hdmi->dev = dev;
> >       hdmi->encoder = encoder;
> >       hdmi_audio_infoframe_init(&hdmi->audio.infoframe);
> > -    hdmi->bridge = msm_hdmi_bridge_init(hdmi);
> > -    if (IS_ERR(hdmi->bridge)) {
> > -        ret = PTR_ERR(hdmi->bridge);
> > +    ret = msm_hdmi_bridge_init(hdmi);
> > +    if (ret) {
> >           DRM_DEV_ERROR(dev->dev, "failed to create HDMI bridge: 
> > %d\n", ret);
> > -        hdmi->bridge = NULL;
> >           goto fail;
> >       }
> > @@ -215,16 +207,9 @@ int msm_hdmi_modeset_init(struct hdmi *hdmi,
> >           goto fail;
> >       }
> > -    priv->bridges[priv->num_bridges++]       = hdmi->bridge;
> > -
> >       return 0;
> >   fail:
> > -    /* bridge is normally destroyed by drm: */
> > -    if (hdmi->bridge) {
> > -        msm_hdmi_bridge_destroy(hdmi->bridge);
> > -        hdmi->bridge = NULL;
> > -    }
> >       if (hdmi->connector) {
> >           hdmi->connector->funcs->destroy(hdmi->connector);
> >           hdmi->connector = NULL;
> > @@ -395,6 +380,9 @@ static void msm_hdmi_unbind(struct device *dev, 
> > struct device *master,
> >           if (priv->hdmi->audio_pdev)
> >               platform_device_unregister(priv->hdmi->audio_pdev);
> > +        if (priv->hdmi->bridge)
> > +            msm_hdmi_hpd_disable(priv->hdmi);
> > +
>
> Now is this the only place where hdmi->bridge is used?
>
> Why cant we just keep msm_hdmi_hpd_disable(priv->hdmi) here since its 
> anyway protected by if (priv->hdmi) and drop hdmi->bridge completely?

Sure, sounds like a good idea, same followup as for the DSI.

> >           msm_hdmi_destroy(priv->hdmi);
> >           priv->hdmi = NULL;
> >       }
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi.h 
> > b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > index e8dbee50637f..ec5786440391 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi.h
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi.h
> > @@ -224,14 +224,13 @@ void msm_hdmi_audio_set_sample_rate(struct hdmi 
> > *hdmi, int rate);
> >    * hdmi bridge:
> >    */
> > -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi);
> > -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge);
> > +int msm_hdmi_bridge_init(struct hdmi *hdmi);
> >   void msm_hdmi_hpd_irq(struct drm_bridge *bridge);
> >   enum drm_connector_status msm_hdmi_bridge_detect(
> >           struct drm_bridge *bridge);
> >   int msm_hdmi_hpd_enable(struct drm_bridge *bridge);
> > -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge);
> > +void msm_hdmi_hpd_disable(struct hdmi *hdmi);
> >   /*
> >    * i2c adapter for ddc:
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c 
> > b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > index 9b1391d27ed3..0b7a6a56677e 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_bridge.c
> > @@ -11,14 +11,6 @@
> >   #include "msm_kms.h"
> >   #include "hdmi.h"
> > -void msm_hdmi_bridge_destroy(struct drm_bridge *bridge)
> > -{
> > -    struct hdmi_bridge *hdmi_bridge = to_hdmi_bridge(bridge);
> > -
> > -    msm_hdmi_hpd_disable(hdmi_bridge);
> > -    drm_bridge_remove(bridge);
> > -}
> > -
> >   static void msm_hdmi_power_on(struct drm_bridge *bridge)
> >   {
> >       struct drm_device *dev = bridge->dev;
> > @@ -317,7 +309,7 @@ msm_hdmi_hotplug_work(struct work_struct *work)
> >   }
> >   /* initialize bridge */
> > -struct drm_bridge *msm_hdmi_bridge_init(struct hdmi *hdmi)
> > +int msm_hdmi_bridge_init(struct hdmi *hdmi)
> >   {
> >       struct drm_bridge *bridge = NULL;
> >       struct hdmi_bridge *hdmi_bridge;
> > @@ -325,10 +317,8 @@ struct drm_bridge *msm_hdmi_bridge_init(struct 
> > hdmi *hdmi)
> >       hdmi_bridge = devm_kzalloc(hdmi->dev->dev,
> >               sizeof(*hdmi_bridge), GFP_KERNEL);
> > -    if (!hdmi_bridge) {
> > -        ret = -ENOMEM;
> > -        goto fail;
> > -    }
> > +    if (!hdmi_bridge)
> > +        return -ENOMEM;
> >       hdmi_bridge->hdmi = hdmi;
> >       INIT_WORK(&hdmi_bridge->hpd_work, msm_hdmi_hotplug_work);
> > @@ -341,17 +331,15 @@ struct drm_bridge *msm_hdmi_bridge_init(struct 
> > hdmi *hdmi)
> >           DRM_BRIDGE_OP_DETECT |
> >           DRM_BRIDGE_OP_EDID;
> > -    drm_bridge_add(bridge);
> > +    ret = devm_drm_bridge_add(&hdmi->pdev->dev, bridge);
> > +    if (ret)
> > +        return ret;
> >       ret = drm_bridge_attach(hdmi->encoder, bridge, NULL, 
> > DRM_BRIDGE_ATTACH_NO_CONNECTOR);
> >       if (ret)
> > -        goto fail;
> > +        return ret;
> > -    return bridge;
> > +    hdmi->bridge = bridge;
> > -fail:
> > -    if (bridge)
> > -        msm_hdmi_bridge_destroy(bridge);
> > -
> > -    return ERR_PTR(ret);
> > +    return 0;
> >   }
> > diff --git a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c 
> > b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> > index bfa827b47989..9ce0ffa35417 100644
> > --- a/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> > +++ b/drivers/gpu/drm/msm/hdmi/hdmi_hpd.c
> > @@ -147,9 +147,8 @@ int msm_hdmi_hpd_enable(struct drm_bridge *bridge)
> >       return ret;
> >   }
> > -void msm_hdmi_hpd_disable(struct hdmi_bridge *hdmi_bridge)
> > +void msm_hdmi_hpd_disable(struct hdmi *hdmi)
> >   {
> > -    struct hdmi *hdmi = hdmi_bridge->hdmi;
> >       const struct hdmi_platform_config *config = hdmi->config;
> >       struct device *dev = &hdmi->pdev->dev;
> >       int ret;

--
With best wishes
Dmitry