Hi,
On 13-11-17 09:47, Benjamin Tissoires wrote:
On Nov 07 2017 or thereabouts, Hans de Goede wrote:
This commit fixes 2 different issues with buttons on touchpads in one go:
1) Devices in "single finger hybrid mode" will send one report per finger,
on some devices only the first report of such a multi-packet frame will
contain a value for BTN_LEFT, in subsequent reports (if multiple fingers
are down) the value is always 0, causing hid-mt to report BTN_LEFT going
1 - 0 - 1 - 0 when pressing a clickpad and putting down a second finger.
This happens for example on USB 0603:0002 mt touchpads.
2) According to the Win8 Precision Touchpad spec, inside the HID_UP_BUTTON
usage-page usage 1 is for a clickpad getting clicked, 2 for an external
left button and 3 for an external right button. Since Linux uses
BTN_LEFT for a clickpad being clicked we end up mapping both usage 1
and 2 to BTN_LEFT and if a single report contains both then we ended
up always reporting the value of both in a single SYN, e.g. :
BTN_LEFT 1, BTN_LEFT 0, SYN. This happens for example with Hantick
HTT5288 i2c mt touchpads.
This commit fixes both issues by not immediately reporting buttons when
we parse the report, but instead storing the values of button fields and
reporting the result from mt_sync_frame() when we've a complete frame.
Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
---
Changes in v2:
-Rewrite of v1 of "HID: multitouch: Fix BTN_LEFT 0-1-0-1 events on Novatek
mt touchpad" to kill two birds with one stone
Changes in v3:
-Delay reporting for all buttons not just for BTN_LEFT
---
drivers/hid/hid-multitouch.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
index 5d3904d0e89d..bfbfa04d023a 100644
--- a/drivers/hid/hid-multitouch.c
+++ b/drivers/hid/hid-multitouch.c
@@ -83,6 +83,8 @@ MODULE_LICENSE("GPL");
#define MT_IO_FLAGS_ACTIVE_SLOTS 1
#define MT_IO_FLAGS_PENDING_SLOTS 2
+#define BTN_COUNT (BTN_JOYSTICK - BTN_MOUSE)
+
struct mt_slot {
__s32 x, y, cx, cy, p, w, h;
__s32 contactid; /* the device ContactID assigned to this slot */
@@ -120,6 +122,7 @@ struct mt_device {
int scantime_index; /* scantime field index in the report */
int scantime_val_index; /* scantime value index in the field */
int prev_scantime; /* scantime reported in the previous packet */
+ int buttons_state; /* buttons state */
unsigned last_slot_field; /* the last field of a slot */
unsigned mt_report_id; /* the report ID of the multitouch device */
unsigned long initial_quirks; /* initial quirks state */
@@ -728,9 +731,17 @@ static void mt_complete_slot(struct mt_device *td, struct input_dev *input)
*/
static void mt_sync_frame(struct mt_device *td, struct input_dev *input)
{
+ int i;
+
+ /* For non Win8 devices buttons_state = 0, so this is a no-op */
+ for (i = 0; i < BTN_COUNT; i++)
+ input_event(input, EV_KEY, BTN_MOUSE + i,
+ (td->buttons_state >> i) & 1);
+
input_mt_sync_frame(input);
input_sync(input);
td->num_received = 0;
+ td->buttons_state = 0;
if (test_bit(MT_IO_FLAGS_ACTIVE_SLOTS, &td->mt_io_flags))
set_bit(MT_IO_FLAGS_PENDING_SLOTS, &td->mt_io_flags);
else
@@ -752,6 +763,7 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
struct hid_usage *usage, __s32 value)
{
struct mt_device *td = hid_get_drvdata(hid);
+ __s32 cls = td->mtclass.name;
__s32 quirks = td->mtclass.quirks;
struct input_dev *input = field->hidinput->input;
@@ -805,6 +817,19 @@ static void mt_process_mt_event(struct hid_device *hid, struct hid_field *field,
break;
default:
+ /*
+ * For Win8 PTP touchpads the button state should be
+ * reported once we've a complete frame, so we store
+ * the state here and report it in mt_sync_frame().
+ */
+ if ((cls == MT_CLS_WIN_8 || cls == MT_CLS_WIN_8_DUAL) &&
+ usage->type == EV_KEY && usage->code >= BTN_MOUSE &&
+ usage->code < BTN_JOYSTICK) {
+ int btn = usage->code - BTN_MOUSE;
+ td->buttons_state |= (!!value) << btn;
+ return;
+ }
+
I think there is no point in storing the data and only matching for
BTN_* events here.
How about you simply extend the test below in case the device is
following Win8 protocol and that we have a scantime matching the one
stored in td->prev_scantime?
Since we properly reset num_expected based on scantime now and
num_received when num_received >= num_expected I would prefer to
use num_received == 0, in case of buggy firmware which either:
a) reports different scantime-s for different packets of a single frame
b) always reports 0.
Also this will fix the issue where the BTN_LEFT field is always 0
in "extra" packets in a multi-packet frame with with the USB 0603:0002
mt touchpads, but ...
It will do nothing for the Hantick HTT5288 i2c mt touchpads, which
contain both usage 1 and usage 2 buttons in their descriptors, which
both get mapped to BTN_LEFT. It only has a click (usage 1) not an
external left button (usage 2), so the field with usage 2 always reports
a value of 0, resulting in: BTN_LEFT 1, BTN_LEFT 0, SYN being
reported when the pad is clicked, with the BTN_LEFT 1 being the
result of the field with usage 1 and the BTN_LEFT 0 of the field
with usage 2. To fix this we need to either not map usage 2
(check if there already is a BTN_LEFT mapped, skip or quirk?),
or or together all BTN_LEFT values and report the result at the
end of the frame.
Regards,
Hans
This way we would only deal with non-multitouch events if they are
reported in the first frame, and ignore all of the others.
I do not believe there are other events than BTN_* that are handled
here, so we should be safe.
Cheers,
Benjamin
if (usage->type)
input_event(input, usage->type, usage->code,
value);
--
2.14.3
--
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
