[PATCH 2/3 V2] iio: mxs: Implement support for touchscreen

[Dmitry Torokhov]

On Thu, Jan 10, 2013 at 10:48:37AM +0100, Marek Vasut wrote:
> Dear Dmitry Torokhov,
> 
> [...]
> > > > +	enum mxs_lradc_ts	use_touchscreen;
> > > > +	unsigned int		stop_touchscreen:1;
> > > > +	unsigned int		use_touchbutton:1;
> > 
> > Can we make them bools instead of bit fields?
> 
> Sure.
> [...]
> 
> > > > +static void mxs_lradc_ts_close(struct input_dev *dev)
> > > > +{
> > > > +	struct mxs_lradc *lradc = input_get_drvdata(dev);
> > > > +
> > > > +	/* Indicate the touchscreen is stopping. */
> > > > +	lradc->stop_touchscreen = 1;
> > > > +
> > > > +	/* Disable touchscreen touch-detect IRQ. */
> > > > +	writel(LRADC_CTRL1_TOUCH_DETECT_IRQ_EN,
> > > > +		lradc->base + LRADC_CTRL1 + STMP_OFFSET_REG_CLR);
> > > > +
> > > > +	/* Power-down touchscreen touch-detect circuitry. */
> > > > +	writel(LRADC_CTRL0_TOUCH_DETECT_ENABLE,
> > > > +		lradc->base + LRADC_CTRL0 + STMP_OFFSET_REG_CLR);
> > 
> > These 2 writes are racing with writes in mxs_lradc_ts_work(). I think
> > you need to:
> > 
> > 	lradc->stop_touchscreen = true;
> > 	mb();
> 
> Nice catch, do we need the memory barrier here though, is it not enough to 
> reorder the cancel_work_sync() just before the register writes?

You really need to make sure that setting lradc->stop_touchscreen =
true; is not reordered, because otherwise you may cancel the work,
interrupt happens and reschedules it, and then you set up the flag.

Thanks.

-- 
Dmitry