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

[Dmitry Baryshkov]

On 10/10/2023 00:04, Abhinav Kumar wrote:
> 
> 
> On 10/9/2023 1:53 PM, Dmitry Baryshkov wrote:
>> On 09/10/2023 22:21, Dmitry Baryshkov wrote:
>>> 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 at gmail.com>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>> ---
>>>>>   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.
>>
>> I was wrong here. hdmi::bridge is used by the driver (e.g. for HPD 
>> reporting).
>>
> 
> hmmm, I thought HPD module uses hdmi_bridge->hdmi. here we are talking 
> about hdmi->bridge?

See msm_hdmi_irq().

-- 
With best wishes
Dmitry