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

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Pavel,

On 7/24/05, Pavel Machek <pavel@suse.cz> wrote:
> 
> 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).

If the problem is that you have a single piece of hardware you need to
bind several drivers to - I guess you will have to create a new
sub-device bus for that. Or just register sub-devices on the same bus
the parent device is registered on - I am not sure what is best in
this particular case - I am not familiar with the arch. It is my
understanding that the purpose of interfaces to to present different
"views" to userspace and therefore they are not quie suited for what
you are trying to do...

> +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.

> +
> +       /*
> +        * 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?

Thanks!

-- 
Dmitry