Hi, Not real feedback, just some nitpicks.
On Tue, July 24, 2007 06:45, Dmitry Torokhov wrote: > +static int input_defuzz_abs_event(int value, int old_val, int fuzz) > +{ > + if (fuzz) { > + if (value > old_val - fuzz / 2 && value < old_val + fuzz / 2) > + return value; > > - add_input_randomness(type, code, value); > + if (value > old_val - fuzz && value < old_val + fuzz) > + return (old_val * 3 + value) / 4; > > - switch (type) { > + if (value > old_val - fuzz * 2 && value < old_val + fuzz * 2) > + return (old_val + value) / 2; > + } Shouldn't the return values of the second and third case be reversed? In the 2nd check the new values is weighted for 1/4, while in the 3rd case it counts for 1/2, which breaks the "account new value more when it is closer to the old one" logic that I thought I saw here. So to sum up, should the second return be "return (old_val + value * 3) / 4"? > +/* > + * Generate software autorepeat event. Note that we take > + * dev->event_lock here to avoid racing with input_event > + * which may cause keys get "stuck". > + */ Hurray. :-) > - if (code > SW_MAX || !test_bit(code, dev->swbit) || > !!test_bit(code, dev->sw) == value) > - return; > + if (dev->rep[REP_PERIOD]) > + mod_timer(&dev->timer, jiffies + > + msecs_to_jiffies(dev->rep[REP_PERIOD])); > + } Perhaps use a local var for the "msecs_to_jiffies(dev->rep[REP_PERIOD])" part. > +static void input_start_autorepeat(struct input_dev *dev, int code) > +{ > + if (test_bit(EV_REP, dev->evbit) && > + dev->rep[REP_PERIOD] && dev->rep[REP_DELAY] && > + dev->timer.data) { > + dev->repeat_key = code; > + mod_timer(&dev->timer, > + jiffies + msecs_to_jiffies(dev->rep[REP_DELAY])); > + } > +} Same here. > + case EV_KEY: > + if (is_event_supported(code, dev->keybit, KEY_MAX) && > + !!test_bit(code, dev->key) != value) { A bit confusing, test_bit(0 only returns 0 or 1 anyway, doesn't it? So "test_bit(code, dev->key) != value" should be all right. I noticed that the old code did it too, but still. > - case EV_MSC: > + case EV_SW: > + if (is_event_supported(code, dev->swbit, SW_MAX) && > + !!test_bit(code, dev->sw) != value) { Same. > - break; > + case EV_LED: > + if (is_event_supported(code, dev->ledbit, LED_MAX) && > + !!test_bit(code, dev->led) != value) { And here. > +void input_inject_event(struct input_handle *handle, > + unsigned int type, unsigned int code, int value) > { > - struct input_dev *dev = (void *) data; > + struct input_dev *dev = handle->dev; > + struct input_handle *grab; > > - if (!test_bit(dev->repeat_key, dev->key)) > - return; > + if (is_event_supported(type, dev->evbit, EV_MAX)) { > + spin_lock_irq(&dev->event_lock); > > - input_event(dev, EV_KEY, dev->repeat_key, 2); > - input_sync(dev); > + grab = rcu_dereference(dev->grab); > + if (!grab || grab == handle) > + input_handle_event(dev, type, code, value); 'handle' can't be NULL, so can drop the "!grab" check, as checking "grab == handle" should be sufficient. > +/** > + * input_open_device - open input device > + * @handle: handle through which device is being accessed > + * > + * This function should be called by input handlers when they > + * want to start receive events from given input device. > + */ > int input_open_device(struct input_handle *handle) > { > struct input_dev *dev = handle->dev; > - int err; > + int retval; > > - err = mutex_lock_interruptible(&dev->mutex); > - if (err) > - return err; > + retval = mutex_lock_interruptible(&dev->mutex); > + if (retval) > + return retval; > + > + if (dev->going_away) { > + retval = -ENODEV; > + goto out; > + } > > handle->open++; > > if (!dev->users++ && dev->open) Ugh, not your code, and perhaps it's me, but that looks weird. The ++ hidden inthe if check is ugly, and would mean that "users" can be negative, which is strange. > - err = dev->open(dev); > + retval = dev->open(dev); > > - if (err) > - handle->open--; > + if (retval && !--handle->open) { Eek! That -- is hidden well there. Would it hurt to call synchronize_sched() unconditionally? Something like: if (retval) { handle->open--; It's a rare case anyway. > + /* > + * Make sure we are not delivering any more events > + * through this handle > + */ > + synchronize_sched(); > + } > > +/** > + * input_close_device - close input device > + * @handle: handle through which device is being accessed > + * > + * This function should be called by input handlers when they > + * want to stop receive events from given input device. > + */ > void input_close_device(struct input_handle *handle) > { > struct input_dev *dev = handle->dev; > > - input_release_device(handle); > - > mutex_lock(&dev->mutex); > > + __input_release_device(handle); > + > if (!--dev->users && dev->close) > dev->close(dev); > - handle->open--; > + > + if (!--handle->open) { > + /* > + * synchronize_sched() makes sure that input_pass_event() > + * completed and that no more input events are delivered > + * through this handle > + */ > + synchronize_sched(); > + } Same here, though just leaving the original "handle->open--;" there and merely adding the if check would be better too I think. Or just get rid of the whole if thing. > static void input_seq_print_bitmap(struct seq_file *seq, const char *name, > @@ -569,7 +765,9 @@ static const struct file_operations inpu > > static void *input_handlers_seq_start(struct seq_file *seq, loff_t *pos) > { > - /* acquire lock here ... Yes, we do need locking, I knowi, I know... */ ;-) > + if (mutex_lock_interruptible(&input_mutex)) > + return NULL; > + > seq->private = (void *)(unsigned long)*pos; > return seq_list_start(&input_handler_list, *pos); > } Greetings, Indan - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [EMAIL PROTECTED] More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/