Re: [PATCH v3 1/2] Input: goodix - Add support for more then one touch-key

[Dmitry Mastykin <dmastykin@astralinux.ru>]

Hi Dmitry,
we had discussed earlier this point with Hans:

Hans:
 >>> 3) In goodix_ts_report_key you do:
 >>>
 >>>                   for (i = 0; i < GOODIX_MAX_KEYS; ++i)
 >>>                           if (key_value & (1 << i))
 >>>                                   input_report_key(ts->input_dev, 
ts->keymap[i], 1
 >>>
 >>> But if the user then jumps his finger from say touch_key 0 to 
touch_key 1
 >>> without us receiving a "packet" in between with GOODIX_HAVE_KEY set,
 >>> then we never release touch_key 0. So instead this really should be:
 >>>
 >>>                   for (i = 0; i < GOODIX_MAX_KEYS; ++i)
 >>>                           input_report_key(ts->input_dev, 
ts->keymap[i],
 >>>                                            key_value & (1 << i));
 >>>

Me:
 >> It seems, that this problem never happens. When user moves finger from
 >> button to button, we stably receive 2-3 packets without
 >> GOODIX_HAVE_KEY in between, that release all previous touches. From
 >> other hand, your change will not work when the same keycode is
 >> assigned to several buttons - it will be immediately released.

Hans:
 >
 > Hmm, interesting point I did not think of that. That would be a bit
 > weird thing to do, but it is not impossible...
 >
 > I'm afraid Dmitry (the input maintainer) will likely make the same
 > remark as I do though (when you submit this upstream). But we'll see.
 >
 > Keeping this as is is fine with me.

So I'm impressed about your mutual understanding) And waiting for your 
decision.
Thank you!

Kind regards
Dmitry Mastykin

On 3/24/20 9:51 PM, Dmitry Torokhov wrote:
> Hi Dmitry,
> 
> On Mon, Mar 16, 2020 at 10:53:03AM +0300, Dmitry Mastykin wrote:
>> +static void goodix_ts_report_key(struct goodix_ts_data *ts, u8 *data)
>> +{
>> +	int touch_num;
>> +	u8 key_value;
>> +	int i;
>> +
>> +	if (data[0] & GOODIX_HAVE_KEY) {
>> +		touch_num = data[0] & 0x0f;
>> +		key_value = data[1 + ts->contact_size * touch_num];
>> +		for (i = 0; i < GOODIX_MAX_KEYS; ++i)
>> +			if (key_value & (1 << i))
>> +				input_report_key(ts->input_dev, ts->keymap[i], 1);
>> +	} else
>> +		for (i = 0; i < GOODIX_MAX_KEYS; ++i)
>> +			input_report_key(ts->input_dev, ts->keymap[i], 0);
> 
> Should this be written as:
> 
> 	if (data[0] & GOODIX_HAVE_KEY) {
> 		touch_num = data[0] & 0x0f;
> 		key_value = data[1 + ts->contact_size * touch_num];
> 	} else {
> 		/* Release all keys */
> 		key_value = 0;
> 	}
> 
> 	for (i = 0; i < GOODIX_MAX_KEYS; i++)
> 		input_report_key(ts->input_dev, ts->keymap[i],
> 				 key_value & BIT(i);
> 
> or the device may send incremental updates to the keys pressed without
> resending currently pressed keys (sounds unlikely)?
> 
> Thanks.
>