Re: [PATCH v3] drm/bridge: panel: Add a device link between drm device and panel device

[Ulf Hansson]

On Mon, 7 Aug 2023 at 08:06, Liu Ying <victor.liu@xxxxxxx> wrote:
>
> Add the device link when panel bridge is attached and delete the link
> when panel bridge is detached.  The drm device is the consumer while
> the panel device is the supplier.  This makes sure that the drm device
> suspends eariler and resumes later than the panel device, hence resolves
> problems where the order is reversed, like the problematic case mentioned
> in the below link.
>
> Link: https://lore.kernel.org/lkml/CAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL+8fV-7s1CTMqi7u3-Rg@xxxxxxxxxxxxxx/T/
> Suggested-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>
> Signed-off-by: Liu Ying <victor.liu@xxxxxxx>

Looks good to me! Just a minor question though, don't we need to
manage runtime PM too - or this is solely for system wide
suspend/resume?

Reviewed-by: Ulf Hansson <ulf.hansson@xxxxxxxxxx>

Kind regards
Uffe

> ---
> v2->v3:
> * Improve commit message s/swapped/reversed/.
>
> v1->v2:
> * Fix bailout for panel_bridge_attach() in case device_link_add() fails.
>
>  drivers/gpu/drm/bridge/panel.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
>
> diff --git a/drivers/gpu/drm/bridge/panel.c b/drivers/gpu/drm/bridge/panel.c
> index 9316384b4474..a6587d233505 100644
> --- a/drivers/gpu/drm/bridge/panel.c
> +++ b/drivers/gpu/drm/bridge/panel.c
> @@ -4,6 +4,8 @@
>   * Copyright (C) 2017 Broadcom
>   */
>
> +#include <linux/device.h>
> +
>  #include <drm/drm_atomic_helper.h>
>  #include <drm/drm_bridge.h>
>  #include <drm/drm_connector.h>
> @@ -19,6 +21,7 @@ struct panel_bridge {
>         struct drm_bridge bridge;
>         struct drm_connector connector;
>         struct drm_panel *panel;
> +       struct device_link *link;
>         u32 connector_type;
>  };
>
> @@ -60,6 +63,8 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>  {
>         struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>         struct drm_connector *connector = &panel_bridge->connector;
> +       struct drm_panel *panel = panel_bridge->panel;
> +       struct drm_device *drm_dev = bridge->dev;
>         int ret;
>
>         if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> @@ -70,6 +75,14 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>                 return -ENODEV;
>         }
>
> +       panel_bridge->link = device_link_add(drm_dev->dev, panel->dev,
> +                                            DL_FLAG_STATELESS);
> +       if (!panel_bridge->link) {
> +               DRM_ERROR("Failed to add device link between %s and %s\n",
> +                         dev_name(drm_dev->dev), dev_name(panel->dev));
> +               return -EINVAL;
> +       }
> +
>         drm_connector_helper_add(connector,
>                                  &panel_bridge_connector_helper_funcs);
>
> @@ -78,6 +91,7 @@ static int panel_bridge_attach(struct drm_bridge *bridge,
>                                  panel_bridge->connector_type);
>         if (ret) {
>                 DRM_ERROR("Failed to initialize connector\n");
> +               device_link_del(panel_bridge->link);
>                 return ret;
>         }
>
> @@ -100,6 +114,8 @@ static void panel_bridge_detach(struct drm_bridge *bridge)
>         struct panel_bridge *panel_bridge = drm_bridge_to_panel_bridge(bridge);
>         struct drm_connector *connector = &panel_bridge->connector;
>
> +       device_link_del(panel_bridge->link);
> +
>         /*
>          * Cleanup the connector if we know it was initialized.
>          *
> --
> 2.37.1
>