On Fri, Sep 12, 2025 at 04:58:39PM +0800, 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().
>
> Signed-off-by: Damon Ding <damon.d...@rock-chips.com>
>
> ------
>
> Changes in v2:
> - For &drm_bridge.ops, remove DRM_BRIDGE_OP_HPD and add
> DRM_BRIDGE_OP_EDID.
> - Add analogix_dp_bridge_edid_read().
> - Move &analogix_dp_plat_data.skip_connector deletion to the previous
> patches.
>
> Changes in v3:
> - Rebase with the new devm_drm_bridge_alloc() related commit
> 48f05c3b4b70 ("drm/bridge: analogix_dp: Use devm_drm_bridge_alloc()
> API").
> - Expand the commit message.
> - Call drm_bridge_get_modes() in analogix_dp_bridge_get_modes() if the
> bridge is available.
> - Remove unnecessary parameter struct drm_connector* for callback
> &analogix_dp_plat_data.attach.
> - In order to decouple the connector driver and the bridge driver, move
> the bridge connector initilization to the Rockchip and Exynos sides.
>
> Changes in v4:
> - Expand analogix_dp_bridge_detect() parameters to &drm_bridge and
> &drm_connector.
> - Rename the &analogix_dp_plat_data.bridge to
> &analogix_dp_plat_data.next_bridge.
>
> Changes in v5:
> - Set the flag fo drm_bridge_attach() to DRM_BRIDGE_ATTACH_NO_CONNECTOR
> for next bridge attachment of Exynos side.
> - Distinguish the &drm_bridge->ops of Analogix bridge based on whether
> the downstream device is a panel, a bridge or neither.
> - Remove the calls to &analogix_dp_plat_data.get_modes().
> ---
> .../drm/bridge/analogix/analogix_dp_core.c | 151 ++++++++----------
> .../drm/bridge/analogix/analogix_dp_core.h | 1 -
> drivers/gpu/drm/exynos/exynos_dp.c | 25 +--
> .../gpu/drm/rockchip/analogix_dp-rockchip.c | 11 +-
> include/drm/bridge/analogix_dp.h | 3 +-
> 5 files changed, 95 insertions(+), 96 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> index 9bf91d7595d6..156114170c4d 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -856,44 +856,38 @@ static int analogix_dp_disable_psr(struct
> analogix_dp_device *dp)
> return analogix_dp_send_psr_spd(dp, &psr_vsc, true);
> }
>
> -static int analogix_dp_get_modes(struct drm_connector *connector)
> +static int analogix_dp_bridge_get_modes(struct drm_bridge *bridge, struct
> drm_connector *connector)
> {
> - struct analogix_dp_device *dp = to_dp(connector);
> - const struct drm_edid *drm_edid;
> + struct analogix_dp_device *dp = to_dp(bridge);
> int num_modes = 0;
>
> - if (dp->plat_data->panel) {
> + if (dp->plat_data->panel)
> num_modes += drm_panel_get_modes(dp->plat_data->panel,
> connector);
> - } else {
> - drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
>
> - drm_edid_connector_update(&dp->connector, drm_edid);
> -
> - if (drm_edid) {
> - num_modes +=
> drm_edid_connector_add_modes(&dp->connector);
> - drm_edid_free(drm_edid);
> - }
> - }
> + if (dp->plat_data->next_bridge)
> + num_modes += drm_bridge_get_modes(dp->plat_data->next_bridge,
> connector);
This will be already handled by drm_bridge_connector, it will use the
last bridge in chain which implements OP_EDID or OP_MODES (with OP_EDID
having higher priority).
>
> return num_modes;
> }
>
> -static struct drm_encoder *
> -analogix_dp_best_encoder(struct drm_connector *connector)
> +static const struct drm_edid *analogix_dp_bridge_edid_read(struct drm_bridge
> *bridge,
> + struct drm_connector
> *connector)
> {
> - struct analogix_dp_device *dp = to_dp(connector);
> + struct analogix_dp_device *dp = to_dp(bridge);
> + const struct drm_edid *drm_edid = NULL;
>
> - return dp->encoder;
> -}
> + drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
return drm_edid_read_ddc(...);
>
> + return drm_edid;
> +}
>
> -static int analogix_dp_atomic_check(struct drm_connector *connector,
> - struct drm_atomic_state *state)
> +static int analogix_dp_bridge_atomic_check(struct drm_bridge *bridge,
> + struct drm_bridge_state
> *bridge_state,
> + struct drm_crtc_state *crtc_state,
> + struct drm_connector_state
> *conn_state)
> {
> - struct analogix_dp_device *dp = to_dp(connector);
> - struct drm_display_info *di = &connector->display_info;
> - struct drm_connector_state *conn_state;
> - struct drm_crtc_state *crtc_state;
> + struct analogix_dp_device *dp = to_dp(bridge);
> + struct drm_display_info *di = &conn_state->connector->display_info;
> u32 mask = DRM_COLOR_FORMAT_YCBCR444 | DRM_COLOR_FORMAT_YCBCR422;
>
> if (is_rockchip(dp->plat_data->dev_type)) {
> @@ -905,35 +899,18 @@ static int analogix_dp_atomic_check(struct
> drm_connector *connector,
> }
> }
>
> - conn_state = drm_atomic_get_new_connector_state(state, connector);
> - if (WARN_ON(!conn_state))
> - return -ENODEV;
> -
> conn_state->self_refresh_aware = true;
>
> - if (!conn_state->crtc)
> - return 0;
> -
> - crtc_state = drm_atomic_get_new_crtc_state(state, conn_state->crtc);
> - if (!crtc_state)
> - return 0;
> -
> if (crtc_state->self_refresh_active && !dp->psr_supported)
> return -EINVAL;
>
> return 0;
> }
>
> -static const struct drm_connector_helper_funcs
> analogix_dp_connector_helper_funcs = {
> - .get_modes = analogix_dp_get_modes,
> - .best_encoder = analogix_dp_best_encoder,
> - .atomic_check = analogix_dp_atomic_check,
> -};
> -
> static enum drm_connector_status
> -analogix_dp_detect(struct drm_connector *connector, bool force)
> +analogix_dp_bridge_detect(struct drm_bridge *bridge, struct drm_connector
> *connector)
> {
> - struct analogix_dp_device *dp = to_dp(connector);
> + struct analogix_dp_device *dp = to_dp(bridge);
> enum drm_connector_status status = connector_status_disconnected;
>
> if (dp->plat_data->panel)
> @@ -945,21 +922,11 @@ analogix_dp_detect(struct drm_connector *connector,
> bool force)
> return status;
> }
>
> -static const struct drm_connector_funcs analogix_dp_connector_funcs = {
> - .fill_modes = drm_helper_probe_single_connector_modes,
> - .detect = analogix_dp_detect,
> - .destroy = drm_connector_cleanup,
> - .reset = drm_atomic_helper_connector_reset,
> - .atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
> - .atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
> -};
> -
> static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
> struct drm_encoder *encoder,
> enum drm_bridge_attach_flags flags)
> {
> struct analogix_dp_device *dp = to_dp(bridge);
> - struct drm_connector *connector = NULL;
> int ret = 0;
>
> if (flags & DRM_BRIDGE_ATTACH_NO_CONNECTOR) {
> @@ -967,31 +934,8 @@ static int analogix_dp_bridge_attach(struct drm_bridge
> *bridge,
> return -EINVAL;
> }
>
> - if (!dp->plat_data->next_bridge) {
> - connector = &dp->connector;
> - connector->polled = DRM_CONNECTOR_POLL_HPD;
> -
> - ret = drm_connector_init(dp->drm_dev, connector,
> - &analogix_dp_connector_funcs,
> - DRM_MODE_CONNECTOR_eDP);
> - if (ret) {
> - DRM_ERROR("Failed to initialize connector with drm\n");
> - return ret;
> - }
> -
> - drm_connector_helper_add(connector,
> - &analogix_dp_connector_helper_funcs);
> - drm_connector_attach_encoder(connector, encoder);
> - }
> -
> - /*
> - * NOTE: the connector registration is implemented in analogix
> - * platform driver, that to say connector would be exist after
> - * plat_data->attch return, that's why we record the connector
> - * point after plat attached.
> - */
> if (dp->plat_data->attach) {
> - ret = dp->plat_data->attach(dp->plat_data, bridge, connector);
> + ret = dp->plat_data->attach(dp->plat_data, bridge);
> if (ret) {
> DRM_ERROR("Failed at platform attach func\n");
> return ret;
> @@ -1095,14 +1039,21 @@ static int analogix_dp_set_bridge(struct
> analogix_dp_device *dp)
> }
>
> static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
> + struct drm_atomic_state *state,
> const struct drm_display_mode *mode)
> {
> struct analogix_dp_device *dp = to_dp(bridge);
> - struct drm_display_info *display_info = &dp->connector.display_info;
> struct video_info *video = &dp->video_info;
> struct device_node *dp_node = dp->dev->of_node;
> + struct drm_connector *connector;
> + struct drm_display_info *display_info;
> int vic;
>
> + connector = drm_atomic_get_new_connector_for_encoder(state,
> bridge->encoder);
> + if (!connector)
> + return;
> + display_info = &connector->display_info;
> +
> /* Input video interlaces & hsync pol & vsync pol */
> video->interlaced = !!(mode->flags & DRM_MODE_FLAG_INTERLACE);
> video->v_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NVSYNC);
> @@ -1186,7 +1137,7 @@ static void analogix_dp_bridge_atomic_enable(struct
> drm_bridge *bridge,
> new_crtc_state = drm_atomic_get_new_crtc_state(old_state, crtc);
> if (!new_crtc_state)
> return;
> - analogix_dp_bridge_mode_set(bridge, &new_crtc_state->adjusted_mode);
> + analogix_dp_bridge_mode_set(bridge, old_state,
> &new_crtc_state->adjusted_mode);
>
> old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
> /* Not a full enable, just disable PSR and continue */
> @@ -1302,7 +1253,11 @@ static const struct drm_bridge_funcs
> analogix_dp_bridge_funcs = {
> .atomic_enable = analogix_dp_bridge_atomic_enable,
> .atomic_disable = analogix_dp_bridge_atomic_disable,
> .atomic_post_disable = analogix_dp_bridge_atomic_post_disable,
> + .atomic_check = analogix_dp_bridge_atomic_check,
> .attach = analogix_dp_bridge_attach,
> + .get_modes = analogix_dp_bridge_get_modes,
> + .edid_read = analogix_dp_bridge_edid_read,
> + .detect = analogix_dp_bridge_detect,
> };
>
> static int analogix_dp_dt_parse_pdata(struct analogix_dp_device *dp)
> @@ -1532,6 +1487,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 +1501,23 @@ 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)
> + /* If the next is a panel, the EDID parsing is checked by the
> panel driver */
> + bridge->ops = DRM_BRIDGE_OP_MODES | DRM_BRIDGE_OP_DETECT;
> + else if (dp->plat_data->next_bridge)
> + /* If the next is a bridge, the supported operations depend on
> the next bridge */
> + bridge->ops = 0;
And what if the next bridge is dp_connector without a separate HPD pin?
> + else
> + /* In DP mode, the EDID parsing and HPD detection should be
> supported */
> + 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);
> + if (ret)
> + goto err_unregister_aux;
> +
> + ret = drm_bridge_attach(dp->encoder, bridge, NULL, 0);
> if (ret) {
> DRM_ERROR("failed to create bridge (%d)\n", ret);
> goto err_unregister_aux;
--
With best wishes
Dmitry
