All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/4] Switch ads7846 driver to use soft irq
@ 2010-09-16 10:51 Jason Wang
  2010-09-16 10:51 ` [PATCH 1/4] Input: ads7846 - switch to using threaded IRQ Jason Wang
  2010-09-17  1:48 ` [PATCH 0/4] Switch ads7846 driver to use soft irq Jason Wang
  0 siblings, 2 replies; 19+ messages in thread
From: Jason Wang @ 2010-09-16 10:51 UTC (permalink / raw)
  To: dmitry.torokhov, notasas, vapier; +Cc: linux-input

Some explanations:

The first patch is from dmitry, he build a frame work for this driver
to use soft irq. The remaining 3 patches are from me to fix some issues
for the driver of soft irq verison.

We change this driver to use soft irq because under current version,
we must use spin lock to protect racing issues, the regulator
operations are also in the racing issue list but we can't put it
in the spin lock protected area because regulator operations always
implement via i2c/spi transfers, in those transfers often call sleep
funcitons. Now change to use soft irq, we can use mutex instread of
spin lock, so this issue is solved.

I have tested these patches on the ti_omap3530evm board:
 1)use ts_lib after normal boot
 2)use ts_lib after "#echo 1/0 > /sys/bus/spi/devices/spi0.1/disable"
 3)use ts_lib after "#echo mem > /sys/power/state" and "wake up"



^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/4] Input: ads7846 - switch to using threaded IRQ
  2010-09-16 10:51 [PATCH 0/4] Switch ads7846 driver to use soft irq Jason Wang
@ 2010-09-16 10:51 ` Jason Wang
  2010-09-16 10:51   ` [PATCH 2/4] Input: ads7846 - add a include header to prevent building fails Jason Wang
  2010-09-17  1:48 ` [PATCH 0/4] Switch ads7846 driver to use soft irq Jason Wang
  1 sibling, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-09-16 10:51 UTC (permalink / raw)
  To: dmitry.torokhov, notasas, vapier; +Cc: linux-input

From: Dmitry Torokhov <dtor@mail.ru>

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---
 drivers/input/touchscreen/ads7846.c |  876 ++++++++++++++++++-----------------
 1 files changed, 446 insertions(+), 430 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 1603193..d1fc8ef 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -17,6 +17,7 @@
  *  it under the terms of the GNU General Public License version 2 as
  *  published by the Free Software Foundation.
  */
+#include <linux/types.h>
 #include <linux/hwmon.h>
 #include <linux/init.h>
 #include <linux/err.h>
@@ -52,22 +53,23 @@
  * files.
  */
 
-#define TS_POLL_DELAY	(1 * 1000000)	/* ns delay before the first sample */
-#define TS_POLL_PERIOD	(5 * 1000000)	/* ns delay between samples */
+#define TS_POLL_DELAY	1	/* ms delay before the first sample */
+#define TS_POLL_PERIOD	5	/* ms delay between samples */
 
 /* this driver doesn't aim at the peak continuous sample rate */
 #define	SAMPLE_BITS	(8 /*cmd*/ + 16 /*sample*/ + 2 /* before, after */)
 
 struct ts_event {
-	/* For portability, we can't read 12 bit values using SPI (which
-	 * would make the controller deliver them as native byteorder u16
+	/*
+	 * For portability, we can't read 12 bit values using SPI (which
+	 * would make the controller deliver them as native byte order u16
 	 * with msbs zeroed).  Instead, we read them as two 8-bit values,
 	 * *** WHICH NEED BYTESWAPPING *** and range adjustment.
 	 */
 	u16	x;
 	u16	y;
 	u16	z1, z2;
-	int	ignore;
+	bool	ignore;
 	u8	x_buf[3];
 	u8	y_buf[3];
 };
@@ -110,8 +112,11 @@ struct ads7846 {
 
 	struct spi_transfer	xfer[18];
 	struct spi_message	msg[5];
-	struct spi_message	*last_msg;
-	int			msg_idx;
+	int			msg_count;
+	wait_queue_head_t	wait;
+
+	bool			pendown;
+
 	int			read_cnt;
 	int			read_rep;
 	int			last_read;
@@ -122,14 +127,10 @@ struct ads7846 {
 
 	u16			penirq_recheck_delay_usecs;
 
-	spinlock_t		lock;
-	struct hrtimer		timer;
-	unsigned		pendown:1;	/* P: lock */
-	unsigned		pending:1;	/* P: lock */
-// FIXME remove "irq_disabled"
-	unsigned		irq_disabled:1;	/* P: lock */
-	unsigned		disabled:1;
-	unsigned		is_suspended:1;
+	struct mutex		lock;
+	bool			stopped;	/* P: lock */
+	bool			disabled;	/* P: lock */
+	bool			suspended;	/* P: lock */
 
 	int			(*filter)(void *data, int data_idx, int *val);
 	void			*filter_data;
@@ -165,7 +166,7 @@ struct ads7846 {
 #define	ADS_12_BIT		(0 << 3)
 #define	ADS_SER			(1 << 2)	/* non-differential */
 #define	ADS_DFR			(0 << 2)	/* differential */
-#define	ADS_PD10_PDOWN		(0 << 0)	/* lowpower mode + penirq */
+#define	ADS_PD10_PDOWN		(0 << 0)	/* low power mode + penirq */
 #define	ADS_PD10_ADC_ON		(1 << 0)	/* ADC on */
 #define	ADS_PD10_REF_ON		(2 << 0)	/* vREF on + penirq */
 #define	ADS_PD10_ALL_ON		(3 << 0)	/* ADC + vREF on */
@@ -193,6 +194,72 @@ struct ads7846 {
 #define	REF_ON	(READ_12BIT_DFR(x, 1, 1))
 #define	REF_OFF	(READ_12BIT_DFR(y, 0, 0))
 
+/* Must be called with ts->lock held */
+static void ads7846_stop(struct ads7846 *ts)
+{
+	if (!ts->disabled && !ts->suspended) {
+		/* Signal IRQ thread to stop polling and disable the handler. */
+		ts->stopped = true;
+		mb();
+		wake_up(&ts->wait);
+		disable_irq(ts->spi->irq);
+	}
+}
+
+/* Must be called with ts->lock held */
+static void ads7846_restart(struct ads7846 *ts)
+{
+	if (!ts->disabled && !ts->suspended) {
+		/* Tell IRQ thread that it may poll the device. */
+		ts->stopped = false;
+		mb();
+		enable_irq(ts->spi->irq);
+	}
+}
+
+/* Must be called with ts->lock held */
+static void __ads7846_disable(struct ads7846 *ts)
+{
+	ads7846_stop(ts);
+	regulator_disable(ts->reg);
+
+	/*
+	 * We know the chip's in low power mode since we always
+	 * leave it that way after every request
+	 */
+}
+
+/* Must be called with ts->lock held */
+static void __ads7846_enable(struct ads7846 *ts)
+{
+	regulator_enable(ts->reg);
+	ads7846_restart(ts);
+}
+
+static void ads7846_disable(struct ads7846 *ts)
+{
+	mutex_lock(&ts->lock);
+
+	if (!ts->disabled && !ts->suspended)
+		__ads7846_disable(ts);
+
+	ts->disabled = true;
+
+	mutex_unlock(&ts->lock);
+}
+
+static void ads7846_enable(struct ads7846 *ts)
+{
+	mutex_lock(&ts->lock);
+
+	if (ts->disabled && !ts->suspended)
+		__ads7846_enable(ts);
+
+	ts->disabled = false;
+
+	mutex_unlock(&ts->lock);
+}
+
 /*--------------------------------------------------------------------------*/
 
 /*
@@ -219,23 +286,15 @@ struct ads7845_ser_req {
 	struct spi_transfer	xfer[2];
 };
 
-static void ads7846_enable(struct ads7846 *ts);
-static void ads7846_disable(struct ads7846 *ts);
-
-static int device_suspended(struct device *dev)
-{
-	struct ads7846 *ts = dev_get_drvdata(dev);
-	return ts->is_suspended || ts->disabled;
-}
-
 static int ads7846_read12_ser(struct device *dev, unsigned command)
 {
-	struct spi_device	*spi = to_spi_device(dev);
-	struct ads7846		*ts = dev_get_drvdata(dev);
-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status;
-	int			use_internal;
+	struct spi_device *spi = to_spi_device(dev);
+	struct ads7846 *ts = dev_get_drvdata(dev);
+	struct ser_req *req;
+	int status;
+	int use_internal;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -282,11 +341,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 	CS_CHANGE(req->xfer[5]);
 	spi_message_add_tail(&req->xfer[5], &req->msg);
 
-	ts->irq_disabled = 1;
-	disable_irq(spi->irq);
+	mutex_lock(&ts->lock);
+	ads7846_stop(ts);
 	status = spi_sync(spi, &req->msg);
-	ts->irq_disabled = 0;
-	enable_irq(spi->irq);
+	ads7846_restart(ts);
+	mutex_unlock(&ts->lock);
 
 	if (status == 0) {
 		/* on-wire is a must-ignore bit, a BE12 value, then padding */
@@ -301,11 +360,12 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 
 static int ads7845_read12_ser(struct device *dev, unsigned command)
 {
-	struct spi_device	*spi = to_spi_device(dev);
-	struct ads7846		*ts = dev_get_drvdata(dev);
-	struct ads7845_ser_req	*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status;
+	struct spi_device *spi = to_spi_device(dev);
+	struct ads7846 *ts = dev_get_drvdata(dev);
+	struct ads7845_ser_req *req;
+	int status;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -317,11 +377,11 @@ static int ads7845_read12_ser(struct device *dev, unsigned command)
 	req->xfer[0].len = 3;
 	spi_message_add_tail(&req->xfer[0], &req->msg);
 
-	ts->irq_disabled = 1;
-	disable_irq(spi->irq);
+	mutex_lock(&ts->lock);
+	ads7846_stop(ts);
 	status = spi_sync(spi, &req->msg);
-	ts->irq_disabled = 0;
-	enable_irq(spi->irq);
+	ads7846_restart(ts);
+	mutex_unlock(&ts->lock);
 
 	if (status == 0) {
 		/* BE12 value, then padding */
@@ -374,6 +434,7 @@ static inline unsigned vaux_adjust(struct ads7846 *ts, ssize_t v)
 	/* external resistors may scale vAUX into 0..vREF */
 	retval *= ts->vref_mv;
 	retval = retval >> 12;
+
 	return retval;
 }
 
@@ -384,13 +445,13 @@ static inline unsigned vbatt_adjust(struct ads7846 *ts, ssize_t v)
 	/* ads7846 has a resistor ladder to scale this signal down */
 	if (ts->model == 7846)
 		retval *= 4;
+
 	return retval;
 }
 
 SHOW(in0_input, vaux, vaux_adjust)
 SHOW(in1_input, vbatt, vbatt_adjust)
 
-
 static struct attribute *ads7846_attributes[] = {
 	&dev_attr_temp0.attr,
 	&dev_attr_temp1.attr,
@@ -498,17 +559,12 @@ static inline void ads784x_hwmon_unregister(struct spi_device *spi,
 }
 #endif
 
-static int is_pen_down(struct device *dev)
-{
-	struct ads7846	*ts = dev_get_drvdata(dev);
-
-	return ts->pendown;
-}
-
 static ssize_t ads7846_pen_down_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%u\n", is_pen_down(dev));
+	struct ads7846 *ts = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", ts->pendown);
 }
 
 static DEVICE_ATTR(pen_down, S_IRUGO, ads7846_pen_down_show, NULL);
@@ -516,7 +572,7 @@ static DEVICE_ATTR(pen_down, S_IRUGO, ads7846_pen_down_show, NULL);
 static ssize_t ads7846_disable_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct ads7846	*ts = dev_get_drvdata(dev);
+	struct ads7846 *ts = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", ts->disabled);
 }
@@ -531,15 +587,11 @@ static ssize_t ads7846_disable_store(struct device *dev,
 	if (strict_strtoul(buf, 10, &i))
 		return -EINVAL;
 
-	spin_lock_irq(&ts->lock);
-
 	if (i)
 		ads7846_disable(ts);
 	else
 		ads7846_enable(ts);
 
-	spin_unlock_irq(&ts->lock);
-
 	return count;
 }
 
@@ -569,23 +621,138 @@ static void null_wait_for_sync(void)
 {
 }
 
-/*
- * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
- * to retrieve touchscreen status.
- *
- * The SPI transfer completion callback does the real work.  It reports
- * touchscreen events and reactivates the timer (or IRQ) as appropriate.
- */
+static int ads7846_debounce_filter(void *ads, int data_idx, int *val)
+{
+	struct ads7846 *ts = ads;
+
+	if (!ts->read_cnt || (abs(ts->last_read - *val) > ts->debounce_tol)) {
+		/* Start over collecting consistent readings. */
+		ts->read_rep = 0;
+		/*
+		 * Repeat it, if this was the first read or the read
+		 * wasn't consistent enough.
+		 */
+		if (ts->read_cnt < ts->debounce_max) {
+			ts->last_read = *val;
+			ts->read_cnt++;
+			return ADS7846_FILTER_REPEAT;
+		} else {
+			/*
+			 * Maximum number of debouncing reached and still
+			 * not enough number of consistent readings. Abort
+			 * the whole sample, repeat it in the next sampling
+			 * period.
+			 */
+			ts->read_cnt = 0;
+			return ADS7846_FILTER_IGNORE;
+		}
+	} else {
+		if (++ts->read_rep > ts->debounce_rep) {
+			/*
+			 * Got a good reading for this coordinate,
+			 * go for the next one.
+			 */
+			ts->read_cnt = 0;
+			ts->read_rep = 0;
+			return ADS7846_FILTER_OK;
+		} else {
+			/* Read more values that are consistent. */
+			ts->read_cnt++;
+			return ADS7846_FILTER_REPEAT;
+		}
+	}
+}
+
+static int ads7846_no_filter(void *ads, int data_idx, int *val)
+{
+	return ADS7846_FILTER_OK;
+}
+
+static int ads7846_get_value(struct ads7846 *ts, struct spi_message *m)
+{
+	struct spi_transfer *t =
+		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
+
+	if (ts->model == 7845) {
+		return be16_to_cpup((__be16 *)&(((char*)t->rx_buf)[1])) >> 3;
+	} else {
+		/*
+		 * adjust:  on-wire is a must-ignore bit, a BE12 value, then
+		 * padding; built from two 8 bit values written msb-first.
+		 */
+		return be16_to_cpup((__be16 *)t->rx_buf) >> 3;
+	}
+}
+
+static void ads7846_update_value(struct spi_message *m, int val)
+{
+	struct spi_transfer *t =
+		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
+
+	*(u16 *)t->rx_buf = val;
+}
+
+static void ads7846_read_state(struct ads7846 *ts)
+{
+	struct ads7846_packet *packet = ts->packet;
+	struct spi_message *m;
+	int msg_idx = 0;
+	int val;
+	int action;
+	int error;
+
+	while (msg_idx < ts->msg_count) {
+
+		ts->wait_for_sync();
+
+		m = &ts->msg[msg_idx];
+		error = spi_sync(ts->spi, m);
+		if (error) {
+			dev_err(&ts->spi->dev, "spi_async --> %d\n", error);
+			packet->tc.ignore = true;
+			return;
+		}
+
+		/*
+		 * Last message is power down request, no need to convert
+		 * or filter the value.
+		 */
+		if (msg_idx < ts->msg_count - 1) {
+
+			val = ads7846_get_value(ts, m);
+
+			action = ts->filter(ts->filter_data, msg_idx, &val);
+			switch (action) {
+			case ADS7846_FILTER_REPEAT:
+				continue;
+
+			case ADS7846_FILTER_IGNORE:
+				packet->tc.ignore = true;
+				return;
 
-static void ads7846_rx(void *ads)
+			case ADS7846_FILTER_OK:
+				ads7846_update_value(m, val);
+				packet->tc.ignore = false;
+				msg_idx++;
+				break;
+
+			default:
+				BUG();
+			}
+		}
+	}
+}
+
+static void ads7846_report_state(struct ads7846 *ts)
 {
-	struct ads7846		*ts = ads;
-	struct ads7846_packet	*packet = ts->packet;
-	unsigned		Rt;
-	u16			x, y, z1, z2;
+	struct ads7846_packet *packet = ts->packet;
+	unsigned int Rt;
+	u16 x, y, z1, z2;
 
-	/* ads7846_rx_val() did in-place conversion (including byteswap) from
-	 * on-the-wire format as part of debouncing to get stable readings.
+	/*
+	 * ads7846_get_value() does in-place conversion (including byte swap)
+	 * from on-the-wire format as part of debouncing to get stable
+	 * readings.
 	 */
 	if (ts->model == 7845) {
 		x = *(u16 *)packet->tc.x_buf;
@@ -623,19 +790,19 @@ static void ads7846_rx(void *ads)
 		Rt = 0;
 	}
 
-	/* Sample found inconsistent by debouncing or pressure is beyond
+	/*
+	 * Sample found inconsistent by debouncing or pressure is beyond
 	 * the maximum. Don't report it to user space, repeat at least
 	 * once more the measurement
 	 */
 	if (packet->tc.ignore || Rt > ts->pressure_max) {
 		dev_vdbg(&ts->spi->dev, "ignored %d pressure %d\n",
 			 packet->tc.ignore, Rt);
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			      HRTIMER_MODE_REL);
 		return;
 	}
 
-	/* Maybe check the pendown state before reporting. This discards
+	/*
+	 * Maybe check the pendown state before reporting. This discards
 	 * false readings when the pen is lifted.
 	 */
 	if (ts->penirq_recheck_delay_usecs) {
@@ -644,8 +811,9 @@ static void ads7846_rx(void *ads)
 			Rt = 0;
 	}
 
-	/* NOTE: We can't rely on the pressure to determine the pen down
-	 * state, even this controller has a pressure sensor.  The pressure
+	/*
+	 * NOTE: We can't rely on the pressure to determine the pen down
+	 * state, even this controller has a pressure sensor. The pressure
 	 * value can fluctuate for quite a while after lifting the pen and
 	 * in some cases may not even settle at the expected value.
 	 *
@@ -655,15 +823,15 @@ static void ads7846_rx(void *ads)
 	if (Rt) {
 		struct input_dev *input = ts->input;
 
+		if (ts->swap_xy)
+			swap(x, y);
+
 		if (!ts->pendown) {
 			input_report_key(input, BTN_TOUCH, 1);
-			ts->pendown = 1;
+			ts->pendown = true;
 			dev_vdbg(&ts->spi->dev, "DOWN\n");
 		}
 
-		if (ts->swap_xy)
-			swap(x, y);
-
 		input_report_abs(input, ABS_X, x);
 		input_report_abs(input, ABS_Y, y);
 		input_report_abs(input, ABS_PRESSURE, ts->pressure_max - Rt);
@@ -671,246 +839,94 @@ static void ads7846_rx(void *ads)
 		input_sync(input);
 		dev_vdbg(&ts->spi->dev, "%4d/%4d/%4d\n", x, y, Rt);
 	}
-
-	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			HRTIMER_MODE_REL);
 }
 
-static int ads7846_debounce(void *ads, int data_idx, int *val)
+static irqreturn_t ads7846_hard_irq(int irq, void *handle)
 {
-	struct ads7846		*ts = ads;
-
-	if (!ts->read_cnt || (abs(ts->last_read - *val) > ts->debounce_tol)) {
-		/* Start over collecting consistent readings. */
-		ts->read_rep = 0;
-		/* Repeat it, if this was the first read or the read
-		 * wasn't consistent enough. */
-		if (ts->read_cnt < ts->debounce_max) {
-			ts->last_read = *val;
-			ts->read_cnt++;
-			return ADS7846_FILTER_REPEAT;
-		} else {
-			/* Maximum number of debouncing reached and still
-			 * not enough number of consistent readings. Abort
-			 * the whole sample, repeat it in the next sampling
-			 * period.
-			 */
-			ts->read_cnt = 0;
-			return ADS7846_FILTER_IGNORE;
-		}
-	} else {
-		if (++ts->read_rep > ts->debounce_rep) {
-			/* Got a good reading for this coordinate,
-			 * go for the next one. */
-			ts->read_cnt = 0;
-			ts->read_rep = 0;
-			return ADS7846_FILTER_OK;
-		} else {
-			/* Read more values that are consistent. */
-			ts->read_cnt++;
-			return ADS7846_FILTER_REPEAT;
-		}
-	}
-}
-
-static int ads7846_no_filter(void *ads, int data_idx, int *val)
-{
-	return ADS7846_FILTER_OK;
-}
-
-static void ads7846_rx_val(void *ads)
-{
-	struct ads7846 *ts = ads;
-	struct ads7846_packet *packet = ts->packet;
-	struct spi_message *m;
-	struct spi_transfer *t;
-	int val;
-	int action;
-	int status;
-
-	m = &ts->msg[ts->msg_idx];
-	t = list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
-
-	if (ts->model == 7845) {
-		val = be16_to_cpup((__be16 *)&(((char*)t->rx_buf)[1])) >> 3;
-	} else {
-		/* adjust:  on-wire is a must-ignore bit, a BE12 value, then
-		 * padding; built from two 8 bit values written msb-first.
-		 */
-		val = be16_to_cpup((__be16 *)t->rx_buf) >> 3;
-	}
+	struct ads7846 *ts = handle;
 
-	action = ts->filter(ts->filter_data, ts->msg_idx, &val);
-	switch (action) {
-	case ADS7846_FILTER_REPEAT:
-		break;
-	case ADS7846_FILTER_IGNORE:
-		packet->tc.ignore = 1;
-		/* Last message will contain ads7846_rx() as the
-		 * completion function.
-		 */
-		m = ts->last_msg;
-		break;
-	case ADS7846_FILTER_OK:
-		*(u16 *)t->rx_buf = val;
-		packet->tc.ignore = 0;
-		m = &ts->msg[++ts->msg_idx];
-		break;
-	default:
-		BUG();
-	}
-	ts->wait_for_sync();
-	status = spi_async(ts->spi, m);
-	if (status)
-		dev_err(&ts->spi->dev, "spi_async --> %d\n",
-				status);
+	return get_pendown_state(ts) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
 }
 
-static enum hrtimer_restart ads7846_timer(struct hrtimer *handle)
-{
-	struct ads7846	*ts = container_of(handle, struct ads7846, timer);
-	int		status = 0;
-
-	spin_lock(&ts->lock);
-
-	if (unlikely(!get_pendown_state(ts) ||
-		     device_suspended(&ts->spi->dev))) {
-		if (ts->pendown) {
-			struct input_dev *input = ts->input;
-
-			input_report_key(input, BTN_TOUCH, 0);
-			input_report_abs(input, ABS_PRESSURE, 0);
-			input_sync(input);
-
-			ts->pendown = 0;
-			dev_vdbg(&ts->spi->dev, "UP\n");
-		}
-
-		/* measurement cycle ended */
-		if (!device_suspended(&ts->spi->dev)) {
-			ts->irq_disabled = 0;
-			enable_irq(ts->spi->irq);
-		}
-		ts->pending = 0;
-	} else {
-		/* pen is still down, continue with the measurement */
-		ts->msg_idx = 0;
-		ts->wait_for_sync();
-		status = spi_async(ts->spi, &ts->msg[0]);
-		if (status)
-			dev_err(&ts->spi->dev, "spi_async --> %d\n", status);
-	}
-
-	spin_unlock(&ts->lock);
-	return HRTIMER_NORESTART;
-}
 
 static irqreturn_t ads7846_irq(int irq, void *handle)
 {
 	struct ads7846 *ts = handle;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ts->lock, flags);
-	if (likely(get_pendown_state(ts))) {
-		if (!ts->irq_disabled) {
-			/* The ARM do_simple_IRQ() dispatcher doesn't act
-			 * like the other dispatchers:  it will report IRQs
-			 * even after they've been disabled.  We work around
-			 * that here.  (The "generic irq" framework may help...)
-			 */
-			ts->irq_disabled = 1;
-			disable_irq_nosync(ts->spi->irq);
-			ts->pending = 1;
-			hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
-					HRTIMER_MODE_REL);
-		}
-	}
-	spin_unlock_irqrestore(&ts->lock, flags);
 
-	return IRQ_HANDLED;
-}
+	/* Start with a small delay before checking pendown state */
+	msleep(TS_POLL_DELAY);
 
-/*--------------------------------------------------------------------------*/
+	while (!ts->stopped && get_pendown_state(ts)) {
 
-/* Must be called with ts->lock held */
-static void ads7846_disable(struct ads7846 *ts)
-{
-	if (ts->disabled)
-		return;
+		/* pen is down, continue with the measurement */
+		ads7846_read_state(ts);
 
-	ts->disabled = 1;
+		if (!ts->stopped)
+			ads7846_report_state(ts);
 
-	/* are we waiting for IRQ, or polling? */
-	if (!ts->pending) {
-		ts->irq_disabled = 1;
-		disable_irq(ts->spi->irq);
-	} else {
-		/* the timer will run at least once more, and
-		 * leave everything in a clean state, IRQ disabled
-		 */
-		while (ts->pending) {
-			spin_unlock_irq(&ts->lock);
-			msleep(1);
-			spin_lock_irq(&ts->lock);
-		}
+		wait_event_timeout(ts->wait, ts->stopped,
+				   msecs_to_jiffies(TS_POLL_PERIOD));
 	}
 
-	regulator_disable(ts->reg);
-
-	/* we know the chip's in lowpower mode since we always
-	 * leave it that way after every request
-	 */
-}
+	if (ts->pendown) {
+		struct input_dev *input = ts->input;
 
-/* Must be called with ts->lock held */
-static void ads7846_enable(struct ads7846 *ts)
-{
-	if (!ts->disabled)
-		return;
+		input_report_key(input, BTN_TOUCH, 0);
+		input_report_abs(input, ABS_PRESSURE, 0);
+		input_sync(input);
 
-	regulator_enable(ts->reg);
+		ts->pendown = false;
+		dev_vdbg(&ts->spi->dev, "UP\n");
+	}
 
-	ts->disabled = 0;
-	ts->irq_disabled = 0;
-	enable_irq(ts->spi->irq);
+	return IRQ_HANDLED;
 }
 
 static int ads7846_suspend(struct spi_device *spi, pm_message_t message)
 {
 	struct ads7846 *ts = dev_get_drvdata(&spi->dev);
 
-	spin_lock_irq(&ts->lock);
+	mutex_lock(&ts->lock);
 
-	ts->is_suspended = 1;
-	ads7846_disable(ts);
+	if (!ts->suspended) {
 
-	spin_unlock_irq(&ts->lock);
+		if (!ts->disabled)
+			__ads7846_disable(ts);
 
-	if (device_may_wakeup(&ts->spi->dev))
-		enable_irq_wake(ts->spi->irq);
+		if (device_may_wakeup(&ts->spi->dev))
+			enable_irq_wake(ts->spi->irq);
 
-	return 0;
+		ts->suspended = true;
+	}
+
+	mutex_unlock(&ts->lock);
 
+	return 0;
 }
 
 static int ads7846_resume(struct spi_device *spi)
 {
 	struct ads7846 *ts = dev_get_drvdata(&spi->dev);
 
-	if (device_may_wakeup(&ts->spi->dev))
-		disable_irq_wake(ts->spi->irq);
+	mutex_lock(&ts->lock);
 
-	spin_lock_irq(&ts->lock);
+	if (ts->suspended) {
 
-	ts->is_suspended = 0;
-	ads7846_enable(ts);
+		if (device_may_wakeup(&ts->spi->dev))
+			disable_irq_wake(ts->spi->irq);
 
-	spin_unlock_irq(&ts->lock);
+		if (!ts->disabled)
+			__ads7846_enable(ts);
+
+		ts->suspended = false;
+	}
+
+	mutex_unlock(&ts->lock);
 
 	return 0;
 }
 
-static int __devinit setup_pendown(struct spi_device *spi, struct ads7846 *ts)
+static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads7846 *ts)
 {
 	struct ads7846_platform_data *pdata = spi->dev.platform_data;
 	int err;
@@ -932,146 +948,40 @@ static int __devinit setup_pendown(struct spi_device *spi, struct ads7846 *ts)
 	err = gpio_request(pdata->gpio_pendown, "ads7846_pendown");
 	if (err) {
 		dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
-				pdata->gpio_pendown);
+			pdata->gpio_pendown);
 		return err;
 	}
 
 	ts->gpio_pendown = pdata->gpio_pendown;
+
 	return 0;
 }
 
-static int __devinit ads7846_probe(struct spi_device *spi)
+/*
+ * Set up the transfers to read touchscreen state; this assumes we
+ * use formula #2 for pressure, not #3.
+ */
+static void __devinit ads7846_setup_spi_msg(struct ads7846 *ts,
+				const struct ads7846_platform_data *pdata)
 {
-	struct ads7846 *ts;
-	struct ads7846_packet *packet;
-	struct input_dev *input_dev;
-	const struct ads7846_platform_data *pdata = spi->dev.platform_data;
-	struct spi_message *m;
-	struct spi_transfer *x;
-	unsigned long irq_flags;
-	int vref;
-	int err;
-
-	if (!spi->irq) {
-		dev_dbg(&spi->dev, "no IRQ?\n");
-		return -ENODEV;
-	}
-
-	if (!pdata) {
-		dev_dbg(&spi->dev, "no platform data?\n");
-		return -ENODEV;
-	}
-
-	/* don't exceed max specified sample rate */
-	if (spi->max_speed_hz > (125000 * SAMPLE_BITS)) {
-		dev_dbg(&spi->dev, "f(sample) %d KHz?\n",
-				(spi->max_speed_hz/SAMPLE_BITS)/1000);
-		return -EINVAL;
-	}
-
-	/* We'd set TX wordsize 8 bits and RX wordsize to 13 bits ... except
-	 * that even if the hardware can do that, the SPI controller driver
-	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
-	 */
-	spi->bits_per_word = 8;
-	spi->mode = SPI_MODE_0;
-	err = spi_setup(spi);
-	if (err < 0)
-		return err;
-
-	ts = kzalloc(sizeof(struct ads7846), GFP_KERNEL);
-	packet = kzalloc(sizeof(struct ads7846_packet), GFP_KERNEL);
-	input_dev = input_allocate_device();
-	if (!ts || !packet || !input_dev) {
-		err = -ENOMEM;
-		goto err_free_mem;
-	}
-
-	dev_set_drvdata(&spi->dev, ts);
-
-	ts->packet = packet;
-	ts->spi = spi;
-	ts->input = input_dev;
-	ts->vref_mv = pdata->vref_mv;
-	ts->swap_xy = pdata->swap_xy;
-
-	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	ts->timer.function = ads7846_timer;
-
-	spin_lock_init(&ts->lock);
-
-	ts->model = pdata->model ? : 7846;
-	ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
-	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
-	ts->pressure_max = pdata->pressure_max ? : ~0;
-
-	if (pdata->filter != NULL) {
-		if (pdata->filter_init != NULL) {
-			err = pdata->filter_init(pdata, &ts->filter_data);
-			if (err < 0)
-				goto err_free_mem;
-		}
-		ts->filter = pdata->filter;
-		ts->filter_cleanup = pdata->filter_cleanup;
-	} else if (pdata->debounce_max) {
-		ts->debounce_max = pdata->debounce_max;
-		if (ts->debounce_max < 2)
-			ts->debounce_max = 2;
-		ts->debounce_tol = pdata->debounce_tol;
-		ts->debounce_rep = pdata->debounce_rep;
-		ts->filter = ads7846_debounce;
-		ts->filter_data = ts;
-	} else
-		ts->filter = ads7846_no_filter;
-
-	err = setup_pendown(spi, ts);
-	if (err)
-		goto err_cleanup_filter;
-
-	if (pdata->penirq_recheck_delay_usecs)
-		ts->penirq_recheck_delay_usecs =
-				pdata->penirq_recheck_delay_usecs;
-
-	ts->wait_for_sync = pdata->wait_for_sync ? : null_wait_for_sync;
-
-	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
-	snprintf(ts->name, sizeof(ts->name), "ADS%d Touchscreen", ts->model);
-
-	input_dev->name = ts->name;
-	input_dev->phys = ts->phys;
-	input_dev->dev.parent = &spi->dev;
-
-	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
-	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
-	input_set_abs_params(input_dev, ABS_X,
-			pdata->x_min ? : 0,
-			pdata->x_max ? : MAX_12BIT,
-			0, 0);
-	input_set_abs_params(input_dev, ABS_Y,
-			pdata->y_min ? : 0,
-			pdata->y_max ? : MAX_12BIT,
-			0, 0);
-	input_set_abs_params(input_dev, ABS_PRESSURE,
-			pdata->pressure_min, pdata->pressure_max, 0, 0);
-
-	vref = pdata->keep_vref_on;
+	struct spi_message *m = &ts->msg[0];
+	struct spi_transfer *x = ts->xfer;
+	struct ads7846_packet *packet = ts->packet;
+	int vref = pdata->keep_vref_on;
 
 	if (ts->model == 7873) {
-		/* The AD7873 is almost identical to the ADS7846
+		/*
+		 * The AD7873 is almost identical to the ADS7846
 		 * keep VREF off during differential/ratiometric
-		 * conversion modes
+		 * conversion modes.
 		 */
 		ts->model = 7846;
 		vref = 0;
 	}
 
-	/* set up the transfers to read touchscreen state; this assumes we
-	 * use formula #2 for pressure, not #3.
-	 */
-	m = &ts->msg[0];
-	x = ts->xfer;
-
+	ts->msg_count = 1;
 	spi_message_init(m);
+	m->context = ts;
 
 	if (ts->model == 7845) {
 		packet->read_y_cmd[0] = READ_Y(vref);
@@ -1094,7 +1004,8 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		spi_message_add_tail(x, m);
 	}
 
-	/* the first sample after switching drivers can be low quality;
+	/*
+	 * The first sample after switching drivers can be low quality;
 	 * optionally discard it, using a second one after the signals
 	 * have had enough time to stabilize.
 	 */
@@ -1112,11 +1023,10 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		spi_message_add_tail(x, m);
 	}
 
-	m->complete = ads7846_rx_val;
-	m->context = ts;
-
+	ts->msg_count++;
 	m++;
 	spi_message_init(m);
+	m->context = ts;
 
 	if (ts->model == 7845) {
 		x++;
@@ -1156,13 +1066,12 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		spi_message_add_tail(x, m);
 	}
 
-	m->complete = ads7846_rx_val;
-	m->context = ts;
-
 	/* turn y+ off, x- on; we'll use formula #2 */
 	if (ts->model == 7846) {
+		ts->msg_count++;
 		m++;
 		spi_message_init(m);
+		m->context = ts;
 
 		x++;
 		packet->read_z1 = READ_Z1(vref);
@@ -1190,11 +1099,10 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 			spi_message_add_tail(x, m);
 		}
 
-		m->complete = ads7846_rx_val;
-		m->context = ts;
-
+		ts->msg_count++;
 		m++;
 		spi_message_init(m);
+		m->context = ts;
 
 		x++;
 		packet->read_z2 = READ_Z2(vref);
@@ -1221,14 +1129,13 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 			x->len = 2;
 			spi_message_add_tail(x, m);
 		}
-
-		m->complete = ads7846_rx_val;
-		m->context = ts;
 	}
 
 	/* power down */
+	ts->msg_count++;
 	m++;
 	spi_message_init(m);
+	m->context = ts;
 
 	if (ts->model == 7845) {
 		x++;
@@ -1251,11 +1158,119 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 
 	CS_CHANGE(*x);
 	spi_message_add_tail(x, m);
+}
 
-	m->complete = ads7846_rx;
-	m->context = ts;
+static int __devinit ads7846_probe(struct spi_device *spi)
+{
+	struct ads7846 *ts;
+	struct ads7846_packet *packet;
+	struct input_dev *input_dev;
+	struct ads7846_platform_data *pdata = spi->dev.platform_data;
+	unsigned long irq_flags;
+	int err;
 
-	ts->last_msg = m;
+	if (!spi->irq) {
+		dev_dbg(&spi->dev, "no IRQ?\n");
+		return -ENODEV;
+	}
+
+	if (!pdata) {
+		dev_dbg(&spi->dev, "no platform data?\n");
+		return -ENODEV;
+	}
+
+	/* don't exceed max specified sample rate */
+	if (spi->max_speed_hz > (125000 * SAMPLE_BITS)) {
+		dev_dbg(&spi->dev, "f(sample) %d KHz?\n",
+				(spi->max_speed_hz/SAMPLE_BITS)/1000);
+		return -EINVAL;
+	}
+
+	/* We'd set TX word size 8 bits and RX word size to 13 bits ... except
+	 * that even if the hardware can do that, the SPI controller driver
+	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
+	 */
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_0;
+	err = spi_setup(spi);
+	if (err < 0)
+		return err;
+
+	ts = kzalloc(sizeof(struct ads7846), GFP_KERNEL);
+	packet = kzalloc(sizeof(struct ads7846_packet), GFP_KERNEL);
+	input_dev = input_allocate_device();
+	if (!ts || !packet || !input_dev) {
+		err = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	dev_set_drvdata(&spi->dev, ts);
+
+	ts->packet = packet;
+	ts->spi = spi;
+	ts->input = input_dev;
+	ts->vref_mv = pdata->vref_mv;
+	ts->swap_xy = pdata->swap_xy;
+
+	mutex_init(&ts->lock);
+	init_waitqueue_head(&ts->wait);
+
+	ts->model = pdata->model ? : 7846;
+	ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
+	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
+	ts->pressure_max = pdata->pressure_max ? : ~0;
+
+	if (pdata->filter != NULL) {
+		if (pdata->filter_init != NULL) {
+			err = pdata->filter_init(pdata, &ts->filter_data);
+			if (err < 0)
+				goto err_free_mem;
+		}
+		ts->filter = pdata->filter;
+		ts->filter_cleanup = pdata->filter_cleanup;
+	} else if (pdata->debounce_max) {
+		ts->debounce_max = pdata->debounce_max;
+		if (ts->debounce_max < 2)
+			ts->debounce_max = 2;
+		ts->debounce_tol = pdata->debounce_tol;
+		ts->debounce_rep = pdata->debounce_rep;
+		ts->filter = ads7846_debounce_filter;
+		ts->filter_data = ts;
+	} else {
+		ts->filter = ads7846_no_filter;
+	}
+
+	err = ads7846_setup_pendown(spi, ts);
+	if (err)
+		goto err_cleanup_filter;
+
+	if (pdata->penirq_recheck_delay_usecs)
+		ts->penirq_recheck_delay_usecs =
+				pdata->penirq_recheck_delay_usecs;
+
+	ts->wait_for_sync = pdata->wait_for_sync ? : null_wait_for_sync;
+
+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
+	snprintf(ts->name, sizeof(ts->name), "ADS%d Touchscreen", ts->model);
+
+	input_dev->name = ts->name;
+	input_dev->phys = ts->phys;
+	input_dev->dev.parent = &spi->dev;
+
+	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+	input_set_abs_params(input_dev, ABS_X,
+			pdata->x_min ? : 0,
+			pdata->x_max ? : MAX_12BIT,
+			0, 0);
+	input_set_abs_params(input_dev, ABS_Y,
+			pdata->y_min ? : 0,
+			pdata->y_max ? : MAX_12BIT,
+			0, 0);
+	input_set_abs_params(input_dev, ABS_PRESSURE,
+			pdata->pressure_min, pdata->pressure_max, 0, 0);
+
+	ads7846_setup_spi_msg(ts, pdata);
 
 	ts->reg = regulator_get(&spi->dev, "vcc");
 	if (IS_ERR(ts->reg)) {
@@ -1271,16 +1286,17 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	}
 
 	irq_flags = pdata->irq_flags ? : IRQF_TRIGGER_FALLING;
+	irq_flags |= IRQF_ONESHOT;
 
-	err = request_irq(spi->irq, ads7846_irq, irq_flags,
-			  spi->dev.driver->name, ts);
-
+	err = request_threaded_irq(spi->irq, ads7846_hard_irq, ads7846_irq,
+				   irq_flags, spi->dev.driver->name, ts);
 	if (err && !pdata->irq_flags) {
 		dev_info(&spi->dev,
 			"trying pin change workaround on irq %d\n", spi->irq);
-		err = request_irq(spi->irq, ads7846_irq,
-				  IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
-				  spi->dev.driver->name, ts);
+		irq_flags |= IRQF_TRIGGER_RISING;
+		err = request_threaded_irq(spi->irq,
+				  ads7846_hard_irq, ads7846_irq,
+				  irq_flags, spi->dev.driver->name, ts);
 	}
 
 	if (err) {
@@ -1294,7 +1310,8 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 
 	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
 
-	/* take a first sample, leaving nPENIRQ active and vREF off; avoid
+	/*
+	 * Take a first sample, leaving nPENIRQ active and vREF off; avoid
 	 * the touchscreen, in case it's not connected.
 	 */
 	if (ts->model == 7845)
@@ -1340,20 +1357,18 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 
 static int __devexit ads7846_remove(struct spi_device *spi)
 {
-	struct ads7846		*ts = dev_get_drvdata(&spi->dev);
+	struct ads7846 *ts = dev_get_drvdata(&spi->dev);
 
 	device_init_wakeup(&spi->dev, false);
 
-	ads784x_hwmon_unregister(spi, ts);
-	input_unregister_device(ts->input);
-
-	ads7846_suspend(spi, PMSG_SUSPEND);
-
 	sysfs_remove_group(&spi->dev.kobj, &ads784x_attr_group);
 
+	ads7846_disable(ts);
 	free_irq(ts->spi->irq, ts);
-	/* suspend left the IRQ disabled */
-	enable_irq(ts->spi->irq);
+
+	input_unregister_device(ts->input);
+
+	ads784x_hwmon_unregister(spi, ts);
 
 	regulator_disable(ts->reg);
 	regulator_put(ts->reg);
@@ -1368,6 +1383,7 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 	kfree(ts);
 
 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
+
 	return 0;
 }
 
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/4] Input: ads7846 - add a include header to prevent building fails
  2010-09-16 10:51 ` [PATCH 1/4] Input: ads7846 - switch to using threaded IRQ Jason Wang
@ 2010-09-16 10:51   ` Jason Wang
  2010-09-16 10:51     ` [PATCH 3/4] Input: ads7846 - restore ADC to powerdown mode if no messgaes needed Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-09-16 10:51 UTC (permalink / raw)
  To: dmitry.torokhov, notasas, vapier; +Cc: linux-input

After we switch this driver to use thread IRQ, we introduce some
kthread macros, when we build this driver, we will get errors like
that:
ds7846.c: In function 'ads7846_stop':
ads7846.c:205: error: 'TASK_NORMAL' undeclared (first use in ...)
ads7846.c:205: error: (Each undeclared identifier is ...)
ads7846.c:205: error: for each function it appears in.)

Here add a header file to fix this error.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
---
 drivers/input/touchscreen/ads7846.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index d1fc8ef..5ddaeea 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -21,6 +21,7 @@
 #include <linux/hwmon.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/4] Input: ads7846 - restore ADC to powerdown mode if no messgaes needed
  2010-09-16 10:51   ` [PATCH 2/4] Input: ads7846 - add a include header to prevent building fails Jason Wang
@ 2010-09-16 10:51     ` Jason Wang
  2010-09-16 10:51       ` [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-09-16 10:51 UTC (permalink / raw)
  To: dmitry.torokhov, notasas, vapier; +Cc: linux-input

When the ADC get wrong datas and have to ignore remaining messages in
a transfer turn, we should set ADC to powerdown mode instead of
leaving it at previous operation mode, otherwise we can't get the
correct pendown state and the irq will not trigger any more.

After the last message is transfered, we should add msg_idx to let
the code to skip out the loop.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
---
 drivers/input/touchscreen/ads7846.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 5ddaeea..eab8b0b 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -729,7 +729,8 @@ static void ads7846_read_state(struct ads7846 *ts)
 
 			case ADS7846_FILTER_IGNORE:
 				packet->tc.ignore = true;
-				return;
+				msg_idx = ts->msg_count - 1;
+				continue;
 
 			case ADS7846_FILTER_OK:
 				ads7846_update_value(m, val);
@@ -740,6 +741,8 @@ static void ads7846_read_state(struct ads7846 *ts)
 			default:
 				BUG();
 			}
+		} else {
+			msg_idx++;
 		}
 	}
 }
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-09-16 10:51     ` [PATCH 3/4] Input: ads7846 - restore ADC to powerdown mode if no messgaes needed Jason Wang
@ 2010-09-16 10:51       ` Jason Wang
  2010-09-17  6:39         ` Dmitry Torokhov
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-09-16 10:51 UTC (permalink / raw)
  To: dmitry.torokhov, notasas, vapier; +Cc: linux-input

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.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
---
 drivers/input/touchscreen/ads7846.c |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index eab8b0b..349031d 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -198,7 +198,7 @@ struct ads7846 {
 /* Must be called with ts->lock held */
 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();
@@ -223,7 +223,7 @@ static void __ads7846_disable(struct ads7846 *ts)
 {
 	ads7846_stop(ts);
 	regulator_disable(ts->reg);
-
+	ts->disabled = true;
 	/*
 	 * We know the chip's in low power mode since we always
 	 * leave it that way after every request
@@ -235,6 +235,7 @@ static void __ads7846_enable(struct ads7846 *ts)
 {
 	regulator_enable(ts->reg);
 	ads7846_restart(ts);
+	ts->disabled = false;
 }
 
 static void ads7846_disable(struct ads7846 *ts)
@@ -244,8 +245,6 @@ static void ads7846_disable(struct ads7846 *ts)
 	if (!ts->disabled && !ts->suspended)
 		__ads7846_disable(ts);
 
-	ts->disabled = true;
-
 	mutex_unlock(&ts->lock);
 }
 
@@ -256,8 +255,6 @@ static void ads7846_enable(struct ads7846 *ts)
 	if (ts->disabled && !ts->suspended)
 		__ads7846_enable(ts);
 
-	ts->disabled = false;
-
 	mutex_unlock(&ts->lock);
 }
 
@@ -919,7 +916,7 @@ static int ads7846_resume(struct spi_device *spi)
 		if (device_may_wakeup(&ts->spi->dev))
 			disable_irq_wake(ts->spi->irq);
 
-		if (!ts->disabled)
+		if (ts->disabled)
 			__ads7846_enable(ts);
 
 		ts->suspended = false;
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 0/4] Switch ads7846 driver to use soft irq
  2010-09-16 10:51 [PATCH 0/4] Switch ads7846 driver to use soft irq Jason Wang
  2010-09-16 10:51 ` [PATCH 1/4] Input: ads7846 - switch to using threaded IRQ Jason Wang
@ 2010-09-17  1:48 ` Jason Wang
  1 sibling, 0 replies; 19+ messages in thread
From: Jason Wang @ 2010-09-17  1:48 UTC (permalink / raw)
  To: Jason Wang; +Cc: dmitry.torokhov, notasas, vapier, linux-input

Sorry for using soft irq here, should be *thread irq*.


Jason Wang wrote:
> Some explanations:
>
> The first patch is from dmitry, he build a frame work for this driver
> to use soft irq. The remaining 3 patches are from me to fix some issues
> for the driver of soft irq verison.
>
> We change this driver to use soft irq because under current version,
> we must use spin lock to protect racing issues, the regulator
> operations are also in the racing issue list but we can't put it
> in the spin lock protected area because regulator operations always
> implement via i2c/spi transfers, in those transfers often call sleep
> funcitons. Now change to use soft irq, we can use mutex instread of
> spin lock, so this issue is solved.
>
> I have tested these patches on the ti_omap3530evm board:
>  1)use ts_lib after normal boot
>  2)use ts_lib after "#echo 1/0 > /sys/bus/spi/devices/spi0.1/disable"
>  3)use ts_lib after "#echo mem > /sys/power/state" and "wake up"
>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-input" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>
>   


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-09-16 10:51       ` [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions Jason Wang
@ 2010-09-17  6:39         ` Dmitry Torokhov
  2010-09-17  9:20           ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2010-09-17  6:39 UTC (permalink / raw)
  To: Jason Wang; +Cc: notasas, vapier, linux-input

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.

-- 
Dmitry

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-09-17  6:39         ` Dmitry Torokhov
@ 2010-09-17  9:20           ` Jason Wang
  2010-09-17 16:07             ` Dmitry Torokhov
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-09-17  9:20 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Jason Wang, notasas, vapier, linux-input

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.

Thanks,
Jason.


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-09-17  9:20           ` Jason Wang
@ 2010-09-17 16:07             ` Dmitry Torokhov
  2010-09-20  8:18               ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2010-09-17 16:07 UTC (permalink / raw)
  To: Jason Wang; +Cc: notasas, vapier, linux-input

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!

-- 
Dmitry

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-09-17 16:07             ` Dmitry Torokhov
@ 2010-09-20  8:18               ` Jason Wang
  2010-10-12  9:58                 ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-09-20  8:18 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Jason Wang, notasas, vapier, linux-input

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.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-09-20  8:18               ` Jason Wang
@ 2010-10-12  9:58                 ` Jason Wang
  2010-10-12 16:00                   ` Dmitry Torokhov
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-10-12  9:58 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: notasas, vapier, linux-input

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@gmail.com>
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@gmail.com>
---
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.


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-12  9:58                 ` Jason Wang
@ 2010-10-12 16:00                   ` Dmitry Torokhov
  2010-10-13  3:12                     ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Dmitry Torokhov @ 2010-10-12 16:00 UTC (permalink / raw)
  To: Jason Wang; +Cc: notasas, vapier, linux-input

Hi Jason.

On Tue, Oct 12, 2010 at 05:58:36PM +0800, Jason Wang wrote:
> 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.

Sorry about that.

> @@ -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);

If the device was both disabled and suspended then enable will never
succeed.

I actually was looking over the patches yesterday and I believe I
corrected the problem you mentioned. I rolled all patches together (no
reason to keep them separate - they have not been yet applied to
non-rebasing branch and they are simple fix-ups to the issues introduced
by the fisrt patch). Could you please try the patch below and if it
still works I'll queue for .37.

Thanks!

-- 
Dmitry

Input: ads7846 - switch to using threaded IRQ

From: Jason Wang <jason77.wang@gmail.com>

Commit 9114337 introduces regulator operations in ads7846 touchscreen
driver. Among these operations, some are called while holding a
spinlock. On many platforms regulators reside on slow buses, such as
I2C/SPI and require sleep while accessing them.

The touchscreen itself is also a SPI device and currently relies on
asynchronous SPI access to avoid sleeping in interrupt context. Let's
switch to using threaded IRQ to be able to access SPI bus
synchronously (which simplifies driver a bit); it also allows safe
access to the regulators as well.

This has been tested on the ti_omap3530evm board:
 1) using ts_lib after normal boot
 2) using ts_lib after "#echo 1/0 > /sys/bus/spi/devices/spi0.1/disable"
 3) using ts_lib after "#echo mem > /sys/power/state" and "wake up"

Based on original patch by Dmitry Torokhov.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/ads7846.c |  886 ++++++++++++++++++-----------------
 1 files changed, 456 insertions(+), 430 deletions(-)


diff --git a/drivers/input/touchscreen/ads7846.c b/drivers/input/touchscreen/ads7846.c
index 1603193..14ea54b 100644
--- a/drivers/input/touchscreen/ads7846.c
+++ b/drivers/input/touchscreen/ads7846.c
@@ -17,9 +17,11 @@
  *  it under the terms of the GNU General Public License version 2 as
  *  published by the Free Software Foundation.
  */
+#include <linux/types.h>
 #include <linux/hwmon.h>
 #include <linux/init.h>
 #include <linux/err.h>
+#include <linux/sched.h>
 #include <linux/delay.h>
 #include <linux/input.h>
 #include <linux/interrupt.h>
@@ -52,22 +54,23 @@
  * files.
  */
 
-#define TS_POLL_DELAY	(1 * 1000000)	/* ns delay before the first sample */
-#define TS_POLL_PERIOD	(5 * 1000000)	/* ns delay between samples */
+#define TS_POLL_DELAY	1	/* ms delay before the first sample */
+#define TS_POLL_PERIOD	5	/* ms delay between samples */
 
 /* this driver doesn't aim at the peak continuous sample rate */
 #define	SAMPLE_BITS	(8 /*cmd*/ + 16 /*sample*/ + 2 /* before, after */)
 
 struct ts_event {
-	/* For portability, we can't read 12 bit values using SPI (which
-	 * would make the controller deliver them as native byteorder u16
+	/*
+	 * For portability, we can't read 12 bit values using SPI (which
+	 * would make the controller deliver them as native byte order u16
 	 * with msbs zeroed).  Instead, we read them as two 8-bit values,
 	 * *** WHICH NEED BYTESWAPPING *** and range adjustment.
 	 */
 	u16	x;
 	u16	y;
 	u16	z1, z2;
-	int	ignore;
+	bool	ignore;
 	u8	x_buf[3];
 	u8	y_buf[3];
 };
@@ -110,8 +113,11 @@ struct ads7846 {
 
 	struct spi_transfer	xfer[18];
 	struct spi_message	msg[5];
-	struct spi_message	*last_msg;
-	int			msg_idx;
+	int			msg_count;
+	wait_queue_head_t	wait;
+
+	bool			pendown;
+
 	int			read_cnt;
 	int			read_rep;
 	int			last_read;
@@ -122,14 +128,10 @@ struct ads7846 {
 
 	u16			penirq_recheck_delay_usecs;
 
-	spinlock_t		lock;
-	struct hrtimer		timer;
-	unsigned		pendown:1;	/* P: lock */
-	unsigned		pending:1;	/* P: lock */
-// FIXME remove "irq_disabled"
-	unsigned		irq_disabled:1;	/* P: lock */
-	unsigned		disabled:1;
-	unsigned		is_suspended:1;
+	struct mutex		lock;
+	bool			stopped;	/* P: lock */
+	bool			disabled;	/* P: lock */
+	bool			suspended;	/* P: lock */
 
 	int			(*filter)(void *data, int data_idx, int *val);
 	void			*filter_data;
@@ -165,7 +167,7 @@ struct ads7846 {
 #define	ADS_12_BIT		(0 << 3)
 #define	ADS_SER			(1 << 2)	/* non-differential */
 #define	ADS_DFR			(0 << 2)	/* differential */
-#define	ADS_PD10_PDOWN		(0 << 0)	/* lowpower mode + penirq */
+#define	ADS_PD10_PDOWN		(0 << 0)	/* low power mode + penirq */
 #define	ADS_PD10_ADC_ON		(1 << 0)	/* ADC on */
 #define	ADS_PD10_REF_ON		(2 << 0)	/* vREF on + penirq */
 #define	ADS_PD10_ALL_ON		(3 << 0)	/* ADC + vREF on */
@@ -193,6 +195,78 @@ struct ads7846 {
 #define	REF_ON	(READ_12BIT_DFR(x, 1, 1))
 #define	REF_OFF	(READ_12BIT_DFR(y, 0, 0))
 
+/* Must be called with ts->lock held */
+static void ads7846_stop(struct ads7846 *ts)
+{
+	if (!ts->disabled && !ts->suspended) {
+		/* Signal IRQ thread to stop polling and disable the handler. */
+		ts->stopped = true;
+		mb();
+		wake_up(&ts->wait);
+		disable_irq(ts->spi->irq);
+	}
+}
+
+/* Must be called with ts->lock held */
+static void ads7846_restart(struct ads7846 *ts)
+{
+	if (!ts->disabled && !ts->suspended) {
+		/* Tell IRQ thread that it may poll the device. */
+		ts->stopped = false;
+		mb();
+		enable_irq(ts->spi->irq);
+	}
+}
+
+/* Must be called with ts->lock held */
+static void __ads7846_disable(struct ads7846 *ts)
+{
+	ads7846_stop(ts);
+	regulator_disable(ts->reg);
+
+	/*
+	 * We know the chip's in low power mode since we always
+	 * leave it that way after every request
+	 */
+}
+
+/* Must be called with ts->lock held */
+static void __ads7846_enable(struct ads7846 *ts)
+{
+	regulator_enable(ts->reg);
+	ads7846_restart(ts);
+}
+
+static void ads7846_disable(struct ads7846 *ts)
+{
+	mutex_lock(&ts->lock);
+
+	if (!ts->disabled) {
+
+		if  (!ts->suspended)
+			__ads7846_disable(ts);
+
+		ts->disabled = true;
+	}
+
+	mutex_unlock(&ts->lock);
+}
+
+static void ads7846_enable(struct ads7846 *ts)
+{
+	mutex_lock(&ts->lock);
+
+	if (ts->disabled) {
+
+		ts->disabled = false;
+
+		if (!ts->suspended)
+			__ads7846_enable(ts);
+	}
+
+	mutex_unlock(&ts->lock);
+}
+
 /*--------------------------------------------------------------------------*/
 
 /*
@@ -219,23 +293,15 @@ struct ads7845_ser_req {
 	struct spi_transfer	xfer[2];
 };
 
-static void ads7846_enable(struct ads7846 *ts);
-static void ads7846_disable(struct ads7846 *ts);
-
-static int device_suspended(struct device *dev)
-{
-	struct ads7846 *ts = dev_get_drvdata(dev);
-	return ts->is_suspended || ts->disabled;
-}
-
 static int ads7846_read12_ser(struct device *dev, unsigned command)
 {
-	struct spi_device	*spi = to_spi_device(dev);
-	struct ads7846		*ts = dev_get_drvdata(dev);
-	struct ser_req		*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status;
-	int			use_internal;
+	struct spi_device *spi = to_spi_device(dev);
+	struct ads7846 *ts = dev_get_drvdata(dev);
+	struct ser_req *req;
+	int status;
+	int use_internal;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -282,11 +348,11 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 	CS_CHANGE(req->xfer[5]);
 	spi_message_add_tail(&req->xfer[5], &req->msg);
 
-	ts->irq_disabled = 1;
-	disable_irq(spi->irq);
+	mutex_lock(&ts->lock);
+	ads7846_stop(ts);
 	status = spi_sync(spi, &req->msg);
-	ts->irq_disabled = 0;
-	enable_irq(spi->irq);
+	ads7846_restart(ts);
+	mutex_unlock(&ts->lock);
 
 	if (status == 0) {
 		/* on-wire is a must-ignore bit, a BE12 value, then padding */
@@ -301,11 +367,12 @@ static int ads7846_read12_ser(struct device *dev, unsigned command)
 
 static int ads7845_read12_ser(struct device *dev, unsigned command)
 {
-	struct spi_device	*spi = to_spi_device(dev);
-	struct ads7846		*ts = dev_get_drvdata(dev);
-	struct ads7845_ser_req	*req = kzalloc(sizeof *req, GFP_KERNEL);
-	int			status;
+	struct spi_device *spi = to_spi_device(dev);
+	struct ads7846 *ts = dev_get_drvdata(dev);
+	struct ads7845_ser_req *req;
+	int status;
 
+	req = kzalloc(sizeof *req, GFP_KERNEL);
 	if (!req)
 		return -ENOMEM;
 
@@ -317,11 +384,11 @@ static int ads7845_read12_ser(struct device *dev, unsigned command)
 	req->xfer[0].len = 3;
 	spi_message_add_tail(&req->xfer[0], &req->msg);
 
-	ts->irq_disabled = 1;
-	disable_irq(spi->irq);
+	mutex_lock(&ts->lock);
+	ads7846_stop(ts);
 	status = spi_sync(spi, &req->msg);
-	ts->irq_disabled = 0;
-	enable_irq(spi->irq);
+	ads7846_restart(ts);
+	mutex_unlock(&ts->lock);
 
 	if (status == 0) {
 		/* BE12 value, then padding */
@@ -374,6 +441,7 @@ static inline unsigned vaux_adjust(struct ads7846 *ts, ssize_t v)
 	/* external resistors may scale vAUX into 0..vREF */
 	retval *= ts->vref_mv;
 	retval = retval >> 12;
+
 	return retval;
 }
 
@@ -384,13 +452,13 @@ static inline unsigned vbatt_adjust(struct ads7846 *ts, ssize_t v)
 	/* ads7846 has a resistor ladder to scale this signal down */
 	if (ts->model == 7846)
 		retval *= 4;
+
 	return retval;
 }
 
 SHOW(in0_input, vaux, vaux_adjust)
 SHOW(in1_input, vbatt, vbatt_adjust)
 
-
 static struct attribute *ads7846_attributes[] = {
 	&dev_attr_temp0.attr,
 	&dev_attr_temp1.attr,
@@ -498,17 +566,12 @@ static inline void ads784x_hwmon_unregister(struct spi_device *spi,
 }
 #endif
 
-static int is_pen_down(struct device *dev)
-{
-	struct ads7846	*ts = dev_get_drvdata(dev);
-
-	return ts->pendown;
-}
-
 static ssize_t ads7846_pen_down_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	return sprintf(buf, "%u\n", is_pen_down(dev));
+	struct ads7846 *ts = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", ts->pendown);
 }
 
 static DEVICE_ATTR(pen_down, S_IRUGO, ads7846_pen_down_show, NULL);
@@ -516,7 +579,7 @@ static DEVICE_ATTR(pen_down, S_IRUGO, ads7846_pen_down_show, NULL);
 static ssize_t ads7846_disable_show(struct device *dev,
 				     struct device_attribute *attr, char *buf)
 {
-	struct ads7846	*ts = dev_get_drvdata(dev);
+	struct ads7846 *ts = dev_get_drvdata(dev);
 
 	return sprintf(buf, "%u\n", ts->disabled);
 }
@@ -531,15 +594,11 @@ static ssize_t ads7846_disable_store(struct device *dev,
 	if (strict_strtoul(buf, 10, &i))
 		return -EINVAL;
 
-	spin_lock_irq(&ts->lock);
-
 	if (i)
 		ads7846_disable(ts);
 	else
 		ads7846_enable(ts);
 
-	spin_unlock_irq(&ts->lock);
-
 	return count;
 }
 
@@ -569,23 +628,141 @@ static void null_wait_for_sync(void)
 {
 }
 
-/*
- * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
- * to retrieve touchscreen status.
- *
- * The SPI transfer completion callback does the real work.  It reports
- * touchscreen events and reactivates the timer (or IRQ) as appropriate.
- */
+static int ads7846_debounce_filter(void *ads, int data_idx, int *val)
+{
+	struct ads7846 *ts = ads;
+
+	if (!ts->read_cnt || (abs(ts->last_read - *val) > ts->debounce_tol)) {
+		/* Start over collecting consistent readings. */
+		ts->read_rep = 0;
+		/*
+		 * Repeat it, if this was the first read or the read
+		 * wasn't consistent enough.
+		 */
+		if (ts->read_cnt < ts->debounce_max) {
+			ts->last_read = *val;
+			ts->read_cnt++;
+			return ADS7846_FILTER_REPEAT;
+		} else {
+			/*
+			 * Maximum number of debouncing reached and still
+			 * not enough number of consistent readings. Abort
+			 * the whole sample, repeat it in the next sampling
+			 * period.
+			 */
+			ts->read_cnt = 0;
+			return ADS7846_FILTER_IGNORE;
+		}
+	} else {
+		if (++ts->read_rep > ts->debounce_rep) {
+			/*
+			 * Got a good reading for this coordinate,
+			 * go for the next one.
+			 */
+			ts->read_cnt = 0;
+			ts->read_rep = 0;
+			return ADS7846_FILTER_OK;
+		} else {
+			/* Read more values that are consistent. */
+			ts->read_cnt++;
+			return ADS7846_FILTER_REPEAT;
+		}
+	}
+}
+
+static int ads7846_no_filter(void *ads, int data_idx, int *val)
+{
+	return ADS7846_FILTER_OK;
+}
+
+static int ads7846_get_value(struct ads7846 *ts, struct spi_message *m)
+{
+	struct spi_transfer *t =
+		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
+
+	if (ts->model == 7845) {
+		return be16_to_cpup((__be16 *)&(((char*)t->rx_buf)[1])) >> 3;
+	} else {
+		/*
+		 * adjust:  on-wire is a must-ignore bit, a BE12 value, then
+		 * padding; built from two 8 bit values written msb-first.
+		 */
+		return be16_to_cpup((__be16 *)t->rx_buf) >> 3;
+	}
+}
+
+static void ads7846_update_value(struct spi_message *m, int val)
+{
+	struct spi_transfer *t =
+		list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
+
+	*(u16 *)t->rx_buf = val;
+}
+
+static void ads7846_read_state(struct ads7846 *ts)
+{
+	struct ads7846_packet *packet = ts->packet;
+	struct spi_message *m;
+	int msg_idx = 0;
+	int val;
+	int action;
+	int error;
+
+	while (msg_idx < ts->msg_count) {
+
+		ts->wait_for_sync();
+
+		m = &ts->msg[msg_idx];
+		error = spi_sync(ts->spi, m);
+		if (error) {
+			dev_err(&ts->spi->dev, "spi_async --> %d\n", error);
+			packet->tc.ignore = true;
+			return;
+		}
+
+		/*
+		 * Last message is power down request, no need to convert
+		 * or filter the value.
+		 */
+		if (msg_idx < ts->msg_count - 1) {
 
-static void ads7846_rx(void *ads)
+			val = ads7846_get_value(ts, m);
+
+			action = ts->filter(ts->filter_data, msg_idx, &val);
+			switch (action) {
+			case ADS7846_FILTER_REPEAT:
+				continue;
+
+			case ADS7846_FILTER_IGNORE:
+				packet->tc.ignore = true;
+				msg_idx = ts->msg_count - 1;
+				continue;
+
+			case ADS7846_FILTER_OK:
+				ads7846_update_value(m, val);
+				packet->tc.ignore = false;
+				msg_idx++;
+				break;
+
+			default:
+				BUG();
+			}
+		} else {
+			msg_idx++;
+		}
+	}
+}
+
+static void ads7846_report_state(struct ads7846 *ts)
 {
-	struct ads7846		*ts = ads;
-	struct ads7846_packet	*packet = ts->packet;
-	unsigned		Rt;
-	u16			x, y, z1, z2;
+	struct ads7846_packet *packet = ts->packet;
+	unsigned int Rt;
+	u16 x, y, z1, z2;
 
-	/* ads7846_rx_val() did in-place conversion (including byteswap) from
-	 * on-the-wire format as part of debouncing to get stable readings.
+	/*
+	 * ads7846_get_value() does in-place conversion (including byte swap)
+	 * from on-the-wire format as part of debouncing to get stable
+	 * readings.
 	 */
 	if (ts->model == 7845) {
 		x = *(u16 *)packet->tc.x_buf;
@@ -623,19 +800,19 @@ static void ads7846_rx(void *ads)
 		Rt = 0;
 	}
 
-	/* Sample found inconsistent by debouncing or pressure is beyond
+	/*
+	 * Sample found inconsistent by debouncing or pressure is beyond
 	 * the maximum. Don't report it to user space, repeat at least
 	 * once more the measurement
 	 */
 	if (packet->tc.ignore || Rt > ts->pressure_max) {
 		dev_vdbg(&ts->spi->dev, "ignored %d pressure %d\n",
 			 packet->tc.ignore, Rt);
-		hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			      HRTIMER_MODE_REL);
 		return;
 	}
 
-	/* Maybe check the pendown state before reporting. This discards
+	/*
+	 * Maybe check the pendown state before reporting. This discards
 	 * false readings when the pen is lifted.
 	 */
 	if (ts->penirq_recheck_delay_usecs) {
@@ -644,8 +821,9 @@ static void ads7846_rx(void *ads)
 			Rt = 0;
 	}
 
-	/* NOTE: We can't rely on the pressure to determine the pen down
-	 * state, even this controller has a pressure sensor.  The pressure
+	/*
+	 * NOTE: We can't rely on the pressure to determine the pen down
+	 * state, even this controller has a pressure sensor. The pressure
 	 * value can fluctuate for quite a while after lifting the pen and
 	 * in some cases may not even settle at the expected value.
 	 *
@@ -655,15 +833,15 @@ static void ads7846_rx(void *ads)
 	if (Rt) {
 		struct input_dev *input = ts->input;
 
+		if (ts->swap_xy)
+			swap(x, y);
+
 		if (!ts->pendown) {
 			input_report_key(input, BTN_TOUCH, 1);
-			ts->pendown = 1;
+			ts->pendown = true;
 			dev_vdbg(&ts->spi->dev, "DOWN\n");
 		}
 
-		if (ts->swap_xy)
-			swap(x, y);
-
 		input_report_abs(input, ABS_X, x);
 		input_report_abs(input, ABS_Y, y);
 		input_report_abs(input, ABS_PRESSURE, ts->pressure_max - Rt);
@@ -671,246 +849,94 @@ static void ads7846_rx(void *ads)
 		input_sync(input);
 		dev_vdbg(&ts->spi->dev, "%4d/%4d/%4d\n", x, y, Rt);
 	}
-
-	hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_PERIOD),
-			HRTIMER_MODE_REL);
-}
-
-static int ads7846_debounce(void *ads, int data_idx, int *val)
-{
-	struct ads7846		*ts = ads;
-
-	if (!ts->read_cnt || (abs(ts->last_read - *val) > ts->debounce_tol)) {
-		/* Start over collecting consistent readings. */
-		ts->read_rep = 0;
-		/* Repeat it, if this was the first read or the read
-		 * wasn't consistent enough. */
-		if (ts->read_cnt < ts->debounce_max) {
-			ts->last_read = *val;
-			ts->read_cnt++;
-			return ADS7846_FILTER_REPEAT;
-		} else {
-			/* Maximum number of debouncing reached and still
-			 * not enough number of consistent readings. Abort
-			 * the whole sample, repeat it in the next sampling
-			 * period.
-			 */
-			ts->read_cnt = 0;
-			return ADS7846_FILTER_IGNORE;
-		}
-	} else {
-		if (++ts->read_rep > ts->debounce_rep) {
-			/* Got a good reading for this coordinate,
-			 * go for the next one. */
-			ts->read_cnt = 0;
-			ts->read_rep = 0;
-			return ADS7846_FILTER_OK;
-		} else {
-			/* Read more values that are consistent. */
-			ts->read_cnt++;
-			return ADS7846_FILTER_REPEAT;
-		}
-	}
 }
 
-static int ads7846_no_filter(void *ads, int data_idx, int *val)
+static irqreturn_t ads7846_hard_irq(int irq, void *handle)
 {
-	return ADS7846_FILTER_OK;
-}
-
-static void ads7846_rx_val(void *ads)
-{
-	struct ads7846 *ts = ads;
-	struct ads7846_packet *packet = ts->packet;
-	struct spi_message *m;
-	struct spi_transfer *t;
-	int val;
-	int action;
-	int status;
-
-	m = &ts->msg[ts->msg_idx];
-	t = list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
-
-	if (ts->model == 7845) {
-		val = be16_to_cpup((__be16 *)&(((char*)t->rx_buf)[1])) >> 3;
-	} else {
-		/* adjust:  on-wire is a must-ignore bit, a BE12 value, then
-		 * padding; built from two 8 bit values written msb-first.
-		 */
-		val = be16_to_cpup((__be16 *)t->rx_buf) >> 3;
-	}
+	struct ads7846 *ts = handle;
 
-	action = ts->filter(ts->filter_data, ts->msg_idx, &val);
-	switch (action) {
-	case ADS7846_FILTER_REPEAT:
-		break;
-	case ADS7846_FILTER_IGNORE:
-		packet->tc.ignore = 1;
-		/* Last message will contain ads7846_rx() as the
-		 * completion function.
-		 */
-		m = ts->last_msg;
-		break;
-	case ADS7846_FILTER_OK:
-		*(u16 *)t->rx_buf = val;
-		packet->tc.ignore = 0;
-		m = &ts->msg[++ts->msg_idx];
-		break;
-	default:
-		BUG();
-	}
-	ts->wait_for_sync();
-	status = spi_async(ts->spi, m);
-	if (status)
-		dev_err(&ts->spi->dev, "spi_async --> %d\n",
-				status);
+	return get_pendown_state(ts) ? IRQ_WAKE_THREAD : IRQ_HANDLED;
 }
 
-static enum hrtimer_restart ads7846_timer(struct hrtimer *handle)
-{
-	struct ads7846	*ts = container_of(handle, struct ads7846, timer);
-	int		status = 0;
-
-	spin_lock(&ts->lock);
-
-	if (unlikely(!get_pendown_state(ts) ||
-		     device_suspended(&ts->spi->dev))) {
-		if (ts->pendown) {
-			struct input_dev *input = ts->input;
-
-			input_report_key(input, BTN_TOUCH, 0);
-			input_report_abs(input, ABS_PRESSURE, 0);
-			input_sync(input);
-
-			ts->pendown = 0;
-			dev_vdbg(&ts->spi->dev, "UP\n");
-		}
-
-		/* measurement cycle ended */
-		if (!device_suspended(&ts->spi->dev)) {
-			ts->irq_disabled = 0;
-			enable_irq(ts->spi->irq);
-		}
-		ts->pending = 0;
-	} else {
-		/* pen is still down, continue with the measurement */
-		ts->msg_idx = 0;
-		ts->wait_for_sync();
-		status = spi_async(ts->spi, &ts->msg[0]);
-		if (status)
-			dev_err(&ts->spi->dev, "spi_async --> %d\n", status);
-	}
-
-	spin_unlock(&ts->lock);
-	return HRTIMER_NORESTART;
-}
 
 static irqreturn_t ads7846_irq(int irq, void *handle)
 {
 	struct ads7846 *ts = handle;
-	unsigned long flags;
-
-	spin_lock_irqsave(&ts->lock, flags);
-	if (likely(get_pendown_state(ts))) {
-		if (!ts->irq_disabled) {
-			/* The ARM do_simple_IRQ() dispatcher doesn't act
-			 * like the other dispatchers:  it will report IRQs
-			 * even after they've been disabled.  We work around
-			 * that here.  (The "generic irq" framework may help...)
-			 */
-			ts->irq_disabled = 1;
-			disable_irq_nosync(ts->spi->irq);
-			ts->pending = 1;
-			hrtimer_start(&ts->timer, ktime_set(0, TS_POLL_DELAY),
-					HRTIMER_MODE_REL);
-		}
-	}
-	spin_unlock_irqrestore(&ts->lock, flags);
 
-	return IRQ_HANDLED;
-}
+	/* Start with a small delay before checking pendown state */
+	msleep(TS_POLL_DELAY);
 
-/*--------------------------------------------------------------------------*/
+	while (!ts->stopped && get_pendown_state(ts)) {
 
-/* Must be called with ts->lock held */
-static void ads7846_disable(struct ads7846 *ts)
-{
-	if (ts->disabled)
-		return;
+		/* pen is down, continue with the measurement */
+		ads7846_read_state(ts);
 
-	ts->disabled = 1;
+		if (!ts->stopped)
+			ads7846_report_state(ts);
 
-	/* are we waiting for IRQ, or polling? */
-	if (!ts->pending) {
-		ts->irq_disabled = 1;
-		disable_irq(ts->spi->irq);
-	} else {
-		/* the timer will run at least once more, and
-		 * leave everything in a clean state, IRQ disabled
-		 */
-		while (ts->pending) {
-			spin_unlock_irq(&ts->lock);
-			msleep(1);
-			spin_lock_irq(&ts->lock);
-		}
+		wait_event_timeout(ts->wait, ts->stopped,
+				   msecs_to_jiffies(TS_POLL_PERIOD));
 	}
 
-	regulator_disable(ts->reg);
-
-	/* we know the chip's in lowpower mode since we always
-	 * leave it that way after every request
-	 */
-}
+	if (ts->pendown) {
+		struct input_dev *input = ts->input;
 
-/* Must be called with ts->lock held */
-static void ads7846_enable(struct ads7846 *ts)
-{
-	if (!ts->disabled)
-		return;
+		input_report_key(input, BTN_TOUCH, 0);
+		input_report_abs(input, ABS_PRESSURE, 0);
+		input_sync(input);
 
-	regulator_enable(ts->reg);
+		ts->pendown = false;
+		dev_vdbg(&ts->spi->dev, "UP\n");
+	}
 
-	ts->disabled = 0;
-	ts->irq_disabled = 0;
-	enable_irq(ts->spi->irq);
+	return IRQ_HANDLED;
 }
 
 static int ads7846_suspend(struct spi_device *spi, pm_message_t message)
 {
 	struct ads7846 *ts = dev_get_drvdata(&spi->dev);
 
-	spin_lock_irq(&ts->lock);
+	mutex_lock(&ts->lock);
 
-	ts->is_suspended = 1;
-	ads7846_disable(ts);
+	if (!ts->suspended) {
 
-	spin_unlock_irq(&ts->lock);
+		if (!ts->disabled)
+			__ads7846_disable(ts);
 
-	if (device_may_wakeup(&ts->spi->dev))
-		enable_irq_wake(ts->spi->irq);
+		if (device_may_wakeup(&ts->spi->dev))
+			enable_irq_wake(ts->spi->irq);
 
-	return 0;
+		ts->suspended = true;
+	}
+
+	mutex_unlock(&ts->lock);
 
+	return 0;
 }
 
 static int ads7846_resume(struct spi_device *spi)
 {
 	struct ads7846 *ts = dev_get_drvdata(&spi->dev);
 
-	if (device_may_wakeup(&ts->spi->dev))
-		disable_irq_wake(ts->spi->irq);
+	mutex_lock(&ts->lock);
+
+	if (ts->suspended) {
 
-	spin_lock_irq(&ts->lock);
+		ts->suspended = false;
 
-	ts->is_suspended = 0;
-	ads7846_enable(ts);
+		if (device_may_wakeup(&ts->spi->dev))
+			disable_irq_wake(ts->spi->irq);
 
-	spin_unlock_irq(&ts->lock);
+		if (!ts->disabled)
+			__ads7846_enable(ts);
+	}
+
+	mutex_unlock(&ts->lock);
 
 	return 0;
 }
 
-static int __devinit setup_pendown(struct spi_device *spi, struct ads7846 *ts)
+static int __devinit ads7846_setup_pendown(struct spi_device *spi, struct ads7846 *ts)
 {
 	struct ads7846_platform_data *pdata = spi->dev.platform_data;
 	int err;
@@ -932,146 +958,40 @@ static int __devinit setup_pendown(struct spi_device *spi, struct ads7846 *ts)
 	err = gpio_request(pdata->gpio_pendown, "ads7846_pendown");
 	if (err) {
 		dev_err(&spi->dev, "failed to request pendown GPIO%d\n",
-				pdata->gpio_pendown);
+			pdata->gpio_pendown);
 		return err;
 	}
 
 	ts->gpio_pendown = pdata->gpio_pendown;
+
 	return 0;
 }
 
-static int __devinit ads7846_probe(struct spi_device *spi)
+/*
+ * Set up the transfers to read touchscreen state; this assumes we
+ * use formula #2 for pressure, not #3.
+ */
+static void __devinit ads7846_setup_spi_msg(struct ads7846 *ts,
+				const struct ads7846_platform_data *pdata)
 {
-	struct ads7846 *ts;
-	struct ads7846_packet *packet;
-	struct input_dev *input_dev;
-	const struct ads7846_platform_data *pdata = spi->dev.platform_data;
-	struct spi_message *m;
-	struct spi_transfer *x;
-	unsigned long irq_flags;
-	int vref;
-	int err;
-
-	if (!spi->irq) {
-		dev_dbg(&spi->dev, "no IRQ?\n");
-		return -ENODEV;
-	}
-
-	if (!pdata) {
-		dev_dbg(&spi->dev, "no platform data?\n");
-		return -ENODEV;
-	}
-
-	/* don't exceed max specified sample rate */
-	if (spi->max_speed_hz > (125000 * SAMPLE_BITS)) {
-		dev_dbg(&spi->dev, "f(sample) %d KHz?\n",
-				(spi->max_speed_hz/SAMPLE_BITS)/1000);
-		return -EINVAL;
-	}
-
-	/* We'd set TX wordsize 8 bits and RX wordsize to 13 bits ... except
-	 * that even if the hardware can do that, the SPI controller driver
-	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
-	 */
-	spi->bits_per_word = 8;
-	spi->mode = SPI_MODE_0;
-	err = spi_setup(spi);
-	if (err < 0)
-		return err;
-
-	ts = kzalloc(sizeof(struct ads7846), GFP_KERNEL);
-	packet = kzalloc(sizeof(struct ads7846_packet), GFP_KERNEL);
-	input_dev = input_allocate_device();
-	if (!ts || !packet || !input_dev) {
-		err = -ENOMEM;
-		goto err_free_mem;
-	}
-
-	dev_set_drvdata(&spi->dev, ts);
-
-	ts->packet = packet;
-	ts->spi = spi;
-	ts->input = input_dev;
-	ts->vref_mv = pdata->vref_mv;
-	ts->swap_xy = pdata->swap_xy;
-
-	hrtimer_init(&ts->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
-	ts->timer.function = ads7846_timer;
-
-	spin_lock_init(&ts->lock);
-
-	ts->model = pdata->model ? : 7846;
-	ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
-	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
-	ts->pressure_max = pdata->pressure_max ? : ~0;
-
-	if (pdata->filter != NULL) {
-		if (pdata->filter_init != NULL) {
-			err = pdata->filter_init(pdata, &ts->filter_data);
-			if (err < 0)
-				goto err_free_mem;
-		}
-		ts->filter = pdata->filter;
-		ts->filter_cleanup = pdata->filter_cleanup;
-	} else if (pdata->debounce_max) {
-		ts->debounce_max = pdata->debounce_max;
-		if (ts->debounce_max < 2)
-			ts->debounce_max = 2;
-		ts->debounce_tol = pdata->debounce_tol;
-		ts->debounce_rep = pdata->debounce_rep;
-		ts->filter = ads7846_debounce;
-		ts->filter_data = ts;
-	} else
-		ts->filter = ads7846_no_filter;
-
-	err = setup_pendown(spi, ts);
-	if (err)
-		goto err_cleanup_filter;
-
-	if (pdata->penirq_recheck_delay_usecs)
-		ts->penirq_recheck_delay_usecs =
-				pdata->penirq_recheck_delay_usecs;
-
-	ts->wait_for_sync = pdata->wait_for_sync ? : null_wait_for_sync;
-
-	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
-	snprintf(ts->name, sizeof(ts->name), "ADS%d Touchscreen", ts->model);
-
-	input_dev->name = ts->name;
-	input_dev->phys = ts->phys;
-	input_dev->dev.parent = &spi->dev;
-
-	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
-	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
-	input_set_abs_params(input_dev, ABS_X,
-			pdata->x_min ? : 0,
-			pdata->x_max ? : MAX_12BIT,
-			0, 0);
-	input_set_abs_params(input_dev, ABS_Y,
-			pdata->y_min ? : 0,
-			pdata->y_max ? : MAX_12BIT,
-			0, 0);
-	input_set_abs_params(input_dev, ABS_PRESSURE,
-			pdata->pressure_min, pdata->pressure_max, 0, 0);
-
-	vref = pdata->keep_vref_on;
+	struct spi_message *m = &ts->msg[0];
+	struct spi_transfer *x = ts->xfer;
+	struct ads7846_packet *packet = ts->packet;
+	int vref = pdata->keep_vref_on;
 
 	if (ts->model == 7873) {
-		/* The AD7873 is almost identical to the ADS7846
+		/*
+		 * The AD7873 is almost identical to the ADS7846
 		 * keep VREF off during differential/ratiometric
-		 * conversion modes
+		 * conversion modes.
 		 */
 		ts->model = 7846;
 		vref = 0;
 	}
 
-	/* set up the transfers to read touchscreen state; this assumes we
-	 * use formula #2 for pressure, not #3.
-	 */
-	m = &ts->msg[0];
-	x = ts->xfer;
-
+	ts->msg_count = 1;
 	spi_message_init(m);
+	m->context = ts;
 
 	if (ts->model == 7845) {
 		packet->read_y_cmd[0] = READ_Y(vref);
@@ -1094,7 +1014,8 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		spi_message_add_tail(x, m);
 	}
 
-	/* the first sample after switching drivers can be low quality;
+	/*
+	 * The first sample after switching drivers can be low quality;
 	 * optionally discard it, using a second one after the signals
 	 * have had enough time to stabilize.
 	 */
@@ -1112,11 +1033,10 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		spi_message_add_tail(x, m);
 	}
 
-	m->complete = ads7846_rx_val;
-	m->context = ts;
-
+	ts->msg_count++;
 	m++;
 	spi_message_init(m);
+	m->context = ts;
 
 	if (ts->model == 7845) {
 		x++;
@@ -1156,13 +1076,12 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 		spi_message_add_tail(x, m);
 	}
 
-	m->complete = ads7846_rx_val;
-	m->context = ts;
-
 	/* turn y+ off, x- on; we'll use formula #2 */
 	if (ts->model == 7846) {
+		ts->msg_count++;
 		m++;
 		spi_message_init(m);
+		m->context = ts;
 
 		x++;
 		packet->read_z1 = READ_Z1(vref);
@@ -1190,11 +1109,10 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 			spi_message_add_tail(x, m);
 		}
 
-		m->complete = ads7846_rx_val;
-		m->context = ts;
-
+		ts->msg_count++;
 		m++;
 		spi_message_init(m);
+		m->context = ts;
 
 		x++;
 		packet->read_z2 = READ_Z2(vref);
@@ -1221,14 +1139,13 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 			x->len = 2;
 			spi_message_add_tail(x, m);
 		}
-
-		m->complete = ads7846_rx_val;
-		m->context = ts;
 	}
 
 	/* power down */
+	ts->msg_count++;
 	m++;
 	spi_message_init(m);
+	m->context = ts;
 
 	if (ts->model == 7845) {
 		x++;
@@ -1251,11 +1168,119 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 
 	CS_CHANGE(*x);
 	spi_message_add_tail(x, m);
+}
 
-	m->complete = ads7846_rx;
-	m->context = ts;
+static int __devinit ads7846_probe(struct spi_device *spi)
+{
+	struct ads7846 *ts;
+	struct ads7846_packet *packet;
+	struct input_dev *input_dev;
+	struct ads7846_platform_data *pdata = spi->dev.platform_data;
+	unsigned long irq_flags;
+	int err;
+
+	if (!spi->irq) {
+		dev_dbg(&spi->dev, "no IRQ?\n");
+		return -ENODEV;
+	}
+
+	if (!pdata) {
+		dev_dbg(&spi->dev, "no platform data?\n");
+		return -ENODEV;
+	}
+
+	/* don't exceed max specified sample rate */
+	if (spi->max_speed_hz > (125000 * SAMPLE_BITS)) {
+		dev_dbg(&spi->dev, "f(sample) %d KHz?\n",
+				(spi->max_speed_hz/SAMPLE_BITS)/1000);
+		return -EINVAL;
+	}
+
+	/* We'd set TX word size 8 bits and RX word size to 13 bits ... except
+	 * that even if the hardware can do that, the SPI controller driver
+	 * may not.  So we stick to very-portable 8 bit words, both RX and TX.
+	 */
+	spi->bits_per_word = 8;
+	spi->mode = SPI_MODE_0;
+	err = spi_setup(spi);
+	if (err < 0)
+		return err;
 
-	ts->last_msg = m;
+	ts = kzalloc(sizeof(struct ads7846), GFP_KERNEL);
+	packet = kzalloc(sizeof(struct ads7846_packet), GFP_KERNEL);
+	input_dev = input_allocate_device();
+	if (!ts || !packet || !input_dev) {
+		err = -ENOMEM;
+		goto err_free_mem;
+	}
+
+	dev_set_drvdata(&spi->dev, ts);
+
+	ts->packet = packet;
+	ts->spi = spi;
+	ts->input = input_dev;
+	ts->vref_mv = pdata->vref_mv;
+	ts->swap_xy = pdata->swap_xy;
+
+	mutex_init(&ts->lock);
+	init_waitqueue_head(&ts->wait);
+
+	ts->model = pdata->model ? : 7846;
+	ts->vref_delay_usecs = pdata->vref_delay_usecs ? : 100;
+	ts->x_plate_ohms = pdata->x_plate_ohms ? : 400;
+	ts->pressure_max = pdata->pressure_max ? : ~0;
+
+	if (pdata->filter != NULL) {
+		if (pdata->filter_init != NULL) {
+			err = pdata->filter_init(pdata, &ts->filter_data);
+			if (err < 0)
+				goto err_free_mem;
+		}
+		ts->filter = pdata->filter;
+		ts->filter_cleanup = pdata->filter_cleanup;
+	} else if (pdata->debounce_max) {
+		ts->debounce_max = pdata->debounce_max;
+		if (ts->debounce_max < 2)
+			ts->debounce_max = 2;
+		ts->debounce_tol = pdata->debounce_tol;
+		ts->debounce_rep = pdata->debounce_rep;
+		ts->filter = ads7846_debounce_filter;
+		ts->filter_data = ts;
+	} else {
+		ts->filter = ads7846_no_filter;
+	}
+
+	err = ads7846_setup_pendown(spi, ts);
+	if (err)
+		goto err_cleanup_filter;
+
+	if (pdata->penirq_recheck_delay_usecs)
+		ts->penirq_recheck_delay_usecs =
+				pdata->penirq_recheck_delay_usecs;
+
+	ts->wait_for_sync = pdata->wait_for_sync ? : null_wait_for_sync;
+
+	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", dev_name(&spi->dev));
+	snprintf(ts->name, sizeof(ts->name), "ADS%d Touchscreen", ts->model);
+
+	input_dev->name = ts->name;
+	input_dev->phys = ts->phys;
+	input_dev->dev.parent = &spi->dev;
+
+	input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_ABS);
+	input_dev->keybit[BIT_WORD(BTN_TOUCH)] = BIT_MASK(BTN_TOUCH);
+	input_set_abs_params(input_dev, ABS_X,
+			pdata->x_min ? : 0,
+			pdata->x_max ? : MAX_12BIT,
+			0, 0);
+	input_set_abs_params(input_dev, ABS_Y,
+			pdata->y_min ? : 0,
+			pdata->y_max ? : MAX_12BIT,
+			0, 0);
+	input_set_abs_params(input_dev, ABS_PRESSURE,
+			pdata->pressure_min, pdata->pressure_max, 0, 0);
+
+	ads7846_setup_spi_msg(ts, pdata);
 
 	ts->reg = regulator_get(&spi->dev, "vcc");
 	if (IS_ERR(ts->reg)) {
@@ -1271,16 +1296,17 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 	}
 
 	irq_flags = pdata->irq_flags ? : IRQF_TRIGGER_FALLING;
+	irq_flags |= IRQF_ONESHOT;
 
-	err = request_irq(spi->irq, ads7846_irq, irq_flags,
-			  spi->dev.driver->name, ts);
-
+	err = request_threaded_irq(spi->irq, ads7846_hard_irq, ads7846_irq,
+				   irq_flags, spi->dev.driver->name, ts);
 	if (err && !pdata->irq_flags) {
 		dev_info(&spi->dev,
 			"trying pin change workaround on irq %d\n", spi->irq);
-		err = request_irq(spi->irq, ads7846_irq,
-				  IRQF_TRIGGER_FALLING | IRQF_TRIGGER_RISING,
-				  spi->dev.driver->name, ts);
+		irq_flags |= IRQF_TRIGGER_RISING;
+		err = request_threaded_irq(spi->irq,
+				  ads7846_hard_irq, ads7846_irq,
+				  irq_flags, spi->dev.driver->name, ts);
 	}
 
 	if (err) {
@@ -1294,7 +1320,8 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 
 	dev_info(&spi->dev, "touchscreen, irq %d\n", spi->irq);
 
-	/* take a first sample, leaving nPENIRQ active and vREF off; avoid
+	/*
+	 * Take a first sample, leaving nPENIRQ active and vREF off; avoid
 	 * the touchscreen, in case it's not connected.
 	 */
 	if (ts->model == 7845)
@@ -1340,20 +1367,18 @@ static int __devinit ads7846_probe(struct spi_device *spi)
 
 static int __devexit ads7846_remove(struct spi_device *spi)
 {
-	struct ads7846		*ts = dev_get_drvdata(&spi->dev);
+	struct ads7846 *ts = dev_get_drvdata(&spi->dev);
 
 	device_init_wakeup(&spi->dev, false);
 
-	ads784x_hwmon_unregister(spi, ts);
-	input_unregister_device(ts->input);
-
-	ads7846_suspend(spi, PMSG_SUSPEND);
-
 	sysfs_remove_group(&spi->dev.kobj, &ads784x_attr_group);
 
+	ads7846_disable(ts);
 	free_irq(ts->spi->irq, ts);
-	/* suspend left the IRQ disabled */
-	enable_irq(ts->spi->irq);
+
+	input_unregister_device(ts->input);
+
+	ads784x_hwmon_unregister(spi, ts);
 
 	regulator_disable(ts->reg);
 	regulator_put(ts->reg);
@@ -1368,6 +1393,7 @@ static int __devexit ads7846_remove(struct spi_device *spi)
 	kfree(ts);
 
 	dev_dbg(&spi->dev, "unregistered touchscreen\n");
+
 	return 0;
 }
 

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-12 16:00                   ` Dmitry Torokhov
@ 2010-10-13  3:12                     ` Jason Wang
  2010-10-13 22:17                       ` Grazvydas Ignotas
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-10-13  3:12 UTC (permalink / raw)
  To: Dmitry Torokhov; +Cc: Jason Wang, notasas, vapier, linux-input

Hi Dmitry,

I have validated the new patch based off the 2.6.36-rc7 on
the ti_omap3530evm, it works fine.

Thanks,
Jason.

Dmitry Torokhov wrote:
> Hi Jason.
>
> On Tue, Oct 12, 2010 at 05:58:36PM +0800, Jason Wang wrote:
>   
>> 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.
>>     
>
> Sorry about that.
>
>   
>> @@ -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);
>>     
>
> If the device was both disabled and suspended then enable will never
> succeed.
>
> I actually was looking over the patches yesterday and I believe I
> corrected the problem you mentioned. I rolled all patches together (no
> reason to keep them separate - they have not been yet applied to
> non-rebasing branch and they are simple fix-ups to the issues introduced
> by the fisrt patch). Could you please try the patch below and if it
> still works I'll queue for .37.
>
> Thanks!
>
>   


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-13  3:12                     ` Jason Wang
@ 2010-10-13 22:17                       ` Grazvydas Ignotas
  2010-10-14  2:14                         ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Grazvydas Ignotas @ 2010-10-13 22:17 UTC (permalink / raw)
  To: Jason Wang; +Cc: Dmitry Torokhov, vapier, linux-input, linux-omap

On Wed, Oct 13, 2010 at 6:12 AM, Jason Wang <jason77.wang@gmail.com> wrote:
> Hi Dmitry,
>
> I have validated the new patch based off the 2.6.36-rc7 on
> the ti_omap3530evm, it works fine.

Working here on pandora too, applied on top of linux-next (did not
test suspend though).

Not caused by this patch, but the old problem where lower measurement
bits get lost is back - when I draw diagonal line it looks like
stairs. I think it's caused by OMAP SPI driver, CCing linux-omap.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-13 22:17                       ` Grazvydas Ignotas
@ 2010-10-14  2:14                         ` Jason Wang
  2010-10-14 21:40                           ` Grazvydas Ignotas
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-10-14  2:14 UTC (permalink / raw)
  To: Grazvydas Ignotas
  Cc: Jason Wang, Dmitry Torokhov, vapier, linux-input, linux-omap,
	grant.likely, roman.tereshonkov

Grazvydas Ignotas wrote:
> On Wed, Oct 13, 2010 at 6:12 AM, Jason Wang <jason77.wang@gmail.com> wrote:
>   
>> Hi Dmitry,
>>
>> I have validated the new patch based off the 2.6.36-rc7 on
>> the ti_omap3530evm, it works fine.
>>     
>
> Working here on pandora too, applied on top of linux-next (did not
> test suspend though).
>
> Not caused by this patch, but the old problem where lower measurement
> bits get lost is back - when I draw diagonal line it looks like
> stairs. I think it's caused by OMAP SPI driver, CCing linux-omap.
>
>   
Yes, it is OMAP spi driver issues, I have met the same problem before, 
please refer to:
http://www.spinics.net/lists/arm-kernel/msg91538.html

you can apply this patch and test again.



 From 393f445ab899abaf5d60a55a54f242c430507047 Mon Sep 17 00:00:00 2001
From: Jason Wang <jason77.wang@gmail.com>
Date: Thu, 14 Oct 2010 10:08:07 +0800
Subject: [PATCH] spi/omap2_mcspi: disable channel after TX_ONLY transfer 
in PIO mode

In TX_ONLY transfer, the SPI controller can also receive data
simultaneously and saves them in rx register. After the TX_ONLY
transfer, the rx register will hold random data received during
last tx transaction.

If the next transfer is RX_ONLY, this random data has
the possibility to affect this transfer like this:

  If the spi controller is changed from TX_ONLY to RX_ONLY mode,
  The random data makes the rx register full imediately and
  triggers a dummy write (in SPI RX_ONLY transfer, we need a dummy
  write to trigger the first transaction).

  So the first data received in RX_ONLY transfer will be that
  random data instead of something meaningfug.

We can avoid this by a Disable/reenable toggle of the spi channel
after the TX_ONLY transfer, since it purges the rx register.

Signed-off-by: Jason Wang <jason77.wang@gmail.com>
---
 drivers/spi/omap2_mcspi.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
index b3a94ca..43fab41 100644
--- a/drivers/spi/omap2_mcspi.c
+++ b/drivers/spi/omap2_mcspi.c
@@ -644,6 +644,12 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct 
spi_transfer *xfer)
                } else if (mcspi_wait_for_reg_bit(chstat_reg,
                                OMAP2_MCSPI_CHSTAT_EOT) < 0)
                        dev_err(&spi->dev, "EOT timed out\n");
+
+               /* disable chan to purge rx datas received in TX_ONLY 
transfer,
+                * otherwise these rx datas will affect the direct following
+                * RX_ONLY transfer.
+                */
+               omap2_mcspi_set_enable(spi, 0);
        }
 out:
        omap2_mcspi_set_enable(spi, 1);
-- 
1.5.6.5


^ permalink raw reply related	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-14  2:14                         ` Jason Wang
@ 2010-10-14 21:40                           ` Grazvydas Ignotas
  2010-10-15  2:29                             ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Grazvydas Ignotas @ 2010-10-14 21:40 UTC (permalink / raw)
  To: Jason Wang
  Cc: Dmitry Torokhov, vapier, linux-input, linux-omap, grant.likely,
	roman.tereshonkov

On Thu, Oct 14, 2010 at 5:14 AM, Jason Wang <jason77.wang@gmail.com> wrote:
> Grazvydas Ignotas wrote:
>> Working here on pandora too, applied on top of linux-next (did not
>> test suspend though).
>>
>> Not caused by this patch, but the old problem where lower measurement
>> bits get lost is back - when I draw diagonal line it looks like
>> stairs. I think it's caused by OMAP SPI driver, CCing linux-omap.
>>
>>
>
> Yes, it is OMAP spi driver issues, I have met the same problem before,
> please refer to:
> http://www.spinics.net/lists/arm-kernel/msg91538.html
>
> you can apply this patch and test again.

It solves my issue, thanks
Tested-by: Grazvydas Ignotas <notasas@gmail.com>

BTW I have done similar patch almost 2 years ago for exactly the same problem:
http://marc.info/?l=linux-omap&m=122565580616920&w=2
but then problem went away after random kernel update (from what I
remember), and my patch was no longer needed.


> From 393f445ab899abaf5d60a55a54f242c430507047 Mon Sep 17 00:00:00 2001
> From: Jason Wang <jason77.wang@gmail.com>
> Date: Thu, 14 Oct 2010 10:08:07 +0800
> Subject: [PATCH] spi/omap2_mcspi: disable channel after TX_ONLY transfer in
> PIO mode
>
> In TX_ONLY transfer, the SPI controller can also receive data
> simultaneously and saves them in rx register. After the TX_ONLY
> transfer, the rx register will hold random data received during
> last tx transaction.
>
> If the next transfer is RX_ONLY, this random data has
> the possibility to affect this transfer like this:
>
>  If the spi controller is changed from TX_ONLY to RX_ONLY mode,
>  The random data makes the rx register full imediately and
>  triggers a dummy write (in SPI RX_ONLY transfer, we need a dummy
>  write to trigger the first transaction).
>
>  So the first data received in RX_ONLY transfer will be that
>  random data instead of something meaningfug.
>
> We can avoid this by a Disable/reenable toggle of the spi channel
> after the TX_ONLY transfer, since it purges the rx register.
>
> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
> ---
> drivers/spi/omap2_mcspi.c |    6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
> index b3a94ca..43fab41 100644
> --- a/drivers/spi/omap2_mcspi.c
> +++ b/drivers/spi/omap2_mcspi.c
> @@ -644,6 +644,12 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct
> spi_transfer *xfer)
>               } else if (mcspi_wait_for_reg_bit(chstat_reg,
>                               OMAP2_MCSPI_CHSTAT_EOT) < 0)
>                       dev_err(&spi->dev, "EOT timed out\n");
> +
> +               /* disable chan to purge rx datas received in TX_ONLY
> transfer,
> +                * otherwise these rx datas will affect the direct following
> +                * RX_ONLY transfer.
> +                */
> +               omap2_mcspi_set_enable(spi, 0);
>       }
> out:
>       omap2_mcspi_set_enable(spi, 1);
> --
> 1.5.6.5
>
>
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-14 21:40                           ` Grazvydas Ignotas
@ 2010-10-15  2:29                             ` Jason Wang
  2010-10-18 23:19                               ` Tony Lindgren
  0 siblings, 1 reply; 19+ messages in thread
From: Jason Wang @ 2010-10-15  2:29 UTC (permalink / raw)
  To: Grazvydas Ignotas
  Cc: Jason Wang, Dmitry Torokhov, vapier, linux-input, linux-omap,
	grant.likely, roman.tereshonkov

Grazvydas Ignotas wrote:
> On Thu, Oct 14, 2010 at 5:14 AM, Jason Wang <jason77.wang@gmail.com> wrote:
>   
>> Grazvydas Ignotas wrote:
>>     
>>> Working here on pandora too, applied on top of linux-next (did not
>>> test suspend though).
>>>
>>> Not caused by this patch, but the old problem where lower measurement
>>> bits get lost is back - when I draw diagonal line it looks like
>>> stairs. I think it's caused by OMAP SPI driver, CCing linux-omap.
>>>
>>>
>>>       
>> Yes, it is OMAP spi driver issues, I have met the same problem before,
>> please refer to:
>> http://www.spinics.net/lists/arm-kernel/msg91538.html
>>
>> you can apply this patch and test again.
>>     
>
> It solves my issue, thanks
> Tested-by: Grazvydas Ignotas <notasas@gmail.com>
>
> BTW I have done similar patch almost 2 years ago for exactly the same problem:
> http://marc.info/?l=linux-omap&m=122565580616920&w=2
> but then problem went away after random kernel update (from what I
> remember), and my patch was no longer needed.
>
>   
Really similar problem and similar solution, :-)

>   
>> From 393f445ab899abaf5d60a55a54f242c430507047 Mon Sep 17 00:00:00 2001
>> From: Jason Wang <jason77.wang@gmail.com>
>> Date: Thu, 14 Oct 2010 10:08:07 +0800
>> Subject: [PATCH] spi/omap2_mcspi: disable channel after TX_ONLY transfer in
>> PIO mode
>>
>> In TX_ONLY transfer, the SPI controller can also receive data
>> simultaneously and saves them in rx register. After the TX_ONLY
>> transfer, the rx register will hold random data received during
>> last tx transaction.
>>
>> If the next transfer is RX_ONLY, this random data has
>> the possibility to affect this transfer like this:
>>
>>  If the spi controller is changed from TX_ONLY to RX_ONLY mode,
>>  The random data makes the rx register full imediately and
>>  triggers a dummy write (in SPI RX_ONLY transfer, we need a dummy
>>  write to trigger the first transaction).
>>
>>  So the first data received in RX_ONLY transfer will be that
>>  random data instead of something meaningfug.
>>
>> We can avoid this by a Disable/reenable toggle of the spi channel
>> after the TX_ONLY transfer, since it purges the rx register.
>>
>> Signed-off-by: Jason Wang <jason77.wang@gmail.com>
>> ---
>> drivers/spi/omap2_mcspi.c |    6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/spi/omap2_mcspi.c b/drivers/spi/omap2_mcspi.c
>> index b3a94ca..43fab41 100644
>> --- a/drivers/spi/omap2_mcspi.c
>> +++ b/drivers/spi/omap2_mcspi.c
>> @@ -644,6 +644,12 @@ omap2_mcspi_txrx_pio(struct spi_device *spi, struct
>> spi_transfer *xfer)
>>               } else if (mcspi_wait_for_reg_bit(chstat_reg,
>>                               OMAP2_MCSPI_CHSTAT_EOT) < 0)
>>                       dev_err(&spi->dev, "EOT timed out\n");
>> +
>> +               /* disable chan to purge rx datas received in TX_ONLY
>> transfer,
>> +                * otherwise these rx datas will affect the direct following
>> +                * RX_ONLY transfer.
>> +                */
>> +               omap2_mcspi_set_enable(spi, 0);
>>       }
>> out:
>>       omap2_mcspi_set_enable(spi, 1);
>> --
>> 1.5.6.5
>>
>>
>>     
>
>   


^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-15  2:29                             ` Jason Wang
@ 2010-10-18 23:19                               ` Tony Lindgren
  2010-10-19  1:25                                 ` Jason Wang
  0 siblings, 1 reply; 19+ messages in thread
From: Tony Lindgren @ 2010-10-18 23:19 UTC (permalink / raw)
  To: Jason Wang
  Cc: Grazvydas Ignotas, Dmitry Torokhov, vapier, linux-input,
	linux-omap, grant.likely, roman.tereshonkov

* Jason Wang <jason77.wang@gmail.com> [101014 19:17]:
> Grazvydas Ignotas wrote:
> >On Thu, Oct 14, 2010 at 5:14 AM, Jason Wang <jason77.wang@gmail.com> wrote:
> >>Grazvydas Ignotas wrote:
> >>>Working here on pandora too, applied on top of linux-next (did not
> >>>test suspend though).
> >>>
> >>>Not caused by this patch, but the old problem where lower measurement
> >>>bits get lost is back - when I draw diagonal line it looks like
> >>>stairs. I think it's caused by OMAP SPI driver, CCing linux-omap.
> >>>
> >>>
> >>Yes, it is OMAP spi driver issues, I have met the same problem before,
> >>please refer to:
> >>http://www.spinics.net/lists/arm-kernel/msg91538.html
> >>
> >>you can apply this patch and test again.
> >
> >It solves my issue, thanks
> >Tested-by: Grazvydas Ignotas <notasas@gmail.com>
> >
> >BTW I have done similar patch almost 2 years ago for exactly the same problem:
> >http://marc.info/?l=linux-omap&m=122565580616920&w=2
> >but then problem went away after random kernel update (from what I
> >remember), and my patch was no longer needed.
> >
> Really similar problem and similar solution, :-)

Let's get this fix into the mainline tree then. Jason, care to
refresh (it's wrapped at least) and then repost with the acks so
Grant can queue it up for the upcoming merge window?

Acked-by: Tony Lindgren <tony@atomide.com>

^ permalink raw reply	[flat|nested] 19+ messages in thread

* Re: [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions
  2010-10-18 23:19                               ` Tony Lindgren
@ 2010-10-19  1:25                                 ` Jason Wang
  0 siblings, 0 replies; 19+ messages in thread
From: Jason Wang @ 2010-10-19  1:25 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Jason Wang, Grazvydas Ignotas, Dmitry Torokhov, vapier,
	linux-input, linux-omap, grant.likely, roman.tereshonkov

Tony Lindgren wrote:
> * Jason Wang <jason77.wang@gmail.com> [101014 19:17]:
>   
>> Grazvydas Ignotas wrote:
>>     
>>> On Thu, Oct 14, 2010 at 5:14 AM, Jason Wang <jason77.wang@gmail.com> wrote:
>>>       
>>>> Grazvydas Ignotas wrote:
>>>>         
>>>>> Working here on pandora too, applied on top of linux-next (did not
>>>>> test suspend though).
>>>>>
>>>>> Not caused by this patch, but the old problem where lower measurement
>>>>> bits get lost is back - when I draw diagonal line it looks like
>>>>> stairs. I think it's caused by OMAP SPI driver, CCing linux-omap.
>>>>>
>>>>>
>>>>>           
>>>> Yes, it is OMAP spi driver issues, I have met the same problem before,
>>>> please refer to:
>>>> http://www.spinics.net/lists/arm-kernel/msg91538.html
>>>>
>>>> you can apply this patch and test again.
>>>>         
>>> It solves my issue, thanks
>>> Tested-by: Grazvydas Ignotas <notasas@gmail.com>
>>>
>>> BTW I have done similar patch almost 2 years ago for exactly the same problem:
>>> http://marc.info/?l=linux-omap&m=122565580616920&w=2
>>> but then problem went away after random kernel update (from what I
>>> remember), and my patch was no longer needed.
>>>
>>>       
>> Really similar problem and similar solution, :-)
>>     
>
> Let's get this fix into the mainline tree then. Jason, care to
> refresh (it's wrapped at least) and then repost with the acks so
> Grant can queue it up for the upcoming merge window?
>
> Acked-by: Tony Lindgren <tony@atomide.com>
>
>   
OK, i will refresh it and re-post it to linux-spi and Grant.

Thanks,
Jason.


^ permalink raw reply	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2010-10-19  1:25 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-09-16 10:51 [PATCH 0/4] Switch ads7846 driver to use soft irq Jason Wang
2010-09-16 10:51 ` [PATCH 1/4] Input: ads7846 - switch to using threaded IRQ Jason Wang
2010-09-16 10:51   ` [PATCH 2/4] Input: ads7846 - add a include header to prevent building fails Jason Wang
2010-09-16 10:51     ` [PATCH 3/4] Input: ads7846 - restore ADC to powerdown mode if no messgaes needed Jason Wang
2010-09-16 10:51       ` [PATCH 4/4] Input: ads7846 - modificatons of _stop()/_disable() conditions Jason Wang
2010-09-17  6:39         ` Dmitry Torokhov
2010-09-17  9:20           ` Jason Wang
2010-09-17 16:07             ` Dmitry Torokhov
2010-09-20  8:18               ` Jason Wang
2010-10-12  9:58                 ` Jason Wang
2010-10-12 16:00                   ` Dmitry Torokhov
2010-10-13  3:12                     ` Jason Wang
2010-10-13 22:17                       ` Grazvydas Ignotas
2010-10-14  2:14                         ` Jason Wang
2010-10-14 21:40                           ` Grazvydas Ignotas
2010-10-15  2:29                             ` Jason Wang
2010-10-18 23:19                               ` Tony Lindgren
2010-10-19  1:25                                 ` Jason Wang
2010-09-17  1:48 ` [PATCH 0/4] Switch ads7846 driver to use soft irq Jason Wang

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.