[PATCH v1 0/3] Apply drm_bridge_connector helper for the Analogix DP driver

[PATCH v1 0/3] Apply drm_bridge_connector helper for the Analogix DP driver

[Damon Ding]

PATCH 1 is a small format optimization.
PATCH 2 is to perform mode setting in &drm_bridge_funcs.atomic_enable.
PATCH 3 is to apply the drm_bridge_connector helper.

Damon Ding (3):
  drm/bridge: analogix_dp: Formalize the struct analogid_dp_device
  drm/bridge: analogix_dp: Move &drm_bridge_funcs.mode_set to
    &drm_bridge_funcs.atomic_enable
  drm/bridge: analogix_dp: Apply drm_bridge_connector helper

 .../drm/bridge/analogix/analogix_dp_core.c    | 312 +++++++++---------
 .../drm/bridge/analogix/analogix_dp_core.h    |   8 +-
 include/drm/bridge/analogix_dp.h              |   1 -
 3 files changed, 153 insertions(+), 168 deletions(-)

-- 
2.34.1

[PATCH v1 1/3] drm/bridge: analogix_dp: Formalize the struct analogid_dp_device

[Damon Ding]

Use the tap instead of the space for &analogix_dp_device.aux and
&analogix_dp_device.force_hpd.

Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
---
 drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
index 2b54120ba4a3..9f9e492da80f 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -155,7 +155,7 @@ struct analogix_dp_device {
 	struct drm_device	*drm_dev;
 	struct drm_connector	connector;
 	struct drm_bridge	*bridge;
-	struct drm_dp_aux       aux;
+	struct drm_dp_aux	aux;
 	struct clk		*clock;
 	unsigned int		irq;
 	void __iomem		*reg_base;
@@ -165,7 +165,7 @@ struct analogix_dp_device {
 	struct phy		*phy;
 	int			dpms_mode;
 	struct gpio_desc	*hpd_gpiod;
-	bool                    force_hpd;
+	bool			force_hpd;
 	bool			fast_train_enable;
 	bool			psr_supported;
 
-- 
2.34.1

[PATCH v1 2/3] drm/bridge: analogix_dp: Move &drm_bridge_funcs.mode_set to &drm_bridge_funcs.atomic_enable

[Damon Ding]

According to the include/drm/drm_bridge.h, the callback
&drm_bridge_funcs.mode_set is deprecated and it should be better to
include the mode setting in the &drm_bridge_funcs.atomic_enable instead.

Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 161 +++++++++---------
 1 file changed, 82 insertions(+), 79 deletions(-)

diff --git a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
index a761941bc3c2..2c51d3193120 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -1176,12 +1176,88 @@ static int analogix_dp_set_bridge(struct analogix_dp_device *dp)
 	return ret;
 }
 
+static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
+					const struct drm_display_mode *mode)
+{
+	struct analogix_dp_device *dp = bridge->driver_private;
+	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;
+	int vic;
+
+	/* 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);
+	video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
+
+	/* Input video dynamic_range & colorimetry */
+	vic = drm_match_cea_mode(mode);
+	if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) ||
+	    (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) {
+		video->dynamic_range = CEA;
+		video->ycbcr_coeff = COLOR_YCBCR601;
+	} else if (vic) {
+		video->dynamic_range = CEA;
+		video->ycbcr_coeff = COLOR_YCBCR709;
+	} else {
+		video->dynamic_range = VESA;
+		video->ycbcr_coeff = COLOR_YCBCR709;
+	}
+
+	/* Input vide bpc and color_formats */
+	switch (display_info->bpc) {
+	case 12:
+		video->color_depth = COLOR_12;
+		break;
+	case 10:
+		video->color_depth = COLOR_10;
+		break;
+	case 8:
+		video->color_depth = COLOR_8;
+		break;
+	case 6:
+		video->color_depth = COLOR_6;
+		break;
+	default:
+		video->color_depth = COLOR_8;
+		break;
+	}
+	if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
+		video->color_space = COLOR_YCBCR444;
+	else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
+		video->color_space = COLOR_YCBCR422;
+	else
+		video->color_space = COLOR_RGB;
+
+	/*
+	 * NOTE: those property parsing code is used for providing backward
+	 * compatibility for samsung platform.
+	 * Due to we used the "of_property_read_u32" interfaces, when this
+	 * property isn't present, the "video_info" can keep the original
+	 * values and wouldn't be modified.
+	 */
+	of_property_read_u32(dp_node, "samsung,color-space",
+			     &video->color_space);
+	of_property_read_u32(dp_node, "samsung,dynamic-range",
+			     &video->dynamic_range);
+	of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
+			     &video->ycbcr_coeff);
+	of_property_read_u32(dp_node, "samsung,color-depth",
+			     &video->color_depth);
+	if (of_property_read_bool(dp_node, "hsync-active-high"))
+		video->h_sync_polarity = true;
+	if (of_property_read_bool(dp_node, "vsync-active-high"))
+		video->v_sync_polarity = true;
+	if (of_property_read_bool(dp_node, "interlaced"))
+		video->interlaced = true;
+}
+
 static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 					     struct drm_atomic_state *old_state)
 {
 	struct analogix_dp_device *dp = bridge->driver_private;
 	struct drm_crtc *crtc;
-	struct drm_crtc_state *old_crtc_state;
+	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int timeout_loop = 0;
 	int ret;
 
@@ -1189,6 +1265,11 @@ static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 	if (!crtc)
 		return;
 
+	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);
+
 	old_crtc_state = drm_atomic_get_old_crtc_state(old_state, crtc);
 	/* Not a full enable, just disable PSR and continue */
 	if (old_crtc_state && old_crtc_state->self_refresh_active) {
@@ -1295,83 +1376,6 @@ static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
 		DRM_ERROR("Failed to enable psr (%d)\n", ret);
 }
 
-static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
-				const struct drm_display_mode *orig_mode,
-				const struct drm_display_mode *mode)
-{
-	struct analogix_dp_device *dp = bridge->driver_private;
-	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;
-	int vic;
-
-	/* 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);
-	video->h_sync_polarity = !!(mode->flags & DRM_MODE_FLAG_NHSYNC);
-
-	/* Input video dynamic_range & colorimetry */
-	vic = drm_match_cea_mode(mode);
-	if ((vic == 6) || (vic == 7) || (vic == 21) || (vic == 22) ||
-	    (vic == 2) || (vic == 3) || (vic == 17) || (vic == 18)) {
-		video->dynamic_range = CEA;
-		video->ycbcr_coeff = COLOR_YCBCR601;
-	} else if (vic) {
-		video->dynamic_range = CEA;
-		video->ycbcr_coeff = COLOR_YCBCR709;
-	} else {
-		video->dynamic_range = VESA;
-		video->ycbcr_coeff = COLOR_YCBCR709;
-	}
-
-	/* Input vide bpc and color_formats */
-	switch (display_info->bpc) {
-	case 12:
-		video->color_depth = COLOR_12;
-		break;
-	case 10:
-		video->color_depth = COLOR_10;
-		break;
-	case 8:
-		video->color_depth = COLOR_8;
-		break;
-	case 6:
-		video->color_depth = COLOR_6;
-		break;
-	default:
-		video->color_depth = COLOR_8;
-		break;
-	}
-	if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR444)
-		video->color_space = COLOR_YCBCR444;
-	else if (display_info->color_formats & DRM_COLOR_FORMAT_YCBCR422)
-		video->color_space = COLOR_YCBCR422;
-	else
-		video->color_space = COLOR_RGB;
-
-	/*
-	 * NOTE: those property parsing code is used for providing backward
-	 * compatibility for samsung platform.
-	 * Due to we used the "of_property_read_u32" interfaces, when this
-	 * property isn't present, the "video_info" can keep the original
-	 * values and wouldn't be modified.
-	 */
-	of_property_read_u32(dp_node, "samsung,color-space",
-			     &video->color_space);
-	of_property_read_u32(dp_node, "samsung,dynamic-range",
-			     &video->dynamic_range);
-	of_property_read_u32(dp_node, "samsung,ycbcr-coeff",
-			     &video->ycbcr_coeff);
-	of_property_read_u32(dp_node, "samsung,color-depth",
-			     &video->color_depth);
-	if (of_property_read_bool(dp_node, "hsync-active-high"))
-		video->h_sync_polarity = true;
-	if (of_property_read_bool(dp_node, "vsync-active-high"))
-		video->v_sync_polarity = true;
-	if (of_property_read_bool(dp_node, "interlaced"))
-		video->interlaced = true;
-}
-
 static const struct drm_bridge_funcs analogix_dp_bridge_funcs = {
 	.atomic_duplicate_state = drm_atomic_helper_bridge_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_bridge_destroy_state,
@@ -1380,7 +1384,6 @@ 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,
-	.mode_set = analogix_dp_bridge_mode_set,
 	.attach = analogix_dp_bridge_attach,
 };
 
-- 
2.34.1

[PATCH v1 3/3] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

[Damon Ding]

Apply drm_bridge_connector helper for Analogix DP driver.

The following changes have been made:
- Remove &analogix_dp_device.connector and change
  &analogix_dp_device.bridge from a pointer to an instance.
- Apply drm_bridge_connector helper to get rid of &drm_connector_funcs
  and &drm_connector_helper_funcs.
- Remove &analogix_dp_plat_data.skip_connector.

Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
---
 .../drm/bridge/analogix/analogix_dp_core.c    | 157 ++++++++----------
 .../drm/bridge/analogix/analogix_dp_core.h    |   4 +-
 include/drm/bridge/analogix_dp.h              |   1 -
 3 files changed, 72 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 2c51d3193120..d67afd63d999 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
@@ -22,6 +22,7 @@
 #include <drm/drm_atomic.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_device.h>
 #include <drm/drm_edid.h>
@@ -946,9 +947,10 @@ 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);
+	struct analogix_dp_device *dp = to_dp(bridge);
 	const struct drm_edid *drm_edid;
 	int num_modes = 0;
 
@@ -957,10 +959,10 @@ static int analogix_dp_get_modes(struct drm_connector *connector)
 	} else {
 		drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
 
-		drm_edid_connector_update(&dp->connector, drm_edid);
+		drm_edid_connector_update(connector, drm_edid);
 
 		if (drm_edid) {
-			num_modes += drm_edid_connector_add_modes(&dp->connector);
+			num_modes += drm_edid_connector_add_modes(connector);
 			drm_edid_free(drm_edid);
 		}
 	}
@@ -971,51 +973,25 @@ 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 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);
-
-	return dp->encoder;
-}
-
-
-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;
-
-	conn_state = drm_atomic_get_new_connector_state(state, connector);
-	if (WARN_ON(!conn_state))
-		return -ENODEV;
+	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 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)
@@ -1027,20 +1003,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 = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_connector *connector = NULL;
 	int ret = 0;
 
@@ -1049,23 +1016,15 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
 		return -EINVAL;
 	}
 
-	if (!dp->plat_data->skip_connector) {
-		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);
+	connector = drm_bridge_connector_init(dp->drm_dev, encoder);
+	if (IS_ERR(connector)) {
+		ret = PTR_ERR(connector);
+		dev_err(dp->dev, "Failed to initialize connector with drm\n");
+		return ret;
 	}
 
+	drm_connector_attach_encoder(connector, encoder);
+
 	/*
 	 * NOTE: the connector registration is implemented in analogix
 	 * platform driver, that to say connector would be exist after
@@ -1124,7 +1083,7 @@ struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
 static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
 						 struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state;
 
@@ -1177,14 +1136,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 = bridge->driver_private;
-	struct drm_display_info *display_info = &dp->connector.display_info;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	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);
@@ -1255,7 +1221,7 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
 static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 					     struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
 	int timeout_loop = 0;
@@ -1268,7 +1234,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 */
@@ -1297,7 +1263,7 @@ static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
 
 static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 
 	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
 		return;
@@ -1320,7 +1286,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
 static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 					      struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *old_crtc, *new_crtc;
 	struct drm_crtc_state *old_crtc_state = NULL;
 	struct drm_crtc_state *new_crtc_state = NULL;
@@ -1358,7 +1324,7 @@ static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
 static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
 						   struct drm_atomic_state *old_state)
 {
-	struct analogix_dp_device *dp = bridge->driver_private;
+	struct analogix_dp_device *dp = to_dp(bridge);
 	struct drm_crtc *crtc;
 	struct drm_crtc_state *new_crtc_state;
 	int ret;
@@ -1384,24 +1350,27 @@ 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,
+	.detect = analogix_dp_bridge_detect,
 };
 
 static int analogix_dp_create_bridge(struct drm_device *drm_dev,
 				     struct analogix_dp_device *dp)
 {
-	struct drm_bridge *bridge;
-
-	bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge), GFP_KERNEL);
-	if (!bridge) {
-		DRM_ERROR("failed to allocate for drm bridge\n");
-		return -ENOMEM;
-	}
+	struct drm_bridge *bridge = &dp->bridge;
+	int ret;
 
-	dp->bridge = bridge;
+	bridge->ops = DRM_BRIDGE_OP_DETECT |
+		      DRM_BRIDGE_OP_HPD |
+		      DRM_BRIDGE_OP_MODES;
+	bridge->of_node = dp->dev->of_node;
+	bridge->type = DRM_MODE_CONNECTOR_eDP;
 
-	bridge->driver_private = dp;
-	bridge->funcs = &analogix_dp_bridge_funcs;
+	ret = devm_drm_bridge_add(dp->dev, &dp->bridge);
+	if (ret)
+		return ret;
 
 	return drm_bridge_attach(dp->encoder, bridge, NULL, 0);
 }
@@ -1493,9 +1462,10 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
 		return ERR_PTR(-EINVAL);
 	}
 
-	dp = devm_kzalloc(dev, sizeof(struct analogix_dp_device), GFP_KERNEL);
-	if (!dp)
-		return ERR_PTR(-ENOMEM);
+	dp = devm_drm_bridge_alloc(dev, struct analogix_dp_device, bridge,
+				   &analogix_dp_bridge_funcs);
+	if (IS_ERR(dp))
+		return ERR_CAST(dp);
 
 	dp->dev = &pdev->dev;
 	dp->dpms_mode = DRM_MODE_DPMS_OFF;
@@ -1670,8 +1640,7 @@ 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);
+	analogix_dp_bridge_disable(&dp->bridge);
 
 	drm_panel_unprepare(dp->plat_data->panel);
 
@@ -1681,7 +1650,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",
@@ -1689,13 +1659,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 9f9e492da80f..22f28384b4ec 100644
--- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
+++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
@@ -10,6 +10,7 @@
 #define _ANALOGIX_DP_CORE_H
 
 #include <drm/display/drm_dp_helper.h>
+#include <drm/drm_bridge.h>
 #include <drm/drm_crtc.h>
 
 #define DP_TIMEOUT_LOOP_COUNT 100
@@ -153,8 +154,7 @@ 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_bridge	bridge;
 	struct drm_dp_aux	aux;
 	struct clk		*clock;
 	unsigned int		irq;
diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
index cf17646c1310..cb9663ff61fb 100644
--- a/include/drm/bridge/analogix_dp.h
+++ b/include/drm/bridge/analogix_dp.h
@@ -29,7 +29,6 @@ struct analogix_dp_plat_data {
 	struct drm_panel *panel;
 	struct drm_encoder *encoder;
 	struct drm_connector *connector;
-	bool skip_connector;
 
 	int (*power_on)(struct analogix_dp_plat_data *);
 	int (*power_off)(struct analogix_dp_plat_data *);
-- 
2.34.1

Re: [PATCH v1 3/3] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

[kernel test robot]

Hi Damon,

kernel test robot noticed the following build errors:

[auto build test ERROR on drm-misc/drm-misc-next]
[also build test ERROR on next-20250526]
[cannot apply to linus/master v6.15]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Damon-Ding/drm-bridge-analogix_dp-Formalize-the-struct-analogid_dp_device/20250526-201358
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20250526120742.3195812-4-damon.ding%40rock-chips.com
patch subject: [PATCH v1 3/3] drm/bridge: analogix_dp: Apply drm_bridge_connector helper
config: arm64-randconfig-001-20250527 (https://download.01.org/0day-ci/archive/20250527/202505270453.H3muv5WW-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 8.5.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250527/202505270453.H3muv5WW-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505270453.H3muv5WW-lkp@intel.com/

All errors (new ones prefixed by >>):

   drivers/gpu/drm/exynos/exynos_dp.c: In function 'exynos_dp_probe':
>> drivers/gpu/drm/exynos/exynos_dp.c:240:16: error: 'struct analogix_dp_plat_data' has no member named 'skip_connector'; did you mean 'connector'?
     dp->plat_data.skip_connector = !!bridge;
                   ^~~~~~~~~~~~~~
                   connector


vim +240 drivers/gpu/drm/exynos/exynos_dp.c

f37cd5e8098441a drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2014-05-09  194  
f37cd5e8098441a drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2014-05-09  195  static int exynos_dp_probe(struct platform_device *pdev)
f37cd5e8098441a drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2014-05-09  196  {
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  197  	struct device *dev = &pdev->dev;
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  198  	struct device_node *np;
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  199  	struct exynos_dp_device *dp;
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  200  	struct drm_panel *panel;
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  201  	struct drm_bridge *bridge;
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  202  	int ret;
df5225bc9a87f15 drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2014-05-29  203  
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  204  	dp = devm_kzalloc(&pdev->dev, sizeof(struct exynos_dp_device),
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  205  			  GFP_KERNEL);
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  206  	if (!dp)
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  207  		return -ENOMEM;
5f1dcd8b7ec8189 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2014-07-31  208  
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  209  	dp->dev = dev;
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  210  	/*
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  211  	 * We just use the drvdata until driver run into component
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  212  	 * add function, and then we would set drvdata to null, so
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  213  	 * that analogix dp driver would take charge of the drvdata.
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  214  	 */
1df6e5fb79f6141 drivers/gpu/drm/exynos/exynos_dp_core.c Andrzej Hajda            2014-11-17  215  	platform_set_drvdata(pdev, dp);
1df6e5fb79f6141 drivers/gpu/drm/exynos/exynos_dp_core.c Andrzej Hajda            2014-11-17  216  
a9fa852886fd5a7 drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2015-11-26  217  	/* This is for the backward compatibility. */
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  218  	np = of_parse_phandle(dev->of_node, "panel", 0);
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  219  	if (np) {
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  220  		dp->plat_data.panel = of_drm_find_panel(np);
5fa8e4a22182df8 drivers/gpu/drm/exynos/exynos_dp.c      Boris Brezillon          2018-05-09  221  
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  222  		of_node_put(np);
5fa8e4a22182df8 drivers/gpu/drm/exynos/exynos_dp.c      Boris Brezillon          2018-05-09  223  		if (IS_ERR(dp->plat_data.panel))
5fa8e4a22182df8 drivers/gpu/drm/exynos/exynos_dp.c      Boris Brezillon          2018-05-09  224  			return PTR_ERR(dp->plat_data.panel);
5fa8e4a22182df8 drivers/gpu/drm/exynos/exynos_dp.c      Boris Brezillon          2018-05-09  225  
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  226  		goto out;
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  227  	}
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  228  
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  229  	ret = drm_of_find_panel_or_bridge(dev->of_node, 0, 0, &panel, &bridge);
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  230  	if (ret)
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  231  		return ret;
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  232  
37e110625eeeaba drivers/gpu/drm/exynos/exynos_dp_core.c Javier Martinez Canillas 2016-01-29  233  	/* The remote port can be either a panel or a bridge */
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  234  	dp->plat_data.panel = panel;
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  235  	dp->plat_data.dev_type = EXYNOS_DP;
6d4618ad04e1a14 drivers/gpu/drm/exynos/exynos_dp.c      Lucas Stach              2024-06-19  236  	dp->plat_data.power_on = exynos_dp_poweron;
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  237  	dp->plat_data.power_off = exynos_dp_poweroff;
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  238  	dp->plat_data.attach = exynos_dp_bridge_attach;
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  239  	dp->plat_data.get_modes = exynos_dp_get_modes;
2e9b3e74b4a184f drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2018-03-05 @240  	dp->plat_data.skip_connector = !!bridge;
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  241  
ebc9446135671b8 drivers/gpu/drm/exynos/exynos_dp.c      Rob Herring              2017-03-29  242  	dp->ptn_bridge = bridge;
801855671ad1dc7 drivers/gpu/drm/exynos/exynos_dp_core.c Ajay Kumar               2015-01-20  243  
a9fa852886fd5a7 drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2015-11-26  244  out:
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  245  	dp->adp = analogix_dp_probe(dev, &dp->plat_data);
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  246  	if (IS_ERR(dp->adp))
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  247  		return PTR_ERR(dp->adp);
152cce0006abf7e drivers/gpu/drm/exynos/exynos_dp.c      Marek Szyprowski         2020-03-10  248  
3424e3a4f844c0a drivers/gpu/drm/exynos/exynos_dp_core.c Yakir Yang               2016-03-29  249  	return component_add(&pdev->dev, &exynos_dp_ops);
f37cd5e8098441a drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2014-05-09  250  }
f37cd5e8098441a drivers/gpu/drm/exynos/exynos_dp_core.c Inki Dae                 2014-05-09  251  

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v1 1/3] drm/bridge: analogix_dp: Formalize the struct analogid_dp_device

[Dmitry Baryshkov]

On Mon, May 26, 2025 at 08:07:40PM +0800, Damon Ding wrote:
> Use the tap instead of the space for &analogix_dp_device.aux and
> &analogix_dp_device.force_hpd.
> 
> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> ---
>  drivers/gpu/drm/bridge/analogix/analogix_dp_core.h | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>

-- 
With best wishes
Dmitry

Re: [PATCH v1 2/3] drm/bridge: analogix_dp: Move &drm_bridge_funcs.mode_set to &drm_bridge_funcs.atomic_enable

[Dmitry Baryshkov]

On Mon, May 26, 2025 at 08:07:41PM +0800, Damon Ding wrote:
> According to the include/drm/drm_bridge.h, the callback
> &drm_bridge_funcs.mode_set is deprecated and it should be better to
> include the mode setting in the &drm_bridge_funcs.atomic_enable instead.
> 
> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 161 +++++++++---------
>  1 file changed, 82 insertions(+), 79 deletions(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v1 3/3] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

[Dmitry Baryshkov]

On Mon, May 26, 2025 at 08:07:42PM +0800, Damon Ding wrote:
> Apply drm_bridge_connector helper for Analogix DP driver.
> 
> The following changes have been made:
> - Remove &analogix_dp_device.connector and change
>   &analogix_dp_device.bridge from a pointer to an instance.
> - Apply drm_bridge_connector helper to get rid of &drm_connector_funcs
>   and &drm_connector_helper_funcs.
> - Remove &analogix_dp_plat_data.skip_connector.

You've missed the exynos_dp.c which uses it.

I think there is slightly more to be handled here. For example, I'd
suggest moving panel / bridge parsing and attachment to the core driver
too. Exynos handles several backwards-compatibility cases, e.g. using
the "panel" property or just passing video timings in the DT node. You
might need to implement instantiating a panel from videomode specially
for exynos_dp.c

This would allow you to cleanup plat_data interface as a preparation for
this patch.

> 
> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
> ---
>  .../drm/bridge/analogix/analogix_dp_core.c    | 157 ++++++++----------
>  .../drm/bridge/analogix/analogix_dp_core.h    |   4 +-
>  include/drm/bridge/analogix_dp.h              |   1 -
>  3 files changed, 72 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 2c51d3193120..d67afd63d999 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
> @@ -22,6 +22,7 @@
>  #include <drm/drm_atomic.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_device.h>
>  #include <drm/drm_edid.h>
> @@ -946,9 +947,10 @@ 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);
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	const struct drm_edid *drm_edid;
>  	int num_modes = 0;
>  
> @@ -957,10 +959,10 @@ static int analogix_dp_get_modes(struct drm_connector *connector)
>  	} else {
>  		drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
>  
> -		drm_edid_connector_update(&dp->connector, drm_edid);
> +		drm_edid_connector_update(connector, drm_edid);
>  
>  		if (drm_edid) {
> -			num_modes += drm_edid_connector_add_modes(&dp->connector);
> +			num_modes += drm_edid_connector_add_modes(connector);
>  			drm_edid_free(drm_edid);
>  		}
>  	}

This should be split into get_modes() and edid_read(). Use
DRM_BRIDGE_OP_ flags to control which implementation is actually being
used.


> @@ -971,51 +973,25 @@ 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 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);
> -
> -	return dp->encoder;
> -}
> -
> -
> -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;
> -
> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
> -	if (WARN_ON(!conn_state))
> -		return -ENODEV;
> +	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 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)
> @@ -1027,20 +1003,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 = bridge->driver_private;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	struct drm_connector *connector = NULL;
>  	int ret = 0;
>  
> @@ -1049,23 +1016,15 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
>  		return -EINVAL;
>  	}
>  
> -	if (!dp->plat_data->skip_connector) {
> -		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);
> +	connector = drm_bridge_connector_init(dp->drm_dev, encoder);
> +	if (IS_ERR(connector)) {
> +		ret = PTR_ERR(connector);
> +		dev_err(dp->dev, "Failed to initialize connector with drm\n");
> +		return ret;
>  	}
>  
> +	drm_connector_attach_encoder(connector, encoder);
> +
>  	/*
>  	 * NOTE: the connector registration is implemented in analogix
>  	 * platform driver, that to say connector would be exist after
> @@ -1124,7 +1083,7 @@ struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
>  static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>  						 struct drm_atomic_state *old_state)
>  {
> -	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state;
>  
> @@ -1177,14 +1136,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 = bridge->driver_private;
> -	struct drm_display_info *display_info = &dp->connector.display_info;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	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);
> @@ -1255,7 +1221,7 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
>  static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  					     struct drm_atomic_state *old_state)
>  {
> -	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>  	int timeout_loop = 0;
> @@ -1268,7 +1234,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 */
> @@ -1297,7 +1263,7 @@ static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>  
>  static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>  {
> -	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  
>  	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>  		return;
> @@ -1320,7 +1286,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>  static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  					      struct drm_atomic_state *old_state)
>  {
> -	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	struct drm_crtc *old_crtc, *new_crtc;
>  	struct drm_crtc_state *old_crtc_state = NULL;
>  	struct drm_crtc_state *new_crtc_state = NULL;
> @@ -1358,7 +1324,7 @@ static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>  static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
>  						   struct drm_atomic_state *old_state)
>  {
> -	struct analogix_dp_device *dp = bridge->driver_private;
> +	struct analogix_dp_device *dp = to_dp(bridge);
>  	struct drm_crtc *crtc;
>  	struct drm_crtc_state *new_crtc_state;
>  	int ret;
> @@ -1384,24 +1350,27 @@ 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,
> +	.detect = analogix_dp_bridge_detect,
>  };
>  
>  static int analogix_dp_create_bridge(struct drm_device *drm_dev,
>  				     struct analogix_dp_device *dp)
>  {
> -	struct drm_bridge *bridge;
> -
> -	bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge), GFP_KERNEL);
> -	if (!bridge) {
> -		DRM_ERROR("failed to allocate for drm bridge\n");
> -		return -ENOMEM;
> -	}
> +	struct drm_bridge *bridge = &dp->bridge;
> +	int ret;
>  
> -	dp->bridge = bridge;
> +	bridge->ops = DRM_BRIDGE_OP_DETECT |
> +		      DRM_BRIDGE_OP_HPD |
> +		      DRM_BRIDGE_OP_MODES;
> +	bridge->of_node = dp->dev->of_node;
> +	bridge->type = DRM_MODE_CONNECTOR_eDP;
>  
> -	bridge->driver_private = dp;
> -	bridge->funcs = &analogix_dp_bridge_funcs;
> +	ret = devm_drm_bridge_add(dp->dev, &dp->bridge);
> +	if (ret)
> +		return ret;
>  
>  	return drm_bridge_attach(dp->encoder, bridge, NULL, 0);
>  }
> @@ -1493,9 +1462,10 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>  		return ERR_PTR(-EINVAL);
>  	}
>  
> -	dp = devm_kzalloc(dev, sizeof(struct analogix_dp_device), GFP_KERNEL);
> -	if (!dp)
> -		return ERR_PTR(-ENOMEM);
> +	dp = devm_drm_bridge_alloc(dev, struct analogix_dp_device, bridge,
> +				   &analogix_dp_bridge_funcs);
> +	if (IS_ERR(dp))
> +		return ERR_CAST(dp);
>  
>  	dp->dev = &pdev->dev;
>  	dp->dpms_mode = DRM_MODE_DPMS_OFF;
> @@ -1670,8 +1640,7 @@ 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);
> +	analogix_dp_bridge_disable(&dp->bridge);
>  
>  	drm_panel_unprepare(dp->plat_data->panel);
>  
> @@ -1681,7 +1650,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",
> @@ -1689,13 +1659,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;

This requires that Analogix is the first bridge in the chain. It might
be better to loop through all bridges, checking for one with matching
ops.

I'll check, maybe we have other bridges which can generate CRC data. If
so, we can add DRM_BRIDGE_OP_CRC interface and loop that through
drm_bridge_connector.

> +
> +	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 9f9e492da80f..22f28384b4ec 100644
> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
> @@ -10,6 +10,7 @@
>  #define _ANALOGIX_DP_CORE_H
>  
>  #include <drm/display/drm_dp_helper.h>
> +#include <drm/drm_bridge.h>
>  #include <drm/drm_crtc.h>
>  
>  #define DP_TIMEOUT_LOOP_COUNT 100
> @@ -153,8 +154,7 @@ 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_bridge	bridge;
>  	struct drm_dp_aux	aux;
>  	struct clk		*clock;
>  	unsigned int		irq;
> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
> index cf17646c1310..cb9663ff61fb 100644
> --- a/include/drm/bridge/analogix_dp.h
> +++ b/include/drm/bridge/analogix_dp.h
> @@ -29,7 +29,6 @@ struct analogix_dp_plat_data {
>  	struct drm_panel *panel;
>  	struct drm_encoder *encoder;
>  	struct drm_connector *connector;
> -	bool skip_connector;
>  
>  	int (*power_on)(struct analogix_dp_plat_data *);
>  	int (*power_off)(struct analogix_dp_plat_data *);
> -- 
> 2.34.1
> 

-- 
With best wishes
Dmitry

Re: [PATCH v1 3/3] drm/bridge: analogix_dp: Apply drm_bridge_connector helper

[Damon Ding]

Hi Dmitry,

On 2025/6/9 1:44, Dmitry Baryshkov wrote:
> On Mon, May 26, 2025 at 08:07:42PM +0800, Damon Ding wrote:
>> Apply drm_bridge_connector helper for Analogix DP driver.
>>
>> The following changes have been made:
>> - Remove &analogix_dp_device.connector and change
>>    &analogix_dp_device.bridge from a pointer to an instance.
>> - Apply drm_bridge_connector helper to get rid of &drm_connector_funcs
>>    and &drm_connector_helper_funcs.
>> - Remove &analogix_dp_plat_data.skip_connector.
> 
> You've missed the exynos_dp.c which uses it.
> 
> I think there is slightly more to be handled here. For example, I'd
> suggest moving panel / bridge parsing and attachment to the core driver
> too. Exynos handles several backwards-compatibility cases, e.g. using
> the "panel" property or just passing video timings in the DT node. You
> might need to implement instantiating a panel from videomode specially
> for exynos_dp.c
> 

For this patch series, I'd like to place the DRM bridge on the Analogix 
side and the DRM connector/encoder on the Rockchip side.

Following your recommendation, it would be better to handle the 
panel/bridge parsing on the Analogix side. I believe this approach makes 
sense because the panel/bridge should logically be positioned behind the 
Analogix bridge in the display pipeline.

I will implement you suggestion and update the exynos_dp.c driver 
synchronously.

> This would allow you to cleanup plat_data interface as a preparation for
> this patch.
> 
>>
>> Signed-off-by: Damon Ding <damon.ding@rock-chips.com>
>> ---
>>   .../drm/bridge/analogix/analogix_dp_core.c    | 157 ++++++++----------
>>   .../drm/bridge/analogix/analogix_dp_core.h    |   4 +-
>>   include/drm/bridge/analogix_dp.h              |   1 -
>>   3 files changed, 72 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 2c51d3193120..d67afd63d999 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.c
>> @@ -22,6 +22,7 @@
>>   #include <drm/drm_atomic.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_device.h>
>>   #include <drm/drm_edid.h>
>> @@ -946,9 +947,10 @@ 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);
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	const struct drm_edid *drm_edid;
>>   	int num_modes = 0;
>>   
>> @@ -957,10 +959,10 @@ static int analogix_dp_get_modes(struct drm_connector *connector)
>>   	} else {
>>   		drm_edid = drm_edid_read_ddc(connector, &dp->aux.ddc);
>>   
>> -		drm_edid_connector_update(&dp->connector, drm_edid);
>> +		drm_edid_connector_update(connector, drm_edid);
>>   
>>   		if (drm_edid) {
>> -			num_modes += drm_edid_connector_add_modes(&dp->connector);
>> +			num_modes += drm_edid_connector_add_modes(connector);
>>   			drm_edid_free(drm_edid);
>>   		}
>>   	}
> 
> This should be split into get_modes() and edid_read(). Use
> DRM_BRIDGE_OP_ flags to control which implementation is actually being
> used.
> 

Yeah, I will do it.

> 
>> @@ -971,51 +973,25 @@ 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 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);
>> -
>> -	return dp->encoder;
>> -}
>> -
>> -
>> -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;
>> -
>> -	conn_state = drm_atomic_get_new_connector_state(state, connector);
>> -	if (WARN_ON(!conn_state))
>> -		return -ENODEV;
>> +	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 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)
>> @@ -1027,20 +1003,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 = bridge->driver_private;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	struct drm_connector *connector = NULL;
>>   	int ret = 0;
>>   
>> @@ -1049,23 +1016,15 @@ static int analogix_dp_bridge_attach(struct drm_bridge *bridge,
>>   		return -EINVAL;
>>   	}
>>   
>> -	if (!dp->plat_data->skip_connector) {
>> -		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);
>> +	connector = drm_bridge_connector_init(dp->drm_dev, encoder);
>> +	if (IS_ERR(connector)) {
>> +		ret = PTR_ERR(connector);
>> +		dev_err(dp->dev, "Failed to initialize connector with drm\n");
>> +		return ret;
>>   	}
>>   
>> +	drm_connector_attach_encoder(connector, encoder);
>> +
>>   	/*
>>   	 * NOTE: the connector registration is implemented in analogix
>>   	 * platform driver, that to say connector would be exist after
>> @@ -1124,7 +1083,7 @@ struct drm_crtc *analogix_dp_get_new_crtc(struct analogix_dp_device *dp,
>>   static void analogix_dp_bridge_atomic_pre_enable(struct drm_bridge *bridge,
>>   						 struct drm_atomic_state *old_state)
>>   {
>> -	struct analogix_dp_device *dp = bridge->driver_private;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	struct drm_crtc *crtc;
>>   	struct drm_crtc_state *old_crtc_state;
>>   
>> @@ -1177,14 +1136,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 = bridge->driver_private;
>> -	struct drm_display_info *display_info = &dp->connector.display_info;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	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);
>> @@ -1255,7 +1221,7 @@ static void analogix_dp_bridge_mode_set(struct drm_bridge *bridge,
>>   static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>>   					     struct drm_atomic_state *old_state)
>>   {
>> -	struct analogix_dp_device *dp = bridge->driver_private;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	struct drm_crtc *crtc;
>>   	struct drm_crtc_state *old_crtc_state, *new_crtc_state;
>>   	int timeout_loop = 0;
>> @@ -1268,7 +1234,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 */
>> @@ -1297,7 +1263,7 @@ static void analogix_dp_bridge_atomic_enable(struct drm_bridge *bridge,
>>   
>>   static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>   {
>> -	struct analogix_dp_device *dp = bridge->driver_private;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   
>>   	if (dp->dpms_mode != DRM_MODE_DPMS_ON)
>>   		return;
>> @@ -1320,7 +1286,7 @@ static void analogix_dp_bridge_disable(struct drm_bridge *bridge)
>>   static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>>   					      struct drm_atomic_state *old_state)
>>   {
>> -	struct analogix_dp_device *dp = bridge->driver_private;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	struct drm_crtc *old_crtc, *new_crtc;
>>   	struct drm_crtc_state *old_crtc_state = NULL;
>>   	struct drm_crtc_state *new_crtc_state = NULL;
>> @@ -1358,7 +1324,7 @@ static void analogix_dp_bridge_atomic_disable(struct drm_bridge *bridge,
>>   static void analogix_dp_bridge_atomic_post_disable(struct drm_bridge *bridge,
>>   						   struct drm_atomic_state *old_state)
>>   {
>> -	struct analogix_dp_device *dp = bridge->driver_private;
>> +	struct analogix_dp_device *dp = to_dp(bridge);
>>   	struct drm_crtc *crtc;
>>   	struct drm_crtc_state *new_crtc_state;
>>   	int ret;
>> @@ -1384,24 +1350,27 @@ 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,
>> +	.detect = analogix_dp_bridge_detect,
>>   };
>>   
>>   static int analogix_dp_create_bridge(struct drm_device *drm_dev,
>>   				     struct analogix_dp_device *dp)
>>   {
>> -	struct drm_bridge *bridge;
>> -
>> -	bridge = devm_kzalloc(drm_dev->dev, sizeof(*bridge), GFP_KERNEL);
>> -	if (!bridge) {
>> -		DRM_ERROR("failed to allocate for drm bridge\n");
>> -		return -ENOMEM;
>> -	}
>> +	struct drm_bridge *bridge = &dp->bridge;
>> +	int ret;
>>   
>> -	dp->bridge = bridge;
>> +	bridge->ops = DRM_BRIDGE_OP_DETECT |
>> +		      DRM_BRIDGE_OP_HPD |
>> +		      DRM_BRIDGE_OP_MODES;
>> +	bridge->of_node = dp->dev->of_node;
>> +	bridge->type = DRM_MODE_CONNECTOR_eDP;
>>   
>> -	bridge->driver_private = dp;
>> -	bridge->funcs = &analogix_dp_bridge_funcs;
>> +	ret = devm_drm_bridge_add(dp->dev, &dp->bridge);
>> +	if (ret)
>> +		return ret;
>>   
>>   	return drm_bridge_attach(dp->encoder, bridge, NULL, 0);
>>   }
>> @@ -1493,9 +1462,10 @@ analogix_dp_probe(struct device *dev, struct analogix_dp_plat_data *plat_data)
>>   		return ERR_PTR(-EINVAL);
>>   	}
>>   
>> -	dp = devm_kzalloc(dev, sizeof(struct analogix_dp_device), GFP_KERNEL);
>> -	if (!dp)
>> -		return ERR_PTR(-ENOMEM);
>> +	dp = devm_drm_bridge_alloc(dev, struct analogix_dp_device, bridge,
>> +				   &analogix_dp_bridge_funcs);
>> +	if (IS_ERR(dp))
>> +		return ERR_CAST(dp);
>>   
>>   	dp->dev = &pdev->dev;
>>   	dp->dpms_mode = DRM_MODE_DPMS_OFF;
>> @@ -1670,8 +1640,7 @@ 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);
>> +	analogix_dp_bridge_disable(&dp->bridge);
>>   
>>   	drm_panel_unprepare(dp->plat_data->panel);
>>   
>> @@ -1681,7 +1650,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",
>> @@ -1689,13 +1659,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;
> 
> This requires that Analogix is the first bridge in the chain. It might
> be better to loop through all bridges, checking for one with matching
> ops.
>
> I'll check, maybe we have other bridges which can generate CRC data. If
> so, we can add DRM_BRIDGE_OP_CRC interface and loop that through
> drm_bridge_connector.
> 

So far, the function is only for the use of rockchip_drm_vop.c driver, 
and it is called in &drm_crtc_funcs.set_crc_source().

It may be nice to make CRC calculation being a part of struct 
drm_bridge_funcs. I will also try to check whether any serdes bridge 
supports the CRC function.

>> +
>> +	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 9f9e492da80f..22f28384b4ec 100644
>> --- a/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> +++ b/drivers/gpu/drm/bridge/analogix/analogix_dp_core.h
>> @@ -10,6 +10,7 @@
>>   #define _ANALOGIX_DP_CORE_H
>>   
>>   #include <drm/display/drm_dp_helper.h>
>> +#include <drm/drm_bridge.h>
>>   #include <drm/drm_crtc.h>
>>   
>>   #define DP_TIMEOUT_LOOP_COUNT 100
>> @@ -153,8 +154,7 @@ 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_bridge	bridge;
>>   	struct drm_dp_aux	aux;
>>   	struct clk		*clock;
>>   	unsigned int		irq;
>> diff --git a/include/drm/bridge/analogix_dp.h b/include/drm/bridge/analogix_dp.h
>> index cf17646c1310..cb9663ff61fb 100644
>> --- a/include/drm/bridge/analogix_dp.h
>> +++ b/include/drm/bridge/analogix_dp.h
>> @@ -29,7 +29,6 @@ struct analogix_dp_plat_data {
>>   	struct drm_panel *panel;
>>   	struct drm_encoder *encoder;
>>   	struct drm_connector *connector;
>> -	bool skip_connector;
>>   
>>   	int (*power_on)(struct analogix_dp_plat_data *);
>>   	int (*power_off)(struct analogix_dp_plat_data *);
>> -- 
>> 2.34.1
>>
> 

Best regards,
Damon