Messages in this thread | | | Date | Thu, 16 Nov 2023 17:14:34 +0800 | Subject | Re: [PATCH 8/8] drm/bridge: it66121: Allow link this driver as a lib | From | Sui Jingfeng <> |
| |
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@linux.dev> wrote: >> From: Sui Jingfeng <suijingfeng@loongson.cn> >> >> 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@loongson.cn> >> --- >> 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.
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.
>> + >> + 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" >> }; >
| |