Re: [PATCH 2/2] Add a driver for the Winbond WPCD376I IR functionality.

> >> + return -EINVAL;
> >> +
> >> + *keycode = (int)wbcir_do_getkeycode(data, (u32)scancode);
> >
> > uneeded casts.
> >
> > Something has gone wrong with the types here.  Where does the fault lie
> > - this driver, or input core?
>
> Two issues:
>
> a) the input layer API confused me
>
> include/linux/input.h provides:
>
> struct input_event {
>          struct timeval time;
>          __u16 type;
>          __u16 code;
>          __s32 value;
> };
>
> (keycode is an unsigned 16 bit integer)
>
> int input_get_keycode(struct input_dev *dev, int scancode, int *keycode);
> int input_set_keycode(struct input_dev *dev, int scancode, int keycode);
>
> (keycode is an int)
>
> static inline void input_report_key(struct input_dev *dev,
>                                     unsigned int code,
>                                     int value)
> {
>         input_event(dev, EV_KEY, code, !!value);
> }
>
> (keycode is an uint)
>
>
> b) 32 bit scancodes
>
> I wanted to use 32 bit scancodes in my driver since the largest IR message
> supported by the driver is RC6 mode 6A which can potentially have a 1 + 15
> bits "customer" field + 8 bits address + 8 bits command = 32 bits.
>
> Casting the int scancode passed to input_[get__set]_keycode to an uint and
> assuming it would be at least 32 bits on all platforms using the chip was
> the best solution I could come up with without changing the input API.

> >> +{
> >> +        struct wbcir_data *data = (struct wbcir_data *)cookie;
> >> +        unsigned long flags;
> >> +
> >> +        /*
> >> +         * data->keyup_jiffies is used to prevent a race condition if a
> >> +         * hardware interrupt occurs at this point and the keyup timer
> >> +         * event is moved further into the future as a result.
> >> +         */
> >
> >hm.  I don't see what the race is, nor how the comparison fixes it.  If
> >I _did_ understand this, perhaps I could suggest alternative fixes.
> >But I don't so I can't.  Oh well.
>
> When the interrupt service routine detects an IR command it reports a
> keydown event and sets a timer to report a keyup event in the future if no
> repeated ir messages are detected (in which case the timer-driven keyup
> should be pushed further into the future to allow the input core to do its
> auto-repeat-handling magic).
>
> What I wanted to protect against was something like this:
>
> Thread 1                        Thread 2
> --------                        --------
> ISR called, grabs
> wbcir_lock, reports
> keydown for key X,
> sets up keyup timer,
> releases lock, exits
>
>                     (many ms later)
>
>                                 keyup timer function called
>                                 and preempted before grabbing
>                                 wbcir_lock
>
> ISR called, grabs wbcir_lock,
> notices a repeat event for
> key X, pushes the keyup timer
> further into the future using
> mod_timer (thus reenabling the
> timer), releases lock, exits
>                                 keyup timer function grabs
>                                 wbcir_lock, reports keyup,
>                                 exits.
>                     (many ms later)
>
>                                 keyup timer function called *again*,
>                                 reports keyup, exits.
>
> The result would be (if I understood the timer implementation correctly)
> that a keyup event is reported immediately after the second ISR even
> though the "first" timer function call should have been cancelled/pushed
> further into the future at that point.
>
> Does this make any sense? :)