Re: [PATCH] input: add EETI eGalax I2C capacitive multi touch driver.

Hi Zhang,

On Tue, Jul 26, 2011 at 05:25:54PM +0800, Zhang Jiejing wrote:

+
+	if (buf[0] == REPORT_MODE_SINGLE) {
+		input_report_abs(input_dev, ABS_X, x);
+		input_report_abs(input_dev, ABS_Y, y);
+		input_report_key(input_dev, BTN_TOUCH, !!state);
+		input_sync(input_dev);
+		return IRQ_HANDLED;

In what cases does the device signal REPORT_MODE_SINGLE? When only one
finger touches the surface? Because we should still be using MT protocol
even if at the moment only one finger touches the surface. And it is
nice to provide non-MT compatibility data for legacy applications
regardless of the number of contacts currently active (see
drivers/input/input-mt.c for helpers).

+	}
+
+	/* deal with multiple touch  */
+	valid = state & EVENT_VAILD_MASK;
+	id = (state & EVENT_ID_MASK) >> EVENT_ID_OFFSET;
+	down = state & EVENT_DOWN_UP;
+
+	if (!valid || id > MAX_SUPPORT_POINTS) {
+		dev_dbg(&client->dev, "point invalid\n");
+		return IRQ_HANDLED;
+	}
+
+	if (down) {
+		/* since egalax only report one of multi touch event,
+		 * so we need record pervious event with different id
+		 * and report them.*/
+		events[id].valid = valid;
+		events[id].status = down;
+		events[id].x = x;
+		events[id].y = y;
+
+		for (i = 0; i < MAX_SUPPORT_POINTS; i++) {
+			if (!events[i].valid)
+				continue;
+			dev_dbg(&client->dev, "report id:%d valid:%d x:%d y:%d",
+				i, valid, x, y);
+
+			input_report_abs(input_dev,
+					 ABS_MT_TRACKING_ID, i);
+			input_report_abs(input_dev,
+					 ABS_MT_TOUCH_MAJOR, 1);

This does not provide any information, please drop.

+			input_report_abs(input_dev,
+					 ABS_MT_POSITION_X, events[i].x);
+			input_report_abs(input_dev,
+					 ABS_MT_POSITION_Y, events[i].y);
+			input_mt_sync(input_dev);

It would be much better to use MT-B protocol since the device seems to
provide proper tracking IDs for the contacts.

+
+	__set_bit(EV_ABS, input_dev->evbit);
+	__set_bit(EV_KEY, input_dev->evbit);
+	__set_bit(BTN_TOUCH, input_dev->keybit);
+	__set_bit(ABS_X, input_dev->absbit);
+	__set_bit(ABS_Y, input_dev->absbit);
+	input_set_abs_params(input_dev, ABS_X, 0, 32767, 0, 0);
+	input_set_abs_params(input_dev, ABS_Y, 0, 32767, 0, 0);

You do not set any ABS_MT_* events here so your MT events will be
suppressed by input core.

+
+static __devexit int egalax_ts_remove(struct i2c_client *client)
+{
+	struct egalax_ts *data = i2c_get_clientdata(client);
+
+	free_irq(client->irq, data);
+	input_unregister_device(data->input_dev);
+	input_free_device(data->input_dev);

No calls to input_free_device() after calling input_unregister_device().

Thanks.

-- 
Dmitry