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
--
To unsubscribe from this list: send the line "unsubscribe linux-iio" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
