Re: [PATCH libinput 1/2] Hook up libevdev as backend

2014-02-23 Thread Peter Hutterer 
On Sat, Feb 22, 2014 at 03:51:57PM +0100, Jonas Ådahl wrote:
 On Tue, Feb 18, 2014 at 04:09:09PM +1000, Peter Hutterer wrote:
  libevdev wraps the various peculiarities of the evdev kernel API into a
  type-safe API. It also buffers the device so checking for specific features 
  at
  a later time is easier than re-issuing the ioctls. Plus, it gives us almost
  free support for SYN_DROPPED events (in the following patch).
  
  This patch switches all the bit checks over to libevdev and leaves the event
  processing as-is. Makes it easier to review.
  
  Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
 
 Looks good to me as well, with one comment inline.
 
 Reviewed-by: Jonas Ådahl jad...@gmail.com

..

  @@ -624,6 +607,10 @@ evdev_device_create(struct libinput_seat *seat,
   
  libinput_device_init(device-base, seat);
   
  +   rc = libevdev_new_from_fd(fd, device-evdev);
  +   if (rc != 0)
  +   return NULL;
  +
  device-seat_caps = 0;
  device-is_mt = 0;
  device-mtdev = NULL;
  @@ -635,10 +622,7 @@ evdev_device_create(struct libinput_seat *seat,
  device-dispatch = NULL;
  device-fd = fd;
  device-pending_event = EVDEV_NONE;
  -
  -   ioctl(device-fd, EVIOCGNAME(sizeof(devname)), devname);
  -   devname[sizeof(devname) - 1] = '\0';
  -   device-devname = strdup(devname);
  +   device-devname = libevdev_get_name(device-evdev);
 
 This makes the assumption that the const char * returned by
 libevdev_get_name() is valid until we destroy the device. Is this
 guaranteed anywhere by libevdev?

It's guaranteed by the kernel. There is no facility to set the name through
the API and there is no facility to notify the caller if the name would
change. so libevdev (which has a copy, obviuosly) wouldn't know that it
changed. libevdev_change_fd() doesn't re-sync the name, so yes, this name
is constant.

Cheers,
   Peter

 
   
  libinput_seat_ref(seat);
   
  @@ -742,8 +726,7 @@ evdev_device_destroy(struct evdev_device *device)
  dispatch-interface-destroy(dispatch);
   
  libinput_seat_unref(device-base.seat);
  -
  -   free(device-devname);
  +   libevdev_free(device-evdev);
  free(device-devnode);
  free(device-sysname);
  free(device);
  diff --git a/src/evdev.h b/src/evdev.h
  index 3c9f93a..a9e27bf 100644
  --- a/src/evdev.h
  +++ b/src/evdev.h
  @@ -27,6 +27,7 @@
   #include config.h
   
   #include linux/input.h
  +#include libevdev/libevdev.h
   
   #include libinput-private.h
   
  @@ -55,10 +56,11 @@ struct evdev_device {
  struct libinput_source *source;
   
  struct evdev_dispatch *dispatch;
  +   struct libevdev *evdev;
  char *output_name;
  char *devnode;
  char *sysname;
  -   char *devname;
  +   const char *devname;
  int fd;
  struct {
  int min_x, max_x, min_y, max_y;
  @@ -86,16 +88,6 @@ struct evdev_device {
  int is_mt;
   };
   
  -/* copied from udev/extras/input_id/input_id.c */
  -/* we must use this kernel-compatible implementation */
  -#define BITS_PER_LONG (sizeof(unsigned long) * 8)
  -#define NBITS(x) x)-1)/BITS_PER_LONG)+1)
  -#define OFF(x)  ((x)%BITS_PER_LONG)
  -#define BIT(x)  (1ULOFF(x))
  -#define LONG(x) ((x)/BITS_PER_LONG)
  -#define TEST_BIT(array, bit)((array[LONG(bit)]  OFF(bit))  1)
  -/* end copied */
  -
   #define EVDEV_UNHANDLED_DEVICE ((struct evdev_device *) 1)
   
   struct evdev_dispatch;
  -- 
  1.8.4.2
  
  ___
  wayland-devel mailing list
  wayland-devel@lists.freedesktop.org
  http://lists.freedesktop.org/mailman/listinfo/wayland-devel
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 1/2] Hook up libevdev as backend

2014-02-23 Thread Peter Hutterer 
On Mon, Feb 24, 2014 at 09:28:49AM +1000, Peter Hutterer wrote:
 On Sat, Feb 22, 2014 at 03:51:57PM +0100, Jonas Ådahl wrote:
  On Tue, Feb 18, 2014 at 04:09:09PM +1000, Peter Hutterer wrote:
   libevdev wraps the various peculiarities of the evdev kernel API into a
   type-safe API. It also buffers the device so checking for specific 
   features at
   a later time is easier than re-issuing the ioctls. Plus, it gives us 
   almost
   free support for SYN_DROPPED events (in the following patch).
   
   This patch switches all the bit checks over to libevdev and leaves the 
   event
   processing as-is. Makes it easier to review.
   
   Signed-off-by: Peter Hutterer peter.hutte...@who-t.net
  
  Looks good to me as well, with one comment inline.
  
  Reviewed-by: Jonas Ådahl jad...@gmail.com
 
 ...
 
   @@ -624,6 +607,10 @@ evdev_device_create(struct libinput_seat *seat,

 libinput_device_init(device-base, seat);

   + rc = libevdev_new_from_fd(fd, device-evdev);
   + if (rc != 0)
   + return NULL;
   +
 device-seat_caps = 0;
 device-is_mt = 0;
 device-mtdev = NULL;
   @@ -635,10 +622,7 @@ evdev_device_create(struct libinput_seat *seat,
 device-dispatch = NULL;
 device-fd = fd;
 device-pending_event = EVDEV_NONE;
   -
   - ioctl(device-fd, EVIOCGNAME(sizeof(devname)), devname);
   - devname[sizeof(devname) - 1] = '\0';
   - device-devname = strdup(devname);
   + device-devname = libevdev_get_name(device-evdev);
  
  This makes the assumption that the const char * returned by
  libevdev_get_name() is valid until we destroy the device. Is this
  guaranteed anywhere by libevdev?
 
 It's guaranteed by the kernel. There is no facility to set the name through
 the API and there is no facility to notify the caller if the name would
 change. so libevdev (which has a copy, obviuosly) wouldn't know that it
 changed. libevdev_change_fd() doesn't re-sync the name, so yes, this name
 is constant.
 
I should've been more precise here: there is no facility to set the name
through the _kernel_ API. libevdev enables a caller to change the name (and
thus free the string) though I'm strongly inclined to say that's a
caller problem that we don't need to worry about.

I'll add some extra notes to the libevdev documentation here.

Cheers,
   Peter
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel

Re: [PATCH libinput 1/2] Hook up libevdev as backend

2014-02-22 Thread Jonas Ådahl 
On Tue, Feb 18, 2014 at 04:09:09PM +1000, Peter Hutterer wrote:
 libevdev wraps the various peculiarities of the evdev kernel API into a
 type-safe API. It also buffers the device so checking for specific features at
 a later time is easier than re-issuing the ioctls. Plus, it gives us almost
 free support for SYN_DROPPED events (in the following patch).
 
 This patch switches all the bit checks over to libevdev and leaves the event
 processing as-is. Makes it easier to review.
 
 Signed-off-by: Peter Hutterer peter.hutte...@who-t.net

Looks good to me as well, with one comment inline.

Reviewed-by: Jonas Ådahl jad...@gmail.com

 ---
  configure.ac |  7 ++---
  src/Makefile.am  |  2 ++
  src/evdev-touchpad.c | 25 ++-
  src/evdev.c  | 87 
 +---
  src/evdev.h  | 14 ++---
  5 files changed, 52 insertions(+), 83 deletions(-)
 
 diff --git a/configure.ac b/configure.ac
 index 44729a9..68e1d35 100644
 --- a/configure.ac
 +++ b/configure.ac
 @@ -44,6 +44,7 @@ AC_CHECK_DECL(CLOCK_MONOTONIC,[],
  PKG_PROG_PKG_CONFIG()
  PKG_CHECK_MODULES(MTDEV, [mtdev = 1.1.0])
  PKG_CHECK_MODULES(LIBUDEV, [libudev])
 +PKG_CHECK_MODULES(LIBEVDEV, [libevdev = 0.4])
  
  if test x$GCC = xyes; then
   GCC_CFLAGS=-Wall -Wextra -Wno-unused-parameter -g -Wstrict-prototypes 
 -Wmissing-prototypes -fvisibility=hidden
 @@ -64,20 +65,16 @@ AC_ARG_ENABLE(tests,
 [build_tests=$enableval],
 [build_tests=auto])
  
 -PKG_CHECK_MODULES(LIBEVDEV, [libevdev = 0.4], [HAVE_LIBEVDEV=yes], 
 [HAVE_LIBEVDEV=no])
  PKG_CHECK_MODULES(CHECK, [check = 0.9.9], [HAVE_CHECK=yes], 
 [HAVE_CHECK=no])
  
  if test x$build_tests = xauto; then
 - if test x$HAVE_CHECK = xyes -a x$HAVE_LIBEVDEV = xyes; then
 + if test x$HAVE_CHECK = xyes; then
   build_tests=yes
   fi
  fi
  if test x$build_tests = xyes -a x$HAVE_CHECK = xno; then
 AC_MSG_ERROR([Cannot build tests, check is missing])
  fi
 -if test x$build_tests = xyes -a x$HAVE_LIBEVDEV = xno; then
 -   AC_MSG_ERROR([Cannot build tests, libevdev is missing])
 -fi
  
  AM_CONDITIONAL(BUILD_TESTS, [test x$build_tests = xyes])
  
 diff --git a/src/Makefile.am b/src/Makefile.am
 index 6e27b3b..f544ccd 100644
 --- a/src/Makefile.am
 +++ b/src/Makefile.am
 @@ -20,9 +20,11 @@ libinput_la_SOURCES =  \
  
  libinput_la_LIBADD = $(MTDEV_LIBS) \
$(LIBUDEV_LIBS) \
 +  $(LIBEVDEV_LIBS) \
-lm
  libinput_la_CFLAGS = $(MTDEV_CFLAGS) \
$(LIBUDEV_CFLAGS)  \
 +  $(LIBEVDEV_CFLAGS) \
$(GCC_CFLAGS)
  
  pkgconfigdir = $(libdir)/pkgconfig
 diff --git a/src/evdev-touchpad.c b/src/evdev-touchpad.c
 index d65ebb2..8185bf2 100644
 --- a/src/evdev-touchpad.c
 +++ b/src/evdev-touchpad.c
 @@ -170,16 +170,16 @@ struct touchpad_dispatch {
  static enum touchpad_model
  get_touchpad_model(struct evdev_device *device)
  {
 - struct input_id id;
 + int vendor, product;
   unsigned int i;
  
 - if (ioctl(device-fd, EVIOCGID, id)  0)
 - return TOUCHPAD_MODEL_UNKNOWN;
 + vendor = libevdev_get_id_vendor(device-evdev);
 + product = libevdev_get_id_product(device-evdev);
  
   for (i = 0; i  ARRAY_LENGTH(touchpad_spec_table); i++)
 - if (touchpad_spec_table[i].vendor == id.vendor 
 + if (touchpad_spec_table[i].vendor == vendor 
   (!touchpad_spec_table[i].product ||
 -  touchpad_spec_table[i].product == id.product))
 +  touchpad_spec_table[i].product == product))
   return touchpad_spec_table[i].model;
  
   return TOUCHPAD_MODEL_UNKNOWN;
 @@ -730,9 +730,7 @@ touchpad_init(struct touchpad_dispatch *touchpad,
  {
   struct motion_filter *accel;
  
 - unsigned long prop_bits[INPUT_PROP_MAX];
 - struct input_absinfo absinfo;
 - unsigned long abs_bits[NBITS(ABS_MAX)];
 + const struct input_absinfo *absinfo;
  
   bool has_buttonpad;
  
 @@ -746,16 +744,13 @@ touchpad_init(struct touchpad_dispatch *touchpad,
   /* Detect model */
   touchpad-model = get_touchpad_model(device);
  
 - ioctl(device-fd, EVIOCGPROP(sizeof(prop_bits)), prop_bits);
 - has_buttonpad = TEST_BIT(prop_bits, INPUT_PROP_BUTTONPAD);
 + has_buttonpad = libevdev_has_property(device-evdev, 
 INPUT_PROP_BUTTONPAD);
  
   /* Configure pressure */
 - ioctl(device-fd, EVIOCGBIT(EV_ABS, sizeof(abs_bits)), abs_bits);
 - if (TEST_BIT(abs_bits, ABS_PRESSURE)) {
 - ioctl(device-fd, EVIOCGABS(ABS_PRESSURE), absinfo);
 + if ((absinfo = libevdev_get_abs_info(device-evdev, ABS_PRESSURE))) {
   configure_touchpad_pressure(touchpad,
 - absinfo.minimum,
 - absinfo.maximum);
 + absinfo-minimum,

Re: [PATCH libinput 1/2] Hook up libevdev as backend

2014-02-18 Thread Rui Tiago Cação Matos 
On 18 February 2014 07:09, Peter Hutterer peter.hutte...@who-t.net wrote:
 libevdev wraps the various peculiarities of the evdev kernel API into a
 type-safe API. It also buffers the device so checking for specific features at
 a later time is easier than re-issuing the ioctls. Plus, it gives us almost
 free support for SYN_DROPPED events (in the following patch).

 This patch switches all the bit checks over to libevdev and leaves the event
 processing as-is. Makes it easier to review.

This looks fine to me. Can we also add a

libevdev_set_clock_id(device-evdev, CLOCK_MONOTONIC);

on device creation? CLOCK_MONOTONIC is already a hard dependency in
configure.ac and it's required for good interoperability between
xwayland (which uses it for its time stamps) and mutter.

Rui
___
wayland-devel mailing list
wayland-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/wayland-devel