[PATCH 2/2] tsc2007: make platform callbacks optional

[PATCH 2/2] tsc2007: make platform callbacks optional

[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.

Signed-off-by: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>
---
Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
===================================================================
--- linux-2.6.30/drivers/input/touchscreen/tsc2007.c	(revision 943)
+++ linux-2.6.30/drivers/input/touchscreen/tsc2007.c	(revision 945)
@@ -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));

Re: [PATCH 2/2] tsc2007: make platform callbacks optional

[Andrew Morton]

On Tue, 23 Jun 2009 13:54:54 +0200
Richard R__jfors <richard.rojfors.ext@mocean-labs.com> wrote:

> 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.

Re: [PATCH 2/2] tsc2007: make platform callbacks optional

[Richard Röjfors]

On 6/25/09 11:23 PM, Andrew Morton wrote:
> On Tue, 23 Jun 2009 13:54:54 +0200
> Richard R__jfors<richard.rojfors.ext@mocean-labs.com>  wrote:
>
>> 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?

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

Re: [PATCH 2/2] tsc2007: make platform callbacks optional

[Dmitry Torokhov]

Hi Richard,

On Tue, Jun 23, 2009 at 01:54:54PM +0200, Richard Röjfors wrote:
> The platform callbacks are only called if supplied. Makes the driver
> to fallback on only pressure calculation to decide when the pen is up.
> 
> Signed-off-by: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>
> ---
> Index: linux-2.6.30/drivers/input/touchscreen/tsc2007.c
> ===================================================================
> --- linux-2.6.30/drivers/input/touchscreen/tsc2007.c	(revision 943)
> +++ linux-2.6.30/drivers/input/touchscreen/tsc2007.c	(revision 945)
> @@ -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?

> +
>  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;
> 

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?

>  	schedule_delayed_work(&ts->work, TS_POLL_PERIOD);
>  }

Thanks.

-- 
Dmitry
--
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