Re: [PATCH 1/1] input: AD7879 Touchscreen driver

[Andrew Morton <akpm@linux-foundation.org>]

On Fri, 17 Oct 2008 01:57:16 +0800
Bryan Wu <cooloney@kernel.org> wrote:

a little thing..

> +static void ad7879_disable(struct ad7879 *ts)
> +{
> +	unsigned long flags;
> +
> +	if (ts->disabled)
> +		return;
> +
> +	spin_lock_irqsave(&ts->lock, flags);
> +	ts->disabled = 1;
> +	disable_irq(ts->bus->irq);
> +	spin_unlock_irqrestore(&ts->lock, flags);
> +
> +	cancel_work_sync(&ts->work);
> +
> +	if (del_timer_sync(&ts->timer))
> +		ad7879_ts_event_release(ts);
> +
> +	/* we know the chip's in lowpower mode since we always
> +	 * leave it that way after every request
> +	 */
> +}
> +
> +static void ad7879_enable(struct ad7879 *ts)
> +{
> +	unsigned long flags;
> +
> +	if (!ts->disabled)
> +		return;
> +
> +	spin_lock_irqsave(&ts->lock, flags);
> +	ts->disabled = 0;
> +	enable_irq(ts->bus->irq);
> +	spin_unlock_irqrestore(&ts->lock, flags);
> +}

It would looks less racy if ts->disabled was tested while the lock was held.

Also, it would be more grammatically pleasing if ad7879_destruct() was
called ad7879_destroy(), but the current spelling will still compile
and run :)