Re: [PATCH 2.4] Wolfson AC97 touch screen driver - Input Event interface

[Jeff Garzik <jgarzik@pobox.com>]

Liam Girdwood wrote:
> This patch adds support for the kernel input event interface to the
> Wolfson WM9705/WM9712 touchscreen driver.
> 
> Patch is against 2.4.24-pre1
> 
> Changes:-
> 
> o Added Input event interface (Stanley Cai)
> o Improved driver CPU utilisation (Ian Molton)
> o Removed ADC source bits from output (me)
> o Added coordinate streaming mode on XScale (me) 
> o Removed old h3600 TS_EVENT interface (me)

Patches look mostly OK...  a few comments though.



> +/*
> + * Pen down detection
> + * 
> + * Pen down detection can either be via an interrupt (preferred) or
> + * by polling the PDEN bit. This is an option because some systems may
> + * not support the pen down interrupt.
> + *
> + * Set pen_int to 1 to enable interrupt driven pen down detection.
> + */
> +MODULE_PARM(pen_int,"i");
> +MODULE_PARM_DESC(pen_int, "Set pen down interrupt");
> +static int pen_int = 0;	

I wouldn't call this "detection"  ;-)

This module option is awful for users.  How do users know what to do? 
(rhetorical question...)   IMO this should be selected automatically on 
a per-chipset basis.  I'm sure there is _some_ way to notice 
programmatically, for instance selected by chipset or selecting by 
sending a test interrupt.

XScale PDA users just aren't going to know this kind of information off 
the top of their heads...


>  /*
>   * ADC sample delay times in uS
>   */
>  static const int delay_table[16] = {
> -	21,		// 1 AC97 Link frames
> -	42,		// 2
> -	84,		// 4
> -	167,		// 8
> -	333,		// 16
> -	667,		// 32
> -	1000,		// 48
> -	1333,		// 64
> -	2000,		// 96
> -	2667,		// 128
> -	3333,		// 160
> -	4000,		// 192
> -	4667,		// 224
> -	5333,		// 256
> -	6000,		// 288
> -	0 		// No delay, switch matrix always on
> +	21,// 1 AC97 Link frames
> +	42,// 2
> +	84,// 4
> +	167,// 8
> +	333,// 16
> +	667,// 32
> +	1000,// 48
> +	1333,// 64
> +	2000,// 96
> +	2667,// 128
> +	3333,// 160
> +	4000,// 192
> +	4667,// 224
> +	5333,// 256
> +	6000,// 288
> +	0 // No delay, switch matrix always on
>  };

is this a mistake?  You just made the code more difficult to read.

Also in general it would be nice to include whitespace (non-content) 
changes in a separate patch, for easier reviewing of your _real_ changes.


> +// Todo
> +static void wm97xx_interrupt(int irq, void *dev_id, struct pt_regs *regs)
> +{
> +	info("int recv");
> +}

hmmmm.

This change doesn't seem terribly appropriate for the 2.4.x stable series...



> +/*
> + * add a touch event
> + */
> +static int wm97xx_ts_evt_add(wm97xx_ts_t* ts, u16 pressure, u16 x, u16 y)
> +{
> +	/* add event and remove adc src bits */
> +	input_report_abs(ts->idev, ABS_X, x & 0xfff);
> +	input_report_abs(ts->idev, ABS_Y, y & 0xfff);
> +	input_report_abs(ts->idev, ABS_PRESSURE, pressure & 0xfff);
> +	return 0;
> +}
> +
> +/*
> + * add a pen up event
> + */
> +static int wm97xx_ts_evt_release(wm97xx_ts_t* ts)
> +{
> +//	dbg("release the stylus.\n");
> +	input_report_abs(ts->idev, ABS_PRESSURE, 0);
> +	return 0;
> +}
> +
> +/*
> + * The touchscreen sample reader thread
> + */
> +static int wm97xx_thread(void *_ts)
> +{
> +	wm97xx_ts_t *ts = (wm97xx_ts_t *)_ts;
> +	struct task_struct *tsk = current;
> +	int valid;
> +
> +	ts->rtask = tsk;
> +
> +	/* set up thread context */
> +	daemonize();
> +	reparent_to_init();
> +	strcpy(tsk->comm, "ktsd");
> +	tsk->tty = NULL;
> +
> +	/* we will die when we receive SIGKILL */
> +	spin_lock_irq(&tsk->sigmask_lock);
> +	siginitsetinv(&tsk->blocked, sigmask(SIGKILL));
> +	recalc_sigpending(tsk);
> +	spin_unlock_irq(&tsk->sigmask_lock);
> +
> +	/* init is complete */
> +	complete(&ts->init_exit);
> +	valid = 0;

(bookmark... will mention a bit lower down...)

> +	/* touch reader loop */
> +	for (;;) {
> +		ts->restart = 0;
> +		if( signal_pending(tsk))
> +			break;
> +
> +		if(pendown(ts)) {
> +			switch (mode) {
> +				case 0:
> +					wm97xx_poll_touch(ts);
> +					valid = 1;
> +					break;
> +				case 1:
> +					wm97xx_poll_coord_touch(ts);
> +					valid = 1;
> +					break;
> +				case 2:
> +					wm97xx_cont_touch(ts);
> +					valid = 1;
> +					break;
> +			}
> +		} else {
> +			if (valid) {
> +				valid = 0;
> +				wm97xx_ts_evt_release(ts);
> +			}
> +		}
> +
> +		set_task_state(tsk, TASK_INTERRUPTIBLE);
> +		schedule_timeout(HZ/100);

Hopefully this won't run on a machine with HZ < 100 ;-)


> +	}
> +	ts->rtask = NULL;
> +	complete_and_exit(&ts->init_exit, 0);

You don't want to use the _same_ completion to signal (a) thread init is 
complete and (b) thread has exited.  That's definitely an error.

For (a), just use a semaphore.  The completion should only be used once, 
at the end of the thread where you call complete_and_exit().



> +/*
> + * Start the touchscreen thread and 
> + * the touch digitiser.
> + */
> +static int wm97xx_ts_input_open(struct input_dev *idev)
> +{
> +	wm97xx_ts_t *ts = (wm97xx_ts_t *) &wm97xx_ts;
> +	u32 flags;
> +	int ret, val;
> +
> +	if ( ts->use_count++ != 0 ) 
> +		return 0;
> +
> +	/* start touchscreen thread */
> +	ts->idev = idev;
> +    init_completion(&ts->init_exit);
> +    ret = kernel_thread(wm97xx_thread, ts, 0);
> +    if (ret >= 0)
> +		wait_for_completion(&ts->init_exit); 
> +
> +	spin_lock_irqsave( &ts->lock, flags );
> +
> +    /* start digitiser */
> +    val = ts->codec->codec_read(ts->codec, AC97_WM97XX_DIGITISER2);
> +    ts->codec->codec_write(ts->codec, AC97_WM97XX_DIGITISER2,
> +                               val | WM97XX_PRP_DET_DIG);
> +
> +	spin_unlock_irqrestore( &ts->lock, flags );
> +
> +	return 0;
> +}
> +
> +/*
> + * Kill the touchscreen thread and stop
> + * the touch digitiser.
> + */
> +static void wm97xx_ts_input_close(struct input_dev *idev)
> +{
> +	wm97xx_ts_t *ts = (wm97xx_ts_t *) &wm97xx_ts;
> +	u32 flags;
> +	int val;
> +
> +	if (--ts->use_count == 0) {
> +		/* kill thread */
> +		if (ts->rtask) {
> +			send_sig(SIGKILL, ts->rtask, 1);
> +			wait_for_completion(&ts->init_exit);
> +		}
> +
> +		down(&ts->sem);
> +		spin_lock_irqsave(&ts->lock, flags);
> +
> +		/* stop digitiser */
> +		val = ts->codec->codec_read(ts->codec, AC97_WM97XX_DIGITISER2);
> +		ts->codec->codec_write(ts->codec, AC97_WM97XX_DIGITISER2,
> +				       val & ~WM97XX_PRP_DET_DIG);
> +		
> +		spin_unlock_irqrestore(&ts->lock, flags);
> +		up(&ts->sem);
> +	}
> +}

What lock or semaphore protects ts->use_count ?  That seems clearly 
non-atomic and potentially racy to me.