diff mbox

[v2,13/28] drm/i2c: tda998x: use irq for connection status and EDID read

Message ID 20140109120412.7697d23c@armhf
State New
Headers show

Commit Message

Jean-Francois Moine Jan. 9, 2014, 11:04 a.m. UTC 
This patch adds the optional treatment of the tda998x IRQ.

The interrupt function is used to know the display connection status
without polling and to speedup reading the EDID.

Signed-off-by: Jean-Francois Moine <moinejf@free.fr>
---
 drivers/gpu/drm/i2c/tda998x_drv.c            | 116 +++++++++++++++++++--
 .../devicetree/bindings/drm/i2c/tda998x.txt  |   3 +++
 2 file changed, 108 insertions(+), 11 deletions(-)

Comments

Russell King - ARM Linux Jan. 11, 2014, 6:14 p.m. UTC | #1 
On Thu, Jan 09, 2014 at 12:04:12PM +0100, Jean-Francois Moine wrote:
> +	if (priv->int_irq != NO_IRQ) {
> +		priv->wq_edid_wait = 1;
> +		i = wait_event_timeout(priv->wq_edid,
> +					!priv->wq_edid_wait,
> +					msecs_to_jiffies(100));

This looks wrong on two counts.

First, this is racy.  What you're expecting is that this function is
called before the EDID is read, in order for wq_edid_wait to be set.
You're then hoping that the interrupt is received afterwards.

What if this function wasn't called before the EDID read interrupt
occurred?  We time out.

Secondly, what happens if this function is called more than once?
The second time, we won't see an interrupt, because no state has
changed.

This approach is just too buggy.  Please re-work it to be race free
and safe.  You need to maintain a flag which indicates when there is
valid EDID data present - set this when the device indicates that's
the case, and clear it on disconnect.  Then the above becomes a
wait_event_timeout() based on that.

The last thing which needs to be considered here is whether it's possible
to use wait_event_interruptible_timeout() in this path, but that requires
a bit of research into whether DRM is able to restart a call to this
function after handling a signal.

> @@ -1250,6 +1311,39 @@ tda998x_encoder_init(struct i2c_client *client,
>  		priv->vip_cntrl_2 = video;
>  	}
>  
> +	/* install the optional HDMI connect IRQ */
> +	priv->int_irq = irq_of_parse_and_map(np, 0);
> +	if (priv->int_irq < 0)
> +		priv->int_irq = NO_IRQ;
> +	if (priv->int_irq != NO_IRQ) {

NAK.  Do not use NO_IRQ.  Use <= 0 instead, or just test against zero for
no IRQ.  It would also be nice to offer this facility to non-DT platforms
via client->irq.  Not every arch in the Linux kernel uses DT.

> +
> +		/* init read EDID waitqueue */
> +		init_waitqueue_head(&priv->wq_edid);
> +/*		priv->wq_edid_wait = 0;	*/
> +
> +		/* clear pending interrupts */
> +		reg_read(priv, REG_INT_FLAGS_0);
> +		reg_read(priv, REG_INT_FLAGS_1);
> +		reg_read(priv, REG_INT_FLAGS_2);
> +
> +		ret = request_threaded_irq(priv->int_irq, NULL,
> +					   tda998x_irq_thread,
> +					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
> +					   "tda998x-int", priv);
> +		if (ret) {
> +			dev_err(&client->dev, "failed to request IRQ#%u: %d\n",
> +				priv->int_irq, ret);
> +			goto fail;
> +		}
> +
> +		/* enable HPD irq */
> +		cec_write(priv, REG_CEC_RXSHPDINTENA,
> +				CEC_RXSHPDLEV_HPD | CEC_RXSHPDLEV_RXSENS);
> +
> +		/* treat the first irq if any */
> +		msleep(10);

This comment makes no sense, and doesn't describe what this delay is
actually here for, and why 10ms is the right amount of time to wait.
Sebastian Hesselbarth Jan. 11, 2014, 6:35 p.m. UTC | #2 
On 01/11/2014 07:14 PM, Russell King - ARM Linux wrote:
> On Thu, Jan 09, 2014 at 12:04:12PM +0100, Jean-Francois Moine wrote:
>> @@ -1250,6 +1311,39 @@ tda998x_encoder_init(struct i2c_client *client,
>>   		priv->vip_cntrl_2 = video;
>>   	}
>>
>> +	/* install the optional HDMI connect IRQ */
>> +	priv->int_irq = irq_of_parse_and_map(np, 0);
>> +	if (priv->int_irq < 0)
>> +		priv->int_irq = NO_IRQ;
>> +	if (priv->int_irq != NO_IRQ) {
>
> NAK.  Do not use NO_IRQ.  Use <= 0 instead, or just test against zero for
> no IRQ.  It would also be nice to offer this facility to non-DT platforms
> via client->irq.  Not every arch in the Linux kernel uses DT.

At least for the DT part, I'd suggest to not ask for interrupt directly
but use a proper gpios property. The can of course be converted to
priv->int_irq in some tda998x_dt_probe.

Sebastian
Jean-Francois Moine Jan. 12, 2014, 6:51 p.m. UTC | #3 
On Sat, 11 Jan 2014 19:35:21 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> At least for the DT part, I'd suggest to not ask for interrupt directly
> but use a proper gpios property. The can of course be converted to
> priv->int_irq in some tda998x_dt_probe.

May you give me more information?
Sebastian Hesselbarth Jan. 12, 2014, 7:26 p.m. UTC | #4 
On 01/12/2014 07:51 PM, Jean-Francois Moine wrote:
> On Sat, 11 Jan 2014 19:35:21 +0100
> Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
>
>> At least for the DT part, I'd suggest to not ask for interrupt directly
>> but use a proper gpios property. The can of course be converted to
>> priv->int_irq in some tda998x_dt_probe.
>
> May you give me more information?

Sure, see [1].

[1] http://lists.freedesktop.org/archives/dri-devel/2013-May/038822.html

Sebastian
Jean-Francois Moine Jan. 17, 2014, 5:27 p.m. UTC | #5 
On Sun, 12 Jan 2014 20:26:21 +0100
Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:

> On 01/12/2014 07:51 PM, Jean-Francois Moine wrote:
> > On Sat, 11 Jan 2014 19:35:21 +0100
> > Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com> wrote:
> >
> >> At least for the DT part, I'd suggest to not ask for interrupt directly
> >> but use a proper gpios property. The can of course be converted to
> >> priv->int_irq in some tda998x_dt_probe.
> >
> > May you give me more information?
> 
> Sure, see [1].
> 
> [1] http://lists.freedesktop.org/archives/dri-devel/2013-May/038822.html

Thanks for the link, but I still don't see the advantage of the gpio
(which value?) over the irq number.
diff mbox

Patch

diff --git a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
index f07339e1..83d7880 100644
--- a/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
+++ b/Documentation/devicetree/bindings/drm/i2c/tda998x.txt
@@ -4,6 +4,7 @@  Required properties;
   - compatible: must be "nxp,tda998x"
 
 Optional properties:
+  - interrupts: interrupt number for HDMI exchanges - default: by polling
   - video-ports: 24 bits value - default: <0x230145>
 
 Example:
@@ -11,6 +12,8 @@  Example:
 	tda998x: hdmi-encoder {
 		compatible = "nxp,tda998x";
 		reg = <0x70>;
+		interrupt-parent = <&gpio0>;
+		interrupts = <27 2>;		/* falling edge */
 		pinctrl-0 = <&pmx_camera>;
 		pinctrl-names = "default";
 	};
diff --git a/drivers/gpu/drm/i2c/tda998x_drv.c b/drivers/gpu/drm/i2c/tda998x_drv.c
index b87802f..2187d63 100644
--- a/drivers/gpu/drm/i2c/tda998x_drv.c
+++ b/drivers/gpu/drm/i2c/tda998x_drv.c
@@ -19,6 +19,7 @@ 
 
 #include <linux/hdmi.h>
 #include <linux/module.h>
+#include <linux/of_irq.h>
 
 #include <drm/drmP.h>
 #include <drm/drm_crtc_helper.h>
@@ -44,6 +45,11 @@  struct tda998x_priv {
 	u8 audio_type;		/* audio type */
 	u8 audio_frame[6];
 	u32 audio_port;
+
+	int int_irq;
+	wait_queue_head_t wq_edid;
+	volatile int wq_edid_wait;
+	struct drm_encoder *encoder;
 };
 
 #define to_tda998x_priv(x)  ((struct tda998x_priv *)to_encoder_slave(x)->slave_priv)
@@ -310,11 +316,16 @@  struct tda998x_priv {
 
 /* CEC registers: (not paged)
  */
+#define REG_CEC_INTSTATUS         0xee                /* read */
+# define CEC_INTSTATUS_CEC        (1 << 0)
+# define CEC_INTSTATUS_HDMI       (1 << 1)
 #define REG_CEC_FRO_IM_CLK_CTRL   0xfb                /* read/write */
 # define CEC_FRO_IM_CLK_CTRL_GHOST_DIS (1 << 7)
 # define CEC_FRO_IM_CLK_CTRL_ENA_OTP   (1 << 6)
 # define CEC_FRO_IM_CLK_CTRL_IMCLK_SEL (1 << 1)
 # define CEC_FRO_IM_CLK_CTRL_FRO_DIV   (1 << 0)
+#define REG_CEC_RXSHPDINTENA      0xfc                /* read/write */
+#define REG_CEC_RXSHPDINT         0xfd                /* read */
 #define REG_CEC_RXSHPDLEV         0xfe                /* read */
 # define CEC_RXSHPDLEV_RXSENS     (1 << 0)
 # define CEC_RXSHPDLEV_HPD        (1 << 1)
@@ -520,6 +531,34 @@  tda998x_reset(struct tda998x_priv *priv)
 	reg_write(priv, REG_MUX_VP_VIP_OUT, 0x24);
 }
 
+/*
+ * only 2 interrupts may occur: screen plug/unplug and EDID read
+ */
+static irqreturn_t tda998x_irq_thread(int irq, void *data)
+{
+	struct tda998x_priv *priv = data;
+	u8 sta, cec, lvl, flag0, flag1, flag2;
+
+	if (!priv)
+		return IRQ_HANDLED;
+	sta = cec_read(priv, REG_CEC_INTSTATUS);
+	cec = cec_read(priv, REG_CEC_RXSHPDINT);
+	lvl = cec_read(priv, REG_CEC_RXSHPDLEV);
+	flag0 = reg_read(priv, REG_INT_FLAGS_0);
+	flag1 = reg_read(priv, REG_INT_FLAGS_1);
+	flag2 = reg_read(priv, REG_INT_FLAGS_2);
+	DRM_DEBUG("tda irq sta %02x cec %02x lvl %02x f0 %02x f1 %02x f2 %02x\n",
+			sta, cec, lvl, flag0, flag1, flag2);
+	if ((flag2 & INT_FLAGS_2_EDID_BLK_RD) && priv->wq_edid_wait) {
+		priv->wq_edid_wait = 0;
+		wake_up(&priv->wq_edid);
+	} else if (cec != 0) {			/* level change */
+		if (priv->encoder && priv->encoder->dev)
+			drm_helper_hpd_irq_event(priv->encoder->dev);
+	}
+	return IRQ_HANDLED;
+}
+
 static uint8_t tda998x_cksum(uint8_t *buf, size_t bytes)
 {
 	uint8_t sum = 0;
@@ -982,7 +1021,8 @@  read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	int ret, i;
 
 	/* enable EDID read irq: */
-	reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
+	if (blk == 0)
+		reg_set(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
 
 	offset = (blk & 1) ? 128 : 0;
 	segptr = blk / 2;
@@ -999,17 +1039,30 @@  read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 	reg_write(priv, REG_EDID_CTRL, 0x0);
 
 	/* wait for block read to complete: */
-	for (i = 100; i > 0; i--) {
-		ret = reg_read(priv, REG_INT_FLAGS_2);
-		if (ret < 0)
-			return ret;
-		if (ret & INT_FLAGS_2_EDID_BLK_RD)
-			break;
-		msleep(1);
+	if (priv->int_irq != NO_IRQ) {
+		priv->wq_edid_wait = 1;
+		i = wait_event_timeout(priv->wq_edid,
+					!priv->wq_edid_wait,
+					msecs_to_jiffies(100));
+		if (i < 0) {
+			dev_err(encoder->dev->dev, "read edid wait err %d\n", i);
+			return i;
+		}
+	} else {
+		for (i = 10; i > 0; i--) {
+			msleep(10);
+			ret = reg_read(priv, REG_INT_FLAGS_2);
+			if (ret < 0)
+				return ret;
+			if (ret & INT_FLAGS_2_EDID_BLK_RD)
+				break;
+		}
 	}
 
-	if (i == 0)
+	if (i == 0) {
+		dev_err(encoder->dev->dev, "read edid timeout\n");
 		return -ETIMEDOUT;
+	}
 
 	ret = reg_read_range(priv, REG_EDID_DATA_0, buf, EDID_LENGTH);
 	if (ret != EDID_LENGTH) {
@@ -1018,8 +1071,6 @@  read_edid_block(struct drm_encoder *encoder, uint8_t *buf, int blk)
 		return ret;
 	}
 
-	reg_clear(priv, REG_INT_FLAGS_2, INT_FLAGS_2_EDID_BLK_RD);
-
 	return 0;
 }
 
@@ -1109,7 +1160,14 @@  static int
 tda998x_encoder_create_resources(struct drm_encoder *encoder,
 				struct drm_connector *connector)
 {
+	struct tda998x_priv *priv = to_tda998x_priv(encoder);
+
 	DBG("");
+	if (priv->int_irq != NO_IRQ)
+		connector->polled = DRM_CONNECTOR_POLL_HPD;
+	else
+		connector->polled = DRM_CONNECTOR_POLL_CONNECT |
+			DRM_CONNECTOR_POLL_DISCONNECT;
 	return 0;
 }
 
@@ -1128,6 +1186,8 @@  tda998x_encoder_destroy(struct drm_encoder *encoder)
 {
 	struct tda998x_priv *priv = to_tda998x_priv(encoder);
 	drm_i2c_encoder_destroy(encoder);
+	if (priv->int_irq != NO_IRQ)
+		free_irq(priv->int_irq, priv);
 	if (priv->cec)
 		i2c_unregister_device(priv->cec);
 	kfree(priv);
@@ -1186,6 +1246,7 @@  tda998x_encoder_init(struct i2c_client *client,
 	priv->cec = i2c_new_dummy(client->adapter, 0x34);
 	if (!priv->cec)
 		return -ENODEV;
+	priv->encoder = &encoder_slave->base;
 	priv->dpms = DRM_MODE_DPMS_OFF;
 
 	encoder_slave->slave_priv = priv;
@@ -1250,6 +1311,39 @@  tda998x_encoder_init(struct i2c_client *client,
 		priv->vip_cntrl_2 = video;
 	}
 
+	/* install the optional HDMI connect IRQ */
+	priv->int_irq = irq_of_parse_and_map(np, 0);
+	if (priv->int_irq < 0)
+		priv->int_irq = NO_IRQ;
+	if (priv->int_irq != NO_IRQ) {
+
+		/* init read EDID waitqueue */
+		init_waitqueue_head(&priv->wq_edid);
+/*		priv->wq_edid_wait = 0;	*/
+
+		/* clear pending interrupts */
+		reg_read(priv, REG_INT_FLAGS_0);
+		reg_read(priv, REG_INT_FLAGS_1);
+		reg_read(priv, REG_INT_FLAGS_2);
+
+		ret = request_threaded_irq(priv->int_irq, NULL,
+					   tda998x_irq_thread,
+					   IRQF_TRIGGER_FALLING | IRQF_ONESHOT,
+					   "tda998x-int", priv);
+		if (ret) {
+			dev_err(&client->dev, "failed to request IRQ#%u: %d\n",
+				priv->int_irq, ret);
+			goto fail;
+		}
+
+		/* enable HPD irq */
+		cec_write(priv, REG_CEC_RXSHPDINTENA,
+				CEC_RXSHPDLEV_HPD | CEC_RXSHPDLEV_RXSENS);
+
+		/* treat the first irq if any */
+		msleep(10);
+	}
+
 	return 0;
 
 fail: