Re: [PATCH 1/2] tsc2007: remove HR timer

["Richard Röjfors" <richard.rojfors.ext@mocean-labs.com>]

On 7/14/09 6:59 AM, Dmitry Torokhov wrote:
> Hi Richard,
>
> On Tue, Jun 23, 2009 at 01:54:48PM +0200, Richard Röjfors wrote:
>> 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<richard.rojfors.ext@mocean-labs.com>
>> ---
>> Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
>> ===================================================================
>> --- linux-2.6.30/drivers/input/touchscreen/tsc2007.c	(revision 932)
>> +++ linux-2.6.30/drivers/input/touchscreen/tsc2007.c	(revision 943)
>> @@ -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);
>
> 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.

>
>>   	}
>>
>>   	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);
>
> Why don't we cancel work here?

We should.

>
>>    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?
>
>>   	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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html