bridge: analogix_dp: Apply drm_bridge_connector helper

Damon Ding Wed, 20 Aug 2025 02:19:00 -0700

Hi Dmitry,

On 8/17/2025 12:43 AM, Dmitry Baryshkov wrote:

On Thu, Aug 14, 2025 at 06:47:47PM +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.
---
  .../drm/bridge/analogix/analogix_dp_core.c    | 145 ++++++++----------
  .../drm/bridge/analogix/analogix_dp_core.h    |   1 -
  drivers/gpu/drm/exynos/exynos_dp.c            |  18 ++-
  .../gpu/drm/rockchip/analogix_dp-rockchip.c   |  11 +-
  include/drm/bridge/analogix_dp.h              |   3 +-
  5 files changed, 88 insertions(+), 90 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index 7876b310aaed..a8ed44ec8ef5 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -947,24 +947,16 @@ 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);


If there is a next bridge which provides OP_MODES, then
drm_bridge_connector will use it for get_modes() and skip this one
completely. I'm not sure what's the value of this call.

Following your advice, it is really a good idea to distinguish thedrm_bridge_ops between the panel and the bridge. Will add it in v5.

if (dp->plat_data->get_modes)

                num_modes += dp->plat_data->get_modes(dp->plat_data, connector);
@@ -972,51 +964,39 @@ static int analogix_dp_get_modes(struct drm_connector 
*connector)
        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);

+ if (dp->plat_data->get_modes)

+               dp->plat_data->get_modes(dp->plat_data, connector);



So, we have DDC, but we still want to return platform modes? What is the
usecase for that?

There might be some, but I think it deserves a comment in the source
file.

For Rockchip side, since RK3588 and RK3576 can support YUV formats whilethe other can not, the &analogix_dp_plat_data.get_modes() help filterout YUV formats for some platforms(The YUV feature support may not befit for this patch series and will come later).


For Exynos side, I think &analogix_dp_plat_data.get_modes() can help

parse the video mode set in the eDP DT node when there is no availablepanel or bridge.


I will add some comments about it in the next version.

-static int analogix_dp_atomic_check(struct drm_connector *connector,

-                                   struct drm_atomic_state *state)
-{
-       struct analogix_dp_device *dp = to_dp(connector);
-       struct drm_connector_state *conn_state;
-       struct drm_crtc_state *crtc_state;
+       return drm_edid;
+}

- conn_state = drm_atomic_get_new_connector_state(state, connector);

-       if (WARN_ON(!conn_state))
-               return -ENODEV;
+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(bridge);

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)

@@ -1028,21 +1008,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) {

@@ -1050,31 +1020,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;
@@ -1178,14 +1125,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);
@@ -1269,7 +1223,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 */
@@ -1385,7 +1339,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)

@@ -1615,6 +1573,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;

@@ -1628,7 +1587,16 @@ 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);

+       bridge->ops = DRM_BRIDGE_OP_DETECT |
+                     DRM_BRIDGE_OP_EDID |


Should this be limited to the !panel cases? Otherwise OP_EDID overrides
OP_MODES and the analogix_dp_bridge_get_modes() will never be called.

+                     DRM_BRIDGE_OP_MODES;
+       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;
@@ -1646,7 +1614,6 @@ EXPORT_SYMBOL_GPL(analogix_dp_bind);
  void analogix_dp_unbind(struct analogix_dp_device *dp)
  {
        analogix_dp_bridge_disable(&dp->bridge);
-       dp->connector.funcs->destroy(&dp->connector);

drm_panel_unprepare(dp->plat_data->panel);@@ -1656,7 +1623,8 @@ EXPORT_SYMBOL_GPL(analogix_dp_unbind);int analogix_dp_start_crc(struct drm_connector *connector)

  {
-       struct analogix_dp_device *dp = to_dp(connector);
+       struct analogix_dp_device *dp;
+       struct drm_bridge *bridge;

if (!connector->state->crtc) {

                DRM_ERROR("Connector %s doesn't currently have a CRTC.\n",
@@ -1664,13 +1632,26 @@ int analogix_dp_start_crc(struct drm_connector 
*connector)
                return -EINVAL;
        }

+ bridge = drm_bridge_chain_get_first_bridge(connector->encoder);

+       if (bridge->type != DRM_MODE_CONNECTOR_eDP)
+               return -EINVAL;
+
+       dp = to_dp(bridge);
+
        return drm_dp_start_crc(&dp->aux, connector->state->crtc);
  }
  EXPORT_SYMBOL_GPL(analogix_dp_start_crc);

int analogix_dp_stop_crc(struct drm_connector *connector)

  {
-       struct analogix_dp_device *dp = to_dp(connector);
+       struct analogix_dp_device *dp;
+       struct drm_bridge *bridge;
+
+       bridge = drm_bridge_chain_get_first_bridge(connector->encoder);
+       if (bridge->type != DRM_MODE_CONNECTOR_eDP)
+               return -EINVAL;
+
+       dp = to_dp(bridge);

return drm_dp_stop_crc(&dp->aux);

  }
diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h 
b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 91b215c6a0cf..17347448c6b0 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -154,7 +154,6 @@ struct analogix_dp_device {
        struct drm_encoder      *encoder;
        struct device           *dev;
        struct drm_device       *drm_dev;
-       struct drm_connector    connector;
        struct drm_bridge       bridge;
        struct drm_dp_aux       aux;
        struct clk              *clock;
diff --git a/drivers/gpu/drm/exynos/exynos_dp.c 
b/drivers/gpu/drm/exynos/exynos_dp.c
index 702128d76ae3..65579873ceea 100644
--- a/drivers/gpu/drm/exynos/exynos_dp.c
+++ b/drivers/gpu/drm/exynos/exynos_dp.c
@@ -21,6 +21,7 @@
  #include <drm/bridge/analogix_dp.h>
  #include <drm/drm_atomic_helper.h>
  #include <drm/drm_bridge.h>
+#include <drm/drm_bridge_connector.h>
  #include <drm/drm_crtc.h>
  #include <drm/drm_of.h>
  #include <drm/drm_panel.h>
@@ -95,8 +96,7 @@ static int exynos_dp_get_modes(struct analogix_dp_plat_data 
*plat_data,
  }

static int exynos_dp_bridge_attach(struct analogix_dp_plat_data *plat_data,

-                                  struct drm_bridge *bridge,
-                                  struct drm_connector *connector)
+                                  struct drm_bridge *bridge)
  {
        struct exynos_dp_device *dp = to_dp(plat_data);
        int ret;
@@ -147,6 +147,7 @@ static int exynos_dp_bind(struct device *dev, struct device 
*master, void *data)
        struct exynos_dp_device *dp = dev_get_drvdata(dev);
        struct drm_encoder *encoder = &dp->encoder;
        struct drm_device *drm_dev = data;
+       struct drm_connector *connector;
        int ret;

dp->drm_dev = drm_dev;

@@ -168,10 +169,19 @@ static int exynos_dp_bind(struct device *dev, struct 
device *master, void *data)
        dp->plat_data.encoder = encoder;

ret = analogix_dp_bind(dp->adp, dp->drm_dev);

-       if (ret)
+       if (ret) {
                dp->encoder.funcs->destroy(&dp->encoder);
+               return ret;
+       }
+
+       connector = drm_bridge_connector_init(dp->drm_dev, 
dp->plat_data.encoder);
+       if (IS_ERR(connector)) {
+               ret = PTR_ERR(connector);
+               dev_err(dp->dev, "Failed to initialize bridge_connector\n");
+               return ret;
+       }

- return ret;

+       return drm_connector_attach_encoder(connector, dp->plat_data.encoder);
  }

static void exynos_dp_unbind(struct device *dev, struct device *master,

diff --git a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c 
b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
index d30f0983a53a..87dfb48206db 100644
--- a/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
+++ b/drivers/gpu/drm/rockchip/analogix_dp-rockchip.c
@@ -25,6 +25,7 @@
  #include <drm/display/drm_dp_helper.h>
  #include <drm/drm_atomic.h>
  #include <drm/drm_atomic_helper.h>
+#include <drm/drm_bridge_connector.h>
  #include <drm/bridge/analogix_dp.h>
  #include <drm/drm_of.h>
  #include <drm/drm_panel.h>
@@ -394,6 +395,7 @@ static int rockchip_dp_bind(struct device *dev, struct 
device *master,
  {
        struct rockchip_dp_device *dp = dev_get_drvdata(dev);
        struct drm_device *drm_dev = data;
+       struct drm_connector *connector;
        int ret;

dp->drm_dev = drm_dev;

@@ -413,7 +415,14 @@ static int rockchip_dp_bind(struct device *dev, struct 
device *master,
        if (ret)
                goto err_cleanup_encoder;

- return 0;

+       connector = drm_bridge_connector_init(dp->drm_dev, 
dp->plat_data.encoder);
+       if (IS_ERR(connector)) {
+               ret = PTR_ERR(connector);
+               dev_err(dp->dev, "Failed to initialize bridge_connector\n");
+               goto err_cleanup_encoder;
+       }
+
+       return drm_connector_attach_encoder(connector, dp->plat_data.encoder);
  err_cleanup_encoder:
        dp->encoder.encoder.funcs->destroy(&dp->encoder.encoder);
        return ret;
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index f06da105d8f2..ffc05f3de232 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -33,8 +33,7 @@ struct analogix_dp_plat_data {

int (*power_on)(struct analogix_dp_plat_data *);

        int (*power_off)(struct analogix_dp_plat_data *);
-       int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *,
-                     struct drm_connector *);
+       int (*attach)(struct analogix_dp_plat_data *, struct drm_bridge *);
        int (*get_modes)(struct analogix_dp_plat_data *,
                         struct drm_connector *);
  };
--
2.34.1


Best regards,
Damon

Re: [PATCH v4 07/13] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

Reply via email to