Hi Joonyoung,
[...]
>> +static void qt602240_input_read(struct qt602240_data *data)
>> +{
>> + struct qt602240_message message;
>> + struct qt602240_object *object;
>> + struct device *dev = &data->client->dev;
>> + struct input_dev *input_dev = data->input_dev;
>> + u8 reportid;
>> + u8 max_reportid;
>> + u8 min_reportid;
>> +
>> +repeat:
>> + if (qt602240_read_message(data, &message)) {
>> + dev_err(dev, "Failed to read message\n");
>> + return;
>> + }
>> +
>> + reportid = message.reportid;
>> +
>> + /* Check it remains the message to process */
>> + if (reportid == 0xff)
>> + return;
>> +
>> + /* whether reportid is thing of QT602240_TOUCH_MULTI */
>> + object = qt602240_get_object(data, QT602240_TOUCH_MULTI);
>> + if (!object)
>> + return;
>> +
>> + max_reportid = object->max_reportid;
>> + min_reportid = max_reportid - object->num_report_ids + 1;
>> +
>> + if ((reportid >= min_reportid) && (reportid <= max_reportid)) {
>> + u8 id;
>> + u8 status;
>> + int x;
>> + int y;
>> + int area;
>> + int finger_num = 0;
>> +
>> + id = reportid - min_reportid;
>> + status = message.message[0];
>> +
>> + /* Check the touch is present on the screen */
>> + if (!(status & QT602240_DETECT))
>> + goto release;
>> +
>> + /* Check only AMP detection */
>> + if (!(status & (QT602240_PRESS | QT602240_MOVE)))
>> + goto repeat;
>> +
>> + x = (message.message[1] << 2) |
>> + ((message.message[3] & ~0x3f) >> 6);
>> + y = (message.message[2] << 2) |
>> + ((message.message[3] & ~0xf3) >> 2);
>> + area = message.message[4];
>> +
>> + dev_dbg(dev, "[%d] %s x: %d, y: %d, area: %d\n", id,
>> + status & QT602240_MOVE ? "moved" : "pressed",
>> + x, y, area);
>> +
>> + data->finger[id].status = status & QT602240_MOVE ?
>> + QT602240_MOVE : QT602240_PRESS;
>> + data->finger[id].x = x;
>> + data->finger[id].y = y;
>> + data->finger[id].area = area;
>> +
>> + input_report_key(input_dev, BTN_TOUCH, 1);
>> + input_report_abs(input_dev, ABS_X, x);
>> + input_report_abs(input_dev, ABS_Y, y);
>> +
>> + goto mt_report;
>> +
>> +release:
>> + if (status & QT602240_RELEASE) {
>> + dev_dbg(dev, "[%d] released\n", id);
>> +
>> + data->finger[id].status = QT602240_RELEASE;
>> + data->finger[id].area = 0;
>> + }
>> +
>> +mt_report:
>> + for (id = 0; id < QT602240_MAX_FINGER; id++) {
>> + if (!data->finger[id].status)
>> + continue;
>> +
>> + input_report_abs(input_dev, ABS_MT_TRACKING_ID, id);
>> + input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>> + data->finger[id].area);
>> +
>> + if (data->finger[id].status == QT602240_RELEASE)
>> + data->finger[id].status = 0;
>> + else {
>> + input_report_abs(input_dev, ABS_MT_POSITION_X,
>> + data->finger[id].x);
>> + input_report_abs(input_dev, ABS_MT_POSITION_Y,
>> + data->finger[id].y);
>> + finger_num++;
>> + }
The finger[id].status is checked twice in this block, is there any particular
reason for it? Either way, reporting only a part of the finger properties before
input_mt_sync() is wrong. Perhaps one can move the second test up together with
the first one?
>> +
>> + input_mt_sync(input_dev);
>> + }
>> +
>> + if (!finger_num)
>> + input_report_key(input_dev, BTN_TOUCH, 0);
The lines above can be combined with the first BTN_TOUCH instance to something
like this:
input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
The input core will not emit the key unless it actually changes.
>> + input_sync(input_dev);
>> + } else {
>> + qt602240_dump_message(dev, &message);
>> + qt602240_check_config_error(data, &message, reportid);
>> + }
>> +
>> + goto repeat;
[...]
>> + __set_bit(EV_ABS, input_dev->evbit);
>> + __set_bit(EV_KEY, input_dev->evbit);
>> + __set_bit(BTN_TOUCH, input_dev->keybit);
>> +
>> + /* For single touch */
>> + input_set_abs_params(input_dev, ABS_X, 0, QT602240_MAX_XC, 0,
>> + 0);
>> + input_set_abs_params(input_dev, ABS_Y, 0, QT602240_MAX_YC, 0,
>> + 0);
>> +
>> + /* For multi touch */
>> + input_set_abs_params(input_dev, ABS_MT_TRACKING_ID, 0,
>> + QT602240_MAX_ID, 0, 0);
What is a normal value for QT602240_MAX_ID? Is it modified every time there is a
new touch?
>> + input_set_abs_params(input_dev, ABS_MT_TOUCH_MAJOR, 0,
>> + QT602240_MAX_AREA, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_POSITION_X, 0,
>> + QT602240_MAX_XC, 0, 0);
>> + input_set_abs_params(input_dev, ABS_MT_POSITION_Y, 0,
>> + QT602240_MAX_YC, 0, 0);
>> +
>> + input_set_drvdata(input_dev, data);
[...]
In general, I think the functions of this driver are too long. Splitting them up
might do good.
Thanks!
Henrik
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
