Re: [PATCH v9 10/15] drm/bridge: analogix_dp: Add new API analogix_dp_finish_probe()

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

Hi Luca,

On 3/9/2026 7:51 PM, Damon Ding wrote:
> Hi Luca,
> 
> On 3/3/2026 5:54 PM, Luca Ceresoli wrote:
>> Hello Damon,
>>
>> On Tue Feb 10, 2026 at 8:12 AM CET, Damon Ding wrote:
>>> Since the panel/bridge should logically be positioned behind the
>>> Analogix bridge in the display pipeline, it makes sense to handle
>>> the panel/bridge parsing on the Analogix side. Therefore, we add
>>> a new API analogix_dp_finish_probe(), which combines the panel/bridge
>>> parsing with component addition, to do it.
>>>
>>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>>> Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
>>> Tested-by: Marek Szyprowski <m.szyprowski@samsung.com>
>>> Tested-by: Heiko Stuebner <heiko@sntech.de> (on rk3588)
>>
>> ...
>>
>>> @@ -1581,6 +1583,52 @@ struct drm_dp_aux *analogix_dp_get_aux(struct 
>>> analogix_dp_device *dp)
>>>   }
>>>   EXPORT_SYMBOL_GPL(analogix_dp_get_aux);
>>>
>>> +static int analogix_dp_aux_done_probing(struct drm_dp_aux *aux)
>>> +{
>>> +    struct analogix_dp_device *dp = to_dp(aux);
>>> +    struct analogix_dp_plat_data *plat_data = dp->plat_data;
>>> +    int port = plat_data->dev_type == EXYNOS_DP ? 0 : 1;
>>> +    int ret;
>>> +
>>> +    /*
>>> +     * If drm_of_find_panel_or_bridge() returns -ENODEV, there may 
>>> be no valid panel
>>> +     * or bridge nodes. The driver should go on for the driver-free 
>>> bridge or the DP
>>> +     * mode applications.
>>> +     */
>>> +    ret = drm_of_find_panel_or_bridge(dp->dev->of_node, port, 0,
>>> +                      &plat_data->panel, &plat_data->next_bridge);
>>> +    if (ret && ret != -ENODEV)
>>> +        return ret;
>>> +
>>> +    return component_add(dp->dev, plat_data->ops);
>>> +}
>>> +
>>> +int analogix_dp_finish_probe(struct analogix_dp_device *dp)
>>> +{
>>> +    int ret;
>>> +
>>> +    ret = devm_of_dp_aux_populate_bus(&dp->aux, 
>>> analogix_dp_aux_done_probing);
>>> +    if (ret) {
>>> +        /*
>>> +         * If devm_of_dp_aux_populate_bus() returns -ENODEV, the 
>>> done_probing() will
>>> +         * not be called because there are no EP devices. Then the 
>>> callback function
>>> +         * analogix_dp_aux_done_probing() will be called directly in 
>>> order to support
>>> +         * the other valid DT configurations.
>>> +         *
>>> +         * NOTE: The devm_of_dp_aux_populate_bus() is allowed to 
>>> return -EPROBE_DEFER.
>>
>> Uhm, if it is allowed to return -EPROBE_DEFER...
>>
>>> +         */
>>> +        if (ret != -ENODEV) {
>>> +            dev_err(dp->dev, "failed to populate aux bus\n");
>>> +            return ret;
>>> +        }
>>
>> ...then you shouldn't dev_err() when -EPROBE_DEFER is returned.
>>
>> Either use dev_err_probe() (which would also simplify your code) or check
>> for if (ret != -ENODEV && ret != -EPROBE_DEFER).
> 
> Will fix in v10.
> 
>>
>>> +
>>> +        return analogix_dp_aux_done_probing(&dp->aux);
>>> +    }
>>> +
>>> +    return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(analogix_dp_finish_probe);
>>> +
>>>   MODULE_AUTHOR("Jingoo Han <jg1.han@samsung.com>");
>>>   MODULE_DESCRIPTION("Analogix DP Core Driver");
>>>   MODULE_LICENSE("GPL v2");
>>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/ 
>>> analogix_dp.h
>>> index 3428ffff24c5..bae969dec63a 100644
>>> --- a/include/drm/bridge/analogix_dp.h
>>> +++ b/include/drm/bridge/analogix_dp.h
>>> @@ -30,6 +30,7 @@ struct analogix_dp_plat_data {
>>>       struct drm_bridge *next_bridge;
>>>       struct drm_encoder *encoder;
>>>       struct drm_connector *connector;
>>> +    const struct component_ops *ops;
>>
>> Is adding a new stored field a good idea? Can it be instead passed as an
>> argument to analogix_dp_finish_probe()?
>>
>> Note I don't have a strong opinion here, just the added struct field 
>> seems
>> overkill for being used just once.
>>
> 
> I agree this is a better approach, since the &component_ops is only used 
> during probing.

Sorry for another shamefully incorrect reply!

The &dp_aux_ep_device_with_data.done_probing only supports passing the 
&drm_dp_aux parameter, meaning we can't directly pass &component_ops.

I'll also add extra context to the commit msg – similar to what I did 
for v9 patch 9/15 – so everyone understands why this change was made and 
avoids confusion.

> 
>>> @@ -49,5 +50,6 @@ int analogix_dp_stop_crc(struct drm_connector 
>>> *connector);
>>>
>>>   struct analogix_dp_plat_data *analogix_dp_aux_to_plat_data(struct 
>>> drm_dp_aux *aux);
>>>   struct drm_dp_aux *analogix_dp_get_aux(struct analogix_dp_device *dp);
>>> +int analogix_dp_finish_probe(struct analogix_dp_device *dp);
>>>
>>>   #endif /* _ANALOGIX_DP_H_ */
>>
> 

Best regards,
Damon