On Thu, Jan 10, 2013 at 07:19:34PM +0100, Lars-Peter Clausen wrote:
> On 01/10/2013 06:46 PM, Dmitry Torokhov wrote:
> > 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.
>
> Does it really matter? If it is rescheduled you get one event more reported
> after the device has been closed, but input core should ignore it anyway, or
> doesn't it?
But the problem is that if work runs again it will enable the interrupt.
It is better to have all ducks in a row.
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
