[PATCH libevdev 7/7] When enabling EV_REP, set the delay/period values

[Peter Hutterer]

On Tue, Jul 30, 2013 at 10:55:59AM +0200, Benjamin Tissoires wrote:
> On Mon, Jul 29, 2013 at 6:06 AM, Peter Hutterer
> <peter.hutterer at who-t.net> wrote:
> > Just enabling EV_REP sets them to zero, but when enabling them directly,
> > a value is required.
> >
> > Signed-off-by: Peter Hutterer <peter.hutterer at who-t.net>
> > ---
> >  libevdev/libevdev.c | 18 +++++++++++++++---
> >  libevdev/libevdev.h |  4 ++--
> >  2 files changed, 17 insertions(+), 5 deletions(-)
> >
> > diff --git a/libevdev/libevdev.c b/libevdev/libevdev.c
> > index eb7401a..e474835 100644
> > --- a/libevdev/libevdev.c
> > +++ b/libevdev/libevdev.c
> > @@ -826,8 +826,16 @@ libevdev_enable_event_type(struct libevdev *dev, unsigned int type)
> >         if (type > EV_MAX)
> >                 return -1;
> >
> > +       if (libevdev_has_event_type(dev, type))
> > +               return 0;
> > +
> >         set_bit(dev->bits, type);
> >
> > +       if (type == EV_REP) {
> > +               int delay = 0, period = 0;
> > +               libevdev_enable_event_code(dev, EV_REP, REP_DELAY, &delay);
> > +               libevdev_enable_event_code(dev, EV_REP, REP_PERIOD, &period);
> > +       }
> >         return 0;
> >  }
> >
> > @@ -852,9 +860,10 @@ libevdev_enable_event_code(struct libevdev *dev, unsigned int type,
> >         if (libevdev_enable_event_type(dev, type))
> >                 return -1;
> >
> > -       if (type != EV_ABS && data != NULL)
> > -               return -1;
> > -       else if (type == EV_ABS && data == NULL)
> > +       if (type == EV_ABS || type == EV_REP) {
> > +               if (data == NULL)
> > +                       return -1;
> > +       } else if (data != NULL)
> >                 return -1;
> 
> This looks a little bit messy... :(
> I find that using a switch instead of the if statements a little bit
> more readable, but in the end, it's the exact same...

good point. Was thinking of it and then decided I was too lazy :)
new diff for this hunk:

@@ -852,13 +860,19 @@ libevdev_enable_event_code(struct libevdev *dev, unsigned int type,
        if (libevdev_enable_event_type(dev, type))
                return -1;

-       if (type != EV_ABS && data != NULL)
-               return -1;
-       else if (type == EV_ABS && data == NULL)
-               return -1;
-
-       if (type == EV_SYN)
-               return 0;
+       switch(type) {
+               case EV_SYN:
+                       return 0;
+               case EV_ABS:
+               case EV_REP:
+                       if (data == NULL)
+                               return -1;
+                       break;
+               default:
+                       if (data != NULL)
+                               return -1;
+                       break;
+       }

        max = type_to_mask(dev, type, &mask);


> 
> >
> >         if (type == EV_SYN)
> > @@ -870,6 +879,9 @@ libevdev_enable_event_code(struct libevdev *dev, unsigned int type,
> >         if (type == EV_ABS) {
> >                 const struct input_absinfo *abs = data;
> >                 dev->abs_info[code] = *abs;
> > +       } else if (type == EV_REP) {
> > +               const int *value = data;
> > +               dev->rep_values[code] = *value;
> >         }
> >
> >         return 0;
> > diff --git a/libevdev/libevdev.h b/libevdev/libevdev.h
> > index 32100fb..606501b 100644
> > --- a/libevdev/libevdev.h
> > +++ b/libevdev/libevdev.h
> > @@ -871,8 +871,8 @@ int libevdev_disable_event_type(struct libevdev *dev, unsigned int type);
> >   * @param dev The evdev device, already initialized with libevdev_set_fd()
> >   * @param type The event type to enable (EV_ABS, EV_KEY, ...)
> >   * @param code The event code to enable (ABS_X, REL_X, etc.)
> > - * @param data If type is EV_ABS, data points to a struct input_absinfo. Otherwise, data must be
> > - * NULL
> > + * @param data If type is EV_ABS, data points to a struct input_absinfo. If type is EV_REP, data
> > + * points to an integer. Otherwise, data must be NULL.
> 
> Shouldn't we add an assert test in test-libevdev-has-event like in 4/7?

yes. Test added, thanks, I've just added a one-line test here, there are more EV_REP tests
on the way but they do rely on the new uinput bits.

@@ -572,6 +572,7 @@ START_TEST(test_device_enable_bit_invalid)
        ck_assert_int_eq(libevdev_enable_event_type(dev, EV_MAX + 1), -1);
 
        ck_assert_int_eq(libevdev_enable_event_code(dev, EV_ABS, ABS_Y, NULL), -1);
+       ck_assert_int_eq(libevdev_enable_event_code(dev, EV_REP, REP_DELAY, NULL), -1);
 
        uinput_device_free(uidev);
        libevdev_free(dev);


> Other than that:
> Reviewed-by: Benjamin Tissoires <benjamin.tissoires at gmail.com>

thanks for the reviews, much appreciated.

Cheers,
   Peter