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

Sui Jingfeng
2023-11-16T09:15:06Z

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"
>> };
>