* [PATCH 0/6] rc-core - protocol in keytables
@ 2017-04-27 20:33 David Härdeman
2017-04-27 20:33 ` [PATCH 1/6] rc-core: fix input repeat handling David Härdeman
` (5 more replies)
0 siblings, 6 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:33 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The first three patches are just some cleanups that I noticed
while working on the other patches.
The fourth and fifth patch change rc-core over to use NEC32
scancodes everywhere. That might seem like a recipe for
breaking userspace...but, as you'll see with the sixth patch,
we can't really avoid it if we want to improve the
EVIOC[GS]KEYCODE_V2 ioctl():s to support protocols.
And since it was requested last time around, I have written
a much longer explanation of the NEC32/ioctl patches and
posted here (with pretty pictures):
https://david.hardeman.nu/rccore/
Most of you might want to skip the introduction part :)
---
David Härdeman (6):
rc-core: fix input repeat handling
rc-core: cleanup rc_register_device
rc-core: cleanup rc_register_device pt2
rc-core: use the full 32 bits for NEC scancodes in wakefilters
rc-core: use the full 32 bits for NEC scancodes
rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
drivers/media/pci/cx88/cx88-input.c | 4
drivers/media/pci/saa7134/saa7134-input.c | 4
drivers/media/rc/ati_remote.c | 1
drivers/media/rc/igorplugusb.c | 4
drivers/media/rc/img-ir/img-ir-nec.c | 92 +------
drivers/media/rc/imon.c | 7 -
drivers/media/rc/ir-nec-decoder.c | 63 +----
drivers/media/rc/rc-core-priv.h | 2
drivers/media/rc/rc-ir-raw.c | 34 ++
drivers/media/rc/rc-main.c | 406 ++++++++++++++++++-----------
drivers/media/rc/winbond-cir.c | 32 --
drivers/media/usb/au0828/au0828-input.c | 3
drivers/media/usb/dvb-usb-v2/af9015.c | 30 --
drivers/media/usb/dvb-usb-v2/af9035.c | 27 --
drivers/media/usb/dvb-usb-v2/az6007.c | 25 --
drivers/media/usb/dvb-usb-v2/lmedm04.c | 5
drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 29 +-
drivers/media/usb/dvb-usb/dib0700_core.c | 25 --
drivers/media/usb/dvb-usb/dtt200u.c | 25 +-
drivers/media/usb/em28xx/em28xx-input.c | 22 --
include/media/rc-core.h | 28 ++
include/media/rc-map.h | 53 ++--
22 files changed, 412 insertions(+), 509 deletions(-)
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* [PATCH 1/6] rc-core: fix input repeat handling
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
@ 2017-04-27 20:33 ` David Härdeman
2017-04-27 20:34 ` [PATCH 2/6] rc-core: cleanup rc_register_device David Härdeman
` (4 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:33 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The call to input_register_device() needs to take place
before the repeat parameters are set or the input subsystem
repeat handling will be disabled (as was already noted in
the comments in that function).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/rc-main.c | 20 ++++++++++----------
1 file changed, 10 insertions(+), 10 deletions(-)
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 6ec73357fa47..802e559cc30e 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1703,6 +1703,16 @@ static int rc_setup_rx_device(struct rc_dev *dev)
if (dev->close)
dev->input_dev->close = ir_close;
+ dev->input_dev->dev.parent = &dev->dev;
+ memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
+ dev->input_dev->phys = dev->input_phys;
+ dev->input_dev->name = dev->input_name;
+
+ /* rc_open will be called here */
+ rc = input_register_device(dev->input_dev);
+ if (rc)
+ goto out_table;
+
/*
* Default delay of 250ms is too short for some protocols, especially
* since the timeout is currently set to 250ms. Increase it to 500ms,
@@ -1718,16 +1728,6 @@ static int rc_setup_rx_device(struct rc_dev *dev)
*/
dev->input_dev->rep[REP_PERIOD] = 125;
- dev->input_dev->dev.parent = &dev->dev;
- memcpy(&dev->input_dev->id, &dev->input_id, sizeof(dev->input_id));
- dev->input_dev->phys = dev->input_phys;
- dev->input_dev->name = dev->input_name;
-
- /* rc_open will be called here */
- rc = input_register_device(dev->input_dev);
- if (rc)
- goto out_table;
-
return 0;
out_table:
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 2/6] rc-core: cleanup rc_register_device
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
2017-04-27 20:33 ` [PATCH 1/6] rc-core: fix input repeat handling David Härdeman
@ 2017-04-27 20:34 ` David Härdeman
2017-05-01 16:49 ` Sean Young
2017-04-27 20:34 ` [PATCH 3/6] rc-core: cleanup rc_register_device pt2 David Härdeman
` (3 subsequent siblings)
5 siblings, 1 reply; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:34 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The device core infrastructure is based on the presumption that
once a driver calls device_add(), it must be ready to accept
userspace interaction.
This requires splitting rc_setup_rx_device() into two functions
and reorganizing rc_register_device() so that as much work
as possible is performed before calling device_add().
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/rc-core-priv.h | 2 +
drivers/media/rc/rc-ir-raw.c | 34 ++++++++++++------
drivers/media/rc/rc-main.c | 75 +++++++++++++++++++++++++--------------
3 files changed, 73 insertions(+), 38 deletions(-)
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 0455b273c2fc..b3e7cac2c3ee 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max,
* Routines from rc-raw.c to be used internally and by decoders
*/
u64 ir_raw_get_allowed_protocols(void);
+int ir_raw_event_prepare(struct rc_dev *dev);
int ir_raw_event_register(struct rc_dev *dev);
+void ir_raw_event_free(struct rc_dev *dev);
void ir_raw_event_unregister(struct rc_dev *dev);
int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 90f66dc7c0d7..ae7785c4fbe7 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode);
/*
* Used to (un)register raw event clients
*/
-int ir_raw_event_register(struct rc_dev *dev)
+int ir_raw_event_prepare(struct rc_dev *dev)
{
- int rc;
- struct ir_raw_handler *handler;
+ static bool raw_init; /* 'false' default value, raw decoders loaded? */
if (!dev)
return -EINVAL;
+ if (!raw_init) {
+ request_module("ir-lirc-codec");
+ raw_init = true;
+ }
+
dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
if (!dev->raw)
return -ENOMEM;
@@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->change_protocol = change_protocol;
INIT_KFIFO(dev->raw->kfifo);
+ return 0;
+}
+
+int ir_raw_event_register(struct rc_dev *dev)
+{
+ struct ir_raw_handler *handler;
+
/*
* raw transmitters do not need any event registration
* because the event is coming from userspace
@@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev)
dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
"rc%u", dev->minor);
- if (IS_ERR(dev->raw->thread)) {
- rc = PTR_ERR(dev->raw->thread);
- goto out;
- }
+ if (IS_ERR(dev->raw->thread))
+ return PTR_ERR(dev->raw->thread);
}
mutex_lock(&ir_raw_handler_lock);
@@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
mutex_unlock(&ir_raw_handler_lock);
return 0;
+}
+
+void ir_raw_event_free(struct rc_dev *dev)
+{
+ if (!dev)
+ return;
-out:
kfree(dev->raw);
dev->raw = NULL;
- return rc;
}
void ir_raw_event_unregister(struct rc_dev *dev)
@@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
handler->raw_unregister(dev);
mutex_unlock(&ir_raw_handler_lock);
- kfree(dev->raw);
- dev->raw = NULL;
+ ir_raw_event_free(dev);
}
/*
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 802e559cc30e..44189366f232 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
}
EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
-static int rc_setup_rx_device(struct rc_dev *dev)
+static int rc_prepare_rx_device(struct rc_dev *dev)
{
int rc;
struct rc_map *rc_map;
@@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev)
dev->input_dev->phys = dev->input_phys;
dev->input_dev->name = dev->input_name;
+ return 0;
+
+out_table:
+ ir_free_table(&dev->rc_map);
+
+ return rc;
+}
+
+static int rc_setup_rx_device(struct rc_dev *dev)
+{
+ int rc;
+
/* rc_open will be called here */
rc = input_register_device(dev->input_dev);
if (rc)
- goto out_table;
+ return rc;
/*
* Default delay of 250ms is too short for some protocols, especially
@@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev)
dev->input_dev->rep[REP_PERIOD] = 125;
return 0;
-
-out_table:
- ir_free_table(&dev->rc_map);
-
- return rc;
}
static void rc_free_rx_device(struct rc_dev *dev)
{
- if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX)
+ if (!dev)
return;
- ir_free_table(&dev->rc_map);
+ if (dev->input_dev) {
+ input_unregister_device(dev->input_dev);
+ dev->input_dev = NULL;
+ }
- input_unregister_device(dev->input_dev);
- dev->input_dev = NULL;
+ ir_free_table(&dev->rc_map);
}
int rc_register_device(struct rc_dev *dev)
{
- static bool raw_init; /* 'false' default value, raw decoders loaded? */
const char *path;
int attr = 0;
int minor;
@@ -1776,30 +1784,39 @@ int rc_register_device(struct rc_dev *dev)
dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp;
dev->sysfs_groups[attr++] = NULL;
+ if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+ rc = rc_prepare_rx_device(dev);
+ if (rc)
+ goto out_minor;
+ }
+
+ if (dev->driver_type == RC_DRIVER_IR_RAW ||
+ dev->driver_type == RC_DRIVER_IR_RAW_TX) {
+ rc = ir_raw_event_prepare(dev);
+ if (rc < 0)
+ goto out_rx_free;
+ }
+
rc = device_add(&dev->dev);
if (rc)
- goto out_unlock;
+ goto out_raw;
path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
dev_info(&dev->dev, "%s as %s\n",
dev->input_name ?: "Unspecified device", path ?: "N/A");
kfree(path);
+ if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
+ rc = rc_setup_rx_device(dev);
+ if (rc)
+ goto out_dev;
+ }
+
if (dev->driver_type == RC_DRIVER_IR_RAW ||
dev->driver_type == RC_DRIVER_IR_RAW_TX) {
- if (!raw_init) {
- request_module_nowait("ir-lirc-codec");
- raw_init = true;
- }
rc = ir_raw_event_register(dev);
if (rc < 0)
- goto out_dev;
- }
-
- if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
- rc = rc_setup_rx_device(dev);
- if (rc)
- goto out_raw;
+ goto out_rx;
}
/* Allow the RC sysfs nodes to be accessible */
@@ -1811,11 +1828,15 @@ int rc_register_device(struct rc_dev *dev)
return 0;
-out_raw:
- ir_raw_event_unregister(dev);
+out_rx:
+ rc_free_rx_device(dev);
out_dev:
device_del(&dev->dev);
-out_unlock:
+out_raw:
+ ir_raw_event_free(dev);
+out_rx_free:
+ ir_free_table(&dev->rc_map);
+out_minor:
ida_simple_remove(&rc_ida, minor);
return rc;
}
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 3/6] rc-core: cleanup rc_register_device pt2
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
2017-04-27 20:33 ` [PATCH 1/6] rc-core: fix input repeat handling David Härdeman
2017-04-27 20:34 ` [PATCH 2/6] rc-core: cleanup rc_register_device David Härdeman
@ 2017-04-27 20:34 ` David Härdeman
2017-04-27 20:34 ` [PATCH 4/6] rc-core: use the full 32 bits for NEC scancodes in wakefilters David Härdeman
` (2 subsequent siblings)
5 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:34 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Now that rc_register_device() is reorganised, the dev->initialized
hack can be removed. Any driver which calls rc_register_device()
must be prepared for the device to go live immediately.
The dev->initialized commits that are relevant are:
c73bbaa4ec3eb225ffe468f80d45724d0496bf03
08aeb7c9a42ab7aa8b53c8f7779ec58f860a565c
The original problem was that show_protocols() would access
dev->rc_map.* and various other bits which are now properly
initialized before device_add() is called.
At the same time, remove the bogus "device is being removed"
check (quiz: when would container_of give a NULL value???).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/rc-main.c | 67 +++++++-------------------------------------
include/media/rc-core.h | 2 -
2 files changed, 10 insertions(+), 59 deletions(-)
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 44189366f232..0acc8f27abeb 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -15,7 +15,6 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include <media/rc-core.h>
-#include <linux/atomic.h>
#include <linux/spinlock.h>
#include <linux/delay.h>
#include <linux/input.h>
@@ -934,8 +933,8 @@ static bool lirc_is_present(void)
* It returns the protocol names of supported protocols.
* Enabled protocols are printed in brackets.
*
- * dev->lock is taken to guard against races between device
- * registration, store_protocols and show_protocols.
+ * dev->lock is taken to guard against races between
+ * store_protocols and show_protocols.
*/
static ssize_t show_protocols(struct device *device,
struct device_attribute *mattr, char *buf)
@@ -945,13 +944,6 @@ static ssize_t show_protocols(struct device *device,
char *tmp = buf;
int i;
- /* Device is being removed */
- if (!dev)
- return -EINVAL;
-
- if (!atomic_read(&dev->initialized))
- return -ERESTARTSYS;
-
mutex_lock(&dev->lock);
enabled = dev->enabled_protocols;
@@ -1106,8 +1098,8 @@ static void ir_raw_load_modules(u64 *protocols)
* See parse_protocol_change() for the valid commands.
* Returns @len on success or a negative error code.
*
- * dev->lock is taken to guard against races between device
- * registration, store_protocols and show_protocols.
+ * dev->lock is taken to guard against races between
+ * store_protocols and show_protocols.
*/
static ssize_t store_protocols(struct device *device,
struct device_attribute *mattr,
@@ -1119,13 +1111,6 @@ static ssize_t store_protocols(struct device *device,
u64 old_protocols, new_protocols;
ssize_t rc;
- /* Device is being removed */
- if (!dev)
- return -EINVAL;
-
- if (!atomic_read(&dev->initialized))
- return -ERESTARTSYS;
-
IR_dprintk(1, "Normal protocol change requested\n");
current_protocols = &dev->enabled_protocols;
filter = &dev->scancode_filter;
@@ -1200,7 +1185,7 @@ static ssize_t store_protocols(struct device *device,
* Bits of the filter value corresponding to set bits in the filter mask are
* compared against input scancodes and non-matching scancodes are discarded.
*
- * dev->lock is taken to guard against races between device registration,
+ * dev->lock is taken to guard against races between
* store_filter and show_filter.
*/
static ssize_t show_filter(struct device *device,
@@ -1212,13 +1197,6 @@ static ssize_t show_filter(struct device *device,
struct rc_scancode_filter *filter;
u32 val;
- /* Device is being removed */
- if (!dev)
- return -EINVAL;
-
- if (!atomic_read(&dev->initialized))
- return -ERESTARTSYS;
-
mutex_lock(&dev->lock);
if (fattr->type == RC_FILTER_NORMAL)
@@ -1251,7 +1229,7 @@ static ssize_t show_filter(struct device *device,
* Bits of the filter value corresponding to set bits in the filter mask are
* compared against input scancodes and non-matching scancodes are discarded.
*
- * dev->lock is taken to guard against races between device registration,
+ * dev->lock is taken to guard against races between
* store_filter and show_filter.
*/
static ssize_t store_filter(struct device *device,
@@ -1265,13 +1243,6 @@ static ssize_t store_filter(struct device *device,
unsigned long val;
int (*set_filter)(struct rc_dev *dev, struct rc_scancode_filter *filter);
- /* Device is being removed */
- if (!dev)
- return -EINVAL;
-
- if (!atomic_read(&dev->initialized))
- return -ERESTARTSYS;
-
ret = kstrtoul(buf, 0, &val);
if (ret < 0)
return ret;
@@ -1372,8 +1343,8 @@ static const char * const proto_variant_names[] = {
* It returns the protocol names of supported protocols.
* The enabled protocols are printed in brackets.
*
- * dev->lock is taken to guard against races between device
- * registration, store_protocols and show_protocols.
+ * dev->lock is taken to guard against races between
+ * store_wakeup_protocols and show_wakeup_protocols.
*/
static ssize_t show_wakeup_protocols(struct device *device,
struct device_attribute *mattr,
@@ -1385,13 +1356,6 @@ static ssize_t show_wakeup_protocols(struct device *device,
char *tmp = buf;
int i;
- /* Device is being removed */
- if (!dev)
- return -EINVAL;
-
- if (!atomic_read(&dev->initialized))
- return -ERESTARTSYS;
-
mutex_lock(&dev->lock);
allowed = dev->allowed_wakeup_protocols;
@@ -1431,8 +1395,8 @@ static ssize_t show_wakeup_protocols(struct device *device,
* It is trigged by writing to /sys/class/rc/rc?/wakeup_protocols.
* Returns @len on success or a negative error code.
*
- * dev->lock is taken to guard against races between device
- * registration, store_protocols and show_protocols.
+ * dev->lock is taken to guard against races between
+ * store_wakeup_protocols and show_wakeup_protocols.
*/
static ssize_t store_wakeup_protocols(struct device *device,
struct device_attribute *mattr,
@@ -1444,13 +1408,6 @@ static ssize_t store_wakeup_protocols(struct device *device,
u64 allowed;
int i;
- /* Device is being removed */
- if (!dev)
- return -EINVAL;
-
- if (!atomic_read(&dev->initialized))
- return -ERESTARTSYS;
-
mutex_lock(&dev->lock);
allowed = dev->allowed_wakeup_protocols;
@@ -1773,7 +1730,6 @@ int rc_register_device(struct rc_dev *dev)
dev->minor = minor;
dev_set_name(&dev->dev, "rc%u", dev->minor);
dev_set_drvdata(&dev->dev, dev);
- atomic_set(&dev->initialized, 0);
dev->dev.groups = dev->sysfs_groups;
if (dev->driver_type != RC_DRIVER_IR_RAW_TX)
@@ -1819,9 +1775,6 @@ int rc_register_device(struct rc_dev *dev)
goto out_rx;
}
- /* Allow the RC sysfs nodes to be accessible */
- atomic_set(&dev->initialized, 1);
-
IR_dprintk(1, "Registered rc%u (driver: %s)\n",
dev->minor,
dev->driver_name ? dev->driver_name : "unknown");
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 73ddd721d7ba..78dea39a9b39 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -70,7 +70,6 @@ enum rc_filter_type {
/**
* struct rc_dev - represents a remote control device
* @dev: driver model's view of this device
- * @initialized: 1 if the device init has completed, 0 otherwise
* @managed_alloc: devm_rc_allocate_device was used to create rc_dev
* @sysfs_groups: sysfs attribute groups
* @input_name: name of the input child device
@@ -137,7 +136,6 @@ enum rc_filter_type {
*/
struct rc_dev {
struct device dev;
- atomic_t initialized;
bool managed_alloc;
const struct attribute_group *sysfs_groups[5];
const char *input_name;
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 4/6] rc-core: use the full 32 bits for NEC scancodes in wakefilters
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
` (2 preceding siblings ...)
2017-04-27 20:34 ` [PATCH 3/6] rc-core: cleanup rc_register_device pt2 David Härdeman
@ 2017-04-27 20:34 ` David Härdeman
2017-04-27 20:34 ` [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes David Härdeman
2017-04-27 20:34 ` [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl David Härdeman
5 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:34 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
The new sysfs wakefilter API will expose the difference between the NEC
protocols to userspace for no good reason and once exposed, it will be much
more difficult to change the logic.
By only allowing full NEC32 scancodes to be set, any heuristics in the kernel
can be avoided.
This is a minor preparation for the next patch which moves the rest of
rc-core over to only using NEC32.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/rc-main.c | 17 ++++-------------
drivers/media/rc/winbond-cir.c | 32 ++------------------------------
2 files changed, 6 insertions(+), 43 deletions(-)
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 0acc8f27abeb..8355f86a460b 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -741,8 +741,6 @@ static int rc_validate_filter(struct rc_dev *dev,
[RC_TYPE_SONY15] = 0xff007f,
[RC_TYPE_SONY20] = 0x1fff7f,
[RC_TYPE_JVC] = 0xffff,
- [RC_TYPE_NEC] = 0xffff,
- [RC_TYPE_NECX] = 0xffffff,
[RC_TYPE_NEC32] = 0xffffffff,
[RC_TYPE_SANYO] = 0x1fffff,
[RC_TYPE_MCIR2_KBD] = 0xffff,
@@ -758,14 +756,9 @@ static int rc_validate_filter(struct rc_dev *dev,
enum rc_type protocol = dev->wakeup_protocol;
switch (protocol) {
+ case RC_TYPE_NEC:
case RC_TYPE_NECX:
- if ((((s >> 16) ^ ~(s >> 8)) & 0xff) == 0)
- return -EINVAL;
- break;
- case RC_TYPE_NEC32:
- if ((((s >> 24) ^ ~(s >> 16)) & 0xff) == 0)
- return -EINVAL;
- break;
+ return -EINVAL;
case RC_TYPE_RC6_MCE:
if ((s & 0xffff0000) != 0x800f0000)
return -EINVAL;
@@ -1301,7 +1294,7 @@ static ssize_t store_filter(struct device *device,
/*
* This is the list of all variants of all protocols, which is used by
* the wakeup_protocols sysfs entry. In the protocols sysfs entry some
- * some protocols are grouped together (e.g. nec = nec + necx + nec32).
+ * some protocols are grouped together.
*
* For wakeup we need to know the exact protocol variant so the hardware
* can be programmed exactly what to expect.
@@ -1316,9 +1309,7 @@ static const char * const proto_variant_names[] = {
[RC_TYPE_SONY12] = "sony-12",
[RC_TYPE_SONY15] = "sony-15",
[RC_TYPE_SONY20] = "sony-20",
- [RC_TYPE_NEC] = "nec",
- [RC_TYPE_NECX] = "nec-x",
- [RC_TYPE_NEC32] = "nec-32",
+ [RC_TYPE_NEC32] = "nec",
[RC_TYPE_SANYO] = "sanyo",
[RC_TYPE_MCIR2_KBD] = "mcir2-kbd",
[RC_TYPE_MCIR2_MSE] = "mcir2-mse",
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 5a4d4a611197..6ef0e7232356 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -714,34 +714,6 @@ wbcir_shutdown(struct pnp_dev *device)
proto = IR_PROTOCOL_RC5;
break;
- case RC_TYPE_NEC:
- mask[1] = bitrev8(mask_sc);
- mask[0] = mask[1];
- mask[3] = bitrev8(mask_sc >> 8);
- mask[2] = mask[3];
-
- match[1] = bitrev8(wake_sc);
- match[0] = ~match[1];
- match[3] = bitrev8(wake_sc >> 8);
- match[2] = ~match[3];
-
- proto = IR_PROTOCOL_NEC;
- break;
-
- case RC_TYPE_NECX:
- mask[1] = bitrev8(mask_sc);
- mask[0] = mask[1];
- mask[2] = bitrev8(mask_sc >> 8);
- mask[3] = bitrev8(mask_sc >> 16);
-
- match[1] = bitrev8(wake_sc);
- match[0] = ~match[1];
- match[2] = bitrev8(wake_sc >> 8);
- match[3] = bitrev8(wake_sc >> 16);
-
- proto = IR_PROTOCOL_NEC;
- break;
-
case RC_TYPE_NEC32:
mask[0] = bitrev8(mask_sc);
mask[1] = bitrev8(mask_sc >> 8);
@@ -1087,8 +1059,8 @@ wbcir_probe(struct pnp_dev *device, const struct pnp_device_id *dev_id)
data->dev->max_timeout = 10 * IR_DEFAULT_TIMEOUT;
data->dev->rx_resolution = US_TO_NS(2);
data->dev->allowed_protocols = RC_BIT_ALL_IR_DECODER;
- data->dev->allowed_wakeup_protocols = RC_BIT_NEC | RC_BIT_NECX |
- RC_BIT_NEC32 | RC_BIT_RC5 | RC_BIT_RC6_0 |
+ data->dev->allowed_wakeup_protocols =
+ RC_BIT_NEC | RC_BIT_RC5 | RC_BIT_RC6_0 |
RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE;
data->dev->wakeup_protocol = RC_TYPE_RC6_MCE;
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
` (3 preceding siblings ...)
2017-04-27 20:34 ` [PATCH 4/6] rc-core: use the full 32 bits for NEC scancodes in wakefilters David Härdeman
@ 2017-04-27 20:34 ` David Härdeman
2017-04-28 11:58 ` Sean Young
2017-04-27 20:34 ` [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl David Härdeman
5 siblings, 1 reply; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:34 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
and the nec decoder without any loss of functionality. At the same time
it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
removes lots of duplication (as you can see from the patch, the same NEC
disambiguation logic is contained in several different drivers).
Using NEC32 also removes ambiguity. For example, consider these two NEC
messages:
NEC16 message to address 0x05, command 0x03
NEC24 message to address 0x0005, command 0x03
They'll both have scancode 0x00000503, and there's no way to tell which
message was received.
In order to maintain backwards compatibility, some heuristics are added
in rc-main.c to convert scancodes to NEC32 as necessary when userspace
adds entries to the keytable using the regular input ioctls. These
heuristics are essentially the same as the ones that are currently in
drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
with this patch).
While this is a change which cannot be 100% sure to be backwards
compatible, it should be noted that heuristics and such breakage cannot
be avoided when we introduce the protocol into the keytable
(see next patch).
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/pci/cx88/cx88-input.c | 4 +
drivers/media/pci/saa7134/saa7134-input.c | 4 +
drivers/media/rc/igorplugusb.c | 4 +
drivers/media/rc/img-ir/img-ir-nec.c | 92 ++++++-----------------------
drivers/media/rc/ir-nec-decoder.c | 63 +++-----------------
drivers/media/rc/rc-main.c | 65 +++++++++++++++++---
drivers/media/rc/winbond-cir.c | 2 -
drivers/media/usb/au0828/au0828-input.c | 3 -
drivers/media/usb/dvb-usb-v2/af9015.c | 30 ++-------
drivers/media/usb/dvb-usb-v2/af9035.c | 27 ++-------
drivers/media/usb/dvb-usb-v2/az6007.c | 25 ++------
drivers/media/usb/dvb-usb-v2/lmedm04.c | 5 +-
drivers/media/usb/dvb-usb-v2/rtl28xxu.c | 29 ++-------
drivers/media/usb/dvb-usb/dib0700_core.c | 25 ++------
drivers/media/usb/dvb-usb/dtt200u.c | 25 ++------
drivers/media/usb/em28xx/em28xx-input.c | 22 ++-----
include/media/rc-map.h | 48 ++++++++-------
17 files changed, 162 insertions(+), 311 deletions(-)
diff --git a/drivers/media/pci/cx88/cx88-input.c b/drivers/media/pci/cx88/cx88-input.c
index 01f2e472a2a0..61c46763ac97 100644
--- a/drivers/media/pci/cx88/cx88-input.c
+++ b/drivers/media/pci/cx88/cx88-input.c
@@ -146,7 +146,7 @@ static void cx88_ir_handle_key(struct cx88_IR *ir)
scancode = RC_SCANCODE_NECX(addr, cmd);
if (0 == (gpio & ir->mask_keyup))
- rc_keydown_notimeout(ir->dev, RC_TYPE_NECX, scancode,
+ rc_keydown_notimeout(ir->dev, RC_TYPE_NEC, scancode,
0);
else
rc_keyup(ir->dev);
@@ -348,7 +348,7 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci)
* 002-T mini RC, provided with newer PV hardware
*/
ir_codes = RC_MAP_PIXELVIEW_MK12;
- rc_type = RC_BIT_NECX;
+ rc_type = RC_BIT_NEC;
ir->gpio_addr = MO_GP1_IO;
ir->mask_keyup = 0x80;
ir->polling = 10; /* ms */
diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c
index 78849c19f68a..47d8e055ddd3 100644
--- a/drivers/media/pci/saa7134/saa7134-input.c
+++ b/drivers/media/pci/saa7134/saa7134-input.c
@@ -338,7 +338,7 @@ static int get_key_beholdm6xx(struct IR_i2c *ir, enum rc_type *protocol,
if (data[9] != (unsigned char)(~data[8]))
return 0;
- *protocol = RC_TYPE_NECX;
+ *protocol = RC_TYPE_NEC;
*scancode = RC_SCANCODE_NECX(data[11] << 8 | data[10], data[9]);
*toggle = 0;
return 1;
@@ -1028,7 +1028,7 @@ void saa7134_probe_i2c_ir(struct saa7134_dev *dev)
dev->init_data.name = "BeholdTV";
dev->init_data.get_key = get_key_beholdm6xx;
dev->init_data.ir_codes = RC_MAP_BEHOLD;
- dev->init_data.type = RC_BIT_NECX;
+ dev->init_data.type = RC_BIT_NEC;
info.addr = 0x2d;
break;
case SAA7134_BOARD_AVERMEDIA_CARDBUS_501:
diff --git a/drivers/media/rc/igorplugusb.c b/drivers/media/rc/igorplugusb.c
index cb6d4f1247da..9e3119c94368 100644
--- a/drivers/media/rc/igorplugusb.c
+++ b/drivers/media/rc/igorplugusb.c
@@ -203,8 +203,8 @@ static int igorplugusb_probe(struct usb_interface *intf,
* for the NEC protocol and many others.
*/
rc->allowed_protocols = RC_BIT_ALL_IR_DECODER & ~(RC_BIT_NEC |
- RC_BIT_NECX | RC_BIT_NEC32 | RC_BIT_RC6_6A_20 |
- RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE |
+ RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 |
+ RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE |
RC_BIT_SONY20 | RC_BIT_SANYO);
rc->priv = ir;
diff --git a/drivers/media/rc/img-ir/img-ir-nec.c b/drivers/media/rc/img-ir/img-ir-nec.c
index 044fd42b22a0..56159bb44f9c 100644
--- a/drivers/media/rc/img-ir/img-ir-nec.c
+++ b/drivers/media/rc/img-ir/img-ir-nec.c
@@ -28,28 +28,15 @@ static int img_ir_nec_scancode(int len, u64 raw, u64 enabled_protocols,
addr_inv = (raw >> 8) & 0xff;
data = (raw >> 16) & 0xff;
data_inv = (raw >> 24) & 0xff;
- if ((data_inv ^ data) != 0xff) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: as transmitted, MSBit = first received bit */
- request->scancode = bitrev8(addr) << 24 |
- bitrev8(addr_inv) << 16 |
- bitrev8(data) << 8 |
- bitrev8(data_inv);
- request->protocol = RC_TYPE_NEC32;
- } else if ((addr_inv ^ addr) != 0xff) {
- /* Extended NEC */
- /* scan encoding: AAaaDD */
- request->scancode = addr << 16 |
- addr_inv << 8 |
- data;
- request->protocol = RC_TYPE_NECX;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- request->scancode = addr << 8 |
- data;
- request->protocol = RC_TYPE_NEC;
- }
+
+ /* 32-bit NEC */
+ /* scan encoding: as transmitted, MSBit = first received bit */
+ request->scancode =
+ bitrev8(addr) << 24 |
+ bitrev8(addr_inv) << 16 |
+ bitrev8(data) << 8 |
+ bitrev8(data_inv);
+ request->protocol = RC_TYPE_NEC;
return IMG_IR_SCANCODE;
}
@@ -60,55 +47,16 @@ static int img_ir_nec_filter(const struct rc_scancode_filter *in,
unsigned int addr, addr_inv, data, data_inv;
unsigned int addr_m, addr_inv_m, data_m, data_inv_m;
- data = in->data & 0xff;
- data_m = in->mask & 0xff;
-
- protocols &= RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32;
-
- /*
- * If only one bit is set, we were requested to do an exact
- * protocol. This should be the case for wakeup filters; for
- * normal filters, guess the protocol from the scancode.
- */
- if (!is_power_of_2(protocols)) {
- if ((in->data | in->mask) & 0xff000000)
- protocols = RC_BIT_NEC32;
- else if ((in->data | in->mask) & 0x00ff0000)
- protocols = RC_BIT_NECX;
- else
- protocols = RC_BIT_NEC;
- }
-
- if (protocols == RC_BIT_NEC32) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: as transmitted, MSBit = first received bit */
- addr = bitrev8(in->data >> 24);
- addr_m = bitrev8(in->mask >> 24);
- addr_inv = bitrev8(in->data >> 16);
- addr_inv_m = bitrev8(in->mask >> 16);
- data = bitrev8(in->data >> 8);
- data_m = bitrev8(in->mask >> 8);
- data_inv = bitrev8(in->data >> 0);
- data_inv_m = bitrev8(in->mask >> 0);
- } else if (protocols == RC_BIT_NECX) {
- /* Extended NEC */
- /* scan encoding AAaaDD */
- addr = (in->data >> 16) & 0xff;
- addr_m = (in->mask >> 16) & 0xff;
- addr_inv = (in->data >> 8) & 0xff;
- addr_inv_m = (in->mask >> 8) & 0xff;
- data_inv = data ^ 0xff;
- data_inv_m = data_m;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- addr = (in->data >> 8) & 0xff;
- addr_m = (in->mask >> 8) & 0xff;
- addr_inv = addr ^ 0xff;
- addr_inv_m = addr_m;
- data_inv = data ^ 0xff;
- data_inv_m = data_m;
- }
+ /* 32-bit NEC */
+ /* scan encoding: as transmitted, MSBit = first received bit */
+ addr = bitrev8(in->data >> 24);
+ addr_m = bitrev8(in->mask >> 24);
+ addr_inv = bitrev8(in->data >> 16);
+ addr_inv_m = bitrev8(in->mask >> 16);
+ data = bitrev8(in->data >> 8);
+ data_m = bitrev8(in->mask >> 8);
+ data_inv = bitrev8(in->data >> 0);
+ data_inv_m = bitrev8(in->mask >> 0);
/* raw encoding: ddDDaaAA */
out->data = data_inv << 24 |
@@ -128,7 +76,7 @@ static int img_ir_nec_filter(const struct rc_scancode_filter *in,
* http://wiki.altium.com/display/ADOH/NEC+Infrared+Transmission+Protocol
*/
struct img_ir_decoder img_ir_nec = {
- .type = RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32,
+ .type = RC_BIT_NEC,
.control = {
.decoden = 1,
.code_type = IMG_IR_CODETYPE_PULSEDIST,
diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 3ce850314dca..1f137dfa3eb3 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -49,9 +49,7 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
{
struct nec_dec *data = &dev->raw->nec;
u32 scancode;
- enum rc_type rc_type;
u8 address, not_address, command, not_command;
- bool send_32bits = false;
if (!is_timing_event(ev)) {
if (ev.reset)
@@ -161,39 +159,14 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
command = bitrev8((data->bits >> 8) & 0xff);
not_command = bitrev8((data->bits >> 0) & 0xff);
- if ((command ^ not_command) != 0xff) {
- IR_dprintk(1, "NEC checksum error: received 0x%08x\n",
- data->bits);
- send_32bits = true;
- }
-
- if (send_32bits) {
- /* NEC transport, but modified protocol, used by at
- * least Apple and TiVo remotes */
- scancode = not_address << 24 |
- address << 16 |
- not_command << 8 |
- command;
- IR_dprintk(1, "NEC (modified) scancode 0x%08x\n", scancode);
- rc_type = RC_TYPE_NEC32;
- } else if ((address ^ not_address) != 0xff) {
- /* Extended NEC */
- scancode = address << 16 |
- not_address << 8 |
- command;
- IR_dprintk(1, "NEC (Ext) scancode 0x%06x\n", scancode);
- rc_type = RC_TYPE_NECX;
- } else {
- /* Normal NEC */
- scancode = address << 8 | command;
- IR_dprintk(1, "NEC scancode 0x%04x\n", scancode);
- rc_type = RC_TYPE_NEC;
- }
+ scancode = RC_SCANCODE_NEC32(address << 24 | not_address << 16 |
+ command << 8 | not_command);
+ IR_dprintk(1, "NEC scancode 0x%08x\n", scancode);
if (data->is_nec_x)
data->necx_repeat = true;
- rc_keydown(dev, rc_type, scancode, 0);
+ rc_keydown(dev, RC_TYPE_NEC, scancode, 0);
data->state = STATE_INACTIVE;
return 0;
}
@@ -214,27 +187,11 @@ static u32 ir_nec_scancode_to_raw(enum rc_type protocol, u32 scancode)
{
unsigned int addr, addr_inv, data, data_inv;
- data = scancode & 0xff;
-
- if (protocol == RC_TYPE_NEC32) {
- /* 32-bit NEC (used by Apple and TiVo remotes) */
- /* scan encoding: aaAAddDD */
- addr_inv = (scancode >> 24) & 0xff;
- addr = (scancode >> 16) & 0xff;
- data_inv = (scancode >> 8) & 0xff;
- } else if (protocol == RC_TYPE_NECX) {
- /* Extended NEC */
- /* scan encoding AAaaDD */
- addr = (scancode >> 16) & 0xff;
- addr_inv = (scancode >> 8) & 0xff;
- data_inv = data ^ 0xff;
- } else {
- /* Normal NEC */
- /* scan encoding: AADD */
- addr = (scancode >> 8) & 0xff;
- addr_inv = addr ^ 0xff;
- data_inv = data ^ 0xff;
- }
+ /* 32-bit NEC, scan encoding: aaAAddDD */
+ addr_inv = (scancode >> 24) & 0xff;
+ addr = (scancode >> 16) & 0xff;
+ data_inv = (scancode >> 8) & 0xff;
+ data = (scancode >> 0) & 0xff;
/* raw encoding: ddDDaaAA */
return data_inv << 24 |
@@ -285,7 +242,7 @@ static int ir_nec_encode(enum rc_type protocol, u32 scancode,
}
static struct ir_raw_handler nec_handler = {
- .protocols = RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32,
+ .protocols = RC_BIT_NEC,
.decode = ir_nec_decode,
.encode = ir_nec_encode,
};
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 8355f86a460b..881af208a19a 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -326,6 +326,49 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
}
/**
+ * guess_protocol() - heuristics to guess the protocol for a scancode
+ * @rdev: the struct rc_dev device descriptor
+ * @return: the guessed RC_TYPE_* protocol
+ *
+ * Internal routine to guess the current IR protocol for legacy ioctls.
+ */
+static inline enum rc_type guess_protocol(struct rc_dev *rdev)
+{
+ struct rc_map *rc_map = &rdev->rc_map;
+
+ if (hweight64(rdev->enabled_protocols) == 1)
+ return rc_bitmap_to_type(rdev->enabled_protocols);
+ else if (hweight64(rdev->allowed_protocols) == 1)
+ return rc_bitmap_to_type(rdev->allowed_protocols);
+ else
+ return rc_map->rc_type;
+}
+
+/**
+ * to_nec32() - helper function to try to convert misc NEC scancodes to NEC32
+ * @orig: original scancode
+ * @return: NEC32 scancode
+ *
+ * This helper routine is used to provide backwards compatibility.
+ */
+static u64 to_nec32(u64 orig)
+{
+ u8 b3 = (u8)(orig >> 16);
+ u8 b2 = (u8)(orig >> 8);
+ u8 b1 = (u8)(orig >> 0);
+
+ if (orig <= 0xffff)
+ /* Plain old NEC */
+ return b2 << 24 | ((u8)~b2) << 16 | b1 << 8 | ((u8)~b1);
+ else if (orig <= 0xffffff)
+ /* NEC extended */
+ return b3 << 24 | b2 << 16 | b1 << 8 | ((u8)~b1);
+ else
+ /* NEC32 */
+ return orig;
+}
+
+/**
* ir_setkeycode() - set a keycode in the scancode->keycode table
* @idev: the struct input_dev device descriptor
* @scancode: the desired scancode
@@ -358,6 +401,9 @@ static int ir_setkeycode(struct input_dev *idev,
if (retval)
goto out;
+ if (guess_protocol(rdev) == RC_TYPE_NEC)
+ scancode = to_nec32(scancode);
+
index = ir_establish_scancode(rdev, rc_map, scancode, true);
if (index >= rc_map->len) {
retval = -ENOMEM;
@@ -398,7 +444,10 @@ static int ir_setkeytable(struct rc_dev *dev,
for (i = 0; i < from->size; i++) {
index = ir_establish_scancode(dev, rc_map,
- from->scan[i].scancode, false);
+ from->rc_type == RC_TYPE_NEC ?
+ to_nec32(from->scan[i].scancode) :
+ from->scan[i].scancode,
+ false);
if (index >= rc_map->len) {
rc = -ENOMEM;
break;
@@ -472,6 +521,8 @@ static int ir_getkeycode(struct input_dev *idev,
if (retval)
goto out;
+ if (guess_protocol(rdev) == RC_TYPE_NEC)
+ scancode = to_nec32(scancode);
index = ir_lookup_by_scancode(rc_map, scancode);
}
@@ -668,7 +719,6 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
led_trigger_event(led_feedback, LED_FULL);
}
-
input_sync(dev->input_dev);
}
@@ -741,7 +791,7 @@ static int rc_validate_filter(struct rc_dev *dev,
[RC_TYPE_SONY15] = 0xff007f,
[RC_TYPE_SONY20] = 0x1fff7f,
[RC_TYPE_JVC] = 0xffff,
- [RC_TYPE_NEC32] = 0xffffffff,
+ [RC_TYPE_NEC] = 0xffffffff,
[RC_TYPE_SANYO] = 0x1fffff,
[RC_TYPE_MCIR2_KBD] = 0xffff,
[RC_TYPE_MCIR2_MSE] = 0x1fffff,
@@ -756,9 +806,6 @@ static int rc_validate_filter(struct rc_dev *dev,
enum rc_type protocol = dev->wakeup_protocol;
switch (protocol) {
- case RC_TYPE_NEC:
- case RC_TYPE_NECX:
- return -EINVAL;
case RC_TYPE_RC6_MCE:
if ((s & 0xffff0000) != 0x800f0000)
return -EINVAL;
@@ -857,9 +904,7 @@ static const struct {
{ RC_BIT_UNKNOWN, "unknown", NULL },
{ RC_BIT_RC5 |
RC_BIT_RC5X_20, "rc-5", "ir-rc5-decoder" },
- { RC_BIT_NEC |
- RC_BIT_NECX |
- RC_BIT_NEC32, "nec", "ir-nec-decoder" },
+ { RC_BIT_NEC, "nec", "ir-nec-decoder" },
{ RC_BIT_RC6_0 |
RC_BIT_RC6_6A_20 |
RC_BIT_RC6_6A_24 |
@@ -1309,7 +1354,7 @@ static const char * const proto_variant_names[] = {
[RC_TYPE_SONY12] = "sony-12",
[RC_TYPE_SONY15] = "sony-15",
[RC_TYPE_SONY20] = "sony-20",
- [RC_TYPE_NEC32] = "nec",
+ [RC_TYPE_NEC] = "nec",
[RC_TYPE_SANYO] = "sanyo",
[RC_TYPE_MCIR2_KBD] = "mcir2-kbd",
[RC_TYPE_MCIR2_MSE] = "mcir2-mse",
diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 6ef0e7232356..914eab8df819 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -714,7 +714,7 @@ wbcir_shutdown(struct pnp_dev *device)
proto = IR_PROTOCOL_RC5;
break;
- case RC_TYPE_NEC32:
+ case RC_TYPE_NEC:
mask[0] = bitrev8(mask_sc);
mask[1] = bitrev8(mask_sc >> 8);
mask[2] = bitrev8(mask_sc >> 16);
diff --git a/drivers/media/usb/au0828/au0828-input.c b/drivers/media/usb/au0828/au0828-input.c
index 9ec919c68482..545741feff2f 100644
--- a/drivers/media/usb/au0828/au0828-input.c
+++ b/drivers/media/usb/au0828/au0828-input.c
@@ -343,8 +343,7 @@ int au0828_rc_register(struct au0828_dev *dev)
rc->input_id.product = le16_to_cpu(dev->usbdev->descriptor.idProduct);
rc->dev.parent = &dev->usbdev->dev;
rc->driver_name = "au0828-input";
- rc->allowed_protocols = RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32 |
- RC_BIT_RC5;
+ rc->allowed_protocols = RC_BIT_NEC | RC_BIT_RC5;
/* all done */
err = rc_register_device(rc);
diff --git a/drivers/media/usb/dvb-usb-v2/af9015.c b/drivers/media/usb/dvb-usb-v2/af9015.c
index caa1e6101f58..ef1800206ca6 100644
--- a/drivers/media/usb/dvb-usb-v2/af9015.c
+++ b/drivers/media/usb/dvb-usb-v2/af9015.c
@@ -1218,7 +1218,6 @@ static int af9015_rc_query(struct dvb_usb_device *d)
/* Only process key if canary killed */
if (buf[16] != 0xff && buf[0] != 0x01) {
- enum rc_type proto;
dev_dbg(&d->udev->dev, "%s: key pressed %*ph\n",
__func__, 4, buf + 12);
@@ -1229,28 +1228,11 @@ static int af9015_rc_query(struct dvb_usb_device *d)
/* Remember this key */
memcpy(state->rc_last, &buf[12], 4);
- if (buf[14] == (u8) ~buf[15]) {
- if (buf[12] == (u8) ~buf[13]) {
- /* NEC */
- state->rc_keycode = RC_SCANCODE_NEC(buf[12],
- buf[14]);
- proto = RC_TYPE_NEC;
- } else {
- /* NEC extended*/
- state->rc_keycode = RC_SCANCODE_NECX(buf[12] << 8 |
- buf[13],
- buf[14]);
- proto = RC_TYPE_NECX;
- }
- } else {
- /* 32 bit NEC */
- state->rc_keycode = RC_SCANCODE_NEC32(buf[12] << 24 |
- buf[13] << 16 |
- buf[14] << 8 |
- buf[15]);
- proto = RC_TYPE_NEC32;
- }
- rc_keydown(d->rc_dev, proto, state->rc_keycode, 0);
+ state->rc_keycode = RC_SCANCODE_NEC32(buf[12] << 24 |
+ buf[13] << 16 |
+ buf[14] << 8 |
+ buf[15]);
+ rc_keydown(d->rc_dev, RC_TYPE_NEC, state->rc_keycode, 0);
} else {
dev_dbg(&d->udev->dev, "%s: no key press\n", __func__);
/* Invalidate last keypress */
@@ -1317,7 +1299,7 @@ static int af9015_get_rc_config(struct dvb_usb_device *d, struct dvb_usb_rc *rc)
if (!rc->map_name)
rc->map_name = RC_MAP_EMPTY;
- rc->allowed_protos = RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32;
+ rc->allowed_protos = RC_BIT_NEC;
rc->query = af9015_rc_query;
rc->interval = 500;
diff --git a/drivers/media/usb/dvb-usb-v2/af9035.c b/drivers/media/usb/dvb-usb-v2/af9035.c
index 4df9486e19b9..ccb2b4c673db 100644
--- a/drivers/media/usb/dvb-usb-v2/af9035.c
+++ b/drivers/media/usb/dvb-usb-v2/af9035.c
@@ -1828,8 +1828,6 @@ static int af9035_rc_query(struct dvb_usb_device *d)
{
struct usb_interface *intf = d->intf;
int ret;
- enum rc_type proto;
- u32 key;
u8 buf[4];
struct usb_req req = { CMD_IR_GET, 0, 0, NULL, 4, buf };
@@ -1839,26 +1837,12 @@ static int af9035_rc_query(struct dvb_usb_device *d)
else if (ret < 0)
goto err;
- if ((buf[2] + buf[3]) == 0xff) {
- if ((buf[0] + buf[1]) == 0xff) {
- /* NEC standard 16bit */
- key = RC_SCANCODE_NEC(buf[0], buf[2]);
- proto = RC_TYPE_NEC;
- } else {
- /* NEC extended 24bit */
- key = RC_SCANCODE_NECX(buf[0] << 8 | buf[1], buf[2]);
- proto = RC_TYPE_NECX;
- }
- } else {
- /* NEC full code 32bit */
- key = RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 |
- buf[2] << 8 | buf[3]);
- proto = RC_TYPE_NEC32;
- }
-
dev_dbg(&intf->dev, "%*ph\n", 4, buf);
- rc_keydown(d->rc_dev, proto, key, 0);
+ rc_keydown(d->rc_dev, RC_TYPE_NEC,
+ RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 |
+ buf[2] << 8 | buf[3]),
+ 0);
return 0;
@@ -1881,8 +1865,7 @@ static int af9035_get_rc_config(struct dvb_usb_device *d, struct dvb_usb_rc *rc)
switch (state->ir_type) {
case 0: /* NEC */
default:
- rc->allowed_protos = RC_BIT_NEC | RC_BIT_NECX |
- RC_BIT_NEC32;
+ rc->allowed_protos = RC_BIT_NEC;
break;
case 1: /* RC6 */
rc->allowed_protos = RC_BIT_RC6_MCE;
diff --git a/drivers/media/usb/dvb-usb-v2/az6007.c b/drivers/media/usb/dvb-usb-v2/az6007.c
index 50c07fe7dacb..7e3827843042 100644
--- a/drivers/media/usb/dvb-usb-v2/az6007.c
+++ b/drivers/media/usb/dvb-usb-v2/az6007.c
@@ -208,31 +208,18 @@ static int az6007_rc_query(struct dvb_usb_device *d)
{
struct az6007_device_state *st = d_to_priv(d);
unsigned code;
- enum rc_type proto;
az6007_read(d, AZ6007_READ_IR, 0, 0, st->data, 10);
if (st->data[1] == 0x44)
return 0;
- if ((st->data[3] ^ st->data[4]) == 0xff) {
- if ((st->data[1] ^ st->data[2]) == 0xff) {
- code = RC_SCANCODE_NEC(st->data[1], st->data[3]);
- proto = RC_TYPE_NEC;
- } else {
- code = RC_SCANCODE_NECX(st->data[1] << 8 | st->data[2],
- st->data[3]);
- proto = RC_TYPE_NECX;
- }
- } else {
- code = RC_SCANCODE_NEC32(st->data[1] << 24 |
- st->data[2] << 16 |
- st->data[3] << 8 |
- st->data[4]);
- proto = RC_TYPE_NEC32;
- }
+ code = RC_SCANCODE_NEC32(st->data[1] << 24 |
+ st->data[2] << 16 |
+ st->data[3] << 8 |
+ st->data[4]);
- rc_keydown(d->rc_dev, proto, code, st->data[5]);
+ rc_keydown(d->rc_dev, RC_TYPE_NEC, code, st->data[5]);
return 0;
}
@@ -241,7 +228,7 @@ static int az6007_get_rc_config(struct dvb_usb_device *d, struct dvb_usb_rc *rc)
{
pr_debug("Getting az6007 Remote Control properties\n");
- rc->allowed_protos = RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32;
+ rc->allowed_protos = RC_BIT_NEC;
rc->query = az6007_rc_query;
rc->interval = 400;
diff --git a/drivers/media/usb/dvb-usb-v2/lmedm04.c b/drivers/media/usb/dvb-usb-v2/lmedm04.c
index 924adfdb660d..860e9cf2ee4e 100644
--- a/drivers/media/usb/dvb-usb-v2/lmedm04.c
+++ b/drivers/media/usb/dvb-usb-v2/lmedm04.c
@@ -349,8 +349,7 @@ static void lme2510_int_response(struct urb *lme_urb)
ibuf[5]);
deb_info(1, "INT Key = 0x%08x", key);
- rc_keydown(adap_to_d(adap)->rc_dev, RC_TYPE_NEC32, key,
- 0);
+ rc_keydown(adap_to_d(adap)->rc_dev, RC_TYPE_NEC, key, 0);
break;
case 0xbb:
switch (st->tuner_config) {
@@ -1233,7 +1232,7 @@ static int lme2510_get_stream_config(struct dvb_frontend *fe, u8 *ts_type,
static int lme2510_get_rc_config(struct dvb_usb_device *d,
struct dvb_usb_rc *rc)
{
- rc->allowed_protos = RC_BIT_NEC32;
+ rc->allowed_protos = RC_BIT_NEC;
return 0;
}
diff --git a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
index e16ca07acf1d..06219abaef7b 100644
--- a/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
+++ b/drivers/media/usb/dvb-usb-v2/rtl28xxu.c
@@ -1597,7 +1597,7 @@ static int rtl2831u_rc_query(struct dvb_usb_device *d)
int ret, i;
struct rtl28xxu_dev *dev = d->priv;
u8 buf[5];
- u32 rc_code;
+ u64 rc_code;
struct rtl28xxu_reg_val rc_nec_tab[] = {
{ 0x3033, 0x80 },
{ 0x3020, 0x43 },
@@ -1631,27 +1631,12 @@ static int rtl2831u_rc_query(struct dvb_usb_device *d)
goto err;
if (buf[4] & 0x01) {
- enum rc_type proto;
+ rc_code = RC_SCANCODE_NEC32(buf[0] << 24 |
+ buf[1] << 16 |
+ buf[2] << 8 |
+ buf[3]);
- if (buf[2] == (u8) ~buf[3]) {
- if (buf[0] == (u8) ~buf[1]) {
- /* NEC standard (16 bit) */
- rc_code = RC_SCANCODE_NEC(buf[0], buf[2]);
- proto = RC_TYPE_NEC;
- } else {
- /* NEC extended (24 bit) */
- rc_code = RC_SCANCODE_NECX(buf[0] << 8 | buf[1],
- buf[2]);
- proto = RC_TYPE_NECX;
- }
- } else {
- /* NEC full (32 bit) */
- rc_code = RC_SCANCODE_NEC32(buf[0] << 24 | buf[1] << 16 |
- buf[2] << 8 | buf[3]);
- proto = RC_TYPE_NEC32;
- }
-
- rc_keydown(d->rc_dev, proto, rc_code, 0);
+ rc_keydown(d->rc_dev, RC_TYPE_NEC, rc_code, 0);
ret = rtl28xxu_wr_reg(d, SYS_IRRC_SR, 1);
if (ret)
@@ -1673,7 +1658,7 @@ static int rtl2831u_get_rc_config(struct dvb_usb_device *d,
struct dvb_usb_rc *rc)
{
rc->map_name = RC_MAP_EMPTY;
- rc->allowed_protos = RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32;
+ rc->allowed_protos = RC_BIT_NEC;
rc->query = rtl2831u_rc_query;
rc->interval = 400;
diff --git a/drivers/media/usb/dvb-usb/dib0700_core.c b/drivers/media/usb/dvb-usb/dib0700_core.c
index 08acdd32e412..3267bc7ea9c5 100644
--- a/drivers/media/usb/dvb-usb/dib0700_core.c
+++ b/drivers/media/usb/dvb-usb/dib0700_core.c
@@ -734,6 +734,7 @@ static void dib0700_rc_urb_completion(struct urb *purb)
switch (d->props.rc.core.protocol) {
case RC_BIT_NEC:
+ protocol = RC_TYPE_NEC;
toggle = 0;
/* NEC protocol sends repeat code as 0 0 0 FF */
@@ -746,26 +747,10 @@ static void dib0700_rc_urb_completion(struct urb *purb)
goto resubmit;
}
- if ((poll_reply->nec.data ^ poll_reply->nec.not_data) != 0xff) {
- deb_data("NEC32 protocol\n");
- keycode = RC_SCANCODE_NEC32(poll_reply->nec.system << 24 |
- poll_reply->nec.not_system << 16 |
- poll_reply->nec.data << 8 |
- poll_reply->nec.not_data);
- protocol = RC_TYPE_NEC32;
- } else if ((poll_reply->nec.system ^ poll_reply->nec.not_system) != 0xff) {
- deb_data("NEC extended protocol\n");
- keycode = RC_SCANCODE_NECX(poll_reply->nec.system << 8 |
- poll_reply->nec.not_system,
- poll_reply->nec.data);
-
- protocol = RC_TYPE_NECX;
- } else {
- deb_data("NEC normal protocol\n");
- keycode = RC_SCANCODE_NEC(poll_reply->nec.system,
- poll_reply->nec.data);
- protocol = RC_TYPE_NEC;
- }
+ keycode = RC_SCANCODE_NEC32(poll_reply->nec.system << 24 |
+ poll_reply->nec.not_system << 16 |
+ poll_reply->nec.data << 8 |
+ poll_reply->nec.not_data);
break;
default:
diff --git a/drivers/media/usb/dvb-usb/dtt200u.c b/drivers/media/usb/dvb-usb/dtt200u.c
index fcbff7fb0c4e..ffe987f72590 100644
--- a/drivers/media/usb/dvb-usb/dtt200u.c
+++ b/drivers/media/usb/dvb-usb/dtt200u.c
@@ -89,7 +89,6 @@ static int dtt200u_pid_filter(struct dvb_usb_adapter *adap, int index, u16 pid,
static int dtt200u_rc_query(struct dvb_usb_device *d)
{
struct dtt200u_state *st = d->priv;
- u32 scancode;
int ret;
mutex_lock(&d->data_mutex);
@@ -100,23 +99,13 @@ static int dtt200u_rc_query(struct dvb_usb_device *d)
goto ret;
if (st->data[0] == 1) {
- enum rc_type proto = RC_TYPE_NEC;
-
- scancode = st->data[1];
- if ((u8) ~st->data[1] != st->data[2]) {
- /* Extended NEC */
- scancode = scancode << 8;
- scancode |= st->data[2];
- proto = RC_TYPE_NECX;
- }
- scancode = scancode << 8;
- scancode |= st->data[3];
-
- /* Check command checksum is ok */
- if ((u8) ~st->data[3] == st->data[4])
- rc_keydown(d->rc_dev, proto, scancode, 0);
- else
- rc_keyup(d->rc_dev);
+ u32 scancode;
+
+ scancode = RC_SCANCODE_NEC32((st->data[1] << 24) |
+ (st->data[2] << 16) |
+ (st->data[3] << 8) |
+ (st->data[4] << 0));
+ rc_keydown(d->rc_dev, RC_TYPE_NEC, scancode, 0);
} else if (st->data[0] == 2) {
rc_repeat(d->rc_dev);
} else {
diff --git a/drivers/media/usb/em28xx/em28xx-input.c b/drivers/media/usb/em28xx/em28xx-input.c
index eba75736e654..93c3fca7849a 100644
--- a/drivers/media/usb/em28xx/em28xx-input.c
+++ b/drivers/media/usb/em28xx/em28xx-input.c
@@ -259,21 +259,11 @@ static int em2874_polling_getkey(struct em28xx_IR *ir,
break;
case RC_BIT_NEC:
- poll_result->scancode = msg[1] << 8 | msg[2];
- if ((msg[3] ^ msg[4]) != 0xff) { /* 32 bits NEC */
- poll_result->protocol = RC_TYPE_NEC32;
- poll_result->scancode = RC_SCANCODE_NEC32((msg[1] << 24) |
- (msg[2] << 16) |
- (msg[3] << 8) |
- (msg[4]));
- } else if ((msg[1] ^ msg[2]) != 0xff) { /* 24 bits NEC */
- poll_result->protocol = RC_TYPE_NECX;
- poll_result->scancode = RC_SCANCODE_NECX(msg[1] << 8 |
- msg[2], msg[3]);
- } else { /* Normal NEC */
- poll_result->protocol = RC_TYPE_NEC;
- poll_result->scancode = RC_SCANCODE_NEC(msg[1], msg[3]);
- }
+ poll_result->protocol = RC_TYPE_NEC;
+ poll_result->scancode = RC_SCANCODE_NEC32((msg[1] << 24) |
+ (msg[2] << 16) |
+ (msg[3] << 8) |
+ (msg[4]));
break;
case RC_BIT_RC6_0:
@@ -780,7 +770,7 @@ static int em28xx_ir_init(struct em28xx *dev)
case CHIP_ID_EM28178:
ir->get_key = em2874_polling_getkey;
rc->allowed_protocols = RC_BIT_RC5 | RC_BIT_NEC |
- RC_BIT_NECX | RC_BIT_NEC32 | RC_BIT_RC6_0;
+ RC_BIT_RC6_0;
break;
default:
err = -ENODEV;
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index 1a815a572fa1..e5d0559dc523 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -24,8 +24,6 @@
* @RC_TYPE_SONY15: Sony 15 bit protocol
* @RC_TYPE_SONY20: Sony 20 bit protocol
* @RC_TYPE_NEC: NEC protocol
- * @RC_TYPE_NECX: Extended NEC protocol
- * @RC_TYPE_NEC32: NEC 32 bit protocol
* @RC_TYPE_SANYO: Sanyo protocol
* @RC_TYPE_MCIR2_KBD: RC6-ish MCE keyboard
* @RC_TYPE_MCIR2_MSE: RC6-ish MCE mouse
@@ -49,21 +47,20 @@ enum rc_type {
RC_TYPE_SONY15 = 7,
RC_TYPE_SONY20 = 8,
RC_TYPE_NEC = 9,
- RC_TYPE_NECX = 10,
- RC_TYPE_NEC32 = 11,
- RC_TYPE_SANYO = 12,
- RC_TYPE_MCIR2_KBD = 13,
- RC_TYPE_MCIR2_MSE = 14,
- RC_TYPE_RC6_0 = 15,
- RC_TYPE_RC6_6A_20 = 16,
- RC_TYPE_RC6_6A_24 = 17,
- RC_TYPE_RC6_6A_32 = 18,
- RC_TYPE_RC6_MCE = 19,
- RC_TYPE_SHARP = 20,
- RC_TYPE_XMP = 21,
- RC_TYPE_CEC = 22,
+ RC_TYPE_SANYO = 10,
+ RC_TYPE_MCIR2_KBD = 11,
+ RC_TYPE_MCIR2_MSE = 12,
+ RC_TYPE_RC6_0 = 13,
+ RC_TYPE_RC6_6A_20 = 14,
+ RC_TYPE_RC6_6A_24 = 15,
+ RC_TYPE_RC6_6A_32 = 16,
+ RC_TYPE_RC6_MCE = 17,
+ RC_TYPE_SHARP = 18,
+ RC_TYPE_XMP = 19,
+ RC_TYPE_CEC = 20,
};
+#define rc_bitmap_to_type(x) (fls64(x) - 1)
#define RC_BIT_NONE 0ULL
#define RC_BIT_UNKNOWN BIT_ULL(RC_TYPE_UNKNOWN)
#define RC_BIT_OTHER BIT_ULL(RC_TYPE_OTHER)
@@ -75,8 +72,6 @@ enum rc_type {
#define RC_BIT_SONY15 BIT_ULL(RC_TYPE_SONY15)
#define RC_BIT_SONY20 BIT_ULL(RC_TYPE_SONY20)
#define RC_BIT_NEC BIT_ULL(RC_TYPE_NEC)
-#define RC_BIT_NECX BIT_ULL(RC_TYPE_NECX)
-#define RC_BIT_NEC32 BIT_ULL(RC_TYPE_NEC32)
#define RC_BIT_SANYO BIT_ULL(RC_TYPE_SANYO)
#define RC_BIT_MCIR2_KBD BIT_ULL(RC_TYPE_MCIR2_KBD)
#define RC_BIT_MCIR2_MSE BIT_ULL(RC_TYPE_MCIR2_MSE)
@@ -93,7 +88,7 @@ enum rc_type {
RC_BIT_RC5 | RC_BIT_RC5X_20 | RC_BIT_RC5_SZ | \
RC_BIT_JVC | \
RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
- RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32 | \
+ RC_BIT_NEC | \
RC_BIT_SANYO | \
RC_BIT_MCIR2_KBD | RC_BIT_MCIR2_MSE | \
RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
@@ -104,7 +99,7 @@ enum rc_type {
(RC_BIT_RC5 | RC_BIT_RC5X_20 | RC_BIT_RC5_SZ | \
RC_BIT_JVC | \
RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
- RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32 | \
+ RC_BIT_NEC | \
RC_BIT_SANYO | RC_BIT_MCIR2_KBD | RC_BIT_MCIR2_MSE | \
RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE | RC_BIT_SHARP | \
@@ -114,7 +109,7 @@ enum rc_type {
(RC_BIT_RC5 | RC_BIT_RC5X_20 | RC_BIT_RC5_SZ | \
RC_BIT_JVC | \
RC_BIT_SONY12 | RC_BIT_SONY15 | RC_BIT_SONY20 | \
- RC_BIT_NEC | RC_BIT_NECX | RC_BIT_NEC32 | \
+ RC_BIT_NEC | \
RC_BIT_SANYO | RC_BIT_MCIR2_KBD | RC_BIT_MCIR2_MSE | \
RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | \
RC_BIT_RC6_6A_32 | RC_BIT_RC6_MCE | \
@@ -122,13 +117,20 @@ enum rc_type {
#define RC_SCANCODE_UNKNOWN(x) (x)
#define RC_SCANCODE_OTHER(x) (x)
-#define RC_SCANCODE_NEC(addr, cmd) (((addr) << 8) | (cmd))
-#define RC_SCANCODE_NECX(addr, cmd) (((addr) << 8) | (cmd))
-#define RC_SCANCODE_NEC32(data) ((data) & 0xffffffff)
#define RC_SCANCODE_RC5(sys, cmd) (((sys) << 8) | (cmd))
#define RC_SCANCODE_RC5_SZ(sys, cmd) (((sys) << 8) | (cmd))
#define RC_SCANCODE_RC6_0(sys, cmd) (((sys) << 8) | (cmd))
#define RC_SCANCODE_RC6_6A(vendor, sys, cmd) (((vendor) << 16) | ((sys) << 8) | (cmd))
+#define RC_SCANCODE_NEC(addr, cmd) \
+ ((((addr) & 0xff) << 24) | \
+ ((~(addr) & 0xff) << 16) | \
+ (((cmd) & 0xff) << 8 ) | \
+ ((~(cmd) & 0xff) << 0 ))
+#define RC_SCANCODE_NECX(addr, cmd) \
+ ((((addr) & 0xffff) << 16) | \
+ (((cmd) & 0x00ff) << 8) | \
+ ((~(cmd) & 0x00ff) << 0))
+#define RC_SCANCODE_NEC32(data) ((data) & 0xffffffff)
/**
* struct rc_map_table - represents a scancode/keycode pair
^ permalink raw reply related [flat|nested] 22+ messages in thread
* [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
` (4 preceding siblings ...)
2017-04-27 20:34 ` [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes David Härdeman
@ 2017-04-27 20:34 ` David Härdeman
2017-04-28 11:31 ` Mauro Carvalho Chehab
2017-04-28 11:40 ` Sean Young
5 siblings, 2 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-27 20:34 UTC (permalink / raw)
To: linux-media; +Cc: mchehab, sean
It is currently impossible to distinguish between scancodes which have
been generated using different protocols (and scancodes can, and will,
overlap).
For example:
RC5 message to address 0x00, command 0x03 has scancode 0x00000503
JVC message to address 0x00, command 0x03 has scancode 0x00000503
It is only possible to distinguish (and parse) scancodes by known the
scancode *and* the protocol.
Setting and getting keycodes in the input subsystem used to be done via
the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
and one for the keycode).
The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
which uses the following struct:
struct input_keymap_entry {
__u8 flags;
__u8 len;
__u16 index;
__u32 keycode;
__u8 scancode[32];
};
(scancode can of course be even bigger, thanks to the len member).
This patch changes how the "input_keymap_entry" struct is interpreted
by rc-core by casting it to "rc_keymap_entry":
struct rc_scancode {
__u16 protocol;
__u16 reserved[3];
__u64 scancode;
}
struct rc_keymap_entry {
__u8 flags;
__u8 len;
__u16 index;
__u32 keycode;
union {
struct rc_scancode rc;
__u8 raw[32];
};
};
The u64 scancode member is large enough for all current protocols and it
would be possible to extend it in the future should it be necessary for
some exotic protocol.
The main advantage with this change is that the protocol is made explicit,
which means that we're not throwing away data (the protocol type).
This also means that struct rc_map no longer hardcodes the protocol, meaning
that keytables with mixed entries are possible.
Heuristics are also added to hopefully do the right thing with older
ioctls in order to preserve backwards compatibility.
Note that the heuristics are not 100% guaranteed to get things right.
That is unavoidable since the protocol information simply isn't there
when userspace calls the previous ioctl() types.
However, that is somewhat mitigated by the fact that the "only"
userspace binary which might need to change is ir-keytable. Userspace
programs which simply consume input events (i.e. the vast majority)
won't have to change.
Signed-off-by: David Härdeman <david@hardeman.nu>
---
drivers/media/rc/ati_remote.c | 1
drivers/media/rc/imon.c | 7 +-
drivers/media/rc/rc-main.c | 188 +++++++++++++++++++++++++++++------------
include/media/rc-core.h | 26 +++++-
include/media/rc-map.h | 5 +
5 files changed, 165 insertions(+), 62 deletions(-)
diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 9cf3e69de16a..cc81b938471f 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -546,6 +546,7 @@ static void ati_remote_input_report(struct urb *urb)
* set, assume this is a scrollwheel up/down event.
*/
wheel_keycode = rc_g_keycode_from_table(ati_remote->rdev,
+ RC_TYPE_OTHER,
scancode & 0x78);
if (wheel_keycode == KEY_RESERVED) {
diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
index 3489010601b5..c724a1a5e9cd 100644
--- a/drivers/media/rc/imon.c
+++ b/drivers/media/rc/imon.c
@@ -1274,14 +1274,15 @@ static u32 imon_remote_key_lookup(struct imon_context *ictx, u32 scancode)
bool is_release_code = false;
/* Look for the initial press of a button */
- keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
+ keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
ictx->rc_toggle = 0x0;
ictx->rc_scancode = scancode;
/* Look for the release of a button */
if (keycode == KEY_RESERVED) {
release = scancode & ~0x4000;
- keycode = rc_g_keycode_from_table(ictx->rdev, release);
+ keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type,
+ release);
if (keycode != KEY_RESERVED)
is_release_code = true;
}
@@ -1310,7 +1311,7 @@ static u32 imon_mce_key_lookup(struct imon_context *ictx, u32 scancode)
scancode = scancode | MCE_KEY_MASK | MCE_TOGGLE_BIT;
ictx->rc_scancode = scancode;
- keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
+ keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
/* not used in mce mode, but make sure we know its false */
ictx->release_code = false;
diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
index 881af208a19a..ad5545301588 100644
--- a/drivers/media/rc/rc-main.c
+++ b/drivers/media/rc/rc-main.c
@@ -105,7 +105,7 @@ EXPORT_SYMBOL_GPL(rc_map_unregister);
static struct rc_map_table empty[] = {
- { 0x2a, KEY_COFFEE },
+ { RC_TYPE_OTHER, 0x2a, KEY_COFFEE },
};
static struct rc_map_list empty_map = {
@@ -121,7 +121,6 @@ static struct rc_map_list empty_map = {
* ir_create_table() - initializes a scancode table
* @rc_map: the rc_map to initialize
* @name: name to assign to the table
- * @rc_type: ir type to assign to the new table
* @size: initial size of the table
* @return: zero on success or a negative error code
*
@@ -129,12 +128,11 @@ static struct rc_map_list empty_map = {
* memory to hold at least the specified number of elements.
*/
static int ir_create_table(struct rc_map *rc_map,
- const char *name, u64 rc_type, size_t size)
+ const char *name, size_t size)
{
rc_map->name = kstrdup(name, GFP_KERNEL);
if (!rc_map->name)
return -ENOMEM;
- rc_map->rc_type = rc_type;
rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
@@ -234,16 +232,20 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
/* Did the user wish to remove the mapping? */
if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
- IR_dprintk(1, "#%d: Deleting scan 0x%04x\n",
- index, rc_map->scan[index].scancode);
+ IR_dprintk(1, "#%d: Deleting proto 0x%04x, scan 0x%08llx\n",
+ index, rc_map->scan[index].protocol,
+ (unsigned long long)rc_map->scan[index].scancode);
rc_map->len--;
memmove(&rc_map->scan[index], &rc_map->scan[index+ 1],
(rc_map->len - index) * sizeof(struct rc_map_table));
} else {
- IR_dprintk(1, "#%d: %s scan 0x%04x with key 0x%04x\n",
+ IR_dprintk(1, "#%d: %s proto 0x%04x, scan 0x%08llx "
+ "with key 0x%04x\n",
index,
old_keycode == KEY_RESERVED ? "New" : "Replacing",
- rc_map->scan[index].scancode, new_keycode);
+ rc_map->scan[index].protocol,
+ (unsigned long long)rc_map->scan[index].scancode,
+ new_keycode);
rc_map->scan[index].keycode = new_keycode;
__set_bit(new_keycode, dev->input_dev->keybit);
}
@@ -270,9 +272,9 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
* ir_establish_scancode() - set a keycode in the scancode->keycode table
* @dev: the struct rc_dev device descriptor
* @rc_map: scancode table to be searched
- * @scancode: the desired scancode
- * @resize: controls whether we allowed to resize the table to
- * accommodate not yet present scancodes
+ * @entry: the entry to be added to the table
+ * @resize: controls whether we are allowed to resize the table to
+ * accomodate not yet present scancodes
* @return: index of the mapping containing scancode in question
* or -1U in case of failure.
*
@@ -282,7 +284,7 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
*/
static unsigned int ir_establish_scancode(struct rc_dev *dev,
struct rc_map *rc_map,
- unsigned int scancode,
+ struct rc_map_table *entry,
bool resize)
{
unsigned int i;
@@ -296,16 +298,27 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
* indicate the valid bits of the scancodes.
*/
if (dev->scancode_mask)
- scancode &= dev->scancode_mask;
+ entry->scancode &= dev->scancode_mask;
- /* First check if we already have a mapping for this ir command */
+ /*
+ * First check if we already have a mapping for this command.
+ * Note that the keytable is sorted first on protocol and second
+ * on scancode (lowest to highest).
+ */
for (i = 0; i < rc_map->len; i++) {
- if (rc_map->scan[i].scancode == scancode)
- return i;
+ if (rc_map->scan[i].protocol < entry->protocol)
+ continue;
+
+ if (rc_map->scan[i].protocol > entry->protocol)
+ break;
- /* Keytable is sorted from lowest to highest scancode */
- if (rc_map->scan[i].scancode >= scancode)
+ if (rc_map->scan[i].scancode < entry->scancode)
+ continue;
+
+ if (rc_map->scan[i].scancode > entry->scancode)
break;
+
+ return i;
}
/* No previous mapping found, we might need to grow the table */
@@ -318,7 +331,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
if (i < rc_map->len)
memmove(&rc_map->scan[i + 1], &rc_map->scan[i],
(rc_map->len - i) * sizeof(struct rc_map_table));
- rc_map->scan[i].scancode = scancode;
+ rc_map->scan[i].scancode = entry->scancode;
+ rc_map->scan[i].protocol = entry->protocol;
rc_map->scan[i].keycode = KEY_RESERVED;
rc_map->len++;
@@ -340,8 +354,10 @@ static inline enum rc_type guess_protocol(struct rc_dev *rdev)
return rc_bitmap_to_type(rdev->enabled_protocols);
else if (hweight64(rdev->allowed_protocols) == 1)
return rc_bitmap_to_type(rdev->allowed_protocols);
+ else if (rc_map->len > 0)
+ return rc_map->scan[0].protocol;
else
- return rc_map->rc_type;
+ return RC_TYPE_OTHER;
}
/**
@@ -384,10 +400,12 @@ static int ir_setkeycode(struct input_dev *idev,
struct rc_dev *rdev = input_get_drvdata(idev);
struct rc_map *rc_map = &rdev->rc_map;
unsigned int index;
- unsigned int scancode;
+ struct rc_map_table entry;
int retval = 0;
unsigned long flags;
+ entry.keycode = ke->keycode;
+
spin_lock_irqsave(&rc_map->lock, flags);
if (ke->flags & INPUT_KEYMAP_BY_INDEX) {
@@ -396,19 +414,42 @@ static int ir_setkeycode(struct input_dev *idev,
retval = -EINVAL;
goto out;
}
- } else {
+ } else if (ke->len == sizeof(int)) {
+ /* Old EVIOCSKEYCODE[_V2] ioctl */
+ u32 scancode;
retval = input_scancode_to_scalar(ke, &scancode);
if (retval)
goto out;
- if (guess_protocol(rdev) == RC_TYPE_NEC)
- scancode = to_nec32(scancode);
+ entry.scancode = scancode;
+ entry.protocol = guess_protocol(rdev);
+ if (entry.protocol == RC_TYPE_NEC)
+ entry.scancode = to_nec32(scancode);
- index = ir_establish_scancode(rdev, rc_map, scancode, true);
+ index = ir_establish_scancode(rdev, rc_map, &entry, true);
if (index >= rc_map->len) {
retval = -ENOMEM;
goto out;
}
+ } else if (ke->len == sizeof(struct rc_scancode)) {
+ /* New EVIOCSKEYCODE_V2 ioctl */
+ const struct rc_keymap_entry *rke = (struct rc_keymap_entry *)ke;
+ entry.protocol = rke->rc.protocol;
+ entry.scancode = rke->rc.scancode;
+
+ if (rke->rc.reserved[0] || rke->rc.reserved[1] || rke->rc.reserved[2]) {
+ retval = -EINVAL;
+ goto out;
+ }
+
+ index = ir_establish_scancode(rdev, rc_map, &entry, true);
+ if (index >= rc_map->len) {
+ retval = -ENOMEM;
+ goto out;
+ }
+ } else {
+ retval = -EINVAL;
+ goto out;
}
*old_keycode = ir_update_mapping(rdev, rc_map, index, ke->keycode);
@@ -431,11 +472,11 @@ static int ir_setkeytable(struct rc_dev *dev,
const struct rc_map *from)
{
struct rc_map *rc_map = &dev->rc_map;
+ struct rc_map_table entry;
unsigned int i, index;
int rc;
- rc = ir_create_table(rc_map, from->name,
- from->rc_type, from->size);
+ rc = ir_create_table(rc_map, from->name, from->size);
if (rc)
return rc;
@@ -443,18 +484,19 @@ static int ir_setkeytable(struct rc_dev *dev,
rc_map->size, rc_map->alloc);
for (i = 0; i < from->size; i++) {
- index = ir_establish_scancode(dev, rc_map,
- from->rc_type == RC_TYPE_NEC ?
- to_nec32(from->scan[i].scancode) :
- from->scan[i].scancode,
- false);
+ if (from->rc_type == RC_TYPE_NEC)
+ entry.scancode = to_nec32(from->scan[i].scancode);
+ else
+ entry.scancode = from->scan[i].scancode;
+
+ entry.protocol = from->rc_type;
+ index = ir_establish_scancode(dev, rc_map, &entry, false);
if (index >= rc_map->len) {
rc = -ENOMEM;
break;
}
- ir_update_mapping(dev, rc_map, index,
- from->scan[i].keycode);
+ ir_update_mapping(dev, rc_map, index, from->scan[i].keycode);
}
if (rc)
@@ -466,6 +508,7 @@ static int ir_setkeytable(struct rc_dev *dev,
/**
* ir_lookup_by_scancode() - locate mapping by scancode
* @rc_map: the struct rc_map to search
+ * @protocol: protocol to look for in the table
* @scancode: scancode to look for in the table
* @return: index in the table, -1U if not found
*
@@ -473,17 +516,24 @@ static int ir_setkeytable(struct rc_dev *dev,
* given scancode.
*/
static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
- unsigned int scancode)
+ u16 protocol, u64 scancode)
{
int start = 0;
int end = rc_map->len - 1;
int mid;
+ struct rc_map_table *m;
while (start <= end) {
mid = (start + end) / 2;
- if (rc_map->scan[mid].scancode < scancode)
+ m = &rc_map->scan[mid];
+
+ if (m->protocol < protocol)
+ start = mid + 1;
+ else if (m->protocol > protocol)
+ end = mid - 1;
+ else if (m->scancode < scancode)
start = mid + 1;
- else if (rc_map->scan[mid].scancode > scancode)
+ else if (m->scancode > scancode)
end = mid - 1;
else
return mid;
@@ -504,35 +554,60 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
static int ir_getkeycode(struct input_dev *idev,
struct input_keymap_entry *ke)
{
+ struct rc_keymap_entry *rke = (struct rc_keymap_entry *)ke;
struct rc_dev *rdev = input_get_drvdata(idev);
struct rc_map *rc_map = &rdev->rc_map;
struct rc_map_table *entry;
unsigned long flags;
unsigned int index;
- unsigned int scancode;
int retval;
spin_lock_irqsave(&rc_map->lock, flags);
if (ke->flags & INPUT_KEYMAP_BY_INDEX) {
index = ke->index;
- } else {
+ } else if (ke->len == sizeof(int)) {
+ /* Legacy EVIOCGKEYCODE ioctl */
+ u32 scancode;
+ u16 protocol;
+
retval = input_scancode_to_scalar(ke, &scancode);
if (retval)
goto out;
- if (guess_protocol(rdev) == RC_TYPE_NEC)
+ protocol = guess_protocol(rdev);
+ if (protocol == RC_TYPE_NEC)
scancode = to_nec32(scancode);
- index = ir_lookup_by_scancode(rc_map, scancode);
+
+ index = ir_lookup_by_scancode(rc_map, protocol, scancode);
+
+ } else if (ke->len == sizeof(struct rc_scancode)) {
+ /* New EVIOCGKEYCODE_V2 ioctl */
+ if (rke->rc.reserved[0] || rke->rc.reserved[1] || rke->rc.reserved[2]) {
+ retval = -EINVAL;
+ goto out;
+ }
+
+ index = ir_lookup_by_scancode(rc_map,
+ rke->rc.protocol, rke->rc.scancode);
+
+ } else {
+ retval = -EINVAL;
+ goto out;
}
if (index < rc_map->len) {
entry = &rc_map->scan[index];
-
ke->index = index;
ke->keycode = entry->keycode;
- ke->len = sizeof(entry->scancode);
- memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode));
+ if (ke->len == sizeof(int)) {
+ u32 scancode = entry->scancode;
+ memcpy(ke->scancode, &scancode, sizeof(scancode));
+ } else {
+ ke->len = sizeof(struct rc_scancode);
+ rke->rc.protocol = entry->protocol;
+ rke->rc.scancode = entry->scancode;
+ }
} else if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) {
/*
@@ -557,6 +632,7 @@ static int ir_getkeycode(struct input_dev *idev,
/**
* rc_g_keycode_from_table() - gets the keycode that corresponds to a scancode
* @dev: the struct rc_dev descriptor of the device
+ * @protocol: the protocol to look for
* @scancode: the scancode to look for
* @return: the corresponding keycode, or KEY_RESERVED
*
@@ -564,7 +640,8 @@ static int ir_getkeycode(struct input_dev *idev,
* keycode. Normally it should not be used since drivers should have no
* interest in keycodes.
*/
-u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode)
+u32 rc_g_keycode_from_table(struct rc_dev *dev,
+ enum rc_type protocol, u64 scancode)
{
struct rc_map *rc_map = &dev->rc_map;
unsigned int keycode;
@@ -573,15 +650,16 @@ u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode)
spin_lock_irqsave(&rc_map->lock, flags);
- index = ir_lookup_by_scancode(rc_map, scancode);
+ index = ir_lookup_by_scancode(rc_map, protocol, scancode);
keycode = index < rc_map->len ?
rc_map->scan[index].keycode : KEY_RESERVED;
spin_unlock_irqrestore(&rc_map->lock, flags);
if (keycode != KEY_RESERVED)
- IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n",
- dev->input_name, scancode, keycode);
+ IR_dprintk(1, "%s: protocol 0x%04x scancode 0x%08llx keycode 0x%02x\n",
+ dev->input_name, protocol,
+ (unsigned long long)scancode, keycode);
return keycode;
}
@@ -733,10 +811,11 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
* This routine is used to signal that a key has been pressed on the
* remote control.
*/
-void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle)
+void rc_keydown(struct rc_dev *dev, enum rc_type protocol,
+ u64 scancode, u8 toggle)
{
unsigned long flags;
- u32 keycode = rc_g_keycode_from_table(dev, scancode);
+ u32 keycode = rc_g_keycode_from_table(dev, protocol, scancode);
spin_lock_irqsave(&dev->keylock, flags);
ir_do_keydown(dev, protocol, scancode, keycode, toggle);
@@ -762,10 +841,10 @@ EXPORT_SYMBOL_GPL(rc_keydown);
* remote control. The driver must manually call rc_keyup() at a later stage.
*/
void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol,
- u32 scancode, u8 toggle)
+ u64 scancode, u8 toggle)
{
unsigned long flags;
- u32 keycode = rc_g_keycode_from_table(dev, scancode);
+ u32 keycode = rc_g_keycode_from_table(dev, protocol, scancode);
spin_lock_irqsave(&dev->keylock, flags);
ir_do_keydown(dev, protocol, scancode, keycode, toggle);
@@ -1675,7 +1754,10 @@ static int rc_prepare_rx_device(struct rc_dev *dev)
if (rc)
return rc;
- rc_type = BIT_ULL(rc_map->rc_type);
+ if (rc_map->len > 0)
+ rc_type = BIT_ULL(rc_map->scan[0].protocol);
+ else
+ rc_type = RC_TYPE_UNKNOWN;
if (dev->change_protocol) {
rc = dev->change_protocol(dev, &rc_type);
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 78dea39a9b39..ad0ed23a9194 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -44,6 +44,24 @@ enum rc_driver_type {
RC_DRIVER_IR_RAW_TX,
};
+/* This is used for the input EVIOC[SG]KEYCODE_V2 ioctls */
+struct rc_scancode {
+ __u16 protocol;
+ __u16 reserved[3];
+ __u64 scancode;
+};
+
+struct rc_keymap_entry {
+ __u8 flags;
+ __u8 len;
+ __u16 index;
+ __u32 keycode;
+ union {
+ struct rc_scancode rc;
+ __u8 raw[32];
+ };
+};
+
/**
* struct rc_scancode_filter - Filter scan codes.
* @data: Scancode data to match.
@@ -166,7 +184,7 @@ struct rc_dev {
struct timer_list timer_keyup;
u32 last_keycode;
enum rc_type last_protocol;
- u32 last_scancode;
+ u64 last_scancode;
u8 last_toggle;
u32 timeout;
u32 min_timeout;
@@ -262,10 +280,10 @@ int rc_open(struct rc_dev *rdev);
void rc_close(struct rc_dev *rdev);
void rc_repeat(struct rc_dev *dev);
-void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle);
-void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle);
+void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u64 scancode, u8 toggle);
+void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol, u64 scancode, u8 toggle);
void rc_keyup(struct rc_dev *dev);
-u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode);
+u32 rc_g_keycode_from_table(struct rc_dev *dev, enum rc_type protocol, u64 scancode);
/*
* From rc-raw.c
diff --git a/include/media/rc-map.h b/include/media/rc-map.h
index e5d0559dc523..02f53ed25493 100644
--- a/include/media/rc-map.h
+++ b/include/media/rc-map.h
@@ -139,8 +139,9 @@ enum rc_type {
* @keycode: Linux input keycode
*/
struct rc_map_table {
- u32 scancode;
- u32 keycode;
+ u64 scancode;
+ u32 keycode;
+ enum rc_type protocol;
};
/**
^ permalink raw reply related [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-27 20:34 ` [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl David Härdeman
@ 2017-04-28 11:31 ` Mauro Carvalho Chehab
2017-04-28 16:59 ` David Härdeman
2017-04-28 11:40 ` Sean Young
1 sibling, 1 reply; 22+ messages in thread
From: Mauro Carvalho Chehab @ 2017-04-28 11:31 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, sean
Em Thu, 27 Apr 2017 22:34:23 +0200
David Härdeman <david@hardeman.nu> escreveu:
> It is currently impossible to distinguish between scancodes which have
> been generated using different protocols (and scancodes can, and will,
> overlap).
>
> For example:
> RC5 message to address 0x00, command 0x03 has scancode 0x00000503
> JVC message to address 0x00, command 0x03 has scancode 0x00000503
>
> It is only possible to distinguish (and parse) scancodes by known the
> scancode *and* the protocol.
>
> Setting and getting keycodes in the input subsystem used to be done via
> the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
> and one for the keycode).
>
> The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
> which uses the following struct:
>
> struct input_keymap_entry {
> __u8 flags;
> __u8 len;
> __u16 index;
> __u32 keycode;
> __u8 scancode[32];
> };
>
> (scancode can of course be even bigger, thanks to the len member).
>
> This patch changes how the "input_keymap_entry" struct is interpreted
> by rc-core by casting it to "rc_keymap_entry":
>
> struct rc_scancode {
> __u16 protocol;
> __u16 reserved[3];
> __u64 scancode;
> }
>
> struct rc_keymap_entry {
> __u8 flags;
> __u8 len;
> __u16 index;
> __u32 keycode;
> union {
> struct rc_scancode rc;
> __u8 raw[32];
> };
> };
>
> The u64 scancode member is large enough for all current protocols and it
> would be possible to extend it in the future should it be necessary for
> some exotic protocol.
>
> The main advantage with this change is that the protocol is made explicit,
> which means that we're not throwing away data (the protocol type).
>
> This also means that struct rc_map no longer hardcodes the protocol, meaning
> that keytables with mixed entries are possible.
>
> Heuristics are also added to hopefully do the right thing with older
> ioctls in order to preserve backwards compatibility.
>
> Note that the heuristics are not 100% guaranteed to get things right.
> That is unavoidable since the protocol information simply isn't there
> when userspace calls the previous ioctl() types.
>
> However, that is somewhat mitigated by the fact that the "only"
> userspace binary which might need to change is ir-keytable. Userspace
> programs which simply consume input events (i.e. the vast majority)
> won't have to change.
Nack.
No userspace breakages are allowed. There's no way to warrant that
ir-keytable version is compatible with a certain Kernel version.
Thanks,
Mauro
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-27 20:34 ` [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl David Härdeman
2017-04-28 11:31 ` Mauro Carvalho Chehab
@ 2017-04-28 11:40 ` Sean Young
2017-04-28 16:46 ` David Härdeman
1 sibling, 1 reply; 22+ messages in thread
From: Sean Young @ 2017-04-28 11:40 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Thu, Apr 27, 2017 at 10:34:23PM +0200, David Härdeman wrote:
> It is currently impossible to distinguish between scancodes which have
> been generated using different protocols (and scancodes can, and will,
> overlap).
>
> For example:
> RC5 message to address 0x00, command 0x03 has scancode 0x00000503
> JVC message to address 0x00, command 0x03 has scancode 0x00000503
>
> It is only possible to distinguish (and parse) scancodes by known the
> scancode *and* the protocol.
>
> Setting and getting keycodes in the input subsystem used to be done via
> the EVIOC[GS]KEYCODE ioctl and "unsigned int[2]" (one int for scancode
> and one for the keycode).
>
> The interface has now been extended to use the EVIOC[GS]KEYCODE_V2 ioctl
> which uses the following struct:
>
> struct input_keymap_entry {
> __u8 flags;
> __u8 len;
> __u16 index;
> __u32 keycode;
> __u8 scancode[32];
> };
>
> (scancode can of course be even bigger, thanks to the len member).
>
> This patch changes how the "input_keymap_entry" struct is interpreted
> by rc-core by casting it to "rc_keymap_entry":
>
> struct rc_scancode {
> __u16 protocol;
> __u16 reserved[3];
> __u64 scancode;
> }
>
> struct rc_keymap_entry {
> __u8 flags;
> __u8 len;
> __u16 index;
> __u32 keycode;
> union {
> struct rc_scancode rc;
> __u8 raw[32];
> };
> };
>
> The u64 scancode member is large enough for all current protocols and it
> would be possible to extend it in the future should it be necessary for
> some exotic protocol.
>
> The main advantage with this change is that the protocol is made explicit,
> which means that we're not throwing away data (the protocol type).
>
> This also means that struct rc_map no longer hardcodes the protocol, meaning
> that keytables with mixed entries are possible.
>
> Heuristics are also added to hopefully do the right thing with older
> ioctls in order to preserve backwards compatibility.
The current ioctls do not provide any protocol information, so they should
continue to match any protocol. Those heuristics aren't good enough.
Another way of doing is to have a bitmask of protocols, and default to
RC_BIT_ALL for current ioctls.
> Note that the heuristics are not 100% guaranteed to get things right.
> That is unavoidable since the protocol information simply isn't there
> when userspace calls the previous ioctl() types.
>
> However, that is somewhat mitigated by the fact that the "only"
> userspace binary which might need to change is ir-keytable. Userspace
> programs which simply consume input events (i.e. the vast majority)
> won't have to change.
For this to be accepted we would need ir-keytable changes too so it can
be tested.
>
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/ati_remote.c | 1
> drivers/media/rc/imon.c | 7 +-
> drivers/media/rc/rc-main.c | 188 +++++++++++++++++++++++++++++------------
> include/media/rc-core.h | 26 +++++-
> include/media/rc-map.h | 5 +
> 5 files changed, 165 insertions(+), 62 deletions(-)
>
> diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
> index 9cf3e69de16a..cc81b938471f 100644
> --- a/drivers/media/rc/ati_remote.c
> +++ b/drivers/media/rc/ati_remote.c
> @@ -546,6 +546,7 @@ static void ati_remote_input_report(struct urb *urb)
> * set, assume this is a scrollwheel up/down event.
> */
> wheel_keycode = rc_g_keycode_from_table(ati_remote->rdev,
> + RC_TYPE_OTHER,
> scancode & 0x78);
>
> if (wheel_keycode == KEY_RESERVED) {
> diff --git a/drivers/media/rc/imon.c b/drivers/media/rc/imon.c
> index 3489010601b5..c724a1a5e9cd 100644
> --- a/drivers/media/rc/imon.c
> +++ b/drivers/media/rc/imon.c
> @@ -1274,14 +1274,15 @@ static u32 imon_remote_key_lookup(struct imon_context *ictx, u32 scancode)
> bool is_release_code = false;
>
> /* Look for the initial press of a button */
> - keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
> + keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
> ictx->rc_toggle = 0x0;
> ictx->rc_scancode = scancode;
>
> /* Look for the release of a button */
> if (keycode == KEY_RESERVED) {
> release = scancode & ~0x4000;
> - keycode = rc_g_keycode_from_table(ictx->rdev, release);
> + keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type,
> + release);
> if (keycode != KEY_RESERVED)
> is_release_code = true;
> }
> @@ -1310,7 +1311,7 @@ static u32 imon_mce_key_lookup(struct imon_context *ictx, u32 scancode)
> scancode = scancode | MCE_KEY_MASK | MCE_TOGGLE_BIT;
>
> ictx->rc_scancode = scancode;
> - keycode = rc_g_keycode_from_table(ictx->rdev, scancode);
> + keycode = rc_g_keycode_from_table(ictx->rdev, ictx->rc_type, scancode);
>
> /* not used in mce mode, but make sure we know its false */
> ictx->release_code = false;
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 881af208a19a..ad5545301588 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -105,7 +105,7 @@ EXPORT_SYMBOL_GPL(rc_map_unregister);
>
>
> static struct rc_map_table empty[] = {
> - { 0x2a, KEY_COFFEE },
> + { RC_TYPE_OTHER, 0x2a, KEY_COFFEE },
> };
>
> static struct rc_map_list empty_map = {
> @@ -121,7 +121,6 @@ static struct rc_map_list empty_map = {
> * ir_create_table() - initializes a scancode table
> * @rc_map: the rc_map to initialize
> * @name: name to assign to the table
> - * @rc_type: ir type to assign to the new table
> * @size: initial size of the table
> * @return: zero on success or a negative error code
> *
> @@ -129,12 +128,11 @@ static struct rc_map_list empty_map = {
> * memory to hold at least the specified number of elements.
> */
> static int ir_create_table(struct rc_map *rc_map,
> - const char *name, u64 rc_type, size_t size)
> + const char *name, size_t size)
> {
> rc_map->name = kstrdup(name, GFP_KERNEL);
> if (!rc_map->name)
> return -ENOMEM;
> - rc_map->rc_type = rc_type;
> rc_map->alloc = roundup_pow_of_two(size * sizeof(struct rc_map_table));
> rc_map->size = rc_map->alloc / sizeof(struct rc_map_table);
> rc_map->scan = kmalloc(rc_map->alloc, GFP_KERNEL);
> @@ -234,16 +232,20 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
>
> /* Did the user wish to remove the mapping? */
> if (new_keycode == KEY_RESERVED || new_keycode == KEY_UNKNOWN) {
> - IR_dprintk(1, "#%d: Deleting scan 0x%04x\n",
> - index, rc_map->scan[index].scancode);
> + IR_dprintk(1, "#%d: Deleting proto 0x%04x, scan 0x%08llx\n",
> + index, rc_map->scan[index].protocol,
> + (unsigned long long)rc_map->scan[index].scancode);
> rc_map->len--;
> memmove(&rc_map->scan[index], &rc_map->scan[index+ 1],
> (rc_map->len - index) * sizeof(struct rc_map_table));
> } else {
> - IR_dprintk(1, "#%d: %s scan 0x%04x with key 0x%04x\n",
> + IR_dprintk(1, "#%d: %s proto 0x%04x, scan 0x%08llx "
> + "with key 0x%04x\n",
> index,
> old_keycode == KEY_RESERVED ? "New" : "Replacing",
> - rc_map->scan[index].scancode, new_keycode);
> + rc_map->scan[index].protocol,
> + (unsigned long long)rc_map->scan[index].scancode,
> + new_keycode);
> rc_map->scan[index].keycode = new_keycode;
> __set_bit(new_keycode, dev->input_dev->keybit);
> }
> @@ -270,9 +272,9 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
> * ir_establish_scancode() - set a keycode in the scancode->keycode table
> * @dev: the struct rc_dev device descriptor
> * @rc_map: scancode table to be searched
> - * @scancode: the desired scancode
> - * @resize: controls whether we allowed to resize the table to
> - * accommodate not yet present scancodes
> + * @entry: the entry to be added to the table
> + * @resize: controls whether we are allowed to resize the table to
> + * accomodate not yet present scancodes
> * @return: index of the mapping containing scancode in question
> * or -1U in case of failure.
> *
> @@ -282,7 +284,7 @@ static unsigned int ir_update_mapping(struct rc_dev *dev,
> */
> static unsigned int ir_establish_scancode(struct rc_dev *dev,
> struct rc_map *rc_map,
> - unsigned int scancode,
> + struct rc_map_table *entry,
> bool resize)
> {
> unsigned int i;
> @@ -296,16 +298,27 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
> * indicate the valid bits of the scancodes.
> */
> if (dev->scancode_mask)
> - scancode &= dev->scancode_mask;
> + entry->scancode &= dev->scancode_mask;
>
> - /* First check if we already have a mapping for this ir command */
> + /*
> + * First check if we already have a mapping for this command.
> + * Note that the keytable is sorted first on protocol and second
> + * on scancode (lowest to highest).
> + */
> for (i = 0; i < rc_map->len; i++) {
> - if (rc_map->scan[i].scancode == scancode)
> - return i;
> + if (rc_map->scan[i].protocol < entry->protocol)
> + continue;
> +
> + if (rc_map->scan[i].protocol > entry->protocol)
> + break;
>
> - /* Keytable is sorted from lowest to highest scancode */
> - if (rc_map->scan[i].scancode >= scancode)
> + if (rc_map->scan[i].scancode < entry->scancode)
> + continue;
> +
> + if (rc_map->scan[i].scancode > entry->scancode)
> break;
> +
> + return i;
> }
>
> /* No previous mapping found, we might need to grow the table */
> @@ -318,7 +331,8 @@ static unsigned int ir_establish_scancode(struct rc_dev *dev,
> if (i < rc_map->len)
> memmove(&rc_map->scan[i + 1], &rc_map->scan[i],
> (rc_map->len - i) * sizeof(struct rc_map_table));
> - rc_map->scan[i].scancode = scancode;
> + rc_map->scan[i].scancode = entry->scancode;
> + rc_map->scan[i].protocol = entry->protocol;
> rc_map->scan[i].keycode = KEY_RESERVED;
> rc_map->len++;
>
> @@ -340,8 +354,10 @@ static inline enum rc_type guess_protocol(struct rc_dev *rdev)
> return rc_bitmap_to_type(rdev->enabled_protocols);
> else if (hweight64(rdev->allowed_protocols) == 1)
> return rc_bitmap_to_type(rdev->allowed_protocols);
> + else if (rc_map->len > 0)
> + return rc_map->scan[0].protocol;
> else
> - return rc_map->rc_type;
> + return RC_TYPE_OTHER;
> }
>
> /**
> @@ -384,10 +400,12 @@ static int ir_setkeycode(struct input_dev *idev,
> struct rc_dev *rdev = input_get_drvdata(idev);
> struct rc_map *rc_map = &rdev->rc_map;
> unsigned int index;
> - unsigned int scancode;
> + struct rc_map_table entry;
> int retval = 0;
> unsigned long flags;
>
> + entry.keycode = ke->keycode;
> +
> spin_lock_irqsave(&rc_map->lock, flags);
>
> if (ke->flags & INPUT_KEYMAP_BY_INDEX) {
> @@ -396,19 +414,42 @@ static int ir_setkeycode(struct input_dev *idev,
> retval = -EINVAL;
> goto out;
> }
> - } else {
> + } else if (ke->len == sizeof(int)) {
> + /* Old EVIOCSKEYCODE[_V2] ioctl */
> + u32 scancode;
> retval = input_scancode_to_scalar(ke, &scancode);
> if (retval)
> goto out;
>
> - if (guess_protocol(rdev) == RC_TYPE_NEC)
> - scancode = to_nec32(scancode);
> + entry.scancode = scancode;
> + entry.protocol = guess_protocol(rdev);
> + if (entry.protocol == RC_TYPE_NEC)
> + entry.scancode = to_nec32(scancode);
>
> - index = ir_establish_scancode(rdev, rc_map, scancode, true);
> + index = ir_establish_scancode(rdev, rc_map, &entry, true);
> if (index >= rc_map->len) {
> retval = -ENOMEM;
> goto out;
> }
> + } else if (ke->len == sizeof(struct rc_scancode)) {
> + /* New EVIOCSKEYCODE_V2 ioctl */
> + const struct rc_keymap_entry *rke = (struct rc_keymap_entry *)ke;
> + entry.protocol = rke->rc.protocol;
> + entry.scancode = rke->rc.scancode;
> +
> + if (rke->rc.reserved[0] || rke->rc.reserved[1] || rke->rc.reserved[2]) {
> + retval = -EINVAL;
> + goto out;
> + }
> +
> + index = ir_establish_scancode(rdev, rc_map, &entry, true);
> + if (index >= rc_map->len) {
> + retval = -ENOMEM;
> + goto out;
> + }
> + } else {
> + retval = -EINVAL;
> + goto out;
> }
>
> *old_keycode = ir_update_mapping(rdev, rc_map, index, ke->keycode);
> @@ -431,11 +472,11 @@ static int ir_setkeytable(struct rc_dev *dev,
> const struct rc_map *from)
> {
> struct rc_map *rc_map = &dev->rc_map;
> + struct rc_map_table entry;
> unsigned int i, index;
> int rc;
>
> - rc = ir_create_table(rc_map, from->name,
> - from->rc_type, from->size);
> + rc = ir_create_table(rc_map, from->name, from->size);
> if (rc)
> return rc;
>
> @@ -443,18 +484,19 @@ static int ir_setkeytable(struct rc_dev *dev,
> rc_map->size, rc_map->alloc);
>
> for (i = 0; i < from->size; i++) {
> - index = ir_establish_scancode(dev, rc_map,
> - from->rc_type == RC_TYPE_NEC ?
> - to_nec32(from->scan[i].scancode) :
> - from->scan[i].scancode,
> - false);
> + if (from->rc_type == RC_TYPE_NEC)
> + entry.scancode = to_nec32(from->scan[i].scancode);
> + else
> + entry.scancode = from->scan[i].scancode;
> +
> + entry.protocol = from->rc_type;
> + index = ir_establish_scancode(dev, rc_map, &entry, false);
> if (index >= rc_map->len) {
> rc = -ENOMEM;
> break;
> }
>
> - ir_update_mapping(dev, rc_map, index,
> - from->scan[i].keycode);
> + ir_update_mapping(dev, rc_map, index, from->scan[i].keycode);
> }
>
> if (rc)
> @@ -466,6 +508,7 @@ static int ir_setkeytable(struct rc_dev *dev,
> /**
> * ir_lookup_by_scancode() - locate mapping by scancode
> * @rc_map: the struct rc_map to search
> + * @protocol: protocol to look for in the table
> * @scancode: scancode to look for in the table
> * @return: index in the table, -1U if not found
> *
> @@ -473,17 +516,24 @@ static int ir_setkeytable(struct rc_dev *dev,
> * given scancode.
> */
> static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
> - unsigned int scancode)
> + u16 protocol, u64 scancode)
> {
> int start = 0;
> int end = rc_map->len - 1;
> int mid;
> + struct rc_map_table *m;
>
> while (start <= end) {
> mid = (start + end) / 2;
> - if (rc_map->scan[mid].scancode < scancode)
> + m = &rc_map->scan[mid];
> +
> + if (m->protocol < protocol)
> + start = mid + 1;
> + else if (m->protocol > protocol)
> + end = mid - 1;
> + else if (m->scancode < scancode)
> start = mid + 1;
> - else if (rc_map->scan[mid].scancode > scancode)
> + else if (m->scancode > scancode)
> end = mid - 1;
> else
> return mid;
> @@ -504,35 +554,60 @@ static unsigned int ir_lookup_by_scancode(const struct rc_map *rc_map,
> static int ir_getkeycode(struct input_dev *idev,
> struct input_keymap_entry *ke)
> {
> + struct rc_keymap_entry *rke = (struct rc_keymap_entry *)ke;
> struct rc_dev *rdev = input_get_drvdata(idev);
> struct rc_map *rc_map = &rdev->rc_map;
> struct rc_map_table *entry;
> unsigned long flags;
> unsigned int index;
> - unsigned int scancode;
> int retval;
>
> spin_lock_irqsave(&rc_map->lock, flags);
>
> if (ke->flags & INPUT_KEYMAP_BY_INDEX) {
> index = ke->index;
> - } else {
> + } else if (ke->len == sizeof(int)) {
> + /* Legacy EVIOCGKEYCODE ioctl */
> + u32 scancode;
> + u16 protocol;
> +
> retval = input_scancode_to_scalar(ke, &scancode);
> if (retval)
> goto out;
>
> - if (guess_protocol(rdev) == RC_TYPE_NEC)
> + protocol = guess_protocol(rdev);
> + if (protocol == RC_TYPE_NEC)
> scancode = to_nec32(scancode);
> - index = ir_lookup_by_scancode(rc_map, scancode);
> +
> + index = ir_lookup_by_scancode(rc_map, protocol, scancode);
> +
> + } else if (ke->len == sizeof(struct rc_scancode)) {
> + /* New EVIOCGKEYCODE_V2 ioctl */
> + if (rke->rc.reserved[0] || rke->rc.reserved[1] || rke->rc.reserved[2]) {
> + retval = -EINVAL;
> + goto out;
> + }
> +
> + index = ir_lookup_by_scancode(rc_map,
> + rke->rc.protocol, rke->rc.scancode);
> +
> + } else {
> + retval = -EINVAL;
> + goto out;
> }
>
> if (index < rc_map->len) {
> entry = &rc_map->scan[index];
> -
> ke->index = index;
> ke->keycode = entry->keycode;
> - ke->len = sizeof(entry->scancode);
> - memcpy(ke->scancode, &entry->scancode, sizeof(entry->scancode));
> + if (ke->len == sizeof(int)) {
> + u32 scancode = entry->scancode;
> + memcpy(ke->scancode, &scancode, sizeof(scancode));
> + } else {
> + ke->len = sizeof(struct rc_scancode);
> + rke->rc.protocol = entry->protocol;
> + rke->rc.scancode = entry->scancode;
> + }
>
> } else if (!(ke->flags & INPUT_KEYMAP_BY_INDEX)) {
> /*
> @@ -557,6 +632,7 @@ static int ir_getkeycode(struct input_dev *idev,
> /**
> * rc_g_keycode_from_table() - gets the keycode that corresponds to a scancode
> * @dev: the struct rc_dev descriptor of the device
> + * @protocol: the protocol to look for
> * @scancode: the scancode to look for
> * @return: the corresponding keycode, or KEY_RESERVED
> *
> @@ -564,7 +640,8 @@ static int ir_getkeycode(struct input_dev *idev,
> * keycode. Normally it should not be used since drivers should have no
> * interest in keycodes.
> */
> -u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode)
> +u32 rc_g_keycode_from_table(struct rc_dev *dev,
> + enum rc_type protocol, u64 scancode)
> {
> struct rc_map *rc_map = &dev->rc_map;
> unsigned int keycode;
> @@ -573,15 +650,16 @@ u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode)
>
> spin_lock_irqsave(&rc_map->lock, flags);
>
> - index = ir_lookup_by_scancode(rc_map, scancode);
> + index = ir_lookup_by_scancode(rc_map, protocol, scancode);
> keycode = index < rc_map->len ?
> rc_map->scan[index].keycode : KEY_RESERVED;
>
> spin_unlock_irqrestore(&rc_map->lock, flags);
>
> if (keycode != KEY_RESERVED)
> - IR_dprintk(1, "%s: scancode 0x%04x keycode 0x%02x\n",
> - dev->input_name, scancode, keycode);
> + IR_dprintk(1, "%s: protocol 0x%04x scancode 0x%08llx keycode 0x%02x\n",
> + dev->input_name, protocol,
> + (unsigned long long)scancode, keycode);
>
> return keycode;
> }
> @@ -733,10 +811,11 @@ static void ir_do_keydown(struct rc_dev *dev, enum rc_type protocol,
> * This routine is used to signal that a key has been pressed on the
> * remote control.
> */
> -void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle)
> +void rc_keydown(struct rc_dev *dev, enum rc_type protocol,
> + u64 scancode, u8 toggle)
> {
> unsigned long flags;
> - u32 keycode = rc_g_keycode_from_table(dev, scancode);
> + u32 keycode = rc_g_keycode_from_table(dev, protocol, scancode);
>
> spin_lock_irqsave(&dev->keylock, flags);
> ir_do_keydown(dev, protocol, scancode, keycode, toggle);
> @@ -762,10 +841,10 @@ EXPORT_SYMBOL_GPL(rc_keydown);
> * remote control. The driver must manually call rc_keyup() at a later stage.
> */
> void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol,
> - u32 scancode, u8 toggle)
> + u64 scancode, u8 toggle)
> {
> unsigned long flags;
> - u32 keycode = rc_g_keycode_from_table(dev, scancode);
> + u32 keycode = rc_g_keycode_from_table(dev, protocol, scancode);
>
> spin_lock_irqsave(&dev->keylock, flags);
> ir_do_keydown(dev, protocol, scancode, keycode, toggle);
> @@ -1675,7 +1754,10 @@ static int rc_prepare_rx_device(struct rc_dev *dev)
> if (rc)
> return rc;
>
> - rc_type = BIT_ULL(rc_map->rc_type);
> + if (rc_map->len > 0)
> + rc_type = BIT_ULL(rc_map->scan[0].protocol);
> + else
> + rc_type = RC_TYPE_UNKNOWN;
>
> if (dev->change_protocol) {
> rc = dev->change_protocol(dev, &rc_type);
> diff --git a/include/media/rc-core.h b/include/media/rc-core.h
> index 78dea39a9b39..ad0ed23a9194 100644
> --- a/include/media/rc-core.h
> +++ b/include/media/rc-core.h
> @@ -44,6 +44,24 @@ enum rc_driver_type {
> RC_DRIVER_IR_RAW_TX,
> };
>
> +/* This is used for the input EVIOC[SG]KEYCODE_V2 ioctls */
> +struct rc_scancode {
> + __u16 protocol;
> + __u16 reserved[3];
> + __u64 scancode;
> +};
> +
> +struct rc_keymap_entry {
> + __u8 flags;
> + __u8 len;
> + __u16 index;
> + __u32 keycode;
> + union {
> + struct rc_scancode rc;
> + __u8 raw[32];
> + };
> +};
> +
> /**
> * struct rc_scancode_filter - Filter scan codes.
> * @data: Scancode data to match.
> @@ -166,7 +184,7 @@ struct rc_dev {
> struct timer_list timer_keyup;
> u32 last_keycode;
> enum rc_type last_protocol;
> - u32 last_scancode;
> + u64 last_scancode;
> u8 last_toggle;
> u32 timeout;
> u32 min_timeout;
> @@ -262,10 +280,10 @@ int rc_open(struct rc_dev *rdev);
> void rc_close(struct rc_dev *rdev);
>
> void rc_repeat(struct rc_dev *dev);
> -void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle);
> -void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol, u32 scancode, u8 toggle);
> +void rc_keydown(struct rc_dev *dev, enum rc_type protocol, u64 scancode, u8 toggle);
> +void rc_keydown_notimeout(struct rc_dev *dev, enum rc_type protocol, u64 scancode, u8 toggle);
> void rc_keyup(struct rc_dev *dev);
> -u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode);
> +u32 rc_g_keycode_from_table(struct rc_dev *dev, enum rc_type protocol, u64 scancode);
>
> /*
> * From rc-raw.c
> diff --git a/include/media/rc-map.h b/include/media/rc-map.h
> index e5d0559dc523..02f53ed25493 100644
> --- a/include/media/rc-map.h
> +++ b/include/media/rc-map.h
> @@ -139,8 +139,9 @@ enum rc_type {
> * @keycode: Linux input keycode
> */
> struct rc_map_table {
> - u32 scancode;
> - u32 keycode;
> + u64 scancode;
> + u32 keycode;
> + enum rc_type protocol;
> };
>
> /**
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes
2017-04-27 20:34 ` [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes David Härdeman
@ 2017-04-28 11:58 ` Sean Young
2017-04-28 16:42 ` David Härdeman
0 siblings, 1 reply; 22+ messages in thread
From: Sean Young @ 2017-04-28 11:58 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Thu, Apr 27, 2017 at 10:34:18PM +0200, David Härdeman wrote:
> Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
> and the nec decoder without any loss of functionality. At the same time
> it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
> removes lots of duplication (as you can see from the patch, the same NEC
> disambiguation logic is contained in several different drivers).
>
> Using NEC32 also removes ambiguity. For example, consider these two NEC
> messages:
> NEC16 message to address 0x05, command 0x03
> NEC24 message to address 0x0005, command 0x03
>
> They'll both have scancode 0x00000503, and there's no way to tell which
> message was received.
It's not ambiguous, the protocol is different (RC_TYPE_NEC vs RC_TYPE_NECX).
> In order to maintain backwards compatibility, some heuristics are added
> in rc-main.c to convert scancodes to NEC32 as necessary when userspace
> adds entries to the keytable using the regular input ioctls. These
> heuristics are essentially the same as the ones that are currently in
> drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
> with this patch).
There are issues with the patch which breaks userspace, as discussed
in the previous patch. None of those issues have been addressed.
In addition, I've read https://david.hardeman.nu/rccore/#problems-nec
There is nothing there what you have not stated before about nec being
"ambiguous", even though the protocol variant is different.
Sean
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes
2017-04-28 11:58 ` Sean Young
@ 2017-04-28 16:42 ` David Härdeman
0 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-28 16:42 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Fri, Apr 28, 2017 at 12:58:32PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:18PM +0200, David Härdeman wrote:
>> Using the full 32 bits for all kinds of NEC scancodes simplifies rc-core
>> and the nec decoder without any loss of functionality. At the same time
>> it ensures that scancodes for NEC16/NEC24/NEC32 do not overlap and
>> removes lots of duplication (as you can see from the patch, the same NEC
>> disambiguation logic is contained in several different drivers).
>>
>> Using NEC32 also removes ambiguity. For example, consider these two NEC
>> messages:
>> NEC16 message to address 0x05, command 0x03
>> NEC24 message to address 0x0005, command 0x03
>>
>> They'll both have scancode 0x00000503, and there's no way to tell which
>> message was received.
>
>It's not ambiguous, the protocol is different (RC_TYPE_NEC vs RC_TYPE_NECX).
It's ambigous in any context where the protocol is not known (e.g. when
old-style ioctl():s are performed) or in contexts where protocols are
bundled (i.e. when the only information about protocols come from the
sysfs file).
Anyway, I'm very open to leaving NEC well alone if that means we can
make some progress on the more important issue of protocol-enabled
keytables. :)
>> In order to maintain backwards compatibility, some heuristics are added
>> in rc-main.c to convert scancodes to NEC32 as necessary when userspace
>> adds entries to the keytable using the regular input ioctls. These
>> heuristics are essentially the same as the ones that are currently in
>> drivers/media/rc/img-ir/img-ir-nec.c (which are rendered unecessary
>> with this patch).
>
>There are issues with the patch which breaks userspace, as discussed
>in the previous patch. None of those issues have been addressed.
It is impossible to add protocol information without affecting
userspace, what we should be focusing on is the best way to ameliorate
the effects.
As a simple example, consider new-style ioctls doing:
EVIOCSKEYCODE_V2 (SONY, 0x001f) = KEY_NUMERIC_2
EVIOCSKEYCODE_V2 (SANYO, 0x001f) = KEY_NUMERIC_1
After that, what should these two ioctl():s perform/return?:
EVIOCGKEYCODE (0x001f) -> ?
EVIOCSKEYCODE (0x001f) = KEY_*
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-28 11:40 ` Sean Young
@ 2017-04-28 16:46 ` David Härdeman
0 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-04-28 16:46 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Fri, Apr 28, 2017 at 12:40:53PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:23PM +0200, David Härdeman wrote:
...
>> This patch changes how the "input_keymap_entry" struct is interpreted
>> by rc-core by casting it to "rc_keymap_entry":
>>
>> struct rc_scancode {
>> __u16 protocol;
>> __u16 reserved[3];
>> __u64 scancode;
>> }
>>
>> struct rc_keymap_entry {
>> __u8 flags;
>> __u8 len;
>> __u16 index;
>> __u32 keycode;
>> union {
>> struct rc_scancode rc;
>> __u8 raw[32];
>> };
>> };
>>
>> The u64 scancode member is large enough for all current protocols and it
>> would be possible to extend it in the future should it be necessary for
>> some exotic protocol.
>>
>> The main advantage with this change is that the protocol is made explicit,
>> which means that we're not throwing away data (the protocol type).
>>
>> This also means that struct rc_map no longer hardcodes the protocol, meaning
>> that keytables with mixed entries are possible.
>>
>> Heuristics are also added to hopefully do the right thing with older
>> ioctls in order to preserve backwards compatibility.
>
>The current ioctls do not provide any protocol information, so they should
>continue to match any protocol. Those heuristics aren't good enough.
>
>Another way of doing is to have a bitmask of protocols, and default to
>RC_BIT_ALL for current ioctls.
I've been mulling that approach as well, but slightly different. My
alternative approach is based on repurposing RC_TYPE_UNKNOWN as a kind
of catch-all which will match any scancode. I'll post a patch showing
the alternative approach straight away.
>> Note that the heuristics are not 100% guaranteed to get things right.
>> That is unavoidable since the protocol information simply isn't there
>> when userspace calls the previous ioctl() types.
>>
>> However, that is somewhat mitigated by the fact that the "only"
>> userspace binary which might need to change is ir-keytable. Userspace
>> programs which simply consume input events (i.e. the vast majority)
>> won't have to change.
>
>For this to be accepted we would need ir-keytable changes too so it can
>be tested.
I know. But I'll postpone those patches until we have more of a
consensus on the right approach to take.
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-28 11:31 ` Mauro Carvalho Chehab
@ 2017-04-28 16:59 ` David Härdeman
2017-04-28 19:42 ` Sean Young
0 siblings, 1 reply; 22+ messages in thread
From: David Härdeman @ 2017-04-28 16:59 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: linux-media, sean
On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
>Em Thu, 27 Apr 2017 22:34:23 +0200
>David Härdeman <david@hardeman.nu> escreveu:
...
>> This patch changes how the "input_keymap_entry" struct is interpreted
>> by rc-core by casting it to "rc_keymap_entry":
>>
>> struct rc_scancode {
>> __u16 protocol;
>> __u16 reserved[3];
>> __u64 scancode;
>> }
>>
>> struct rc_keymap_entry {
>> __u8 flags;
>> __u8 len;
>> __u16 index;
>> __u32 keycode;
>> union {
>> struct rc_scancode rc;
>> __u8 raw[32];
>> };
>> };
>>
...
>
>Nack.
That's not a very constructive approach. If you have a better approach
in mind I'm all ears. Because you're surely not suggesting that we stay
with the current protocol-less approach forever?
>No userspace breakages are allowed.
That's a gross oversimplification. A cursory glance at the linux-api
mailing list shows plenty of examples of changes that might not be 100%
backwards-compatible. Here's an example:
http://marc.info/?l=linux-fsdevel&m=149089166918069
That's the kind of discussion we need to have - i.e. the best way to go
about this and to minimize the damage to userspace. In that vein, I'll
post an alternative approach shortly as the basis for further
discussion.
>There's no way to warrant that
>ir-keytable version is compatible with a certain Kernel version.
I know. But we know when an ioctl() is made whether it is a
protocol-aware one or not.
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-28 16:59 ` David Härdeman
@ 2017-04-28 19:42 ` Sean Young
2017-04-29 8:44 ` David Härdeman
0 siblings, 1 reply; 22+ messages in thread
From: Sean Young @ 2017-04-28 19:42 UTC (permalink / raw)
To: David Härdeman; +Cc: Mauro Carvalho Chehab, linux-media
On Fri, Apr 28, 2017 at 06:59:11PM +0200, David Härdeman wrote:
> On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
> >Em Thu, 27 Apr 2017 22:34:23 +0200
> >David Härdeman <david@hardeman.nu> escreveu:
> ...
> >> This patch changes how the "input_keymap_entry" struct is interpreted
> >> by rc-core by casting it to "rc_keymap_entry":
> >>
> >> struct rc_scancode {
> >> __u16 protocol;
> >> __u16 reserved[3];
> >> __u64 scancode;
> >> }
> >>
> >> struct rc_keymap_entry {
> >> __u8 flags;
> >> __u8 len;
> >> __u16 index;
> >> __u32 keycode;
> >> union {
> >> struct rc_scancode rc;
> >> __u8 raw[32];
> >> };
> >> };
> >>
> ...
> >
> >Nack.
>
> That's not a very constructive approach. If you have a better approach
> in mind I'm all ears. Because you're surely not suggesting that we stay
> with the current protocol-less approach forever?
Well, what problem are we trying to solve actually?
Looking at the keymaps we have already, there are many scancodes which
overlap and only a few of them use a different protocol. So having this
feature will not suddenly make it possible to load all our keymaps, it
will just make it possible to simultaneously load a few more.
> >No userspace breakages are allowed.
>
> That's a gross oversimplification.
This can be implemented without breaking userspace.
Sean
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-28 19:42 ` Sean Young
@ 2017-04-29 8:44 ` David Härdeman
2017-06-11 16:17 ` Sean Young
0 siblings, 1 reply; 22+ messages in thread
From: David Härdeman @ 2017-04-29 8:44 UTC (permalink / raw)
To: Sean Young; +Cc: Mauro Carvalho Chehab, linux-media
On Fri, Apr 28, 2017 at 08:42:13PM +0100, Sean Young wrote:
>On Fri, Apr 28, 2017 at 06:59:11PM +0200, David Härdeman wrote:
>> On Fri, Apr 28, 2017 at 08:31:33AM -0300, Mauro Carvalho Chehab wrote:
>> >Em Thu, 27 Apr 2017 22:34:23 +0200
>> >David Härdeman <david@hardeman.nu> escreveu:
>> ...
>> >> This patch changes how the "input_keymap_entry" struct is interpreted
>> >> by rc-core by casting it to "rc_keymap_entry":
>> >>
>> ...
>> >
>> >Nack.
>>
>> That's not a very constructive approach. If you have a better approach
>> in mind I'm all ears. Because you're surely not suggesting that we stay
>> with the current protocol-less approach forever?
>
>Well, what problem are we trying to solve actually?
I'm not sure what the confusion is? Last time around we discussed this
there seemed to be general agreement that protocol information is
useful?
>Looking at the keymaps we have already, there are many scancodes which
>overlap and only a few of them use a different protocol. So having this
>feature will not suddenly make it possible to load all our keymaps, it
>will just make it possible to simultaneously load a few more.
That's not really the point, overlaps in scancode && protocol cannot be
distinguished. But overlaps in scancode can be. I have remotes which
overlap in scancode even though they have different protocols.
>>
>> That's a gross oversimplification.
>
>This can be implemented without breaking userspace.
How?
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] rc-core: cleanup rc_register_device
2017-04-27 20:34 ` [PATCH 2/6] rc-core: cleanup rc_register_device David Härdeman
@ 2017-05-01 16:49 ` Sean Young
2017-05-01 17:47 ` David Härdeman
0 siblings, 1 reply; 22+ messages in thread
From: Sean Young @ 2017-05-01 16:49 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
> The device core infrastructure is based on the presumption that
> once a driver calls device_add(), it must be ready to accept
> userspace interaction.
>
> This requires splitting rc_setup_rx_device() into two functions
> and reorganizing rc_register_device() so that as much work
> as possible is performed before calling device_add().
>
With this patch applied, I'm no longer getting any scancodes from
my rc devices.
David, please can you test your patches before submitting. I have to go
over them meticulously because I cannot assume you've tested them.
Thanks
Sean
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
> drivers/media/rc/rc-core-priv.h | 2 +
> drivers/media/rc/rc-ir-raw.c | 34 ++++++++++++------
> drivers/media/rc/rc-main.c | 75 +++++++++++++++++++++++++--------------
> 3 files changed, 73 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 0455b273c2fc..b3e7cac2c3ee 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -263,7 +263,9 @@ int ir_raw_gen_pl(struct ir_raw_event **ev, unsigned int max,
> * Routines from rc-raw.c to be used internally and by decoders
> */
> u64 ir_raw_get_allowed_protocols(void);
> +int ir_raw_event_prepare(struct rc_dev *dev);
> int ir_raw_event_register(struct rc_dev *dev);
> +void ir_raw_event_free(struct rc_dev *dev);
> void ir_raw_event_unregister(struct rc_dev *dev);
> int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler);
> void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler);
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 90f66dc7c0d7..ae7785c4fbe7 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -486,14 +486,18 @@ EXPORT_SYMBOL(ir_raw_encode_scancode);
> /*
> * Used to (un)register raw event clients
> */
> -int ir_raw_event_register(struct rc_dev *dev)
> +int ir_raw_event_prepare(struct rc_dev *dev)
> {
> - int rc;
> - struct ir_raw_handler *handler;
> + static bool raw_init; /* 'false' default value, raw decoders loaded? */
>
> if (!dev)
> return -EINVAL;
>
> + if (!raw_init) {
> + request_module("ir-lirc-codec");
> + raw_init = true;
> + }
> +
> dev->raw = kzalloc(sizeof(*dev->raw), GFP_KERNEL);
> if (!dev->raw)
> return -ENOMEM;
> @@ -502,6 +506,13 @@ int ir_raw_event_register(struct rc_dev *dev)
> dev->change_protocol = change_protocol;
> INIT_KFIFO(dev->raw->kfifo);
>
> + return 0;
> +}
> +
> +int ir_raw_event_register(struct rc_dev *dev)
> +{
> + struct ir_raw_handler *handler;
> +
> /*
> * raw transmitters do not need any event registration
> * because the event is coming from userspace
> @@ -510,10 +521,8 @@ int ir_raw_event_register(struct rc_dev *dev)
> dev->raw->thread = kthread_run(ir_raw_event_thread, dev->raw,
> "rc%u", dev->minor);
>
> - if (IS_ERR(dev->raw->thread)) {
> - rc = PTR_ERR(dev->raw->thread);
> - goto out;
> - }
> + if (IS_ERR(dev->raw->thread))
> + return PTR_ERR(dev->raw->thread);
> }
>
> mutex_lock(&ir_raw_handler_lock);
> @@ -524,11 +533,15 @@ int ir_raw_event_register(struct rc_dev *dev)
> mutex_unlock(&ir_raw_handler_lock);
>
> return 0;
> +}
> +
> +void ir_raw_event_free(struct rc_dev *dev)
> +{
> + if (!dev)
> + return;
>
> -out:
> kfree(dev->raw);
> dev->raw = NULL;
> - return rc;
> }
>
> void ir_raw_event_unregister(struct rc_dev *dev)
> @@ -547,8 +560,7 @@ void ir_raw_event_unregister(struct rc_dev *dev)
> handler->raw_unregister(dev);
> mutex_unlock(&ir_raw_handler_lock);
>
> - kfree(dev->raw);
> - dev->raw = NULL;
> + ir_raw_event_free(dev);
> }
>
> /*
> diff --git a/drivers/media/rc/rc-main.c b/drivers/media/rc/rc-main.c
> index 802e559cc30e..44189366f232 100644
> --- a/drivers/media/rc/rc-main.c
> +++ b/drivers/media/rc/rc-main.c
> @@ -1663,7 +1663,7 @@ struct rc_dev *devm_rc_allocate_device(struct device *dev,
> }
> EXPORT_SYMBOL_GPL(devm_rc_allocate_device);
>
> -static int rc_setup_rx_device(struct rc_dev *dev)
> +static int rc_prepare_rx_device(struct rc_dev *dev)
> {
> int rc;
> struct rc_map *rc_map;
> @@ -1708,10 +1708,22 @@ static int rc_setup_rx_device(struct rc_dev *dev)
> dev->input_dev->phys = dev->input_phys;
> dev->input_dev->name = dev->input_name;
>
> + return 0;
> +
> +out_table:
> + ir_free_table(&dev->rc_map);
> +
> + return rc;
> +}
> +
> +static int rc_setup_rx_device(struct rc_dev *dev)
> +{
> + int rc;
> +
> /* rc_open will be called here */
> rc = input_register_device(dev->input_dev);
> if (rc)
> - goto out_table;
> + return rc;
>
> /*
> * Default delay of 250ms is too short for some protocols, especially
> @@ -1729,27 +1741,23 @@ static int rc_setup_rx_device(struct rc_dev *dev)
> dev->input_dev->rep[REP_PERIOD] = 125;
>
> return 0;
> -
> -out_table:
> - ir_free_table(&dev->rc_map);
> -
> - return rc;
> }
>
> static void rc_free_rx_device(struct rc_dev *dev)
> {
> - if (!dev || dev->driver_type == RC_DRIVER_IR_RAW_TX)
> + if (!dev)
> return;
>
> - ir_free_table(&dev->rc_map);
> + if (dev->input_dev) {
> + input_unregister_device(dev->input_dev);
> + dev->input_dev = NULL;
> + }
>
> - input_unregister_device(dev->input_dev);
> - dev->input_dev = NULL;
> + ir_free_table(&dev->rc_map);
> }
>
> int rc_register_device(struct rc_dev *dev)
> {
> - static bool raw_init; /* 'false' default value, raw decoders loaded? */
> const char *path;
> int attr = 0;
> int minor;
> @@ -1776,30 +1784,39 @@ int rc_register_device(struct rc_dev *dev)
> dev->sysfs_groups[attr++] = &rc_dev_wakeup_filter_attr_grp;
> dev->sysfs_groups[attr++] = NULL;
>
> + if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
> + rc = rc_prepare_rx_device(dev);
> + if (rc)
> + goto out_minor;
> + }
> +
> + if (dev->driver_type == RC_DRIVER_IR_RAW ||
> + dev->driver_type == RC_DRIVER_IR_RAW_TX) {
> + rc = ir_raw_event_prepare(dev);
> + if (rc < 0)
> + goto out_rx_free;
> + }
> +
> rc = device_add(&dev->dev);
> if (rc)
> - goto out_unlock;
> + goto out_raw;
>
> path = kobject_get_path(&dev->dev.kobj, GFP_KERNEL);
> dev_info(&dev->dev, "%s as %s\n",
> dev->input_name ?: "Unspecified device", path ?: "N/A");
> kfree(path);
>
> + if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
> + rc = rc_setup_rx_device(dev);
> + if (rc)
> + goto out_dev;
> + }
> +
> if (dev->driver_type == RC_DRIVER_IR_RAW ||
> dev->driver_type == RC_DRIVER_IR_RAW_TX) {
> - if (!raw_init) {
> - request_module_nowait("ir-lirc-codec");
> - raw_init = true;
> - }
> rc = ir_raw_event_register(dev);
> if (rc < 0)
> - goto out_dev;
> - }
> -
> - if (dev->driver_type != RC_DRIVER_IR_RAW_TX) {
> - rc = rc_setup_rx_device(dev);
> - if (rc)
> - goto out_raw;
> + goto out_rx;
> }
>
> /* Allow the RC sysfs nodes to be accessible */
> @@ -1811,11 +1828,15 @@ int rc_register_device(struct rc_dev *dev)
>
> return 0;
>
> -out_raw:
> - ir_raw_event_unregister(dev);
> +out_rx:
> + rc_free_rx_device(dev);
> out_dev:
> device_del(&dev->dev);
> -out_unlock:
> +out_raw:
> + ir_raw_event_free(dev);
> +out_rx_free:
> + ir_free_table(&dev->rc_map);
> +out_minor:
> ida_simple_remove(&rc_ida, minor);
> return rc;
> }
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] rc-core: cleanup rc_register_device
2017-05-01 16:49 ` Sean Young
@ 2017-05-01 17:47 ` David Härdeman
2017-05-02 18:53 ` David Härdeman
0 siblings, 1 reply; 22+ messages in thread
From: David Härdeman @ 2017-05-01 17:47 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
>> The device core infrastructure is based on the presumption that
>> once a driver calls device_add(), it must be ready to accept
>> userspace interaction.
>>
>> This requires splitting rc_setup_rx_device() into two functions
>> and reorganizing rc_register_device() so that as much work
>> as possible is performed before calling device_add().
>>
>
>With this patch applied, I'm no longer getting any scancodes from
>my rc devices.
>
>David, please can you test your patches before submitting. I have to go
>over them meticulously because I cannot assume you've tested them.
I did test this patch and I just redid the tests, both with rc-loopback
and with a mceusb receiver. I'm seeing scancodes on the input device as
well as pulse-space readings on the lirc device in both tests.
I did the tests with only this patch applied and the lirc-use-after-free
(v3). What hardware did you test with?
Meanwhile, I'll try rebasing my patches to the latest version of the
media-master git tree and test again.
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] rc-core: cleanup rc_register_device
2017-05-01 17:47 ` David Härdeman
@ 2017-05-02 18:53 ` David Härdeman
2017-05-02 20:48 ` Sean Young
0 siblings, 1 reply; 22+ messages in thread
From: David Härdeman @ 2017-05-02 18:53 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote:
>On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
>>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
>>> The device core infrastructure is based on the presumption that
>>> once a driver calls device_add(), it must be ready to accept
>>> userspace interaction.
>>>
>>> This requires splitting rc_setup_rx_device() into two functions
>>> and reorganizing rc_register_device() so that as much work
>>> as possible is performed before calling device_add().
>>>
>>
>>With this patch applied, I'm no longer getting any scancodes from
>>my rc devices.
>>
>>David, please can you test your patches before submitting. I have to go
>>over them meticulously because I cannot assume you've tested them.
>
>I did test this patch and I just redid the tests, both with rc-loopback
>and with a mceusb receiver. I'm seeing scancodes on the input device as
>well as pulse-space readings on the lirc device in both tests.
>
>I did the tests with only this patch applied and the lirc-use-after-free
>(v3). What hardware did you test with?
>
>Meanwhile, I'll try rebasing my patches to the latest version of the
>media-master git tree and test again.
I rebased the patches onto media-master (commit
3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still
can't reproduce the problems you're having :/
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] rc-core: cleanup rc_register_device
2017-05-02 18:53 ` David Härdeman
@ 2017-05-02 20:48 ` Sean Young
2017-05-03 9:49 ` David Härdeman
0 siblings, 1 reply; 22+ messages in thread
From: Sean Young @ 2017-05-02 20:48 UTC (permalink / raw)
To: David Härdeman; +Cc: linux-media, mchehab
On Tue, May 02, 2017 at 08:53:02PM +0200, David Härdeman wrote:
> On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote:
> >On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
> >>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
> >>> The device core infrastructure is based on the presumption that
> >>> once a driver calls device_add(), it must be ready to accept
> >>> userspace interaction.
> >>>
> >>> This requires splitting rc_setup_rx_device() into two functions
> >>> and reorganizing rc_register_device() so that as much work
> >>> as possible is performed before calling device_add().
> >>>
> >>
> >>With this patch applied, I'm no longer getting any scancodes from
> >>my rc devices.
> >>
> >>David, please can you test your patches before submitting. I have to go
> >>over them meticulously because I cannot assume you've tested them.
> >
> >I did test this patch and I just redid the tests, both with rc-loopback
> >and with a mceusb receiver. I'm seeing scancodes on the input device as
> >well as pulse-space readings on the lirc device in both tests.
> >
> >I did the tests with only this patch applied and the lirc-use-after-free
> >(v3). What hardware did you test with?
> >
> >Meanwhile, I'll try rebasing my patches to the latest version of the
> >media-master git tree and test again.
>
> I rebased the patches onto media-master (commit
> 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still
> can't reproduce the problems you're having :/
The protocol is not set properly. In rc_prepare_rx_device(),
dev->change_protocol() is call if not null. At this point it still is
null, since it will only be set up in ir_raw_event_prepare(), which
is called afterwards.
Presumably you have udev set up to execute ir-keytable, which sets the
protocol up (again).
There is another problem with your patches, you've introduced a race
condition. When device_add() is called, the protocol is not set up yet.
So anyone reading the protocols sysfs attribute early enough will get
false information. Is it not possible to make sure that it is all setup
correctly at the point of device_add()?
Sean
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 2/6] rc-core: cleanup rc_register_device
2017-05-02 20:48 ` Sean Young
@ 2017-05-03 9:49 ` David Härdeman
0 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-05-03 9:49 UTC (permalink / raw)
To: Sean Young; +Cc: linux-media, mchehab
On Tue, May 02, 2017 at 09:48:26PM +0100, Sean Young wrote:
>On Tue, May 02, 2017 at 08:53:02PM +0200, David Härdeman wrote:
>> On Mon, May 01, 2017 at 07:47:25PM +0200, David Härdeman wrote:
>> >On Mon, May 01, 2017 at 05:49:53PM +0100, Sean Young wrote:
>> >>On Thu, Apr 27, 2017 at 10:34:03PM +0200, David Härdeman wrote:
>> >>> The device core infrastructure is based on the presumption that
>> >>> once a driver calls device_add(), it must be ready to accept
>> >>> userspace interaction.
>> >>>
>> >>> This requires splitting rc_setup_rx_device() into two functions
>> >>> and reorganizing rc_register_device() so that as much work
>> >>> as possible is performed before calling device_add().
>> >>>
>> >>
>> >>With this patch applied, I'm no longer getting any scancodes from
>> >>my rc devices.
>> >>
>> >>David, please can you test your patches before submitting. I have to go
>> >>over them meticulously because I cannot assume you've tested them.
>> >
>> >I did test this patch and I just redid the tests, both with rc-loopback
>> >and with a mceusb receiver. I'm seeing scancodes on the input device as
>> >well as pulse-space readings on the lirc device in both tests.
>> >
>> >I did the tests with only this patch applied and the lirc-use-after-free
>> >(v3). What hardware did you test with?
>> >
>> >Meanwhile, I'll try rebasing my patches to the latest version of the
>> >media-master git tree and test again.
>>
>> I rebased the patches onto media-master (commit
>> 3622d3e77ecef090b5111e3c5423313f11711dfa) and tested again. I still
>> can't reproduce the problems you're having :/
>
>The protocol is not set properly. In rc_prepare_rx_device(),
>dev->change_protocol() is call if not null. At this point it still is
>null, since it will only be set up in ir_raw_event_prepare(), which
>is called afterwards.
Ah, good catch. Since ir_raw_event_prepare() only does a kmalloc() and
sets the change_protocol pointer it should be fine to call
ir_raw_event_prepare() before rc_prepare_rx_device(). I'll prepare a v2
of the patch.
>Presumably you have udev set up to execute ir-keytable, which sets the
>protocol up (again).
Well, kind of. In the automated testing I use rc-loopback which has the
"rc-empty" keytable so it doesn't set the protocols. In my manual
testing I used mceusb with a NEC remote so I anyway needed to set the
protocols manually and I missed the fact that "[rc-6]" was no longer set
in sysfs.
>There is another problem with your patches, you've introduced a race
>condition. When device_add() is called, the protocol is not set up yet.
>So anyone reading the protocols sysfs attribute early enough will get
>false information. Is it not possible to make sure that it is all setup
>correctly at the point of device_add()?
Isn't this the same problem? If dev->change_protocol() isn't NULL when
rc_prepare_rx_device() is called then the protocol will be set up (i.e.
dev->enabled_protcols will be set to the right value). Or did I
misunderstand you?
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-04-29 8:44 ` David Härdeman
@ 2017-06-11 16:17 ` Sean Young
2017-06-17 11:20 ` David Härdeman
0 siblings, 1 reply; 22+ messages in thread
From: Sean Young @ 2017-06-11 16:17 UTC (permalink / raw)
To: David Härdeman; +Cc: Mauro Carvalho Chehab, linux-media
On Sat, Apr 29, 2017 at 10:44:58AM +0200, David Härdeman wrote:
> >This can be implemented without breaking userspace.
>
> How?
The current keymaps we have do not specify the protocol variant, only
the protocol (rc6 vs rc6-mce). So to support this, we have to be able
to specify multiple protocols at the same time. So I think the protocol
should be a bitmask.
Also, in your example you re-used RC_TYPE_OTHER to match any protocol;
I don't think that is a good solution since there are already keymaps
which use other.
So if we have an "struct rc_scancode" which looks like:
struct rc_scancode {
u64 protocol;
u64 scancode;
};
Then if the keymap protocol is rc6, ir-keytable should set the protocol
to RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32
| RC_BIT_RC6_MCE.
If the old ioctl is used, then the protocol should be set to RC_BIT_ALL.
I can't think of anything what would break with this scheme.
Thanks
Sean
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl
2017-06-11 16:17 ` Sean Young
@ 2017-06-17 11:20 ` David Härdeman
0 siblings, 0 replies; 22+ messages in thread
From: David Härdeman @ 2017-06-17 11:20 UTC (permalink / raw)
To: Sean Young; +Cc: Mauro Carvalho Chehab, linux-media
On Sun, Jun 11, 2017 at 05:17:40PM +0100, Sean Young wrote:
>On Sat, Apr 29, 2017 at 10:44:58AM +0200, David Härdeman wrote:
>> >This can be implemented without breaking userspace.
>>
>> How?
>
>The current keymaps we have do not specify the protocol variant, only
>the protocol (rc6 vs rc6-mce). So to support this, we have to be able
>to specify multiple protocols at the same time. So I think the protocol
>should be a bitmask.
>
>Also, in your example you re-used RC_TYPE_OTHER to match any protocol;
>I don't think that is a good solution since there are already keymaps
>which use other.
>
>So if we have an "struct rc_scancode" which looks like:
>
>struct rc_scancode {
> u64 protocol;
> u64 scancode;
>};
>
>Then if the keymap protocol is rc6, ir-keytable should set the protocol
>to RC_BIT_RC6_0 | RC_BIT_RC6_6A_20 | RC_BIT_RC6_6A_24 | RC_BIT_RC6_6A_32
> | RC_BIT_RC6_MCE.
>
>If the old ioctl is used, then the protocol should be set to RC_BIT_ALL.
>
>I can't think of anything what would break with this scheme.
I've tried coding up that solution before and the problem is that it'll
still require heuristics in the presence of a mix of old and new style
ioctl():s.
For example:
old_ioctl_set(0x1234 = KEY_A);
new_ioctl_set(PROTOCOL_NEC, 0x1234 = KEY_B);
new_ioctl_set(PROTOCOL_JVC, 0x1234 = KEY_C);
old_ioctl_get(0x1234) = ?
old_ioctl_set(0x1234 = KEY_D);
new_ioctl_get(PROTOCOL_NEC, 0x1234) = ?
--
David Härdeman
^ permalink raw reply [flat|nested] 22+ messages in thread
end of thread, other threads:[~2017-06-17 11:20 UTC | newest]
Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-04-27 20:33 [PATCH 0/6] rc-core - protocol in keytables David Härdeman
2017-04-27 20:33 ` [PATCH 1/6] rc-core: fix input repeat handling David Härdeman
2017-04-27 20:34 ` [PATCH 2/6] rc-core: cleanup rc_register_device David Härdeman
2017-05-01 16:49 ` Sean Young
2017-05-01 17:47 ` David Härdeman
2017-05-02 18:53 ` David Härdeman
2017-05-02 20:48 ` Sean Young
2017-05-03 9:49 ` David Härdeman
2017-04-27 20:34 ` [PATCH 3/6] rc-core: cleanup rc_register_device pt2 David Härdeman
2017-04-27 20:34 ` [PATCH 4/6] rc-core: use the full 32 bits for NEC scancodes in wakefilters David Härdeman
2017-04-27 20:34 ` [PATCH 5/6] rc-core: use the full 32 bits for NEC scancodes David Härdeman
2017-04-28 11:58 ` Sean Young
2017-04-28 16:42 ` David Härdeman
2017-04-27 20:34 ` [PATCH 6/6] rc-core: add protocol to EVIOC[GS]KEYCODE_V2 ioctl David Härdeman
2017-04-28 11:31 ` Mauro Carvalho Chehab
2017-04-28 16:59 ` David Härdeman
2017-04-28 19:42 ` Sean Young
2017-04-29 8:44 ` David Härdeman
2017-06-11 16:17 ` Sean Young
2017-06-17 11:20 ` David Härdeman
2017-04-28 11:40 ` Sean Young
2017-04-28 16:46 ` David Härdeman
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.