OMAP5912 touchscreen irq not working

Hi,

using recent git on OMAP5912 OSK with Mistral module touchscreen irq isn't working. Using configuration from my previous mail, using ts_calibrate no input is recognized.

Can anybody else reproduce this? Maybe still anything with latest GPIO irq changes?

Best regards

Dirk

Just tried it ... /proc/interrupts shows a few IRQs, but I get no touchscreen data.

- Dave

I noticed that the ads7846 sampling rate is set too high in board-osk.c. The spec allows a maximum bus frequency of 2.5MHz which gives a ~96k sample rate. There is also the uwire clock patch in my previous post, which might have an effect on this issue.

--Imre

index e0711d2..bc4269b 100644
@@ -317,7 +317,7 @@ static struct spi_board_info __initdata 
 	.modalias		= "ads7846",
 	.platform_data		= &mistral_ts_info,
 	.irq			= OMAP_GPIO_IRQ(4),
-	.max_speed_hz		= 120000 /* max sample rate at 3V */
+	.max_speed_hz		= 96000 /* max sample rate at 3V */
 					* 26 /* command + data + overhead */,
 	.bus_num		= 2,
 	.chip_select		= 0,

The spec says max sample rate of 125 KHz (at 5V) and describes Fclk in terms of Fsample (at an extremely fast 16 bit/sample rate) ... I saw no limit for Fclk, it's a function of the sample rate.

Regardless, the max sample frequency at 3V is not 96 KHz! The graph shows 120 KHz... 96 KHz would be for about 2.1V.

Unlikely; it worked before, why would it stop working suddenly? Including with the previous SPI clock... it's some other change causing the breakage.

- Dave

There is one table giving min/max timing values for Fclk as well and it specifies 400ns minimum clock period. We are using the 24 bit/sample mode (8 for command, 16 for data) which at 120kHz/sample rate would need
347ns clock period.

I think, there should be a graph for each mode (15, 16, 24 bit/sample), that one is valid for 16 bit/sample only.

Yes, if it worked before with the wrong clock it's probably something else.

--Imre

Had a look to it and added some debug printks to ads7846.c. One in ads7846_irq() and two in ads7846_rx(). There, one before check if samples to be thrown away, one behind (see below).

Looking at ads7846_irq(), while initalization, I get "ads7846 spi2.0: touchscreen + hwmon, irq 164"

With this irq, I only get two outputs of "ads_irq". The first directly while initialization (without touching the screen), the second at first screen touch. Then nothing, regardless how often I touch the screen again. This is reflected by /proc/interrupts as well: 164: 2 spi2.0. I think it's wrong to get ads7846_irq() called already while init without screen touched? And then this irq seems to be locked after first touch.

Looking at ads7846_rx, I permanently get "ads7846_rx called". But never "ads7846_rx valid sample". Is it correct to call ads7846_rx permanently? Shouldn't ads7846_rx() be called only while pen is down?

Please correct me if I'm wrong here, but with irq called at init without screen touched, then after one touch locked, and permanently called ads7846_rx() throwing away all samples, it seems to me that there is anything basically wrong with this new code? At least on OSK with mistral.

Best regards

Dirk

@@ -410,6 +410,8 @@ static void ads7846_rx(void *ads)
        } else
                Rt = 0;
 
+               printk("### ads7846_rx called\n");
+
        /* 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 */
@@ -418,6 +420,8 @@ static void ads7846_rx(void *ads)

                return;
        }

+ printk("### ads7846_rx valid sample\n");
+

        /* NOTE:  "pendown" is inferred from pressure; we 

don't rely on
* being able to check nPENIRQ status, or
"friendly" trigger modes
* (both-edges is much better than just-falling or
low-level).
@@ -543,6 +547,8 @@ static irqreturn_t ads7846_irq(int irq,

        struct ads7846 *ts = handle;
        unsigned long flags;

+ printk("ads_irq\n");
+

        spin_lock_irqsave(&ts->lock, flags);
        if (likely(ts->get_pendown_state())) {
                if (!ts->irq_disabled) {

Locking out after first touch is a problem, of course.

IRQ during init happens with some hardware; I recall concluding it wasn't a problem here, though the details of that have been discarded like a used coffee filter.

ISTR one issue is that sampling will trigger an IRQ, so the driver needs to throttle the IRQs.

There were glitches related to the problem with irqs not getting disabled on request, with comments along the lines of "remove this when the irqs work right" ... I think it's time to maybe sort that issue out.

That said, this problem did not exist before the GPIO IRQ patches were merged ... I certainly saw correct IRQ behavior from my testing.

- Dave

Yes, during init we send send a couple of commands - with IRQs disabled
- to put the device in the initial waiting-for-IRQ state. That will also result in a latched interrupt which will be delivered as soon as we enable the IRQ again.

This spurious IRQ could be avoided by set_irq_type(IRQT_NONE) which would prevent even the latching of it.

In the current code this shouldn't be a problem, since in the IRQ handler we check the actual pen status.

Yes, I think the irq_disabled flag is not necessary any more.

Might be, but this looks more to me to do something with values returned to the _rx() function. Since it's continually called I suppose it never detects the pen up which it determines from the pressure value. Would help if you could enable the pr_debug in the _rx() function to see the returned values.

--Imre

[ Didn't try on OSK, but seems to be the problem you had there. ]

When filtering is disabled the driver will ignore all samples and never detect the pen up event.

index 4369845..2d8ea07 100644
@@ -472,7 +474,8 @@ static void ads7846_debounce(void *ads)
 	m = &ts->msg[ts->msg_idx];
 	t = list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
 	val = (be16_to_cpu(*(__be16 *)t->rx_buf) >> 3) & 0x0fff;
-	if (!ts->read_cnt || (abs(ts->last_read - val) > ts->debounce_tol)) {
+	if (ts->debounce_max && (
+	    !ts->read_cnt || (abs(ts->last_read - val) > ts->debounce_tol))) {
 		/* Repeat it, if this was the first read or the read
 		 * wasn't consistent enough. */
 		if (ts->read_cnt < ts->debounce_max) {

@@ -702,14 +705,9 @@ static int __devinit ads7846_probe(struc
 	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->debounce_max) {
-		ts->debounce_max = pdata->debounce_max;
-		ts->debounce_tol = pdata->debounce_tol;
-		ts->debounce_rep = pdata->debounce_rep;
-		if (ts->debounce_rep > ts->debounce_max + 1)
-			ts->debounce_rep = ts->debounce_max - 1;
-	} else
-		ts->debounce_tol = ~0;
+	ts->debounce_max = pdata->debounce_max;
+	ts->debounce_tol = pdata->debounce_tol;
+	ts->debounce_rep = pdata->debounce_rep;
 	ts->get_pendown_state = pdata->get_pendown_state;
 
 	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", spi->dev.bus_id);

ts_calibrate now works on OSK, but, uhmm, not very good. Because the reaction of the cursor cross is so slow I first touched it several times until it moved to the next position. Then I noticed that it can take 1-2s after touch until it moves.

Using ts_test, the cursor cross is jumping wild if screen is touched, but not usable. I wasn't able to switch from Drag to Draw mode because I couldn't hit Draw.

Sorry, but at least on OSK with Mistral this new stuff isn't usable. Old driver worked quite well.

Regards

Dirk

Thanks a lot for trying it. I don't have an OSK board, so I can't test it myself, but I found some displays having the same problem.

Currently the ads7846 driver depends on the the pressure value to determine when the pen is lifted, which can cause the problem you saw.

I think the two problems you described are related. The long delay after lifting the pen is b/c the pressure value settles rather slow at the level we interpret as a pen up. The cursor jumping on the screen can be the result of readings during this time, that is when the pen is already lifted.

The solution would be to use the IRQ line state - as we already do to determine the pen down - for the pen up event as well.

Following is a patch that does this. Could you give it a try, maybe with debugging enabled?

Yes, I checked the old driver and it uses the pen IRQ for pen down / up events, so that might be the difference.

Thanks,
Imre

index 4369845..94c36b1 100644
@@ -370,6 +370,39 @@ static DEVICE_ATTR(disable, 0664, ads784
 
 /*--------------------------------------------------------------------------*/
 
+static void ads7846_report_pen_state(struct ads7846 *ts, int down)
+{
+	struct input_dev	*input_dev = ts->input;
+
+	input_report_key(input_dev, BTN_TOUCH, down);
+	if (!down)
+		input_report_abs(input_dev, ABS_PRESSURE, 0);
+#ifdef VERBOSE
+	pr_debug("%s: %s\n", ts->spi->dev.bus_id, down ? "DOWN" : "UP");
+#endif
+}
+
+static void ads7846_report_pen_position(struct ads7846 *ts, int x, int y,
+					int pressure)
+{
+	struct input_dev	*input_dev = ts->input;
+
+	input_report_abs(input_dev, ABS_X, x);
+	input_report_abs(input_dev, ABS_Y, y);
+	input_report_abs(input_dev, ABS_PRESSURE, pressure);
+
+#ifdef VERBOSE
+	pr_debug("%s: %d/%d/%d\n", ts->spi->dev.bus_id, x, y, pressure);
+#endif
+}
+
+static void ads7846_sync_events(struct ads7846 *ts)
+{
+	struct input_dev	*input_dev = ts->input;
+
+	input_sync(input_dev);
+}
+
 /*
  * PENIRQ only kicks the timer.  The timer only reissues the SPI transfer,
  * to retrieve touchscreen status.

@@ -381,11 +414,8 @@ static DEVICE_ATTR(disable, 0664, ads784
 static void ads7846_rx(void *ads)
 {
 	struct ads7846		*ts = ads;
-	struct input_dev	*input_dev = ts->input;
 	unsigned		Rt;
-	unsigned		sync = 0;
 	u16			x, y, z1, z2;
-	unsigned long		flags;
 
 	/* adjust:  on-wire is a must-ignore bit, a BE12 value, then padding;
 	 * built from two 8 bit values written msb-first.

@@ -399,7 +429,7 @@ static void ads7846_rx(void *ads)
 	if (x == MAX_12BIT)
 		x = 0;
 
-	if (likely(x && z1 && !device_suspended(&ts->spi->dev))) {
+	if (likely(x && z1)) {
 		/* compute touch pressure resistance using equation #2 */
 		Rt = z2;
 		Rt -= z1;

@@ -414,51 +444,31 @@ static void ads7846_rx(void *ads)
 	* the maximum. Don't report it to user space, repeat at least
 	* once more the measurement */
 	if (ts->tc.ignore || Rt > ts->pressure_max) {
+#ifdef VERBOSE
+		pr_debug("%s: ignored %d pressure %d\n",
+			ts->spi->dev.bus_id, ts->tc.ignore, Rt);
+#endif
 		mod_timer(&ts->timer, jiffies + TS_POLL_PERIOD);
 		return;
 	}
 
-	/* NOTE:  "pendown" is inferred from pressure; we don't rely on
-	 * being able to check nPENIRQ status, or "friendly" trigger modes
-	 * (both-edges is much better than just-falling or low-level).
-	 *
-	 * REVISIT:  some boards may require reading nPENIRQ; it's
-	 * needed on 7843.  and 7845 reads pressure differently...
-	 *
-	 * REVISIT:  the touchscreen might not be connected; this code
-	 * won't notice that, even if nPENIRQ never fires ...
+	/* NOTE: We can't rely on the pressure to determine the pen down
+	 * state. 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. The only safe way to check for the pen up
+	 * condition is in the timer by reading the pen IRQ state.
 	 */
-	if (!ts->pendown && Rt != 0) {
-		input_report_key(input_dev, BTN_TOUCH, 1);
-		sync = 1;
-	} else if (ts->pendown && Rt == 0) {
-		input_report_key(input_dev, BTN_TOUCH, 0);
-		sync = 1;
-	}
-
 	if (Rt) {
-		input_report_abs(input_dev, ABS_X, x);
-		input_report_abs(input_dev, ABS_Y, y);
-		sync = 1;
-	}
-
-	if (sync) {
-		input_report_abs(input_dev, ABS_PRESSURE, Rt);
-		input_sync(input_dev);
+		if (!ts->pendown) {
+			ads7846_report_pen_state(ts, 1);
+			ts->pendown = 1;
+		}
+		ads7846_report_pen_position(ts, x, y, Rt);
+		ads7846_sync_events(ts);
 	}
 
-#ifdef	VERBOSE
-	if (Rt || ts->pendown)
-		pr_debug("%s: %d/%d/%d%s\n", ts->spi->dev.bus_id,
-			x, y, Rt, Rt ? "" : " UP");
-#endif
 
-	spin_lock_irqsave(&ts->lock, flags);
-
-	ts->pendown = (Rt != 0);
 	mod_timer(&ts->timer, jiffies + TS_POLL_PERIOD);
-
-	spin_unlock_irqrestore(&ts->lock, flags);
 }
 
 static void ads7846_debounce(void *ads)

@@ -472,7 +482,8 @@ static void ads7846_debounce(void *ads)
 	m = &ts->msg[ts->msg_idx];
 	t = list_entry(m->transfers.prev, struct spi_transfer, transfer_list);
 	val = (be16_to_cpu(*(__be16 *)t->rx_buf) >> 3) & 0x0fff;
-	if (!ts->read_cnt || (abs(ts->last_read - val) > ts->debounce_tol)) {
+	if (ts->debounce_max && (
+	    !ts->read_cnt || (abs(ts->last_read - val) > ts->debounce_tol))) {
 		/* Repeat it, if this was the first read or the read
 		 * wasn't consistent enough. */
 		if (ts->read_cnt < ts->debounce_max) {

@@ -519,14 +530,20 @@ static void ads7846_timer(unsigned long 
 
 	spin_lock_irq(&ts->lock);
 
-	if (unlikely(ts->msg_idx && !ts->pendown)) {
+	if (unlikely(!ts->get_pendown_state() ||
+		     device_suspended(&ts->spi->dev))) {
+		if (ts->pendown) {
+			ads7846_report_pen_state(ts, 0);
+			ads7846_sync_events(ts);
+			ts->pendown = 0;
+		}
+
 		/* measurment cycle ended */
 		if (!device_suspended(&ts->spi->dev)) {
 			ts->irq_disabled = 0;
 			enable_irq(ts->spi->irq);
 		}
 		ts->pending = 0;
-		ts->msg_idx = 0;
 	} else {
 		/* pen is still down, continue with the measurement */
 		ts->msg_idx = 0;

@@ -702,14 +719,9 @@ static int __devinit ads7846_probe(struc
 	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->debounce_max) {
-		ts->debounce_max = pdata->debounce_max;
-		ts->debounce_tol = pdata->debounce_tol;
-		ts->debounce_rep = pdata->debounce_rep;
-		if (ts->debounce_rep > ts->debounce_max + 1)
-			ts->debounce_rep = ts->debounce_max - 1;
-	} else
-		ts->debounce_tol = ~0;
+	ts->debounce_max = pdata->debounce_max;
+	ts->debounce_tol = pdata->debounce_tol;
+	ts->debounce_rep = pdata->debounce_rep;
 	ts->get_pendown_state = pdata->get_pendown_state;
 
 	snprintf(ts->phys, sizeof(ts->phys), "%s/input0", spi->dev.bus_id);

Well, there have been recent regressions, but the new code _has_ worked quite well in the past ...

Using the pen IRQ for driver state transitions is problematic, since the driver has to work with a variety of triggering modes ... both edges, falling only (or, with hw inverter, rising only), low level (or, with inverter, high).

It's true that having that "poll penirq state" method is a big win.

- Dave

Thanks for the patches.

Ah! This works. At least on OSK ;)

Best regards

Dirk

Ok, it would be good to do some comparison on the readings before and after the breakage. I'll try to get a board and figure this out.

I understand, but since on some platforms I think it is the only way to go and on others it may reduce the latency of pen up events could we still have this feature as an option? If the platform can't define it's pen down state function we could fall back on the current pen up detection method.

--Imre

Certainly. I was never a big fan of that method, but it seemed to be necessary ... if, after sorting out interrelated bugs in gpio irq handling, omap_uwire, and ads7846 code, it all works without that messy pressure sense-as-penup logic, I'd be happy!

- Dave