Re: [patch,rfc] Support for touchscreen on sharp zaurus sl-5500

[Nish Aravamudan <nish.aravamudan@gmail.com>]

On 7/20/05, Pavel Machek <pavel@ucw.cz> wrote:
> Hi!
> 
> This adds support for touchscreen of sharp zaurus sl-5500. I got the
> patches from John Lenz <lenz@cs.wisc.edu>, but lots of copyrights are
> Russell King. To do so, it needs to add quite a bit of
> infrastructure. If there's better place for some code, please let me
> know (I already moved touchscreen parts to drivers/input).

<snip>

> diff --git a/drivers/input/touchscreen/collie_ts.c
> b/drivers/input/touchscreen/collie_ts.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/input/touchscreen/collie_ts.c

<snip>

> +static int ucb1x00_thread(void *_ts)
> +{
> +       struct ucb1x00_ts *ts = _ts;
> +       struct task_struct *tsk = current;
> +       int valid;
> +
> +       ts->rtask = tsk;
> +
> +       daemonize("ktsd");
> +       /* only want to receive SIGKILL */
> +       allow_signal(SIGKILL);
> +
> +       /*
> +        * 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;
> +
> +       complete(&ts->init_exit);
> +
> +       valid = 0;
> +
> +       for (;;) {
> +               unsigned int x, y, p, val;
> +
> +               ts->restart = 0;
> +
> +               ucb1x00_adc_enable(ts->ucb);
> +
> +               x = ucb1x00_ts_read_xpos(ts);
> +               y = ucb1x00_ts_read_ypos(ts);
> +               p = ucb1x00_ts_read_pressure(ts);
> +
> +               /*
> +                * Switch back to interrupt mode.
> +                */
> +               ucb1x00_ts_mode_int(ts);
> +               ucb1x00_adc_disable(ts->ucb);
> +
> +               set_task_state(tsk, TASK_UNINTERRUPTIBLE);
> +               schedule_timeout(HZ / 100);
> +               if (signal_pending(tsk))
> +                       break;

You specifically allow SIGKILL, but then sleep uninterruptibly? And
then you check if signal_pending() :) I think you may want
TASK_INTERRUPTIBLE? Or, go one better and use msleep_interruptible(),
as I don't see any wait-queues in the immediate area of this code...

<snip>

> +                       set_task_state(tsk, TASK_INTERRUPTIBLE);
> +                       schedule_timeout(HZ / 100);
> +               }
> +
> +               if (signal_pending(tsk))
> +                       break;

Then you use INTERRUPTIBLE and check signal_pending() again, so I'm
pretty sure you wanted INTERRUPTIBLE above...But this sleep can be
msleep_interruptible(), as well?

<snip>

> diff --git a/drivers/misc/ucb1x00-core.c b/drivers/misc/ucb1x00-core.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/misc/ucb1x00-core.c

<snip>

> +/**
> + *     ucb1x00_adc_read - read the specified ADC channel
> + *     @ucb: UCB1x00 structure describing chip
> + *     @adc_channel: ADC channel mask
> + *     @sync: wait for syncronisation pulse.
> + *
> + *     Start an ADC conversion and wait for the result.  Note that
> + *     synchronised ADC conversions (via the ADCSYNC pin) must wait
> + *     until the trigger is asserted and the conversion is finished.
> + *
> + *     This function currently spins waiting for the conversion to
> + *     complete (2 frames max without sync).

You technically sleep (schedule_timeout()), not spin...

> + *
> + *     If called for a synchronised ADC conversion, it may sleep
> + *     with the ADC semaphore held.
> + */
> +unsigned int ucb1x00_adc_read(struct ucb1x00 *ucb, int adc_channel, int sync)
> +{
> +       unsigned int val;
> +
> +       if (sync)
> +               adc_channel |= UCB_ADC_SYNC_ENA;
> +
> +       ucb1x00_reg_write(ucb, UCB_ADC_CR, ucb->adc_cr | adc_channel);
> +       ucb1x00_reg_write(ucb, UCB_ADC_CR, ucb->adc_cr | adc_channel | UCB_ADC_START);
> +
> +       for (;;) {
> +               val = ucb1x00_reg_read(ucb, UCB_ADC_DATA);
> +               if (val & UCB_ADC_DAT_VAL)
> +                       break;
> +               /* yield to other processes */
> +               set_current_state(TASK_INTERRUPTIBLE);
> +               schedule_timeout(1);
> +       }

If I ever add a poll_event() interface to the kernel, this would be a
good user. You don't check if signal_pending(), though, even though
you are in INTERRUPTIBLE state... Maybe this case can use
UNINTERRUPTIBLE?

Thanks,
Nish