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

Hi Dmitry,

Thanks for your review.

2011/8/1 Dmitry Torokhov [off-list ref]:

Hi Zhang,

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

quoted

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

This report single mode was wroted in Chip's I2C programming manual.
but in fact, at least in my touch panel, the event type(buf[0]) was
REPORT_MODE_MTTOUCH regardless one finger or two finger pressed on
panel.

I add this code is in case someone's panel is configured with
REPORT_MODE_SINGLE.

It was no tested, maybe I should remove this part of code ? what's
your opinion ?

quoted

+     }
+
+     /* 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.

will do.

quoted

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

I will adjust it to MT-B protocol in next version.

quoted

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

ok.

quoted

+
+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().

ok

Thanks.

--
Dmitry

Thanks,
Jiejing
--
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