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?
Btw. why not use threaded interrupts instead of irq + workqueue combo?
- Lars
--
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
