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

[Ying Liu]

On Wednesday, August 9, 2023 9:54 PM Ulf Hansson <ulf.hansson@xxxxxxxxxx> wrote:
>
> 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.k/
> ernel.org%2Flkml%2FCAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL%2B8fV-
> 7s1CTMqi7u3-
> Rg%40mail.gmail.com%2FT%2F&data=05%7C01%7Cvictor.liu%40nxp.com%7
> Cb498937c20c94ab9148908db98e02662%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638271860697989733%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&sdata=iGMdYWbOeyVxzy9T9THCNh%2Ff%2BbKFLP0tI
> m%2BowL7h5Og%3D&reserved=0
> > 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?

I think this is solely for system wide suspend/resume.
AFAICS, there is no any particular need to manage runtime PM.

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

Thank you for your review.

Regards,
Liu Ying

>
> 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
> >