Re: [PATCH v9 09/15] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

[Damon Ding <damon.ding@rock-chips.com>]

Hi Luca,

On 3/9/2026 7:25 PM, Damon Ding wrote:
> Hi Luca,
> 
> On 3/3/2026 5:42 PM, Luca Ceresoli wrote:
>> Hello Damon,
>>
>> On Tue Feb 10, 2026 at 8:12 AM CET, Damon Ding wrote:
>>> Apply drm_bridge_connector helper for Analogix DP driver.
>>>
>>> The following changes have been made:
>>> - Apply drm_bridge_connector helper to get rid of &drm_connector_funcs
>>>    and &drm_connector_helper_funcs.
>>> - Remove unnecessary parameter struct drm_connector* for callback
>>>    &analogix_dp_plat_data.attach.
>>> - Remove &analogix_dp_device.connector.
>>> - Convert analogix_dp_atomic_check()/analogix_dp_detect() to
>>>    &drm_bridge_funcs.atomic_check()/&drm_bridge_funcs.detect().
>>> - Split analogix_dp_get_modes() into &drm_bridge_funcs.get_modes() and
>>>    &drm_bridge_funcs.edid_read().
>>> - Set flag DRM_BRIDGE_ATTACH_NO_CONNECTOR for bridge attachment while
>>>    binding. Meanwhile, make DRM_BRIDGE_ATTACH_NO_CONNECTOR unsuppported
>>                               ^
>>
>> Do you mean "!DRM_BRIDGE_ATTACH_NO_CONNECTOR" here (i.e. missing '!')?
>>
>> Also, unsuppported -> unsupported (typo)
>>
> 
> Will fix in v10.
> 
>>>    in analogix_dp_bridge_attach().
>>> - Set &drm_bridge.ops according to different cases.
>>>
>>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de> (on rk3588)
>>
>> I had a quick look, looks good overall, for the moment I have only a
>> question, see below.
>>
>> I aim at reviewing this patch in depth, but it's not an easy one to
>> digest. Would it be feasible to split it in smaller logical steps? If it
>> is, please do, it would be very helpful for reviewing.
> 
> Yes, this commit will be split into several smaller ones in v10.
> 
>>
>>> @@ -1532,6 +1481,7 @@ EXPORT_SYMBOL_GPL(analogix_dp_resume);
>>>
>>>   int analogix_dp_bind(struct analogix_dp_device *dp, struct 
>>> drm_device *drm_dev)
>>>   {
>>> +    struct drm_bridge *bridge = &dp->bridge;
>>>       int ret;
>>>
>>>       dp->drm_dev = drm_dev;
>>> @@ -1545,7 +1495,18 @@ int analogix_dp_bind(struct analogix_dp_device 
>>> *dp, struct drm_device *drm_dev)
>>>           return ret;
>>>       }
>>>
>>> -    ret = drm_bridge_attach(dp->encoder, &dp->bridge, NULL, 0);
>>> +    if (dp->plat_data->panel)
>>> +        bridge->ops = DRM_BRIDGE_OP_MODES | DRM_BRIDGE_OP_DETECT;
>>> +    else
>>> +        bridge->ops = DRM_BRIDGE_OP_EDID | DRM_BRIDGE_OP_DETECT;
>>> +
>>> +    bridge->of_node = dp->dev->of_node;
>>> +    bridge->type = DRM_MODE_CONNECTOR_eDP;
>>> +    ret = devm_drm_bridge_add(dp->dev, &dp->bridge);
>>
>> Can devm_drm_bridge_add() be added to analogix_dp_probe() instead?
> 
> Will do in v10.

My apologies for my somewhat irresponsible reply earlier!

When I was splitting this commit, I realized that moving 
devm_drm_bridge_add() to analogix_dp_probe() is actually not feasible – 
the type of the downstream bridge of the Analogix bridge can only be 
determined after the entire probe process is fully completed.

rockchip_dp_probe()/exynos_dp_probe() ->
   analogix_dp_finish_probe() ->
     analogix_dp_aux_done_probing() ->
       drm_of_find_panel_or_bridge() ->
         rockchip_dp_bind()/exynos_dp_bind() ->
           analogix_dp_bind()->
             devm_drm_bridge_add()

To avoid confusion, I’ve added some comments for this easily 
misunderstood logic to the commit message of v10, so the reasoning is 
clear for everyone.

> 
>>
>>> +    if (ret)
>>> +        goto err_unregister_aux;
>>> +
>>> +    ret = drm_bridge_attach(dp->encoder, bridge, NULL, 
>>> DRM_BRIDGE_ATTACH_NO_CONNECTOR);
>>>       if (ret) {
>>>           DRM_ERROR("failed to create bridge (%d)\n", ret);
>>>           goto err_unregister_aux;
> 

Best regards,
Damon