[PATCH v2 0/2] drm: bridge/dw_hdmi: add I2C bus adapter support

[PATCH v2 0/2] drm: bridge/dw_hdmi: add I2C bus adapter support

[Vladimir Zapolskiy]

This change adds support of internal HDMI I2C master controller,
originally the controller has its own separate driver written from
scratch http://patchwork.ozlabs.org/patch/405100 but due to shared
register space and interrupt with HDMI driver, it makes sense to
merge the code of both drivers.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

To use/test the change "ddc-i2c-bus" DT property must be omitted and
pin settings must be updated accordingly, here is an example for
iMX6 SabreLite:

-----------8<-----------
diff --git a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
index 0b28a9d..22d4431 100644
--- a/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
+++ b/arch/arm/boot/dts/imx6qdl-sabrelite.dtsi
@@ -174,7 +174,6 @@
 };
 
 &hdmi {
-	ddc-i2c-bus = <&i2c2>;
+	pinctrl-names = "default";
+	pinctrl-0 = <&pinctrl_hdmi>;
 	status = "okay";
 };
 
@@ -193,13 +192,6 @@
 	};
 };
 
-&i2c2 {
-	clock-frequency = <100000>;
-	pinctrl-names = "default";
-	pinctrl-0 = <&pinctrl_i2c2>;
-	status = "okay";
-};
-
 &i2c3 {
 	clock-frequency = <100000>;
 	pinctrl-names = "default";
@@ -284,10 +276,10 @@
 			>;
 		};
 
-		pinctrl_i2c2: i2c2grp {
+		pinctrl_hdmi: hdmigrp {
 			fsl,pins = <
-				MX6QDL_PAD_KEY_COL3__I2C2_SCL		0x4001b8b1
-				MX6QDL_PAD_KEY_ROW3__I2C2_SDA		0x4001b8b1
+				MX6QDL_PAD_KEY_COL3__HDMI_TX_DDC_SCL	0x4001b8b1
+				MX6QDL_PAD_KEY_ROW3__HDMI_TX_DDC_SDA	0x4001b8b1
 			>;
 		};
 
-----------8<-----------

Changes since v1:
- fixed a devm_kfree() signature
- split completions for read and write operations

Vladimir Zapolskiy (2):
  drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name
  drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

 drivers/gpu/drm/bridge/dw_hdmi.c | 341 ++++++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/bridge/dw_hdmi.h |   8 +-
 2 files changed, 339 insertions(+), 10 deletions(-)

-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

I2CM_ADDRESS became a MESS, fix it, also change guarding define
to __DW_HDMI_H__ , since the driver is not IMX specific.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/gpu/drm/bridge/dw_hdmi.h | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
index 175dbc8..ee7f7ed 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.h
+++ b/drivers/gpu/drm/bridge/dw_hdmi.h
@@ -7,8 +7,8 @@
  * (at your option) any later version.
  */
 
-#ifndef __IMX_HDMI_H__
-#define __IMX_HDMI_H__
+#ifndef __DW_HDMI_H__
+#define __DW_HDMI_H__
 
 /* Identification Registers */
 #define HDMI_DESIGN_ID                          0x0000
@@ -525,7 +525,7 @@
 
 /* I2C Master Registers (E-DDC) */
 #define HDMI_I2CM_SLAVE                         0x7E00
-#define HDMI_I2CMESS                            0x7E01
+#define HDMI_I2CM_ADDRESS                       0x7E01
 #define HDMI_I2CM_DATAO                         0x7E02
 #define HDMI_I2CM_DATAI                         0x7E03
 #define HDMI_I2CM_OPERATION                     0x7E04
@@ -1031,4 +1031,4 @@ enum {
 	HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
 };
 
-#endif /* __IMX_HDMI_H__ */
+#endif /* __DW_HDMI_H__ */
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

The change adds support of internal HDMI I2C master controller, this
subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.

The main purpose of this functionality is to support reading EDID from
an HDMI monitor on boards, which don't have an I2C bus connected to
DDC pins.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes since v1:
- fixed a devm_kfree() signature
- split completions for read and write operations

 drivers/gpu/drm/bridge/dw_hdmi.c | 341 ++++++++++++++++++++++++++++++++++++++-
 1 file changed, 335 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 49cafb6..7f64e73 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -1,15 +1,17 @@
 /*
+ * DesignWare High-Definition Multimedia Interface (HDMI) driver
+ *
+ * Copyright (C) 2013-2015 Mentor Graphics Inc.
  * Copyright (C) 2011-2013 Freescale Semiconductor, Inc.
+ * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
  * the Free Software Foundation; either version 2 of the License, or
  * (at your option) any later version.
  *
- * Designware High-Definition Multimedia Interface (HDMI) driver
- *
- * Copyright (C) 2010, Guennadi Liakhovetski <g.liakhovetski@gmx.de>
  */
+
 #include <linux/module.h>
 #include <linux/irq.h>
 #include <linux/delay.h>
@@ -28,6 +30,26 @@
 
 #include "dw_hdmi.h"
 
+/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */
+#define HDMI_IH_I2CM_STAT0_ERROR		BIT(0)
+#define HDMI_IH_I2CM_STAT0_DONE			BIT(1)
+
+/* HDMI_I2CM_OPERATION register bits */
+#define HDMI_I2CM_OPERATION_READ		BIT(0)
+#define HDMI_I2CM_OPERATION_READ_EXT		BIT(1)
+#define HDMI_I2CM_OPERATION_WRITE		BIT(4)
+
+/* HDMI_I2CM_INT register bits */
+#define HDMI_I2CM_INT_DONE_MASK			BIT(2)
+#define HDMI_I2CM_INT_DONE_POL			BIT(3)
+
+/* HDMI_I2CM_CTLINT register bits */
+#define HDMI_I2CM_CTLINT_ARB_MASK		BIT(2)
+#define HDMI_I2CM_CTLINT_ARB_POL		BIT(3)
+#define HDMI_I2CM_CTLINT_NAC_MASK		BIT(6)
+#define HDMI_I2CM_CTLINT_NAC_POL		BIT(7)
+
+
 #define HDMI_EDID_LEN		512
 
 #define RGB			0
@@ -102,6 +124,19 @@ struct hdmi_data_info {
 	struct hdmi_vmode video_mode;
 };
 
+struct dw_hdmi_i2c {
+	struct i2c_adapter	adap;
+
+	spinlock_t		lock;
+	struct completion	cmp_r;
+	struct completion	cmp_w;
+	u8			stat;
+
+	u8			operation_reg;
+	u8			slave_reg;
+	bool			is_regaddr;
+};
+
 struct dw_hdmi {
 	struct drm_connector connector;
 	struct drm_encoder *encoder;
@@ -111,6 +146,7 @@ struct dw_hdmi {
 	struct device *dev;
 	struct clk *isfr_clk;
 	struct clk *iahb_clk;
+	struct dw_hdmi_i2c *i2c;
 
 	struct hdmi_data_info hdmi_data;
 	const struct dw_hdmi_plat_data *plat_data;
@@ -179,6 +215,249 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
 	hdmi_modb(hdmi, data << shift, mask, reg);
 }
 
+static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&hdmi->i2c->lock, flags);
+
+	/* Set Fast Mode speed */
+	hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
+
+	/* Software reset */
+	hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
+
+	/* Set done, not acknowledged and arbitration interrupt polarities */
+	hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
+	hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
+		    HDMI_I2CM_CTLINT);
+
+	/* Clear DONE and ERROR interrupts */
+	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+		    HDMI_IH_I2CM_STAT0);
+
+	/* Mute DONE and ERROR interrupts */
+	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+		    HDMI_IH_MUTE_I2CM_STAT0);
+
+	spin_unlock_irqrestore(&hdmi->i2c->lock, flags);
+}
+
+static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
+			    unsigned char *buf, int length)
+{
+	int stat;
+	unsigned long flags;
+	struct dw_hdmi_i2c *i2c = hdmi->i2c;
+
+	spin_lock_irqsave(&i2c->lock, flags);
+
+	i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
+
+	if (!i2c->is_regaddr) {
+		dev_dbg(hdmi->dev, "set read register address to 0\n");
+		i2c->slave_reg = 0x00;
+		i2c->is_regaddr = true;
+	}
+
+	while (length--) {
+		reinit_completion(&i2c->cmp_r);
+		i2c->stat = 0;
+
+		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
+		hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
+
+		spin_unlock_irqrestore(&i2c->lock, flags);
+
+		stat = wait_for_completion_interruptible_timeout(&i2c->cmp_r,
+								 HZ / 10);
+		if (!stat)
+			return -ETIMEDOUT;
+		if (stat < 0)
+			return stat;
+
+		spin_lock_irqsave(&i2c->lock, flags);
+
+		/* Check for error condition on the bus */
+		if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) {
+			spin_unlock_irqrestore(&i2c->lock, flags);
+			return -EIO;
+		}
+
+		*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
+	}
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	return 0;
+}
+
+static int dw_hdmi_i2c_write(struct dw_hdmi *hdmi,
+				  unsigned char *buf, int length)
+{
+	int stat;
+	unsigned long flags;
+	struct dw_hdmi_i2c *i2c = hdmi->i2c;
+
+	spin_lock_irqsave(&i2c->lock, flags);
+
+	i2c->operation_reg = HDMI_I2CM_OPERATION_WRITE;
+
+	if (!i2c->is_regaddr) {
+		if (length) {
+			/* Use the first write byte as register address */
+			i2c->slave_reg = buf[0];
+			length--;
+			buf++;
+		} else {
+			dev_dbg(hdmi->dev, "set write register address to 0\n");
+			i2c->slave_reg = 0x00;
+		}
+		i2c->is_regaddr = true;
+	}
+
+	while (length--) {
+		reinit_completion(&i2c->cmp_w);
+		i2c->stat = 0;
+
+		hdmi_writeb(hdmi, *buf++, HDMI_I2CM_DATAO);
+		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
+		hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
+
+		spin_unlock_irqrestore(&i2c->lock, flags);
+
+		stat = wait_for_completion_interruptible_timeout(&i2c->cmp_w,
+								 HZ / 10);
+		if (!stat)
+			return -ETIMEDOUT;
+		if (stat < 0)
+			return stat;
+
+		spin_lock_irqsave(&i2c->lock, flags);
+
+		/* Check for error condition on the bus */
+		if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) {
+			spin_unlock_irqrestore(&i2c->lock, flags);
+			return -EIO;
+		}
+	}
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	return 0;
+}
+
+static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
+			    struct i2c_msg *msgs, int num)
+{
+	struct dw_hdmi *hdmi = i2c_get_adapdata(adap);
+	struct dw_hdmi_i2c *i2c = hdmi->i2c;
+
+	int i, ret;
+	u8 addr;
+	unsigned long flags;
+
+	dev_dbg(hdmi->dev, "xfer: num: %d, addr: 0x%x\n", num, msgs[0].addr);
+
+	spin_lock_irqsave(&i2c->lock, flags);
+
+	hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0);
+
+	/* Set slave device address from the first transaction */
+	addr = msgs[0].addr;
+	hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE);
+
+	/* Set slave device register address on transfer */
+	i2c->is_regaddr = false;
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	for (i = 0; i < num; i++) {
+		dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: 0x%x\n",
+			i + 1, num, msgs[i].len, msgs[i].flags);
+
+		if (msgs[i].addr != addr) {
+			dev_warn(hdmi->dev,
+				 "unsupported transaction, changed slave address\n");
+			ret = -EOPNOTSUPP;
+			break;
+		}
+
+		if (msgs[i].len == 0) {
+			dev_dbg(hdmi->dev,
+				 "unsupported transaction %d/%d, no data\n",
+				 i + 1, num);
+			ret = -EOPNOTSUPP;
+			break;
+		}
+
+		if (msgs[i].flags & I2C_M_RD)
+			ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
+		else
+			ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
+
+		if (ret < 0)
+			break;
+	}
+
+	if (!ret)
+		ret = num;
+
+	spin_lock_irqsave(&i2c->lock, flags);
+
+	/* Mute DONE and ERROR interrupts */
+	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
+		    HDMI_IH_MUTE_I2CM_STAT0);
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	return ret;
+}
+
+static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter)
+{
+	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
+}
+
+static struct i2c_algorithm dw_hdmi_algorithm = {
+	.master_xfer	= dw_hdmi_i2c_xfer,
+	.functionality	= dw_hdmi_i2c_func,
+};
+
+static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi)
+{
+	struct i2c_adapter *adap;
+	int ret;
+
+	hdmi->i2c = devm_kzalloc(hdmi->dev, sizeof(*hdmi->i2c), GFP_KERNEL);
+	if (!hdmi->i2c)
+		return ERR_PTR(-ENOMEM);
+
+	spin_lock_init(&hdmi->i2c->lock);
+	init_completion(&hdmi->i2c->cmp_r);
+	init_completion(&hdmi->i2c->cmp_w);
+
+	adap = &hdmi->i2c->adap;
+	adap->class = I2C_CLASS_DDC;
+	adap->owner = THIS_MODULE;
+	adap->dev.parent = hdmi->dev;
+	adap->algo = &dw_hdmi_algorithm;
+	strlcpy(adap->name, "DesignWare HDMI", sizeof(adap->name));
+	i2c_set_adapdata(adap, hdmi);
+
+	ret = i2c_add_adapter(adap);
+	if (ret) {
+		dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name);
+		devm_kfree(hdmi->dev, hdmi->i2c);
+		hdmi->i2c = NULL;
+		return ERR_PTR(ret);
+	}
+
+	dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name);
+
+	return adap;
+}
+
 static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
 			   unsigned int n)
 {
@@ -1466,16 +1745,47 @@ struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
 	.mode_fixup = dw_hdmi_bridge_mode_fixup,
 };
 
+static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
+{
+	struct dw_hdmi_i2c *i2c = hdmi->i2c;
+	unsigned long flags;
+
+	spin_lock_irqsave(&i2c->lock, flags);
+
+	i2c->stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
+	if (!i2c->stat) {
+		spin_unlock_irqrestore(&i2c->lock, flags);
+		return IRQ_NONE;
+	}
+
+	hdmi_writeb(hdmi, i2c->stat, HDMI_IH_I2CM_STAT0);
+
+	if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)
+		complete(&i2c->cmp_r);
+	else
+		complete(&i2c->cmp_w);
+
+	spin_unlock_irqrestore(&i2c->lock, flags);
+
+	return IRQ_HANDLED;
+}
+
 static irqreturn_t dw_hdmi_hardirq(int irq, void *dev_id)
 {
 	struct dw_hdmi *hdmi = dev_id;
 	u8 intr_stat;
+	irqreturn_t ret = IRQ_NONE;
+
+	if (hdmi->i2c)
+		ret = dw_hdmi_i2c_irq(hdmi);
 
 	intr_stat = hdmi_readb(hdmi, HDMI_IH_PHY_STAT0);
-	if (intr_stat)
+	if (intr_stat) {
 		hdmi_writeb(hdmi, ~0, HDMI_IH_MUTE_PHY_STAT0);
+		return IRQ_WAKE_THREAD;
+	}
 
-	return intr_stat ? IRQ_WAKE_THREAD : IRQ_NONE;
+	return ret;
 }
 
 static irqreturn_t dw_hdmi_irq(int irq, void *dev_id)
@@ -1654,6 +1964,13 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 	 */
 	hdmi_init_clk_regenerator(hdmi);
 
+	/* If DDC bus is not specified, try to register HDMI I2C bus */
+	if (!hdmi->ddc) {
+		hdmi->ddc = dw_hdmi_i2c_adapter(hdmi);
+		if (IS_ERR(hdmi->ddc))
+			hdmi->ddc = NULL;
+	}
+
 	/*
 	 * Configure registers related to HDMI interrupt
 	 * generation before registering IRQ.
@@ -1674,11 +1991,18 @@ int dw_hdmi_bind(struct device *dev, struct device *master,
 	/* Unmute interrupts */
 	hdmi_writeb(hdmi, ~HDMI_IH_PHY_STAT0_HPD, HDMI_IH_MUTE_PHY_STAT0);
 
+	/* Unmute I2CM interrupts and reset HDMI DDC I2C master controller */
+	if (hdmi->i2c)
+		dw_hdmi_i2c_init(hdmi);
+
 	dev_set_drvdata(dev, hdmi);
 
 	return 0;
 
 err_iahb:
+	if (hdmi->i2c)
+		i2c_del_adapter(&hdmi->i2c->adap);
+
 	clk_disable_unprepare(hdmi->iahb_clk);
 err_isfr:
 	clk_disable_unprepare(hdmi->isfr_clk);
@@ -1699,13 +2023,18 @@ void dw_hdmi_unbind(struct device *dev, struct device *master, void *data)
 
 	clk_disable_unprepare(hdmi->iahb_clk);
 	clk_disable_unprepare(hdmi->isfr_clk);
-	i2c_put_adapter(hdmi->ddc);
+
+	if (hdmi->i2c)
+		i2c_del_adapter(&hdmi->i2c->adap);
+	else
+		i2c_put_adapter(hdmi->ddc);
 }
 EXPORT_SYMBOL_GPL(dw_hdmi_unbind);
 
 MODULE_AUTHOR("Sascha Hauer <s.hauer@pengutronix.de>");
 MODULE_AUTHOR("Andy Yan <andy.yan@rock-chips.com>");
 MODULE_AUTHOR("Yakir Yang <ykk@rock-chips.com>");
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
 MODULE_DESCRIPTION("DW HDMI transmitter driver");
 MODULE_LICENSE("GPL");
 MODULE_ALIAS("platform:dw-hdmi");
-- 
2.1.4

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Philipp Zabel]

Hi Vladimir,

Am Montag, den 18.05.2015, 15:32 +0300 schrieb Vladimir Zapolskiy:
> I2CM_ADDRESS became a MESS, fix it, also change guarding define
> to __DW_HDMI_H__ , since the driver is not IMX specific.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Philipp Zabel]

Hi Vladimir,

Am Montag, den 18.05.2015, 15:32 +0300 schrieb Vladimir Zapolskiy:
> I2CM_ADDRESS became a MESS, fix it, also change guarding define
> to __DW_HDMI_H__ , since the driver is not IMX specific.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Acked-by: Philipp Zabel <p.zabel@pengutronix.de>

regards
Philipp



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Philipp Zabel]

Hi Vladimir,

Am Montag, den 18.05.2015, 15:32 +0300 schrieb Vladimir Zapolskiy:
> The change adds support of internal HDMI I2C master controller, this
> subdevice is used by default, if "ddc-i2c-bus" DT property is omitted.
> 
> The main purpose of this functionality is to support reading EDID from
> an HDMI monitor on boards, which don't have an I2C bus connected to
> DDC pins.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Tested-by: Philipp Zabel <p.zabel@pengutronix.de>
on imx6q-nitrogen6x with the DDC pads muxed to HDMI_TX.

regards
Philipp



_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Andy Yan]

Hi Vladimir,
   Thanks for you patch.

On 2015年05月18日 20:32, Vladimir Zapolskiy wrote:
> I2CM_ADDRESS became a MESS, fix it, also change guarding define
> to __DW_HDMI_H__ , since the driver is not IMX specific.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
    Acked-by: Andy Yan <andy.yan@rock-chips.com>
> ---
>   drivers/gpu/drm/bridge/dw_hdmi.h | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
> index 175dbc8..ee7f7ed 100644
> --- a/drivers/gpu/drm/bridge/dw_hdmi.h
> +++ b/drivers/gpu/drm/bridge/dw_hdmi.h
> @@ -7,8 +7,8 @@
>    * (at your option) any later version.
>    */
>   
> -#ifndef __IMX_HDMI_H__
> -#define __IMX_HDMI_H__
> +#ifndef __DW_HDMI_H__
> +#define __DW_HDMI_H__
>   
>   /* Identification Registers */
>   #define HDMI_DESIGN_ID                          0x0000
> @@ -525,7 +525,7 @@
>   
>   /* I2C Master Registers (E-DDC) */
>   #define HDMI_I2CM_SLAVE                         0x7E00
> -#define HDMI_I2CMESS                            0x7E01
> +#define HDMI_I2CM_ADDRESS                       0x7E01
>   #define HDMI_I2CM_DATAO                         0x7E02
>   #define HDMI_I2CM_DATAI                         0x7E03
>   #define HDMI_I2CM_OPERATION                     0x7E04
> @@ -1031,4 +1031,4 @@ enum {
>   	HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
>   };
>   
> -#endif /* __IMX_HDMI_H__ */
> +#endif /* __DW_HDMI_H__ */


_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hi David, Philipp, Andy, Russell,

On 19.05.2015 17:39, Andy Yan wrote:
> Hi Vladimir,
>    Thanks for you patch.
> 
> On 2015年05月18日 20:32, Vladimir Zapolskiy wrote:
>> I2CM_ADDRESS became a MESS, fix it, also change guarding define
>> to __DW_HDMI_H__ , since the driver is not IMX specific.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>     Acked-by: Andy Yan <andy.yan@rock-chips.com>
>> ---
>>   drivers/gpu/drm/bridge/dw_hdmi.h | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
>> index 175dbc8..ee7f7ed 100644
>> --- a/drivers/gpu/drm/bridge/dw_hdmi.h
>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.h
>> @@ -7,8 +7,8 @@
>>    * (at your option) any later version.
>>    */
>>   
>> -#ifndef __IMX_HDMI_H__
>> -#define __IMX_HDMI_H__
>> +#ifndef __DW_HDMI_H__
>> +#define __DW_HDMI_H__
>>   
>>   /* Identification Registers */
>>   #define HDMI_DESIGN_ID                          0x0000
>> @@ -525,7 +525,7 @@
>>   
>>   /* I2C Master Registers (E-DDC) */
>>   #define HDMI_I2CM_SLAVE                         0x7E00
>> -#define HDMI_I2CMESS                            0x7E01
>> +#define HDMI_I2CM_ADDRESS                       0x7E01
>>   #define HDMI_I2CM_DATAO                         0x7E02
>>   #define HDMI_I2CM_DATAI                         0x7E03
>>   #define HDMI_I2CM_OPERATION                     0x7E04
>> @@ -1031,4 +1031,4 @@ enum {
>>   	HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
>>   };
>>   
>> -#endif /* __IMX_HDMI_H__ */
>> +#endif /* __DW_HDMI_H__ */
> 
> 

what would be the next action regarding these two patches? If review is
done, should they go to drm-dwhdmi-devel or drm-next ?

--
With best wishes,
Vladimir
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hello David,

On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
> Hi David, Philipp, Andy, Russell,
> 
> On 19.05.2015 17:39, Andy Yan wrote:
>> Hi Vladimir,
>>    Thanks for you patch.
>>
>> On 2015年05月18日 20:32, Vladimir Zapolskiy wrote:
>>> I2CM_ADDRESS became a MESS, fix it, also change guarding define
>>> to __DW_HDMI_H__ , since the driver is not IMX specific.
>>>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>     Acked-by: Andy Yan <andy.yan@rock-chips.com>
>>> ---
>>>   drivers/gpu/drm/bridge/dw_hdmi.h | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/dw_hdmi.h b/drivers/gpu/drm/bridge/dw_hdmi.h
>>> index 175dbc8..ee7f7ed 100644
>>> --- a/drivers/gpu/drm/bridge/dw_hdmi.h
>>> +++ b/drivers/gpu/drm/bridge/dw_hdmi.h
>>> @@ -7,8 +7,8 @@
>>>    * (at your option) any later version.
>>>    */
>>>   
>>> -#ifndef __IMX_HDMI_H__
>>> -#define __IMX_HDMI_H__
>>> +#ifndef __DW_HDMI_H__
>>> +#define __DW_HDMI_H__
>>>   
>>>   /* Identification Registers */
>>>   #define HDMI_DESIGN_ID                          0x0000
>>> @@ -525,7 +525,7 @@
>>>   
>>>   /* I2C Master Registers (E-DDC) */
>>>   #define HDMI_I2CM_SLAVE                         0x7E00
>>> -#define HDMI_I2CMESS                            0x7E01
>>> +#define HDMI_I2CM_ADDRESS                       0x7E01
>>>   #define HDMI_I2CM_DATAO                         0x7E02
>>>   #define HDMI_I2CM_DATAI                         0x7E03
>>>   #define HDMI_I2CM_OPERATION                     0x7E04
>>> @@ -1031,4 +1031,4 @@ enum {
>>>   	HDMI_A_VIDPOLCFG_HSYNCPOL_ACTIVE_LOW = 0x0,
>>>   };
>>>   
>>> -#endif /* __IMX_HDMI_H__ */
>>> +#endif /* __DW_HDMI_H__ */
>>
>>
> 
> what would be the next action regarding these two patches? If review is
> done, should they go to drm-dwhdmi-devel or drm-next ?

ping.

--
With best wishes,
Vladimir

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Russell King - ARM Linux]

On Fri, Jun 26, 2015 at 05:24:12PM +0300, Vladimir Zapolskiy wrote:
> Hello David,
> 
> On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
> > what would be the next action regarding these two patches? If review is
> > done, should they go to drm-dwhdmi-devel or drm-next ?
> 
> ping.

I don't think it impacts any builds at the moment, so we'll see about
merging it after the current merge window has finished.  Please remind
us after about a week and a half if we haven't already picked it up
by then.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hello Russell, David,

On 26.06.2015 18:02, Russell King - ARM Linux wrote:
> On Fri, Jun 26, 2015 at 05:24:12PM +0300, Vladimir Zapolskiy wrote:
>> Hello David,
>>
>> On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
>>> what would be the next action regarding these two patches? If review is
>>> done, should they go to drm-dwhdmi-devel or drm-next ?
>>
>> ping.
> 
> I don't think it impacts any builds at the moment, so we'll see about
> merging it after the current merge window has finished.  Please remind
> us after about a week and a half if we haven't already picked it up
> by then.
> 

this is a reminder, please review the patches.

--
With best wishes,
Vladimir

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/2] drm: bridge/dw_hdmi: fix register I2CM_ADDRESS register name

[Vladimir Zapolskiy]

Hello Russell, David,

On 24.07.2015 18:09, Vladimir Zapolskiy wrote:
> Hello Russell, David,
> 
> On 26.06.2015 18:02, Russell King - ARM Linux wrote:
>> On Fri, Jun 26, 2015 at 05:24:12PM +0300, Vladimir Zapolskiy wrote:
>>> Hello David,
>>>
>>> On 08.06.2015 17:17, Vladimir Zapolskiy wrote:
>>>> what would be the next action regarding these two patches? If review is
>>>> done, should they go to drm-dwhdmi-devel or drm-next ?
>>>
>>> ping.
>>
>> I don't think it impacts any builds at the moment, so we'll see about
>> merging it after the current merge window has finished.  Please remind
>> us after about a week and a half if we haven't already picked it up
>> by then.
>>
> 
> this is a reminder, please review the patches.

ping.

--
With best wishes,
Vladimir

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Russell King - ARM Linux]

On Mon, May 18, 2015 at 03:32:21PM +0300, Vladimir Zapolskiy wrote:
> +/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */
> +#define HDMI_IH_I2CM_STAT0_ERROR		BIT(0)
> +#define HDMI_IH_I2CM_STAT0_DONE			BIT(1)
> +
> +/* HDMI_I2CM_OPERATION register bits */
> +#define HDMI_I2CM_OPERATION_READ		BIT(0)
> +#define HDMI_I2CM_OPERATION_READ_EXT		BIT(1)
> +#define HDMI_I2CM_OPERATION_WRITE		BIT(4)
> +
> +/* HDMI_I2CM_INT register bits */
> +#define HDMI_I2CM_INT_DONE_MASK			BIT(2)
> +#define HDMI_I2CM_INT_DONE_POL			BIT(3)
> +
> +/* HDMI_I2CM_CTLINT register bits */
> +#define HDMI_I2CM_CTLINT_ARB_MASK		BIT(2)
> +#define HDMI_I2CM_CTLINT_ARB_POL		BIT(3)
> +#define HDMI_I2CM_CTLINT_NAC_MASK		BIT(6)
> +#define HDMI_I2CM_CTLINT_NAC_POL		BIT(7)

Please put these definitions in dw_hdmi.h

> +
> +
>  #define HDMI_EDID_LEN		512
>  
>  #define RGB			0
> @@ -102,6 +124,19 @@ struct hdmi_data_info {
>  	struct hdmi_vmode video_mode;
>  };
>  
> +struct dw_hdmi_i2c {
> +	struct i2c_adapter	adap;
> +
> +	spinlock_t		lock;
> +	struct completion	cmp_r;
> +	struct completion	cmp_w;
> +	u8			stat;
> +
> +	u8			operation_reg;
> +	u8			slave_reg;
> +	bool			is_regaddr;
> +};
> +
>  struct dw_hdmi {
>  	struct drm_connector connector;
>  	struct drm_encoder *encoder;
> @@ -111,6 +146,7 @@ struct dw_hdmi {
>  	struct device *dev;
>  	struct clk *isfr_clk;
>  	struct clk *iahb_clk;
> +	struct dw_hdmi_i2c *i2c;
>  
>  	struct hdmi_data_info hdmi_data;
>  	const struct dw_hdmi_plat_data *plat_data;
> @@ -179,6 +215,249 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
>  	hdmi_modb(hdmi, data << shift, mask, reg);
>  }
>  
> +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&hdmi->i2c->lock, flags);
> +
> +	/* Set Fast Mode speed */
> +	hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
> +
> +	/* Software reset */
> +	hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
> +
> +	/* Set done, not acknowledged and arbitration interrupt polarities */
> +	hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
> +	hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
> +		    HDMI_I2CM_CTLINT);
> +
> +	/* Clear DONE and ERROR interrupts */
> +	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
> +		    HDMI_IH_I2CM_STAT0);
> +
> +	/* Mute DONE and ERROR interrupts */
> +	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
> +		    HDMI_IH_MUTE_I2CM_STAT0);
> +
> +	spin_unlock_irqrestore(&hdmi->i2c->lock, flags);

What exactly is this spinlock protecting against with the above code?

> +}
> +
> +static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
> +			    unsigned char *buf, int length)
> +{
> +	int stat;
> +	unsigned long flags;
> +	struct dw_hdmi_i2c *i2c = hdmi->i2c;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +
> +	i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
> +
> +	if (!i2c->is_regaddr) {
> +		dev_dbg(hdmi->dev, "set read register address to 0\n");
> +		i2c->slave_reg = 0x00;
> +		i2c->is_regaddr = true;
> +	}
> +
> +	while (length--) {
> +		reinit_completion(&i2c->cmp_r);

If you're re-initialising the completion on every byte, why do you need
separate completions for the read path and the write path?

If a single completion can be used, you then don't have to store the
operation register value in struct dw_hdmi_i2c either.

> +		i2c->stat = 0;

What use does zeroing this have?  You don't read it, except after you've
had a successful completion, which implies that the interrupt handler must
have written to it.

> +
> +		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
> +		hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
> +
> +		spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +		stat = wait_for_completion_interruptible_timeout(&i2c->cmp_r,
> +								 HZ / 10);
> +		if (!stat)
> +			return -ETIMEDOUT;
> +		if (stat < 0)
> +			return stat;

Can a DDC read/write really be interrupted and restarted correctly?
Won't this restart the transaction from the very beginning?  Have
you validated that all code paths calling into here can cope with
-ERESTARTSYS?

If you look at drm_do_probe_ddc_edid() for example, it will retry
the transfer if you return -ERESTARTSYS here, but as the signal has
not been handled, wait_for_completion_interruptible_timeout() will
immediately return -ERESTARTSYS again... and again... and again.
Each time will queue another operation _without_ waiting for the
last one to complete.

> +
> +		spin_lock_irqsave(&i2c->lock, flags);
> +
> +		/* Check for error condition on the bus */
> +		if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) {
> +			spin_unlock_irqrestore(&i2c->lock, flags);
> +			return -EIO;
> +		}
> +
> +		*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
> +	}
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);

I'd be very surprised if you need the spinlocks in the above code.
You'll see the update to i2c->stat after the completion, becuase
wait_for_completion*() is in the same class as the other event-waiting
functions, and contains barriers which will ensure that you will not
read i2c->stat before you see the completion even on SMP platforms.

> +
> +	return 0;
> +}
...
> +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
> +			    struct i2c_msg *msgs, int num)
> +{
> +	struct dw_hdmi *hdmi = i2c_get_adapdata(adap);
> +	struct dw_hdmi_i2c *i2c = hdmi->i2c;
> +
> +	int i, ret;
> +	u8 addr;
> +	unsigned long flags;
> +
> +	dev_dbg(hdmi->dev, "xfer: num: %d, addr: 0x%x\n", num, msgs[0].addr);
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +
> +	hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0);
> +
> +	/* Set slave device address from the first transaction */
> +	addr = msgs[0].addr;
> +	hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE);
> +
> +	/* Set slave device register address on transfer */
> +	i2c->is_regaddr = false;
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);
> +
> +	for (i = 0; i < num; i++) {
> +		dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: 0x%x\n",
> +			i + 1, num, msgs[i].len, msgs[i].flags);
> +
> +		if (msgs[i].addr != addr) {
> +			dev_warn(hdmi->dev,
> +				 "unsupported transaction, changed slave address\n");
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}

Probably ought to validate that before starting any transaction?

> +
> +		if (msgs[i].len == 0) {
> +			dev_dbg(hdmi->dev,
> +				 "unsupported transaction %d/%d, no data\n",
> +				 i + 1, num);
> +			ret = -EOPNOTSUPP;
> +			break;
> +		}

Ditto.

> +
> +		if (msgs[i].flags & I2C_M_RD)
> +			ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
> +		else
> +			ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
> +
> +		if (ret < 0)
> +			break;
> +	}
> +
> +	if (!ret)
> +		ret = num;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +
> +	/* Mute DONE and ERROR interrupts */
> +	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
> +		    HDMI_IH_MUTE_I2CM_STAT0);
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);

What exactly is this spinlock protecting us from?  A single write to a
register is always atomic.

> +
> +	return ret;
> +}
> +
> +static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter)
> +{
> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
> +}
> +
> +static struct i2c_algorithm dw_hdmi_algorithm = {

const?

> +	.master_xfer	= dw_hdmi_i2c_xfer,
> +	.functionality	= dw_hdmi_i2c_func,
> +};
> +
> +static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi)
> +{
> +	struct i2c_adapter *adap;
> +	int ret;
> +
> +	hdmi->i2c = devm_kzalloc(hdmi->dev, sizeof(*hdmi->i2c), GFP_KERNEL);
> +	if (!hdmi->i2c)
> +		return ERR_PTR(-ENOMEM);

I'd much prefer:
	struct dw_hdmi_i2c *i2c;

	i2c = devm_kzalloc(...);

> +
> +	spin_lock_init(&hdmi->i2c->lock);
> +	init_completion(&hdmi->i2c->cmp_r);
> +	init_completion(&hdmi->i2c->cmp_w);
> +
> +	adap = &hdmi->i2c->adap;
> +	adap->class = I2C_CLASS_DDC;
> +	adap->owner = THIS_MODULE;
> +	adap->dev.parent = hdmi->dev;
> +	adap->algo = &dw_hdmi_algorithm;
> +	strlcpy(adap->name, "DesignWare HDMI", sizeof(adap->name));
> +	i2c_set_adapdata(adap, hdmi);
> +
> +	ret = i2c_add_adapter(adap);
> +	if (ret) {
> +		dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name);
> +		devm_kfree(hdmi->dev, hdmi->i2c);
> +		hdmi->i2c = NULL;
> +		return ERR_PTR(ret);
> +	}
> +

	hdmi->i2c = i2c;

rather than having to remember to clear out hdmi->i2c in error paths.

> +	dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name);
> +
> +	return adap;
> +}
> +
>  static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
>  			   unsigned int n)
>  {
> @@ -1466,16 +1745,47 @@ struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
>  	.mode_fixup = dw_hdmi_bridge_mode_fixup,
>  };
>  
> +static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
> +{
> +	struct dw_hdmi_i2c *i2c = hdmi->i2c;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&i2c->lock, flags);
> +
> +	i2c->stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
> +	if (!i2c->stat) {
> +		spin_unlock_irqrestore(&i2c->lock, flags);
> +		return IRQ_NONE;
> +	}
> +
> +	hdmi_writeb(hdmi, i2c->stat, HDMI_IH_I2CM_STAT0);
> +
> +	if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)
> +		complete(&i2c->cmp_r);
> +	else
> +		complete(&i2c->cmp_w);
> +
> +	spin_unlock_irqrestore(&i2c->lock, flags);

Again, what function does this spinlock perform?  Wouldn't:

	unsigned int stat;

	stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
	if (stat == 0)
		return IRQ_NONE;

	/* Clear interrupts */
	hdmi_writeb(hdmi, stat, HDMI_IH_I2CM_STAT0);

	i2c->stat = stat;

	if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)
		complete(&i2c->cmp_r);
	else
		complete(&i2c->cmp_w);

be fine, or maybe with the spinlock around the assignment to i2c->stat
and this point if you need the assignment and completion to be atomic?

Note that the write to 'i2c->stat' would be atomic in itself anyway.

In any case, complete() implies a write memory barrier, so even on SMP
systems, the write to i2c->stat will be visible before the completion
"completes".  So, I don't think you need any locking here.

Thanks.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH v2 2/2] drm: bridge/dw_hdmi: add dw hdmi i2c bus adapter support

[Vladimir Zapolskiy]

Hi Russell,

On 14.08.2015 01:56, Russell King - ARM Linux wrote:
> On Mon, May 18, 2015 at 03:32:21PM +0300, Vladimir Zapolskiy wrote:
>> +/* HDMI_IH_I2CM_STAT0 and HDMI_IH_MUTE_I2CM_STAT0 register bits */
>> +#define HDMI_IH_I2CM_STAT0_ERROR		BIT(0)
>> +#define HDMI_IH_I2CM_STAT0_DONE			BIT(1)
>> +
>> +/* HDMI_I2CM_OPERATION register bits */
>> +#define HDMI_I2CM_OPERATION_READ		BIT(0)
>> +#define HDMI_I2CM_OPERATION_READ_EXT		BIT(1)
>> +#define HDMI_I2CM_OPERATION_WRITE		BIT(4)
>> +
>> +/* HDMI_I2CM_INT register bits */
>> +#define HDMI_I2CM_INT_DONE_MASK			BIT(2)
>> +#define HDMI_I2CM_INT_DONE_POL			BIT(3)
>> +
>> +/* HDMI_I2CM_CTLINT register bits */
>> +#define HDMI_I2CM_CTLINT_ARB_MASK		BIT(2)
>> +#define HDMI_I2CM_CTLINT_ARB_POL		BIT(3)
>> +#define HDMI_I2CM_CTLINT_NAC_MASK		BIT(6)
>> +#define HDMI_I2CM_CTLINT_NAC_POL		BIT(7)
> 
> Please put these definitions in dw_hdmi.h
> 

okay.

>> +
>> +
>>  #define HDMI_EDID_LEN		512
>>  
>>  #define RGB			0
>> @@ -102,6 +124,19 @@ struct hdmi_data_info {
>>  	struct hdmi_vmode video_mode;
>>  };
>>  
>> +struct dw_hdmi_i2c {
>> +	struct i2c_adapter	adap;
>> +
>> +	spinlock_t		lock;
>> +	struct completion	cmp_r;
>> +	struct completion	cmp_w;
>> +	u8			stat;
>> +
>> +	u8			operation_reg;
>> +	u8			slave_reg;
>> +	bool			is_regaddr;
>> +};
>> +
>>  struct dw_hdmi {
>>  	struct drm_connector connector;
>>  	struct drm_encoder *encoder;
>> @@ -111,6 +146,7 @@ struct dw_hdmi {
>>  	struct device *dev;
>>  	struct clk *isfr_clk;
>>  	struct clk *iahb_clk;
>> +	struct dw_hdmi_i2c *i2c;
>>  
>>  	struct hdmi_data_info hdmi_data;
>>  	const struct dw_hdmi_plat_data *plat_data;
>> @@ -179,6 +215,249 @@ static void hdmi_mask_writeb(struct dw_hdmi *hdmi, u8 data, unsigned int reg,
>>  	hdmi_modb(hdmi, data << shift, mask, reg);
>>  }
>>  
>> +static void dw_hdmi_i2c_init(struct dw_hdmi *hdmi)
>> +{
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&hdmi->i2c->lock, flags);
>> +
>> +	/* Set Fast Mode speed */
>> +	hdmi_writeb(hdmi, 0x0b, HDMI_I2CM_DIV);
>> +
>> +	/* Software reset */
>> +	hdmi_writeb(hdmi, 0x00, HDMI_I2CM_SOFTRSTZ);
>> +
>> +	/* Set done, not acknowledged and arbitration interrupt polarities */
>> +	hdmi_writeb(hdmi, HDMI_I2CM_INT_DONE_POL, HDMI_I2CM_INT);
>> +	hdmi_writeb(hdmi, HDMI_I2CM_CTLINT_NAC_POL | HDMI_I2CM_CTLINT_ARB_POL,
>> +		    HDMI_I2CM_CTLINT);
>> +
>> +	/* Clear DONE and ERROR interrupts */
>> +	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
>> +		    HDMI_IH_I2CM_STAT0);
>> +
>> +	/* Mute DONE and ERROR interrupts */
>> +	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
>> +		    HDMI_IH_MUTE_I2CM_STAT0);
>> +
>> +	spin_unlock_irqrestore(&hdmi->i2c->lock, flags);
> 
> What exactly is this spinlock protecting against with the above code?
> 

On second inspection I don't see a need of locking here.

>> +}
>> +
>> +static int dw_hdmi_i2c_read(struct dw_hdmi *hdmi,
>> +			    unsigned char *buf, int length)
>> +{
>> +	int stat;
>> +	unsigned long flags;
>> +	struct dw_hdmi_i2c *i2c = hdmi->i2c;
>> +
>> +	spin_lock_irqsave(&i2c->lock, flags);
>> +
>> +	i2c->operation_reg = HDMI_I2CM_OPERATION_READ;
>> +
>> +	if (!i2c->is_regaddr) {
>> +		dev_dbg(hdmi->dev, "set read register address to 0\n");
>> +		i2c->slave_reg = 0x00;
>> +		i2c->is_regaddr = true;
>> +	}
>> +
>> +	while (length--) {
>> +		reinit_completion(&i2c->cmp_r);
> 
> If you're re-initialising the completion on every byte, why do you need
> separate completions for the read path and the write path?
> 
> If a single completion can be used, you then don't have to store the
> operation register value in struct dw_hdmi_i2c either.

Correct, I had only one completion and no i2c->operation_reg in v1, will
revert the added complexity to match the previous version
http://lists.freedesktop.org/archives/dri-devel/2015-May/082887.html

>> +		i2c->stat = 0;
> 
> What use does zeroing this have?  You don't read it, except after you've
> had a successful completion, which implies that the interrupt handler must
> have written to it.

I agree, it can be removed.

>> +
>> +		hdmi_writeb(hdmi, i2c->slave_reg++, HDMI_I2CM_ADDRESS);
>> +		hdmi_writeb(hdmi, i2c->operation_reg, HDMI_I2CM_OPERATION);
>> +
>> +		spin_unlock_irqrestore(&i2c->lock, flags);
>> +
>> +		stat = wait_for_completion_interruptible_timeout(&i2c->cmp_r,
>> +								 HZ / 10);
>> +		if (!stat)
>> +			return -ETIMEDOUT;
>> +		if (stat < 0)
>> +			return stat;
> 
> Can a DDC read/write really be interrupted and restarted correctly?
> Won't this restart the transaction from the very beginning?  Have
> you validated that all code paths calling into here can cope with
> -ERESTARTSYS?
> 
> If you look at drm_do_probe_ddc_edid() for example, it will retry
> the transfer if you return -ERESTARTSYS here, but as the signal has
> not been handled, wait_for_completion_interruptible_timeout() will
> immediately return -ERESTARTSYS again... and again... and again.

For me it sounds like an incomplete error condition checking in
drm_do_probe_ddc_edid(). There are 5 retries defined to complete I2C
transfer,

i2c_transfer() may return -EOPNOTSUPP, -EAGAIN or any
adap->algo->master_xfer() value.

I've checked source code of the last 10 alphabetically sorted I2C bus
drivers from i2c/busses/* with .master_xfer in algo, I have found that
the following error codes may be reported:

i2c-sirf.c	 -EAGAIN on timeout
i2c-st.c	 -ETIMEDOUT on timeout, -EBUSY, -EIO, -EAGAIN
i2c-stu300.c	 -ERESTARTSYS (interrupted), -ETIMEDOUT, -EINVAL, -EIO
i2c-tegra.c	 -ETIMEDOUT, -EBUSY, -EINVAL, -EREMOTEIO, -EIO
i2c-tiny-usb.c	 -ENOMEM, -EREMOTEIO
i2c-viperboard.c -EREMOTEIO, -EPROTO
i2c-wmt.c	 -ETIMEDOUT, -EIO, -EBUSY
i2c-xiic.c	 -ETIMEDOUT, -EIO, -EBUSY
i2c-xlp9xx.c	 -ETIMEDOUT, -EIO
i2c-xlr.c	 -ETIMEDOUT, -EIO

Seems that only 1 of 10 bus drivers is supported correctly by
drm_do_probe_ddc_edid().

> Each time will queue another operation _without_ waiting for the
> last one to complete.

I agree it may happen, what would be the best way to avoid this kind of
problem in your opinion? May be convert
wait_for_completion_interruptible_timeout() to
wait_for_completion_timeout() ?

In any case I would propose to add

diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
index 7087da3..93cb1cd 100644
--- a/drivers/gpu/drm/drm_edid.c
+++ b/drivers/gpu/drm/drm_edid.c
@@ -1242,6 +1242,9 @@ drm_do_probe_ddc_edid(void *data, u8 *buf,
unsigned int block, size_t len)
 			DRM_DEBUG_KMS("drm: skipping non-existent adapter %s\n",
 					adapter->name);
 			break;
+		} else if (ret < 0 && ret != -EAGAIN) {
+			DRM_ERROR("I2C transfer error: %d\n", ret);
+			break;
 		}
 	} while (ret != xfers && --retries);

Or due to the statistics it might be better to use -ETIMEDOUT instead of
-EAGAIN above...

>> +
>> +		spin_lock_irqsave(&i2c->lock, flags);
>> +
>> +		/* Check for error condition on the bus */
>> +		if (i2c->stat & HDMI_IH_I2CM_STAT0_ERROR) {
>> +			spin_unlock_irqrestore(&i2c->lock, flags);
>> +			return -EIO;
>> +		}
>> +
>> +		*buf++ = hdmi_readb(hdmi, HDMI_I2CM_DATAI);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&i2c->lock, flags);
> 
> I'd be very surprised if you need the spinlocks in the above code.
> You'll see the update to i2c->stat after the completion, becuase
> wait_for_completion*() is in the same class as the other event-waiting
> functions, and contains barriers which will ensure that you will not
> read i2c->stat before you see the completion even on SMP platforms.

I have no objections and I will remove this spinlock.

The initial reason for the spinlock was to atomically update pair of
HDMI_I2CM_ADDRESS and HDMI_I2CM_OPERATION registers, but if
dw_hdmi_i2c_read() and dw_hdmi_i2c_write() are serialized (e.g. by
serializing dw_hdmi_i2c_xfer()) the spinlock is not needed then.

>> +
>> +	return 0;
>> +}
> ...
>> +static int dw_hdmi_i2c_xfer(struct i2c_adapter *adap,
>> +			    struct i2c_msg *msgs, int num)
>> +{
>> +	struct dw_hdmi *hdmi = i2c_get_adapdata(adap);
>> +	struct dw_hdmi_i2c *i2c = hdmi->i2c;
>> +
>> +	int i, ret;
>> +	u8 addr;
>> +	unsigned long flags;
>> +
>> +	dev_dbg(hdmi->dev, "xfer: num: %d, addr: 0x%x\n", num, msgs[0].addr);
>> +
>> +	spin_lock_irqsave(&i2c->lock, flags);
>> +
>> +	hdmi_writeb(hdmi, 0x00, HDMI_IH_MUTE_I2CM_STAT0);
>> +
>> +	/* Set slave device address from the first transaction */
>> +	addr = msgs[0].addr;
>> +	hdmi_writeb(hdmi, addr, HDMI_I2CM_SLAVE);
>> +
>> +	/* Set slave device register address on transfer */
>> +	i2c->is_regaddr = false;
>> +
>> +	spin_unlock_irqrestore(&i2c->lock, flags);
>> +
>> +	for (i = 0; i < num; i++) {
>> +		dev_dbg(hdmi->dev, "xfer: num: %d/%d, len: %d, flags: 0x%x\n",
>> +			i + 1, num, msgs[i].len, msgs[i].flags);
>> +
>> +		if (msgs[i].addr != addr) {
>> +			dev_warn(hdmi->dev,
>> +				 "unsupported transaction, changed slave address\n");
>> +			ret = -EOPNOTSUPP;
>> +			break;
>> +		}
> 
> Probably ought to validate that before starting any transaction?
> 
>> +
>> +		if (msgs[i].len == 0) {
>> +			dev_dbg(hdmi->dev,
>> +				 "unsupported transaction %d/%d, no data\n",
>> +				 i + 1, num);
>> +			ret = -EOPNOTSUPP;
>> +			break;
>> +		}
> 
> Ditto.

Do you mean split the cycle to loop over message array for validation
purpose and then attempt to send messages iff all of them are valid?
Probably it makes sense, since the expected length of a message array is
small, I'll implement it.

It is worth to mention that the message array from
drm_do_probe_ddc_edid() discussed above won't pass this validation, if a
monitor has more than 1 extension blocks, the number is taken from the
Extension Flag 0x7E. This case should be handled separately using
unimplemented in my change "I2C Master Interface Extended Read Mode",
unfortunately I don't have such monitor to add/test this feature of DW
HDMI. Probably I can fake a monitor and read EDID with multiple
Extension Blocks from AT24 EEPROM, but I would prefer to postpone this
change, hopefully most of HDMI monitors are supported by this version.

>> +
>> +		if (msgs[i].flags & I2C_M_RD)
>> +			ret = dw_hdmi_i2c_read(hdmi, msgs[i].buf, msgs[i].len);
>> +		else
>> +			ret = dw_hdmi_i2c_write(hdmi, msgs[i].buf, msgs[i].len);
>> +
>> +		if (ret < 0)
>> +			break;
>> +	}
>> +
>> +	if (!ret)
>> +		ret = num;
>> +
>> +	spin_lock_irqsave(&i2c->lock, flags);
>> +
>> +	/* Mute DONE and ERROR interrupts */
>> +	hdmi_writeb(hdmi, HDMI_IH_I2CM_STAT0_ERROR | HDMI_IH_I2CM_STAT0_DONE,
>> +		    HDMI_IH_MUTE_I2CM_STAT0);
>> +
>> +	spin_unlock_irqrestore(&i2c->lock, flags);
> 
> What exactly is this spinlock protecting us from?  A single write to a
> register is always atomic.

Will remove it.

On the other hand I think of adding a mutex to serialize
dw_hdmi_i2c_xfer() calls.

>> +
>> +	return ret;
>> +}
>> +
>> +static u32 dw_hdmi_i2c_func(struct i2c_adapter *adapter)
>> +{
>> +	return I2C_FUNC_I2C | I2C_FUNC_SMBUS_EMUL;
>> +}
>> +
>> +static struct i2c_algorithm dw_hdmi_algorithm = {
> 
> const?
> 

Certainly.

>> +	.master_xfer	= dw_hdmi_i2c_xfer,
>> +	.functionality	= dw_hdmi_i2c_func,
>> +};
>> +
>> +static struct i2c_adapter *dw_hdmi_i2c_adapter(struct dw_hdmi *hdmi)
>> +{
>> +	struct i2c_adapter *adap;
>> +	int ret;
>> +
>> +	hdmi->i2c = devm_kzalloc(hdmi->dev, sizeof(*hdmi->i2c), GFP_KERNEL);
>> +	if (!hdmi->i2c)
>> +		return ERR_PTR(-ENOMEM);
> 
> I'd much prefer:
> 	struct dw_hdmi_i2c *i2c;
> 
> 	i2c = devm_kzalloc(...);
> 
>> +
>> +	spin_lock_init(&hdmi->i2c->lock);
>> +	init_completion(&hdmi->i2c->cmp_r);
>> +	init_completion(&hdmi->i2c->cmp_w);
>> +
>> +	adap = &hdmi->i2c->adap;
>> +	adap->class = I2C_CLASS_DDC;
>> +	adap->owner = THIS_MODULE;
>> +	adap->dev.parent = hdmi->dev;
>> +	adap->algo = &dw_hdmi_algorithm;
>> +	strlcpy(adap->name, "DesignWare HDMI", sizeof(adap->name));
>> +	i2c_set_adapdata(adap, hdmi);
>> +
>> +	ret = i2c_add_adapter(adap);
>> +	if (ret) {
>> +		dev_warn(hdmi->dev, "cannot add %s I2C adapter\n", adap->name);
>> +		devm_kfree(hdmi->dev, hdmi->i2c);
>> +		hdmi->i2c = NULL;
>> +		return ERR_PTR(ret);
>> +	}
>> +
> 
> 	hdmi->i2c = i2c;
> 
> rather than having to remember to clear out hdmi->i2c in error paths.

No objections, the resulting code should be slightly simpler.

>> +	dev_info(hdmi->dev, "registered %s I2C bus driver\n", adap->name);
>> +
>> +	return adap;
>> +}
>> +
>>  static void hdmi_set_cts_n(struct dw_hdmi *hdmi, unsigned int cts,
>>  			   unsigned int n)
>>  {
>> @@ -1466,16 +1745,47 @@ struct drm_bridge_funcs dw_hdmi_bridge_funcs = {
>>  	.mode_fixup = dw_hdmi_bridge_mode_fixup,
>>  };
>>  
>> +static irqreturn_t dw_hdmi_i2c_irq(struct dw_hdmi *hdmi)
>> +{
>> +	struct dw_hdmi_i2c *i2c = hdmi->i2c;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&i2c->lock, flags);
>> +
>> +	i2c->stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
>> +	if (!i2c->stat) {
>> +		spin_unlock_irqrestore(&i2c->lock, flags);
>> +		return IRQ_NONE;
>> +	}
>> +
>> +	hdmi_writeb(hdmi, i2c->stat, HDMI_IH_I2CM_STAT0);
>> +
>> +	if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)
>> +		complete(&i2c->cmp_r);
>> +	else
>> +		complete(&i2c->cmp_w);
>> +
>> +	spin_unlock_irqrestore(&i2c->lock, flags);
> 
> Again, what function does this spinlock perform?  Wouldn't:
> 
> 	unsigned int stat;
> 
> 	stat = hdmi_readb(hdmi, HDMI_IH_I2CM_STAT0);
> 	if (stat == 0)
> 		return IRQ_NONE;
> 
> 	/* Clear interrupts */
> 	hdmi_writeb(hdmi, stat, HDMI_IH_I2CM_STAT0);
> 
> 	i2c->stat = stat;
> 
> 	if (i2c->operation_reg & HDMI_I2CM_OPERATION_READ)
> 		complete(&i2c->cmp_r);
> 	else
> 		complete(&i2c->cmp_w);
> 
> be fine, or maybe with the spinlock around the assignment to i2c->stat
> and this point if you need the assignment and completion to be atomic?
> 
> Note that the write to 'i2c->stat' would be atomic in itself anyway.
> 
> In any case, complete() implies a write memory barrier, so even on SMP
> systems, the write to i2c->stat will be visible before the completion
> "completes".  So, I don't think you need any locking here.

Nice trick with a local variable, I agree it is safe to remove the
spinlock from the handler.

> Thanks.
> 

Thank you for review, I'll send the updates. Please check my proposals
regarding drm_do_probe_ddc_edid(),

--
With best wishes,
Vladimir
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel