Re: [patch 2/2] Touchscreen support for sharp sl-5500

[Pavel Machek <pavel@suse.cz>]

Hi!

> > This adds support for touchscreen on Sharp Zaurus sl-5500. Vojtech,
> > please apply,
> 
> I have couple more commnets...

Sorry, I never really worked with kthreads. Applied all those, 

> > +static int ucb1x00_ts_open(struct input_dev *idev)
> > +{
> > +	struct ucb1x00_ts *ts = (struct ucb1x00_ts *)idev;
> > +	int ret = 0;
> > +	struct task_struct *task;
> > +
> > +	if (ts->rtask)
> > +		panic("ucb1x00: rtask running?");
> > +
> 
> Do you really need to panic here???

Does BUG_ON() seem better :-). We could also just return failure here,
but I do not see how it could happen => I guess I'd better catch it
with BUG().

Here's what I did, updated patch will follow.
								Pavel

diff --git a/drivers/input/touchscreen/collie_ts.c b/drivers/input/touchscreen/collie_ts.c
--- a/drivers/input/touchscreen/collie_ts.c
+++ b/drivers/input/touchscreen/collie_ts.c
@@ -41,7 +41,6 @@ struct ucb1x00_ts {
 	struct ucb1x00		*ucb;
 
 	struct semaphore	irq_wait;
-	struct completion	init_exit;
 	struct task_struct	*rtask;
 	u16			x_res;
 	u16			y_res;
@@ -160,7 +159,6 @@ static int ucb1x00_thread(void *_ts)
 	int valid;
 
 	ts->rtask = tsk;
-	allow_signal(SIGKILL);
 
 	/*
 	 * We run as a real-time thread.  However, thus far
@@ -169,10 +167,7 @@ static int ucb1x00_thread(void *_ts)
 	tsk->policy = SCHED_FIFO;
 	tsk->rt_priority = 1;
 
-	complete(&ts->init_exit);
-
 	valid = 0;
-
 	for (;;) {
 		unsigned int x, y, p, val;
 
@@ -237,12 +232,12 @@ static int ucb1x00_thread(void *_ts)
 			msleep_interruptible(10);
 		}
 
-		if (signal_pending(tsk))
+		if (kthread_should_stop())
 			break;
 	}
 
 	ts->rtask = NULL;
-	complete_and_exit(&ts->init_exit, 0);
+	return 0;
 }
 
 /*
@@ -262,8 +257,7 @@ static int ucb1x00_ts_open(struct input_
 	int ret = 0;
 	struct task_struct *task;
 
-	if (ts->rtask)
-		panic("ucb1x00: rtask running?");
+	BUG_ON(ts->rtask);
 
 	sema_init(&ts->irq_wait, 0);
 	ret = ucb1x00_hook_irq(ts->ucb, UCB_IRQ_TSPX, ucb1x00_ts_irq, ts);
@@ -279,10 +273,8 @@ static int ucb1x00_ts_open(struct input_
 	ts->y_res = ucb1x00_ts_read_yres(ts);
 	ucb1x00_adc_disable(ts->ucb);
 
-	init_completion(&ts->init_exit);
 	task = kthread_run(ucb1x00_thread, ts, "ktsd");
 	if (!IS_ERR(task)) {
-		wait_for_completion(&ts->init_exit);
 		ret = 0;
 	} else {
 		ucb1x00_free_irq(ts->ucb, UCB_IRQ_TSPX, ts);
@@ -300,10 +292,8 @@ static void ucb1x00_ts_close(struct inpu
 {
 	struct ucb1x00_ts *ts = (struct ucb1x00_ts *)idev;
 
-	if (ts->rtask) {
-		send_sig(SIGKILL, ts->rtask, 1);
-		wait_for_completion(&ts->init_exit);
-	}
+	if (ts->rtask)
+		kthread_stop(ts->rtask);
 
 	ucb1x00_enable(ts->ucb);
 	ucb1x00_free_irq(ts->ucb, UCB_IRQ_TSPX, ts);



-- 
teflon -- maybe it is a trademark, but it should not be.