Re: [PATCH v2] input: add synaptics_i2c driver

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi Mike,

On Wed, Jun 03, 2009 at 10:59:27AM +0300, Mike Rapoport wrote:
> 
> This driver supports Synaptics I2C touchpad controller on eXeda
> mobile device.
> 
>

Looks much better, one concern still though:
 
> +
> +/* Work Handler */
> +static void synaptics_i2c_work_handler(struct work_struct *work)
> +{
> +	int data = 1;
> +	struct synaptics_i2c *touch =
> +			container_of(work, struct synaptics_i2c, dwork.work);
> +	unsigned long delay;
> +
> +	synaptics_i2c_check_params(touch);
> +
> +	do {
> +		data = synaptics_i2c_get_input(touch);
> +		delay = synaptics_i2c_fix_delay(touch, data);
> +	} while (data);
> +

This will spin in the work handler for the duration of the touch
hogging keventd on this CPU and delaying all other scheduled works. I
don't think we can do that.

Please try the patchg below and if it still works I will fold it all
together and queue for upstream. Thanks!

-- 
Dmitry

Input: synaptics_i2c fixups

From: Dmitry Torokhov <dmitry.torokhov@gmail.com>

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

 drivers/input/mouse/synaptics_i2c.c |   94 ++++++++++++++++++++---------------
 1 files changed, 53 insertions(+), 41 deletions(-)


diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
index 32a5d87..eac9fdd 100644
--- a/drivers/input/mouse/synaptics_i2c.c
+++ b/drivers/input/mouse/synaptics_i2c.c
@@ -241,10 +241,10 @@ static s32 synaptics_i2c_reg_get(struct i2c_client *client, u16 reg)
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
-	if (ret)
-		return ret;
+	if (ret == 0)
+		ret = i2c_smbus_read_byte_data(client, reg & 0xff);
 
-	return i2c_smbus_read_byte_data(client, reg & 0xff);
+	return ret;
 }
 
 static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8 val)
@@ -252,10 +252,10 @@ static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8 val)
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
-	if (ret)
-		return ret;
+	if (ret == 0)
+		ret = i2c_smbus_write_byte_data(client, reg & 0xff, val);
 
-	return i2c_smbus_write_byte_data(client, reg & 0xff, val);
+	return ret;
 }
 
 static s32 synaptics_i2c_word_get(struct i2c_client *client, u16 reg)
@@ -263,10 +263,10 @@ static s32 synaptics_i2c_word_get(struct i2c_client *client, u16 reg)
 	int ret;
 
 	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
-	if (ret)
-		return ret;
+	if (ret == 0)
+		ret = i2c_smbus_read_word_data(client, reg & 0xff);
 
-	return i2c_smbus_read_word_data(client, reg & 0xff);
+	return ret;
 }
 
 static int synaptics_i2c_config(struct i2c_client *client)
@@ -287,11 +287,11 @@ static int synaptics_i2c_config(struct i2c_client *client)
 
 	control = synaptics_i2c_reg_get(client, GENERAL_2D_CONTROL_REG);
 	/* No Deceleration */
-	control |= (no_decel) ? 1 << NO_DECELERATION : 0;
+	control |= no_decel ? 1 << NO_DECELERATION : 0;
 	/* Reduced Reporting */
-	control |= (reduce_report) ? 1 << REDUCE_REPORTING : 0;
+	control |= reduce_report ? 1 << REDUCE_REPORTING : 0;
 	/* No Filter */
-	control |= (no_filter) ? 1 << NO_FILTER : 0;
+	control |= no_filter ? 1 << NO_FILTER : 0;
 	ret = synaptics_i2c_reg_set(client, GENERAL_2D_CONTROL_REG, control);
 	if (ret)
 		return ret;
@@ -302,6 +302,7 @@ static int synaptics_i2c_config(struct i2c_client *client)
 static int synaptics_i2c_reset_config(struct i2c_client *client)
 {
 	int ret;
+
 	/* Reset the Touchpad */
 	ret = synaptics_i2c_reg_set(client, DEV_COMMAND_REG, RESET_COMMAND);
 	if (ret) {
@@ -329,10 +330,11 @@ static int synaptics_i2c_check_error(struct i2c_client *client)
 	return ret;
 }
 
-static int synaptics_i2c_get_input(struct synaptics_i2c *touch)
+static bool synaptics_i2c_get_input(struct synaptics_i2c *touch)
 {
 	struct input_dev *input = touch->input;
 	int xy_delta, gesture;
+	s32 data;
 	s8 x_delta, y_delta;
 
 	/* Deal with spontanious resets and errors */
@@ -340,8 +342,8 @@ static int synaptics_i2c_get_input(struct synaptics_i2c *touch)
 		return 0;
 
 	/* Get Gesture Bit */
-	gesture = (synaptics_i2c_reg_get(touch->client, DATA_REG0)
-							>> GESTURE) & 0x1;
+	data = synaptics_i2c_reg_get(touch->client, DATA_REG0);
+	gesture = (data >> GESTURE) & 0x1;
 
 	/*
 	 * Get Relative axes. we have to get them in one shot,
@@ -361,7 +363,7 @@ static int synaptics_i2c_get_input(struct synaptics_i2c *touch)
 	input_report_rel(input, REL_Y, -y_delta);
 	input_sync(input);
 
-	return (xy_delta || gesture) ? 1 : 0;
+	return xy_delta || gesture;
 }
 
 static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
@@ -369,8 +371,9 @@ static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
 	struct synaptics_i2c *touch = dev_id;
 
 	/*
-	 * This cancels the work scheduled in work handler.
-	 * See explanation on this in work handler func.
+	 * We want to have the work run immediately but it might have
+	 * already been scheduled with a delay, that's why we have to
+	 * cancel it first.
 	 */
 	cancel_delayed_work(&touch->dwork);
 	schedule_delayed_work(&touch->dwork, 0);
@@ -380,34 +383,39 @@ static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
 
 static void synaptics_i2c_check_params(struct synaptics_i2c *touch)
 {
-	int reset = 0;
+	bool reset = false;
+
 	if (scan_rate != touch->scan_rate_param)
 		set_scan_rate(touch, scan_rate);
 
 	if (no_decel != touch->no_decel_param) {
 		touch->no_decel_param = no_decel;
-		reset = 1;
+		reset = true;
 	}
+
 	if (no_filter != touch->no_filter_param) {
 		touch->no_filter_param = no_filter;
-		reset = 1;
+		reset = true;
 	}
+
 	if (reduce_report != touch->reduce_report_param) {
 		touch->reduce_report_param = reduce_report;
-		reset = 1;
+		reset = true;
 	}
+
 	if (reset)
 		synaptics_i2c_reset_config(touch->client);
 }
 
 /* Control the Device polling rate / Work Handler sleep time */
-unsigned long synaptics_i2c_fix_delay(struct synaptics_i2c *touch, int data)
+unsigned long synaptics_i2c_adjust_delay(struct synaptics_i2c *touch,
+					 bool have_data)
 {
-	int delay, nodata_count_thres;
+	unsigned long delay, nodata_count_thres;
 
 	if (polling_req) {
 		delay = touch->scan_ms;
-		if (data) {
+		if (have_data) {
 			touch->no_data_count = 0;
 		} else {
 			nodata_count_thres = NO_DATA_THRES / touch->scan_ms;
@@ -416,26 +424,25 @@ unsigned long synaptics_i2c_fix_delay(struct synaptics_i2c *touch, int data)
 			else
 				delay = NO_DATA_SLEEP_MSECS;
 		}
-	} else
-		delay = THREAD_IRQ_SLEEP_MSECS;
-
-	return msecs_to_jiffies(delay);
+		return msecs_to_jiffies(delay);
+	} else {
+		delay = msecs_to_jiffies(THREAD_IRQ_SLEEP_MSECS);
+		return round_jiffies_relative(delay);
+	}
 }
 
 /* Work Handler */
 static void synaptics_i2c_work_handler(struct work_struct *work)
 {
-	int data = 1;
+	bool have_data;
 	struct synaptics_i2c *touch =
 			container_of(work, struct synaptics_i2c, dwork.work);
 	unsigned long delay;
 
 	synaptics_i2c_check_params(touch);
 
-	do {
-		data = synaptics_i2c_get_input(touch);
-		delay = synaptics_i2c_fix_delay(touch, data);
-	} while (data);
+	have_data = synaptics_i2c_get_input(touch);
+	delay = synaptics_i2c_adjust_delay(touch, have_data);
 
 	/*
 	 * While interrupt driven, there is no real need to poll the device.
@@ -450,8 +457,8 @@ static void synaptics_i2c_work_handler(struct work_struct *work)
 
 static int synaptics_i2c_open(struct input_dev *input)
 {
-	int ret = 0;
 	struct synaptics_i2c *touch = input_get_drvdata(input);
+	int ret;
 
 	ret = synaptics_i2c_reset_config(touch->client);
 	if (ret)
@@ -461,7 +468,7 @@ static int synaptics_i2c_open(struct input_dev *input)
 		schedule_delayed_work(&touch->dwork,
 				       msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
 
-	return ret;
+	return 0;
 }
 
 static void synaptics_i2c_close(struct input_dev *input)
@@ -492,13 +499,13 @@ static void synaptics_i2c_set_input_params(struct synaptics_i2c *touch)
 	input_set_drvdata(input, touch);
 
 	/* Register the device as mouse */
-	set_bit(EV_REL, input->evbit);
-	set_bit(REL_X, input->relbit);
-	set_bit(REL_Y, input->relbit);
+	__set_bit(EV_REL, input->evbit);
+	__set_bit(REL_X, input->relbit);
+	__set_bit(REL_Y, input->relbit);
 
 	/* Register device's buttons and keys */
-	set_bit(EV_KEY, input->evbit);
-	set_bit(BTN_LEFT, input->keybit);
+	__set_bit(EV_KEY, input->evbit);
+	__set_bit(BTN_LEFT, input->keybit);
 }
 
 struct synaptics_i2c *synaptics_i2c_touch_create(struct i2c_client *client)
@@ -598,6 +605,7 @@ static int __devexit synaptics_i2c_remove(struct i2c_client *client)
 	return 0;
 }
 
+#ifdef CONFIG_PM
 static int synaptics_i2c_suspend(struct i2c_client *client, pm_message_t mesg)
 {
 	struct synaptics_i2c *touch = i2c_get_clientdata(client);
@@ -624,6 +632,10 @@ static int synaptics_i2c_resume(struct i2c_client *client)
 
 	return 0;
 }
+#else
+#define synaptics_i2c_suspend	NULL
+#define synaptics_i2c_resume	NULL
+#endif
 
 static const struct i2c_device_id synaptics_i2c_id_table[] = {
 	{ "synaptics_i2c", 0 },