lkml.org 
[lkml]   [2009]   [Jul]   [1]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH 2/2] Add a driver for the Winbond WPCD376I IR functionality.
    On Wed, 1 Jul 2009 09:47:46 +0200 (CEST)
    David H__rdeman <david@hardeman.nu> wrote:

    > >> + struct wbcir_data *data = input_get_drvdata(dev);
    > >> +
    > >> + if (scancode < 0 || scancode > 0xFFFFFFFF)
    > >
    > > Neither of the comparisons in this expression can ever be true.
    >
    > Didn't know if I could be certain that int is always 32 bit on all
    > platforms which use/might use the chip...can I?

    Yep, int and unsigned int are always 32-bit.

    It's not a big deal at all - the compiler will optimise the tests away
    completely. The tests may have some value as code commentary, and they
    provide robustness should someone change the type of `scancode'.

    > >> + 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.

    erp. Hopefully this is all something which Dmitry can help us with.

    > >> +{
    > >> + 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? :)

    yes. The timer will be rescheduledin the scenario which you describe.

    Here's my problem: often when I ask a question about some code, what I
    _really_ mean is "if I didn't understand this code today, others
    probably won't understand it when reading it a year from now. Hence it
    perhaps needs additional commentary to prevent this". But I tire of
    complaining about code comments ;)



    \
     
     \ /
      Last update: 2009-07-01 20:09    [W:0.030 / U:0.764 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site