[PATCH 0/8] drm/ast: Untangle connector helpers

[PATCH 0/8] drm/ast: Untangle connector helpers

[Thomas Zimmermann]

The ast driver supports different transmitter chips to support DVI
and HDMI connectors. It's al been packed into the same helpers
functons and exported as VGA connector.

Introduce a separate set of connector helpers for each transmitter
chip, and thus connector type. Also provide the correct encoder for
each connector.

This change mostly affects the connector's get_modes helper, where
VGA-, DVI- and HDMI-related code was lumped into the same function.
It's now all separate. While at it, also rework and cleanup the
initialization of the related data structures.

Tested on AST 2100 and AST 2300 hardware. I don't have hardware with
special transmitter chips (SIL164, DP501), so I could only test the VGA
code.

Thomas Zimmermann (8):
  drm/ast: Fail if connector initialization fails
  drm/ast: Move connector mode_valid function to CRTC
  drm/ast: Remove AST_TX_ITE66121 constant
  drm/ast: Remove unused value dp501_maxclk
  drm/ast: Rename struct ast_connector to struct ast_vga_connector
  drm/ast: Initialize encoder and connector for VGA in helper function
  drm/ast: Move DP501-based connector code into separate helpers
  drm/ast: Move SIL164-based connector code into separate helpers

 drivers/gpu/drm/ast/ast_dp501.c |  58 -----
 drivers/gpu/drm/ast/ast_drv.h   |  37 ++-
 drivers/gpu/drm/ast/ast_mode.c  | 413 +++++++++++++++++++++++---------
 3 files changed, 331 insertions(+), 177 deletions(-)


base-commit: fbce7b8d8df5af8d404b6aeaf63779f91bdbeb5d
prerequisite-patch-id: c2b2f08f0eccc9f5df0c0da49fa1d36267deb11d
prerequisite-patch-id: c67e5d886a47b7d0266d81100837557fda34cb24
-- 
2.34.1

[PATCH 1/8] drm/ast: Fail if connector initialization fails

[Thomas Zimmermann]

Update the connector code to fail if the connector could not be
initialized. The current code just ignored the error and failed
later when the connector was supposed to be used.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 956c8982192b..20626c78a693 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1319,18 +1319,21 @@ static int ast_connector_init(struct drm_device *dev)
 	struct ast_connector *ast_connector = &ast->connector;
 	struct drm_connector *connector = &ast_connector->base;
 	struct drm_encoder *encoder = &ast->encoder;
+	int ret;
 
 	ast_connector->i2c = ast_i2c_create(dev);
 	if (!ast_connector->i2c)
 		drm_err(dev, "failed to add ddc bus for connector\n");
 
 	if (ast_connector->i2c)
-		drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs,
-					    DRM_MODE_CONNECTOR_VGA,
-					    &ast_connector->i2c->adapter);
+		ret = drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs,
+						  DRM_MODE_CONNECTOR_VGA,
+						  &ast_connector->i2c->adapter);
 	else
-		drm_connector_init(dev, connector, &ast_connector_funcs,
-				   DRM_MODE_CONNECTOR_VGA);
+		ret = drm_connector_init(dev, connector, &ast_connector_funcs,
+					 DRM_MODE_CONNECTOR_VGA);
+	if (ret)
+		return ret;
 
 	drm_connector_helper_add(connector, &ast_connector_helper_funcs);
 
-- 
2.34.1

[PATCH 2/8] drm/ast: Move connector mode_valid function to CRTC

[Thomas Zimmermann]

The tests in ast_mode_valid() verify the correct resolution for the
supplied mode. This is a limitation of the CRTC, so move the function
to the CRTC helpers. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_mode.c | 129 +++++++++++++++++----------------
 1 file changed, 66 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 20626c78a693..c555960a488a 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1002,6 +1002,71 @@ static void ast_crtc_dpms(struct drm_crtc *crtc, int mode)
 	}
 }
 
+static enum drm_mode_status
+ast_crtc_helper_mode_valid(struct drm_crtc *crtc, const struct drm_display_mode *mode)
+{
+	struct ast_private *ast = to_ast_private(crtc->dev);
+	enum drm_mode_status status;
+	uint32_t jtemp;
+
+	if (ast->support_wide_screen) {
+		if ((mode->hdisplay == 1680) && (mode->vdisplay == 1050))
+			return MODE_OK;
+		if ((mode->hdisplay == 1280) && (mode->vdisplay == 800))
+			return MODE_OK;
+		if ((mode->hdisplay == 1440) && (mode->vdisplay == 900))
+			return MODE_OK;
+		if ((mode->hdisplay == 1360) && (mode->vdisplay == 768))
+			return MODE_OK;
+		if ((mode->hdisplay == 1600) && (mode->vdisplay == 900))
+			return MODE_OK;
+
+		if ((ast->chip == AST2100) || (ast->chip == AST2200) ||
+		    (ast->chip == AST2300) || (ast->chip == AST2400) ||
+		    (ast->chip == AST2500)) {
+			if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080))
+				return MODE_OK;
+
+			if ((mode->hdisplay == 1920) && (mode->vdisplay == 1200)) {
+				jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
+				if (jtemp & 0x01)
+					return MODE_NOMODE;
+				else
+					return MODE_OK;
+			}
+		}
+	}
+
+	status = MODE_NOMODE;
+
+	switch (mode->hdisplay) {
+	case 640:
+		if (mode->vdisplay == 480)
+			status = MODE_OK;
+		break;
+	case 800:
+		if (mode->vdisplay == 600)
+			status = MODE_OK;
+		break;
+	case 1024:
+		if (mode->vdisplay == 768)
+			status = MODE_OK;
+		break;
+	case 1280:
+		if (mode->vdisplay == 1024)
+			status = MODE_OK;
+		break;
+	case 1600:
+		if (mode->vdisplay == 1200)
+			status = MODE_OK;
+		break;
+	default:
+		break;
+	}
+
+	return status;
+}
+
 static int ast_crtc_helper_atomic_check(struct drm_crtc *crtc,
 					struct drm_atomic_state *state)
 {
@@ -1104,6 +1169,7 @@ ast_crtc_helper_atomic_disable(struct drm_crtc *crtc,
 }
 
 static const struct drm_crtc_helper_funcs ast_crtc_helper_funcs = {
+	.mode_valid = ast_crtc_helper_mode_valid,
 	.atomic_check = ast_crtc_helper_atomic_check,
 	.atomic_flush = ast_crtc_helper_atomic_flush,
 	.atomic_enable = ast_crtc_helper_atomic_enable,
@@ -1238,71 +1304,8 @@ static int ast_get_modes(struct drm_connector *connector)
 	return 0;
 }
 
-static enum drm_mode_status ast_mode_valid(struct drm_connector *connector,
-			  struct drm_display_mode *mode)
-{
-	struct ast_private *ast = to_ast_private(connector->dev);
-	int flags = MODE_NOMODE;
-	uint32_t jtemp;
-
-	if (ast->support_wide_screen) {
-		if ((mode->hdisplay == 1680) && (mode->vdisplay == 1050))
-			return MODE_OK;
-		if ((mode->hdisplay == 1280) && (mode->vdisplay == 800))
-			return MODE_OK;
-		if ((mode->hdisplay == 1440) && (mode->vdisplay == 900))
-			return MODE_OK;
-		if ((mode->hdisplay == 1360) && (mode->vdisplay == 768))
-			return MODE_OK;
-		if ((mode->hdisplay == 1600) && (mode->vdisplay == 900))
-			return MODE_OK;
-
-		if ((ast->chip == AST2100) || (ast->chip == AST2200) ||
-		    (ast->chip == AST2300) || (ast->chip == AST2400) ||
-		    (ast->chip == AST2500)) {
-			if ((mode->hdisplay == 1920) && (mode->vdisplay == 1080))
-				return MODE_OK;
-
-			if ((mode->hdisplay == 1920) && (mode->vdisplay == 1200)) {
-				jtemp = ast_get_index_reg_mask(ast, AST_IO_CRTC_PORT, 0xd1, 0xff);
-				if (jtemp & 0x01)
-					return MODE_NOMODE;
-				else
-					return MODE_OK;
-			}
-		}
-	}
-	switch (mode->hdisplay) {
-	case 640:
-		if (mode->vdisplay == 480)
-			flags = MODE_OK;
-		break;
-	case 800:
-		if (mode->vdisplay == 600)
-			flags = MODE_OK;
-		break;
-	case 1024:
-		if (mode->vdisplay == 768)
-			flags = MODE_OK;
-		break;
-	case 1280:
-		if (mode->vdisplay == 1024)
-			flags = MODE_OK;
-		break;
-	case 1600:
-		if (mode->vdisplay == 1200)
-			flags = MODE_OK;
-		break;
-	default:
-		return flags;
-	}
-
-	return flags;
-}
-
 static const struct drm_connector_helper_funcs ast_connector_helper_funcs = {
 	.get_modes = ast_get_modes,
-	.mode_valid = ast_mode_valid,
 };
 
 static const struct drm_connector_funcs ast_connector_funcs = {
-- 
2.34.1

[PATCH 3/8] drm/ast: Remove AST_TX_ITE66121 constant

[Thomas Zimmermann]

The ITE66121 is an HDMI transmitter chip. There's no code for
detecting or programming the chip within ast. Remove the enum
constant.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 00bfa41ff7cb..6e77be1d06d3 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -69,7 +69,6 @@ enum ast_chip {
 enum ast_tx_chip {
 	AST_TX_NONE,
 	AST_TX_SIL164,
-	AST_TX_ITE66121,
 	AST_TX_DP501,
 };
 
-- 
2.34.1

[PATCH 4/8] drm/ast: Remove unused value dp501_maxclk

[Thomas Zimmermann]

Remove reading the link-rate. The value is maintained by the connector
code but never used.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_dp501.c | 58 ---------------------------------
 drivers/gpu/drm/ast/ast_drv.h   |  1 -
 drivers/gpu/drm/ast/ast_mode.c  |  7 ++--
 3 files changed, 3 insertions(+), 63 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_dp501.c b/drivers/gpu/drm/ast/ast_dp501.c
index cd93c44f2662..204c926a18ea 100644
--- a/drivers/gpu/drm/ast/ast_dp501.c
+++ b/drivers/gpu/drm/ast/ast_dp501.c
@@ -272,64 +272,6 @@ static bool ast_launch_m68k(struct drm_device *dev)
 	return true;
 }
 
-u8 ast_get_dp501_max_clk(struct drm_device *dev)
-{
-	struct ast_private *ast = to_ast_private(dev);
-	u32 boot_address, offset, data;
-	u8 linkcap[4], linkrate, linklanes, maxclk = 0xff;
-	u32 *plinkcap;
-
-	if (ast->config_mode == ast_use_p2a) {
-		boot_address = get_fw_base(ast);
-
-		/* validate FW version */
-		offset = AST_DP501_GBL_VERSION;
-		data = ast_mindwm(ast, boot_address + offset);
-		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
-			return maxclk;
-
-		/* Read Link Capability */
-		offset  = AST_DP501_LINKRATE;
-		plinkcap = (u32 *)linkcap;
-		*plinkcap  = ast_mindwm(ast, boot_address + offset);
-		if (linkcap[2] == 0) {
-			linkrate = linkcap[0];
-			linklanes = linkcap[1];
-			data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes);
-			if (data > 0xff)
-				data = 0xff;
-			maxclk = (u8)data;
-		}
-	} else {
-		if (!ast->dp501_fw_buf)
-			return AST_DP501_DEFAULT_DCLK;	/* 1024x768 as default */
-
-		/* dummy read */
-		offset = 0x0000;
-		data = readl(ast->dp501_fw_buf + offset);
-
-		/* validate FW version */
-		offset = AST_DP501_GBL_VERSION;
-		data = readl(ast->dp501_fw_buf + offset);
-		if ((data & AST_DP501_FW_VERSION_MASK) != AST_DP501_FW_VERSION_1) /* version: 1x */
-			return maxclk;
-
-		/* Read Link Capability */
-		offset = AST_DP501_LINKRATE;
-		plinkcap = (u32 *)linkcap;
-		*plinkcap = readl(ast->dp501_fw_buf + offset);
-		if (linkcap[2] == 0) {
-			linkrate = linkcap[0];
-			linklanes = linkcap[1];
-			data = (linkrate == 0x0a) ? (90 * linklanes) : (54 * linklanes);
-			if (data > 0xff)
-				data = 0xff;
-			maxclk = (u8)data;
-		}
-	}
-	return maxclk;
-}
-
 bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata)
 {
 	struct ast_private *ast = to_ast_private(dev);
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 6e77be1d06d3..479bb120dd05 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -171,7 +171,6 @@ struct ast_private {
 	} config_mode;
 
 	enum ast_tx_chip tx_chip_type;
-	u8 dp501_maxclk;
 	u8 *dp501_fw_addr;
 	const struct firmware *dp501_fw;	/* dp501 fw */
 };
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index c555960a488a..0a8aa6e3aa38 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1281,16 +1281,15 @@ static int ast_get_modes(struct drm_connector *connector)
 	int ret;
 
 	if (ast->tx_chip_type == AST_TX_DP501) {
-		ast->dp501_maxclk = 0xff;
 		edid = kmalloc(128, GFP_KERNEL);
 		if (!edid)
 			return -ENOMEM;
 
 		flags = ast_dp501_read_edid(connector->dev, (u8 *)edid);
-		if (flags)
-			ast->dp501_maxclk = ast_get_dp501_max_clk(connector->dev);
-		else
+		if (!flags) {
 			kfree(edid);
+			edid = NULL;
+		}
 	}
 	if (!flags && ast_connector->i2c)
 		edid = drm_get_edid(connector, &ast_connector->i2c->adapter);
-- 
2.34.1

[PATCH 5/8] drm/ast: Rename struct ast_connector to struct ast_vga_connector

[Thomas Zimmermann]

Prepare for introducing other connectors besides VGA. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 10 ++++----
 drivers/gpu/drm/ast/ast_mode.c | 45 +++++++++++++++++-----------------
 2 files changed, 27 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 479bb120dd05..e1cb31acdaac 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -129,15 +129,15 @@ struct ast_i2c_chan {
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_connector {
+struct ast_vga_connector {
 	struct drm_connector base;
 	struct ast_i2c_chan *i2c;
 };
 
-static inline struct ast_connector *
-to_ast_connector(struct drm_connector *connector)
+static inline struct ast_vga_connector *
+to_ast_vga_connector(struct drm_connector *connector)
 {
-	return container_of(connector, struct ast_connector, base);
+	return container_of(connector, struct ast_vga_connector, base);
 }
 
 /*
@@ -161,7 +161,7 @@ struct ast_private {
 	struct ast_cursor_plane cursor_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
-	struct ast_connector connector;
+	struct ast_vga_connector connector;
 
 	bool support_wide_screen;
 	enum {
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 0a8aa6e3aa38..f7f4034cc91e 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1269,12 +1269,12 @@ static int ast_encoder_init(struct drm_device *dev)
 }
 
 /*
- * Connector
+ * VGA Connector
  */
 
-static int ast_get_modes(struct drm_connector *connector)
+static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct ast_connector *ast_connector = to_ast_connector(connector);
+	struct ast_vga_connector *ast_vga_connector = to_ast_vga_connector(connector);
 	struct ast_private *ast = to_ast_private(connector->dev);
 	struct edid *edid = NULL;
 	bool flags = false;
@@ -1291,23 +1291,23 @@ static int ast_get_modes(struct drm_connector *connector)
 			edid = NULL;
 		}
 	}
-	if (!flags && ast_connector->i2c)
-		edid = drm_get_edid(connector, &ast_connector->i2c->adapter);
+	if (!flags && ast_vga_connector->i2c)
+		edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
 	if (edid) {
-		drm_connector_update_edid_property(&ast_connector->base, edid);
+		drm_connector_update_edid_property(connector, edid);
 		ret = drm_add_edid_modes(connector, edid);
 		kfree(edid);
 		return ret;
 	}
-	drm_connector_update_edid_property(&ast_connector->base, NULL);
+	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
 
-static const struct drm_connector_helper_funcs ast_connector_helper_funcs = {
-	.get_modes = ast_get_modes,
+static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
+	.get_modes = ast_vga_connector_helper_get_modes,
 };
 
-static const struct drm_connector_funcs ast_connector_funcs = {
+static const struct drm_connector_funcs ast_vga_connector_funcs = {
 	.reset = drm_atomic_helper_connector_reset,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.destroy = drm_connector_cleanup,
@@ -1315,29 +1315,29 @@ static const struct drm_connector_funcs ast_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int ast_connector_init(struct drm_device *dev)
+static int ast_vga_connector_init(struct drm_device *dev)
 {
 	struct ast_private *ast = to_ast_private(dev);
-	struct ast_connector *ast_connector = &ast->connector;
-	struct drm_connector *connector = &ast_connector->base;
+	struct ast_vga_connector *ast_vga_connector = &ast->connector;
+	struct drm_connector *connector = &ast_vga_connector->base;
 	struct drm_encoder *encoder = &ast->encoder;
 	int ret;
 
-	ast_connector->i2c = ast_i2c_create(dev);
-	if (!ast_connector->i2c)
+	ast_vga_connector->i2c = ast_i2c_create(dev);
+	if (!ast_vga_connector->i2c)
 		drm_err(dev, "failed to add ddc bus for connector\n");
 
-	if (ast_connector->i2c)
-		ret = drm_connector_init_with_ddc(dev, connector, &ast_connector_funcs,
+	if (ast_vga_connector->i2c)
+		ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
 						  DRM_MODE_CONNECTOR_VGA,
-						  &ast_connector->i2c->adapter);
+						  &ast_vga_connector->i2c->adapter);
 	else
-		ret = drm_connector_init(dev, connector, &ast_connector_funcs,
+		ret = drm_connector_init(dev, connector, &ast_vga_connector_funcs,
 					 DRM_MODE_CONNECTOR_VGA);
 	if (ret)
 		return ret;
 
-	drm_connector_helper_add(connector, &ast_connector_helper_funcs);
+	drm_connector_helper_add(connector, &ast_vga_connector_helper_funcs);
 
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
@@ -1353,8 +1353,7 @@ static int ast_connector_init(struct drm_device *dev)
  * Mode config
  */
 
-static const struct drm_mode_config_helper_funcs
-ast_mode_config_helper_funcs = {
+static const struct drm_mode_config_helper_funcs ast_mode_config_helper_funcs = {
 	.atomic_commit_tail = drm_atomic_helper_commit_tail_rpm,
 };
 
@@ -1407,7 +1406,7 @@ int ast_mode_config_init(struct ast_private *ast)
 
 	ast_crtc_init(dev);
 	ast_encoder_init(dev);
-	ast_connector_init(dev);
+	ast_vga_connector_init(dev);
 
 	drm_mode_config_reset(dev);
 
-- 
2.34.1

[PATCH 6/8] drm/ast: Initialize encoder and connector for VGA in helper function

[Thomas Zimmermann]

Move encoder and connector initialization into a single helper and
put all related mode-setting structures into a single place. Done in
preparation of moving transmitter code into separate helpers. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |  8 +++--
 drivers/gpu/drm/ast/ast_mode.c | 62 ++++++++++++++++++++--------------
 2 files changed, 42 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index e1cb31acdaac..cda50fb887ed 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -160,8 +160,12 @@ struct ast_private {
 	struct drm_plane primary_plane;
 	struct ast_cursor_plane cursor_plane;
 	struct drm_crtc crtc;
-	struct drm_encoder encoder;
-	struct ast_vga_connector connector;
+	union {
+		struct {
+			struct drm_encoder encoder;
+			struct ast_vga_connector vga_connector;
+		} vga;
+	} output;
 
 	bool support_wide_screen;
 	enum {
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index f7f4034cc91e..17e4e038a3ed 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1249,25 +1249,6 @@ static int ast_crtc_init(struct drm_device *dev)
 	return 0;
 }
 
-/*
- * Encoder
- */
-
-static int ast_encoder_init(struct drm_device *dev)
-{
-	struct ast_private *ast = to_ast_private(dev);
-	struct drm_encoder *encoder = &ast->encoder;
-	int ret;
-
-	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
-	if (ret)
-		return ret;
-
-	encoder->possible_crtcs = 1;
-
-	return 0;
-}
-
 /*
  * VGA Connector
  */
@@ -1315,12 +1296,10 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = {
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
-static int ast_vga_connector_init(struct drm_device *dev)
+static int ast_vga_connector_init(struct drm_device *dev,
+				  struct ast_vga_connector *ast_vga_connector)
 {
-	struct ast_private *ast = to_ast_private(dev);
-	struct ast_vga_connector *ast_vga_connector = &ast->connector;
 	struct drm_connector *connector = &ast_vga_connector->base;
-	struct drm_encoder *encoder = &ast->encoder;
 	int ret;
 
 	ast_vga_connector->i2c = ast_i2c_create(dev);
@@ -1344,7 +1323,30 @@ static int ast_vga_connector_init(struct drm_device *dev)
 
 	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
 
-	drm_connector_attach_encoder(connector, encoder);
+	return 0;
+}
+
+static int ast_vga_output_init(struct ast_private *ast)
+{
+	struct drm_device *dev = &ast->base;
+	struct drm_crtc *crtc = &ast->crtc;
+	struct drm_encoder *encoder = &ast->output.vga.encoder;
+	struct ast_vga_connector *ast_vga_connector = &ast->output.vga.vga_connector;
+	struct drm_connector *connector = &ast_vga_connector->base;
+	int ret;
+
+	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	ret = ast_vga_connector_init(dev, ast_vga_connector);
+	if (ret)
+		return ret;
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
 
 	return 0;
 }
@@ -1405,8 +1407,16 @@ int ast_mode_config_init(struct ast_private *ast)
 		return ret;
 
 	ast_crtc_init(dev);
-	ast_encoder_init(dev);
-	ast_vga_connector_init(dev);
+
+	switch (ast->tx_chip_type) {
+	case AST_TX_NONE:
+	case AST_TX_SIL164:
+	case AST_TX_DP501:
+		ret = ast_vga_output_init(ast);
+		break;
+	}
+	if (ret)
+		return ret;
 
 	drm_mode_config_reset(dev);
 
-- 
2.34.1

[PATCH 7/8] drm/ast: Move DP501-based connector code into separate helpers

[Thomas Zimmermann]

Add helpers for DP501-based connectors. DP501 provides output via
DisplayPort. This used to be handled by the VGA connector code.

If a DP501 chip has been detected, ast will now create a DisplayPort
connector instead of a VGA connector.

Remove the DP501 code from ast_vga_connector_helper_get_modes(). Also
remove the call to drm_connector_update_edid_property(), which is
performed by drm_get_edid().

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  |   4 ++
 drivers/gpu/drm/ast/ast_mode.c | 128 +++++++++++++++++++++++++++------
 2 files changed, 109 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index cda50fb887ed..420d19d8459e 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -165,6 +165,10 @@ struct ast_private {
 			struct drm_encoder encoder;
 			struct ast_vga_connector vga_connector;
 		} vga;
+		struct {
+			struct drm_encoder encoder;
+			struct drm_connector connector;
+		} dp501;
 	} output;
 
 	bool support_wide_screen;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 17e4e038a3ed..a0f4f042141e 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -40,6 +40,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_gem_framebuffer_helper.h>
 #include <drm/drm_gem_vram_helper.h>
+#include <drm/drm_managed.h>
 #include <drm/drm_plane_helper.h>
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
@@ -1256,30 +1257,22 @@ static int ast_crtc_init(struct drm_device *dev)
 static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
 	struct ast_vga_connector *ast_vga_connector = to_ast_vga_connector(connector);
-	struct ast_private *ast = to_ast_private(connector->dev);
-	struct edid *edid = NULL;
-	bool flags = false;
-	int ret;
+	struct edid *edid;
+	int count;
 
-	if (ast->tx_chip_type == AST_TX_DP501) {
-		edid = kmalloc(128, GFP_KERNEL);
-		if (!edid)
-			return -ENOMEM;
+	if (!ast_vga_connector->i2c)
+		goto err_drm_connector_update_edid_property;
 
-		flags = ast_dp501_read_edid(connector->dev, (u8 *)edid);
-		if (!flags) {
-			kfree(edid);
-			edid = NULL;
-		}
-	}
-	if (!flags && ast_vga_connector->i2c)
-		edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
-	if (edid) {
-		drm_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		kfree(edid);
-		return ret;
-	}
+	edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
+	if (!edid)
+		goto err_drm_connector_update_edid_property;
+
+	count = drm_add_edid_modes(connector, edid);
+	kfree(edid);
+
+	return count;
+
+err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1351,6 +1344,92 @@ static int ast_vga_output_init(struct ast_private *ast)
 	return 0;
 }
 
+/*
+ * DP501 Connector
+ */
+
+static int ast_dp501_connector_helper_get_modes(struct drm_connector *connector)
+{
+	void *edid;
+	bool succ;
+	int count;
+
+	edid = kmalloc(EDID_LENGTH, GFP_KERNEL);
+	if (!edid)
+		goto err_drm_connector_update_edid_property;
+
+	succ = ast_dp501_read_edid(connector->dev, edid);
+	if (!succ)
+		goto err_kfree;
+
+	drm_connector_update_edid_property(connector, edid);
+	count = drm_add_edid_modes(connector, edid);
+	kfree(edid);
+
+	return count;
+
+err_kfree:
+	kfree(edid);
+err_drm_connector_update_edid_property:
+	drm_connector_update_edid_property(connector, NULL);
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs ast_dp501_connector_helper_funcs = {
+	.get_modes = ast_dp501_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs ast_dp501_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int ast_dp501_connector_init(struct drm_device *dev, struct drm_connector *connector)
+{
+	int ret;
+
+	ret = drm_connector_init(dev, connector, &ast_dp501_connector_funcs,
+				 DRM_MODE_CONNECTOR_DisplayPort);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(connector, &ast_dp501_connector_helper_funcs);
+
+	connector->interlace_allowed = 0;
+	connector->doublescan_allowed = 0;
+
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+
+	return 0;
+}
+
+static int ast_dp501_output_init(struct ast_private *ast)
+{
+	struct drm_device *dev = &ast->base;
+	struct drm_crtc *crtc = &ast->crtc;
+	struct drm_encoder *encoder = &ast->output.dp501.encoder;
+	struct drm_connector *connector = &ast->output.dp501.connector;
+	int ret;
+
+	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	ret = ast_dp501_connector_init(dev, connector);
+	if (ret)
+		return ret;
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /*
  * Mode config
  */
@@ -1411,10 +1490,13 @@ int ast_mode_config_init(struct ast_private *ast)
 	switch (ast->tx_chip_type) {
 	case AST_TX_NONE:
 	case AST_TX_SIL164:
-	case AST_TX_DP501:
 		ret = ast_vga_output_init(ast);
 		break;
+	case AST_TX_DP501:
+		ret = ast_dp501_output_init(ast);
+		break;
 	}
+
 	if (ret)
 		return ret;
 
-- 
2.34.1

[PATCH 8/8] drm/ast: Move SIL164-based connector code into separate helpers

[Thomas Zimmermann]

Add helpers for initializing SIL164-based connectors. These used to be
handled by the VGA connector code. But SIL164 provides output via DVI-I,
so set the encoder and connector types accordingly.

If a SIL164 chip has been detected, ast will now create a DVI-I
connector instead of a VGA connector.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.h  | 15 ++++++
 drivers/gpu/drm/ast/ast_mode.c | 99 +++++++++++++++++++++++++++++++++-
 2 files changed, 112 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 420d19d8459e..c3a582372649 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -140,6 +140,17 @@ to_ast_vga_connector(struct drm_connector *connector)
 	return container_of(connector, struct ast_vga_connector, base);
 }
 
+struct ast_sil164_connector {
+	struct drm_connector base;
+	struct ast_i2c_chan *i2c;
+};
+
+static inline struct ast_sil164_connector *
+to_ast_sil164_connector(struct drm_connector *connector)
+{
+	return container_of(connector, struct ast_sil164_connector, base);
+}
+
 /*
  * Device
  */
@@ -165,6 +176,10 @@ struct ast_private {
 			struct drm_encoder encoder;
 			struct ast_vga_connector vga_connector;
 		} vga;
+		struct {
+			struct drm_encoder encoder;
+			struct ast_sil164_connector sil164_connector;
+		} sil164;
 		struct {
 			struct drm_encoder encoder;
 			struct drm_connector connector;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index a0f4f042141e..f9daeb8d801a 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1344,6 +1344,100 @@ static int ast_vga_output_init(struct ast_private *ast)
 	return 0;
 }
 
+/*
+ * SIL164 Connector
+ */
+
+static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
+{
+	struct ast_sil164_connector *ast_sil164_connector = to_ast_sil164_connector(connector);
+	struct edid *edid;
+	int count;
+
+	if (!ast_sil164_connector->i2c)
+		goto err_drm_connector_update_edid_property;
+
+	edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
+	if (!edid)
+		goto err_drm_connector_update_edid_property;
+
+	count = drm_add_edid_modes(connector, edid);
+	kfree(edid);
+
+	return count;
+
+err_drm_connector_update_edid_property:
+	drm_connector_update_edid_property(connector, NULL);
+	return 0;
+}
+
+static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
+	.get_modes = ast_sil164_connector_helper_get_modes,
+};
+
+static const struct drm_connector_funcs ast_sil164_connector_funcs = {
+	.reset = drm_atomic_helper_connector_reset,
+	.fill_modes = drm_helper_probe_single_connector_modes,
+	.destroy = drm_connector_cleanup,
+	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
+	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
+};
+
+static int ast_sil164_connector_init(struct drm_device *dev,
+				     struct ast_sil164_connector *ast_sil164_connector)
+{
+	struct drm_connector *connector = &ast_sil164_connector->base;
+	int ret;
+
+	ast_sil164_connector->i2c = ast_i2c_create(dev);
+	if (!ast_sil164_connector->i2c)
+		drm_err(dev, "failed to add ddc bus for connector\n");
+
+	if (ast_sil164_connector->i2c)
+		ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
+						  DRM_MODE_CONNECTOR_DVII,
+						  &ast_sil164_connector->i2c->adapter);
+	else
+		ret = drm_connector_init(dev, connector, &ast_sil164_connector_funcs,
+					 DRM_MODE_CONNECTOR_DVII);
+	if (ret)
+		return ret;
+
+	drm_connector_helper_add(connector, &ast_sil164_connector_helper_funcs);
+
+	connector->interlace_allowed = 0;
+	connector->doublescan_allowed = 0;
+
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+
+	return 0;
+}
+
+static int ast_sil164_output_init(struct ast_private *ast)
+{
+	struct drm_device *dev = &ast->base;
+	struct drm_crtc *crtc = &ast->crtc;
+	struct drm_encoder *encoder = &ast->output.sil164.encoder;
+	struct ast_sil164_connector *ast_sil164_connector = &ast->output.sil164.sil164_connector;
+	struct drm_connector *connector = &ast_sil164_connector->base;
+	int ret;
+
+	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
+	if (ret)
+		return ret;
+	encoder->possible_crtcs = drm_crtc_mask(crtc);
+
+	ret = ast_sil164_connector_init(dev, ast_sil164_connector);
+	if (ret)
+		return ret;
+
+	ret = drm_connector_attach_encoder(connector, encoder);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
 /*
  * DP501 Connector
  */
@@ -1489,14 +1583,15 @@ int ast_mode_config_init(struct ast_private *ast)
 
 	switch (ast->tx_chip_type) {
 	case AST_TX_NONE:
-	case AST_TX_SIL164:
 		ret = ast_vga_output_init(ast);
 		break;
+	case AST_TX_SIL164:
+		ret = ast_sil164_output_init(ast);
+		break;
 	case AST_TX_DP501:
 		ret = ast_dp501_output_init(ast);
 		break;
 	}
-
 	if (ret)
 		return ret;
 
-- 
2.34.1

Re: [PATCH 1/8] drm/ast: Fail if connector initialization fails

[Javier Martinez Canillas]

Hello Thomas,

On 1/11/22 13:00, Thomas Zimmermann wrote:
> Update the connector code to fail if the connector could not be
> initialized. The current code just ignored the error and failed
> later when the connector was supposed to be used.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 2/8] drm/ast: Move connector mode_valid function to CRTC

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> The tests in ast_mode_valid() verify the correct resolution for the
> supplied mode. This is a limitation of the CRTC, so move the function
> to the CRTC helpers. No functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 3/8] drm/ast: Remove AST_TX_ITE66121 constant

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> The ITE66121 is an HDMI transmitter chip. There's no code for
> detecting or programming the chip within ast. Remove the enum
> constant.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 4/8] drm/ast: Remove unused value dp501_maxclk

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> Remove reading the link-rate. The value is maintained by the connector
> code but never used.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 5/8] drm/ast: Rename struct ast_connector to struct ast_vga_connector

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> Prepare for introducing other connectors besides VGA. No functional
> changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 6/8] drm/ast: Initialize encoder and connector for VGA in helper function

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> Move encoder and connector initialization into a single helper and
> put all related mode-setting structures into a single place. Done in
> preparation of moving transmitter code into separate helpers. No
> functional changes.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

[snip]

> -	encoder->possible_crtcs = 1;

[snip]

> +	encoder->possible_crtcs = drm_crtc_mask(crtc);

This is a somewhat unrelated change. It's OK because is fairly simple
but I would probably still do the cleanups as separate patches.

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 7/8] drm/ast: Move DP501-based connector code into separate helpers

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> Add helpers for DP501-based connectors. DP501 provides output via
> DisplayPort. This used to be handled by the VGA connector code.
> 
> If a DP501 chip has been detected, ast will now create a DisplayPort
> connector instead of a VGA connector.
> 
> Remove the DP501 code from ast_vga_connector_helper_get_modes(). Also
> remove the call to drm_connector_update_edid_property(), which is
> performed by drm_get_edid().
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 8/8] drm/ast: Move SIL164-based connector code into separate helpers

[Javier Martinez Canillas]

On 1/11/22 13:00, Thomas Zimmermann wrote:
> Add helpers for initializing SIL164-based connectors. These used to be
> handled by the VGA connector code. But SIL164 provides output via DVI-I,
> so set the encoder and connector types accordingly.
> 
> If a SIL164 chip has been detected, ast will now create a DVI-I
> connector instead of a VGA connector.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_drv.h  | 15 ++++++
>  drivers/gpu/drm/ast/ast_mode.c | 99 +++++++++++++++++++++++++++++++++-
>  2 files changed, 112 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
> index 420d19d8459e..c3a582372649 100644
> --- a/drivers/gpu/drm/ast/ast_drv.h
> +++ b/drivers/gpu/drm/ast/ast_drv.h
> @@ -140,6 +140,17 @@ to_ast_vga_connector(struct drm_connector *connector)
>  	return container_of(connector, struct ast_vga_connector, base);
>  }
>  

[snip]

> +static int ast_sil164_connector_init(struct drm_device *dev,
> +				     struct ast_sil164_connector *ast_sil164_connector)
> +{
> +	struct drm_connector *connector = &ast_sil164_connector->base;
> +	int ret;
> +
> +	ast_sil164_connector->i2c = ast_i2c_create(dev);
> +	if (!ast_sil164_connector->i2c)
> +		drm_err(dev, "failed to add ddc bus for connector\n");
> +
> +	if (ast_sil164_connector->i2c)
> +		ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
> +						  DRM_MODE_CONNECTOR_DVII,
> +						  &ast_sil164_connector->i2c->adapter);
> +	else
> +		ret = drm_connector_init(dev, connector, &ast_sil164_connector_funcs,
> +					 DRM_MODE_CONNECTOR_DVII);
> +	if (ret)

I think you want a kfree(i2c) here before returning.

And where is the struct ast_i2c_chan freed if the function succeeds ?

With that,

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Best regards,
-- 
Javier Martinez Canillas
Linux Engineering
Red Hat

Re: [PATCH 6/8] drm/ast: Initialize encoder and connector for VGA in helper function

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1025 bytes --]

Hi

Am 03.02.22 um 18:43 schrieb Javier Martinez Canillas:
> On 1/11/22 13:00, Thomas Zimmermann wrote:
>> Move encoder and connector initialization into a single helper and
>> put all related mode-setting structures into a single place. Done in
>> preparation of moving transmitter code into separate helpers. No
>> functional changes.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> [snip]
> 
>> -	encoder->possible_crtcs = 1;
> 
> [snip]
> 
>> +	encoder->possible_crtcs = drm_crtc_mask(crtc);
> 
> This is a somewhat unrelated change. It's OK because is fairly simple
> but I would probably still do the cleanups as separate patches.

I'll put this change into a separate patch.

Best regards
Thomas

> 
> Best regards,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 8/8] drm/ast: Move SIL164-based connector code into separate helpers

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2472 bytes --]

Hi

Am 03.02.22 um 18:57 schrieb Javier Martinez Canillas:
> On 1/11/22 13:00, Thomas Zimmermann wrote:
>> Add helpers for initializing SIL164-based connectors. These used to be
>> handled by the VGA connector code. But SIL164 provides output via DVI-I,
>> so set the encoder and connector types accordingly.
>>
>> If a SIL164 chip has been detected, ast will now create a DVI-I
>> connector instead of a VGA connector.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_drv.h  | 15 ++++++
>>   drivers/gpu/drm/ast/ast_mode.c | 99 +++++++++++++++++++++++++++++++++-
>>   2 files changed, 112 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
>> index 420d19d8459e..c3a582372649 100644
>> --- a/drivers/gpu/drm/ast/ast_drv.h
>> +++ b/drivers/gpu/drm/ast/ast_drv.h
>> @@ -140,6 +140,17 @@ to_ast_vga_connector(struct drm_connector *connector)
>>   	return container_of(connector, struct ast_vga_connector, base);
>>   }
>>   
> 
> [snip]
> 
>> +static int ast_sil164_connector_init(struct drm_device *dev,
>> +				     struct ast_sil164_connector *ast_sil164_connector)
>> +{
>> +	struct drm_connector *connector = &ast_sil164_connector->base;
>> +	int ret;
>> +
>> +	ast_sil164_connector->i2c = ast_i2c_create(dev);
>> +	if (!ast_sil164_connector->i2c)
>> +		drm_err(dev, "failed to add ddc bus for connector\n");
>> +
>> +	if (ast_sil164_connector->i2c)
>> +		ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
>> +						  DRM_MODE_CONNECTOR_DVII,
>> +						  &ast_sil164_connector->i2c->adapter);
>> +	else
>> +		ret = drm_connector_init(dev, connector, &ast_sil164_connector_funcs,
>> +					 DRM_MODE_CONNECTOR_DVII);
>> +	if (ret)
> 
> I think you want a kfree(i2c) here before returning.
> 
> And where is the struct ast_i2c_chan freed if the function succeeds ?

The memory and data structure is managed with 
drmm_add_action_or_reset(). It will be released together with the DRM 
driver (either on success or failure). See ast_i2c_create() for the details.

Best regards
Thomas

> 
> With that,
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Best regards,

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]