[PATCH 2/2] tsc2007: reduced number of I2C transfers

[PATCH 2/2] tsc2007: reduced number of I2C transfers

[Richard Röjfors]

Decreases the number of I2C transactions transferred by the driver.
During probe we don't need to ask for the coordinates from the controller.
When polling the controller we don't need to power down and enable IRQ 
if we are going to poll again.

Signed-off-by: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>
---
Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
===================================================================
--- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1040)
+++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1053)
@@ -178,6 +178,12 @@
  		ts->penstate = PEN_STATE_UP;
  }

+static void tsc2007_power_down(struct tsc2007 *tsc)
+{
+	/* power down */
+	tsc2007_xfer(tsc, PWRDOWN);
+}
+
  static int tsc2007_read_values(struct tsc2007 *tsc)
  {
  	/* y- still on; turn on only y+ (and ADC) */
@@ -188,11 +194,8 @@

  	/* turn y+ off, x- on; we'll use formula #1 */
  	tsc->tc.z1 = tsc2007_xfer(tsc, READ_Z1);
-	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2);
+	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2 | TSC2007_POWER_OFF_IRQ_EN);

-	/* power down */
-	tsc2007_xfer(tsc, PWRDOWN);
-
  	return 0;
  }

@@ -217,6 +220,7 @@
  		input_sync(input);

  		ts->penstate = PEN_STATE_UP;
+		tsc2007_power_down(ts);
  		enable_irq(ts->irq);
  	} else {
  		/* pen is still down, continue with the measurement */
@@ -305,7 +309,7 @@
  	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, 0, 0);
  	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT, 0, 0);

-	tsc2007_read_values(ts);
+	tsc2007_power_down(ts);

  	ts->irq = client->irq;
--
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

Re: [PATCH 2/2] tsc2007: reduced number of I2C transfers

[Dmitry Torokhov]

On Fri, Jul 24, 2009 at 06:14:37PM +0200, Richard Röjfors wrote:
> Decreases the number of I2C transactions transferred by the driver.
> During probe we don't need to ask for the coordinates from the controller.
> When polling the controller we don't need to power down and enable IRQ  
> if we are going to poll again.
>
> Signed-off-by: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>
> ---
> Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
> ===================================================================
> --- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1040)
> +++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1053)
> @@ -178,6 +178,12 @@
>  		ts->penstate = PEN_STATE_UP;
>  }
>
> +static void tsc2007_power_down(struct tsc2007 *tsc)
> +{
> +	/* power down */
> +	tsc2007_xfer(tsc, PWRDOWN);
> +}
> +
>  static int tsc2007_read_values(struct tsc2007 *tsc)
>  {
>  	/* y- still on; turn on only y+ (and ADC) */
> @@ -188,11 +194,8 @@
>
>  	/* turn y+ off, x- on; we'll use formula #1 */
>  	tsc->tc.z1 = tsc2007_xfer(tsc, READ_Z1);
> -	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2);
> +	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2 | TSC2007_POWER_OFF_IRQ_EN);

I think this leaves the controller powered on and with with PENIRQ
disabled.

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

Re: [PATCH 2/2] tsc2007: reduced number of I2C transfers

[Richard Röjfors]

On 7/24/09 8:00 PM, Dmitry Torokhov wrote:
> On Fri, Jul 24, 2009 at 06:14:37PM +0200, Richard Röjfors wrote:
>> Decreases the number of I2C transactions transferred by the driver.
>> During probe we don't need to ask for the coordinates from the controller.
>> When polling the controller we don't need to power down and enable IRQ
>> if we are going to poll again.
>>
>> Signed-off-by: Richard Röjfors<richard.rojfors.ext@mocean-labs.com>
>> ---
>> Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
>> ===================================================================
>> --- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1040)
>> +++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1053)
>> @@ -178,6 +178,12 @@
>>   		ts->penstate = PEN_STATE_UP;
>>   }
>>
>> +static void tsc2007_power_down(struct tsc2007 *tsc)
>> +{
>> +	/* power down */
>> +	tsc2007_xfer(tsc, PWRDOWN);
>> +}
>> +
>>   static int tsc2007_read_values(struct tsc2007 *tsc)
>>   {
>>   	/* y- still on; turn on only y+ (and ADC) */
>> @@ -188,11 +194,8 @@
>>
>>   	/* turn y+ off, x- on; we'll use formula #1 */
>>   	tsc->tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>> -	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>> +	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2 | TSC2007_POWER_OFF_IRQ_EN);
>
> I think this leaves the controller powered on and with with PENIRQ
> disabled.
>

You are right, I think we should leave the patch like below, just get 
rid of the unnecessary read during startup.

--Richard

Input: tsc2007: Do not read coordinated when probing driver

From: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>

Don't read coordinates during probe of the driver, just powering down 
the controller and wait for interrupts.


Signed-off-by: Richard Röjfors<richard.rojfors.ext@mocean-labs.com>
---
Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
===================================================================
--- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1040)
+++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1056)
@@ -178,6 +178,12 @@
  		ts->penstate = PEN_STATE_UP;
  }

+static void tsc2007_power_down(struct tsc2007 *tsc)
+{
+	/* power down */
+	tsc2007_xfer(tsc, PWRDOWN);
+}
+
  static int tsc2007_read_values(struct tsc2007 *tsc)
  {
  	/* y- still on; turn on only y+ (and ADC) */
@@ -190,8 +196,7 @@
  	tsc->tc.z1 = tsc2007_xfer(tsc, READ_Z1);
  	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2);

-	/* power down */
-	tsc2007_xfer(tsc, PWRDOWN);
+	tsc2007_power_down(tsc);

  	return 0;
  }
@@ -305,7 +310,7 @@
  	input_set_abs_params(input_dev, ABS_Y, 0, MAX_12BIT, 0, 0);
  	input_set_abs_params(input_dev, ABS_PRESSURE, 0, MAX_12BIT, 0, 0);

-	tsc2007_read_values(ts);
+	tsc2007_power_down(ts);

  	ts->irq = client->irq;

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

Re: [PATCH 2/2] tsc2007: reduced number of I2C transfers

[Dmitry Torokhov]

On Fri, Jul 24, 2009 at 09:37:27PM +0200, Richard Röjfors wrote:
> On 7/24/09 8:00 PM, Dmitry Torokhov wrote:
>> On Fri, Jul 24, 2009 at 06:14:37PM +0200, Richard Röjfors wrote:
>>> Decreases the number of I2C transactions transferred by the driver.
>>> During probe we don't need to ask for the coordinates from the controller.
>>> When polling the controller we don't need to power down and enable IRQ
>>> if we are going to poll again.
>>>
>>> Signed-off-by: Richard Röjfors<richard.rojfors.ext@mocean-labs.com>
>>> ---
>>> Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
>>> ===================================================================
>>> --- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1040)
>>> +++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1053)
>>> @@ -178,6 +178,12 @@
>>>   		ts->penstate = PEN_STATE_UP;
>>>   }
>>>
>>> +static void tsc2007_power_down(struct tsc2007 *tsc)
>>> +{
>>> +	/* power down */
>>> +	tsc2007_xfer(tsc, PWRDOWN);
>>> +}
>>> +
>>>   static int tsc2007_read_values(struct tsc2007 *tsc)
>>>   {
>>>   	/* y- still on; turn on only y+ (and ADC) */
>>> @@ -188,11 +194,8 @@
>>>
>>>   	/* turn y+ off, x- on; we'll use formula #1 */
>>>   	tsc->tc.z1 = tsc2007_xfer(tsc, READ_Z1);
>>> -	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2);
>>> +	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2 | TSC2007_POWER_OFF_IRQ_EN);
>>
>> I think this leaves the controller powered on and with with PENIRQ
>> disabled.
>>
>
> You are right, I think we should leave the patch like below, just get  
> rid of the unnecessary read during startup.
>

I modified the patch a bit before applying. In fact there were a few
changes to all the patces so it would be nice if you could take a look
at the 'next' branch of my tree:

	git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next

Also, I di dnot quite like the logic of the patch making
get_pendown_state callback optional; could you please try the patch
below and let me know of it works for you?

Thanks!

-- 
Dmitry


Input: tsc2007 - make get_pendown_state platform callback optional

In cases when get_pendown_state callback is not available have
the driver to fallback on pressure calculation to determine if
the pen is up.

Signed-off-by: Dmitry Torokhov <dtor@mail.ru>
---

 drivers/input/touchscreen/tsc2007.c |  170 +++++++++++++++++++----------------
 1 files changed, 92 insertions(+), 78 deletions(-)


diff --git a/drivers/input/touchscreen/tsc2007.c b/drivers/input/touchscreen/tsc2007.c
index f710af4..ac5e0e8 100644
--- a/drivers/input/touchscreen/tsc2007.c
+++ b/drivers/input/touchscreen/tsc2007.c
@@ -70,7 +70,6 @@ struct tsc2007 {
 	struct input_dev	*input;
 	char			phys[32];
 	struct delayed_work	work;
-	struct ts_event		tc;
 
 	struct i2c_client	*client;
 
@@ -106,51 +105,96 @@ static inline int tsc2007_xfer(struct tsc2007 *tsc, u8 cmd)
 	return val;
 }
 
-static void tsc2007_send_event(void *tsc)
+static void tsc2007_read_values(struct tsc2007 *tsc, struct ts_event *tc)
 {
-	struct tsc2007	*ts = tsc;
-	u32		rt;
-	u16		x, y, z1, z2;
+	/* y- still on; turn on only y+ (and ADC) */
+	tc->y = tsc2007_xfer(tsc, READ_Y);
+
+	/* turn y- off, x+ on, then leave in lowpower */
+	tc->x = tsc2007_xfer(tsc, READ_X);
+
+	/* turn y+ off, x- on; we'll use formula #1 */
+	tc->z1 = tsc2007_xfer(tsc, READ_Z1);
+	tc->z2 = tsc2007_xfer(tsc, READ_Z2);
 
-	x = ts->tc.x;
-	y = ts->tc.y;
-	z1 = ts->tc.z1;
-	z2 = ts->tc.z2;
+	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
+	tsc2007_xfer(tsc, PWRDOWN);
+}
+
+static u32 tsc2007_calculate_pressure(struct tsc2007 *tsc, struct ts_event *tc)
+{
+	u32 rt = 0;
 
 	/* range filtering */
-	if (x == MAX_12BIT)
-		x = 0;
+	if (tc->x == MAX_12BIT)
+		tc->x = 0;
 
-	if (likely(x && z1)) {
+	if (likely(tc->x && tc->z1)) {
 		/* compute touch pressure resistance using equation #1 */
-		rt = z2;
-		rt -= z1;
-		rt *= x;
-		rt *= ts->x_plate_ohms;
-		rt /= z1;
+		rt = tc->z2 - tc->z1;
+		rt *= tc->x;
+		rt *= tsc->x_plate_ohms;
+		rt /= tc->z1;
 		rt = (rt + 2047) >> 12;
-	} else
-		rt = 0;
-
-	/*
-	 * 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 (rt > MAX_12BIT) {
-		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
-		return;
 	}
 
+	return rt;
+}
+
+static void tsc2007_send_up_event(struct tsc2007 *tsc)
+{
+	struct input_dev *input = tsc->input;
+
+	dev_dbg(&tsc->client->dev, "UP\n");
+
+	input_report_key(input, BTN_TOUCH, 0);
+	input_report_abs(input, ABS_PRESSURE, 0);
+	input_sync(input);
+}
+
+static void tsc2007_work(struct work_struct *work)
+{
+	struct tsc2007 *ts =
+		container_of(to_delayed_work(work), struct tsc2007, work);
+	struct ts_event tc;
+	u32 rt;
+
 	/*
 	 * 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.
+	 * state, even though 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.
 	 *
 	 * 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).
+	 * work function by reading the pen signal state (it's a GPIO
+	 * and IRQ). Unfortunately such callback is not always available,
+	 * in that case we have rely on the pressure anyway.
 	 */
+	if (ts->get_pendown_state) {
+		if (unlikely(!ts->get_pendown_state())) {
+			tsc2007_send_up_event(ts);
+			ts->pendown = false;
+			goto out;
+		}
+
+		dev_dbg(&ts->client->dev, "pen is still down\n");
+	}
+
+	tsc2007_read_values(ts, &tc);
+
+	rt = tsc2007_calculate_pressure(ts, &tc);
+	if (rt > MAX_12BIT) {
+		/*
+		 * 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.
+		 */
+		dev_dbg(&ts->client->dev, "ignored pressure %d\n", rt);
+		goto out;
+
+	}
+
 	if (rt) {
 		struct input_dev *input = ts->input;
 
@@ -161,68 +205,38 @@ static void tsc2007_send_event(void *tsc)
 			ts->pendown = true;
 		}
 
-		input_report_abs(input, ABS_X, x);
-		input_report_abs(input, ABS_Y, y);
+		input_report_abs(input, ABS_X, tc.x);
+		input_report_abs(input, ABS_Y, tc.y);
 		input_report_abs(input, ABS_PRESSURE, rt);
 
 		input_sync(input);
 
 		dev_dbg(&ts->client->dev, "point(%4d,%4d), pressure (%4u)\n",
-			x, y, rt);
-	}
-}
-
-static int tsc2007_read_values(struct tsc2007 *tsc)
-{
-	/* y- still on; turn on only y+ (and ADC) */
-	tsc->tc.y = tsc2007_xfer(tsc, READ_Y);
-
-	/* turn y- off, x+ on, then leave in lowpower */
-	tsc->tc.x = tsc2007_xfer(tsc, READ_X);
-
-	/* turn y+ off, x- on; we'll use formula #1 */
-	tsc->tc.z1 = tsc2007_xfer(tsc, READ_Z1);
-	tsc->tc.z2 = tsc2007_xfer(tsc, READ_Z2);
-
-	/* Prepare for next touch reading - power down ADC, enable PENIRQ */
-	tsc2007_xfer(tsc, PWRDOWN);
-
-	return 0;
-}
-
-static void tsc2007_work(struct work_struct *work)
-{
-	struct tsc2007 *ts =
-		container_of(to_delayed_work(work), struct tsc2007, work);
-
-	if (unlikely(!ts->get_pendown_state() && ts->pendown)) {
-		struct input_dev *input = ts->input;
-
-		dev_dbg(&ts->client->dev, "UP\n");
-
-		input_report_key(input, BTN_TOUCH, 0);
-		input_report_abs(input, ABS_PRESSURE, 0);
-		input_sync(input);
+			tc.x, tc.y, rt);
 
+	} else if (!ts->get_pendown_state && ts->pendown) {
+		/*
+		 * We don't have callback to check pendown state, so we
+		 * have to assume that since pressure reported is 0 the
+		 * pen was lifted up.
+		 */
+		tsc2007_send_up_event(ts);
 		ts->pendown = false;
-		enable_irq(ts->irq);
-	} else {
-		/* pen is still down, continue with the measurement */
-		dev_dbg(&ts->client->dev, "pen is still down\n");
-
-		tsc2007_read_values(ts);
-		tsc2007_send_event(ts);
+	}
 
+ out:
+	if (ts->pendown)
 		schedule_delayed_work(&ts->work,
 				      msecs_to_jiffies(TS_POLL_PERIOD));
-	}
+	else
+		enable_irq(ts->irq);
 }
 
 static irqreturn_t tsc2007_irq(int irq, void *handle)
 {
 	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);
 		schedule_delayed_work(&ts->work,
 				      msecs_to_jiffies(TS_POLL_DELAY));

Re: [PATCH 2/2] tsc2007: reduced number of I2C transfers

[Richard Röjfors]

Dmitry Torokhov wrote:
> 
> I modified the patch a bit before applying. In fact there were a few
> changes to all the patces so it would be nice if you could take a look
> at the 'next' branch of my tree:
> 
> 	git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
> 
> Also, I di dnot quite like the logic of the patch making
> get_pendown_state callback optional; could you please try the patch
> below and let me know of it works for you?

I like your changes, there is just a small miss while checking the 
platform data. You forgot to remove the check for get_pen_down_state in 
the probe function, see the patch below (to be applied on top of yours):

I also would like a check if the power down failed or not during probe.

--Richard

Input: tsc2007 - Check if I2C communication works during probe

Check the result when sending the power down command to the controller.

Signed-off-by: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>
---
Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
===================================================================
--- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1071)
+++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1073)
@@ -269,7 +269,7 @@
  	struct input_dev *input_dev;
  	int err;

-	if (!pdata || !pdata->get_pendown_state) {
+	if (!pdata) {
  		dev_err(&client->dev, "platform data is required!\n");
  		return -EINVAL;
  	}
@@ -326,10 +326,14 @@
  	i2c_set_clientdata(client, ts);

  	/* Prepare for touch readings - power down ADC and enable PENIRQ */
-	tsc2007_xfer(ts, PWRDOWN);
+	err = tsc2007_xfer(ts, PWRDOWN);
+	if (err < 0)
+		goto err_unreg_dev;

  	return 0;

+ err_unreg_dev:
+	input_unregister_device(ts->input);
   err_free_irq:
  	tsc2007_free_irq(ts);
  	if (pdata->exit_platform_hw)
--
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

Re: [PATCH 2/2] tsc2007: reduced number of I2C transfers

[Dmitry Torokhov]

On Tue, Aug 04, 2009 at 02:12:35PM +0200, Richard Röjfors wrote:
> Dmitry Torokhov wrote:
>>
>> I modified the patch a bit before applying. In fact there were a few
>> changes to all the patces so it would be nice if you could take a look
>> at the 'next' branch of my tree:
>>
>> 	git://git.kernel.org/pub/scm/linux/kernel/git/dtor/input.git next
>>
>> Also, I di dnot quite like the logic of the patch making
>> get_pendown_state callback optional; could you please try the patch
>> below and let me know of it works for you?
>
> I like your changes, there is just a small miss while checking the  
> platform data. You forgot to remove the check for get_pen_down_state in  
> the probe function, see the patch below (to be applied on top of yours):
>
> I also would like a check if the power down failed or not during probe.
>
> --Richard
>
> Input: tsc2007 - Check if I2C communication works during probe
>
> Check the result when sending the power down command to the controller.
>
> Signed-off-by: Richard Röjfors <richard.rojfors.ext@mocean-labs.com>
> ---
> Index: linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c
> ===================================================================
> --- linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1071)
> +++ linux-2.6.31-rc2/drivers/input/touchscreen/tsc2007.c	(revision 1073)
> @@ -269,7 +269,7 @@
>  	struct input_dev *input_dev;
>  	int err;
>
> -	if (!pdata || !pdata->get_pendown_state) {
> +	if (!pdata) {
>  		dev_err(&client->dev, "platform data is required!\n");
>  		return -EINVAL;
>  	}


Ah, yes, thanks.

> @@ -326,10 +326,14 @@
>  	i2c_set_clientdata(client, ts);
>
>  	/* Prepare for touch readings - power down ADC and enable PENIRQ */
> -	tsc2007_xfer(ts, PWRDOWN);
> +	err = tsc2007_xfer(ts, PWRDOWN);
> +	if (err < 0)
> +		goto err_unreg_dev;
>
>  	return 0;
>
> + err_unreg_dev:
> +	input_unregister_device(ts->input);

This will case double free.. I will adjust locally.

>   err_free_irq:
>  	tsc2007_free_irq(ts);
>  	if (pdata->exit_platform_hw)

-- 
Dmitry