Re: [PATCH v2] Input: ads7846 - Replace spinlock by mutex to wrap disable()/enable()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Dmitry Torokhov wrote:
On Wed, Sep 01, 2010 at 09:48:18AM -0700, Dmitry Torokhov wrote:
On Wed, Sep 01, 2010 at 06:07:30PM +0800, Jason Wang wrote:
Dmitry Torokhov wrote:
Hi Jason,

On Wed, Aug 25, 2010 at 11:45:27AM +0800, Jason Wang wrote:
Hi dmitry and others,

could you please to help me review this patch?

Thanks,
Jason.

Jason Wang wrote:
The commit 9114337 introduces regulator operations in the ads7846
touchscreen driver. Among these operations, some are called in the
spinlock protected context.
On most platforms, the regulator operation is achieved through
i2c/spi bus transfer operations, some bus transfer operations will
call wait_for_completion function. It isn't allowable  to call
sleepable function in the atomic context. So replace the spinlock with
mutex to protect ads7846_disable()/ads7846_enable().

I am afraid the patch is not correct. ads7846_disable() and
ads7846_enable() check and modify flags (such as irq_disabled and
pending) that are also accessed form timer/interrupt context. Moving to
mutex removes the serialization that used to be there.
Thanks for your comments. you are right it is dangerous and
unreasonable to use different locks to protect a critical resource.
I wonder if we should start by converting the driver to used threaded
IRQ model with "long playing" interrupt handler so all access happens in
process context and shutdown sequence is simplified.
It can solve most issues, but it can't solve this situation. Because
here the atomic region in which conflicts happened is from spin_lock
instead of irq handler.

Right, but switching to threaded IRQ will allow to use mutex in place
of the spinlock everywhere.


I wonder if the following patch works (or can be made to work)...

Hi Dmitry,

I have made a quick test of this patch, currently two issues found:
1) missing #include <linux/sched.h> , it will produce a building error.
2) touch panel can't work, it seems that the input dev doesn't report pen up events.

Your patch is a good frame work, i will continue working basing off your patch. Maybe several dates later, i will send out an extra patch to strengthen your patch.


Thanks,
Jason.








--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Media Devel]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Linux Wireless Networking]     [Linux Omap]

  Powered by Linux