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

[Pavel Machek <pavel@suse.cz>]

Hi!

> > I have made quite a lot of cleanups to touchscreen part, and it seems
> > to be acceptable by input people. I think it should go into
> > drivers/input/touchscreen/collie_ts.c... Also it looks to me like
> > mcp.h should go into asm/arch-sa1100, so that other drivers can use it...
> 
> I have couple of nitpicks (below) and one bigger concern - I am
> surprised that a driver for a physical device is implemented as an
> interface to a class device. This precludes implementing any kind of
> power management in the driver and pushes it into the parent and is
> generally speaking is a wrong thing to do (IMHO).

I'll port my changes to newer version of rmk's tree, that should solve
it.

> > +static int ucb1x00_thread(void *_ts)
> > +{
> > +       struct ucb1x00_ts *ts = _ts;
> > +       struct task_struct *tsk = current;
> > +       int valid;
> > +
> > +       ts->rtask = tsk;
> 
> Just move that assignment into ucb1x00_input_open and kill all this
> "current" stuff.

It will still want to set the priority, but yes, it cleaned it.

> > +       /*
> > +        * We run as a real-time thread.  However, thus far
> > +        * this doesn't seem to be necessary.
> > +        */
> > +       tsk->policy = SCHED_FIFO;
> > +       tsk->rt_priority = 1;
> > +
> > +       valid = 0;
> > +       for (;;) {
> 
> Can we change this to "while (!kthread_should_stop())" to make me
> completely happy?

:-) Ok.

[Just FYI, I'll post agregated patch when I solve file placement and
port to newer version.]

Cleanups suggested by Dmitri.

---
commit 60814924ed695d863fa226c24b3d4e96054c8b66
tree 0e88e8272c23926c5654ae10bfe14d4cb2af84ab
parent ff30d8505b88064a5f6e6e70bd42028150a864e2
author <pavel@amd.(none)> Mon, 25 Jul 2005 23:35:21 +0200
committer <pavel@amd.(none)> Mon, 25 Jul 2005 23:35:21 +0200

 drivers/input/touchscreen/collie_ts.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

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
@@ -158,8 +158,6 @@ static int ucb1x00_thread(void *_ts)
 	struct task_struct *tsk = current;
 	int valid;
 
-	ts->rtask = tsk;
-
 	/*
 	 * We run as a real-time thread.  However, thus far
 	 * this doesn't seem to be necessary.
@@ -168,7 +166,7 @@ static int ucb1x00_thread(void *_ts)
 	tsk->rt_priority = 1;
 
 	valid = 0;
-	for (;;) {
+	while (!kthread_should_stop()) {
 		unsigned int x, y, p, val;
 
 		ts->restart = 0;
@@ -231,9 +229,6 @@ static int ucb1x00_thread(void *_ts)
 
 			msleep_interruptible(10);
 		}
-
-		if (kthread_should_stop())
-			break;
 	}
 
 	ts->rtask = NULL;
@@ -273,11 +268,12 @@ static int ucb1x00_ts_open(struct input_
 	ts->y_res = ucb1x00_ts_read_yres(ts);
 	ucb1x00_adc_disable(ts->ucb);
 
-	task = kthread_run(ucb1x00_thread, ts, "ktsd");
-	if (!IS_ERR(task)) {
+	ts->rtask = kthread_run(ucb1x00_thread, ts, "ktsd");
+	if (!IS_ERR(ts->task)) {
 		ret = 0;
 	} else {
 		ucb1x00_free_irq(ts->ucb, UCB_IRQ_TSPX, ts);
+		ts->rtask = NULL;
 		ret = -EFAULT;
 	}
 


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