Re: [PATCH v2] input: qt602240 - Add ATMEL QT602240 touchscreen driver

[Joonyoung Shim <jy0922.shim@samsung.com>]

On 6/25/2010 11:08 PM, Henrik Rydberg wrote:
> Hi Joonyoung,
> 
> some follow-up comments on the MT events.
> 
>> +static void qt602240_input_report(struct qt602240_data *data, int single_id)
>> +{
>> +	struct qt602240_finger *finger = data->finger;
>> +	struct input_dev *input_dev = data->input_dev;
>> +	int finger_num = 0;
>> +	int id;
>> +
>> +	for (id = 0; id < QT602240_MAX_FINGER; id++) {
>> +		if (!finger[id].status)
>> +			continue;
>> +
>> +		input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
>> +				finger[id].area);
>> +
>> +		if (finger[id].status == QT602240_RELEASE)
>> +			finger[id].status = 0;
>> +		else {
>> +			input_report_abs(input_dev, ABS_MT_POSITION_X,
>> +					finger[id].x);
>> +			input_report_abs(input_dev, ABS_MT_POSITION_Y,
>> +					finger[id].y);
>> +			finger_num++;
>> +		}
>> +
>> +		input_mt_sync(input_dev);
>> +	}
>> +
>> +	input_report_key(input_dev, BTN_TOUCH, finger_num > 0);
>> +
>> +	if (finger[single_id].status != QT602240_RELEASE) {
>> +		input_report_abs(input_dev, ABS_X, finger[single_id].x);
>> +		input_report_abs(input_dev, ABS_Y, finger[single_id].y);
>> +	}
>> +
>> +	input_sync(input_dev);
>> +}
> 
> The problem still persists here. I will try to explain in code form instead:
> 
> for (id = 0; id < QT602240_MAX_FINGER; id++) {
> 	if (!finger[id].status)
> 		continue;
> 
> 	input_report_abs(input_dev, ABS_MT_TOUCH_MAJOR,
> 		finger[id].status != QT602240_RELEASE ? finger[id].area : 0);
> 	input_report_abs(input_dev, ABS_MT_POSITION_X,
> 			finger[id].x);
> 	input_report_abs(input_dev, ABS_MT_POSITION_Y,
> 			finger[id].y);

Hmm, is it OK to report ABS_MT_POSITION_X/Y when releases?

> 	input_mt_sync(input_dev);
> 
> 	if (finger[id].status == QT602240_RELEASE)
> 		finger[id].status = 0;
> 	else
> 		finger_num++;
> }
> 
> Regarding the single_id, I can understand the need for it, but the logic for a
> single touch is slightly confusing. If single_id is to be interpreted as the
> contact currently being tracked as the single pointer, and that single_id
> changes as the number of fingers on the pad is reduced, until there is only one
> left, then it would still be clearer logically to do something like this:
> 
> if (finger_num > 0) {
> 	input_report_key(input_dev, BTN_TOUCH, 1);
> 	input_report_abs(input_dev, ABS_X, finger[single_id].x);
> 	input_report_abs(input_dev, ABS_Y, finger[single_id].y);
> } else {
> 	input_report_key(input_dev, BTN_TOUCH, 0);
> }
> input_sync(input_dev);
> 
> Would it not?
> 

There is a case which fingers more than one are touched and current
status of single_id finger is release, then this will report ABS_X/Y 
events even if it is the release event.

How about codes like this? :

+       int status = finger[single_id].status;

[snip]

+       if (finger_num > 0) {
+               if (status != QT602240_RELEASE) {
+                       input_report_key(input_dev, BTN_TOUCH, 1);
+                       input_report_abs(input_dev, ABS_X, finger[single_id].x);
+                       input_report_abs(input_dev, ABS_Y, finger[single_id].y);
+               }
+       } else
+               input_report_key(input_dev, BTN_TOUCH, 0);