Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Ying Liu <victor.liu@nxp.com>
To:     Ulf Hansson <ulf.hansson@linaro.org>
CC:     "dri-devel@lists.freedesktop.org" <dri-devel@lists.freedesktop.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "andrzej.hajda@intel.com" <andrzej.hajda@intel.com>,
        "neil.armstrong@linaro.org" <neil.armstrong@linaro.org>,
        "rfoss@kernel.org" <rfoss@kernel.org>,
        "Laurent.pinchart@ideasonboard.com" 
        <Laurent.pinchart@ideasonboard.com>,
        "jonas@kwiboo.se" <jonas@kwiboo.se>,
        "jernej.skrabec@gmail.com" <jernej.skrabec@gmail.com>,
        "airlied@gmail.com" <airlied@gmail.com>,
        "daniel@ffwll.ch" <daniel@ffwll.ch>
Subject: RE: [PATCH v3] drm/bridge: panel: Add a device link between drm
 device and panel device
Thread-Topic: [PATCH v3] drm/bridge: panel: Add a device link between drm
 device and panel device
Thread-Index: AQHZyPVXeBJTD139FkmL7cmc6zCmuK/h//CAgADZrYA=
Date:   Thu, 10 Aug 2023 03:10:53 +0000
Message-ID: <AM7PR04MB70462E947D9669E11B699DDA9813A@AM7PR04MB7046.eurprd04.prod.outlook.com>
References: <20230807061115.3244501-1-victor.liu@nxp.com>
 <CAPDyKFpyQqojAFtmY5NP1gQjL9ag1V-kHLUBRAx2UHBZ6rgVSg@mail.gmail.com>
In-Reply-To: <CAPDyKFpyQqojAFtmY5NP1gQjL9ag1V-kHLUBRAx2UHBZ6rgVSg@mail.gmail.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?QkWtaJeQTim56DRQH4iaFCCP76uAMPltHqLI5N5UVP294FPInNVrftw16Lst?=
 =?us-ascii?Q?cg98VNBfPGqHSAosQcLYdQthYS3ZnxOL0VuJWfzL0h95f8PpYRgy2Dfb7CxJ?=
 =?us-ascii?Q?zSj9oi+i34NDY9/TeoV1fMvh7N7kRo3j4J/sjLSIAYA9vvNxA1fxtgxbDhAg?=
 =?us-ascii?Q?ulIw+p1WVrMlSew8u7ESaCW08RKboAZ3bZ9WIDCudIRIQPVlfunVEf7q4Qxf?=
 =?us-ascii?Q?39yn5n4Cg28/xZKvCbE/jLXrY7BDuS8amX7znJQYygY0Kthi0eFxYzDLBtwY?=
 =?us-ascii?Q?Boh7k7i1SXCLDNQ6ulI+NThareiM4xufBRsTia+2gXkylEH02+fBg9CGiI7r?=
 =?us-ascii?Q?zqokNJXkZvZTmKzENc0fNlKRi722fTH3Yo1JR1DNHGuykXI6+IyQESDv2EHt?=
 =?us-ascii?Q?mIoQ0D23F9Ts+k2bpYhG0tSlkoFU2vRkxFTF1vcR1dScVT9tYNEKhxFOqksU?=
 =?us-ascii?Q?yeSwjvqqbum6BGLaVXLMXLUwBKxy9UjLADKyyz3OQmdj/NwDLcgjZv3Nqz1z?=
 =?us-ascii?Q?jgiXuw9h4t02TuMrqt4y9SIDisx/T7CkLrXJtK+/o0q/9yiABKUvB0sKyv/a?=
 =?us-ascii?Q?z5xAr5S+cNd9YhFTRT/F5bgJxSAmsj2sKKdvh+iXTfSfeByISdRDWDdVc6t0?=
 =?us-ascii?Q?ZQ5VOniLyRpWp3XmR6JC4gKO0zMPZsc5VUn1+5jzqtjwF39XwpM3tpqAy1n3?=
 =?us-ascii?Q?HvX7+fXBPPLfFMf6V7lf+5Kfwmp87iDXDNKNdVhLIqUwcH4HXhxtFN6WQa1y?=
 =?us-ascii?Q?6X6apqwWvE3BF42Psu2uaurrmp0aaaQ42o2vmXQ0+vpEw8J6qu3/bfDCmnwY?=
 =?us-ascii?Q?vfu0ZRbyVjMSamOQto7wVmOsFYudmv/FHLcuLOwfbFKf6hZIEyB/tnjNOOvE?=
 =?us-ascii?Q?boDKRopxgnZmu0PZlYOXTxMbpocmOAa4HM0S46Ddb9mSvI37PYn/fuxIrJzv?=
 =?us-ascii?Q?j2B7Tb94OkQqRtDVYtAo7P9jnWj3tag9Lq+3kNueZEojWI2zTqCak9iPK25z?=
 =?us-ascii?Q?hCpCDwH9DE/gplMeSpOW3A8faDN9joKesxGCbp3qQ+vqMIyyaaoPNj80i7Fk?=
 =?us-ascii?Q?fv8Jf+ak2M+XzZlHPvOQkdUZKuhWKBlV85MYTErEh8F5+X8p4bW4jPr2Kgtv?=
 =?us-ascii?Q?gaRA8eJ2yI309AHkdKFUWQy1EV3cwebvYCvGbFowEWLS8D0LEvfQQ8rDWNAD?=
 =?us-ascii?Q?AquqMPKPZEEkRJkojQPIbTQSvkD8Yczct+WncjA6nuuwpIRdD9xH3WTyaqFh?=
 =?us-ascii?Q?9CQE4C+wy2BeW9pwSs789xFEnp0HtLKnt/dHQ2cXJ7SRgiJyuqG3oliML75R?=
 =?us-ascii?Q?j0UZg9++xkWmbUB8UMD9nnF39aKTAT1VpEN1Z3HG0pBsGl7kd2VD6MCS2bTo?=
 =?us-ascii?Q?I1da5fh5iZNodrPzn1zC1tDYK/ID+dlR3uP64l3rQ/qbUW9KbZqEifhtiTv1?=
 =?us-ascii?Q?o34nIt9EGlAkAtsmeR/hlMzCxzWsNsQ8t9u0XgAafQTP2BVaxBIMgY9Kz0ob?=
 =?us-ascii?Q?56stWcf3WwBGbuj6VGX/PYmDeK/5Jwk83zWWJHmdFrS4qA4TY30gXKeHPEe/?=
 =?us-ascii?Q?FaCjHZtaZzzz6vh1dNU=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: nxp.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: AM7PR04MB7046.eurprd04.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: f7712a10-7417-4c2c-c86b-08db994f6897
X-MS-Exchange-CrossTenant-originalarrivaltime: 10 Aug 2023 03:10:53.8825
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 686ea1d3-bc2b-4c6f-a92c-d99c5c301635
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: Isq3gNT95qfGvsnVSVWcdTu1nKK0P1nD3JYjypO4HWTq20wdd4iGgSWFh2/DzCYqerN31tCJuMttSWXZH9S+/Q==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: AS8PR04MB7637
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_BLOCKED,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On Wednesday, August 9, 2023 9:54 PM Ulf Hansson <ulf.hansson@linaro.org> w=
rote:
>
> On Mon, 7 Aug 2023 at 08:06, Liu Ying <victor.liu@nxp.com> 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 resolve=
s
> > problems where the order is reversed, like the problematic case mention=
ed
> > in the below link.
> >
> > Link:
> https://lore.k/
> ernel.org%2Flkml%2FCAPDyKFr0XjrU_udKoUKQ_q8RWaUkyqL%2B8fV-
> 7s1CTMqi7u3-
> Rg%40mail.gmail.com%2FT%2F&data=3D05%7C01%7Cvictor.liu%40nxp.com%7
> Cb498937c20c94ab9148908db98e02662%7C686ea1d3bc2b4c6fa92cd99c5c30
> 1635%7C0%7C0%7C638271860697989733%7CUnknown%7CTWFpbGZsb3d8e
> yJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D
> %7C3000%7C%7C%7C&sdata=3DiGMdYWbOeyVxzy9T9THCNh%2Ff%2BbKFLP0tI
> m%2BowL7h5Og%3D&reserved=3D0
> > Suggested-by: Ulf Hansson <ulf.hansson@linaro.org>
> > Signed-off-by: Liu Ying <victor.liu@nxp.com>
>
> 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@linaro.org>

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 =3D
> drm_bridge_to_panel_bridge(bridge);
> >         struct drm_connector *connector =3D &panel_bridge->connector;
> > +       struct drm_panel *panel =3D panel_bridge->panel;
> > +       struct drm_device *drm_dev =3D 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 =3D 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 =3D
> drm_bridge_to_panel_bridge(bridge);
> >         struct drm_connector *connector =3D &panel_bridge->connector;
> >
> > +       device_link_del(panel_bridge->link);
> > +
> >         /*
> >          * Cleanup the connector if we know it was initialized.
> >          *
> > --
> > 2.37.1
> >