[PATCH v4 00/12] drm/ast: Detect connector status for VGA and SIL164

[PATCH v4 00/12] drm/ast: Detect connector status for VGA and SIL164

[Thomas Zimmermann]

(was: drm/ast: Connector cleanups and polling)

Detect the status of the VGA or SIL164 conenctor by polling the DDC
channel. Update the status at runtime and flip the BMC status as well.

Patches 1 adds a missing include statement that will be required to
make later patches compile.

Patches 2 and 3 simplify the VGA and SIL164 connectors, such that no
additional data structure is required. These patches have been reviewed
before as part of the series at [1].

Patches 4 to 10 improve the I2C code that is used to retrieve the
monitor's EDID data. It's now fully managed, it acquires the necessary
lock automatically and it is called DDC, which better represents its
purpose than I2C.

Patches 11 and 12 finally implement polling. Patch 11 updates ast's
EDID code to be up-to-date. The helper drm_connector_get_modes() reads
the EDID via DDC and updates the property. No driver code is required.
Patch 12 uses a similar pattern to detect the presence of the monitor
and sets the connector status accordingly.

Tested on AST2500 hardware and BMC output. The BMC connector now also
flips its status correctly at runtime.

[1] https://patchwork.freedesktop.org/series/104547/

Thomas Zimmermann (12):
  drm/ast: Include <linux/of.h> where necessary
  drm/ast: Fail probing if DDC channel could not be initialized
  drm/ast: Remove struct ast_{vga,sil165}_connector
  drm/ast: Allocate instance of struct ast_i2c_chan with managed helpers
  drm/ast: Move DDC code to ast_ddc.{c,h}
  drm/ast: Rename struct ast_i2c_chan to struct ast_ddc
  drm/ast: Pass AST device to ast_ddc_create()
  drm/ast: Store AST device in struct ast_ddc
  drm/ast: Rename struct i2c_algo_bit_data callbacks and their
    parameters
  drm/ast: Acquire I/O-register lock in DDC code
  drm/ast: Use drm_connector_helper_get_modes()
  drm/ast: Implement polling for VGA and SIL164 connectors

 drivers/gpu/drm/ast/Makefile                 |  10 +-
 drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} | 120 +++++++++------
 drivers/gpu/drm/ast/ast_ddc.h                |  20 +++
 drivers/gpu/drm/ast/ast_drv.c                |   1 +
 drivers/gpu/drm/ast/ast_drv.h                |  39 +----
 drivers/gpu/drm/ast/ast_main.c               |   1 +
 drivers/gpu/drm/ast/ast_mode.c               | 153 +++++++------------
 7 files changed, 162 insertions(+), 182 deletions(-)
 rename drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} (54%)
 create mode 100644 drivers/gpu/drm/ast/ast_ddc.h

-- 
2.44.0

[PATCH v4 01/12] drm/ast: Include <linux/of.h> where necessary

[Thomas Zimmermann]

Include <linux/of.h> to get of_property_read_u32() in the source
files that need it. Avoids the proxy include via <linux/i2c.h>.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_drv.c  | 1 +
 drivers/gpu/drm/ast/ast_main.c | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/ast/ast_drv.c b/drivers/gpu/drm/ast/ast_drv.c
index 90bcb1eb9cd94..f8c49ba68e789 100644
--- a/drivers/gpu/drm/ast/ast_drv.c
+++ b/drivers/gpu/drm/ast/ast_drv.c
@@ -27,6 +27,7 @@
  */
 
 #include <linux/module.h>
+#include <linux/of.h>
 #include <linux/pci.h>
 
 #include <drm/drm_aperture.h>
diff --git a/drivers/gpu/drm/ast/ast_main.c b/drivers/gpu/drm/ast/ast_main.c
index 2f3ad5f949fcb..0637abb70361c 100644
--- a/drivers/gpu/drm/ast/ast_main.c
+++ b/drivers/gpu/drm/ast/ast_main.c
@@ -26,6 +26,7 @@
  * Authors: Dave Airlie <airlied@redhat.com>
  */
 
+#include <linux/of.h>
 #include <linux/pci.h>
 
 #include <drm/drm_atomic_helper.h>
-- 
2.44.0

[PATCH v4 02/12] drm/ast: Fail probing if DDC channel could not be initialized

[Thomas Zimmermann]

Expect the hardware to provide a DDC channel. Fail probing if its
initialization fails. Failing to initialize the DDC indicates a
larger problem, so there's no point in continuing.

v4:
	* give a rational in the commit message

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
---
 drivers/gpu/drm/ast/ast_drv.h  |  2 --
 drivers/gpu/drm/ast/ast_i2c.c  |  7 +++--
 drivers/gpu/drm/ast/ast_mode.c | 52 +++++++++++++---------------------
 3 files changed, 24 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 3be5ccf1f5f4d..cb0e4f332be80 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -160,7 +160,6 @@ struct ast_i2c_chan {
 
 struct ast_vga_connector {
 	struct drm_connector base;
-	struct ast_i2c_chan *i2c;
 };
 
 static inline struct ast_vga_connector *
@@ -171,7 +170,6 @@ to_ast_vga_connector(struct drm_connector *connector)
 
 struct ast_sil164_connector {
 	struct drm_connector base;
-	struct ast_i2c_chan *i2c;
 };
 
 static inline struct ast_sil164_connector *
diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_i2c.c
index e5d3f7121de42..c3046223a4919 100644
--- a/drivers/gpu/drm/ast/ast_i2c.c
+++ b/drivers/gpu/drm/ast/ast_i2c.c
@@ -117,7 +117,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 
 	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
 	if (!i2c)
-		return NULL;
+		return ERR_PTR(-ENOMEM);
 
 	i2c->adapter.owner = THIS_MODULE;
 	i2c->adapter.dev.parent = dev->dev;
@@ -142,10 +142,11 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 
 	ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
 	if (ret)
-		return NULL;
+		return ERR_PTR(ret);
+
 	return i2c;
 
 out_kfree:
 	kfree(i2c);
-	return NULL;
+	return ERR_PTR(ret);
 }
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index a718646a66b8f..38c31a7283a69 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1345,22 +1345,18 @@ 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 drm_device *dev = connector->dev;
 	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	if (!ast_vga_connector->i2c)
-		goto err_drm_connector_update_edid_property;
-
 	/*
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
 	mutex_lock(&ast->modeset_lock);
 
-	edid = drm_get_edid(connector, &ast_vga_connector->i2c->adapter);
+	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
 		goto err_mutex_unlock;
 
@@ -1373,7 +1369,6 @@ static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 
 err_mutex_unlock:
 	mutex_unlock(&ast->modeset_lock);
-err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1394,19 +1389,18 @@ static int ast_vga_connector_init(struct drm_device *dev,
 				  struct ast_vga_connector *ast_vga_connector)
 {
 	struct drm_connector *connector = &ast_vga_connector->base;
+	struct ast_i2c_chan *i2c;
 	int ret;
 
-	ast_vga_connector->i2c = ast_i2c_create(dev);
-	if (!ast_vga_connector->i2c)
-		drm_err(dev, "failed to add ddc bus for connector\n");
+	i2c = ast_i2c_create(dev);
+	if (IS_ERR(i2c)) {
+		ret = PTR_ERR(i2c);
+		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+		return ret;
+	}
 
-	if (ast_vga_connector->i2c)
-		ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
-						  DRM_MODE_CONNECTOR_VGA,
-						  &ast_vga_connector->i2c->adapter);
-	else
-		ret = drm_connector_init(dev, connector, &ast_vga_connector_funcs,
-					 DRM_MODE_CONNECTOR_VGA);
+	ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
+					  DRM_MODE_CONNECTOR_VGA, &i2c->adapter);
 	if (ret)
 		return ret;
 
@@ -1451,22 +1445,18 @@ static int ast_vga_output_init(struct ast_device *ast)
 
 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 drm_device *dev = connector->dev;
 	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	if (!ast_sil164_connector->i2c)
-		goto err_drm_connector_update_edid_property;
-
 	/*
 	 * Protect access to I/O registers from concurrent modesetting
 	 * by acquiring the I/O-register lock.
 	 */
 	mutex_lock(&ast->modeset_lock);
 
-	edid = drm_get_edid(connector, &ast_sil164_connector->i2c->adapter);
+	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
 		goto err_mutex_unlock;
 
@@ -1479,7 +1469,6 @@ static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector
 
 err_mutex_unlock:
 	mutex_unlock(&ast->modeset_lock);
-err_drm_connector_update_edid_property:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1500,19 +1489,18 @@ 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;
+	struct ast_i2c_chan *i2c;
 	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");
+	i2c = ast_i2c_create(dev);
+	if (IS_ERR(i2c)) {
+		ret = PTR_ERR(i2c);
+		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+		return ret;
+	}
 
-	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);
+	ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
+					  DRM_MODE_CONNECTOR_DVII, &i2c->adapter);
 	if (ret)
 		return ret;
 
-- 
2.44.0

[PATCH v4 03/12] drm/ast: Remove struct ast_{vga,sil165}_connector

[Thomas Zimmermann]

Both, struct ast_vga_connector and struct ast_sil164_connector, are
now wrappers around struct drm_connector. Remove them.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Patrik Jakobsson <patrik.r.jakobsson@gmail.com>
---
 drivers/gpu/drm/ast/ast_drv.h  | 24 ++----------------------
 drivers/gpu/drm/ast/ast_mode.c | 22 ++++++++--------------
 2 files changed, 10 insertions(+), 36 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index cb0e4f332be80..12ad1c0fe151b 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -158,26 +158,6 @@ struct ast_i2c_chan {
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_vga_connector {
-	struct drm_connector base;
-};
-
-static inline struct ast_vga_connector *
-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;
-};
-
-static inline struct ast_sil164_connector *
-to_ast_sil164_connector(struct drm_connector *connector)
-{
-	return container_of(connector, struct ast_sil164_connector, base);
-}
-
 struct ast_bmc_connector {
 	struct drm_connector base;
 	struct drm_connector *physical_connector;
@@ -220,11 +200,11 @@ struct ast_device {
 	struct {
 		struct {
 			struct drm_encoder encoder;
-			struct ast_vga_connector vga_connector;
+			struct drm_connector connector;
 		} vga;
 		struct {
 			struct drm_encoder encoder;
-			struct ast_sil164_connector sil164_connector;
+			struct drm_connector connector;
 		} sil164;
 		struct {
 			struct drm_encoder encoder;
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 38c31a7283a69..cbda04fca7107 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1385,10 +1385,8 @@ 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,
-				  struct ast_vga_connector *ast_vga_connector)
+static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct drm_connector *connector = &ast_vga_connector->base;
 	struct ast_i2c_chan *i2c;
 	int ret;
 
@@ -1419,8 +1417,7 @@ static int ast_vga_output_init(struct ast_device *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;
+	struct drm_connector *connector = &ast->output.vga.connector;
 	int ret;
 
 	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_DAC);
@@ -1428,7 +1425,7 @@ static int ast_vga_output_init(struct ast_device *ast)
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	ret = ast_vga_connector_init(dev, ast_vga_connector);
+	ret = ast_vga_connector_init(dev, connector);
 	if (ret)
 		return ret;
 
@@ -1485,10 +1482,8 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = {
 	.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)
+static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct drm_connector *connector = &ast_sil164_connector->base;
 	struct ast_i2c_chan *i2c;
 	int ret;
 
@@ -1519,8 +1514,7 @@ static int ast_sil164_output_init(struct ast_device *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;
+	struct drm_connector *connector = &ast->output.sil164.connector;
 	int ret;
 
 	ret = drm_simple_encoder_init(dev, encoder, DRM_MODE_ENCODER_TMDS);
@@ -1528,7 +1522,7 @@ static int ast_sil164_output_init(struct ast_device *ast)
 		return ret;
 	encoder->possible_crtcs = drm_crtc_mask(crtc);
 
-	ret = ast_sil164_connector_init(dev, ast_sil164_connector);
+	ret = ast_sil164_connector_init(dev, connector);
 	if (ret)
 		return ret;
 
@@ -1940,13 +1934,13 @@ int ast_mode_config_init(struct ast_device *ast)
 		ret = ast_vga_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.vga.vga_connector.base;
+		physical_connector = &ast->output.vga.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_SIL164_BIT) {
 		ret = ast_sil164_output_init(ast);
 		if (ret)
 			return ret;
-		physical_connector = &ast->output.sil164.sil164_connector.base;
+		physical_connector = &ast->output.sil164.connector;
 	}
 	if (ast->tx_chip_types & AST_TX_DP501_BIT) {
 		ret = ast_dp501_output_init(ast);
-- 
2.44.0

[PATCH v4 04/12] drm/ast: Allocate instance of struct ast_i2c_chan with managed helpers

[Thomas Zimmermann]

Replace kzalloc() with drmm_kzalloc() and thereby put the release of
the I2C instance into a separate action. Avoids explicit error roll-
back in ast_i2c_chan_create(). No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_i2c.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_i2c.c
index c3046223a4919..dc28a5cbb1c2a 100644
--- a/drivers/gpu/drm/ast/ast_i2c.c
+++ b/drivers/gpu/drm/ast/ast_i2c.c
@@ -107,7 +107,6 @@ static void ast_i2c_release(struct drm_device *dev, void *res)
 	struct ast_i2c_chan *i2c = res;
 
 	i2c_del_adapter(&i2c->adapter);
-	kfree(i2c);
 }
 
 struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
@@ -115,7 +114,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	struct ast_i2c_chan *i2c;
 	int ret;
 
-	i2c = kzalloc(sizeof(struct ast_i2c_chan), GFP_KERNEL);
+	i2c = drmm_kzalloc(dev->dev, sizeof(*i2c), GFP_KERNEL);
 	if (!i2c)
 		return ERR_PTR(-ENOMEM);
 
@@ -137,7 +136,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	ret = i2c_bit_add_bus(&i2c->adapter);
 	if (ret) {
 		drm_err(dev, "Failed to register bit i2c\n");
-		goto out_kfree;
+		return ERR_PTR(ret);
 	}
 
 	ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
@@ -145,8 +144,4 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 		return ERR_PTR(ret);
 
 	return i2c;
-
-out_kfree:
-	kfree(i2c);
-	return ERR_PTR(ret);
 }
-- 
2.44.0

[PATCH v4 05/12] drm/ast: Move DDC code to ast_ddc.{c,h}

[Thomas Zimmermann]

Rename ast_i2c.c to ast_ddc.c and move its interface into the
new header ast_ddc.h. Update all include statements as necessary
and change the adapter name to 'AST DDC bus'.

This avoids including I2C headers in the driver's main header file,
which doesn't need them. Renaming files to _ddc indicates that the
code is about the DDC. I2C is really just the underlying bus here.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/Makefile                 | 10 +++++++++-
 drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} |  4 ++--
 drivers/gpu/drm/ast/ast_ddc.h                | 19 +++++++++++++++++++
 drivers/gpu/drm/ast/ast_drv.h                | 13 +------------
 drivers/gpu/drm/ast/ast_mode.c               |  1 +
 5 files changed, 32 insertions(+), 15 deletions(-)
 rename drivers/gpu/drm/ast/{ast_i2c.c => ast_ddc.c} (97%)
 create mode 100644 drivers/gpu/drm/ast/ast_ddc.h

diff --git a/drivers/gpu/drm/ast/Makefile b/drivers/gpu/drm/ast/Makefile
index 5a53ce51fb249..d794c076bc242 100644
--- a/drivers/gpu/drm/ast/Makefile
+++ b/drivers/gpu/drm/ast/Makefile
@@ -3,6 +3,14 @@
 # Makefile for the drm device driver.  This driver provides support for the
 # Direct Rendering Infrastructure (DRI) in XFree86 4.1.0 and higher.
 
-ast-y := ast_drv.o ast_i2c.o ast_main.o ast_mm.o ast_mode.o ast_post.o ast_dp501.o ast_dp.o
+ast-y := \
+	ast_ddc.o \
+	ast_dp501.o \
+	ast_dp.o \
+	ast_drv.o \
+	ast_main.o \
+	ast_mm.o \
+	ast_mode.o \
+	ast_post.o
 
 obj-$(CONFIG_DRM_AST) := ast.o
diff --git a/drivers/gpu/drm/ast/ast_i2c.c b/drivers/gpu/drm/ast/ast_ddc.c
similarity index 97%
rename from drivers/gpu/drm/ast/ast_i2c.c
rename to drivers/gpu/drm/ast/ast_ddc.c
index dc28a5cbb1c2a..df604b4e9673c 100644
--- a/drivers/gpu/drm/ast/ast_i2c.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -24,6 +24,7 @@
 #include <drm/drm_managed.h>
 #include <drm/drm_print.h>
 
+#include "ast_ddc.h"
 #include "ast_drv.h"
 
 static void ast_i2c_setsda(void *i2c_priv, int data)
@@ -122,8 +123,7 @@ struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
 	i2c->adapter.dev.parent = dev->dev;
 	i2c->dev = dev;
 	i2c_set_adapdata(&i2c->adapter, i2c);
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name),
-		 "AST i2c bit bus");
+	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "AST DDC bus");
 	i2c->adapter.algo_data = &i2c->bit;
 
 	i2c->bit.udelay = 20;
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
new file mode 100644
index 0000000000000..244666fd6c530
--- /dev/null
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -0,0 +1,19 @@
+/* SPDX-License-Identifier: MIT */
+
+#ifndef __AST_DDC_H__
+#define __AST_DDC_H__
+
+#include <linux/i2c.h>
+#include <linux/i2c-algo-bit.h>
+
+struct drm_device;
+
+struct ast_i2c_chan {
+	struct i2c_adapter adapter;
+	struct drm_device *dev;
+	struct i2c_algo_bit_data bit;
+};
+
+struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
+
+#endif
diff --git a/drivers/gpu/drm/ast/ast_drv.h b/drivers/gpu/drm/ast/ast_drv.h
index 12ad1c0fe151b..ba3d86973995f 100644
--- a/drivers/gpu/drm/ast/ast_drv.h
+++ b/drivers/gpu/drm/ast/ast_drv.h
@@ -28,8 +28,6 @@
 #ifndef __AST_DRV_H__
 #define __AST_DRV_H__
 
-#include <linux/i2c.h>
-#include <linux/i2c-algo-bit.h>
 #include <linux/io.h>
 #include <linux/types.h>
 
@@ -149,15 +147,9 @@ static inline struct ast_plane *to_ast_plane(struct drm_plane *plane)
 }
 
 /*
- * Connector with i2c channel
+ * BMC
  */
 
-struct ast_i2c_chan {
-	struct i2c_adapter adapter;
-	struct drm_device *dev;
-	struct i2c_algo_bit_data bit;
-};
-
 struct ast_bmc_connector {
 	struct drm_connector base;
 	struct drm_connector *physical_connector;
@@ -476,9 +468,6 @@ bool ast_dp501_read_edid(struct drm_device *dev, u8 *ediddata);
 u8 ast_get_dp501_max_clk(struct drm_device *dev);
 void ast_init_3rdtx(struct drm_device *dev);
 
-/* ast_i2c.c */
-struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
-
 /* aspeed DP */
 bool ast_astdp_is_connected(struct ast_device *ast);
 int ast_astdp_read_edid(struct drm_device *dev, u8 *ediddata);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index cbda04fca7107..bdef2160726e6 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -46,6 +46,7 @@
 #include <drm/drm_probe_helper.h>
 #include <drm/drm_simple_kms_helper.h>
 
+#include "ast_ddc.h"
 #include "ast_drv.h"
 #include "ast_tables.h"
 
-- 
2.44.0

[PATCH v4 06/12] drm/ast: Rename struct ast_i2c_chan to struct ast_ddc

[Thomas Zimmermann]

The struct struct ast_i2c_chan represents the Display Data Channel
(DDC); I2C is the underlying bus. Rename the structure, the variables
and the helper ast_i2c_create() to ddc-like terms. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c  | 71 ++++++++++++++++++----------------
 drivers/gpu/drm/ast/ast_ddc.h  |  4 +-
 drivers/gpu/drm/ast/ast_mode.c | 24 ++++++------
 3 files changed, 52 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index df604b4e9673c..c0e5d03c028d8 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -29,8 +29,8 @@
 
 static void ast_i2c_setsda(void *i2c_priv, int data)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -45,8 +45,8 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 
 static void ast_i2c_setscl(void *i2c_priv, int clock)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -61,8 +61,8 @@ static void ast_i2c_setscl(void *i2c_priv, int clock)
 
 static int ast_i2c_getsda(void *i2c_priv)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -83,8 +83,8 @@ static int ast_i2c_getsda(void *i2c_priv)
 
 static int ast_i2c_getscl(void *i2c_priv)
 {
-	struct ast_i2c_chan *i2c = i2c_priv;
-	struct ast_device *ast = to_ast_device(i2c->dev);
+	struct ast_ddc *ddc = i2c_priv;
+	struct ast_device *ast = to_ast_device(ddc->dev);
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -103,45 +103,50 @@ static int ast_i2c_getscl(void *i2c_priv)
 	return val & 1 ? 1 : 0;
 }
 
-static void ast_i2c_release(struct drm_device *dev, void *res)
+static void ast_ddc_release(struct drm_device *dev, void *res)
 {
-	struct ast_i2c_chan *i2c = res;
+	struct ast_ddc *ddc = res;
 
-	i2c_del_adapter(&i2c->adapter);
+	i2c_del_adapter(&ddc->adapter);
 }
 
-struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev)
+struct ast_ddc *ast_ddc_create(struct drm_device *dev)
 {
-	struct ast_i2c_chan *i2c;
+	struct ast_ddc *ddc;
+	struct i2c_adapter *adapter;
+	struct i2c_algo_bit_data *bit;
 	int ret;
 
-	i2c = drmm_kzalloc(dev->dev, sizeof(*i2c), GFP_KERNEL);
-	if (!i2c)
+	ddc = drmm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
+	if (!ddc)
 		return ERR_PTR(-ENOMEM);
-
-	i2c->adapter.owner = THIS_MODULE;
-	i2c->adapter.dev.parent = dev->dev;
-	i2c->dev = dev;
-	i2c_set_adapdata(&i2c->adapter, i2c);
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "AST DDC bus");
-	i2c->adapter.algo_data = &i2c->bit;
-
-	i2c->bit.udelay = 20;
-	i2c->bit.timeout = 2;
-	i2c->bit.data = i2c;
-	i2c->bit.setsda = ast_i2c_setsda;
-	i2c->bit.setscl = ast_i2c_setscl;
-	i2c->bit.getsda = ast_i2c_getsda;
-	i2c->bit.getscl = ast_i2c_getscl;
-	ret = i2c_bit_add_bus(&i2c->adapter);
+	ddc->dev = dev;
+
+	adapter = &ddc->adapter;
+	adapter->owner = THIS_MODULE;
+	adapter->dev.parent = dev->dev;
+	i2c_set_adapdata(adapter, ddc);
+	snprintf(adapter->name, sizeof(adapter->name), "AST DDC bus");
+
+	bit = &ddc->bit;
+	bit->udelay = 20;
+	bit->timeout = 2;
+	bit->data = ddc;
+	bit->setsda = ast_i2c_setsda;
+	bit->setscl = ast_i2c_setscl;
+	bit->getsda = ast_i2c_getsda;
+	bit->getscl = ast_i2c_getscl;
+
+	adapter->algo_data = bit;
+	ret = i2c_bit_add_bus(adapter);
 	if (ret) {
 		drm_err(dev, "Failed to register bit i2c\n");
 		return ERR_PTR(ret);
 	}
 
-	ret = drmm_add_action_or_reset(dev, ast_i2c_release, i2c);
+	ret = drmm_add_action_or_reset(dev, ast_ddc_release, ddc);
 	if (ret)
 		return ERR_PTR(ret);
 
-	return i2c;
+	return ddc;
 }
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
index 244666fd6c530..071f9be3e27de 100644
--- a/drivers/gpu/drm/ast/ast_ddc.h
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -8,12 +8,12 @@
 
 struct drm_device;
 
-struct ast_i2c_chan {
+struct ast_ddc {
 	struct i2c_adapter adapter;
 	struct drm_device *dev;
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_i2c_chan *ast_i2c_create(struct drm_device *dev);
+struct ast_ddc *ast_ddc_create(struct drm_device *dev);
 
 #endif
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index bdef2160726e6..40cb495acc908 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1388,18 +1388,18 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = {
 
 static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct ast_i2c_chan *i2c;
+	struct ast_ddc *ddc;
 	int ret;
 
-	i2c = ast_i2c_create(dev);
-	if (IS_ERR(i2c)) {
-		ret = PTR_ERR(i2c);
-		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+	ddc = ast_ddc_create(dev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
+		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector, &ast_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA, &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, &ddc->adapter);
 	if (ret)
 		return ret;
 
@@ -1485,18 +1485,18 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = {
 
 static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
-	struct ast_i2c_chan *i2c;
+	struct ast_ddc *ddc;
 	int ret;
 
-	i2c = ast_i2c_create(dev);
-	if (IS_ERR(i2c)) {
-		ret = PTR_ERR(i2c);
-		drm_err(dev, "failed to add ddc bus for connector; ret=%d\n", ret);
+	ddc = ast_ddc_create(dev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
+		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector, &ast_sil164_connector_funcs,
-					  DRM_MODE_CONNECTOR_DVII, &i2c->adapter);
+					  DRM_MODE_CONNECTOR_DVII, &ddc->adapter);
 	if (ret)
 		return ret;
 
-- 
2.44.0

[PATCH v4 07/12] drm/ast: Pass AST device to ast_ddc_create()

[Thomas Zimmermann]

The DDC code needs the AST device. Pass it to ast_ddc_create() and
avoid an internal upcast. Improves type safety within the DDC code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c  | 3 ++-
 drivers/gpu/drm/ast/ast_ddc.h  | 3 ++-
 drivers/gpu/drm/ast/ast_mode.c | 6 ++++--
 3 files changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index c0e5d03c028d8..24b7d589f0d4c 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -110,8 +110,9 @@ static void ast_ddc_release(struct drm_device *dev, void *res)
 	i2c_del_adapter(&ddc->adapter);
 }
 
-struct ast_ddc *ast_ddc_create(struct drm_device *dev)
+struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 {
+	struct drm_device *dev = &ast->base;
 	struct ast_ddc *ddc;
 	struct i2c_adapter *adapter;
 	struct i2c_algo_bit_data *bit;
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
index 071f9be3e27de..d423b144a3458 100644
--- a/drivers/gpu/drm/ast/ast_ddc.h
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -6,6 +6,7 @@
 #include <linux/i2c.h>
 #include <linux/i2c-algo-bit.h>
 
+struct ast_device;
 struct drm_device;
 
 struct ast_ddc {
@@ -14,6 +15,6 @@ struct ast_ddc {
 	struct i2c_algo_bit_data bit;
 };
 
-struct ast_ddc *ast_ddc_create(struct drm_device *dev);
+struct ast_ddc *ast_ddc_create(struct ast_device *ast);
 
 #endif
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 40cb495acc908..fc73d3b65b2a1 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1388,10 +1388,11 @@ static const struct drm_connector_funcs ast_vga_connector_funcs = {
 
 static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
+	struct ast_device *ast = to_ast_device(dev);
 	struct ast_ddc *ddc;
 	int ret;
 
-	ddc = ast_ddc_create(dev);
+	ddc = ast_ddc_create(ast);
 	if (IS_ERR(ddc)) {
 		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
@@ -1485,10 +1486,11 @@ static const struct drm_connector_funcs ast_sil164_connector_funcs = {
 
 static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connector *connector)
 {
+	struct ast_device *ast = to_ast_device(dev);
 	struct ast_ddc *ddc;
 	int ret;
 
-	ddc = ast_ddc_create(dev);
+	ddc = ast_ddc_create(ast);
 	if (IS_ERR(ddc)) {
 		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus for connector; ret=%d\n", ret);
-- 
2.44.0

[PATCH v4 08/12] drm/ast: Store AST device in struct ast_ddc

[Thomas Zimmermann]

The DDC code needs the AST device. Store a pointer in struct ast_ddc
and avoid internal upcasts. Improves type safety within the DDC code.

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

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index 24b7d589f0d4c..47670285fd765 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -30,7 +30,7 @@
 static void ast_i2c_setsda(void *i2c_priv, int data)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -46,7 +46,7 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 static void ast_i2c_setscl(void *i2c_priv, int clock)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
@@ -62,7 +62,7 @@ static void ast_i2c_setscl(void *i2c_priv, int clock)
 static int ast_i2c_getsda(void *i2c_priv)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -84,7 +84,7 @@ static int ast_i2c_getsda(void *i2c_priv)
 static int ast_i2c_getscl(void *i2c_priv)
 {
 	struct ast_ddc *ddc = i2c_priv;
-	struct ast_device *ast = to_ast_device(ddc->dev);
+	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
 	count = 0;
@@ -121,7 +121,7 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 	ddc = drmm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
 	if (!ddc)
 		return ERR_PTR(-ENOMEM);
-	ddc->dev = dev;
+	ddc->ast = ast;
 
 	adapter = &ddc->adapter;
 	adapter->owner = THIS_MODULE;
diff --git a/drivers/gpu/drm/ast/ast_ddc.h b/drivers/gpu/drm/ast/ast_ddc.h
index d423b144a3458..08f3994e09ccd 100644
--- a/drivers/gpu/drm/ast/ast_ddc.h
+++ b/drivers/gpu/drm/ast/ast_ddc.h
@@ -7,11 +7,11 @@
 #include <linux/i2c-algo-bit.h>
 
 struct ast_device;
-struct drm_device;
 
 struct ast_ddc {
+	struct ast_device *ast;
+
 	struct i2c_adapter adapter;
-	struct drm_device *dev;
 	struct i2c_algo_bit_data bit;
 };
 
-- 
2.44.0

[PATCH v4 09/12] drm/ast: Rename struct i2c_algo_bit_data callbacks and their parameters

[Thomas Zimmermann]

Align the names of the algo-bit helpers with ast's convention of
using an ast prefix plus the struct's name plus the callback's name
for such function symbols. Change the parameter names of these
helpers to 'data' and 'state', as used in the declaration of struct
i2c_algo_bit_data. No functional changes.

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

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index 47670285fd765..b84e656124f18 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -27,15 +27,15 @@
 #include "ast_ddc.h"
 #include "ast_drv.h"
 
-static void ast_i2c_setsda(void *i2c_priv, int data)
+static void ast_ddc_algo_bit_data_setsda(void *data, int state)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
 	for (i = 0; i < 0x10000; i++) {
-		ujcrb7 = ((data & 0x01) ? 0 : 1) << 2;
+		ujcrb7 = ((state & 0x01) ? 0 : 1) << 2;
 		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0xf1, ujcrb7);
 		jtemp = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0x04);
 		if (ujcrb7 == jtemp)
@@ -43,15 +43,15 @@ static void ast_i2c_setsda(void *i2c_priv, int data)
 	}
 }
 
-static void ast_i2c_setscl(void *i2c_priv, int clock)
+static void ast_ddc_algo_bit_data_setscl(void *data, int state)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	int i;
 	u8 ujcrb7, jtemp;
 
 	for (i = 0; i < 0x10000; i++) {
-		ujcrb7 = ((clock & 0x01) ? 0 : 1);
+		ujcrb7 = ((state & 0x01) ? 0 : 1);
 		ast_set_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0xf4, ujcrb7);
 		jtemp = ast_get_index_reg_mask(ast, AST_IO_VGACRI, 0xb7, 0x01);
 		if (ujcrb7 == jtemp)
@@ -59,9 +59,9 @@ static void ast_i2c_setscl(void *i2c_priv, int clock)
 	}
 }
 
-static int ast_i2c_getsda(void *i2c_priv)
+static int ast_ddc_algo_bit_data_getsda(void *data)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
@@ -81,9 +81,9 @@ static int ast_i2c_getsda(void *i2c_priv)
 	return val & 1 ? 1 : 0;
 }
 
-static int ast_i2c_getscl(void *i2c_priv)
+static int ast_ddc_algo_bit_data_getscl(void *data)
 {
-	struct ast_ddc *ddc = i2c_priv;
+	struct ast_ddc *ddc = data;
 	struct ast_device *ast = ddc->ast;
 	uint32_t val, val2, count, pass;
 
@@ -133,10 +133,10 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 	bit->udelay = 20;
 	bit->timeout = 2;
 	bit->data = ddc;
-	bit->setsda = ast_i2c_setsda;
-	bit->setscl = ast_i2c_setscl;
-	bit->getsda = ast_i2c_getsda;
-	bit->getscl = ast_i2c_getscl;
+	bit->setsda = ast_ddc_algo_bit_data_setsda;
+	bit->setscl = ast_ddc_algo_bit_data_setscl;
+	bit->getsda = ast_ddc_algo_bit_data_getsda;
+	bit->getscl = ast_ddc_algo_bit_data_getscl;
 
 	adapter->algo_data = bit;
 	ret = i2c_bit_add_bus(adapter);
-- 
2.44.0

[PATCH v4 10/12] drm/ast: Acquire I/O-register lock in DDC code

[Thomas Zimmermann]

The modeset lock protects the DDC code from concurrent modeset
operations, which use the same registers. Move that code from the
connector helpers into the DDC helpers .pre_xfer() and .post_xfer().

Both, .pre_xfer() and .post_xfer(), enclose the transfer of data blocks
over the I2C channel in the internal I2C function bit_xfer(). Both
calls are executed unconditionally if present. Invoking DDC transfers
from any where within the driver now takes the lock.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/ast/ast_ddc.c  | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/ast/ast_mode.c | 30 ++++--------------------------
 2 files changed, 28 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/ast/ast_ddc.c b/drivers/gpu/drm/ast/ast_ddc.c
index b84e656124f18..b7718084422f3 100644
--- a/drivers/gpu/drm/ast/ast_ddc.c
+++ b/drivers/gpu/drm/ast/ast_ddc.c
@@ -59,6 +59,28 @@ static void ast_ddc_algo_bit_data_setscl(void *data, int state)
 	}
 }
 
+static int ast_ddc_algo_bit_data_pre_xfer(struct i2c_adapter *adapter)
+{
+	struct ast_ddc *ddc = i2c_get_adapdata(adapter);
+	struct ast_device *ast = ddc->ast;
+
+	/*
+	 * Protect access to I/O registers from concurrent modesetting
+	 * by acquiring the I/O-register lock.
+	 */
+	mutex_lock(&ast->modeset_lock);
+
+	return 0;
+}
+
+static void ast_ddc_algo_bit_data_post_xfer(struct i2c_adapter *adapter)
+{
+	struct ast_ddc *ddc = i2c_get_adapdata(adapter);
+	struct ast_device *ast = ddc->ast;
+
+	mutex_unlock(&ast->modeset_lock);
+}
+
 static int ast_ddc_algo_bit_data_getsda(void *data)
 {
 	struct ast_ddc *ddc = data;
@@ -137,6 +159,8 @@ struct ast_ddc *ast_ddc_create(struct ast_device *ast)
 	bit->setscl = ast_ddc_algo_bit_data_setscl;
 	bit->getsda = ast_ddc_algo_bit_data_getsda;
 	bit->getscl = ast_ddc_algo_bit_data_getscl;
+	bit->pre_xfer = ast_ddc_algo_bit_data_pre_xfer;
+	bit->post_xfer = ast_ddc_algo_bit_data_post_xfer;
 
 	adapter->algo_data = bit;
 	ret = i2c_bit_add_bus(adapter);
diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index fc73d3b65b2a1..8766a0f2eb3c7 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1346,30 +1346,19 @@ static int ast_crtc_init(struct drm_device *dev)
 
 static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	/*
-	 * Protect access to I/O registers from concurrent modesetting
-	 * by acquiring the I/O-register lock.
-	 */
-	mutex_lock(&ast->modeset_lock);
-
 	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
-		goto err_mutex_unlock;
-
-	mutex_unlock(&ast->modeset_lock);
+		goto err_drm_get_edid;
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
 	return count;
 
-err_mutex_unlock:
-	mutex_unlock(&ast->modeset_lock);
+err_drm_get_edid:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
@@ -1444,30 +1433,19 @@ static int ast_vga_output_init(struct ast_device *ast)
 
 static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct drm_device *dev = connector->dev;
-	struct ast_device *ast = to_ast_device(dev);
 	struct edid *edid;
 	int count;
 
-	/*
-	 * Protect access to I/O registers from concurrent modesetting
-	 * by acquiring the I/O-register lock.
-	 */
-	mutex_lock(&ast->modeset_lock);
-
 	edid = drm_get_edid(connector, connector->ddc);
 	if (!edid)
-		goto err_mutex_unlock;
-
-	mutex_unlock(&ast->modeset_lock);
+		goto err_drm_get_edid;
 
 	count = drm_add_edid_modes(connector, edid);
 	kfree(edid);
 
 	return count;
 
-err_mutex_unlock:
-	mutex_unlock(&ast->modeset_lock);
+err_drm_get_edid:
 	drm_connector_update_edid_property(connector, NULL);
 	return 0;
 }
-- 
2.44.0

[PATCH v4 11/12] drm/ast: Use drm_connector_helper_get_modes()

[Thomas Zimmermann]

The .get_modes() code for VGA and SIL164 connectors does not depend
on either type of connector. Replace the driver code with the common
helper drm_connector_helper_get_modes(). It reads EDID data via
DDC and updates the connector's EDID property.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 8766a0f2eb3c7..71cc681d6188f 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1344,27 +1344,8 @@ static int ast_crtc_init(struct drm_device *dev)
  * VGA Connector
  */
 
-static int ast_vga_connector_helper_get_modes(struct drm_connector *connector)
-{
-	struct edid *edid;
-	int count;
-
-	edid = drm_get_edid(connector, connector->ddc);
-	if (!edid)
-		goto err_drm_get_edid;
-
-	count = drm_add_edid_modes(connector, edid);
-	kfree(edid);
-
-	return count;
-
-err_drm_get_edid:
-	drm_connector_update_edid_property(connector, NULL);
-	return 0;
-}
-
 static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
-	.get_modes = ast_vga_connector_helper_get_modes,
+	.get_modes = drm_connector_helper_get_modes,
 };
 
 static const struct drm_connector_funcs ast_vga_connector_funcs = {
@@ -1431,27 +1412,8 @@ static int ast_vga_output_init(struct ast_device *ast)
  * SIL164 Connector
  */
 
-static int ast_sil164_connector_helper_get_modes(struct drm_connector *connector)
-{
-	struct edid *edid;
-	int count;
-
-	edid = drm_get_edid(connector, connector->ddc);
-	if (!edid)
-		goto err_drm_get_edid;
-
-	count = drm_add_edid_modes(connector, edid);
-	kfree(edid);
-
-	return count;
-
-err_drm_get_edid:
-	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,
+	.get_modes = drm_connector_helper_get_modes,
 };
 
 static const struct drm_connector_funcs ast_sil164_connector_funcs = {
-- 
2.44.0

[PATCH v4 12/12] drm/ast: Implement polling for VGA and SIL164 connectors

[Thomas Zimmermann]

Implement polling for VGA and SIL164 connectors. Set the flag
DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
for each type of connector by testing for EDID data. The code for
both types of connectors is identical for now. Maybe this can later
become a common helper function for various drivers.

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

diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
index 71cc681d6188f..f740b8706a38b 100644
--- a/drivers/gpu/drm/ast/ast_mode.c
+++ b/drivers/gpu/drm/ast/ast_mode.c
@@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev)
  * VGA Connector
  */
 
+static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
+					       struct drm_modeset_acquire_ctx *ctx,
+					       bool force)
+{
+	enum drm_connector_status status = connector_status_disconnected;
+	const struct drm_edid *edid;
+
+	edid = drm_edid_read(connector);
+	if (edid)
+		status = connector_status_connected;
+	drm_edid_free(edid);
+
+	return status;
+}
+
 static const struct drm_connector_helper_funcs ast_vga_connector_helper_funcs = {
 	.get_modes = drm_connector_helper_get_modes,
+	.detect_ctx = ast_vga_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_vga_connector_funcs = {
@@ -1379,7 +1395,7 @@ static int ast_vga_connector_init(struct drm_device *dev, struct drm_connector *
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return 0;
 }
@@ -1412,8 +1428,24 @@ static int ast_vga_output_init(struct ast_device *ast)
  * SIL164 Connector
  */
 
+static int ast_sil164_connector_helper_detect_ctx(struct drm_connector *connector,
+						  struct drm_modeset_acquire_ctx *ctx,
+						  bool force)
+{
+	enum drm_connector_status status = connector_status_disconnected;
+	const struct drm_edid *edid;
+
+	edid = drm_edid_read(connector);
+	if (edid)
+		status = connector_status_connected;
+	drm_edid_free(edid);
+
+	return status;
+}
+
 static const struct drm_connector_helper_funcs ast_sil164_connector_helper_funcs = {
 	.get_modes = drm_connector_helper_get_modes,
+	.detect_ctx = ast_sil164_connector_helper_detect_ctx,
 };
 
 static const struct drm_connector_funcs ast_sil164_connector_funcs = {
@@ -1447,7 +1479,7 @@ static int ast_sil164_connector_init(struct drm_device *dev, struct drm_connecto
 	connector->interlace_allowed = 0;
 	connector->doublescan_allowed = 0;
 
-	connector->polled = DRM_CONNECTOR_POLL_CONNECT;
+	connector->polled = DRM_CONNECTOR_POLL_CONNECT | DRM_CONNECTOR_POLL_DISCONNECT;
 
 	return 0;
 }
-- 
2.44.0

Re: [PATCH v4 12/12] drm/ast: Implement polling for VGA and SIL164 connectors

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 1734 bytes --]

Hi,

On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote:
> Implement polling for VGA and SIL164 connectors. Set the flag
> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
> for each type of connector by testing for EDID data. The code for
> both types of connectors is identical for now. Maybe this can later
> become a common helper function for various drivers.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++--
>  1 file changed, 34 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
> index 71cc681d6188f..f740b8706a38b 100644
> --- a/drivers/gpu/drm/ast/ast_mode.c
> +++ b/drivers/gpu/drm/ast/ast_mode.c
> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev)
>   * VGA Connector
>   */
>  
> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
> +					       struct drm_modeset_acquire_ctx *ctx,
> +					       bool force)
> +{
> +	enum drm_connector_status status = connector_status_disconnected;
> +	const struct drm_edid *edid;
> +
> +	edid = drm_edid_read(connector);
> +	if (edid)
> +		status = connector_status_connected;
> +	drm_edid_free(edid);
> +
> +	return status;
> +}
> +

Yeah, I think it would be worth turning it into a helper. We have a
number of drivers using some variation of that already
(display-connector, loongson).

It's probably better to use drm_probe_ddc here too instead of parsing
and updating the EDID property each time we call detect.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH v4 12/12] drm/ast: Implement polling for VGA and SIL164 connectors

[Thomas Zimmermann]

Hi

Am 19.03.24 um 10:37 schrieb Maxime Ripard:
> Hi,
>
> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote:
>> Implement polling for VGA and SIL164 connectors. Set the flag
>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
>> for each type of connector by testing for EDID data. The code for
>> both types of connectors is identical for now. Maybe this can later
>> become a common helper function for various drivers.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
>>   drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++--
>>   1 file changed, 34 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>> index 71cc681d6188f..f740b8706a38b 100644
>> --- a/drivers/gpu/drm/ast/ast_mode.c
>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev)
>>    * VGA Connector
>>    */
>>   
>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
>> +					       struct drm_modeset_acquire_ctx *ctx,
>> +					       bool force)
>> +{
>> +	enum drm_connector_status status = connector_status_disconnected;
>> +	const struct drm_edid *edid;
>> +
>> +	edid = drm_edid_read(connector);
>> +	if (edid)
>> +		status = connector_status_connected;
>> +	drm_edid_free(edid);
>> +
>> +	return status;
>> +}
>> +
> Yeah, I think it would be worth turning it into a helper. We have a
> number of drivers using some variation of that already
> (display-connector, loongson).
>
> It's probably better to use drm_probe_ddc here too instead of parsing
> and updating the EDID property each time we call detect.

I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update 
the property, which is probably better anyway.

Best regard
Thomas

>
> Maxime

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Re: [PATCH v4 12/12] drm/ast: Implement polling for VGA and SIL164 connectors

[Jani Nikula]

On Tue, 19 Mar 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Hi
>
> Am 19.03.24 um 10:37 schrieb Maxime Ripard:
>> Hi,
>>
>> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote:
>>> Implement polling for VGA and SIL164 connectors. Set the flag
>>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
>>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
>>> for each type of connector by testing for EDID data. The code for
>>> both types of connectors is identical for now. Maybe this can later
>>> become a common helper function for various drivers.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> ---
>>>   drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++--
>>>   1 file changed, 34 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>>> index 71cc681d6188f..f740b8706a38b 100644
>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev)
>>>    * VGA Connector
>>>    */
>>>   
>>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
>>> +					       struct drm_modeset_acquire_ctx *ctx,
>>> +					       bool force)
>>> +{
>>> +	enum drm_connector_status status = connector_status_disconnected;
>>> +	const struct drm_edid *edid;
>>> +
>>> +	edid = drm_edid_read(connector);
>>> +	if (edid)
>>> +		status = connector_status_connected;
>>> +	drm_edid_free(edid);
>>> +
>>> +	return status;
>>> +}
>>> +
>> Yeah, I think it would be worth turning it into a helper. We have a
>> number of drivers using some variation of that already
>> (display-connector, loongson).
>>
>> It's probably better to use drm_probe_ddc here too instead of parsing
>> and updating the EDID property each time we call detect.
>
> I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update 
> the property, which is probably better anyway.

The struct drm_edid based drm_edid_read() and friends do *not* parse the
EDID (apart from what's necessary to read the EDID) nor update the EDID
property. You need to call drm_edid_connector_update() separately for
that. This is by design.

The struct edid based drm_get_edid() does parse and update the property,
and I think adding that was a mistake that a lot of drivers later
depended on, and couldn't be removed.

As a design consideration, IMO it's also a fine approach to read and
cache the EDID and update the property at ->detect, and then use
drm_edid_connector_add_modes() in ->get_modes to avoid re-reading the
EDID at that time. It's not uncommon to need stuff from the EDID between
those hooks.


BR,
Jani.


-- 
Jani Nikula, Intel

Re: [PATCH v4 12/12] drm/ast: Implement polling for VGA and SIL164 connectors

[Jani Nikula]

On Tue, 19 Mar 2024, Jani Nikula <jani.nikula@linux.intel.com> wrote:
> On Tue, 19 Mar 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 19.03.24 um 10:37 schrieb Maxime Ripard:
>>> Hi,
>>>
>>> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote:
>>>> Implement polling for VGA and SIL164 connectors. Set the flag
>>>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
>>>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
>>>> for each type of connector by testing for EDID data. The code for
>>>> both types of connectors is identical for now. Maybe this can later
>>>> become a common helper function for various drivers.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>   drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++--
>>>>   1 file changed, 34 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>>>> index 71cc681d6188f..f740b8706a38b 100644
>>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev)
>>>>    * VGA Connector
>>>>    */
>>>>   
>>>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
>>>> +					       struct drm_modeset_acquire_ctx *ctx,
>>>> +					       bool force)
>>>> +{
>>>> +	enum drm_connector_status status = connector_status_disconnected;
>>>> +	const struct drm_edid *edid;
>>>> +
>>>> +	edid = drm_edid_read(connector);
>>>> +	if (edid)
>>>> +		status = connector_status_connected;
>>>> +	drm_edid_free(edid);
>>>> +
>>>> +	return status;
>>>> +}
>>>> +
>>> Yeah, I think it would be worth turning it into a helper. We have a
>>> number of drivers using some variation of that already
>>> (display-connector, loongson).
>>>
>>> It's probably better to use drm_probe_ddc here too instead of parsing
>>> and updating the EDID property each time we call detect.
>>
>> I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update 
>> the property, which is probably better anyway.
>
> The struct drm_edid based drm_edid_read() and friends do *not* parse the
> EDID (apart from what's necessary to read the EDID) nor update the EDID
> property. You need to call drm_edid_connector_update() separately for
> that. This is by design.
>
> The struct edid based drm_get_edid() does parse and update the property,
> and I think adding that was a mistake that a lot of drivers later
> depended on, and couldn't be removed.
>
> As a design consideration, IMO it's also a fine approach to read and
> cache the EDID and update the property at ->detect, and then use
> drm_edid_connector_add_modes() in ->get_modes to avoid re-reading the
> EDID at that time. It's not uncommon to need stuff from the EDID between
> those hooks.

But if drm_probe_ddc() at ->detect and drm_connector_helper_get_modes()
for ->get_modes are sufficient, nothing wrong with that either.

BR,
Jani.

>
>
> BR,
> Jani.

-- 
Jani Nikula, Intel

Re: [PATCH v4 12/12] drm/ast: Implement polling for VGA and SIL164 connectors

[Thomas Zimmermann]

Hi

Am 19.03.24 um 11:08 schrieb Jani Nikula:
> On Tue, 19 Mar 2024, Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Hi
>>
>> Am 19.03.24 um 10:37 schrieb Maxime Ripard:
>>> Hi,
>>>
>>> On Tue, Mar 19, 2024 at 09:00:32AM +0100, Thomas Zimmermann wrote:
>>>> Implement polling for VGA and SIL164 connectors. Set the flag
>>>> DRM_CONNECTOR_POLL_DISCONNECT for each to detect the removal of the
>>>> monitor cable. Implement struct drm_connector_helper_funcs.detect_ctx
>>>> for each type of connector by testing for EDID data. The code for
>>>> both types of connectors is identical for now. Maybe this can later
>>>> become a common helper function for various drivers.
>>>>
>>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>>> ---
>>>>    drivers/gpu/drm/ast/ast_mode.c | 36 ++++++++++++++++++++++++++++++++--
>>>>    1 file changed, 34 insertions(+), 2 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/ast/ast_mode.c b/drivers/gpu/drm/ast/ast_mode.c
>>>> index 71cc681d6188f..f740b8706a38b 100644
>>>> --- a/drivers/gpu/drm/ast/ast_mode.c
>>>> +++ b/drivers/gpu/drm/ast/ast_mode.c
>>>> @@ -1344,8 +1344,24 @@ static int ast_crtc_init(struct drm_device *dev)
>>>>     * VGA Connector
>>>>     */
>>>>    
>>>> +static int ast_vga_connector_helper_detect_ctx(struct drm_connector *connector,
>>>> +					       struct drm_modeset_acquire_ctx *ctx,
>>>> +					       bool force)
>>>> +{
>>>> +	enum drm_connector_status status = connector_status_disconnected;
>>>> +	const struct drm_edid *edid;
>>>> +
>>>> +	edid = drm_edid_read(connector);
>>>> +	if (edid)
>>>> +		status = connector_status_connected;
>>>> +	drm_edid_free(edid);
>>>> +
>>>> +	return status;
>>>> +}
>>>> +
>>> Yeah, I think it would be worth turning it into a helper. We have a
>>> number of drivers using some variation of that already
>>> (display-connector, loongson).
>>>
>>> It's probably better to use drm_probe_ddc here too instead of parsing
>>> and updating the EDID property each time we call detect.
>> I see. I'll update the patch accordingly. drm_probe_ddc() doesn't update
>> the property, which is probably better anyway.
> The struct drm_edid based drm_edid_read() and friends do *not* parse the
> EDID (apart from what's necessary to read the EDID) nor update the EDID
> property. You need to call drm_edid_connector_update() separately for
> that. This is by design.

Right. I'm confusing things. Sorry.

Best regards
Thomas

>
> The struct edid based drm_get_edid() does parse and update the property,
> and I think adding that was a mistake that a lot of drivers later
> depended on, and couldn't be removed.
>
> As a design consideration, IMO it's also a fine approach to read and
> cache the EDID and update the property at ->detect, and then use
> drm_edid_connector_add_modes() in ->get_modes to avoid re-reading the
> EDID at that time. It's not uncommon to need stuff from the EDID between
> those hooks.
>
>
> BR,
> Jani.
>
>

-- 
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)