Hi Dmitry,
Still remember the thread irq modificition for the ads7846.c? I sent 4
patches,
you pointed out that the last patch(the 4th one) seems not right, i think it
over and find i was wrong because i mingled two independent flags(
disabled/suspended). Then i gave my understanding and two plans to
be chosen 3 weeks ago, but by now don't get feedback.
Now i re-generate the 4th patch(according to my 2rd plan), Could
you please to review it?
From a48a5ef246217269a0be2b30080d24b406ba3c96 Mon Sep 17 00:00:00 2001
From: Jason Wang <jason77.wang@xxxxxxxxx>
Date: Tue, 12 Oct 2010 17:43:47 +0800
Subject: [PATCH 4/4] Input: ads7846 - move flags update in the
enable()/resume() forward
The condition of executing ads7846_restart() is that the ads7846 is
neither diabled nor suspended. When we call enable()/resume() to
restart ads7846, we should update the corresponding flags before we
call ads7846_restart(), otherwise the ads7846 will never be restarted.
Signed-off-by: Jason Wang <jason77.wang@xxxxxxxxx>
---
drivers/input/touchscreen/ads7846.c | 9 +++++----
1 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/input/touchscreen/ads7846.c
b/drivers/input/touchscreen/ads7846.c
index eab8b0b..25333db 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -253,9 +253,10 @@ static void ads7846_enable(struct ads7846 *ts)
{
mutex_lock(&ts->lock);
- if (ts->disabled && !ts->suspended)
+ if (ts->disabled && !ts->suspended) {
+ ts->disabled = false;
__ads7846_enable(ts);
-
+ }
ts->disabled = false;
mutex_unlock(&ts->lock);
@@ -919,10 +920,10 @@ static int ads7846_resume(struct spi_device *spi)
if (device_may_wakeup(&ts->spi->dev))
disable_irq_wake(ts->spi->irq);
+ ts->suspended = false;
+
if (!ts->disabled)
__ads7846_enable(ts);
-
- ts->suspended = false;
}
mutex_unlock(&ts->lock);
--
1.5.6.5
Jason Wang wrote:
Dmitry Torokhov wrote:
On Fri, Sep 17, 2010 at 05:20:14PM +0800, Jason Wang wrote:
Dmitry Torokhov wrote:
Hi Jason,
On Thursday, September 16, 2010 03:51:26 am Jason Wang wrote:
The design like that, we can stop or disable ADC, one difference is
the disable not only stop the ADC but also close the power regulator.
So we change the condition flag to stopped in the _stop() function.
Because we call __ads7846_disable() in the suspend/resume process, so
move disabled flag modification from ads7846_disable() to the
__ads7846_disable(), otherwise when we call _disable() in the
suspend,
and the ADC is really suspended but the flag shows that it isn't.
I disagree with the patch. "Disable" knob is separate from PM
knobs (suspend/resume); and thus that attribute should only read
"1" when
user forcibly disabled the device via sysfs. Same goes for open/close.
Thanks.
OK, i will fix this patch.
Hmm, I am not sure what there is to fix... Maybe I misunderstood your
description. Could you please try again to explain what you are trying
to achieve?
Thanks!
It seems that i mis-understand your original design. I am wrong for this
patch because i mingled disabled and suspended flags.
Your original design is like that, these two flags are separate ones,
either one
is true, we should stop ADC, Both of them is false we can restart ADC.
But current design(don't apply my 0004-xxx.patch) has some little bugs:
the condition in the ads7846_restart is,
static void ads7846_restart(struct ads7846 *ts)
{
if (!ts->disabled && !ts->suspended) {
But, when we call ads7846_enable()/ads7846_resume(), the
disabled/suspended flag's
update are all at the place after the ads7846_restart() is called, so
this will cause the
ads7846_restart() can't be executed. User will see after they
enable/wakeup this driver
through sysfs, the driver still doesn't work.
There are two smallest fixes, i don't know which one is better?
1) replace the stop()/restart() condition flag to ->stopped:
static void ads7846_stop(struct ads7846 *ts)
{
- if (!ts->disabled && !ts->suspended) {
+ if (!ts->stopped) {
/* Signal IRQ thread to stop polling and disable the handler. */
ts->stopped = true;
mb();
@@ -210,7 +210,7 @@ static void ads7846_stop(struct ads7846 *ts)
/* Must be called with ts->lock held */
static void ads7846_restart(struct ads7846 *ts)
{
- if (!ts->disabled && !ts->suspended) {
+ if (ts->stopped) {
/* Tell IRQ thread that it may poll the device. */
ts->stopped = false;
mb();
2) move the flags update in the enable()/resume() a little bit forward:
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -253,10 +253,11 @@ static void ads7846_enable(struct ads7846 *ts)
{
mutex_lock(&ts->lock);
- if (ts->disabled && !ts->suspended)
+ if (ts->disabled && !ts->suspended) {
+ ts->disabled = false;
__ads7846_enable(ts);
-
- ts->disabled = false;
+ } else
+ ts->disabled = false;
mutex_unlock(&ts->lock);
}
@@ -919,10 +920,10 @@ static int ads7846_resume(struct spi_device *spi)
if (device_may_wakeup(&ts->spi->dev))
disable_irq_wake(ts->spi->irq);
+ ts->suspended = false;
+
if (!ts->disabled)
__ads7846_enable(ts);
-
- ts->suspended = false;
}
mutex_unlock(&ts->lock);
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
