Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib

[Dmitry Baryshkov]

On Thu, 16 Nov 2023 at 11:14, Sui Jingfeng <sui.jingfeng@xxxxxxxxx> wrote:
>
> Hi,
>
> Thanks a lot for reviewing!
>
>
> On 2023/11/15 00:30, Dmitry Baryshkov wrote:
> > On Tue, 14 Nov 2023 at 17:09, Sui Jingfeng <sui.jingfeng@xxxxxxxxx> wrote:
> >> From: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >>
> >> The it66121_create_bridge() and it66121_destroy_bridge() are added to
> >> export the core functionalities. Create a connector manually by using
> >> bridge connector helpers when link as a lib.
> >>
> >> Signed-off-by: Sui Jingfeng <suijingfeng@xxxxxxxxxxx>
> >> ---
> >>   drivers/gpu/drm/bridge/ite-it66121.c | 134 +++++++++++++++++++--------
> >>   include/drm/bridge/ite-it66121.h     |  17 ++++
> >>   2 files changed, 113 insertions(+), 38 deletions(-)
> >>   create mode 100644 include/drm/bridge/ite-it66121.h
> >>
> >> diff --git a/drivers/gpu/drm/bridge/ite-it66121.c b/drivers/gpu/drm/bridge/ite-it66121.c
> >> index 8971414a2a60..f5968b679c5d 100644
> >> --- a/drivers/gpu/drm/bridge/ite-it66121.c
> >> +++ b/drivers/gpu/drm/bridge/ite-it66121.c
> >> @@ -22,6 +22,7 @@
> >>
> >>   #include <drm/drm_atomic_helper.h>
> >>   #include <drm/drm_bridge.h>
> >> +#include <drm/drm_bridge_connector.h>
> >>   #include <drm/drm_edid.h>
> >>   #include <drm/drm_modes.h>
> >>   #include <drm/drm_print.h>
> >> @@ -703,14 +704,32 @@ static int it66121_bridge_attach(struct drm_bridge *bridge,
> >>                                   enum drm_bridge_attach_flags flags)
> >>   {
> >>          struct it66121_ctx *ctx = bridge_to_it66121(bridge);
> >> +       struct drm_bridge *next_bridge = ctx->next_bridge;
> >> +       struct drm_encoder *encoder = bridge->encoder;
> >>          int ret;
> >>
> >> -       if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR))
> >> -               return -EINVAL;
> >> +       if (next_bridge) {
> >> +               if (!(flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)) {
> >> +                       WARN_ON(1);
> > Why? At least use WARN() instead
>
> Originally I want to
>
>
>
>
> >> +                       flags |= DRM_BRIDGE_ATTACH_NO_CONNECTOR;
> >> +               }
> >> +               ret = drm_bridge_attach(encoder, next_bridge, bridge, flags);
> >> +               if (ret)
> >> +                       return ret;
> >> +       } else {
> >> +               struct drm_connector *connector;
> >>
> >> -       ret = drm_bridge_attach(bridge->encoder, ctx->next_bridge, bridge, flags);
> >> -       if (ret)
> >> -               return ret;
> >> +               if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR)
> >> +                       WARN_ON(1);
> > No. It is perfectly fine to create attach a bridge with no next_bridge
> > and with the DRM_BRIDGE_ATTACH_NO_CONNECTOR flag.
> >
>
> The document say when DRM_BRIDGE_ATTACH_NO_CONNECTOR flag is set
> the bridge shall not create a drm_connector. So I think if a display
> bridge driver don't have a next bridge attached (Currently, this is
> told by the DT), it says that this is a non-DT environment. On such
> a case, this display bridge driver(it66121.ko) should behavior like
> a *agent*. Because the upstream port of it66121 is the DVO port of
> the display controller, the downstream port of it66121 is the HDMI
> connector. it66121 is on the middle. So I think the it66121.ko should
> handle all of troubles on behalf of the display controller drivers.

No. Don't make decisions for the other drivers. They might have different needs.

> Therefore (when in non-DT use case), the display controller drivers
> side should not set DRM_BRIDGE_ATTACH_NO_CONNECTOR flag anymore.
> Which is to hint that the it66121 should totally in charge of those
> tasks (either by using bridge connector helper or create a connector
> manually). I don't understand on such a case, why bother display
> controller drivers anymore.

This is the reason why we had introduced this flag. It allows the
driver to customise the connector. It even allows the driver to
implement a connector on its own, completely ignoring the
drm_bridge_connector.

>
>
> >> +
> >> +               connector = drm_bridge_connector_init(bridge->dev, encoder);
> >> +               if (IS_ERR(connector))
> >> +                       return PTR_ERR(connector);
> >> +
> >> +               drm_connector_attach_encoder(connector, encoder);
> > This goes into your device driver.
> >
> >> +
> >> +               ctx->connector = connector;
> >> +       }
> >>
> >>          if (ctx->info->id == ID_IT66121) {
> >>                  ret = regmap_write_bits(ctx->regmap, IT66121_CLK_BANK_REG,
> >> @@ -1632,16 +1651,13 @@ static const char * const it66121_supplies[] = {
> >>          "vcn33", "vcn18", "vrf12"
> >>   };
> >



-- 
With best wishes
Dmitry