Re: [PATCH libinput] touchpad: fix DWT pairing for Macbook Pro 2015

[Hans de Goede]

Hi,

On 06-01-16 07:52, Peter Hutterer wrote:

From: Caibin Chen 

Label internal keyboards through the udev hwdb and only pair the internal
(usb) Apple touchpads with those keyboards labelled as such.

https://bugs.freedesktop.org/show_bug.cgi?id=93367

Co-authored-by: Peter Hutterer 
Signed-off-by: Peter Hutterer 


Patch code looks good to me:

Reviewed-by: Hans de Goede 

I do worry a bit if this will not break dwt on older macbooks
though. Maybe we need to also check the bus type in this block:

> +  /* For Apple touchpads, always use its internal keyboard */
> +  if (vendor_tp == VENDOR_ID_APPLE) {
> +  return vendor_kbd == vendor_tp &&
> + keyboard->model_flags &
> +  EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD;
> +  }
> +

Since we are using the usb-vendor id of apple here, right ?

Regards,

Hans




---
  src/evdev-mt-touchpad.c  |   9 +
  src/evdev.c  |   1 +
  src/evdev.h  |   1 +
  src/libinput-util.h  |   1 +
  test/Makefile.am |   1 +
  test/litest-device-apple-internal-keyboard.c | 239 +++
  test/litest.c|   2 +
  test/litest.h|   1 +
  test/touchpad.c  |  87 --
  udev/90-libinput-model-quirks.hwdb   |   3 +
  10 files changed, 328 insertions(+), 17 deletions(-)
  create mode 100644 test/litest-device-apple-internal-keyboard.c

diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
index d78a54b..aa123cd 100644
--- a/src/evdev-mt-touchpad.c
+++ b/src/evdev-mt-touchpad.c
@@ -1285,6 +1285,8 @@ tp_want_dwt(struct evdev_device *touchpad,
  {
unsigned int bus_tp = libevdev_get_id_bustype(touchpad->evdev),
 bus_kbd = libevdev_get_id_bustype(keyboard->evdev);
+   unsigned int vendor_tp = evdev_device_get_id_vendor(touchpad);
+   unsigned int vendor_kbd = evdev_device_get_id_vendor(keyboard);

if (tp_dwt_device_is_blacklisted(touchpad) ||
tp_dwt_device_is_blacklisted(keyboard))
@@ -1295,6 +1297,13 @@ tp_want_dwt(struct evdev_device *touchpad,
if (bus_tp == BUS_I8042 && bus_kbd != bus_tp)
return false;

+   /* For Apple touchpads, always use its internal keyboard */
+   if (vendor_tp == VENDOR_ID_APPLE) {
+   return vendor_kbd == vendor_tp &&
+  keyboard->model_flags &
+   EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD;
+   }
+
/* everything else we don't really know, so we have to assume
   they go together */

diff --git a/src/evdev.c b/src/evdev.c
index d798097..bfaf2ed 100644
--- a/src/evdev.c
+++ b/src/evdev.c
@@ -1662,6 +1662,7 @@ evdev_read_model_flags(struct evdev_device *device)
{ "LIBINPUT_MODEL_SYNAPTICS_SERIAL_TOUCHPAD", 
EVDEV_MODEL_SYNAPTICS_SERIAL_TOUCHPAD },
{ "LIBINPUT_MODEL_JUMPING_SEMI_MT", EVDEV_MODEL_JUMPING_SEMI_MT 
},
{ "LIBINPUT_MODEL_ELANTECH_TOUCHPAD", 
EVDEV_MODEL_ELANTECH_TOUCHPAD },
+   { "LIBINPUT_MODEL_APPLE_INTERNAL_KEYBOARD", 
EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD },
{ NULL, EVDEV_MODEL_DEFAULT },
};
const struct model_map *m = model_map;
diff --git a/src/evdev.h b/src/evdev.h
index 36bf7b4..97177ec 100644
--- a/src/evdev.h
+++ b/src/evdev.h
@@ -108,6 +108,7 @@ enum evdev_device_model {
EVDEV_MODEL_JUMPING_SEMI_MT = (1 << 10),
EVDEV_MODEL_ELANTECH_TOUCHPAD = (1 << 11),
EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81 = (1 << 12),
+   EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD = (1 << 13),
  };

  struct mt_slot {
diff --git a/src/libinput-util.h b/src/libinput-util.h
index a627e5d..25de8e5 100644
--- a/src/libinput-util.h
+++ b/src/libinput-util.h
@@ -38,6 +38,7 @@
  #define VENDOR_ID_APPLE 0x5ac
  #define VENDOR_ID_WACOM 0x56a
  #define VENDOR_ID_SYNAPTICS_SERIAL 0x002
+#define PRODUCT_ID_APPLE_KBD_TOUCHPAD 0x273
  #define PRODUCT_ID_SYNAPTICS_SERIAL 0x007

  /* The HW DPI rate we normalize to before calculating pointer acceleration */
diff --git a/test/Makefile.am b/test/Makefile.am
index 1b3090e..4c394bf 100644
--- a/test/Makefile.am
+++ b/test/Makefile.am
@@ -16,6 +16,7 @@ liblitest_la_SOURCES = \
litest-device-alps-semi-mt.c \
litest-device-alps-dualpoint.c \
litest-device-anker-mouse-kbd.c \
+   litest-device-apple-internal-keyboard.c \
litest-device-asus-rog-gladius.c \
litest-device-atmel-hover.c \
litest-device-bcm5974.c \
diff --git a/test/litest-device-apple-internal-keyboard.c 
b/test/litest-device-apple-internal-keyboard.c
new file mode 100644
index 000..c24403d
--- /dev/null
+++ b/test/litest-device-apple-internal-keyboard.c
@@ -0,0 +1,239 

Re: [PATCH libinput] touchpad: fix DWT pairing for Macbook Pro 2015

[Peter Hutterer]

On Wed, Jan 06, 2016 at 10:59:34AM +0100, Hans de Goede wrote:
> Hi,
> 
> On 06-01-16 07:52, Peter Hutterer wrote:
> >From: Caibin Chen 
> >
> >Label internal keyboards through the udev hwdb and only pair the internal
> >(usb) Apple touchpads with those keyboards labelled as such.
> >
> >https://bugs.freedesktop.org/show_bug.cgi?id=93367
> >
> >Co-authored-by: Peter Hutterer 
> >Signed-off-by: Peter Hutterer 
> 
> Patch code looks good to me:
> 
> Reviewed-by: Hans de Goede 
> 
> I do worry a bit if this will not break dwt on older macbooks
> though. Maybe we need to also check the bus type in this block:
> 
> > +   /* For Apple touchpads, always use its internal keyboard */
> > +   if (vendor_tp == VENDOR_ID_APPLE) {
> > +   return vendor_kbd == vendor_tp &&
> > +  keyboard->model_flags &
> > +   EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD;
> > +   }
> > +
> 
> Since we are using the usb-vendor id of apple here, right ?

IIRC all the macbooks use USB touchpads (if we get here we've already ruled
out bluetooth touchpads). The litest bcm5974 device is from a 2009 macbook
pro and it works with this snippet, so I guess that's old enough :)

Cheers,
   Peter

> >---
> >  src/evdev-mt-touchpad.c  |   9 +
> >  src/evdev.c  |   1 +
> >  src/evdev.h  |   1 +
> >  src/libinput-util.h  |   1 +
> >  test/Makefile.am |   1 +
> >  test/litest-device-apple-internal-keyboard.c | 239 
> > +++
> >  test/litest.c|   2 +
> >  test/litest.h|   1 +
> >  test/touchpad.c  |  87 --
> >  udev/90-libinput-model-quirks.hwdb   |   3 +
> >  10 files changed, 328 insertions(+), 17 deletions(-)
> >  create mode 100644 test/litest-device-apple-internal-keyboard.c
> >
> >diff --git a/src/evdev-mt-touchpad.c b/src/evdev-mt-touchpad.c
> >index d78a54b..aa123cd 100644
> >--- a/src/evdev-mt-touchpad.c
> >+++ b/src/evdev-mt-touchpad.c
> >@@ -1285,6 +1285,8 @@ tp_want_dwt(struct evdev_device *touchpad,
> >  {
> > unsigned int bus_tp = libevdev_get_id_bustype(touchpad->evdev),
> >  bus_kbd = libevdev_get_id_bustype(keyboard->evdev);
> >+unsigned int vendor_tp = evdev_device_get_id_vendor(touchpad);
> >+unsigned int vendor_kbd = evdev_device_get_id_vendor(keyboard);
> >
> > if (tp_dwt_device_is_blacklisted(touchpad) ||
> > tp_dwt_device_is_blacklisted(keyboard))
> >@@ -1295,6 +1297,13 @@ tp_want_dwt(struct evdev_device *touchpad,
> > if (bus_tp == BUS_I8042 && bus_kbd != bus_tp)
> > return false;
> >
> >+/* For Apple touchpads, always use its internal keyboard */
> >+if (vendor_tp == VENDOR_ID_APPLE) {
> >+return vendor_kbd == vendor_tp &&
> >+   keyboard->model_flags &
> >+EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD;
> >+}
> >+
> > /* everything else we don't really know, so we have to assume
> >they go together */
> >
> >diff --git a/src/evdev.c b/src/evdev.c
> >index d798097..bfaf2ed 100644
> >--- a/src/evdev.c
> >+++ b/src/evdev.c
> >@@ -1662,6 +1662,7 @@ evdev_read_model_flags(struct evdev_device *device)
> > { "LIBINPUT_MODEL_SYNAPTICS_SERIAL_TOUCHPAD", 
> > EVDEV_MODEL_SYNAPTICS_SERIAL_TOUCHPAD },
> > { "LIBINPUT_MODEL_JUMPING_SEMI_MT", EVDEV_MODEL_JUMPING_SEMI_MT 
> > },
> > { "LIBINPUT_MODEL_ELANTECH_TOUCHPAD", 
> > EVDEV_MODEL_ELANTECH_TOUCHPAD },
> >+{ "LIBINPUT_MODEL_APPLE_INTERNAL_KEYBOARD", 
> >EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD },
> > { NULL, EVDEV_MODEL_DEFAULT },
> > };
> > const struct model_map *m = model_map;
> >diff --git a/src/evdev.h b/src/evdev.h
> >index 36bf7b4..97177ec 100644
> >--- a/src/evdev.h
> >+++ b/src/evdev.h
> >@@ -108,6 +108,7 @@ enum evdev_device_model {
> > EVDEV_MODEL_JUMPING_SEMI_MT = (1 << 10),
> > EVDEV_MODEL_ELANTECH_TOUCHPAD = (1 << 11),
> > EVDEV_MODEL_LENOVO_X220_TOUCHPAD_FW81 = (1 << 12),
> >+EVDEV_MODEL_APPLE_INTERNAL_KEYBOARD = (1 << 13),
> >  };
> >
> >  struct mt_slot {
> >diff --git a/src/libinput-util.h b/src/libinput-util.h
> >index a627e5d..25de8e5 100644
> >--- a/src/libinput-util.h
> >+++ b/src/libinput-util.h
> >@@ -38,6 +38,7 @@
> >  #define VENDOR_ID_APPLE 0x5ac
> >  #define VENDOR_ID_WACOM 0x56a
> >  #define VENDOR_ID_SYNAPTICS_SERIAL 0x002
> >+#define PRODUCT_ID_APPLE_KBD_TOUCHPAD 0x273
> >  #define PRODUCT_ID_SYNAPTICS_SERIAL 0x007
> >
> >  /* The HW DPI rate we normalize to before calculating pointer acceleration 
> > */
> >diff --git a/test/Makefile.am b/test/Makefile.am
> >index 1b3090e..4c394bf 100644
> >--- a/test/Makefile.am
> >+++ b/test/Makefile.am
> >@@ -16,6 +16,7 @@