[PATCH 00/10] drm/mgag200: Refactor DDC code

[PATCH 00/10] drm/mgag200: Refactor DDC code

[Thomas Zimmermann]

Clean up a the driver's DDC code, make it simpler, more robust, and
mostly self contained. The patches in this patchset have previously
been sent as part of rev 1 of [1].

Patches 1 and 2 fix long-standing problems in the DDC code.

Patches 3 to 9 refactor the DDC code. The code then keeps its data
structures internal, acquires locks automatically and is much more
readable overall.

Patch 10 replaces driver code with an equivalent helper.

Tested on various Matrox hardware.

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

Thomas Zimmermann (10):
  drm/mgag200: Set DDC timeout in milliseconds
  drm/mgag200: Bind I2C lifetime to DRM device
  drm/mgag200: Store pointer to struct mga_device in struct mga_i2c_chan
  drm/mgag200: Allocate instance of struct mga_i2c_chan dynamically
  drm/mgag200: Inline mgag200_i2c_init()
  drm/mgag200: Replace struct mga_i2c_chan with struct mgag200_ddc
  drm/mgag200: Rename mgag200_i2c.c to mgag200_ddc.c
  drm/mgag200: Rename struct i2c_algo_bit_data callbacks
  drm/mgag200: Acquire I/O-register lock in DDC code
  drm/mgag200: Use drm_connector_helper_get_modes()

 drivers/gpu/drm/mgag200/Makefile          |   2 +-
 drivers/gpu/drm/mgag200/mgag200_ddc.c     | 179 ++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_ddc.h     |  11 ++
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  18 +--
 drivers/gpu/drm/mgag200/mgag200_g200.c    |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200er.c  |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200se.c  |  11 +-
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  11 +-
 drivers/gpu/drm/mgag200/mgag200_i2c.c     | 129 ----------------
 drivers/gpu/drm/mgag200/mgag200_mode.c    |  27 +---
 14 files changed, 241 insertions(+), 213 deletions(-)
 create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.c
 create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.h
 delete mode 100644 drivers/gpu/drm/mgag200/mgag200_i2c.c

-- 
2.45.0

[PATCH 01/10] drm/mgag200: Set DDC timeout in milliseconds

[Thomas Zimmermann]

Compute the i2c timeout in jiffies from a value in milliseconds. The
original values of 2 jiffies equals 2 milliseconds if HZ has been
configured to a value of 1000. This corresponds to 2.2 milliseconds
used by most other DRM drivers. Update mgag200 accordingly.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: 414c45310625 ("mgag200: initial g200se driver (v2)")
Cc: Dave Airlie <airlied@redhat.com>
Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Cc: Maxime Ripard <mripard@kernel.org>
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v3.5+
---
 drivers/gpu/drm/mgag200/mgag200_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index 423eb302be7eb..1029fef590f9b 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -114,7 +114,7 @@ int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
 	i2c->adapter.algo_data = &i2c->bit;
 
 	i2c->bit.udelay = 10;
-	i2c->bit.timeout = 2;
+	i2c->bit.timeout = usecs_to_jiffies(2200);
 	i2c->bit.data = i2c;
 	i2c->bit.setsda		= mga_gpio_setsda;
 	i2c->bit.setscl		= mga_gpio_setscl;
-- 
2.45.0

[PATCH 02/10] drm/mgag200: Bind I2C lifetime to DRM device

[Thomas Zimmermann]

Managed cleanup with devm_add_action_or_reset() will release the I2C
adapter when the underlying Linux device goes away. But the connector
still refers to it, so this cleanup leaves behind a stale pointer
in struct drm_connector.ddc.

Bind the lifetime of the I2C adapter to the connector's lifetime by
using DRM's managed release. When the DRM device goes away (after
the Linux device) DRM will first clean up the connector and then
clean up the I2C adapter.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Fixes: b279df242972 ("drm/mgag200: Switch I2C code to managed cleanup")
Cc: Thomas Zimmermann <tzimmermann@suse.de>
Cc: Jocelyn Falempe <jfalempe@redhat.com>
Cc: Dave Airlie <airlied@redhat.com>
Cc: dri-devel@lists.freedesktop.org
Cc: <stable@vger.kernel.org> # v6.0+
---
 drivers/gpu/drm/mgag200/mgag200_i2c.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index 1029fef590f9b..4caeb68f3010c 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -31,6 +31,8 @@
 #include <linux/i2c.h>
 #include <linux/pci.h>
 
+#include <drm/drm_managed.h>
+
 #include "mgag200_drv.h"
 
 static int mga_i2c_read_gpio(struct mga_device *mdev)
@@ -86,7 +88,7 @@ static int mga_gpio_getscl(void *data)
 	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
 }
 
-static void mgag200_i2c_release(void *res)
+static void mgag200_i2c_release(struct drm_device *dev, void *res)
 {
 	struct mga_i2c_chan *i2c = res;
 
@@ -125,5 +127,5 @@ int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
 	if (ret)
 		return ret;
 
-	return devm_add_action_or_reset(dev->dev, mgag200_i2c_release, i2c);
+	return drmm_add_action_or_reset(dev, mgag200_i2c_release, i2c);
 }
-- 
2.45.0

[PATCH 03/10] drm/mgag200: Store pointer to struct mga_device in struct mga_i2c_chan

[Thomas Zimmermann]

Avoid upcasting to struct mga_device in i2c code by storing the
pointer directly. No functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h |  2 +-
 drivers/gpu/drm/mgag200/mgag200_i2c.c | 18 +++++++++---------
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 58a0e62eaf183..c7d4047301bfb 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -191,7 +191,7 @@ static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_s
 
 struct mga_i2c_chan {
 	struct i2c_adapter adapter;
-	struct drm_device *dev;
+	struct mga_device *mdev;
 	struct i2c_algo_bit_data bit;
 	int data, clock;
 };
diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index 4caeb68f3010c..effd7c057fce0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -63,29 +63,29 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
 static void mga_gpio_setsda(void *data, int state)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = to_mga_device(i2c->dev);
-	mga_i2c_set(mdev, i2c->data, state);
+
+	mga_i2c_set(i2c->mdev, i2c->data, state);
 }
 
 static void mga_gpio_setscl(void *data, int state)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = to_mga_device(i2c->dev);
-	mga_i2c_set(mdev, i2c->clock, state);
+
+	mga_i2c_set(i2c->mdev, i2c->clock, state);
 }
 
 static int mga_gpio_getsda(void *data)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = to_mga_device(i2c->dev);
-	return (mga_i2c_read_gpio(mdev) & i2c->data) ? 1 : 0;
+
+	return (mga_i2c_read_gpio(i2c->mdev) & i2c->data) ? 1 : 0;
 }
 
 static int mga_gpio_getscl(void *data)
 {
 	struct mga_i2c_chan *i2c = data;
-	struct mga_device *mdev = to_mga_device(i2c->dev);
-	return (mga_i2c_read_gpio(mdev) & i2c->clock) ? 1 : 0;
+
+	return (mga_i2c_read_gpio(i2c->mdev) & i2c->clock) ? 1 : 0;
 }
 
 static void mgag200_i2c_release(struct drm_device *dev, void *res)
@@ -109,7 +109,7 @@ int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
 	i2c->clock = BIT(info->i2c.clock_bit);
 	i2c->adapter.owner = THIS_MODULE;
 	i2c->adapter.dev.parent = dev->dev;
-	i2c->dev = dev;
+	i2c->mdev = mdev;
 	i2c_set_adapdata(&i2c->adapter, i2c);
 	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "mga i2c");
 
-- 
2.45.0

[PATCH 04/10] drm/mgag200: Allocate instance of struct mga_i2c_chan dynamically

[Thomas Zimmermann]

Allocate instances of struct mga_i2c_chan in mgag200_ddc_create()
and return a pointer to the contained i2c adapter. The callers of
the function are now independent from struct mga_i2c_chan.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_ddc.h     | 11 +++++++++++
 drivers/gpu/drm/mgag200/mgag200_drv.h     |  4 ----
 drivers/gpu/drm/mgag200/mgag200_g200.c    | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200eh.c  | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200eh3.c | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200er.c  | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200ev.c  | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200ew3.c | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200se.c  | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_g200wb.c  | 11 ++++++-----
 drivers/gpu/drm/mgag200/mgag200_i2c.c     | 20 +++++++++++++++++++-
 drivers/gpu/drm/mgag200/mgag200_mode.c    |  1 +
 12 files changed, 79 insertions(+), 45 deletions(-)
 create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.h

diff --git a/drivers/gpu/drm/mgag200/mgag200_ddc.h b/drivers/gpu/drm/mgag200/mgag200_ddc.h
new file mode 100644
index 0000000000000..fa21d197cc783
--- /dev/null
+++ b/drivers/gpu/drm/mgag200/mgag200_ddc.h
@@ -0,0 +1,11 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+
+#ifndef __MGAG200_DDC_H__
+#define __MGAG200_DDC_H__
+
+struct i2c_adapter;
+struct mga_device;
+
+struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev);
+
+#endif
diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index c7d4047301bfb..3c834bfd82cf4 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -294,7 +294,6 @@ struct mga_device {
 	struct drm_plane primary_plane;
 	struct drm_crtc crtc;
 	struct drm_encoder encoder;
-	struct mga_i2c_chan i2c;
 	struct drm_connector connector;
 };
 
@@ -453,7 +452,4 @@ int mgag200_mode_config_init(struct mga_device *mdev, resource_size_t vram_avail
 void mgag200_bmc_disable_vidrst(struct mga_device *mdev);
 void mgag200_bmc_enable_vidrst(struct mga_device *mdev);
 
-				/* mgag200_i2c.c */
-int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c);
-
 #endif				/* __MGAG200_DRV_H__ */
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200.c b/drivers/gpu/drm/mgag200/mgag200_g200.c
index bf5d7fe525a3f..39a29d8ffca6e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200.c
@@ -9,6 +9,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 static int mgag200_g200_init_pci_options(struct pci_dev *pdev)
@@ -201,8 +202,8 @@ static int mgag200_g200_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -238,16 +239,16 @@ static int mgag200_g200_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh.c b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
index fad62453a91db..619fee7ffdf5e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh.c
@@ -9,6 +9,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 void mgag200_g200eh_init_registers(struct mga_device *mdev)
@@ -200,8 +201,8 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -237,16 +238,16 @@ static int mgag200_g200eh_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200eh_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
index 0f7d8112cd49f..a172b8a4500a0 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200eh3.c
@@ -8,6 +8,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 /*
@@ -104,8 +105,8 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -141,16 +142,16 @@ static int mgag200_g200eh3_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200eh3_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200er.c b/drivers/gpu/drm/mgag200/mgag200_g200er.c
index 8d4538b710477..a11c91331e43e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200er.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200er.c
@@ -9,6 +9,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 static void mgag200_g200er_init_registers(struct mga_device *mdev)
@@ -243,8 +244,8 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -280,16 +281,16 @@ static int mgag200_g200er_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200er_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ev.c b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
index 56e6f986bff31..dfb641b83842a 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ev.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ev.c
@@ -9,6 +9,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 static void mgag200_g200ev_init_registers(struct mga_device *mdev)
@@ -244,8 +245,8 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -281,16 +282,16 @@ static int mgag200_g200ev_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200ev_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
index 170934414d7dd..525b7f75e6228 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200ew3.c
@@ -8,6 +8,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 static void mgag200_g200ew3_init_registers(struct mga_device *mdev)
@@ -113,8 +114,8 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -150,16 +151,16 @@ static int mgag200_g200ew3_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200ew3_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200se.c b/drivers/gpu/drm/mgag200/mgag200_g200se.c
index ff2b3c6622e7a..ef7606b529ea1 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200se.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200se.c
@@ -9,6 +9,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 static int mgag200_g200se_init_pci_options(struct pci_dev *pdev)
@@ -375,8 +376,8 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -412,16 +413,16 @@ static int mgag200_g200se_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200se_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_g200wb.c b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
index 9baa727ac6f9f..e4def62b1e575 100644
--- a/drivers/gpu/drm/mgag200/mgag200_g200wb.c
+++ b/drivers/gpu/drm/mgag200/mgag200_g200wb.c
@@ -9,6 +9,7 @@
 #include <drm/drm_gem_atomic_helper.h>
 #include <drm/drm_probe_helper.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 void mgag200_g200wb_init_registers(struct mga_device *mdev)
@@ -247,8 +248,8 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
 	struct drm_plane *primary_plane = &mdev->primary_plane;
 	struct drm_crtc *crtc = &mdev->crtc;
 	struct drm_encoder *encoder = &mdev->encoder;
-	struct mga_i2c_chan *i2c = &mdev->i2c;
 	struct drm_connector *connector = &mdev->connector;
+	struct i2c_adapter *ddc;
 	int ret;
 
 	ret = drm_universal_plane_init(dev, primary_plane, 0,
@@ -284,16 +285,16 @@ static int mgag200_g200wb_pipeline_init(struct mga_device *mdev)
 		return ret;
 	}
 
-	ret = mgag200_i2c_init(mdev, i2c);
-	if (ret) {
+	ddc = mgag200_ddc_create(mdev);
+	if (IS_ERR(ddc)) {
+		ret = PTR_ERR(ddc);
 		drm_err(dev, "failed to add DDC bus: %d\n", ret);
 		return ret;
 	}
 
 	ret = drm_connector_init_with_ddc(dev, connector,
 					  &mgag200_g200wb_vga_connector_funcs,
-					  DRM_MODE_CONNECTOR_VGA,
-					  &i2c->adapter);
+					  DRM_MODE_CONNECTOR_VGA, ddc);
 	if (ret) {
 		drm_err(dev, "drm_connector_init_with_ddc() failed: %d\n", ret);
 		return ret;
diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index effd7c057fce0..46fa9f1b4e469 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -33,6 +33,7 @@
 
 #include <drm/drm_managed.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 static int mga_i2c_read_gpio(struct mga_device *mdev)
@@ -95,7 +96,7 @@ static void mgag200_i2c_release(struct drm_device *dev, void *res)
 	i2c_del_adapter(&i2c->adapter);
 }
 
-int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
+static int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
 {
 	struct drm_device *dev = &mdev->base;
 	const struct mgag200_device_info *info = mdev->info;
@@ -129,3 +130,20 @@ int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
 
 	return drmm_add_action_or_reset(dev, mgag200_i2c_release, i2c);
 }
+
+struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
+{
+	struct mga_i2c_chan *i2c;
+	struct drm_device *dev = &mdev->base;
+	int ret;
+
+	i2c = drmm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return ERR_PTR(-ENOMEM);
+
+	ret = mgag200_i2c_init(mdev, i2c);
+	if (ret)
+		return ERR_PTR(ret);
+
+	return &i2c->adapter;
+}
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index fc54851d3384d..cd1f48b2f9986 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -24,6 +24,7 @@
 #include <drm/drm_panic.h>
 #include <drm/drm_print.h>
 
+#include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
 /*
-- 
2.45.0

[PATCH 05/10] drm/mgag200: Inline mgag200_i2c_init()

[Thomas Zimmermann]

The function mgag200_i2c_init() is an internal helper that sets
up the i2c data structure. Inline its code into the only caller.
Rearrange the individual steps to separate among i2c algorithm,
adapter and fields in struct mga_i2c_chan.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_i2c.c | 62 +++++++++++++--------------
 1 file changed, 29 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index 46fa9f1b4e469..ba7aeca55fb40 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -96,54 +96,50 @@ static void mgag200_i2c_release(struct drm_device *dev, void *res)
 	i2c_del_adapter(&i2c->adapter);
 }
 
-static int mgag200_i2c_init(struct mga_device *mdev, struct mga_i2c_chan *i2c)
+struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
 	const struct mgag200_device_info *info = mdev->info;
+	struct mga_i2c_chan *i2c;
+	struct i2c_algo_bit_data *bit;
+	struct i2c_adapter *adapter;
 	int ret;
 
+	i2c = drmm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
+	if (!i2c)
+		return ERR_PTR(-ENOMEM);
+
 	WREG_DAC(MGA1064_GEN_IO_CTL2, 1);
 	WREG_DAC(MGA1064_GEN_IO_DATA, 0xff);
 	WREG_DAC(MGA1064_GEN_IO_CTL, 0);
 
+	i2c->mdev = mdev;
 	i2c->data = BIT(info->i2c.data_bit);
 	i2c->clock = BIT(info->i2c.clock_bit);
-	i2c->adapter.owner = THIS_MODULE;
-	i2c->adapter.dev.parent = dev->dev;
-	i2c->mdev = mdev;
-	i2c_set_adapdata(&i2c->adapter, i2c);
-	snprintf(i2c->adapter.name, sizeof(i2c->adapter.name), "mga i2c");
-
-	i2c->adapter.algo_data = &i2c->bit;
 
-	i2c->bit.udelay = 10;
-	i2c->bit.timeout = usecs_to_jiffies(2200);
-	i2c->bit.data = i2c;
-	i2c->bit.setsda		= mga_gpio_setsda;
-	i2c->bit.setscl		= mga_gpio_setscl;
-	i2c->bit.getsda		= mga_gpio_getsda;
-	i2c->bit.getscl		= mga_gpio_getscl;
-
-	ret = i2c_bit_add_bus(&i2c->adapter);
+	bit = &i2c->bit;
+	bit->data = i2c;
+	bit->setsda = mga_gpio_setsda;
+	bit->setscl = mga_gpio_setscl;
+	bit->getsda = mga_gpio_getsda;
+	bit->getscl = mga_gpio_getscl;
+	bit->udelay = 10;
+	bit->timeout = usecs_to_jiffies(2200);
+
+	adapter = &i2c->adapter;
+	adapter->owner = THIS_MODULE;
+	adapter->algo_data = bit;
+	adapter->dev.parent = dev->dev;
+	snprintf(adapter->name, sizeof(adapter->name), "mga i2c");
+	i2c_set_adapdata(adapter, i2c);
+
+	ret = i2c_bit_add_bus(adapter);
 	if (ret)
-		return ret;
-
-	return drmm_add_action_or_reset(dev, mgag200_i2c_release, i2c);
-}
-
-struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
-{
-	struct mga_i2c_chan *i2c;
-	struct drm_device *dev = &mdev->base;
-	int ret;
-
-	i2c = drmm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
-	if (!i2c)
-		return ERR_PTR(-ENOMEM);
+		return ERR_PTR(ret);
 
-	ret = mgag200_i2c_init(mdev, i2c);
+	ret = drmm_add_action_or_reset(dev, mgag200_i2c_release, i2c);
 	if (ret)
 		return ERR_PTR(ret);
 
-	return &i2c->adapter;
+	return adapter;
 }
-- 
2.45.0

[PATCH 06/10] drm/mgag200: Replace struct mga_i2c_chan with struct mgag200_ddc

[Thomas Zimmermann]

Rename struct mga_i2c_chan to struct mgag200_ddc, define it in the
source file mgag200_i2c.c, and reorder its fields. Rename all related
variables from i2c to ddc. Also rename the i2c adapter accordingly.

Using the term 'ddc' documents the purpose of the code clearly. The
old term 'i2c' could refer to any functionality on an i2c bus. No
functional changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h | 10 -----
 drivers/gpu/drm/mgag200/mgag200_i2c.c | 56 ++++++++++++++++-----------
 2 files changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 3c834bfd82cf4..008fdd5af09c8 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -10,9 +10,6 @@
 #ifndef __MGAG200_DRV_H__
 #define __MGAG200_DRV_H__
 
-#include <linux/i2c-algo-bit.h>
-#include <linux/i2c.h>
-
 #include <video/vga.h>
 
 #include <drm/drm_connector.h>
@@ -189,13 +186,6 @@ static inline struct mgag200_crtc_state *to_mgag200_crtc_state(struct drm_crtc_s
 	return container_of(base, struct mgag200_crtc_state, base);
 }
 
-struct mga_i2c_chan {
-	struct i2c_adapter adapter;
-	struct mga_device *mdev;
-	struct i2c_algo_bit_data bit;
-	int data, clock;
-};
-
 enum mga_type {
 	G200_PCI,
 	G200_AGP,
diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_i2c.c
index ba7aeca55fb40..73ff94c91ca36 100644
--- a/drivers/gpu/drm/mgag200/mgag200_i2c.c
+++ b/drivers/gpu/drm/mgag200/mgag200_i2c.c
@@ -36,6 +36,16 @@
 #include "mgag200_ddc.h"
 #include "mgag200_drv.h"
 
+struct mgag200_ddc {
+	struct mga_device *mdev;
+
+	int data;
+	int clock;
+
+	struct i2c_algo_bit_data bit;
+	struct i2c_adapter adapter;
+};
+
 static int mga_i2c_read_gpio(struct mga_device *mdev)
 {
 	WREG8(DAC_INDEX, MGA1064_GEN_IO_DATA);
@@ -63,62 +73,62 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
 
 static void mga_gpio_setsda(void *data, int state)
 {
-	struct mga_i2c_chan *i2c = data;
+	struct mgag200_ddc *ddc = data;
 
-	mga_i2c_set(i2c->mdev, i2c->data, state);
+	mga_i2c_set(ddc->mdev, ddc->data, state);
 }
 
 static void mga_gpio_setscl(void *data, int state)
 {
-	struct mga_i2c_chan *i2c = data;
+	struct mgag200_ddc *ddc = data;
 
-	mga_i2c_set(i2c->mdev, i2c->clock, state);
+	mga_i2c_set(ddc->mdev, ddc->clock, state);
 }
 
 static int mga_gpio_getsda(void *data)
 {
-	struct mga_i2c_chan *i2c = data;
+	struct mgag200_ddc *ddc = data;
 
-	return (mga_i2c_read_gpio(i2c->mdev) & i2c->data) ? 1 : 0;
+	return (mga_i2c_read_gpio(ddc->mdev) & ddc->data) ? 1 : 0;
 }
 
 static int mga_gpio_getscl(void *data)
 {
-	struct mga_i2c_chan *i2c = data;
+	struct mgag200_ddc *ddc = data;
 
-	return (mga_i2c_read_gpio(i2c->mdev) & i2c->clock) ? 1 : 0;
+	return (mga_i2c_read_gpio(ddc->mdev) & ddc->clock) ? 1 : 0;
 }
 
-static void mgag200_i2c_release(struct drm_device *dev, void *res)
+static void mgag200_ddc_release(struct drm_device *dev, void *res)
 {
-	struct mga_i2c_chan *i2c = res;
+	struct mgag200_ddc *ddc = res;
 
-	i2c_del_adapter(&i2c->adapter);
+	i2c_del_adapter(&ddc->adapter);
 }
 
 struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
 {
 	struct drm_device *dev = &mdev->base;
 	const struct mgag200_device_info *info = mdev->info;
-	struct mga_i2c_chan *i2c;
+	struct mgag200_ddc *ddc;
 	struct i2c_algo_bit_data *bit;
 	struct i2c_adapter *adapter;
 	int ret;
 
-	i2c = drmm_kzalloc(dev, sizeof(*i2c), GFP_KERNEL);
-	if (!i2c)
+	ddc = drmm_kzalloc(dev, sizeof(*ddc), GFP_KERNEL);
+	if (!ddc)
 		return ERR_PTR(-ENOMEM);
 
 	WREG_DAC(MGA1064_GEN_IO_CTL2, 1);
 	WREG_DAC(MGA1064_GEN_IO_DATA, 0xff);
 	WREG_DAC(MGA1064_GEN_IO_CTL, 0);
 
-	i2c->mdev = mdev;
-	i2c->data = BIT(info->i2c.data_bit);
-	i2c->clock = BIT(info->i2c.clock_bit);
+	ddc->mdev = mdev;
+	ddc->data = BIT(info->i2c.data_bit);
+	ddc->clock = BIT(info->i2c.clock_bit);
 
-	bit = &i2c->bit;
-	bit->data = i2c;
+	bit = &ddc->bit;
+	bit->data = ddc;
 	bit->setsda = mga_gpio_setsda;
 	bit->setscl = mga_gpio_setscl;
 	bit->getsda = mga_gpio_getsda;
@@ -126,18 +136,18 @@ struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
 	bit->udelay = 10;
 	bit->timeout = usecs_to_jiffies(2200);
 
-	adapter = &i2c->adapter;
+	adapter = &ddc->adapter;
 	adapter->owner = THIS_MODULE;
 	adapter->algo_data = bit;
 	adapter->dev.parent = dev->dev;
-	snprintf(adapter->name, sizeof(adapter->name), "mga i2c");
-	i2c_set_adapdata(adapter, i2c);
+	snprintf(adapter->name, sizeof(adapter->name), "Matrox DDC bus");
+	i2c_set_adapdata(adapter, ddc);
 
 	ret = i2c_bit_add_bus(adapter);
 	if (ret)
 		return ERR_PTR(ret);
 
-	ret = drmm_add_action_or_reset(dev, mgag200_i2c_release, i2c);
+	ret = drmm_add_action_or_reset(dev, mgag200_ddc_release, ddc);
 	if (ret)
 		return ERR_PTR(ret);
 
-- 
2.45.0

[PATCH 07/10] drm/mgag200: Rename mgag200_i2c.c to mgag200_ddc.c

[Thomas Zimmermann]

Rename the source file according to its content. No functional
changes.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/Makefile                         | 2 +-
 drivers/gpu/drm/mgag200/{mgag200_i2c.c => mgag200_ddc.c} | 0
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename drivers/gpu/drm/mgag200/{mgag200_i2c.c => mgag200_ddc.c} (100%)

diff --git a/drivers/gpu/drm/mgag200/Makefile b/drivers/gpu/drm/mgag200/Makefile
index 182e224c460dd..0b919352046eb 100644
--- a/drivers/gpu/drm/mgag200/Makefile
+++ b/drivers/gpu/drm/mgag200/Makefile
@@ -1,6 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0-only
 mgag200-y := \
 	mgag200_bmc.o \
+	mgag200_ddc.o \
 	mgag200_drv.o \
 	mgag200_g200.o \
 	mgag200_g200eh.o \
@@ -10,7 +11,6 @@ mgag200-y := \
 	mgag200_g200ew3.o \
 	mgag200_g200se.o \
 	mgag200_g200wb.o \
-	mgag200_i2c.o \
 	mgag200_mode.o
 
 obj-$(CONFIG_DRM_MGAG200) += mgag200.o
diff --git a/drivers/gpu/drm/mgag200/mgag200_i2c.c b/drivers/gpu/drm/mgag200/mgag200_ddc.c
similarity index 100%
rename from drivers/gpu/drm/mgag200/mgag200_i2c.c
rename to drivers/gpu/drm/mgag200/mgag200_ddc.c
-- 
2.45.0

[PATCH 08/10] drm/mgag200: Rename struct i2c_algo_bit_data callbacks

[Thomas Zimmermann]

Align the names of the algo-bit helpers with mgag200's convention of
using an mgag200 prefix plus the struct's name plus the callback's name
for such function symbols.

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

diff --git a/drivers/gpu/drm/mgag200/mgag200_ddc.c b/drivers/gpu/drm/mgag200/mgag200_ddc.c
index 73ff94c91ca36..3fa11b190943e 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ddc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ddc.c
@@ -71,28 +71,28 @@ static inline void mga_i2c_set(struct mga_device *mdev, int mask, int state)
 	mga_i2c_set_gpio(mdev, ~mask, state);
 }
 
-static void mga_gpio_setsda(void *data, int state)
+static void mgag200_ddc_algo_bit_data_setsda(void *data, int state)
 {
 	struct mgag200_ddc *ddc = data;
 
 	mga_i2c_set(ddc->mdev, ddc->data, state);
 }
 
-static void mga_gpio_setscl(void *data, int state)
+static void mgag200_ddc_algo_bit_data_setscl(void *data, int state)
 {
 	struct mgag200_ddc *ddc = data;
 
 	mga_i2c_set(ddc->mdev, ddc->clock, state);
 }
 
-static int mga_gpio_getsda(void *data)
+static int mgag200_ddc_algo_bit_data_getsda(void *data)
 {
 	struct mgag200_ddc *ddc = data;
 
 	return (mga_i2c_read_gpio(ddc->mdev) & ddc->data) ? 1 : 0;
 }
 
-static int mga_gpio_getscl(void *data)
+static int mgag200_ddc_algo_bit_data_getscl(void *data)
 {
 	struct mgag200_ddc *ddc = data;
 
@@ -129,10 +129,10 @@ struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
 
 	bit = &ddc->bit;
 	bit->data = ddc;
-	bit->setsda = mga_gpio_setsda;
-	bit->setscl = mga_gpio_setscl;
-	bit->getsda = mga_gpio_getsda;
-	bit->getscl = mga_gpio_getscl;
+	bit->setsda = mgag200_ddc_algo_bit_data_setsda;
+	bit->setscl = mgag200_ddc_algo_bit_data_setscl;
+	bit->getsda = mgag200_ddc_algo_bit_data_getsda;
+	bit->getscl = mgag200_ddc_algo_bit_data_getscl;
 	bit->udelay = 10;
 	bit->timeout = usecs_to_jiffies(2200);
 
-- 
2.45.0

[PATCH 09/10] drm/mgag200: 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/mgag200/mgag200_ddc.c  | 24 ++++++++++++++++++++++++
 drivers/gpu/drm/mgag200/mgag200_mode.c |  9 ---------
 2 files changed, 24 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_ddc.c b/drivers/gpu/drm/mgag200/mgag200_ddc.c
index 3fa11b190943e..6d81ea8931e88 100644
--- a/drivers/gpu/drm/mgag200/mgag200_ddc.c
+++ b/drivers/gpu/drm/mgag200/mgag200_ddc.c
@@ -99,6 +99,28 @@ static int mgag200_ddc_algo_bit_data_getscl(void *data)
 	return (mga_i2c_read_gpio(ddc->mdev) & ddc->clock) ? 1 : 0;
 }
 
+static int mgag200_ddc_algo_bit_data_pre_xfer(struct i2c_adapter *adapter)
+{
+	struct mgag200_ddc *ddc = i2c_get_adapdata(adapter);
+	struct mga_device *mdev = ddc->mdev;
+
+	/*
+	 * Protect access to I/O registers from concurrent modesetting
+	 * by acquiring the I/O-register lock.
+	 */
+	mutex_lock(&mdev->rmmio_lock);
+
+	return 0;
+}
+
+static void mgag200_ddc_algo_bit_data_post_xfer(struct i2c_adapter *adapter)
+{
+	struct mgag200_ddc *ddc = i2c_get_adapdata(adapter);
+	struct mga_device *mdev = ddc->mdev;
+
+	mutex_unlock(&mdev->rmmio_lock);
+}
+
 static void mgag200_ddc_release(struct drm_device *dev, void *res)
 {
 	struct mgag200_ddc *ddc = res;
@@ -133,6 +155,8 @@ struct i2c_adapter *mgag200_ddc_create(struct mga_device *mdev)
 	bit->setscl = mgag200_ddc_algo_bit_data_setscl;
 	bit->getsda = mgag200_ddc_algo_bit_data_getsda;
 	bit->getscl = mgag200_ddc_algo_bit_data_getscl;
+	bit->pre_xfer = mgag200_ddc_algo_bit_data_pre_xfer;
+	bit->post_xfer = mgag200_ddc_algo_bit_data_post_xfer;
 	bit->udelay = 10;
 	bit->timeout = usecs_to_jiffies(2200);
 
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index cd1f48b2f9986..a04c2b550be02 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -743,23 +743,14 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st
 
 int mgag200_vga_connector_helper_get_modes(struct drm_connector *connector)
 {
-	struct mga_device *mdev = to_mga_device(connector->dev);
 	const struct drm_edid *drm_edid;
 	int count;
 
-	/*
-	 * Protect access to I/O registers from concurrent modesetting
-	 * by acquiring the I/O-register lock.
-	 */
-	mutex_lock(&mdev->rmmio_lock);
-
 	drm_edid = drm_edid_read(connector);
 	drm_edid_connector_update(connector, drm_edid);
 	count = drm_edid_connector_add_modes(connector);
 	drm_edid_free(drm_edid);
 
-	mutex_unlock(&mdev->rmmio_lock);
-
 	return count;
 }
 
-- 
2.45.0

[PATCH 10/10] drm/mgag200: Use drm_connector_helper_get_modes()

[Thomas Zimmermann]

Mgag200's .get_modes() function is identical to the common helper.
Use the latter.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/mgag200/mgag200_drv.h  |  4 +---
 drivers/gpu/drm/mgag200/mgag200_mode.c | 17 -----------------
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/mgag200/mgag200_drv.h b/drivers/gpu/drm/mgag200/mgag200_drv.h
index 008fdd5af09c8..20e3710e056b3 100644
--- a/drivers/gpu/drm/mgag200/mgag200_drv.h
+++ b/drivers/gpu/drm/mgag200/mgag200_drv.h
@@ -420,10 +420,8 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st
 #define MGAG200_DAC_ENCODER_FUNCS \
 	.destroy = drm_encoder_cleanup
 
-int mgag200_vga_connector_helper_get_modes(struct drm_connector *connector);
-
 #define MGAG200_VGA_CONNECTOR_HELPER_FUNCS \
-	.get_modes  = mgag200_vga_connector_helper_get_modes
+	.get_modes = drm_connector_helper_get_modes
 
 #define MGAG200_VGA_CONNECTOR_FUNCS \
 	.reset                  = drm_atomic_helper_connector_reset, \
diff --git a/drivers/gpu/drm/mgag200/mgag200_mode.c b/drivers/gpu/drm/mgag200/mgag200_mode.c
index a04c2b550be02..d566e8476bf81 100644
--- a/drivers/gpu/drm/mgag200/mgag200_mode.c
+++ b/drivers/gpu/drm/mgag200/mgag200_mode.c
@@ -737,23 +737,6 @@ void mgag200_crtc_atomic_destroy_state(struct drm_crtc *crtc, struct drm_crtc_st
 	kfree(mgag200_crtc_state);
 }
 
-/*
- * Connector
- */
-
-int mgag200_vga_connector_helper_get_modes(struct drm_connector *connector)
-{
-	const struct drm_edid *drm_edid;
-	int count;
-
-	drm_edid = drm_edid_read(connector);
-	drm_edid_connector_update(connector, drm_edid);
-	count = drm_edid_connector_add_modes(connector);
-	drm_edid_free(drm_edid);
-
-	return count;
-}
-
 /*
  * Mode config
  */
-- 
2.45.0

Re: [PATCH 00/10] drm/mgag200: Refactor DDC code

[Jocelyn Falempe]

Thanks for this refactor of mgag200.

for the whole series:

Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>

-- 

Jocelyn

On 13/05/2024 14:51, Thomas Zimmermann wrote:
> Clean up a the driver's DDC code, make it simpler, more robust, and
> mostly self contained. The patches in this patchset have previously
> been sent as part of rev 1 of [1].
> 
> Patches 1 and 2 fix long-standing problems in the DDC code.
> 
> Patches 3 to 9 refactor the DDC code. The code then keeps its data
> structures internal, acquires locks automatically and is much more
> readable overall.
> 
> Patch 10 replaces driver code with an equivalent helper.
> 
> Tested on various Matrox hardware.
> 
> [1] https://patchwork.freedesktop.org/series/131977/
> 
> Thomas Zimmermann (10):
>    drm/mgag200: Set DDC timeout in milliseconds
>    drm/mgag200: Bind I2C lifetime to DRM device
>    drm/mgag200: Store pointer to struct mga_device in struct mga_i2c_chan
>    drm/mgag200: Allocate instance of struct mga_i2c_chan dynamically
>    drm/mgag200: Inline mgag200_i2c_init()
>    drm/mgag200: Replace struct mga_i2c_chan with struct mgag200_ddc
>    drm/mgag200: Rename mgag200_i2c.c to mgag200_ddc.c
>    drm/mgag200: Rename struct i2c_algo_bit_data callbacks
>    drm/mgag200: Acquire I/O-register lock in DDC code
>    drm/mgag200: Use drm_connector_helper_get_modes()
> 
>   drivers/gpu/drm/mgag200/Makefile          |   2 +-
>   drivers/gpu/drm/mgag200/mgag200_ddc.c     | 179 ++++++++++++++++++++++
>   drivers/gpu/drm/mgag200/mgag200_ddc.h     |  11 ++
>   drivers/gpu/drm/mgag200/mgag200_drv.h     |  18 +--
>   drivers/gpu/drm/mgag200/mgag200_g200.c    |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200eh.c  |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200eh3.c |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200er.c  |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ev.c  |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200ew3.c |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200se.c  |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_g200wb.c  |  11 +-
>   drivers/gpu/drm/mgag200/mgag200_i2c.c     | 129 ----------------
>   drivers/gpu/drm/mgag200/mgag200_mode.c    |  27 +---
>   14 files changed, 241 insertions(+), 213 deletions(-)
>   create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.c
>   create mode 100644 drivers/gpu/drm/mgag200/mgag200_ddc.h
>   delete mode 100644 drivers/gpu/drm/mgag200/mgag200_i2c.c
> 

Re: [PATCH 00/10] drm/mgag200: Refactor DDC code

[Thomas Zimmermann]

Hi

Am 16.05.24 um 11:14 schrieb Jocelyn Falempe:
> Thanks for this refactor of mgag200.
>
> for the whole series:
>
> Reviewed-by: Jocelyn Falempe <jfalempe@redhat.com>
>

Thank you so much!

Best regards
Thomas


-- 
--
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)