* [patch 0/3] ir-core keytable patches @ 2010-04-02 18:58 ` david 0 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media The following patchset provides a number of bug fixes and additional features to ir-core in drivers/media/IR. The first two patches are mostly cleanups to drivers/media/IR/ir-keytable.c while the third patch is an example of a conversion of drivers/media/dvb/ttpci/budget-ci.c to use the new functionality (rather than the drivers/media/IR/ir-functions.c functionality). -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 0/3] ir-core keytable patches @ 2010-04-02 18:58 ` david 0 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: text/plain, Size: 393 bytes --] The following patchset provides a number of bug fixes and additional features to ir-core in drivers/media/IR. The first two patches are mostly cleanups to drivers/media/IR/ir-keytable.c while the third patch is an example of a conversion of drivers/media/dvb/ttpci/budget-ci.c to use the new functionality (rather than the drivers/media/IR/ir-functions.c functionality). -- David Härdeman ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 1/3] [PATCH] drivers/media/IR - improve keytable code 2010-04-02 18:58 ` david @ 2010-04-02 18:58 ` david -1 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Attachment #1: improve-keytable-code.patch --] [-- Type: TEXT/PLAIN, Size: 21953 bytes --] The attached patch rewrites much of the keytable code in drivers/media/IR/ir-keytable.c. The scancodes are now inserted into the array in sorted order which allows for a binary search on lookup. The code has also been shrunk by about 150 lines. In addition it fixes the following bugs: Any use of ir_seek_table() was racy. ir_dev->driver_name is leaked between ir_input_register() and ir_input_unregister(). ir_setkeycode() unconditionally does clear_bit() on dev->keybit when removing a mapping, but there might be another mapping with a different scancode and the same keycode. This version has been updated to incorporate patch feedback from Mauro Carvalho Chehab. Signed-off-by: David Härdeman <david@hardeman.nu> Index: ir/drivers/media/IR/ir-keymaps.c =================================================================== --- ir.orig/drivers/media/IR/ir-keymaps.c 2010-04-01 17:38:16.777724334 +0200 +++ ir/drivers/media/IR/ir-keymaps.c 2010-04-02 12:23:14.952235145 +0200 @@ -39,6 +39,7 @@ struct ir_scancode_table tabname ## _table = { \ .scan = tabname, \ .size = ARRAY_SIZE(tabname), \ + .len = ARRAY_SIZE(tabname), \ .ir_type = type, \ .name = #irname, \ }; \ Index: ir/drivers/media/IR/ir-keytable.c =================================================================== --- ir.orig/drivers/media/IR/ir-keytable.c 2010-04-01 17:38:16.753724054 +0200 +++ ir/drivers/media/IR/ir-keytable.c 2010-04-02 12:32:13.015233729 +0200 @@ -16,344 +16,214 @@ #include <linux/input.h> #include <media/ir-common.h> -#define IR_TAB_MIN_SIZE 32 -#define IR_TAB_MAX_SIZE 1024 +/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ +#define IR_TAB_MIN_SIZE 256 +#define IR_TAB_MAX_SIZE 8192 /** - * ir_seek_table() - returns the element order on the table - * @rc_tab: the ir_scancode_table with the keymap to be used - * @scancode: the scancode that we're seeking + * ir_resize_table() - resizes a scancode table if necessary + * @rc_tab: the ir_scancode_table to resize + * @return: zero on success or a negative error code * - * This routine is used by the input routines when a key is pressed at the - * IR. The scancode is received and needs to be converted into a keycode. - * If the key is not found, it returns KEY_UNKNOWN. Otherwise, returns the - * corresponding keycode from the table. + * This routine will shrink the ir_scancode_table if it has lots of + * unused entries and grow it if it is full. */ -static int ir_seek_table(struct ir_scancode_table *rc_tab, u32 scancode) +static int ir_resize_table(struct ir_scancode_table *rc_tab) { - int rc; - unsigned long flags; - struct ir_scancode *keymap = rc_tab->scan; - - spin_lock_irqsave(&rc_tab->lock, flags); - - /* FIXME: replace it by a binary search */ + unsigned int oldalloc = rc_tab->alloc; + unsigned int newalloc = oldalloc; + struct ir_scancode *oldscan = rc_tab->scan; + struct ir_scancode *newscan; - for (rc = 0; rc < rc_tab->size; rc++) - if (keymap[rc].scancode == scancode) - goto exit; - - /* Not found */ - rc = -EINVAL; + if (rc_tab->size == rc_tab->len) { + /* All entries in use -> grow keytable */ + if (rc_tab->alloc >= IR_TAB_MAX_SIZE) + return -ENOMEM; -exit: - spin_unlock_irqrestore(&rc_tab->lock, flags); - return rc; -} + newalloc *= 2; + IR_dprintk(1, "Growing table to %u bytes\n", newalloc); + } -/** - * ir_roundup_tablesize() - gets an optimum value for the table size - * @n_elems: minimum number of entries to store keycodes - * - * This routine is used to choose the keycode table size. - * - * In order to have some empty space for new keycodes, - * and knowing in advance that kmalloc allocates only power of two - * segments, it optimizes the allocated space to have some spare space - * for those new keycodes by using the maximum number of entries that - * will be effectively be allocated by kmalloc. - * In order to reduce the quantity of table resizes, it has a minimum - * table size of IR_TAB_MIN_SIZE. - */ -static int ir_roundup_tablesize(int n_elems) -{ - size_t size; + if ((rc_tab->len * 3 < rc_tab->size) && (oldalloc > IR_TAB_MIN_SIZE)) { + /* Less than 1/3 of entries in use -> shrink keytable */ + newalloc /= 2; + IR_dprintk(1, "Shrinking table to %u bytes\n", newalloc); + } - if (n_elems < IR_TAB_MIN_SIZE) - n_elems = IR_TAB_MIN_SIZE; + if (newalloc == oldalloc) + return 0; - /* - * As kmalloc only allocates sizes of power of two, get as - * much entries as possible for the allocated memory segment - */ - size = roundup_pow_of_two(n_elems * sizeof(struct ir_scancode)); - n_elems = size / sizeof(struct ir_scancode); + newscan = kmalloc(newalloc, GFP_ATOMIC); + if (!newscan) { + IR_dprintk(1, "Failed to kmalloc %u bytes\n", newalloc); + return -ENOMEM; + } - return n_elems; + memcpy(newscan, rc_tab->scan, rc_tab->len * sizeof(struct ir_scancode)); + rc_tab->scan = newscan; + rc_tab->alloc = newalloc; + rc_tab->size = rc_tab->alloc / sizeof(struct ir_scancode); + kfree(oldscan); + return 0; } /** - * ir_copy_table() - copies a keytable, discarding the unused entries - * @destin: destin table - * @origin: origin table - * - * Copies all entries where the keycode is not KEY_UNKNOWN/KEY_RESERVED - * Also copies table size and table protocol. - * NOTE: It shouldn't copy the lock field - */ - -static int ir_copy_table(struct ir_scancode_table *destin, - const struct ir_scancode_table *origin) -{ - int i, j = 0; - - for (i = 0; i < origin->size; i++) { - if (origin->scan[i].keycode == KEY_UNKNOWN || - origin->scan[i].keycode == KEY_RESERVED) + * ir_do_setkeycode() - internal function to set a keycode in the + * scancode->keycode table + * @dev: the struct input_dev device descriptor + * @rc_tab: the struct ir_scancode_table to set the keycode in + * @scancode: the scancode for the ir command + * @keycode: the keycode for the ir command + * @return: -EINVAL if the keycode could not be inserted, otherwise zero. + * + * This routine is used internally to manipulate the scancode->keycode table. + * The caller has to hold @rc_tab->lock. + */ +static int ir_do_setkeycode(struct input_dev *dev, + struct ir_scancode_table *rc_tab, + int scancode, int keycode) +{ + unsigned int i; + int old_keycode = KEY_RESERVED; + + /* First check if we already have a mapping for this ir command */ + for (i = 0; i < rc_tab->len; i++) { + /* Keytable is sorted from lowest to highest scancode */ + if (rc_tab->scan[i].scancode > scancode) + break; + else if (rc_tab->scan[i].scancode < scancode) continue; - memcpy(&destin->scan[j], &origin->scan[i], sizeof(struct ir_scancode)); - j++; + old_keycode = rc_tab->scan[i].keycode; + rc_tab->scan[i].keycode = keycode; + + /* Did the user wish to remove the mapping? */ + if (keycode == KEY_RESERVED || keycode == KEY_UNKNOWN) { + rc_tab->len--; + memmove(&rc_tab->scan[i], &rc_tab->scan[i + 1], + (rc_tab->len - i) * sizeof(struct ir_scancode)); + } + + /* Possibly shrink the keytable, failure is not a problem */ + ir_resize_table(rc_tab); + break; } - destin->size = j; - destin->ir_type = origin->ir_type; - IR_dprintk(1, "Copied %d scancodes to the new keycode table\n", destin->size); + if (old_keycode == KEY_RESERVED) { + /* No previous mapping found, we might need to grow the table */ + if (ir_resize_table(rc_tab)) + return -ENOMEM; + + /* i is the proper index to insert our new keycode */ + memmove(&rc_tab->scan[i + 1], &rc_tab->scan[i], + (rc_tab->len - i) * sizeof(struct ir_scancode)); + rc_tab->scan[i].scancode = scancode; + rc_tab->scan[i].keycode = keycode; + rc_tab->len++; + set_bit(keycode, dev->keybit); + } else { + /* A previous mapping was updated... */ + clear_bit(old_keycode, dev->keybit); + /* ...but another scancode might use the same keycode */ + for (i = 0; i < rc_tab->len; i++) { + if (rc_tab->scan[i].keycode == old_keycode) { + set_bit(old_keycode, dev->keybit); + break; + } + } + } return 0; } /** - * ir_getkeycode() - get a keycode at the evdev scancode ->keycode table + * ir_setkeycode() - set a keycode in the scancode->keycode table * @dev: the struct input_dev device descriptor * @scancode: the desired scancode - * @keycode: the keycode to be retorned. + * @keycode: result + * @return: -EINVAL if the keycode could not be inserted, otherwise zero. * - * This routine is used to handle evdev EVIOCGKEY ioctl. - * If the key is not found, returns -EINVAL, otherwise, returns 0. + * This routine is used to handle evdev EVIOCSKEY ioctl. */ -static int ir_getkeycode(struct input_dev *dev, - int scancode, int *keycode) +static int ir_setkeycode(struct input_dev *dev, + int scancode, int keycode) { - int elem; + int rc; + unsigned long flags; struct ir_input_dev *ir_dev = input_get_drvdata(dev); struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; - elem = ir_seek_table(rc_tab, scancode); - if (elem >= 0) { - *keycode = rc_tab->scan[elem].keycode; - return 0; - } - - /* - * Scancode not found and table can't be expanded - */ - if (elem < 0 && rc_tab->size == IR_TAB_MAX_SIZE) - return -EINVAL; - - /* - * If is there extra space, returns KEY_RESERVED, - * otherwise, input core won't let ir_setkeycode to work - */ - *keycode = KEY_RESERVED; - return 0; -} - -/** - * ir_is_resize_needed() - Check if the table needs rezise - * @table: keycode table that may need to resize - * @n_elems: minimum number of entries to store keycodes - * - * Considering that kmalloc uses power of two storage areas, this - * routine detects if the real alloced size will change. If not, it - * just returns without doing nothing. Otherwise, it will extend or - * reduce the table size to meet the new needs. - * - * It returns 0 if no resize is needed, 1 otherwise. - */ -static int ir_is_resize_needed(struct ir_scancode_table *table, int n_elems) -{ - int cur_size = ir_roundup_tablesize(table->size); - int new_size = ir_roundup_tablesize(n_elems); - - if (cur_size == new_size) - return 0; - - /* Resize is needed */ - return 1; -} - -/** - * ir_delete_key() - remove a keycode from the table - * @rc_tab: keycode table - * @elem: element to be removed - * - */ -static void ir_delete_key(struct ir_scancode_table *rc_tab, int elem) -{ - unsigned long flags = 0; - int newsize = rc_tab->size - 1; - int resize = ir_is_resize_needed(rc_tab, newsize); - struct ir_scancode *oldkeymap = rc_tab->scan; - struct ir_scancode *newkeymap = NULL; - - if (resize) - newkeymap = kzalloc(ir_roundup_tablesize(newsize) * - sizeof(*newkeymap), GFP_ATOMIC); - - /* There's no memory for resize. Keep the old table */ - if (!resize || !newkeymap) { - newkeymap = oldkeymap; - - /* We'll modify the live table. Lock it */ - spin_lock_irqsave(&rc_tab->lock, flags); - } - - /* - * Copy the elements before the one that will be deleted - * if (!resize), both oldkeymap and newkeymap points - * to the same place, so, there's no need to copy - */ - if (resize && elem > 0) - memcpy(newkeymap, oldkeymap, - elem * sizeof(*newkeymap)); - - /* - * Copy the other elements overwriting the element to be removed - * This operation applies to both resize and non-resize case - */ - if (elem < newsize) - memcpy(&newkeymap[elem], &oldkeymap[elem + 1], - (newsize - elem) * sizeof(*newkeymap)); - - if (resize) { - /* - * As the copy happened to a temporary table, only here - * it needs to lock while replacing the table pointers - * to use the new table - */ - spin_lock_irqsave(&rc_tab->lock, flags); - rc_tab->size = newsize; - rc_tab->scan = newkeymap; - spin_unlock_irqrestore(&rc_tab->lock, flags); - - /* Frees the old keytable */ - kfree(oldkeymap); - } else { - rc_tab->size = newsize; - spin_unlock_irqrestore(&rc_tab->lock, flags); - } + spin_lock_irqsave(&rc_tab->lock, flags); + rc = ir_do_setkeycode(dev, rc_tab, scancode, keycode); + spin_unlock_irqrestore(&rc_tab->lock, flags); + return rc; } /** - * ir_insert_key() - insert a keycode at the table - * @rc_tab: keycode table - * @scancode: the desired scancode - * @keycode: the keycode to be retorned. - * - */ -static int ir_insert_key(struct ir_scancode_table *rc_tab, - int scancode, int keycode) + * ir_setkeytable() - sets several entries in the scancode->keycode table + * @dev: the struct input_dev device descriptor + * @to: the struct ir_scancode_table to copy entries to + * @from: the struct ir_scancode_table to copy entries from + * @return: -EINVAL if all keycodes could not be inserted, otherwise zero. + * + * This routine is used to handle table initialization. + */ +static int ir_setkeytable(struct input_dev *dev, + struct ir_scancode_table *to, + const struct ir_scancode_table *from) { + struct ir_input_dev *ir_dev = input_get_drvdata(dev); + struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; unsigned long flags; - int elem = rc_tab->size; - int newsize = rc_tab->size + 1; - int resize = ir_is_resize_needed(rc_tab, newsize); - struct ir_scancode *oldkeymap = rc_tab->scan; - struct ir_scancode *newkeymap; - - if (resize) { - newkeymap = kzalloc(ir_roundup_tablesize(newsize) * - sizeof(*newkeymap), GFP_ATOMIC); - if (!newkeymap) - return -ENOMEM; - - memcpy(newkeymap, oldkeymap, - rc_tab->size * sizeof(*newkeymap)); - } else - newkeymap = oldkeymap; - - /* Stores the new code at the table */ - IR_dprintk(1, "#%d: New scan 0x%04x with key 0x%04x\n", - rc_tab->size, scancode, keycode); + unsigned int i; + int rc = 0; spin_lock_irqsave(&rc_tab->lock, flags); - rc_tab->size = newsize; - if (resize) { - rc_tab->scan = newkeymap; - kfree(oldkeymap); + for (i = 0; i < from->len; i++) { + rc = ir_do_setkeycode(dev, to, from->scan[i].scancode, + from->scan[i].keycode); + if (rc) + break; } - newkeymap[elem].scancode = scancode; - newkeymap[elem].keycode = keycode; spin_unlock_irqrestore(&rc_tab->lock, flags); - - return 0; + return rc; } /** - * ir_setkeycode() - set a keycode at the evdev scancode ->keycode table + * ir_getkeycode() - get a keycode from the scancode->keycode table * @dev: the struct input_dev device descriptor * @scancode: the desired scancode - * @keycode: the keycode to be retorned. + * @keycode: used to return the keycode, if found, or KEY_RESERVED + * @return: always returns zero. * - * This routine is used to handle evdev EVIOCSKEY ioctl. - * There's one caveat here: how can we increase the size of the table? - * If the key is not found, returns -EINVAL, otherwise, returns 0. + * This routine is used to handle evdev EVIOCGKEY ioctl. */ -static int ir_setkeycode(struct input_dev *dev, - int scancode, int keycode) +static int ir_getkeycode(struct input_dev *dev, + int scancode, int *keycode) { - int rc = 0; + int start, end, mid; + unsigned long flags; + int key = KEY_RESERVED; struct ir_input_dev *ir_dev = input_get_drvdata(dev); struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; - struct ir_scancode *keymap = rc_tab->scan; - unsigned long flags; - - /* - * Handle keycode table deletions - * - * If userspace is adding a KEY_UNKNOWN or KEY_RESERVED, - * deal as a trial to remove an existing scancode attribution - * if table become too big, reduce it to save space - */ - if (keycode == KEY_UNKNOWN || keycode == KEY_RESERVED) { - rc = ir_seek_table(rc_tab, scancode); - if (rc < 0) - return 0; - - IR_dprintk(1, "#%d: Deleting scan 0x%04x\n", rc, scancode); - clear_bit(keymap[rc].keycode, dev->keybit); - ir_delete_key(rc_tab, rc); - return 0; - } - - /* - * Handle keycode replacements - * - * If the scancode exists, just replace by the new value - */ - rc = ir_seek_table(rc_tab, scancode); - if (rc >= 0) { - IR_dprintk(1, "#%d: Replacing scan 0x%04x with key 0x%04x\n", - rc, scancode, keycode); - - clear_bit(keymap[rc].keycode, dev->keybit); - - spin_lock_irqsave(&rc_tab->lock, flags); - keymap[rc].keycode = keycode; - spin_unlock_irqrestore(&rc_tab->lock, flags); - - set_bit(keycode, dev->keybit); - - return 0; + spin_lock_irqsave(&rc_tab->lock, flags); + start = 0; + end = rc_tab->len - 1; + while (start <= end) { + mid = (start + end) / 2; + if (rc_tab->scan[mid].scancode < scancode) + start = mid + 1; + else if (rc_tab->scan[mid].scancode > scancode) + end = mid - 1; + else { + key = rc_tab->scan[mid].keycode; + break; + } } + spin_unlock_irqrestore(&rc_tab->lock, flags); - /* - * Handle new scancode inserts - * - * reallocate table if needed and insert a new keycode - */ - - /* Avoid growing the table indefinitely */ - if (rc_tab->size + 1 > IR_TAB_MAX_SIZE) - return -EINVAL; - - rc = ir_insert_key(rc_tab, scancode, keycode); - if (rc < 0) - return rc; - set_bit(keycode, dev->keybit); - + *keycode = key; return 0; } @@ -369,24 +239,12 @@ */ u32 ir_g_keycode_from_table(struct input_dev *dev, u32 scancode) { - struct ir_input_dev *ir_dev = input_get_drvdata(dev); - struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; - struct ir_scancode *keymap = rc_tab->scan; - int elem; - - elem = ir_seek_table(rc_tab, scancode); - if (elem >= 0) { - IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n", - dev->name, scancode, keymap[elem].keycode); + int keycode; - return rc_tab->scan[elem].keycode; - } - - printk(KERN_INFO "%s: unknown key for scancode 0x%04x\n", - dev->name, scancode); - - /* Reports userspace that an unknown keycode were got */ - return KEY_RESERVED; + ir_getkeycode(dev, scancode, &keycode); + IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n", + dev->name, scancode, keycode); + return keycode; } EXPORT_SYMBOL_GPL(ir_g_keycode_from_table); @@ -476,8 +334,7 @@ const char *driver_name) { struct ir_input_dev *ir_dev; - struct ir_scancode *keymap = rc_tab->scan; - int i, rc; + int rc; if (rc_tab->scan == NULL || !rc_tab->size) return -EINVAL; @@ -486,55 +343,55 @@ if (!ir_dev) return -ENOMEM; - spin_lock_init(&ir_dev->rc_tab.lock); + ir_dev->driver_name = kasprintf(GFP_KERNEL, "%s", driver_name); + if (!ir_dev->driver_name) { + rc = -ENOMEM; + goto out_dev; + } - ir_dev->driver_name = kmalloc(strlen(driver_name) + 1, GFP_KERNEL); - if (!ir_dev->driver_name) - return -ENOMEM; - strcpy(ir_dev->driver_name, driver_name); + input_dev->getkeycode = ir_getkeycode; + input_dev->setkeycode = ir_setkeycode; + input_set_drvdata(input_dev, ir_dev); + + spin_lock_init(&ir_dev->rc_tab.lock); ir_dev->rc_tab.name = rc_tab->name; - ir_dev->rc_tab.size = ir_roundup_tablesize(rc_tab->size); - ir_dev->rc_tab.scan = kzalloc(ir_dev->rc_tab.size * - sizeof(struct ir_scancode), GFP_KERNEL); + ir_dev->rc_tab.ir_type = rc_tab->ir_type; + ir_dev->rc_tab.alloc = roundup_pow_of_two(rc_tab->len * + sizeof(struct ir_scancode)); + ir_dev->rc_tab.scan = kmalloc(ir_dev->rc_tab.alloc, GFP_KERNEL); + ir_dev->rc_tab.size = ir_dev->rc_tab.alloc / sizeof(struct ir_scancode); + if (!ir_dev->rc_tab.scan) { - kfree(ir_dev); - return -ENOMEM; + rc = -ENOMEM; + goto out_name; } - IR_dprintk(1, "Allocated space for %d keycode entries (%zd bytes)\n", - ir_dev->rc_tab.size, - ir_dev->rc_tab.size * sizeof(ir_dev->rc_tab.scan)); + IR_dprintk(1, "Allocated space for %u keycode entries (%u bytes)\n", + ir_dev->rc_tab.size, ir_dev->rc_tab.alloc); + + set_bit(EV_KEY, input_dev->evbit); + if (ir_setkeytable(input_dev, &ir_dev->rc_tab, rc_tab)) { + rc = -ENOMEM; + goto out_table; + } - ir_copy_table(&ir_dev->rc_tab, rc_tab); ir_dev->props = props; if (props && props->open) input_dev->open = ir_open; if (props && props->close) input_dev->close = ir_close; - /* set the bits for the keys */ - IR_dprintk(1, "key map size: %d\n", rc_tab->size); - for (i = 0; i < rc_tab->size; i++) { - IR_dprintk(1, "#%d: setting bit for keycode 0x%04x\n", - i, keymap[i].keycode); - set_bit(keymap[i].keycode, input_dev->keybit); - } - clear_bit(0, input_dev->keybit); - - set_bit(EV_KEY, input_dev->evbit); - - input_dev->getkeycode = ir_getkeycode; - input_dev->setkeycode = ir_setkeycode; - input_set_drvdata(input_dev, ir_dev); - rc = ir_register_class(input_dev); if (rc < 0) - goto err; + goto out_table; return 0; -err: - kfree(rc_tab->scan); +out_table: + kfree(ir_dev->rc_tab.scan); +out_name: + kfree(ir_dev->driver_name); +out_dev: kfree(ir_dev); return rc; } @@ -563,6 +420,7 @@ ir_unregister_class(dev); + kfree(ir_dev->driver_name); kfree(ir_dev); } EXPORT_SYMBOL_GPL(ir_input_unregister); Index: ir/include/media/ir-core.h =================================================================== --- ir.orig/include/media/ir-core.h 2010-04-01 17:38:16.733727686 +0200 +++ ir/include/media/ir-core.h 2010-04-02 12:22:50.836196555 +0200 @@ -46,7 +46,9 @@ struct ir_scancode_table { struct ir_scancode *scan; - int size; + unsigned int size; /* Max number of entries */ + unsigned int len; /* Used number of entries */ + unsigned int alloc; /* Size of *scan in bytes */ u64 ir_type; char *name; spinlock_t lock; -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 1/3] [PATCH] drivers/media/IR - improve keytable code @ 2010-04-02 18:58 ` david 0 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: improve-keytable-code.patch --] [-- Type: text/plain, Size: 21062 bytes --] The attached patch rewrites much of the keytable code in drivers/media/IR/ir-keytable.c. The scancodes are now inserted into the array in sorted order which allows for a binary search on lookup. The code has also been shrunk by about 150 lines. In addition it fixes the following bugs: Any use of ir_seek_table() was racy. ir_dev->driver_name is leaked between ir_input_register() and ir_input_unregister(). ir_setkeycode() unconditionally does clear_bit() on dev->keybit when removing a mapping, but there might be another mapping with a different scancode and the same keycode. This version has been updated to incorporate patch feedback from Mauro Carvalho Chehab. Signed-off-by: David Härdeman <david@hardeman.nu> Index: ir/drivers/media/IR/ir-keymaps.c =================================================================== --- ir.orig/drivers/media/IR/ir-keymaps.c 2010-04-01 17:38:16.777724334 +0200 +++ ir/drivers/media/IR/ir-keymaps.c 2010-04-02 12:23:14.952235145 +0200 @@ -39,6 +39,7 @@ struct ir_scancode_table tabname ## _table = { \ .scan = tabname, \ .size = ARRAY_SIZE(tabname), \ + .len = ARRAY_SIZE(tabname), \ .ir_type = type, \ .name = #irname, \ }; \ Index: ir/drivers/media/IR/ir-keytable.c =================================================================== --- ir.orig/drivers/media/IR/ir-keytable.c 2010-04-01 17:38:16.753724054 +0200 +++ ir/drivers/media/IR/ir-keytable.c 2010-04-02 12:32:13.015233729 +0200 @@ -16,344 +16,214 @@ #include <linux/input.h> #include <media/ir-common.h> -#define IR_TAB_MIN_SIZE 32 -#define IR_TAB_MAX_SIZE 1024 +/* Sizes are in bytes, 256 bytes allows for 32 entries on x64 */ +#define IR_TAB_MIN_SIZE 256 +#define IR_TAB_MAX_SIZE 8192 /** - * ir_seek_table() - returns the element order on the table - * @rc_tab: the ir_scancode_table with the keymap to be used - * @scancode: the scancode that we're seeking + * ir_resize_table() - resizes a scancode table if necessary + * @rc_tab: the ir_scancode_table to resize + * @return: zero on success or a negative error code * - * This routine is used by the input routines when a key is pressed at the - * IR. The scancode is received and needs to be converted into a keycode. - * If the key is not found, it returns KEY_UNKNOWN. Otherwise, returns the - * corresponding keycode from the table. + * This routine will shrink the ir_scancode_table if it has lots of + * unused entries and grow it if it is full. */ -static int ir_seek_table(struct ir_scancode_table *rc_tab, u32 scancode) +static int ir_resize_table(struct ir_scancode_table *rc_tab) { - int rc; - unsigned long flags; - struct ir_scancode *keymap = rc_tab->scan; - - spin_lock_irqsave(&rc_tab->lock, flags); - - /* FIXME: replace it by a binary search */ + unsigned int oldalloc = rc_tab->alloc; + unsigned int newalloc = oldalloc; + struct ir_scancode *oldscan = rc_tab->scan; + struct ir_scancode *newscan; - for (rc = 0; rc < rc_tab->size; rc++) - if (keymap[rc].scancode == scancode) - goto exit; - - /* Not found */ - rc = -EINVAL; + if (rc_tab->size == rc_tab->len) { + /* All entries in use -> grow keytable */ + if (rc_tab->alloc >= IR_TAB_MAX_SIZE) + return -ENOMEM; -exit: - spin_unlock_irqrestore(&rc_tab->lock, flags); - return rc; -} + newalloc *= 2; + IR_dprintk(1, "Growing table to %u bytes\n", newalloc); + } -/** - * ir_roundup_tablesize() - gets an optimum value for the table size - * @n_elems: minimum number of entries to store keycodes - * - * This routine is used to choose the keycode table size. - * - * In order to have some empty space for new keycodes, - * and knowing in advance that kmalloc allocates only power of two - * segments, it optimizes the allocated space to have some spare space - * for those new keycodes by using the maximum number of entries that - * will be effectively be allocated by kmalloc. - * In order to reduce the quantity of table resizes, it has a minimum - * table size of IR_TAB_MIN_SIZE. - */ -static int ir_roundup_tablesize(int n_elems) -{ - size_t size; + if ((rc_tab->len * 3 < rc_tab->size) && (oldalloc > IR_TAB_MIN_SIZE)) { + /* Less than 1/3 of entries in use -> shrink keytable */ + newalloc /= 2; + IR_dprintk(1, "Shrinking table to %u bytes\n", newalloc); + } - if (n_elems < IR_TAB_MIN_SIZE) - n_elems = IR_TAB_MIN_SIZE; + if (newalloc == oldalloc) + return 0; - /* - * As kmalloc only allocates sizes of power of two, get as - * much entries as possible for the allocated memory segment - */ - size = roundup_pow_of_two(n_elems * sizeof(struct ir_scancode)); - n_elems = size / sizeof(struct ir_scancode); + newscan = kmalloc(newalloc, GFP_ATOMIC); + if (!newscan) { + IR_dprintk(1, "Failed to kmalloc %u bytes\n", newalloc); + return -ENOMEM; + } - return n_elems; + memcpy(newscan, rc_tab->scan, rc_tab->len * sizeof(struct ir_scancode)); + rc_tab->scan = newscan; + rc_tab->alloc = newalloc; + rc_tab->size = rc_tab->alloc / sizeof(struct ir_scancode); + kfree(oldscan); + return 0; } /** - * ir_copy_table() - copies a keytable, discarding the unused entries - * @destin: destin table - * @origin: origin table - * - * Copies all entries where the keycode is not KEY_UNKNOWN/KEY_RESERVED - * Also copies table size and table protocol. - * NOTE: It shouldn't copy the lock field - */ - -static int ir_copy_table(struct ir_scancode_table *destin, - const struct ir_scancode_table *origin) -{ - int i, j = 0; - - for (i = 0; i < origin->size; i++) { - if (origin->scan[i].keycode == KEY_UNKNOWN || - origin->scan[i].keycode == KEY_RESERVED) + * ir_do_setkeycode() - internal function to set a keycode in the + * scancode->keycode table + * @dev: the struct input_dev device descriptor + * @rc_tab: the struct ir_scancode_table to set the keycode in + * @scancode: the scancode for the ir command + * @keycode: the keycode for the ir command + * @return: -EINVAL if the keycode could not be inserted, otherwise zero. + * + * This routine is used internally to manipulate the scancode->keycode table. + * The caller has to hold @rc_tab->lock. + */ +static int ir_do_setkeycode(struct input_dev *dev, + struct ir_scancode_table *rc_tab, + int scancode, int keycode) +{ + unsigned int i; + int old_keycode = KEY_RESERVED; + + /* First check if we already have a mapping for this ir command */ + for (i = 0; i < rc_tab->len; i++) { + /* Keytable is sorted from lowest to highest scancode */ + if (rc_tab->scan[i].scancode > scancode) + break; + else if (rc_tab->scan[i].scancode < scancode) continue; - memcpy(&destin->scan[j], &origin->scan[i], sizeof(struct ir_scancode)); - j++; + old_keycode = rc_tab->scan[i].keycode; + rc_tab->scan[i].keycode = keycode; + + /* Did the user wish to remove the mapping? */ + if (keycode == KEY_RESERVED || keycode == KEY_UNKNOWN) { + rc_tab->len--; + memmove(&rc_tab->scan[i], &rc_tab->scan[i + 1], + (rc_tab->len - i) * sizeof(struct ir_scancode)); + } + + /* Possibly shrink the keytable, failure is not a problem */ + ir_resize_table(rc_tab); + break; } - destin->size = j; - destin->ir_type = origin->ir_type; - IR_dprintk(1, "Copied %d scancodes to the new keycode table\n", destin->size); + if (old_keycode == KEY_RESERVED) { + /* No previous mapping found, we might need to grow the table */ + if (ir_resize_table(rc_tab)) + return -ENOMEM; + + /* i is the proper index to insert our new keycode */ + memmove(&rc_tab->scan[i + 1], &rc_tab->scan[i], + (rc_tab->len - i) * sizeof(struct ir_scancode)); + rc_tab->scan[i].scancode = scancode; + rc_tab->scan[i].keycode = keycode; + rc_tab->len++; + set_bit(keycode, dev->keybit); + } else { + /* A previous mapping was updated... */ + clear_bit(old_keycode, dev->keybit); + /* ...but another scancode might use the same keycode */ + for (i = 0; i < rc_tab->len; i++) { + if (rc_tab->scan[i].keycode == old_keycode) { + set_bit(old_keycode, dev->keybit); + break; + } + } + } return 0; } /** - * ir_getkeycode() - get a keycode at the evdev scancode ->keycode table + * ir_setkeycode() - set a keycode in the scancode->keycode table * @dev: the struct input_dev device descriptor * @scancode: the desired scancode - * @keycode: the keycode to be retorned. + * @keycode: result + * @return: -EINVAL if the keycode could not be inserted, otherwise zero. * - * This routine is used to handle evdev EVIOCGKEY ioctl. - * If the key is not found, returns -EINVAL, otherwise, returns 0. + * This routine is used to handle evdev EVIOCSKEY ioctl. */ -static int ir_getkeycode(struct input_dev *dev, - int scancode, int *keycode) +static int ir_setkeycode(struct input_dev *dev, + int scancode, int keycode) { - int elem; + int rc; + unsigned long flags; struct ir_input_dev *ir_dev = input_get_drvdata(dev); struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; - elem = ir_seek_table(rc_tab, scancode); - if (elem >= 0) { - *keycode = rc_tab->scan[elem].keycode; - return 0; - } - - /* - * Scancode not found and table can't be expanded - */ - if (elem < 0 && rc_tab->size == IR_TAB_MAX_SIZE) - return -EINVAL; - - /* - * If is there extra space, returns KEY_RESERVED, - * otherwise, input core won't let ir_setkeycode to work - */ - *keycode = KEY_RESERVED; - return 0; -} - -/** - * ir_is_resize_needed() - Check if the table needs rezise - * @table: keycode table that may need to resize - * @n_elems: minimum number of entries to store keycodes - * - * Considering that kmalloc uses power of two storage areas, this - * routine detects if the real alloced size will change. If not, it - * just returns without doing nothing. Otherwise, it will extend or - * reduce the table size to meet the new needs. - * - * It returns 0 if no resize is needed, 1 otherwise. - */ -static int ir_is_resize_needed(struct ir_scancode_table *table, int n_elems) -{ - int cur_size = ir_roundup_tablesize(table->size); - int new_size = ir_roundup_tablesize(n_elems); - - if (cur_size == new_size) - return 0; - - /* Resize is needed */ - return 1; -} - -/** - * ir_delete_key() - remove a keycode from the table - * @rc_tab: keycode table - * @elem: element to be removed - * - */ -static void ir_delete_key(struct ir_scancode_table *rc_tab, int elem) -{ - unsigned long flags = 0; - int newsize = rc_tab->size - 1; - int resize = ir_is_resize_needed(rc_tab, newsize); - struct ir_scancode *oldkeymap = rc_tab->scan; - struct ir_scancode *newkeymap = NULL; - - if (resize) - newkeymap = kzalloc(ir_roundup_tablesize(newsize) * - sizeof(*newkeymap), GFP_ATOMIC); - - /* There's no memory for resize. Keep the old table */ - if (!resize || !newkeymap) { - newkeymap = oldkeymap; - - /* We'll modify the live table. Lock it */ - spin_lock_irqsave(&rc_tab->lock, flags); - } - - /* - * Copy the elements before the one that will be deleted - * if (!resize), both oldkeymap and newkeymap points - * to the same place, so, there's no need to copy - */ - if (resize && elem > 0) - memcpy(newkeymap, oldkeymap, - elem * sizeof(*newkeymap)); - - /* - * Copy the other elements overwriting the element to be removed - * This operation applies to both resize and non-resize case - */ - if (elem < newsize) - memcpy(&newkeymap[elem], &oldkeymap[elem + 1], - (newsize - elem) * sizeof(*newkeymap)); - - if (resize) { - /* - * As the copy happened to a temporary table, only here - * it needs to lock while replacing the table pointers - * to use the new table - */ - spin_lock_irqsave(&rc_tab->lock, flags); - rc_tab->size = newsize; - rc_tab->scan = newkeymap; - spin_unlock_irqrestore(&rc_tab->lock, flags); - - /* Frees the old keytable */ - kfree(oldkeymap); - } else { - rc_tab->size = newsize; - spin_unlock_irqrestore(&rc_tab->lock, flags); - } + spin_lock_irqsave(&rc_tab->lock, flags); + rc = ir_do_setkeycode(dev, rc_tab, scancode, keycode); + spin_unlock_irqrestore(&rc_tab->lock, flags); + return rc; } /** - * ir_insert_key() - insert a keycode at the table - * @rc_tab: keycode table - * @scancode: the desired scancode - * @keycode: the keycode to be retorned. - * - */ -static int ir_insert_key(struct ir_scancode_table *rc_tab, - int scancode, int keycode) + * ir_setkeytable() - sets several entries in the scancode->keycode table + * @dev: the struct input_dev device descriptor + * @to: the struct ir_scancode_table to copy entries to + * @from: the struct ir_scancode_table to copy entries from + * @return: -EINVAL if all keycodes could not be inserted, otherwise zero. + * + * This routine is used to handle table initialization. + */ +static int ir_setkeytable(struct input_dev *dev, + struct ir_scancode_table *to, + const struct ir_scancode_table *from) { + struct ir_input_dev *ir_dev = input_get_drvdata(dev); + struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; unsigned long flags; - int elem = rc_tab->size; - int newsize = rc_tab->size + 1; - int resize = ir_is_resize_needed(rc_tab, newsize); - struct ir_scancode *oldkeymap = rc_tab->scan; - struct ir_scancode *newkeymap; - - if (resize) { - newkeymap = kzalloc(ir_roundup_tablesize(newsize) * - sizeof(*newkeymap), GFP_ATOMIC); - if (!newkeymap) - return -ENOMEM; - - memcpy(newkeymap, oldkeymap, - rc_tab->size * sizeof(*newkeymap)); - } else - newkeymap = oldkeymap; - - /* Stores the new code at the table */ - IR_dprintk(1, "#%d: New scan 0x%04x with key 0x%04x\n", - rc_tab->size, scancode, keycode); + unsigned int i; + int rc = 0; spin_lock_irqsave(&rc_tab->lock, flags); - rc_tab->size = newsize; - if (resize) { - rc_tab->scan = newkeymap; - kfree(oldkeymap); + for (i = 0; i < from->len; i++) { + rc = ir_do_setkeycode(dev, to, from->scan[i].scancode, + from->scan[i].keycode); + if (rc) + break; } - newkeymap[elem].scancode = scancode; - newkeymap[elem].keycode = keycode; spin_unlock_irqrestore(&rc_tab->lock, flags); - - return 0; + return rc; } /** - * ir_setkeycode() - set a keycode at the evdev scancode ->keycode table + * ir_getkeycode() - get a keycode from the scancode->keycode table * @dev: the struct input_dev device descriptor * @scancode: the desired scancode - * @keycode: the keycode to be retorned. + * @keycode: used to return the keycode, if found, or KEY_RESERVED + * @return: always returns zero. * - * This routine is used to handle evdev EVIOCSKEY ioctl. - * There's one caveat here: how can we increase the size of the table? - * If the key is not found, returns -EINVAL, otherwise, returns 0. + * This routine is used to handle evdev EVIOCGKEY ioctl. */ -static int ir_setkeycode(struct input_dev *dev, - int scancode, int keycode) +static int ir_getkeycode(struct input_dev *dev, + int scancode, int *keycode) { - int rc = 0; + int start, end, mid; + unsigned long flags; + int key = KEY_RESERVED; struct ir_input_dev *ir_dev = input_get_drvdata(dev); struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; - struct ir_scancode *keymap = rc_tab->scan; - unsigned long flags; - - /* - * Handle keycode table deletions - * - * If userspace is adding a KEY_UNKNOWN or KEY_RESERVED, - * deal as a trial to remove an existing scancode attribution - * if table become too big, reduce it to save space - */ - if (keycode == KEY_UNKNOWN || keycode == KEY_RESERVED) { - rc = ir_seek_table(rc_tab, scancode); - if (rc < 0) - return 0; - - IR_dprintk(1, "#%d: Deleting scan 0x%04x\n", rc, scancode); - clear_bit(keymap[rc].keycode, dev->keybit); - ir_delete_key(rc_tab, rc); - return 0; - } - - /* - * Handle keycode replacements - * - * If the scancode exists, just replace by the new value - */ - rc = ir_seek_table(rc_tab, scancode); - if (rc >= 0) { - IR_dprintk(1, "#%d: Replacing scan 0x%04x with key 0x%04x\n", - rc, scancode, keycode); - - clear_bit(keymap[rc].keycode, dev->keybit); - - spin_lock_irqsave(&rc_tab->lock, flags); - keymap[rc].keycode = keycode; - spin_unlock_irqrestore(&rc_tab->lock, flags); - - set_bit(keycode, dev->keybit); - - return 0; + spin_lock_irqsave(&rc_tab->lock, flags); + start = 0; + end = rc_tab->len - 1; + while (start <= end) { + mid = (start + end) / 2; + if (rc_tab->scan[mid].scancode < scancode) + start = mid + 1; + else if (rc_tab->scan[mid].scancode > scancode) + end = mid - 1; + else { + key = rc_tab->scan[mid].keycode; + break; + } } + spin_unlock_irqrestore(&rc_tab->lock, flags); - /* - * Handle new scancode inserts - * - * reallocate table if needed and insert a new keycode - */ - - /* Avoid growing the table indefinitely */ - if (rc_tab->size + 1 > IR_TAB_MAX_SIZE) - return -EINVAL; - - rc = ir_insert_key(rc_tab, scancode, keycode); - if (rc < 0) - return rc; - set_bit(keycode, dev->keybit); - + *keycode = key; return 0; } @@ -369,24 +239,12 @@ */ u32 ir_g_keycode_from_table(struct input_dev *dev, u32 scancode) { - struct ir_input_dev *ir_dev = input_get_drvdata(dev); - struct ir_scancode_table *rc_tab = &ir_dev->rc_tab; - struct ir_scancode *keymap = rc_tab->scan; - int elem; - - elem = ir_seek_table(rc_tab, scancode); - if (elem >= 0) { - IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n", - dev->name, scancode, keymap[elem].keycode); + int keycode; - return rc_tab->scan[elem].keycode; - } - - printk(KERN_INFO "%s: unknown key for scancode 0x%04x\n", - dev->name, scancode); - - /* Reports userspace that an unknown keycode were got */ - return KEY_RESERVED; + ir_getkeycode(dev, scancode, &keycode); + IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n", + dev->name, scancode, keycode); + return keycode; } EXPORT_SYMBOL_GPL(ir_g_keycode_from_table); @@ -476,8 +334,7 @@ const char *driver_name) { struct ir_input_dev *ir_dev; - struct ir_scancode *keymap = rc_tab->scan; - int i, rc; + int rc; if (rc_tab->scan == NULL || !rc_tab->size) return -EINVAL; @@ -486,55 +343,55 @@ if (!ir_dev) return -ENOMEM; - spin_lock_init(&ir_dev->rc_tab.lock); + ir_dev->driver_name = kasprintf(GFP_KERNEL, "%s", driver_name); + if (!ir_dev->driver_name) { + rc = -ENOMEM; + goto out_dev; + } - ir_dev->driver_name = kmalloc(strlen(driver_name) + 1, GFP_KERNEL); - if (!ir_dev->driver_name) - return -ENOMEM; - strcpy(ir_dev->driver_name, driver_name); + input_dev->getkeycode = ir_getkeycode; + input_dev->setkeycode = ir_setkeycode; + input_set_drvdata(input_dev, ir_dev); + + spin_lock_init(&ir_dev->rc_tab.lock); ir_dev->rc_tab.name = rc_tab->name; - ir_dev->rc_tab.size = ir_roundup_tablesize(rc_tab->size); - ir_dev->rc_tab.scan = kzalloc(ir_dev->rc_tab.size * - sizeof(struct ir_scancode), GFP_KERNEL); + ir_dev->rc_tab.ir_type = rc_tab->ir_type; + ir_dev->rc_tab.alloc = roundup_pow_of_two(rc_tab->len * + sizeof(struct ir_scancode)); + ir_dev->rc_tab.scan = kmalloc(ir_dev->rc_tab.alloc, GFP_KERNEL); + ir_dev->rc_tab.size = ir_dev->rc_tab.alloc / sizeof(struct ir_scancode); + if (!ir_dev->rc_tab.scan) { - kfree(ir_dev); - return -ENOMEM; + rc = -ENOMEM; + goto out_name; } - IR_dprintk(1, "Allocated space for %d keycode entries (%zd bytes)\n", - ir_dev->rc_tab.size, - ir_dev->rc_tab.size * sizeof(ir_dev->rc_tab.scan)); + IR_dprintk(1, "Allocated space for %u keycode entries (%u bytes)\n", + ir_dev->rc_tab.size, ir_dev->rc_tab.alloc); + + set_bit(EV_KEY, input_dev->evbit); + if (ir_setkeytable(input_dev, &ir_dev->rc_tab, rc_tab)) { + rc = -ENOMEM; + goto out_table; + } - ir_copy_table(&ir_dev->rc_tab, rc_tab); ir_dev->props = props; if (props && props->open) input_dev->open = ir_open; if (props && props->close) input_dev->close = ir_close; - /* set the bits for the keys */ - IR_dprintk(1, "key map size: %d\n", rc_tab->size); - for (i = 0; i < rc_tab->size; i++) { - IR_dprintk(1, "#%d: setting bit for keycode 0x%04x\n", - i, keymap[i].keycode); - set_bit(keymap[i].keycode, input_dev->keybit); - } - clear_bit(0, input_dev->keybit); - - set_bit(EV_KEY, input_dev->evbit); - - input_dev->getkeycode = ir_getkeycode; - input_dev->setkeycode = ir_setkeycode; - input_set_drvdata(input_dev, ir_dev); - rc = ir_register_class(input_dev); if (rc < 0) - goto err; + goto out_table; return 0; -err: - kfree(rc_tab->scan); +out_table: + kfree(ir_dev->rc_tab.scan); +out_name: + kfree(ir_dev->driver_name); +out_dev: kfree(ir_dev); return rc; } @@ -563,6 +420,7 @@ ir_unregister_class(dev); + kfree(ir_dev->driver_name); kfree(ir_dev); } EXPORT_SYMBOL_GPL(ir_input_unregister); Index: ir/include/media/ir-core.h =================================================================== --- ir.orig/include/media/ir-core.h 2010-04-01 17:38:16.733727686 +0200 +++ ir/include/media/ir-core.h 2010-04-02 12:22:50.836196555 +0200 @@ -46,7 +46,9 @@ struct ir_scancode_table { struct ir_scancode *scan; - int size; + unsigned int size; /* Max number of entries */ + unsigned int len; /* Used number of entries */ + unsigned int alloc; /* Size of *scan in bytes */ u64 ir_type; char *name; spinlock_t lock; ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 2/3] [PATCH] drivers/media/IR - improve keyup/keydown logic 2010-04-02 18:58 ` david @ 2010-04-02 18:58 ` david -1 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Attachment #1: improve-keyup-keydown-logic.patch --] [-- Type: TEXT/PLAIN, Size: 11409 bytes --] The attached patch rewrites the keyup/keydown logic in drivers/media/IR/ir-keytable.c. (applies to http://git.linuxtv.org/mchehab/ir.git) All knowledge of keystates etc is now internal to ir-keytable.c and not scattered around ir-raw-event.c and ir-nec-decoder.c (where it doesn't belong). In addition, I've changed the API slightly so that ir_input_dev is passed as the first argument rather than input_dev. If we're ever going to support multiple keytables we need to move towards making ir_input_dev the main interface from a driver POV and obscure away the input_dev as an implementational detail in ir-core. Mauro, once this patch is merged I'll start working on patches to migrate drivers which use ir_input_* in ir-functions.c over to the ir-keytable.c code instead. Changes since last version: Clarify a comment Refreshed to apply on top of previous patch Let the new functions take input_dev as their main argument (as there's no agreement on this point yet) Signed-off-by: David Härdeman <david@hardeman.nu> Index: ir/drivers/media/IR/ir-keytable.c =================================================================== --- ir.orig/drivers/media/IR/ir-keytable.c 2010-04-02 12:32:13.015233729 +0200 +++ ir/drivers/media/IR/ir-keytable.c 2010-04-02 16:40:37.908276737 +0200 @@ -20,6 +20,9 @@ #define IR_TAB_MIN_SIZE 256 #define IR_TAB_MAX_SIZE 8192 +/* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */ +#define IR_KEYPRESS_TIMEOUT 250 + /** * ir_resize_table() - resizes a scancode table if necessary * @rc_tab: the ir_scancode_table to resize @@ -250,56 +253,124 @@ /** * ir_keyup() - generates input event to cleanup a key press - * @input_dev: the struct input_dev descriptor of the device + * @ir: the struct ir_input_dev descriptor of the device * - * This routine is used by the input routines when a key is pressed at the - * IR. It reports a keyup input event via input_report_key(). + * This routine is used to signal that a key has been released on the + * remote control. It reports a keyup input event via input_report_key(). */ -void ir_keyup(struct input_dev *dev) +static void ir_keyup(struct ir_input_dev *ir) { - struct ir_input_dev *ir = input_get_drvdata(dev); - if (!ir->keypressed) return; - IR_dprintk(1, "keyup key 0x%04x\n", ir->keycode); - input_report_key(dev, ir->keycode, 0); - input_sync(dev); - ir->keypressed = 0; + IR_dprintk(1, "keyup key 0x%04x\n", ir->last_keycode); + input_report_key(ir->input_dev, ir->last_keycode, 0); + input_sync(ir->input_dev); + ir->keypressed = false; +} + +/** + * ir_timer_keyup() - generates a keyup event after a timeout + * @cookie: a pointer to struct ir_input_dev passed to setup_timer() + * + * This routine will generate a keyup event some time after a keydown event + * is generated when no further activity has been detected. + */ +static void ir_timer_keyup(unsigned long cookie) +{ + struct ir_input_dev *ir = (struct ir_input_dev *)cookie; + unsigned long flags; + + /* + * ir->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. + * + * The timer will then be reactivated and this function called + * again in the future. We need to exit gracefully in that case + * to allow the input subsystem to do its auto-repeat magic or + * a keyup event might follow immediately after the keydown. + */ + spin_lock_irqsave(&ir->keylock, flags); + if (time_is_after_eq_jiffies(ir->keyup_jiffies)) + ir_keyup(ir); + spin_unlock_irqrestore(&ir->keylock, flags); +} + +/** + * ir_repeat() - notifies the IR core that a key is still pressed + * @dev: the struct input_dev descriptor of the device + * + * This routine is used by IR decoders when a repeat message which does + * not include the necessary bits to reproduce the scancode has been + * received. + */ +void ir_repeat(struct input_dev *dev) +{ + unsigned long flags; + struct ir_input_dev *ir = input_get_drvdata(dev); + + spin_lock_irqsave(&ir->keylock, flags); + + if (!ir->keypressed) + goto out; + + ir->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT); + mod_timer(&ir->timer_keyup, ir->keyup_jiffies); + +out: + spin_unlock_irqrestore(&ir->keylock, flags); } -EXPORT_SYMBOL_GPL(ir_keyup); +EXPORT_SYMBOL_GPL(ir_repeat); /** * ir_keydown() - generates input event for a key press - * @input_dev: the struct input_dev descriptor of the device + * @dev: the struct input_dev descriptor of the device * @scancode: the scancode that we're seeking + * @toggle: the toggle value (protocol dependent, if the protocol doesn't + * support toggle values, this should be set to zero) * * This routine is used by the input routines when a key is pressed at the * IR. It gets the keycode for a scancode and reports an input event via * input_report_key(). */ -void ir_keydown(struct input_dev *dev, int scancode) +void ir_keydown(struct input_dev *dev, int scancode, u8 toggle) { + unsigned long flags; struct ir_input_dev *ir = input_get_drvdata(dev); u32 keycode = ir_g_keycode_from_table(dev, scancode); - /* If already sent a keydown, do a keyup */ - if (ir->keypressed) - ir_keyup(dev); + spin_lock_irqsave(&ir->keylock, flags); - if (KEY_RESERVED == keycode) - return; + /* Repeat event? */ + if (ir->keypressed && + ir->last_scancode == scancode && + ir->last_toggle == toggle) + goto set_timer; + + /* Release old keypress */ + ir_keyup(ir); + + ir->last_scancode = scancode; + ir->last_toggle = toggle; + ir->last_keycode = keycode; - ir->keycode = keycode; - ir->keypressed = 1; + if (keycode == KEY_RESERVED) + goto out; + /* Register a keypress */ + ir->keypressed = true; IR_dprintk(1, "%s: key down event, key 0x%04x, scancode 0x%04x\n", - dev->name, keycode, scancode); - - input_report_key(dev, ir->keycode, 1); + dev->name, keycode, scancode); + input_report_key(dev, ir->last_keycode, 1); input_sync(dev); - + +set_timer: + ir->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT); + mod_timer(&ir->timer_keyup, ir->keyup_jiffies); +out: + spin_unlock_irqrestore(&ir->keylock, flags); } EXPORT_SYMBOL_GPL(ir_keydown); @@ -352,8 +423,12 @@ input_dev->getkeycode = ir_getkeycode; input_dev->setkeycode = ir_setkeycode; input_set_drvdata(input_dev, ir_dev); + ir_dev->input_dev = input_dev; spin_lock_init(&ir_dev->rc_tab.lock); + spin_lock_init(&ir_dev->keylock); + setup_timer(&ir_dev->timer_keyup, ir_timer_keyup, (unsigned long)ir_dev); + ir_dev->rc_tab.name = rc_tab->name; ir_dev->rc_tab.ir_type = rc_tab->ir_type; ir_dev->rc_tab.alloc = roundup_pow_of_two(rc_tab->len * @@ -370,6 +445,8 @@ ir_dev->rc_tab.size, ir_dev->rc_tab.alloc); set_bit(EV_KEY, input_dev->evbit); + set_bit(EV_REP, input_dev->evbit); + if (ir_setkeytable(input_dev, &ir_dev->rc_tab, rc_tab)) { rc = -ENOMEM; goto out_table; @@ -412,7 +489,7 @@ return; IR_dprintk(1, "Freed keycode table\n"); - + del_timer_sync(&ir_dev->timer_keyup); rc_tab = &ir_dev->rc_tab; rc_tab->size = 0; kfree(rc_tab->scan); Index: ir/drivers/media/IR/ir-nec-decoder.c =================================================================== --- ir.orig/drivers/media/IR/ir-nec-decoder.c 2010-04-02 12:22:50.872195298 +0200 +++ ir/drivers/media/IR/ir-nec-decoder.c 2010-04-02 16:34:32.531192323 +0200 @@ -179,8 +179,7 @@ if (is_repeat(evs, len, *pos)) { *pos += 2; if (ir->keypressed) { - mod_timer(&ir->raw->timer_keyup, - jiffies + msecs_to_jiffies(REPEAT_TIME)); + ir_repeat(input_dev); IR_dprintk(1, "NEC repeat event\n"); return 1; } else { @@ -237,9 +236,7 @@ } IR_dprintk(1, "NEC scancode 0x%04x\n", ircode); - ir_keydown(input_dev, ircode); - mod_timer(&ir->raw->timer_keyup, - jiffies + msecs_to_jiffies(REPEAT_TIME)); + ir_keydown(input_dev, ircode, 0); return 1; err: Index: ir/drivers/media/IR/ir-raw-event.c =================================================================== --- ir.orig/drivers/media/IR/ir-raw-event.c 2010-04-02 12:22:50.856216460 +0200 +++ ir/drivers/media/IR/ir-raw-event.c 2010-04-02 12:34:05.395187844 +0200 @@ -53,13 +53,6 @@ /* Used to load the decoders */ static struct work_struct wq_load; -static void ir_keyup_timer(unsigned long data) -{ - struct input_dev *input_dev = (struct input_dev *)data; - - ir_keyup(input_dev); -} - int ir_raw_event_register(struct input_dev *input_dev) { struct ir_input_dev *ir = input_get_drvdata(input_dev); @@ -72,11 +65,6 @@ size = sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE * 2; size = roundup_pow_of_two(size); - init_timer(&ir->raw->timer_keyup); - ir->raw->timer_keyup.function = ir_keyup_timer; - ir->raw->timer_keyup.data = (unsigned long)input_dev; - set_bit(EV_REP, input_dev->evbit); - rc = kfifo_alloc(&ir->raw->kfifo, size, GFP_KERNEL); if (rc < 0) { kfree (ir->raw); @@ -103,8 +91,6 @@ if (!ir->raw) return; - del_timer_sync(&ir->raw->timer_keyup); - RUN_DECODER(raw_unregister, input_dev); kfifo_free(&ir->raw->kfifo); Index: ir/include/media/ir-core.h =================================================================== --- ir.orig/include/media/ir-core.h 2010-04-02 12:22:50.836196555 +0200 +++ ir/include/media/ir-core.h 2010-04-02 16:23:29.783236946 +0200 @@ -70,7 +70,6 @@ struct ir_raw_event_ctrl { struct kfifo kfifo; /* fifo for the pulse/space events */ struct timespec last_event; /* when last event occurred */ - struct timer_list timer_keyup; /* timer for key release */ }; struct ir_input_dev { @@ -80,10 +79,16 @@ unsigned long devno; /* device number */ const struct ir_dev_props *props; /* Device properties */ struct ir_raw_event_ctrl *raw; /* for raw pulse/space events */ + struct input_dev *input_dev; /* the input device associated with this device */ /* key info - needed by IR keycode handlers */ - u32 keycode; /* linux key code */ - int keypressed; /* current state */ + spinlock_t keylock; /* protects the below members */ + bool keypressed; /* current state */ + unsigned long keyup_jiffies; /* when should the current keypress be released? */ + struct timer_list timer_keyup; /* timer for releasing a keypress */ + u32 last_keycode; /* keycode of last command */ + u32 last_scancode; /* scancode of last command */ + u8 last_toggle; /* toggle of last command */ }; struct ir_raw_handler { @@ -102,8 +107,8 @@ u32 ir_g_keycode_from_table(struct input_dev *input_dev, u32 scancode); -void ir_keyup(struct input_dev *dev); -void ir_keydown(struct input_dev *dev, int scancode); +void ir_repeat(struct input_dev *dev); +void ir_keydown(struct input_dev *dev, int scancode, u8 toggle); int ir_input_register(struct input_dev *dev, const struct ir_scancode_table *ir_codes, const struct ir_dev_props *props, -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 2/3] [PATCH] drivers/media/IR - improve keyup/keydown logic @ 2010-04-02 18:58 ` david 0 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: improve-keyup-keydown-logic.patch --] [-- Type: text/plain, Size: 10882 bytes --] The attached patch rewrites the keyup/keydown logic in drivers/media/IR/ir-keytable.c. (applies to http://git.linuxtv.org/mchehab/ir.git) All knowledge of keystates etc is now internal to ir-keytable.c and not scattered around ir-raw-event.c and ir-nec-decoder.c (where it doesn't belong). In addition, I've changed the API slightly so that ir_input_dev is passed as the first argument rather than input_dev. If we're ever going to support multiple keytables we need to move towards making ir_input_dev the main interface from a driver POV and obscure away the input_dev as an implementational detail in ir-core. Mauro, once this patch is merged I'll start working on patches to migrate drivers which use ir_input_* in ir-functions.c over to the ir-keytable.c code instead. Changes since last version: Clarify a comment Refreshed to apply on top of previous patch Let the new functions take input_dev as their main argument (as there's no agreement on this point yet) Signed-off-by: David Härdeman <david@hardeman.nu> Index: ir/drivers/media/IR/ir-keytable.c =================================================================== --- ir.orig/drivers/media/IR/ir-keytable.c 2010-04-02 12:32:13.015233729 +0200 +++ ir/drivers/media/IR/ir-keytable.c 2010-04-02 16:40:37.908276737 +0200 @@ -20,6 +20,9 @@ #define IR_TAB_MIN_SIZE 256 #define IR_TAB_MAX_SIZE 8192 +/* FIXME: IR_KEYPRESS_TIMEOUT should be protocol specific */ +#define IR_KEYPRESS_TIMEOUT 250 + /** * ir_resize_table() - resizes a scancode table if necessary * @rc_tab: the ir_scancode_table to resize @@ -250,56 +253,124 @@ /** * ir_keyup() - generates input event to cleanup a key press - * @input_dev: the struct input_dev descriptor of the device + * @ir: the struct ir_input_dev descriptor of the device * - * This routine is used by the input routines when a key is pressed at the - * IR. It reports a keyup input event via input_report_key(). + * This routine is used to signal that a key has been released on the + * remote control. It reports a keyup input event via input_report_key(). */ -void ir_keyup(struct input_dev *dev) +static void ir_keyup(struct ir_input_dev *ir) { - struct ir_input_dev *ir = input_get_drvdata(dev); - if (!ir->keypressed) return; - IR_dprintk(1, "keyup key 0x%04x\n", ir->keycode); - input_report_key(dev, ir->keycode, 0); - input_sync(dev); - ir->keypressed = 0; + IR_dprintk(1, "keyup key 0x%04x\n", ir->last_keycode); + input_report_key(ir->input_dev, ir->last_keycode, 0); + input_sync(ir->input_dev); + ir->keypressed = false; +} + +/** + * ir_timer_keyup() - generates a keyup event after a timeout + * @cookie: a pointer to struct ir_input_dev passed to setup_timer() + * + * This routine will generate a keyup event some time after a keydown event + * is generated when no further activity has been detected. + */ +static void ir_timer_keyup(unsigned long cookie) +{ + struct ir_input_dev *ir = (struct ir_input_dev *)cookie; + unsigned long flags; + + /* + * ir->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. + * + * The timer will then be reactivated and this function called + * again in the future. We need to exit gracefully in that case + * to allow the input subsystem to do its auto-repeat magic or + * a keyup event might follow immediately after the keydown. + */ + spin_lock_irqsave(&ir->keylock, flags); + if (time_is_after_eq_jiffies(ir->keyup_jiffies)) + ir_keyup(ir); + spin_unlock_irqrestore(&ir->keylock, flags); +} + +/** + * ir_repeat() - notifies the IR core that a key is still pressed + * @dev: the struct input_dev descriptor of the device + * + * This routine is used by IR decoders when a repeat message which does + * not include the necessary bits to reproduce the scancode has been + * received. + */ +void ir_repeat(struct input_dev *dev) +{ + unsigned long flags; + struct ir_input_dev *ir = input_get_drvdata(dev); + + spin_lock_irqsave(&ir->keylock, flags); + + if (!ir->keypressed) + goto out; + + ir->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT); + mod_timer(&ir->timer_keyup, ir->keyup_jiffies); + +out: + spin_unlock_irqrestore(&ir->keylock, flags); } -EXPORT_SYMBOL_GPL(ir_keyup); +EXPORT_SYMBOL_GPL(ir_repeat); /** * ir_keydown() - generates input event for a key press - * @input_dev: the struct input_dev descriptor of the device + * @dev: the struct input_dev descriptor of the device * @scancode: the scancode that we're seeking + * @toggle: the toggle value (protocol dependent, if the protocol doesn't + * support toggle values, this should be set to zero) * * This routine is used by the input routines when a key is pressed at the * IR. It gets the keycode for a scancode and reports an input event via * input_report_key(). */ -void ir_keydown(struct input_dev *dev, int scancode) +void ir_keydown(struct input_dev *dev, int scancode, u8 toggle) { + unsigned long flags; struct ir_input_dev *ir = input_get_drvdata(dev); u32 keycode = ir_g_keycode_from_table(dev, scancode); - /* If already sent a keydown, do a keyup */ - if (ir->keypressed) - ir_keyup(dev); + spin_lock_irqsave(&ir->keylock, flags); - if (KEY_RESERVED == keycode) - return; + /* Repeat event? */ + if (ir->keypressed && + ir->last_scancode == scancode && + ir->last_toggle == toggle) + goto set_timer; + + /* Release old keypress */ + ir_keyup(ir); + + ir->last_scancode = scancode; + ir->last_toggle = toggle; + ir->last_keycode = keycode; - ir->keycode = keycode; - ir->keypressed = 1; + if (keycode == KEY_RESERVED) + goto out; + /* Register a keypress */ + ir->keypressed = true; IR_dprintk(1, "%s: key down event, key 0x%04x, scancode 0x%04x\n", - dev->name, keycode, scancode); - - input_report_key(dev, ir->keycode, 1); + dev->name, keycode, scancode); + input_report_key(dev, ir->last_keycode, 1); input_sync(dev); - + +set_timer: + ir->keyup_jiffies = jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT); + mod_timer(&ir->timer_keyup, ir->keyup_jiffies); +out: + spin_unlock_irqrestore(&ir->keylock, flags); } EXPORT_SYMBOL_GPL(ir_keydown); @@ -352,8 +423,12 @@ input_dev->getkeycode = ir_getkeycode; input_dev->setkeycode = ir_setkeycode; input_set_drvdata(input_dev, ir_dev); + ir_dev->input_dev = input_dev; spin_lock_init(&ir_dev->rc_tab.lock); + spin_lock_init(&ir_dev->keylock); + setup_timer(&ir_dev->timer_keyup, ir_timer_keyup, (unsigned long)ir_dev); + ir_dev->rc_tab.name = rc_tab->name; ir_dev->rc_tab.ir_type = rc_tab->ir_type; ir_dev->rc_tab.alloc = roundup_pow_of_two(rc_tab->len * @@ -370,6 +445,8 @@ ir_dev->rc_tab.size, ir_dev->rc_tab.alloc); set_bit(EV_KEY, input_dev->evbit); + set_bit(EV_REP, input_dev->evbit); + if (ir_setkeytable(input_dev, &ir_dev->rc_tab, rc_tab)) { rc = -ENOMEM; goto out_table; @@ -412,7 +489,7 @@ return; IR_dprintk(1, "Freed keycode table\n"); - + del_timer_sync(&ir_dev->timer_keyup); rc_tab = &ir_dev->rc_tab; rc_tab->size = 0; kfree(rc_tab->scan); Index: ir/drivers/media/IR/ir-nec-decoder.c =================================================================== --- ir.orig/drivers/media/IR/ir-nec-decoder.c 2010-04-02 12:22:50.872195298 +0200 +++ ir/drivers/media/IR/ir-nec-decoder.c 2010-04-02 16:34:32.531192323 +0200 @@ -179,8 +179,7 @@ if (is_repeat(evs, len, *pos)) { *pos += 2; if (ir->keypressed) { - mod_timer(&ir->raw->timer_keyup, - jiffies + msecs_to_jiffies(REPEAT_TIME)); + ir_repeat(input_dev); IR_dprintk(1, "NEC repeat event\n"); return 1; } else { @@ -237,9 +236,7 @@ } IR_dprintk(1, "NEC scancode 0x%04x\n", ircode); - ir_keydown(input_dev, ircode); - mod_timer(&ir->raw->timer_keyup, - jiffies + msecs_to_jiffies(REPEAT_TIME)); + ir_keydown(input_dev, ircode, 0); return 1; err: Index: ir/drivers/media/IR/ir-raw-event.c =================================================================== --- ir.orig/drivers/media/IR/ir-raw-event.c 2010-04-02 12:22:50.856216460 +0200 +++ ir/drivers/media/IR/ir-raw-event.c 2010-04-02 12:34:05.395187844 +0200 @@ -53,13 +53,6 @@ /* Used to load the decoders */ static struct work_struct wq_load; -static void ir_keyup_timer(unsigned long data) -{ - struct input_dev *input_dev = (struct input_dev *)data; - - ir_keyup(input_dev); -} - int ir_raw_event_register(struct input_dev *input_dev) { struct ir_input_dev *ir = input_get_drvdata(input_dev); @@ -72,11 +65,6 @@ size = sizeof(struct ir_raw_event) * MAX_IR_EVENT_SIZE * 2; size = roundup_pow_of_two(size); - init_timer(&ir->raw->timer_keyup); - ir->raw->timer_keyup.function = ir_keyup_timer; - ir->raw->timer_keyup.data = (unsigned long)input_dev; - set_bit(EV_REP, input_dev->evbit); - rc = kfifo_alloc(&ir->raw->kfifo, size, GFP_KERNEL); if (rc < 0) { kfree (ir->raw); @@ -103,8 +91,6 @@ if (!ir->raw) return; - del_timer_sync(&ir->raw->timer_keyup); - RUN_DECODER(raw_unregister, input_dev); kfifo_free(&ir->raw->kfifo); Index: ir/include/media/ir-core.h =================================================================== --- ir.orig/include/media/ir-core.h 2010-04-02 12:22:50.836196555 +0200 +++ ir/include/media/ir-core.h 2010-04-02 16:23:29.783236946 +0200 @@ -70,7 +70,6 @@ struct ir_raw_event_ctrl { struct kfifo kfifo; /* fifo for the pulse/space events */ struct timespec last_event; /* when last event occurred */ - struct timer_list timer_keyup; /* timer for key release */ }; struct ir_input_dev { @@ -80,10 +79,16 @@ unsigned long devno; /* device number */ const struct ir_dev_props *props; /* Device properties */ struct ir_raw_event_ctrl *raw; /* for raw pulse/space events */ + struct input_dev *input_dev; /* the input device associated with this device */ /* key info - needed by IR keycode handlers */ - u32 keycode; /* linux key code */ - int keypressed; /* current state */ + spinlock_t keylock; /* protects the below members */ + bool keypressed; /* current state */ + unsigned long keyup_jiffies; /* when should the current keypress be released? */ + struct timer_list timer_keyup; /* timer for releasing a keypress */ + u32 last_keycode; /* keycode of last command */ + u32 last_scancode; /* scancode of last command */ + u8 last_toggle; /* toggle of last command */ }; struct ir_raw_handler { @@ -102,8 +107,8 @@ u32 ir_g_keycode_from_table(struct input_dev *input_dev, u32 scancode); -void ir_keyup(struct input_dev *dev); -void ir_keydown(struct input_dev *dev, int scancode); +void ir_repeat(struct input_dev *dev); +void ir_keydown(struct input_dev *dev, int scancode, u8 toggle); int ir_input_register(struct input_dev *dev, const struct ir_scancode_table *ir_codes, const struct ir_dev_props *props, ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core 2010-04-02 18:58 ` david @ 2010-04-02 18:58 ` david -1 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Attachment #1: convert-budget-ci-to-use-ir-core --] [-- Type: TEXT/PLAIN, Size: 3911 bytes --] This patch converts drivers/media/dvb/ttpci/budget-ci.c to use ir-core rather than rolling its own keydown timeout handler and reporting keys via drivers/media/IR/ir-functions.c. Signed-off-by: David Härdeman <david@hardeman.nu> Index: ir/drivers/media/dvb/ttpci/budget-ci.c =================================================================== --- ir.orig/drivers/media/dvb/ttpci/budget-ci.c 2010-04-02 16:41:15.524206900 +0200 +++ ir/drivers/media/dvb/ttpci/budget-ci.c 2010-04-02 16:48:15.668239437 +0200 @@ -35,7 +35,7 @@ #include <linux/interrupt.h> #include <linux/input.h> #include <linux/spinlock.h> -#include <media/ir-common.h> +#include <media/ir-core.h> #include "budget.h" @@ -82,12 +82,6 @@ #define SLOTSTATUS_READY 8 #define SLOTSTATUS_OCCUPIED (SLOTSTATUS_PRESENT|SLOTSTATUS_RESET|SLOTSTATUS_READY) -/* - * Milliseconds during which a key is regarded as pressed. - * If an identical command arrives within this time, the timer will start over. - */ -#define IR_KEYPRESS_TIMEOUT 250 - /* RC5 device wildcard */ #define IR_DEVICE_ANY 255 @@ -104,12 +98,9 @@ struct budget_ci_ir { struct input_dev *dev; struct tasklet_struct msp430_irq_tasklet; - struct timer_list timer_keyup; char name[72]; /* 40 + 32 for (struct saa7146_dev).name */ char phys[32]; - struct ir_input_state state; int rc5_device; - u32 last_raw; u32 ir_key; bool have_command; }; @@ -124,18 +115,11 @@ u8 tuner_pll_address; /* used for philips_tdm1316l configs */ }; -static void msp430_ir_keyup(unsigned long data) -{ - struct budget_ci_ir *ir = (struct budget_ci_ir *) data; - ir_input_nokey(ir->dev, &ir->state); -} - static void msp430_ir_interrupt(unsigned long data) { struct budget_ci *budget_ci = (struct budget_ci *) data; struct input_dev *dev = budget_ci->ir.dev; u32 command = ttpci_budget_debiread(&budget_ci->budget, DEBINOSWAP, DEBIADDR_IR, 2, 1, 0) >> 8; - u32 raw; /* * The msp430 chip can generate two different bytes, command and device @@ -171,20 +155,12 @@ return; budget_ci->ir.have_command = false; + /* FIXME: We should generate complete scancodes with device info */ if (budget_ci->ir.rc5_device != IR_DEVICE_ANY && budget_ci->ir.rc5_device != (command & 0x1f)) return; - /* Is this a repeated key sequence? (same device, command, toggle) */ - raw = budget_ci->ir.ir_key | (command << 8); - if (budget_ci->ir.last_raw != raw || !timer_pending(&budget_ci->ir.timer_keyup)) { - ir_input_nokey(dev, &budget_ci->ir.state); - ir_input_keydown(dev, &budget_ci->ir.state, - budget_ci->ir.ir_key); - budget_ci->ir.last_raw = raw; - } - - mod_timer(&budget_ci->ir.timer_keyup, jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT)); + ir_keydown(dev, budget_ci->ir.ir_key, (command & 0x20) ? 1 : 0); } static int msp430_ir_init(struct budget_ci *budget_ci) @@ -251,11 +227,6 @@ ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); - /* initialise the key-up timeout handler */ - init_timer(&budget_ci->ir.timer_keyup); - budget_ci->ir.timer_keyup.function = msp430_ir_keyup; - budget_ci->ir.timer_keyup.data = (unsigned long) &budget_ci->ir; - budget_ci->ir.last_raw = 0xffff; /* An impossible value */ error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME); if (error) { printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error); @@ -284,9 +255,6 @@ saa7146_setgpio(saa, 3, SAA7146_GPIO_INPUT); tasklet_kill(&budget_ci->ir.msp430_irq_tasklet); - del_timer_sync(&dev->timer); - ir_input_nokey(dev, &budget_ci->ir.state); - ir_input_unregister(dev); } -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core @ 2010-04-02 18:58 ` david 0 siblings, 0 replies; 12+ messages in thread From: david @ 2010-04-02 18:58 UTC (permalink / raw) To: mchehab; +Cc: linux-input, linux-media [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: convert-budget-ci-to-use-ir-core --] [-- Type: text/plain, Size: 3601 bytes --] This patch converts drivers/media/dvb/ttpci/budget-ci.c to use ir-core rather than rolling its own keydown timeout handler and reporting keys via drivers/media/IR/ir-functions.c. Signed-off-by: David Härdeman <david@hardeman.nu> Index: ir/drivers/media/dvb/ttpci/budget-ci.c =================================================================== --- ir.orig/drivers/media/dvb/ttpci/budget-ci.c 2010-04-02 16:41:15.524206900 +0200 +++ ir/drivers/media/dvb/ttpci/budget-ci.c 2010-04-02 16:48:15.668239437 +0200 @@ -35,7 +35,7 @@ #include <linux/interrupt.h> #include <linux/input.h> #include <linux/spinlock.h> -#include <media/ir-common.h> +#include <media/ir-core.h> #include "budget.h" @@ -82,12 +82,6 @@ #define SLOTSTATUS_READY 8 #define SLOTSTATUS_OCCUPIED (SLOTSTATUS_PRESENT|SLOTSTATUS_RESET|SLOTSTATUS_READY) -/* - * Milliseconds during which a key is regarded as pressed. - * If an identical command arrives within this time, the timer will start over. - */ -#define IR_KEYPRESS_TIMEOUT 250 - /* RC5 device wildcard */ #define IR_DEVICE_ANY 255 @@ -104,12 +98,9 @@ struct budget_ci_ir { struct input_dev *dev; struct tasklet_struct msp430_irq_tasklet; - struct timer_list timer_keyup; char name[72]; /* 40 + 32 for (struct saa7146_dev).name */ char phys[32]; - struct ir_input_state state; int rc5_device; - u32 last_raw; u32 ir_key; bool have_command; }; @@ -124,18 +115,11 @@ u8 tuner_pll_address; /* used for philips_tdm1316l configs */ }; -static void msp430_ir_keyup(unsigned long data) -{ - struct budget_ci_ir *ir = (struct budget_ci_ir *) data; - ir_input_nokey(ir->dev, &ir->state); -} - static void msp430_ir_interrupt(unsigned long data) { struct budget_ci *budget_ci = (struct budget_ci *) data; struct input_dev *dev = budget_ci->ir.dev; u32 command = ttpci_budget_debiread(&budget_ci->budget, DEBINOSWAP, DEBIADDR_IR, 2, 1, 0) >> 8; - u32 raw; /* * The msp430 chip can generate two different bytes, command and device @@ -171,20 +155,12 @@ return; budget_ci->ir.have_command = false; + /* FIXME: We should generate complete scancodes with device info */ if (budget_ci->ir.rc5_device != IR_DEVICE_ANY && budget_ci->ir.rc5_device != (command & 0x1f)) return; - /* Is this a repeated key sequence? (same device, command, toggle) */ - raw = budget_ci->ir.ir_key | (command << 8); - if (budget_ci->ir.last_raw != raw || !timer_pending(&budget_ci->ir.timer_keyup)) { - ir_input_nokey(dev, &budget_ci->ir.state); - ir_input_keydown(dev, &budget_ci->ir.state, - budget_ci->ir.ir_key); - budget_ci->ir.last_raw = raw; - } - - mod_timer(&budget_ci->ir.timer_keyup, jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT)); + ir_keydown(dev, budget_ci->ir.ir_key, (command & 0x20) ? 1 : 0); } static int msp430_ir_init(struct budget_ci *budget_ci) @@ -251,11 +227,6 @@ ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); - /* initialise the key-up timeout handler */ - init_timer(&budget_ci->ir.timer_keyup); - budget_ci->ir.timer_keyup.function = msp430_ir_keyup; - budget_ci->ir.timer_keyup.data = (unsigned long) &budget_ci->ir; - budget_ci->ir.last_raw = 0xffff; /* An impossible value */ error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME); if (error) { printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error); @@ -284,9 +255,6 @@ saa7146_setgpio(saa, 3, SAA7146_GPIO_INPUT); tasklet_kill(&budget_ci->ir.msp430_irq_tasklet); - del_timer_sync(&dev->timer); - ir_input_nokey(dev, &budget_ci->ir.state); - ir_input_unregister(dev); } ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core 2010-04-02 18:58 ` david (?) @ 2010-04-08 21:59 ` Mauro Carvalho Chehab 2010-04-08 23:09 ` David Härdeman -1 siblings, 1 reply; 12+ messages in thread From: Mauro Carvalho Chehab @ 2010-04-08 21:59 UTC (permalink / raw) To: david; +Cc: linux-input, linux-media david@hardeman.nu wrote: > This patch converts drivers/media/dvb/ttpci/budget-ci.c to use ir-core > rather than rolling its own keydown timeout handler and reporting keys > via drivers/media/IR/ir-functions.c. Hmm... had you test this patch? It got me an error here: drivers/media/dvb/ttpci/budget-ci.c: In function ‘msp430_ir_init’: drivers/media/dvb/ttpci/budget-ci.c:228: error: implicit declaration of function ‘ir_input_init’ drivers/media/dvb/ttpci/budget-ci.c:228: error: ‘struct budget_ci_ir’ has no member named ‘state’ The fix is trivial. Just drop this line: ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); It shouldn't cause any troubles, since the only things this function currently do are: ir->ir_type = ir_type; if (repeat) set_bit(EV_REP, dev->evbit); As the repeat is inside ir-core, and the ir struct is not used anymore, this removal should cause no harm. So, I am dropping the line at the code I'm committing at v4l-dvb.git, to avoid bisect breakages. > > Signed-off-by: David Härdeman <david@hardeman.nu> > > > Index: ir/drivers/media/dvb/ttpci/budget-ci.c > =================================================================== > --- ir.orig/drivers/media/dvb/ttpci/budget-ci.c 2010-04-02 16:41:15.524206900 +0200 > +++ ir/drivers/media/dvb/ttpci/budget-ci.c 2010-04-02 16:48:15.668239437 +0200 > @@ -35,7 +35,7 @@ > #include <linux/interrupt.h> > #include <linux/input.h> > #include <linux/spinlock.h> > -#include <media/ir-common.h> > +#include <media/ir-core.h> > > #include "budget.h" > > @@ -82,12 +82,6 @@ > #define SLOTSTATUS_READY 8 > #define SLOTSTATUS_OCCUPIED (SLOTSTATUS_PRESENT|SLOTSTATUS_RESET|SLOTSTATUS_READY) > > -/* > - * Milliseconds during which a key is regarded as pressed. > - * If an identical command arrives within this time, the timer will start over. > - */ > -#define IR_KEYPRESS_TIMEOUT 250 > - > /* RC5 device wildcard */ > #define IR_DEVICE_ANY 255 > > @@ -104,12 +98,9 @@ > struct budget_ci_ir { > struct input_dev *dev; > struct tasklet_struct msp430_irq_tasklet; > - struct timer_list timer_keyup; > char name[72]; /* 40 + 32 for (struct saa7146_dev).name */ > char phys[32]; > - struct ir_input_state state; > int rc5_device; > - u32 last_raw; > u32 ir_key; > bool have_command; > }; > @@ -124,18 +115,11 @@ > u8 tuner_pll_address; /* used for philips_tdm1316l configs */ > }; > > -static void msp430_ir_keyup(unsigned long data) > -{ > - struct budget_ci_ir *ir = (struct budget_ci_ir *) data; > - ir_input_nokey(ir->dev, &ir->state); > -} > - > static void msp430_ir_interrupt(unsigned long data) > { > struct budget_ci *budget_ci = (struct budget_ci *) data; > struct input_dev *dev = budget_ci->ir.dev; > u32 command = ttpci_budget_debiread(&budget_ci->budget, DEBINOSWAP, DEBIADDR_IR, 2, 1, 0) >> 8; > - u32 raw; > > /* > * The msp430 chip can generate two different bytes, command and device > @@ -171,20 +155,12 @@ > return; > budget_ci->ir.have_command = false; > > + /* FIXME: We should generate complete scancodes with device info */ > if (budget_ci->ir.rc5_device != IR_DEVICE_ANY && > budget_ci->ir.rc5_device != (command & 0x1f)) > return; > > - /* Is this a repeated key sequence? (same device, command, toggle) */ > - raw = budget_ci->ir.ir_key | (command << 8); > - if (budget_ci->ir.last_raw != raw || !timer_pending(&budget_ci->ir.timer_keyup)) { > - ir_input_nokey(dev, &budget_ci->ir.state); > - ir_input_keydown(dev, &budget_ci->ir.state, > - budget_ci->ir.ir_key); > - budget_ci->ir.last_raw = raw; > - } > - > - mod_timer(&budget_ci->ir.timer_keyup, jiffies + msecs_to_jiffies(IR_KEYPRESS_TIMEOUT)); > + ir_keydown(dev, budget_ci->ir.ir_key, (command & 0x20) ? 1 : 0); > } > > static int msp430_ir_init(struct budget_ci *budget_ci) > @@ -251,11 +227,6 @@ > > ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); > > - /* initialise the key-up timeout handler */ > - init_timer(&budget_ci->ir.timer_keyup); > - budget_ci->ir.timer_keyup.function = msp430_ir_keyup; > - budget_ci->ir.timer_keyup.data = (unsigned long) &budget_ci->ir; > - budget_ci->ir.last_raw = 0xffff; /* An impossible value */ > error = ir_input_register(input_dev, ir_codes, NULL, MODULE_NAME); > if (error) { > printk(KERN_ERR "budget_ci: could not init driver for IR device (code %d)\n", error); > @@ -284,9 +255,6 @@ > saa7146_setgpio(saa, 3, SAA7146_GPIO_INPUT); > tasklet_kill(&budget_ci->ir.msp430_irq_tasklet); > > - del_timer_sync(&dev->timer); > - ir_input_nokey(dev, &budget_ci->ir.state); > - > ir_input_unregister(dev); > } > > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Cheers, Mauro ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core 2010-04-08 21:59 ` Mauro Carvalho Chehab @ 2010-04-08 23:09 ` David Härdeman 0 siblings, 0 replies; 12+ messages in thread From: David Härdeman @ 2010-04-08 23:09 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-input, linux-media On Thu, Apr 08, 2010 at 06:59:30PM -0300, Mauro Carvalho Chehab wrote: >david@hardeman.nu wrote: >> This patch converts drivers/media/dvb/ttpci/budget-ci.c to use ir-core >> rather than rolling its own keydown timeout handler and reporting keys >> via drivers/media/IR/ir-functions.c. > >Hmm... had you test this patch? It got me an error here: Sorry, I must have sent you the wrong one :) >drivers/media/dvb/ttpci/budget-ci.c: In function ‘msp430_ir_init’: >drivers/media/dvb/ttpci/budget-ci.c:228: error: implicit declaration of function ‘ir_input_init’ >drivers/media/dvb/ttpci/budget-ci.c:228: error: ‘struct budget_ci_ir’ has no member named ‘state’ > >The fix is trivial. Just drop this line: > > ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); > >It shouldn't cause any troubles, since the only things this function currently do are: > ir->ir_type = ir_type; > > if (repeat) > set_bit(EV_REP, dev->evbit); > >As the repeat is inside ir-core, and the ir struct is not used anymore, this removal >should cause no harm. > >So, I am dropping the line at the code I'm committing at v4l-dvb.git, to avoid bisect >breakages. You're entirely correct, that line should have been dropped (I even sent the same thing as part of my latest patch series before I read this mail, but if you can fixup the original patch that'd be even better). -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core @ 2010-04-08 23:09 ` David Härdeman 0 siblings, 0 replies; 12+ messages in thread From: David Härdeman @ 2010-04-08 23:09 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-input, linux-media On Thu, Apr 08, 2010 at 06:59:30PM -0300, Mauro Carvalho Chehab wrote: >david@hardeman.nu wrote: >> This patch converts drivers/media/dvb/ttpci/budget-ci.c to use ir-core >> rather than rolling its own keydown timeout handler and reporting keys >> via drivers/media/IR/ir-functions.c. > >Hmm... had you test this patch? It got me an error here: Sorry, I must have sent you the wrong one :) >drivers/media/dvb/ttpci/budget-ci.c: In function ‘msp430_ir_init’: >drivers/media/dvb/ttpci/budget-ci.c:228: error: implicit declaration of function ‘ir_input_init’ >drivers/media/dvb/ttpci/budget-ci.c:228: error: ‘struct budget_ci_ir’ has no member named ‘state’ > >The fix is trivial. Just drop this line: > > ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); > >It shouldn't cause any troubles, since the only things this function currently do are: > ir->ir_type = ir_type; > > if (repeat) > set_bit(EV_REP, dev->evbit); > >As the repeat is inside ir-core, and the ir struct is not used anymore, this removal >should cause no harm. > >So, I am dropping the line at the code I'm committing at v4l-dvb.git, to avoid bisect >breakages. You're entirely correct, that line should have been dropped (I even sent the same thing as part of my latest patch series before I read this mail, but if you can fixup the original patch that'd be even better). -- David Härdeman ^ permalink raw reply [flat|nested] 12+ messages in thread
* Re: [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core 2010-04-08 23:09 ` David Härdeman (?) @ 2010-04-08 23:30 ` Mauro Carvalho Chehab -1 siblings, 0 replies; 12+ messages in thread From: Mauro Carvalho Chehab @ 2010-04-08 23:30 UTC (permalink / raw) To: David Härdeman; +Cc: linux-input, linux-media David Härdeman wrote: > On Thu, Apr 08, 2010 at 06:59:30PM -0300, Mauro Carvalho Chehab wrote: >> david@hardeman.nu wrote: >>> This patch converts drivers/media/dvb/ttpci/budget-ci.c to use ir-core >>> rather than rolling its own keydown timeout handler and reporting keys >>> via drivers/media/IR/ir-functions.c. >> >> Hmm... had you test this patch? It got me an error here: > > Sorry, I must have sent you the wrong one :) > >> drivers/media/dvb/ttpci/budget-ci.c: In function ‘msp430_ir_init’: >> drivers/media/dvb/ttpci/budget-ci.c:228: error: implicit declaration >> of function ‘ir_input_init’ >> drivers/media/dvb/ttpci/budget-ci.c:228: error: ‘struct budget_ci_ir’ >> has no member named ‘state’ >> >> The fix is trivial. Just drop this line: >> >> ir_input_init(input_dev, &budget_ci->ir.state, IR_TYPE_RC5); >> >> It shouldn't cause any troubles, since the only things this function >> currently do are: >> ir->ir_type = ir_type; >> >> if (repeat) >> set_bit(EV_REP, dev->evbit); >> >> As the repeat is inside ir-core, and the ir struct is not used >> anymore, this removal >> should cause no harm. >> >> So, I am dropping the line at the code I'm committing at v4l-dvb.git, >> to avoid bisect >> breakages. > > You're entirely correct, that line should have been dropped (I even sent > the same thing as part of my latest patch series before I read this > mail, but if you can fixup the original patch that'd be even better). While I don't care much on experimental trees, I always do a make allyesconfig and try to compile all drivers before pushing on my master tree. This helps to avoid some silly mistakes to go upstream ;) -- Cheers, Mauro ^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2010-04-08 23:30 UTC | newest] Thread overview: 12+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-04-02 18:58 [patch 0/3] ir-core keytable patches david 2010-04-02 18:58 ` david 2010-04-02 18:58 ` [patch 1/3] [PATCH] drivers/media/IR - improve keytable code david 2010-04-02 18:58 ` david 2010-04-02 18:58 ` [patch 2/3] [PATCH] drivers/media/IR - improve keyup/keydown logic david 2010-04-02 18:58 ` david 2010-04-02 18:58 ` [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core david 2010-04-02 18:58 ` david 2010-04-08 21:59 ` Mauro Carvalho Chehab 2010-04-08 23:09 ` David Härdeman 2010-04-08 23:09 ` David Härdeman 2010-04-08 23:30 ` Mauro Carvalho Chehab
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.