Hi Mike,

On Wednesday 20 May 2009 05:36:17 Mike Rapoport wrote:

We have test_bit() if you don't want to open-code this.

+
+/* Function Masks */
+/* Device Control Register Masks (DEV_CONTROL_REG) */
+#define REPORT_RATE_MSK		0xc0
+#define SLEEP_MODE_MSK		0x07
+
+/* Device Sleep Modes */
+#define FULL_AWAKE		0x0
+#define NORMAL_OP		0x1
+#define LOW_PWR_OP		0x2
+#define VERY_LOW_PWR_OP		0x3
+#define SENS_SLEEP		0x4
+#define SLEEP_MOD		0x5
+#define DEEP_SLEEP		0x6
+#define HIBERNATE		0x7
+
+/* Interrupt Register Mask */
+/* (INT_REQ_STAT_REG | DEVICE_STATUS_REG | INTERRUPT_EN_REG) */
+#define INT_ENA_REQ_MSK		0x07
+#define INT_ENA_ABS_MSK		0x01
+#define INT_ENA_REL_MSK		0x02
+#define INT_ENA_F20_MSK		0x04
+
+/* Device Status Register Masks (DEVICE_STATUS_REG) */
+#define CONFIGURED_MSK		0x40
+#define ERROR_MSK		0x80
+
+/* Data Register 0 Masks */
+#define FINGER_WIDTH_MSK	0xf0
+#define GESTURE_MSK		0x08
+#define SENSOR_STATUS_MSK	0x07
+
+/*
+ * MSB Position Register Masks
+ * ABS_MSB_X_REG | ABS_MSB_Y_REG | SENS_MAX_POS_MSB_REG |
+ * DEV_QUERY_REG3 | DEV_QUERY_REG5
+ */
+#define MSB_POSITION_MSK	0x1f
+
+/* Device Query Registers Masks */
+
+/* DEV_QUERY_REG2 */
+#define NUM_EXTRA_POS_MSK	0x07
+
+/* When in IRQ mode read the device every THREAD_IRQ_SLEEP_SECS */
+#define THREAD_IRQ_SLEEP_SECS	2
+#define THREAD_IRQ_SLEEP_MSECS	(THREAD_IRQ_SLEEP_SECS * MSEC_PER_SEC)
+
+/*
+ * When in Polling mode and no data received for NO_DATA_THRES msecs
+ * reduce the polling rate to NO_DATA_SLEEP_MSECS
+ */
+#define NO_DATA_THRES		(MSEC_PER_SEC)
+#define NO_DATA_SLEEP_MSECS	(MSEC_PER_SEC / 4)
+
+/*
+ * The Acceleration Factor:
+ * from 0.5 (1) to 4 (8) in dozes of 0.5
+ * the transformation is done in order to get 0.5 resolution.
+ * values above 8 will affect poor performance without proper smoothing.
+ */
+#define ACCEL_DIVIDER 10
+#define ACCEL_DOZE 2
+static int accel = 4;
+module_param(accel, int, 0644);
+MODULE_PARM_DESC(accel, "Set Acceleration parameter 1..8. Default = 4");
+
+/* Control touchpad's No Deceleration option */
+static int no_decel = 1;
+module_param(no_decel, bool, 0644);
+MODULE_PARM_DESC(no_decel, "No Deceleration. Default = 1 (on)");
+

The same objection as before - does not belong in the kernel.

+/* Control touchpad's Reduced Reporting option */
+static int reduce_report;
+module_param(reduce_report, bool, 0644);
+MODULE_PARM_DESC(reduce_report, "Reduced Reporting. Default = 0 (off)");
+
+/* Control touchpad's No Filter option */
+static int no_filter;
+module_param(no_filter, bool, 0644);
+MODULE_PARM_DESC(no_filter, "No Filter. Default = 0 (off)");
+
+/*
+ * touchpad Attention line is Active Low and Open Drain,
+ * therefore should be connected to pulled up line
+ * and the irq configuration should be set to Falling Edge Trigger
+ */
+/* Control IRQ / Polling option */
+static int polling_req;
+module_param(polling_req, bool, 0444);
+MODULE_PARM_DESC(polling_req, "Request Polling. Default = 0 (use irq)");
+
+/* Control Polling Rate */
+static int scan_rate = 80;
+module_param(scan_rate, int, 0644);
+MODULE_PARM_DESC(scan_rate, "Polling rate in times/sec. Default = 80");
+
+/* The main device structure */
+struct synaptics_i2c {
+	struct i2c_client	*client;
+	struct input_dev	*input;
+	struct mutex		mutex;
+	struct delayed_work	dwork;
+	int			no_data_count;
+	int			no_decel_param;
+	int			reduce_report_param;
+	int			no_filter_param;
+	int			scan_rate_param;
+	int			accel_param;
+	int			accel;	/* Transformed acceleration */
+	int			scan_ms;
+};
+
+static inline void set_acceleration(struct synaptics_i2c *touch, int
accel) +{
+	int acc = (accel < 1) ? 1 : ((accel > 8) ? 8 : accel);
+	touch->accel = (acc * ACCEL_DIVIDER) / ACCEL_DOZE;
+	touch->accel_param = accel;
+}
+
+static inline void set_scan_rate(struct synaptics_i2c *touch, int
scan_rate) +{
+	touch->scan_ms = MSEC_PER_SEC / scan_rate;
+	touch->scan_rate_param = scan_rate;
+}
+
+static s32 synaptics_i2c_reg_get(struct i2c_client *client, u16 reg)
+{
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&touch->mutex);

Why do you need to take this mutex? I don't see you doing simultaneous
access to the device, but if you do then I'd think you need to protect
larger blocks, like the read-modify-write chunks, not individual read/
write operations.

+
+	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
+	if (!ret)
+		ret = i2c_smbus_read_byte_data(client, reg & 0xff);
+
+	mutex_unlock(&touch->mutex);
+
+	return ret;
+}
+
+static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8
val) +{
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&touch->mutex);
+
+	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
+	if (!ret)
+		ret = i2c_smbus_write_byte_data(client, reg & 0xff, val);
+
+	mutex_unlock(&touch->mutex);
+
+	return ret;
+}
+
+static s32 synaptics_i2c_word_get(struct i2c_client *client, u16 reg)
+{
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+	int ret;
+
+	mutex_lock(&touch->mutex);
+
+	ret = i2c_smbus_write_byte_data(client, PAGE_SEL_REG, reg >> 8);
+	if (!ret)
+		ret = i2c_smbus_read_word_data(client, reg & 0xff);
+
+	mutex_unlock(&touch->mutex);
+
+	return ret;
+}
+
+static int synaptics_i2c_config(struct i2c_client *client)
+{
+	int ret, control;
+	u8 int_en;
+
+	/* set Report Rate to Device Highest (>=80) and Sleep to normal */
+	ret = synaptics_i2c_reg_set(client, DEV_CONTROL_REG, 0xc1);
+	if (ret)
+		return ret;
+
+	/* set Interrupt Disable to Func20 / Enable to Func10) */
+	int_en = (polling_req) ? 0 : INT_ENA_ABS_MSK | INT_ENA_REL_MSK;
+	ret = synaptics_i2c_reg_set(client, INTERRUPT_EN_REG, int_en);
+	if (ret)
+		return ret;
+
+	control = synaptics_i2c_reg_get(client, GENERAL_2D_CONTROL_REG);
+	/* No Deceleration */
+	control |= (no_decel) ? 1 << NO_DECELERATION : 0;
+	/* Reduced Reporting */
+	control |= (reduce_report) ? 1 << REDUCE_REPORTING : 0;
+	/* No Filter */
+	control |= (no_filter) ? 1 << NO_FILTER : 0;
+	ret = synaptics_i2c_reg_set(client, GENERAL_2D_CONTROL_REG, control);
+	if (ret)
+		return ret;
+
+	return 0;
+}
+
+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) {
+		dev_err(&client->dev, "Unable to reset device\n");
+	} else {
+		msleep(SOFT_RESET_DELAY_MS);
+		ret = synaptics_i2c_config(client);
+		if (ret)
+			dev_err(&client->dev, "Unable to config device\n");
+	}
+
+	return ret;
+}
+
+static int synaptics_i2c_check_error(struct i2c_client *client)
+{
+	int status, ret = 0;
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+
+	mutex_lock(&touch->mutex);
+
+	status = i2c_smbus_read_byte_data(client, DEVICE_STATUS_REG) &
+		(CONFIGURED_MSK | ERROR_MSK);
+
+	mutex_unlock(&touch->mutex);
+
+	if (status != CONFIGURED_MSK)
+		ret = synaptics_i2c_reset_config(client);
+
+	return ret;
+}
+
+static int synaptics_i2c_get_input(struct synaptics_i2c *touch)
+{
+	struct input_dev *input = touch->input;
+	int xy_delta, gesture;
+	s8 x_delta, y_delta;
+
+	/* Deal with spontanious resets and errors */
+	if (synaptics_i2c_check_error(touch->client))
+		return 0;
+
+	/* Get Gesture Bit */
+	gesture = GET_BIT(GESTURE,
+			  synaptics_i2c_reg_get(touch->client, DATA_REG0));
+
+	/*
+	 * Get Relative axes. we have to get them in one shot,
+	 * so we get 2 bytes starting from REL_X_REG.
+	 */
+	xy_delta = synaptics_i2c_word_get(touch->client, REL_X_REG) & 0xffff;
+
+	/* Separate X from Y */
+	x_delta = xy_delta & 0xff;
+	y_delta = (xy_delta >> REGISTER_LENGTH) & 0xff;
+
+	/* Report the button event */
+	input_report_key(input, BTN_LEFT, gesture);
+
+	/* Report the deltas multiplied by acceleration factor */
+	input_report_rel(input, REL_X,
+			 (touch->accel * (s16) x_delta) / ACCEL_DIVIDER);
+	input_report_rel(input, REL_Y,
+			 -((touch->accel * (s16) y_delta) / ACCEL_DIVIDER));
+	input_sync(input);
+
+	return (xy_delta || gesture) ? 1 : 0;
+}
+
+static irqreturn_t synaptics_i2c_irq(int irq, void *dev_id)
+{
+	struct synaptics_i2c *touch = dev_id;
+
+	cancel_delayed_work(&touch->dwork);

Why do we need to cancel?

+	schedule_work(&touch->dwork.work);

Do not poke inside delayed work structure, just do

	schedule_delayed_work(&touch->dwork, 0);

to schedule it immediately.

+
+	return IRQ_HANDLED;
+}
+
+static void synaptics_i2c_check_params(struct synaptics_i2c *touch)
+{
+	int reset = 0;
+	if (accel != touch->accel_param)
+		set_acceleration(touch, accel);
+
+	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;
+	}
+	if (no_filter != touch->no_filter_param) {
+		touch->no_filter_param = no_filter;
+		reset = 1;
+	}
+	if (reduce_report != touch->reduce_report_param) {
+		touch->reduce_report_param = reduce_report;
+		reset = 1;
+	}
+	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) +{
+	int delay, nodata_count_thres;
+
+	if (polling_req) {
+		delay = touch->scan_ms;
+		if (data) {
+			touch->no_data_count = 0;
+		} else {
+			nodata_count_thres = NO_DATA_THRES / touch->scan_ms;
+			if (touch->no_data_count < nodata_count_thres)
+				touch->no_data_count++;
+			else
+				delay = NO_DATA_SLEEP_MSECS;
+		}
+	} else
+		delay = THREAD_IRQ_SLEEP_MSECS;
+
+	return msecs_to_jiffies(delay);
+}
+
+/* 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);
+
+	schedule_delayed_work(&touch->dwork, delay);

Do we need to re-schedule the work even if we are in interrupt-driven
mode?

+}
+
+static int synaptics_i2c_open(struct input_dev *input)
+{
+	int ret = 0;
+	struct synaptics_i2c *touch = input_get_drvdata(input);
+
+	ret = synaptics_i2c_reset_config(touch->client);
+	if (ret)
+		return ret;
+
+	if (polling_req)
+		schedule_delayed_work(&touch->dwork,
+				       msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
+	else
+		enable_irq(touch->client->irq);
+
+	return ret;
+}
+
+static void synaptics_i2c_close(struct input_dev *input)
+{
+	struct synaptics_i2c *touch = input_get_drvdata(input);
+
+	if (!polling_req)
+		disable_irq(touch->client->irq);

Just disabling IRQ is pretty heavy-handed. Can't it ever be shared with
another device?

+
+	cancel_delayed_work_sync(&touch->dwork);
+
+	/* Save some power */
+	synaptics_i2c_reg_set(touch->client, DEV_CONTROL_REG, DEEP_SLEEP);
+}
+
+static void synaptics_i2c_set_input_params(struct synaptics_i2c *touch)
+{
+	struct input_dev *input = touch->input;
+
+	input->name = touch->client->name;
+	input->phys = touch->client->adapter->name;
+	input->id.bustype = BUS_I2C;
+	input->id.version = synaptics_i2c_word_get(touch->client,
+						   INFO_QUERY_REG0);
+	input->dev.parent = &touch->client->dev;
+	input->open = synaptics_i2c_open;
+	input->close = synaptics_i2c_close;
+	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);
+
+	/* Register device's buttons and keys */
+	set_bit(EV_KEY, input->evbit);
+	set_bit(BTN_LEFT, input->keybit);
+}
+
+struct synaptics_i2c *synaptics_i2c_touch_create(struct i2c_client
*client) +{
+	struct synaptics_i2c *touch;
+
+	touch = kzalloc(sizeof(struct synaptics_i2c), GFP_KERNEL);
+	if (!touch)
+		return NULL;
+
+	touch->client = client;
+	touch->no_decel_param = no_decel;
+	touch->scan_rate_param = scan_rate;
+	set_scan_rate(touch, scan_rate);
+	touch->accel_param = accel;
+	set_acceleration(touch, accel);
+	INIT_DELAYED_WORK(&touch->dwork, synaptics_i2c_work_handler);
+	mutex_init(&touch->mutex);
+
+	return touch;
+}
+
+static int __devinit synaptics_i2c_probe(struct i2c_client *client,
+			       const struct i2c_device_id *dev_id)
+{
+	int ret;
+	struct synaptics_i2c *touch;
+
+	touch = synaptics_i2c_touch_create(client);
+	if (!touch)
+		return -ENOMEM;
+
+	i2c_set_clientdata(client, touch);
+
+	ret = synaptics_i2c_reset_config(client);
+	if (ret)
+		goto err_mem_free;
+
+	if (client->irq < 1)
+		polling_req = 1;
+
+	touch->input = input_allocate_device();
+	if (!touch->input) {
+		ret = -ENOMEM;
+		goto err_mem_free;
+	}
+
+	synaptics_i2c_set_input_params(touch);
+
+	if (!polling_req) {
+		dev_dbg(&touch->client->dev,
+			 "Requesting IRQ: %d\n", touch->client->irq);
+
+		ret = request_irq(touch->client->irq, synaptics_i2c_irq,
+				  IRQF_DISABLED|IRQ_TYPE_EDGE_FALLING,
+				  DRIVER_NAME, touch);
+		if (ret) {
+			dev_warn(&touch->client->dev,
+				  "IRQ request failed: %d, "
+				  "falling back to polling\n", ret);
+			polling_req = 1;
+		} else
+			disable_irq(touch->client->irq);
+	}
+
+	if (polling_req)
+		dev_dbg(&touch->client->dev,
+			 "Using polling at rate: %d times/sec\n", scan_rate);
+
+	/* Register the device in input subsystem */
+	ret = input_register_device(touch->input);
+	if (ret) {
+		dev_err(&client->dev,
+			 "Input device register failed: %d\n", ret);
+		goto err_input_free;
+	}
+	return 0;
+
+err_input_free:
+	input_free_device(touch->input);
+err_mem_free:
+	i2c_set_clientdata(client, NULL);
+	kfree(touch);
+
+	return ret;
+}
+
+static int __devexit synaptics_i2c_remove(struct i2c_client *client)
+{
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+
+	if (!polling_req)
+		free_irq(touch->client->irq, touch);
+
+	input_unregister_device(touch->input);
+	i2c_set_clientdata(client, NULL);
+	kfree(touch);
+
+	return 0;
+}
+
+static int synaptics_i2c_suspend(struct i2c_client *client, pm_message_t
mesg) +{
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+
+	cancel_delayed_work_sync(&touch->dwork);
+
+	/* Save some power */
+	synaptics_i2c_reg_set(touch->client, DEV_CONTROL_REG, DEEP_SLEEP);
+
+	return 0;
+}
+
+static int synaptics_i2c_resume(struct i2c_client *client)
+{
+	int ret;
+	struct synaptics_i2c *touch = i2c_get_clientdata(client);
+
+	ret = synaptics_i2c_reset_config(client);
+	if (ret)
+		return ret;
+
+	schedule_delayed_work(&touch->dwork,
+			       msecs_to_jiffies(NO_DATA_SLEEP_MSECS));
+
+	return 0;
+}
+
+static const struct i2c_device_id synaptics_i2c_id_table[] = {
+	{ "synaptics_i2c", 0 },
+	{ },
+};
+MODULE_DEVICE_TABLE(i2c, synaptics_i2c_id_table);
+
+static struct i2c_driver synaptics_i2c_driver = {
+	.driver = {
+		.name	= DRIVER_NAME,
+		.owner	= THIS_MODULE,
+	},
+
+	.probe		= synaptics_i2c_probe,
+	.remove		= __devexit_p(synaptics_i2c_remove),
+
+	.suspend	= synaptics_i2c_suspend,
+	.resume		= synaptics_i2c_resume,
+	.id_table	= synaptics_i2c_id_table,
+};
+
+static int __init synaptics_i2c_init(void)
+{
+	return i2c_add_driver(&synaptics_i2c_driver);
+}
+
+static void __exit synaptics_i2c_exit(void)
+{
+	i2c_del_driver(&synaptics_i2c_driver);
+}
+
+module_init(synaptics_i2c_init);
+module_exit(synaptics_i2c_exit);
+
+MODULE_DESCRIPTION("Synaptics I2C touchpad driver");
+MODULE_AUTHOR("Mike Rapoport, Igor Grinberg, Compulab");
+MODULE_LICENSE("GPL");
+

Thanks.

-- 
Dmitry