Skip to contentSkip to conversation

tsc2007: remove HR timer

PATCH In review Richard Röjfors 4 participants 10 messages Jun 23, 2009 – Jul 24, 2009

Subsystems (2)
HID Input drivers
lore
Share & tools
lore

Richard Röjfors

author
#0 lore raw

PATCH 1/2 tsc2007: remove HR timer

This patch removes the HR timer, since it's bad to do synchronous I2C in the HR timer callback context. The new implementation makes use of the global workqueue. The work is scheduled every 5ms when polling rather than 5 us.

Signed-off-by Richard Röjfors <[email protected]>

---

drivers/input/touchscreen/tsc2007.c +14 -35
Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
===================================================================
@@ -21,15 +21,13 @@
  */
 
 #include <linux/module.h>
-#include <linux/hrtimer.h>
 #include <linux/slab.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <linux/i2c/tsc2007.h>
 
-#define TS_POLL_DELAY	(10 * 1000)	/* ns delay before the first sample */
-#define TS_POLL_PERIOD	(5 * 1000)	/* ns delay between samples */
+#define TS_POLL_PERIOD	msecs_to_jiffies(5) /* ms delay between samples */
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)

@@ -70,13 +68,11 @@
 struct tsc2007 {
 	struct input_dev	*input;
 	char			phys[32];
-	struct hrtimer		timer;
+	struct delayed_work	work;
 	struct ts_event		tc;
 
 	struct i2c_client	*client;
 
-	spinlock_t		lock;
-
 	u16			model;
 	u16			x_plate_ohms;
 

@@ -142,8 +138,7 @@
 	if (rt > MAX_12BIT) {
 		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
 
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			      HRTIMER_MODE_REL);
+		schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 		return;
 	}
 

@@ -153,7 +148,7 @@
 	 * in some cases may not even settle at the expected value.
 	 *
 	 * The only safe way to check for the pen up condition is in the
-	 * timer by reading the pen signal state (it's a GPIO _and_ IRQ).
+	 * work function by reading the pen signal state (it's a GPIO and IRQ).
 	 */
 	if (rt) {
 		struct input_dev *input = ts->input;

@@ -175,8 +170,7 @@
 			x, y, rt);
 	}
 
-	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			HRTIMER_MODE_REL);
+	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 }
 
 static int tsc2007_read_values(struct tsc2007 *tsc)

@@ -197,13 +191,10 @@
 	return 0;
 }
 
-static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle)
+static void tsc2007_work(struct work_struct *work)
 {
-	struct tsc2007 *ts = container_of(handle, struct tsc2007, timer);
-	unsigned long flags;
-
-	spin_lock_irqsave(&ts->lock, flags);
-
+	struct tsc2007 *ts =
+		container_of(to_delayed_work(work), struct tsc2007, work);
 	if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
 		struct input_dev *input = ts->input;
 

@@ -222,30 +213,20 @@
 		tsc2007_read_values(ts);
 		tsc2007_send_event(ts);
 	}
-
-	spin_unlock_irqrestore(&ts->lock, flags);
-
-	return HRTIMER_NORESTART;
 }
 
 static irqreturn_t tsc2007_irq(int irq, void *handle)
 {
 	struct tsc2007 *ts = handle;
-	unsigned long flags;
 
-	spin_lock_irqsave(&ts->lock, flags);
-
 	if (likely(ts->get_pendown_state())) {
 		disable_irq_nosync(ts->irq);
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
-					HRTIMER_MODE_REL);
+		schedule_delayed_work(&ts->work, 0);
 	}
 
 	if (ts->clear_penirq)
 		ts->clear_penirq();
 
-	spin_unlock_irqrestore(&ts->lock, flags);
-
 	return IRQ_HANDLED;
 }
 

@@ -278,11 +259,6 @@
 
 	ts->input = input_dev;
 
-	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	ts->timer.function = tsc2007_timer;
-
-	spin_lock_init(&ts->lock);
-
 	ts->model             = pdata->model;
 	ts->x_plate_ohms      = pdata->x_plate_ohms;
 	ts->get_pendown_state = pdata->get_pendown_state;

@@ -308,6 +284,8 @@
 
 	ts->irq = client->irq;
 
+	INIT_DELAYED_WORK(&ts->work, tsc2007_work);
+
 	err = request_irq(ts->irq, tsc2007_irq, 0,
 			client->dev.driver->name, ts);
 	if (err < 0) {

@@ -325,7 +303,6 @@
 
  err_free_irq:
 	free_irq(ts->irq, ts);
-	hrtimer_cancel(&ts->timer);
  err_free_mem:
 	input_free_device(input_dev);
 	kfree(ts);

@@ -337,11 +314,13 @@
 	struct tsc2007	*ts = i2c_get_clientdata(client);
 	struct tsc2007_platform_data *pdata;
 
+	/* cancel any work */
+	cancel_delayed_work(&ts->work);
+
 	pdata = client->dev.platform_data;
 	pdata->exit_platform_hw();
 
 	free_irq(ts->irq, ts);
-	hrtimer_cancel(&ts->timer);
 	input_unregister_device(ts->input);
 	kfree(ts);
#1 lore raw
Quoted 5 lines · On Tue, 23 Jun 2009 13:54:48 +0200 Richard R__jfors…

This patch removes the HR timer, since it's bad to do synchronous I2C in the HR timer callback context. The new implementation makes use of the global workqueue. The work is scheduled every 5ms when polling rather than 5 us.

"it's bad" isn't a very good description of the problem which the patch fixes.

This matters. People wish to make decisions about whether this patch is needed in 2.6.29.x, 2.6.30.x, 2.6.31, 2.6.32, etc. Without knowing the effects of the problem which the patch fixes, we cannot make that decision!

drivers/input/touchscreen/tsc2007.c line 319

Quoted 4 lines
drivers/input/touchscreen/tsc2007.c +3 -0
+	/* cancel any work */
+	cancel_delayed_work(&ts->work);
+

Should this have been cancel_delayed_work_sync()?

/*
* Kill off a pending schedule_delayed_work(). Note that the work callback
* function may still be running on return from cancel_delayed_work(), unless
* it returns 1 and the work doesn't re-arm itself. Run flush_workqueue() or
* cancel_work_sync() to wait on it.
*/
static inline int cancel_delayed_work(struct delayed_work *work)

author
#2 lore raw
Quoted 11 lines · Andrew Morton
Quoted 5 lines · On Tue, 23 Jun 2009 13:54:48 +0200 Richard…

This patch removes the HR timer, since it's bad to do synchronous I2C in the HR timer callback context. The new implementation makes use of the global workqueue. The work is scheduled every 5ms when polling rather than 5 us.

"it's bad" isn't a very good description of the problem which the patch fixes.

The problem is that the synchronous I2C calls in HR-timer callback context might end up in a deadlock.

Quoted 11 lines

This matters. People wish to make decisions about whether this patch is needed in 2.6.29.x, 2.6.30.x, 2.6.31, 2.6.32, etc. Without knowing the effects of the problem which the patch fixes, we cannot make that decision!

drivers/input/touchscreen/tsc2007.c line 319

Quoted 3 lines
drivers/input/touchscreen/tsc2007.c +3 -0
+	/* cancel any work */
+	cancel_delayed_work(&ts->work);
+

Should this have been cancel_delayed_work_sync()?

It should, I will post a new patch shortly which should be applied after these two which fixes this.

--Richard

maintainer
#3 lore raw
In reply to Richard Röjfors

Hi Richard,

Quoted 101 lines · Richard Röjfors

This patch removes the HR timer, since it's bad to do synchronous I2C in the HR timer callback context. The new implementation makes use of the global workqueue. The work is scheduled every 5ms when polling rather than 5 us.

Signed-off-by Richard Röjfors <[email protected]>

---
Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
===================================================================

drivers/input/touchscreen/tsc2007.c +8 -21
@@ -21,15 +21,13 @@
  */
 
 #include <linux/module.h>
-#include <linux/hrtimer.h>
 #include <linux/slab.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
 #include <linux/i2c.h>
 #include <linux/i2c/tsc2007.h>
 
-#define TS_POLL_DELAY	(10 * 1000)	/* ns delay before the first sample */
-#define TS_POLL_PERIOD	(5 * 1000)	/* ns delay between samples */
+#define TS_POLL_PERIOD	msecs_to_jiffies(5) /* ms delay between samples */
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)

@@ -70,13 +68,11 @@
 struct tsc2007 {
 	struct input_dev	*input;
 	char			phys[32];
-	struct hrtimer		timer;
+	struct delayed_work	work;
 	struct ts_event		tc;
 
 	struct i2c_client	*client;
 
-	spinlock_t		lock;
-
 	u16			model;
 	u16			x_plate_ohms;
 

@@ -142,8 +138,7 @@
 	if (rt > MAX_12BIT) {
 		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
 
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			      HRTIMER_MODE_REL);
+		schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 		return;
 	}
 

@@ -153,7 +148,7 @@
 	 * in some cases may not even settle at the expected value.
 	 *
 	 * The only safe way to check for the pen up condition is in the
-	 * timer by reading the pen signal state (it's a GPIO _and_ IRQ).
+	 * work function by reading the pen signal state (it's a GPIO and IRQ).
 	 */
 	if (rt) {
 		struct input_dev *input = ts->input;

@@ -175,8 +170,7 @@
 			x, y, rt);
 	}
 
-	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			HRTIMER_MODE_REL);
+	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 }
 
 static int tsc2007_read_values(struct tsc2007 *tsc)

@@ -197,13 +191,10 @@
 	return 0;
 }
 
-static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle)
+static void tsc2007_work(struct work_struct *work)
 {
-	struct tsc2007 *ts = container_of(handle, struct tsc2007, timer);
-	unsigned long flags;
-
-	spin_lock_irqsave(&ts->lock, flags);
-
+	struct tsc2007 *ts =
+		container_of(to_delayed_work(work), struct tsc2007, work);
 	if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
 		struct input_dev *input = ts->input;
 

@@ -222,30 +213,20 @@
 		tsc2007_read_values(ts);
 		tsc2007_send_event(ts);
 	}
-
-	spin_unlock_irqrestore(&ts->lock, flags);
-
-	return HRTIMER_NORESTART;
 }
 
 static irqreturn_t tsc2007_irq(int irq, void *handle)
 {

drivers/input/touchscreen/tsc2007.c line 224

Quoted 10 lines
drivers/input/touchscreen/tsc2007.c +1 -5
 	struct tsc2007 *ts = handle;
-	unsigned long flags;
 
-	spin_lock_irqsave(&ts->lock, flags);
-
 	if (likely(ts->get_pendown_state())) {
 		disable_irq_nosync(ts->irq);
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
-					HRTIMER_MODE_REL);
+		schedule_delayed_work(&ts->work, 0);

Originally we scheduled reading with timy delay to let the device settle, why don't we schedule with 1ms delay?

Quoted 32 lines
drivers/input/touchscreen/tsc2007.c +2 -7
 	}
 
 	if (ts->clear_penirq)
 		ts->clear_penirq();
 
-	spin_unlock_irqrestore(&ts->lock, flags);
-
 	return IRQ_HANDLED;
 }
 

@@ -278,11 +259,6 @@
 
 	ts->input = input_dev;
 
-	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	ts->timer.function = tsc2007_timer;
-
-	spin_lock_init(&ts->lock);
-
 	ts->model             = pdata->model;
 	ts->x_plate_ohms      = pdata->x_plate_ohms;
 	ts->get_pendown_state = pdata->get_pendown_state;

@@ -308,6 +284,8 @@
 
 	ts->irq = client->irq;
 
+	INIT_DELAYED_WORK(&ts->work, tsc2007_work);
+
 	err = request_irq(ts->irq, tsc2007_irq, 0,
 			client->dev.driver->name, ts);
 	if (err < 0) {

drivers/input/touchscreen/tsc2007.c line 328

Quoted 4 lines
drivers/input/touchscreen/tsc2007.c +0 -1
@@ -325,7 +303,6 @@
 
  err_free_irq:
 	free_irq(ts->irq, ts);
-	hrtimer_cancel(&ts->timer);

Why don't we cancel work here?

drivers/input/touchscreen/tsc2007.c line 322

Quoted 13 lines
drivers/input/touchscreen/tsc2007.c +3 -0
  err_free_mem:
 	input_free_device(input_dev);
 	kfree(ts);

@@ -337,11 +314,13 @@
 	struct tsc2007	*ts = i2c_get_clientdata(client);
 	struct tsc2007_platform_data *pdata;
 
+	/* cancel any work */
+	cancel_delayed_work(&ts->work);
+
 	pdata = client->dev.platform_data;
 	pdata->exit_platform_hw();

So what happens if IRQ is raised here and work is scheduled again?

drivers/input/touchscreen/tsc2007.c line 326

Quoted 5 lines
drivers/input/touchscreen/tsc2007.c +0 -1
 	free_irq(ts->irq, ts);
-	hrtimer_cancel(&ts->timer);
 	input_unregister_device(ts->input);
 	kfree(ts);

Could you please try the patch below and see if the touchscreen still works for you? It should be applied on top of your patch.

Thanks!

Signature · 17 lines
Dmitry

Input: tsc2007 - properly shut off interrupts/delayed work

From: Dmitry Torokhov <[email protected]>

Properly shut off interrupts/delayed work by free-ing IRQ first
and then ensuring that enable/disable is balanced. Also add
__devinit/__devexit markings, restore poll delay/period scheduling
logic, make sure we call exit_platform_hw() method when probe
fails.

Signed-off-by: Dmitry Torokhov <[email protected]>
---

 drivers/input/touchscreen/tsc2007.c |   76 ++++++++++++++++++++++-------------
 1 files changed, 48 insertions(+), 28 deletions(-)
drivers/input/touchscreen/tsc2007.c +48 -28
index b512697..3c71e85 100644
@@ -27,7 +27,8 @@
 #include <linux/i2c.h>
 #include <linux/i2c/tsc2007.h>
 
-#define TS_POLL_PERIOD	msecs_to_jiffies(1) /* ms delay between samples */
+#define TS_POLL_DELAY			1 /* ms delay between samples */
+#define TS_POLL_PERIOD			1 /* ms delay between samples */
 
 #define TSC2007_MEASURE_TEMP0		(0x0 << 4)
 #define TSC2007_MEASURE_AUX		(0x2 << 4)

@@ -76,7 +77,7 @@ struct tsc2007 {
 	u16			model;
 	u16			x_plate_ohms;
 
-	unsigned		pendown;
+	bool			pendown;
 	int			irq;
 
 	int			(*get_pendown_state)(void);

@@ -131,18 +132,18 @@ static void tsc2007_send_event(void *tsc)
 	} else
 		rt = 0;
 
-	/* Sample found inconsistent by debouncing or pressure is beyond
+	/*
+	 * Sample found inconsistent by debouncing or pressure is beyond
 	 * the maximum. Don't report it to user space, repeat at least
 	 * once more the measurement
 	 */
 	if (rt > MAX_12BIT) {
 		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
-
-		schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 		return;
 	}
 
-	/* NOTE: We can't rely on the pressure to determine the pen down
+	/*
+	 * NOTE: We can't rely on the pressure to determine the pen down
 	 * state, even this controller has a pressure sensor.  The pressure
 	 * value can fluctuate for quite a while after lifting the pen and
 	 * in some cases may not even settle at the expected value.

@@ -157,7 +158,7 @@ static void tsc2007_send_event(void *tsc)
 			dev_dbg(&ts->client->dev, "DOWN\n");
 
 			input_report_key(input, BTN_TOUCH, 1);
-			ts->pendown = 1;
+			ts->pendown = true;
 		}
 
 		input_report_abs(input, ABS_X, x);

@@ -169,8 +170,6 @@ static void tsc2007_send_event(void *tsc)
 		dev_dbg(&ts->client->dev, "point(%4d,%4d), pressure (%4u)\n",
 			x, y, rt);
 	}
-
-	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 }
 
 static int tsc2007_read_values(struct tsc2007 *tsc)

@@ -195,6 +194,7 @@ static void tsc2007_work(struct work_struct *work)
 {
 	struct tsc2007 *ts =
 		container_of(to_delayed_work(work), struct tsc2007, work);
+
 	if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
 		struct input_dev *input = ts->input;
 

@@ -204,7 +204,7 @@ static void tsc2007_work(struct work_struct *work)
 		input_report_abs(input, ABS_PRESSURE, 0);
 		input_sync(input);
 
-		ts->pendown = 0;
+		ts->pendown = false;
 		enable_irq(ts->irq);
 	} else {
 		/* pen is still down, continue with the measurement */

@@ -212,6 +212,9 @@ static void tsc2007_work(struct work_struct *work)
 
 		tsc2007_read_values(ts);
 		tsc2007_send_event(ts);
+
+		schedule_delayed_work(&ts->work,
+				      msecs_to_jiffies(TS_POLL_PERIOD));
 	}
 }
 

@@ -221,7 +224,8 @@ static irqreturn_t tsc2007_irq(int irq, void *handle)
 
 	if (likely(ts->get_pendown_state())) {
 		disable_irq_nosync(ts->irq);
-		schedule_delayed_work(&ts->work, 0);
+		schedule_delayed_work(&ts->work,
+				      msecs_to_jiffies(TS_POLL_DELAY));
 	}
 
 	if (ts->clear_penirq)

@@ -230,8 +234,21 @@ static irqreturn_t tsc2007_irq(int irq, void *handle)
 	return IRQ_HANDLED;
 }
 
-static int tsc2007_probe(struct i2c_client *client,
-			const struct i2c_device_id *id)
+static void tsc2007_free_irq(struct tsc2007 *ts)
+{
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disable_irq() done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}
+}
+
+static int __devinit tsc2007_probe(struct i2c_client *client,
+				   const struct i2c_device_id *id)
 {
 	struct tsc2007 *ts;
 	struct tsc2007_platform_data *pdata = pdata = client->dev.platform_data;

@@ -255,17 +272,15 @@ static int tsc2007_probe(struct i2c_client *client,
 	}
 
 	ts->client = client;
-	i2c_set_clientdata(client, ts);
-
+	ts->irq = client->irq;
 	ts->input = input_dev;
+	INIT_DELAYED_WORK(&ts->work, tsc2007_work);
 
 	ts->model             = pdata->model;
 	ts->x_plate_ohms      = pdata->x_plate_ohms;
 	ts->get_pendown_state = pdata->get_pendown_state;
 	ts->clear_penirq      = pdata->clear_penirq;
 
-	pdata->init_platform_hw();
-
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input0", dev_name(&client->dev));
 

@@ -280,12 +295,9 @@ static int tsc2007_probe(struct i2c_client *client,
 	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, 0, 0);
 	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT, 0, 0);
 
+	pdata->init_platform_hw();
 	tsc2007_read_values(ts);
 
-	ts->irq = client->irq;
-
-	INIT_DELAYED_WORK(&ts->work, tsc2007_work);
-
 	err = request_irq(ts->irq, tsc2007_irq, 0,
 			client->dev.driver->name, ts);
 	if (err < 0) {

@@ -297,29 +309,37 @@ static int tsc2007_probe(struct i2c_client *client,
 	if (err)
 		goto err_free_irq;
 
+	i2c_set_clientdata(client, ts);
 	dev_info(&client->dev, "registered with irq (%d)\n", ts->irq);
 
 	return 0;
 
  err_free_irq:
-	free_irq(ts->irq, ts);
+	tsc2007_free_irq(ts);
+	pdata->exit_platform_hw();
  err_free_mem:
 	input_free_device(input_dev);
 	kfree(ts);
 	return err;
 }
 
-static int tsc2007_remove(struct i2c_client *client)
+static int __devexit tsc2007_remove(struct i2c_client *client)
 {
 	struct tsc2007	*ts = i2c_get_clientdata(client);
-	struct tsc2007_platform_data *pdata;
+	struct tsc2007_platform_data *pdata = client->dev.platform_data;
 
-	cancel_delayed_work_sync(&ts->work);
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disabel done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}
 
-	pdata = client->dev.platform_data;
 	pdata->exit_platform_hw();
 
-	free_irq(ts->irq, ts);
 	input_unregister_device(ts->input);
 	kfree(ts);
 

@@ -340,7 +360,7 @@ static struct i2c_driver tsc2007_driver = {
 	},
 	.id_table	= tsc2007_idtable,
 	.probe		= tsc2007_probe,
-	.remove		= tsc2007_remove,
+	.remove		= __devexit_p(tsc2007_remove),
 };
 
 static int __init tsc2007_init(void)

--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

#4 lore raw
Quoted 3 lines · Dmitry Torokhov

Hi Richard,

On Tue, Jun 23, 2009 at 01:54:48PM +0200, Richard Röjfors wrote:

[...]

Quoted 12 lines
drivers/input/touchscreen/tsc2007.c +12 -0
+static void tsc2007_free_irq(struct tsc2007 *ts)
+{
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disable_irq() done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}
+}

[...]

Quoted 7 lines
drivers/input/touchscreen/tsc2007.c +2 -2
-static int tsc2007_remove(struct i2c_client *client)
+static int __devexit tsc2007_remove(struct i2c_client *client)
 {
 	struct tsc2007	*ts = i2c_get_clientdata(client);
-	struct tsc2007_platform_data *pdata;
+	struct tsc2007_platform_data *pdata = client->dev.platform_data;

drivers/input/touchscreen/tsc2007.c static int tsc2007_probe(struct i2c_client *client, · line 339

Quoted 10 lines
drivers/input/touchscreen/tsc2007.c +9 -1
-	cancel_delayed_work_sync(&ts->work);
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disabel done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}

Shouldn't this be tsc2007_free_irq(ts) as well?

Thierry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#5 lore raw
Quoted 25 lines · Thierry Reding
Quoted 3 lines · Dmitry Torokhov

Hi Richard,

On Tue, Jun 23, 2009 at 01:54:48PM +0200, Richard Röjfors wrote:

[...]

Quoted 12 lines
drivers/input/touchscreen/tsc2007.c +12 -0
+static void tsc2007_free_irq(struct tsc2007 *ts)
+{
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disable_irq() done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}
+}

[...]

Quoted 7 lines
drivers/input/touchscreen/tsc2007.c +2 -2
-static int tsc2007_remove(struct i2c_client *client)
+static int __devexit tsc2007_remove(struct i2c_client *client)
  {
  	struct tsc2007	*ts = i2c_get_clientdata(client);
-	struct tsc2007_platform_data *pdata;
+	struct tsc2007_platform_data *pdata = client->dev.platform_data;
Quoted 12 lines

drivers/input/touchscreen/tsc2007.c static int tsc2007_probe(struct i2c_client *client, · line 339

Quoted 10 lines
drivers/input/touchscreen/tsc2007.c +9 -1
-	cancel_delayed_work_sync(&ts->work);
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disabel done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}

Shouldn't this be tsc2007_free_irq(ts) as well?

Is this really good enough? The work function might re-enable the IRQ.

Isn't it better to look at penstate, we know that the IRQ is enabled if the state is UP, else the IRQ is disabled.

--Richard

maintainer
#6 lore raw
Quoted 39 lines · Richard Röjfors
Quoted 25 lines · Thierry Reding
Quoted 3 lines · Dmitry Torokhov

Hi Richard,

On Tue, Jun 23, 2009 at 01:54:48PM +0200, Richard Röjfors wrote:

[...]

Quoted 12 lines
drivers/input/touchscreen/tsc2007.c +12 -0
+static void tsc2007_free_irq(struct tsc2007 *ts)
+{
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disable_irq() done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}
+}

[...]

Quoted 7 lines
drivers/input/touchscreen/tsc2007.c +2 -2
-static int tsc2007_remove(struct i2c_client *client)
+static int __devexit tsc2007_remove(struct i2c_client *client)
  {
  	struct tsc2007	*ts = i2c_get_clientdata(client);
-	struct tsc2007_platform_data *pdata;
+	struct tsc2007_platform_data *pdata = client->dev.platform_data;
Quoted 12 lines

drivers/input/touchscreen/tsc2007.c static int tsc2007_probe(struct i2c_client *client, · line 339

Quoted 10 lines
drivers/input/touchscreen/tsc2007.c +9 -1
-	cancel_delayed_work_sync(&ts->work);
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disabel done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}

Shouldn't this be tsc2007_free_irq(ts) as well?

Quoted 1 line

Is this really good enough? The work function might re-enable the IRQ.

I am pretty sure it is. cancel_delayed_work_sync() will terurn true if there was work scheduled but not yet executed. In this case we know that IRQ was disabled by interrupt handler and will re-enable it.

Signature · 5 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
maintainer
#7 lore raw
In reply to Thierry Reding
Quoted 25 lines · Thierry Reding
Quoted 3 lines · Dmitry Torokhov

Hi Richard,

On Tue, Jun 23, 2009 at 01:54:48PM +0200, Richard Röjfors wrote:

[...]

Quoted 12 lines
drivers/input/touchscreen/tsc2007.c +12 -0
+static void tsc2007_free_irq(struct tsc2007 *ts)
+{
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disable_irq() done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}
+}

[...]

Quoted 7 lines
drivers/input/touchscreen/tsc2007.c +2 -2
-static int tsc2007_remove(struct i2c_client *client)
+static int __devexit tsc2007_remove(struct i2c_client *client)
 {
 	struct tsc2007	*ts = i2c_get_clientdata(client);
-	struct tsc2007_platform_data *pdata;
+	struct tsc2007_platform_data *pdata = client->dev.platform_data;
Quoted 13 lines

drivers/input/touchscreen/tsc2007.c static int tsc2007_probe(struct i2c_client *client, · line 339

Quoted 10 lines
drivers/input/touchscreen/tsc2007.c +9 -1
-	cancel_delayed_work_sync(&ts->work);
+	free_irq(ts->irq, ts);
+	if (cancel_delayed_work_sync(&ts->work)) {
+		/*
+		 * Work was pending, therefore we need to enable
+		 * IRQ here to balance the disabel done in the
+		 * interrupt handler.
+		 */
+		enable_irq(ts->irq);
+	}

Shouldn't this be tsc2007_free_irq(ts) as well?

Yep, it should, thanks.

Signature · 5 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
author
#8 lore raw
In reply to Dmitry Torokhov
Quoted 107 lines · Dmitry Torokhov

Hi Richard,

Quoted 104 lines · Richard Röjfors

This patch removes the HR timer, since it's bad to do synchronous I2C in the HR timer callback context. The new implementation makes use of the global workqueue. The work is scheduled every 5ms when polling rather than 5 us.

Signed-off-by Richard Röjfors<[email protected]>

---
Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
===================================================================

drivers/input/touchscreen/tsc2007.c +8 -22
@@ -21,15 +21,13 @@
   */
 
  #include<linux/module.h>
-#include<linux/hrtimer.h>
  #include<linux/slab.h>
  #include<linux/input.h>
  #include<linux/interrupt.h>
  #include<linux/i2c.h>
  #include<linux/i2c/tsc2007.h>
 
-#define TS_POLL_DELAY	(10 * 1000)	/* ns delay before the first sample */
-#define TS_POLL_PERIOD	(5 * 1000)	/* ns delay between samples */
+#define TS_POLL_PERIOD	msecs_to_jiffies(5) /* ms delay between samples */
 
  #define TSC2007_MEASURE_TEMP0		(0x0<<  4)
  #define TSC2007_MEASURE_AUX		(0x2<<  4)

@@ -70,13 +68,11 @@
  struct tsc2007 {
  	struct input_dev	*input;
  	char			phys[32];
-	struct hrtimer		timer;
+	struct delayed_work	work;
  	struct ts_event		tc;
 
  	struct i2c_client	*client;
 
-	spinlock_t		lock;
-
  	u16			model;
  	u16			x_plate_ohms;
 

@@ -142,8 +138,7 @@
  	if (rt>  MAX_12BIT) {
  		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
 
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			      HRTIMER_MODE_REL);
+		schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
  		return;
  	}
 

@@ -153,7 +148,7 @@
  	 * in some cases may not even settle at the expected value.
  	 *
  	 * The only safe way to check for the pen up condition is in the
-	 * timer by reading the pen signal state (it's a GPIO _and_ IRQ).
+	 * work function by reading the pen signal state (it's a GPIO and IRQ).
  	 */
  	if (rt) {
  		struct input_dev *input = ts->input;

@@ -175,8 +170,7 @@
  			x, y, rt);
  	}
 
-	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			HRTIMER_MODE_REL);
+	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
  }
 
  static int tsc2007_read_values(struct tsc2007 *tsc)

@@ -197,13 +191,10 @@
  	return 0;
  }
 
-static enum hrtimer_restart tsc2007_timer(struct hrtimer *handle)
+static void tsc2007_work(struct work_struct *work)
  {
-	struct tsc2007 *ts = container_of(handle, struct tsc2007, timer);
-	unsigned long flags;
-
-	spin_lock_irqsave(&ts->lock, flags);
-
+	struct tsc2007 *ts =
+		container_of(to_delayed_work(work), struct tsc2007, work);
  	if (unlikely(!ts->get_pendown_state()&&  ts->pendown)) {
  		struct input_dev *input = ts->input;
 

@@ -222,30 +213,20 @@
  		tsc2007_read_values(ts);
  		tsc2007_send_event(ts);
  	}
-
-	spin_unlock_irqrestore(&ts->lock, flags);
-
-	return HRTIMER_NORESTART;
  }
 
  static irqreturn_t tsc2007_irq(int irq, void *handle)
  {
  	struct tsc2007 *ts = handle;
-	unsigned long flags;
Quoted 10 lines

drivers/input/touchscreen/tsc2007.c line 224

Quoted 7 lines
drivers/input/touchscreen/tsc2007.c +1 -4
-	spin_lock_irqsave(&ts->lock, flags);
-
  	if (likely(ts->get_pendown_state())) {
  		disable_irq_nosync(ts->irq);
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
-					HRTIMER_MODE_REL);
+		schedule_delayed_work(&ts->work, 0);

Originally we scheduled reading with timy delay to let the device settle, why don't we schedule with 1ms delay?

Good point, scheduling of the work queue takes some time, but of course that varies.

Quoted 34 lines
Quoted 34 lines
drivers/input/touchscreen/tsc2007.c +2 -7
  	}
 
  	if (ts->clear_penirq)
  		ts->clear_penirq();
 
-	spin_unlock_irqrestore(&ts->lock, flags);
-
  	return IRQ_HANDLED;
  }
 

@@ -278,11 +259,6 @@
 
  	ts->input = input_dev;
 
-	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	ts->timer.function = tsc2007_timer;
-
-	spin_lock_init(&ts->lock);
-
  	ts->model             = pdata->model;
  	ts->x_plate_ohms      = pdata->x_plate_ohms;
  	ts->get_pendown_state = pdata->get_pendown_state;

@@ -308,6 +284,8 @@
 
  	ts->irq = client->irq;
 
+	INIT_DELAYED_WORK(&ts->work, tsc2007_work);
+
  	err = request_irq(ts->irq, tsc2007_irq, 0,
  			client->dev.driver->name, ts);
  	if (err<  0) {

@@ -325,7 +303,6 @@
Quoted 5 lines

drivers/input/touchscreen/tsc2007.c line 328

Quoted 3 lines
drivers/input/touchscreen/tsc2007.c +0 -1
   err_free_irq:
  	free_irq(ts->irq, ts);
-	hrtimer_cancel(&ts->timer);

Why don't we cancel work here?

We should.

Quoted 17 lines
Quoted 5 lines
drivers/input/touchscreen/tsc2007.c +0 -0
   err_free_mem:
  	input_free_device(input_dev);
  	kfree(ts);

drivers/input/touchscreen/tsc2007.c line 322

Quoted 9 lines
drivers/input/touchscreen/tsc2007.c +3 -0
@@ -337,11 +314,13 @@
  	struct tsc2007	*ts = i2c_get_clientdata(client);
  	struct tsc2007_platform_data *pdata;
 
+	/* cancel any work */
+	cancel_delayed_work(&ts->work);
+
  	pdata = client->dev.platform_data;
  	pdata->exit_platform_hw();

So what happens if IRQ is raised here and work is scheduled again?

Quoted 8 lines

drivers/input/touchscreen/tsc2007.c static int tsc2007_probe(struct i2c_client *client, · line 345

Quoted 5 lines
drivers/input/touchscreen/tsc2007.c +0 -1
  	free_irq(ts->irq, ts);
-	hrtimer_cancel(&ts->timer);
  	input_unregister_device(ts->input);
  	kfree(ts);

Could you please try the patch below and see if the touchscreen still works for you? It should be applied on top of your patch.

I have modified my code accordingly to most of your changes. I don't have any platform callbacks in my platform, so make sure those are optional. Will try it out!

--Richard
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html

author
#9 lore raw
In reply to Dmitry Torokhov
Quoted 3 lines · Dmitry Torokhov

Could you please try the patch below and see if the touchscreen still works for you? It should be applied on top of your patch.

Sorry for coming back late, we switched I2C IP so I had to write a new driver for it before trying this out again.

The irq balancing seem to work as in you patch. I didn't run exactly you patch, I took some of the changes into my code, since we can not provide the callbacks.

I have also done some other small changes, to saves us one of five I2C transactions in polling mode.

I Will post this incremental patch in a couple of minutes.

Thanks
--Richard