* [patch 1/3] [PATCH] drivers/media/IR - improve keytable code
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 2/3] [PATCH] drivers/media/IR - improve keyup/keydown logic david
2010-04-02 18:58 ` [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core david
2 siblings, 0 replies; 7+ 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] 7+ messages in thread* [patch 2/3] [PATCH] drivers/media/IR - improve keyup/keydown logic
2010-04-02 18:58 [patch 0/3] ir-core keytable patches 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 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core david
2 siblings, 0 replies; 7+ 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] 7+ messages in thread* [patch 3/3] Convert drivers/media/dvb/ttpci/budget-ci.c to use ir-core
2010-04-02 18:58 [patch 0/3] ir-core keytable patches david
2010-04-02 18:58 ` [patch 1/3] [PATCH] drivers/media/IR - improve keytable code 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-08 21:59 ` Mauro Carvalho Chehab
2 siblings, 1 reply; 7+ 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] 7+ messages in thread