diff mbox

[RESEND,v2,1/3] drm: dw_hdmi: use of_get_i2c_adapter_by_node interface

Message ID 20160818143225.GY1041@n2100.armlinux.org.uk (mailing list archive)
State New, archived
Headers show

Commit Message

Russell King (Oracle) Aug. 18, 2016, 2:32 p.m. UTC
On Tue, Aug 16, 2016 at 11:26:43PM +0300, Vladimir Zapolskiy wrote:
> This change is needed to properly lock I2C bus driver, which serves
> DDC.
> 
> The change fixes an overflow over zero of I2C bus driver user counter:
> 
>   root@imx6q:~# lsmod
>   Not tainted
>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>   dw_hdmi_imx 3498 0 - Live 0xbf00d000
>   dw_hdmi 16398 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000
>   i2c_imx 16687 0 - Live 0xbf017000
> 
>   root@imx6q:~# rmmod dw_hdmi_imx
>   root@imx6q:~# lsmod
>   Not tainted
>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>   dw_hdmi 16398 1 dw_hdmi_ahb_audio, Live 0xbf004000
>   i2c_imx 16687 -1 - Live 0xbf017000
>                 ^^
> 
>   root@imx6q:~# rmmod i2c_imx
>   rmmod: ERROR: Module i2c_imx is in use
> 
> Note that prior to this change put_device() coupled with
> of_find_i2c_adapter_by_node() was missing on error path of
> dw_hdmi_bind(), added i2c_put_adapter() there along with the change.

I *guess* this is the right thing, but it would be nice to see the
results with the patch applied in the commit description.  Nevertheless:

Acked-by: Russell King <rmk+kernel@armlinux.org.uk>


I'd also like to see the DDC bits extracted from the various imx
drivers, because this is surely common code - I've had this floating
around for a few years but never got around to finishing it off and
submitting it.  If folk think this is a good idea, and want to take
the idea on, that's fine by me.

 drivers/gpu/drm/Makefile            |  2 +
 drivers/gpu/drm/bridge/dw-hdmi.c    | 98 +++++++++----------------------------
 drivers/gpu/drm/drm_ddc_connector.c | 91 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/imx/imx-tve.c       | 59 ++++++----------------
 include/drm/drm_ddc_connector.h     | 33 +++++++++++++
 5 files changed, 163 insertions(+), 120 deletions(-)

Comments

Vladimir Zapolskiy Aug. 23, 2016, 10:34 a.m. UTC | #1
Hello Russell,

On 08/18/2016 05:32 PM, Russell King - ARM Linux wrote:
> On Tue, Aug 16, 2016 at 11:26:43PM +0300, Vladimir Zapolskiy wrote:
>> This change is needed to properly lock I2C bus driver, which serves
>> DDC.
>>
>> The change fixes an overflow over zero of I2C bus driver user counter:
>>
>>   root@imx6q:~# lsmod
>>   Not tainted
>>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>>   dw_hdmi_imx 3498 0 - Live 0xbf00d000
>>   dw_hdmi 16398 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000
>>   i2c_imx 16687 0 - Live 0xbf017000
>>
>>   root@imx6q:~# rmmod dw_hdmi_imx
>>   root@imx6q:~# lsmod
>>   Not tainted
>>   dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
>>   dw_hdmi 16398 1 dw_hdmi_ahb_audio, Live 0xbf004000
>>   i2c_imx 16687 -1 - Live 0xbf017000
>>                 ^^
>>
>>   root@imx6q:~# rmmod i2c_imx
>>   rmmod: ERROR: Module i2c_imx is in use
>>
>> Note that prior to this change put_device() coupled with
>> of_find_i2c_adapter_by_node() was missing on error path of
>> dw_hdmi_bind(), added i2c_put_adapter() there along with the change.
>
> I *guess* this is the right thing, but it would be nice to see the
> results with the patch applied in the commit description.  Nevertheless:
>
> Acked-by: Russell King <rmk+kernel@armlinux.org.uk>
>

thank you for review.

For information this is a result after applying the fix (+1 to i2c_imx users):

root@imx6q# lsmod
     Not tainted
dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
i2c_imx 16687 1 - Live 0xbf011000
dw_hdmi_imx 3498 0 - Live 0xbf00d000
dw_hdmi 18925 2 dw_hdmi_ahb_audio,dw_hdmi_imx, Live 0xbf004000

root@imx6q:~# rmmod dw_hdmi_imx

root@imx6q:~# lsmod
     Not tainted
dw_hdmi_ahb_audio 4082 0 - Live 0xbf02c000
i2c_imx 16687 0 - Live 0xbf011000
dw_hdmi 18925 1 dw_hdmi_ahb_audio, Live 0xbf004000

No weird negative use count.

I have another pending change against DW HDMI, to avoid git-am conflicts
I'll rebase it on top of this one and resend today later on.

>
> I'd also like to see the DDC bits extracted from the various imx
> drivers, because this is surely common code - I've had this floating
> around for a few years but never got around to finishing it off and
> submitting it.  If folk think this is a good idea, and want to take
> the idea on, that's fine by me.
>
>  drivers/gpu/drm/Makefile            |  2 +
>  drivers/gpu/drm/bridge/dw-hdmi.c    | 98 +++++++++----------------------------
>  drivers/gpu/drm/drm_ddc_connector.c | 91 ++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/imx/imx-tve.c       | 59 ++++++----------------
>  include/drm/drm_ddc_connector.h     | 33 +++++++++++++
>  5 files changed, 163 insertions(+), 120 deletions(-)

I've briefly reviewed the changes and in my opinion this is a good
to have generalization of DDC connector, hopefully DRM people agree.

Moreover I assume that in case of getting modes over I2C/DDC most of
the .get_modes callbacks share almost the same code sequence:

   drm_get_edid()
   drm_detect_hdmi_monitor()
   drm_mode_connector_update_edid_property()
   drm_add_edid_modes()
   drm_edid_to_eld()

I'm not absolutely sure, but probably this can be generalized as well.

--
With best wishes,
Vladimir
diff mbox

Patch

diff --git a/drivers/gpu/drm/Makefile b/drivers/gpu/drm/Makefile
index 0238bf8bc8c3..e31c72f86f8c 100644
--- a/drivers/gpu/drm/Makefile
+++ b/drivers/gpu/drm/Makefile
@@ -21,6 +21,8 @@  drm-$(CONFIG_DRM_PANEL) += drm_panel.o
 drm-$(CONFIG_OF) += drm_of.o
 drm-$(CONFIG_AGP) += drm_agpsupport.o
 
+drm-y += drm_ddc_connector.o
+
 drm_kms_helper-y := drm_crtc_helper.o drm_dp_helper.o drm_probe_helper.o \
 		drm_plane_helper.o drm_dp_mst_topology.o drm_atomic_helper.o \
 		drm_kms_helper_common.o drm_dp_dual_mode_helper.o \
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c
index 65dd102689ef..786c0c076585 100644
--- a/drivers/gpu/drm/bridge/dw-hdmi.c
+++ b/drivers/gpu/drm/bridge/dw-hdmi.c
@@ -27,6 +27,7 @@ 
 #include <drm/drm_crtc_helper.h>
 #include <drm/drm_edid.h>
 #include <drm/drm_encoder_slave.h>
+#include <drm/drm_ddc_connector.h>
 #include <drm/bridge/dw_hdmi.h>
 
 #include "dw-hdmi.h"
@@ -103,7 +104,7 @@  struct hdmi_data_info {
 };
 
 struct dw_hdmi {
-	struct drm_connector connector;
+	struct drm_ddc_connector *ddc_conn;
 	struct drm_encoder *encoder;
 	struct drm_bridge *bridge;
 
@@ -124,10 +125,7 @@  struct dw_hdmi {
 	bool phy_enabled;
 	struct drm_display_mode previous_mode;
 
-	struct i2c_adapter *ddc;
 	void __iomem *regs;
-	bool sink_is_hdmi;
-	bool sink_has_audio;
 
 	struct mutex mutex;		/* for state below and previous_mode */
 	enum drm_connector_force force;	/* mutex-protected force state */
@@ -862,7 +860,7 @@  static int dw_hdmi_phy_init(struct dw_hdmi *hdmi)
 	bool cscon;
 
 	/*check csc whether needed activated in HDMI mode */
-	cscon = hdmi->sink_is_hdmi && is_color_space_conversion(hdmi);
+	cscon = hdmi->ddc_conn->sink_is_hdmi && is_color_space_conversion(hdmi);
 
 	/* HDMI Phy spec says to do the phy initialization sequence twice */
 	for (i = 0; i < 2; i++) {
@@ -1039,7 +1037,7 @@  static void hdmi_av_composer(struct dw_hdmi *hdmi,
 		HDMI_FC_INVIDCONF_IN_I_P_INTERLACED :
 		HDMI_FC_INVIDCONF_IN_I_P_PROGRESSIVE;
 
-	inv_val |= hdmi->sink_is_hdmi ?
+	inv_val |= hdmi->ddc_conn->sink_is_hdmi ?
 		HDMI_FC_INVIDCONF_DVI_MODEZ_HDMI_MODE :
 		HDMI_FC_INVIDCONF_DVI_MODEZ_DVI_MODE;
 
@@ -1238,7 +1236,7 @@  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	/* HDMI Initialization Step B.3 */
 	dw_hdmi_enable_video_path(hdmi);
 
-	if (hdmi->sink_has_audio) {
+	if (hdmi->ddc_conn->sink_has_audio) {
 		dev_dbg(hdmi->dev, "sink has audio support\n");
 
 		/* HDMI Initialization Step E - Configure audio */
@@ -1262,7 +1260,7 @@  static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	hdmi_tx_hdcp_config(hdmi);
 
 	dw_hdmi_clear_overflow(hdmi);
-	if (hdmi->cable_plugin && hdmi->sink_is_hdmi)
+	if (hdmi->cable_plugin && hdmi->ddc_conn->sink_is_hdmi)
 		hdmi_enable_overflow_interrupts(hdmi);
 
 	return 0;
@@ -1438,8 +1436,7 @@  static void dw_hdmi_bridge_enable(struct drm_bridge *bridge)
 static enum drm_connector_status
 dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 {
-	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
-					     connector);
+	struct dw_hdmi *hdmi = drm_ddc_private(connector);
 
 	mutex_lock(&hdmi->mutex);
 	hdmi->force = DRM_FORCE_UNSPECIFIED;
@@ -1451,43 +1448,11 @@  dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 		connector_status_connected : connector_status_disconnected;
 }
 
-static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
-{
-	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
-					     connector);
-	struct edid *edid;
-	int ret = 0;
-
-	if (!hdmi->ddc)
-		return 0;
-
-	edid = drm_get_edid(connector, hdmi->ddc);
-	if (edid) {
-		dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
-			edid->width_cm, edid->height_cm);
-
-		hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
-		hdmi->sink_has_audio = drm_detect_monitor_audio(edid);
-		drm_mode_connector_update_edid_property(connector, edid);
-		hdmi_event_new_edid(hdmi->dev, edid, 0);
-		ret = drm_add_edid_modes(connector, edid);
-		/* Store the ELD */
-		drm_edid_to_eld(connector, edid);
-		hdmi_event_new_eld(hdmi->dev, connector->eld);
-		kfree(edid);
-	} else {
-		dev_dbg(hdmi->dev, "failed to get edid\n");
-	}
-
-	return ret;
-}
-
 static enum drm_mode_status
 dw_hdmi_connector_mode_valid(struct drm_connector *connector,
 			     struct drm_display_mode *mode)
 {
-	struct dw_hdmi *hdmi = container_of(connector,
-					   struct dw_hdmi, connector);
+	struct dw_hdmi *hdmi = drm_ddc_private(connector);
 	enum drm_mode_status mode_status = MODE_OK;
 
 	/* We don't support double-clocked modes */
@@ -1500,16 +1465,9 @@  dw_hdmi_connector_mode_valid(struct drm_connector *connector,
 	return mode_status;
 }
 
-static void dw_hdmi_connector_destroy(struct drm_connector *connector)
-{
-	drm_connector_unregister(connector);
-	drm_connector_cleanup(connector);
-}
-
 static void dw_hdmi_connector_force(struct drm_connector *connector)
 {
-	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
-					     connector);
+	struct dw_hdmi *hdmi = drm_ddc_private(connector);
 
 	mutex_lock(&hdmi->mutex);
 	hdmi->force = connector->force;
@@ -1522,7 +1480,7 @@  static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
 	.detect = dw_hdmi_connector_detect,
-	.destroy = dw_hdmi_connector_destroy,
+	.destroy = drm_ddc_connector_destroy,
 	.force = dw_hdmi_connector_force,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
@@ -1530,7 +1488,7 @@  static const struct drm_connector_funcs dw_hdmi_connector_funcs = {
 };
 
 static const struct drm_connector_helper_funcs dw_hdmi_connector_helper_funcs = {
-	.get_modes = dw_hdmi_connector_get_modes,
+	.get_modes = drm_ddc_connector_get_modes,
 	.mode_valid = dw_hdmi_connector_mode_valid,
 	.best_encoder = drm_atomic_helper_best_encoder,
 };
@@ -1669,16 +1627,16 @@  static int dw_hdmi_register(struct drm_device *drm, struct dw_hdmi *hdmi)
 	}
 
 	encoder->bridge = bridge;
-	hdmi->connector.polled = DRM_CONNECTOR_POLL_HPD;
+	hdmi->ddc_conn->connector.polled = DRM_CONNECTOR_POLL_HPD;
 
 	drm_connector_helper_add(&hdmi->connector,
 				 &dw_hdmi_connector_helper_funcs);
 
-	drm_connector_init(drm, &hdmi->connector,
-			   &dw_hdmi_connector_funcs,
-			   DRM_MODE_CONNECTOR_HDMIA);
+	drm_ddc_connector_init(drm, hdmi->ddc_conn,
+			       &dw_hdmi_connector_funcs,
+			       DRM_MODE_CONNECTOR_HDMIA);
 
-	drm_mode_connector_attach_encoder(&hdmi->connector, encoder);
+	drm_mode_connector_attach_encoder(&hdmi->ddc_conn->connector, encoder);
 
 	return 0;
 }
@@ -1691,7 +1649,6 @@  int dw_hdmi_bind(struct device *dev, struct device *master,
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
 	struct platform_device_info pdevinfo;
-	struct device_node *ddc_node;
 	struct dw_hdmi_audio_data audio;
 	struct dw_hdmi *hdmi;
 	int ret;
@@ -1701,7 +1658,11 @@  int dw_hdmi_bind(struct device *dev, struct device *master,
 	if (!hdmi)
 		return -ENOMEM;
 
-	hdmi->connector.interlace_allowed = 1;
+	hdmi->ddc_conn = drm_ddc_connector_create(drm, np, hdmi);
+	if (IS_ERR(hdmi->ddc_conn))
+		return PTR_ERR(hdmi->ddc_conn);
+
+	hdmi->ddc_conn->connector.interlace_allowed = 1;
 
 	hdmi->plat_data = plat_data;
 	hdmi->dev = dev;
@@ -1732,19 +1693,6 @@  int dw_hdmi_bind(struct device *dev, struct device *master,
 		return -EINVAL;
 	}
 
-	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
-	if (ddc_node) {
-		hdmi->ddc = of_find_i2c_adapter_by_node(ddc_node);
-		of_node_put(ddc_node);
-		if (!hdmi->ddc) {
-			dev_dbg(hdmi->dev, "failed to read ddc node\n");
-			return -EPROBE_DEFER;
-		}
-
-	} else {
-		dev_dbg(hdmi->dev, "no ddc property found\n");
-	}
-
 	hdmi->regs = devm_ioremap_resource(dev, iores);
 	if (IS_ERR(hdmi->regs))
 		return PTR_ERR(hdmi->regs);
@@ -1826,7 +1774,7 @@  int dw_hdmi_bind(struct device *dev, struct device *master,
 	audio.base = hdmi->regs;
 	audio.irq = irq;
 	audio.hdmi = hdmi;
-	audio.eld = hdmi->connector.eld;
+	audio.eld = hdmi->ddc_conn->connector.eld;
 
 	pdevinfo.data = &audio;
 	pdevinfo.size_data = sizeof(audio);
@@ -1865,12 +1813,10 @@  void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
 	/* Disable all interrupts */
 	hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
 
-	hdmi->connector.funcs->destroy(&hdmi->connector);
 	hdmi->encoder->funcs->destroy(hdmi->encoder);
 
 	clk_disable_unprepare(hdmi->iahb_clk);
 	clk_disable_unprepare(hdmi->isfr_clk);
-	i2c_put_adapter(hdmi->ddc);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
 
diff --git a/drivers/gpu/drm/drm_ddc_connector.c b/drivers/gpu/drm/drm_ddc_connector.c
new file mode 100644
index 000000000000..1476db8cd549
--- /dev/null
+++ b/drivers/gpu/drm/drm_ddc_connector.c
@@ -0,0 +1,89 @@ 
+#include <linux/i2c.h>
+#include <linux/module.h>
+#include <drm/drmP.h>
+#include <drm/drm_crtc_helper.h>
+#include <drm/drm_edid.h>
+#include <drm/drm_ddc_connector.h>
+
+enum drm_connector_status
+drm_ddc_connector_always_connected(struct drm_connector *connector, bool force)
+{
+	return connector_status_connected;
+}
+EXPORT_SYMBOL_GPL(drm_ddc_connector_always_connected);
+
+int drm_ddc_connector_get_modes(struct drm_connector *connector)
+{
+	struct drm_ddc_connector *ddc_conn = to_ddc_conn(connector);
+	struct edid *edid;
+	int ret = 0;
+
+	if (!ddc_conn->ddc)
+		return 0;
+
+	edid = drm_get_edid(connector, ddc_conn->ddc);
+	if (edid) {
+		ddc_conn->sink_is_hdmi = drm_detect_hdmi_monitor(edid);
+		ddc_conn->sink_has_audio = drm_detect_monitor_audio(edid);
+		drm_mode_connector_update_edid_property(connector, edid);
+		hdmi_event_new_edid(hdmi->dev, edid, 0);
+		ret = drm_add_edid_modes(connector, edid);
+		/* Store the ELD */
+		drm_edid_to_eld(connector, edid);
+		hdmi_event_new_eld(hdmi->dev, connector->eld);
+		kfree(edid);
+	}
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(drm_ddc_connector_get_modes);
+
+void drm_ddc_connector_destroy(struct drm_connector *connector)
+{
+	struct drm_ddc_connector *ddc_conn = to_ddc_conn(connector);
+
+	drm_connector_unregister(connector);
+	drm_connector_cleanup(connector);
+	if (ddc_conn->ddc)
+		i2c_put_adapter(ddc_conn->ddc);
+	kfree(ddc_conn);
+}
+EXPORT_SYMBOL_GPL(drm_ddc_connector_destroy);
+
+void drm_ddc_connector_init(struct drm_device *drm,
+	struct drm_ddc_connector *ddc_conn,
+	struct drm_connector_funcs *funcs, int connector_type)
+{
+	drm_connector_init(drm, &ddc_conn->connector, funcs, connector_type);
+}
+EXPORT_SYMBOL_GPL(drm_ddc_connector_init);
+
+struct drm_ddc_connector *drm_ddc_connector_create(struct drm_device *drm,
+	struct device_node *np, void *private)
+{
+	struct drm_ddc_connector *ddc_conn;
+	struct device_node *ddc_node;
+
+	ddc_conn = kzalloc(sizeof(*ddc_conn), GFP_KERNEL);
+	if (!ddc_conn)
+		return ERR_PTR(-ENOMEM);
+
+	ddc_conn->private = private;
+
+	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
+	if (ddc_node) {
+		ddc_conn->ddc = of_get_i2c_adapter_by_node(ddc_node);
+		of_node_put(ddc_node);
+		if (!ddc_conn->ddc) {
+			kfree(ddc_conn);
+			return ERR_PTR(-EPROBE_DEFER);
+		}
+	}
+
+	return ddc_conn;
+}
+EXPORT_SYMBOL_GPL(drm_ddc_connector_create);
+
+MODULE_AUTHOR("Russell King <rmk+kernel@arm.linux.org.uk>");
+MODULE_DESCRIPTION("Generic DRM DDC connector module");
+MODULE_LICENSE("GPL v2");
diff --git a/drivers/gpu/drm/imx/imx-tve.c b/drivers/gpu/drm/imx/imx-tve.c
index 5e875944ffa2..e1955e4cd90b 100644
--- a/drivers/gpu/drm/imx/imx-tve.c
+++ b/drivers/gpu/drm/imx/imx-tve.c
@@ -17,7 +17,6 @@ 
 #include <linux/clk-provider.h>
 #include <linux/component.h>
 #include <linux/module.h>
-#include <linux/i2c.h>
 #include <linux/regmap.h>
 #include <linux/regulator/consumer.h>
 #include <linux/spinlock.h>
@@ -26,6 +25,7 @@ 
 #include <drm/drm_atomic_helper.h>
 #include <drm/drm_fb_helper.h>
 #include <drm/drm_crtc_helper.h>
+#include <drm/drm_ddc_connector.h>
 #include <video/imx-ipu-v3.h>
 
 #include "imx-drm.h"
@@ -104,7 +104,7 @@  enum {
 };
 
 struct imx_tve {
-	struct drm_connector connector;
+	struct drm_ddc_connector *ddc_conn;
 	struct drm_encoder encoder;
 	struct device *dev;
 	spinlock_t lock;	/* register lock */
@@ -115,7 +115,6 @@  struct imx_tve {
 
 	struct regmap *regmap;
 	struct regulator *dac_reg;
-	struct i2c_adapter *ddc;
 	struct clk *clk;
 	struct clk *di_sel_clk;
 	struct clk_hw clk_hw_di;
@@ -227,31 +226,6 @@  static int tve_setup_vga(struct imx_tve *tve)
 				 TVE_TVDAC_TEST_MODE_MASK, 1);
 }
 
-static enum drm_connector_status imx_tve_connector_detect(
-				struct drm_connector *connector, bool force)
-{
-	return connector_status_connected;
-}
-
-static int imx_tve_connector_get_modes(struct drm_connector *connector)
-{
-	struct imx_tve *tve = con_to_tve(connector);
-	struct edid *edid;
-	int ret = 0;
-
-	if (!tve->ddc)
-		return 0;
-
-	edid = drm_get_edid(connector, tve->ddc);
-	if (edid) {
-		drm_mode_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		kfree(edid);
-	}
-
-	return ret;
-}
-
 static int imx_tve_connector_mode_valid(struct drm_connector *connector,
 					struct drm_display_mode *mode)
 {
@@ -277,7 +251,7 @@  static int imx_tve_connector_mode_valid(struct drm_connector *connector,
 static struct drm_encoder *imx_tve_connector_best_encoder(
 		struct drm_connector *connector)
 {
-	struct imx_tve *tve = con_to_tve(connector);
+	struct imx_tve *tve = drm_ddc_private(connector);
 
 	return &tve->encoder;
 }
@@ -352,15 +326,15 @@  static int imx_tve_atomic_check(struct drm_encoder *encoder,
 static const struct drm_connector_funcs imx_tve_connector_funcs = {
 	.dpms = drm_atomic_helper_connector_dpms,
 	.fill_modes = drm_helper_probe_single_connector_modes,
-	.detect = imx_tve_connector_detect,
-	.destroy = imx_drm_connector_destroy,
+	.detect = drm_ddc_connector_always_connected,
+	.destroy = drm_ddc_connector_destroy,
 	.reset = drm_atomic_helper_connector_reset,
 	.atomic_duplicate_state = drm_atomic_helper_connector_duplicate_state,
 	.atomic_destroy_state = drm_atomic_helper_connector_destroy_state,
 };
 
 static const struct drm_connector_helper_funcs imx_tve_connector_helper_funcs = {
-	.get_modes = imx_tve_connector_get_modes,
+	.get_modes = drm_ddc_connector_get_modes,
 	.best_encoder = imx_tve_connector_best_encoder,
 	.mode_valid = imx_tve_connector_mode_valid,
 };
@@ -499,12 +473,13 @@  static int imx_tve_register(struct drm_device *drm, struct imx_tve *tve)
 	drm_encoder_init(drm, &tve->encoder, &imx_tve_encoder_funcs,
 			 encoder_type, NULL);
 
-	drm_connector_helper_add(&tve->connector,
+	drm_connector_helper_add(&tve->ddc_conn->connector,
 			&imx_tve_connector_helper_funcs);
-	drm_connector_init(drm, &tve->connector, &imx_tve_connector_funcs,
-			   DRM_MODE_CONNECTOR_VGA);
+	drm_ddc_connector_init(drm, tve->ddc_conn, &imx_tve_connector_funcs,
+			       DRM_MODE_CONNECTOR_VGA);
 
-	drm_mode_connector_attach_encoder(&tve->connector, &tve->encoder);
+	drm_mode_connector_attach_encoder(&tve->ddc_conn->connector,
+					  &tve->encoder);
 
 	return 0;
 }
@@ -553,7 +528,6 @@  static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	struct platform_device *pdev = to_platform_device(dev);
 	struct drm_device *drm = data;
 	struct device_node *np = dev->of_node;
-	struct device_node *ddc_node;
 	struct imx_tve *tve;
 	struct resource *res;
 	void __iomem *base;
@@ -565,15 +539,13 @@  static int imx_tve_bind(struct device *dev, struct device *master, void *data)
 	if (!tve)
 		return -ENOMEM;
 
+	tve->ddc_conn = drm_ddc_connector_create(drm, np, tve);
+	if (IS_ERR(tve->ddc_conn))
+		return PTR_ERR(tve->ddc_conn);
+
 	tve->dev = dev;
 	spin_lock_init(&tve->lock);
 
-	ddc_node = of_parse_phandle(np, "ddc-i2c-bus", 0);
-	if (ddc_node) {
-		tve->ddc = of_find_i2c_adapter_by_node(ddc_node);
-		of_node_put(ddc_node);
-	}
-
 	tve->mode = of_get_tve_mode(np);
 	if (tve->mode != TVE_MODE_VGA) {
 		dev_err(dev, "only VGA mode supported, currently\n");
@@ -685,7 +657,6 @@  static void imx_tve_unbind(struct device *dev, struct device *master,
 {
 	struct imx_tve *tve = dev_get_drvdata(dev);
 
-	tve->connector.funcs->destroy(&tve->connector);
 	tve->encoder.funcs->destroy(&tve->encoder);
 
 	if (!IS_ERR(tve->dac_reg))
diff --git a/include/drm/drm_ddc_connector.h b/include/drm/drm_ddc_connector.h
new file mode 100644
index 000000000000..fb8cdf66d5fd
--- /dev/null
+++ b/include/drm/drm_ddc_connector.h
@@ -0,0 +1,33 @@ 
+#ifndef DRM_DDC_CONNECTOR_H
+#define DRM_DDC_CONNECTOR_H
+
+#include <drm/drm_crtc.h>
+
+struct drm_ddc_connector {
+	struct i2c_adapter *ddc;
+	struct drm_connector connector;
+	unsigned int sink_is_hdmi:1;
+	unsigned int sink_has_audio:1;
+	void *private;
+};
+
+#define to_ddc_conn(c) container_of(c, struct drm_ddc_connector, connector)
+
+enum drm_connector_status drm_ddc_connector_always_connected(
+	struct drm_connector *connector, bool force);
+int drm_ddc_connector_get_modes(struct drm_connector *connector);
+void drm_ddc_connector_init(struct drm_device *drm,
+	struct drm_ddc_connector *ddc_conn,
+	struct drm_connector_funcs *funcs, int connector_type);
+void drm_ddc_connector_destroy(struct drm_connector *connector);
+struct drm_ddc_connector *drm_ddc_connector_create(struct drm_device *drm,
+	struct device_node *np, void *private);
+
+static inline void *drm_ddc_private(struct drm_connector *connector)
+{
+	struct drm_ddc_connector *ddc_conn = to_ddc_conn(connector);
+
+	return ddc_conn->private;
+}
+
+#endif