Skip to contentSkip to conversation

tsc2007: make platform callbacks optional

PATCH In review Richard Röjfors 3 participants 4 messages Jun 23, 2009 – Jul 14, 2009

Subsystems (2)
lore
Share & tools
lore
#0 lore raw

PATCH 2/2 tsc2007: make platform callbacks optional

The platform callbacks are only called if supplied. Makes the driver to fallback on only pressure calculation to decide when the pen is up.

---

drivers/input/touchscreen/tsc2007.c +26 -8
Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
===================================================================
@@ -59,6 +59,10 @@
 #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
 #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
 
+#define PEN_STATE_UP	0x00
+#define PEN_STATE_DOWN	0x01
+#define PEN_STATE_IRQ	0x01
+
 struct ts_event {
 	u16	x;
 	u16	y;
@@ -76,7 +80,7 @@
 	u16			model;
 	u16			x_plate_ohms;
 
-	unsigned		pendown;
+	unsigned		penstate;
 	int			irq;
 
 	int			(*get_pendown_state)(void);
@@ -149,15 +153,18 @@
 	 *
 	 * The only safe way to check for the pen up condition is in the
 	 * work function by reading the pen signal state (it's a GPIO and IRQ).
+	 *
+	 * But sadly we don't always have the possibility to use such callback
+	 * in that case rely on the pressure anyway
 	 */
 	if (rt) {
 		struct input_dev *input = ts->input;
 
-		if (!ts->pendown) {
+		if (ts->penstate != PEN_STATE_DOWN) {
 			dev_dbg(&ts->client->dev, "DOWN\n");
 
 			input_report_key(input, BTN_TOUCH, 1);
-			ts->pendown = 1;
+			ts->penstate = PEN_STATE_DOWN;
 		}
 
 		input_report_abs(input, ABS_X, x);
@@ -168,7 +175,9 @@
 
 		dev_dbg(&ts->client->dev, "point(%4d,%4d), pressure (%4u)\n",
 			x, y, rt);
-	}
+	} else if (!ts->get_pendown_state)
+		/* no callback to check pendown state, use pressure */
+		ts->penstate = PEN_STATE_UP;
 
 	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 }
@@ -195,7 +204,14 @@
 {
 	struct tsc2007 *ts =
 		container_of(to_delayed_work(work), struct tsc2007, work);
-	if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
+
+	/* either we have the pendown callback, and use it, otherwise
+	 * we look at the pendown variable
+	 */
+	if ((ts->get_pendown_state && unlikely(!ts->get_pendown_state()) &&
+		ts->penstate != PEN_STATE_UP) ||
+		(!ts->get_pendown_state &&
+		unlikely(ts->penstate == PEN_STATE_UP))) {
 		struct input_dev *input = ts->input;
 
 		dev_dbg(&ts->client->dev, "UP\n");
@@ -204,7 +220,7 @@
 		input_report_abs(input, ABS_PRESSURE, 0);
 		input_sync(input);
 
-		ts->pendown = 0;
+		ts->penstate = PEN_STATE_UP;
 		enable_irq(ts->irq);
 	} else {
 		/* pen is still down, continue with the measurement */
@@ -219,8 +235,9 @@
 {
 	struct tsc2007 *ts = handle;
 
-	if (likely(ts->get_pendown_state())) {
+	if (!ts->get_pendown_state || likely(ts->get_pendown_state())) {
 		disable_irq_nosync(ts->irq);
+		ts->penstate = PEN_STATE_IRQ;
 		schedule_delayed_work(&ts->work, 0);
 	}
 
@@ -264,7 +281,8 @@
 	ts->get_pendown_state = pdata->get_pendown_state;
 	ts->clear_penirq      = pdata->clear_penirq;
 
-	pdata->init_platform_hw();
+	if (pdata->init_platform_hw)
+		pdata->init_platform_hw();
 
 	snprintf(ts->phys, sizeof(ts->phys),
 		 "%s/input0", dev_name(&client->dev));
#1 lore raw
Quoted 3 lines · On Tue, 23 Jun 2009 13:54:54 +0200 Richard R__jfors…

The platform callbacks are only called if supplied. Makes the driver to fallback on only pressure calculation to decide when the pen is up.

Again, I don't understand the reason for the change from the above description.

Is there some driver in the tree which does not implement ->get_pendown_state()? If so, it will oops, won't it? Which driver is that?

Or is there some other driver which you're developing which does not implement ->get_pendown_state()?

If the latter, why should the problem be solved in this way, rather than implementing an empty ->get_pendown_state() within that driver?

etc. More details, please.

#2 lore raw
Quoted 14 lines · Andrew Morton
Quoted 3 lines · On Tue, 23 Jun 2009 13:54:54 +0200 Richard…

The platform callbacks are only called if supplied. Makes the driver to fallback on only pressure calculation to decide when the pen is up.

Again, I don't understand the reason for the change from the above description.

Is there some driver in the tree which does not implement ->get_pendown_state()? If so, it will oops, won't it? Which driver is that?

Quoted 5 lines

Or is there some other driver which you're developing which does not implement ->get_pendown_state()?

If the latter, why should the problem be solved in this way, rather than implementing an empty ->get_pendown_state() within that driver?

On the board I'm currently writing drivers for we don't have any chance of getting the pendown state, we have to trust the touch controller, (which is not very accurate in all cases).

So we can not implement a dummy function, because there is no dummy default value to return. In that case the function must also do I2C calls to the touch controller, which is the responsibility of this driver.

--Richard

maintainer
#3 lore raw
In reply to Richard Röjfors

Hi Richard,

Quoted 10 lines · Richard Röjfors

The platform callbacks are only called if supplied. Makes the driver to fallback on only pressure calculation to decide when the pen is up.

---
Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
===================================================================

drivers/input/touchscreen/tsc2007.c line 64

Quoted 6 lines
drivers/input/touchscreen/tsc2007.c +3 -0
@@ -59,6 +59,10 @@
 #define READ_X		(ADC_ON_12BIT | TSC2007_MEASURE_X)
 #define PWRDOWN		(TSC2007_12BIT | TSC2007_POWER_OFF_IRQ_EN)
 
+#define PEN_STATE_UP	0x00
+#define PEN_STATE_DOWN	0x01
+#define PEN_STATE_IRQ	0x01

Why the last 2 are the same?

Quoted 35 lines
drivers/input/touchscreen/tsc2007.c +7 -3
+
 struct ts_event {
 	u16	x;
 	u16	y;
@@ -76,7 +80,7 @@
 	u16			model;
 	u16			x_plate_ohms;
 
-	unsigned		pendown;
+	unsigned		penstate;
 	int			irq;
 
 	int			(*get_pendown_state)(void);
@@ -149,15 +153,18 @@
 	 *
 	 * The only safe way to check for the pen up condition is in the
 	 * work function by reading the pen signal state (it's a GPIO and IRQ).
+	 *
+	 * But sadly we don't always have the possibility to use such callback
+	 * in that case rely on the pressure anyway
 	 */
 	if (rt) {
 		struct input_dev *input = ts->input;
 
-		if (!ts->pendown) {
+		if (ts->penstate != PEN_STATE_DOWN) {
 			dev_dbg(&ts->client->dev, "DOWN\n");
 
 			input_report_key(input, BTN_TOUCH, 1);
-			ts->pendown = 1;
+			ts->penstate = PEN_STATE_DOWN;
 		}
 
 		input_report_abs(input, ABS_X, x);

drivers/input/touchscreen/tsc2007.c line 181

Quoted 8 lines
drivers/input/touchscreen/tsc2007.c +3 -1
@@ -168,7 +175,9 @@
 
 		dev_dbg(&ts->client->dev, "point(%4d,%4d), pressure (%4u)\n",
 			x, y, rt);
-	}
+	} else if (!ts->get_pendown_state)
+		/* no callback to check pendown state, use pressure */
+		ts->penstate = PEN_STATE_UP;
 

Since we are not going to re-check pen state why don't we report "pen up" event here right away and forego rescheduling the work?

drivers/input/touchscreen/tsc2007.c line 183

Quoted 2 lines
drivers/input/touchscreen/tsc2007.c +0 -0
 	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
 }

Thanks.

Signature · 5 lines
Dmitry
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html