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

[Pavel Machek]

Hi!

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

Okay, I think this should be uninterruptible. The signal can be
delivered during next interruptible sleep. Fixes.

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

Well, it also spins :-).

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

Ok, UNINTERRUPTIBLE here...
								Pavel

-- 
Boycott Kodak -- for their patent abuse against Java.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/