* [PATCH 0/4] ir-core sysfs protocol selection simplification
@ 2010-04-24 21:13 David Härdeman
2010-04-24 21:14 ` [PATCH 1/4] ir-core: remove IR_TYPE_PD David Härdeman
` (3 more replies)
0 siblings, 4 replies; 25+ messages in thread
From: David Härdeman @ 2010-04-24 21:13 UTC (permalink / raw)
To: mchehab; +Cc: linux-media, linux-input
The following series changes the sysfs implementation in ir-core to
make the protocol selection work in the same manner for hardware
decoders and software decoders (the distinction between the two
should be hidden from the user as much as possible IMHO).
This also allows for a nice reduction of duplicated code between
the raw ir protocol decoders.
The first patch is merely preparatory and should hopefully not
be controversial.
The second and third patch should be considered RFC's on the
implementation of the sysfs interface.
The last patch is orthogonal to the rest of the patchset and should
hopefully not be controversial (though it would be nice if someone
with the actual hardware could test it).
---
David Härdeman (4):
ir-core: remove IR_TYPE_PD
ir-core: centralize sysfs raw decoder enabling/disabling
ir-core: move decoding state to ir_raw_event_ctrl
ir-core: remove ir-functions usage from cx231xx
drivers/media/IR/ir-core-priv.h | 40 ++++
drivers/media/IR/ir-jvc-decoder.c | 152 +---------------
drivers/media/IR/ir-nec-decoder.c | 151 +---------------
drivers/media/IR/ir-raw-event.c | 136 ++++++--------
drivers/media/IR/ir-rc5-decoder.c | 165 ++---------------
drivers/media/IR/ir-rc6-decoder.c | 154 +---------------
drivers/media/IR/ir-sony-decoder.c | 155 +---------------
drivers/media/IR/ir-sysfs.c | 262 +++++++++++++++------------
drivers/media/video/cx231xx/cx231xx-input.c | 47 +----
drivers/media/video/cx231xx/cx231xx.h | 2
drivers/media/video/cx88/cx88-input.c | 8 -
include/media/ir-kbd-i2c.h | 2
include/media/rc-map.h | 9 -
13 files changed, 322 insertions(+), 961 deletions(-)
--
David Härdeman
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 25+ messages in thread* [PATCH 1/4] ir-core: remove IR_TYPE_PD 2010-04-24 21:13 [PATCH 0/4] ir-core sysfs protocol selection simplification David Härdeman @ 2010-04-24 21:14 ` David Härdeman 2010-04-24 21:14 ` [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling David Härdeman ` (2 subsequent siblings) 3 siblings, 0 replies; 25+ messages in thread From: David Härdeman @ 2010-04-24 21:14 UTC (permalink / raw) To: mchehab; +Cc: linux-media, linux-input Pulse-distance is not a protocol, it is a line coding (used by some protocols, like NEC). Looking at the uses of IR_TYPE_PD, the real protocol seems to be NEC in all cases (drivers/media/video/cx88/cx88-input.c is the only user). So, remove IR_TYPE_PD while it is still easy to do so. Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/IR/ir-sysfs.c | 6 ------ drivers/media/video/cx88/cx88-input.c | 8 ++++---- include/media/ir-kbd-i2c.h | 2 +- include/media/rc-map.h | 9 ++++----- 4 files changed, 9 insertions(+), 16 deletions(-) diff --git a/drivers/media/IR/ir-sysfs.c b/drivers/media/IR/ir-sysfs.c index 501dc2f..facca11 100644 --- a/drivers/media/IR/ir-sysfs.c +++ b/drivers/media/IR/ir-sysfs.c @@ -56,8 +56,6 @@ static ssize_t show_protocol(struct device *d, s = "Unknown"; else if (ir_type == IR_TYPE_RC5) s = "rc-5"; - else if (ir_type == IR_TYPE_PD) - s = "pulse-distance"; else if (ir_type == IR_TYPE_NEC) s = "nec"; else if (ir_type == IR_TYPE_RC6) @@ -99,8 +97,6 @@ static ssize_t store_protocol(struct device *d, while ((buf = strsep((char **) &data, " \n")) != NULL) { if (!strcasecmp(buf, "rc-5") || !strcasecmp(buf, "rc5")) ir_type |= IR_TYPE_RC5; - if (!strcasecmp(buf, "pd") || !strcasecmp(buf, "pulse-distance")) - ir_type |= IR_TYPE_PD; if (!strcasecmp(buf, "nec")) ir_type |= IR_TYPE_NEC; if (!strcasecmp(buf, "jvc")) @@ -145,8 +141,6 @@ static ssize_t show_supported_protocols(struct device *d, buf += sprintf(buf, "unknown "); if (ir_dev->props->allowed_protos & IR_TYPE_RC5) buf += sprintf(buf, "rc-5 "); - if (ir_dev->props->allowed_protos & IR_TYPE_PD) - buf += sprintf(buf, "pulse-distance "); if (ir_dev->props->allowed_protos & IR_TYPE_NEC) buf += sprintf(buf, "nec "); if (buf == orgbuf) diff --git a/drivers/media/video/cx88/cx88-input.c b/drivers/media/video/cx88/cx88-input.c index 5e60b48..0de9bdf 100644 --- a/drivers/media/video/cx88/cx88-input.c +++ b/drivers/media/video/cx88/cx88-input.c @@ -271,7 +271,7 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci) break; case CX88_BOARD_TERRATEC_CINERGY_1400_DVB_T1: ir_codes = RC_MAP_CINERGY_1400; - ir_type = IR_TYPE_PD; + ir_type = IR_TYPE_NEC; ir->sampling = 0xeb04; /* address */ break; case CX88_BOARD_HAUPPAUGE: @@ -374,18 +374,18 @@ int cx88_ir_init(struct cx88_core *core, struct pci_dev *pci) case CX88_BOARD_PROF_7301: case CX88_BOARD_PROF_6200: ir_codes = RC_MAP_TBS_NEC; - ir_type = IR_TYPE_PD; + ir_type = IR_TYPE_NEC; ir->sampling = 0xff00; /* address */ break; case CX88_BOARD_TEVII_S460: case CX88_BOARD_TEVII_S420: ir_codes = RC_MAP_TEVII_NEC; - ir_type = IR_TYPE_PD; + ir_type = IR_TYPE_NEC; ir->sampling = 0xff00; /* address */ break; case CX88_BOARD_DNTV_LIVE_DVB_T_PRO: ir_codes = RC_MAP_DNTV_LIVE_DVBT_PRO; - ir_type = IR_TYPE_PD; + ir_type = IR_TYPE_NEC; ir->sampling = 0xff00; /* address */ break; case CX88_BOARD_NORWOOD_MICRO: diff --git a/include/media/ir-kbd-i2c.h b/include/media/ir-kbd-i2c.h index 057ff64..0506e45 100644 --- a/include/media/ir-kbd-i2c.h +++ b/include/media/ir-kbd-i2c.h @@ -36,7 +36,7 @@ enum ir_kbd_get_key_fn { struct IR_i2c_init_data { char *ir_codes; const char *name; - u64 type; /* IR_TYPE_RC5, IR_TYPE_PD, etc */ + u64 type; /* IR_TYPE_RC5, etc */ /* * Specify either a function pointer or a value indicating one of * ir_kbd_i2c's internal get_key functions diff --git a/include/media/rc-map.h b/include/media/rc-map.h index 67af24e..ba53fe2 100644 --- a/include/media/rc-map.h +++ b/include/media/rc-map.h @@ -13,11 +13,10 @@ #define IR_TYPE_UNKNOWN 0 #define IR_TYPE_RC5 (1 << 0) /* Philips RC5 protocol */ -#define IR_TYPE_PD (1 << 1) /* Pulse distance encoded IR */ -#define IR_TYPE_NEC (1 << 2) -#define IR_TYPE_RC6 (1 << 3) /* Philips RC6 protocol */ -#define IR_TYPE_JVC (1 << 4) /* JVC protocol */ -#define IR_TYPE_SONY (1 << 5) /* Sony12/15/20 protocol */ +#define IR_TYPE_NEC (1 << 1) +#define IR_TYPE_RC6 (1 << 2) /* Philips RC6 protocol */ +#define IR_TYPE_JVC (1 << 3) /* JVC protocol */ +#define IR_TYPE_SONY (1 << 4) /* Sony12/15/20 protocol */ #define IR_TYPE_OTHER (1u << 31) struct ir_scancode { -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 25+ messages in thread
* [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling 2010-04-24 21:13 [PATCH 0/4] ir-core sysfs protocol selection simplification David Härdeman 2010-04-24 21:14 ` [PATCH 1/4] ir-core: remove IR_TYPE_PD David Härdeman @ 2010-04-24 21:14 ` David Härdeman 2010-05-03 19:49 ` Mauro Carvalho Chehab 2010-04-24 21:14 ` [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl David Härdeman 2010-04-24 21:14 ` [PATCH 4/4] ir-core: remove ir-functions usage from cx231xx David Härdeman 3 siblings, 1 reply; 25+ messages in thread From: David Härdeman @ 2010-04-24 21:14 UTC (permalink / raw) To: mchehab; +Cc: linux-media, linux-input With the current logic, each raw decoder needs to add a copy of the exact same sysfs code. This is both unnecessary and also means that (re)loading an IR driver after raw decoder modules have been loaded won't work as expected. This patch moves that logic into ir-raw-event and adds a single sysfs file per device. Reading that file returns something like: "rc5 [rc6] nec jvc [sony]" (with enabled protocols in [] brackets) Writing either "+protocol" or "-protocol" to that file will enable or disable the according protocol decoder. An additional benefit is that the disabling of a decoder will be remembered across module removal/insertion so a previously disabled decoder won't suddenly be activated again. The default setting is to enable all decoders. This is also necessary for the next patch which moves even more decoder state into the central raw decoding structs. Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/IR/ir-core-priv.h | 3 drivers/media/IR/ir-jvc-decoder.c | 64 --------- drivers/media/IR/ir-nec-decoder.c | 64 --------- drivers/media/IR/ir-raw-event.c | 112 +++++++++------- drivers/media/IR/ir-rc5-decoder.c | 64 --------- drivers/media/IR/ir-rc6-decoder.c | 64 --------- drivers/media/IR/ir-sony-decoder.c | 64 --------- drivers/media/IR/ir-sysfs.c | 252 +++++++++++++++++++++--------------- 8 files changed, 231 insertions(+), 456 deletions(-) diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h index 04962a6..821d012 100644 --- a/drivers/media/IR/ir-core-priv.h +++ b/drivers/media/IR/ir-core-priv.h @@ -21,6 +21,7 @@ struct ir_raw_handler { struct list_head list; + u64 protocols; /* which are handled by this handler */ int (*decode)(struct input_dev *input_dev, struct ir_raw_event event); int (*raw_register)(struct input_dev *input_dev); int (*raw_unregister)(struct input_dev *input_dev); @@ -32,6 +33,7 @@ struct ir_raw_event_ctrl { ktime_t last_event; /* when last event occurred */ enum raw_event_type last_type; /* last event type */ struct input_dev *input_dev; /* pointer to the parent input_dev */ + u64 enabled_protocols; /* enabled raw protocol decoders */ }; /* macros for IR decoders */ @@ -76,6 +78,7 @@ void ir_unregister_class(struct input_dev *input_dev); /* * Routines from ir-raw-event.c to be used internally and by decoders */ +u64 ir_raw_get_allowed_protocols(void); int ir_raw_event_register(struct input_dev *input_dev); void ir_raw_event_unregister(struct input_dev *input_dev); int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler); diff --git a/drivers/media/IR/ir-jvc-decoder.c b/drivers/media/IR/ir-jvc-decoder.c index 0b80494..1055de4 100644 --- a/drivers/media/IR/ir-jvc-decoder.c +++ b/drivers/media/IR/ir-jvc-decoder.c @@ -41,7 +41,6 @@ enum jvc_state { struct decoder_data { struct list_head list; struct ir_input_dev *ir_dev; - int enabled:1; /* State machine control */ enum jvc_state state; @@ -72,53 +71,6 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) return data; } -static ssize_t store_enabled(struct device *d, - struct device_attribute *mattr, - const char *buf, - size_t len) -{ - unsigned long value; - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (strict_strtoul(buf, 10, &value) || value > 1) - return -EINVAL; - - data->enabled = value; - - return len; -} - -static ssize_t show_enabled(struct device *d, - struct device_attribute *mattr, char *buf) -{ - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (data->enabled) - return sprintf(buf, "1\n"); - else - return sprintf(buf, "0\n"); -} - -static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled); - -static struct attribute *decoder_attributes[] = { - &dev_attr_enabled.attr, - NULL -}; - -static struct attribute_group decoder_attribute_group = { - .name = "jvc_decoder", - .attrs = decoder_attributes, -}; - /** * ir_jvc_decode() - Decode one JVC pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -135,7 +87,7 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!data) return -EINVAL; - if (!data->enabled) + if (!(ir_dev->raw->enabled_protocols & IR_TYPE_JVC)) return 0; if (IS_RESET(ev)) { @@ -253,21 +205,12 @@ static int ir_jvc_register(struct input_dev *input_dev) { struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); struct decoder_data *data; - int rc; - - rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group); - if (rc < 0) - return rc; data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) { - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); + if (!data) return -ENOMEM; - } data->ir_dev = ir_dev; - data->enabled = 1; - spin_lock(&decoder_lock); list_add_tail(&data->list, &decoder_list); spin_unlock(&decoder_lock); @@ -284,8 +227,6 @@ static int ir_jvc_unregister(struct input_dev *input_dev) if (!data) return 0; - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); - spin_lock(&decoder_lock); list_del(&data->list); spin_unlock(&decoder_lock); @@ -294,6 +235,7 @@ static int ir_jvc_unregister(struct input_dev *input_dev) } static struct ir_raw_handler jvc_handler = { + .protocols = IR_TYPE_JVC, .decode = ir_jvc_decode, .raw_register = ir_jvc_register, .raw_unregister = ir_jvc_unregister, diff --git a/drivers/media/IR/ir-nec-decoder.c b/drivers/media/IR/ir-nec-decoder.c index ba79233..2cc2b92 100644 --- a/drivers/media/IR/ir-nec-decoder.c +++ b/drivers/media/IR/ir-nec-decoder.c @@ -43,7 +43,6 @@ enum nec_state { struct decoder_data { struct list_head list; struct ir_input_dev *ir_dev; - int enabled:1; /* State machine control */ enum nec_state state; @@ -71,53 +70,6 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) return data; } -static ssize_t store_enabled(struct device *d, - struct device_attribute *mattr, - const char *buf, - size_t len) -{ - unsigned long value; - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (strict_strtoul(buf, 10, &value) || value > 1) - return -EINVAL; - - data->enabled = value; - - return len; -} - -static ssize_t show_enabled(struct device *d, - struct device_attribute *mattr, char *buf) -{ - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (data->enabled) - return sprintf(buf, "1\n"); - else - return sprintf(buf, "0\n"); -} - -static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled); - -static struct attribute *decoder_attributes[] = { - &dev_attr_enabled.attr, - NULL -}; - -static struct attribute_group decoder_attribute_group = { - .name = "nec_decoder", - .attrs = decoder_attributes, -}; - /** * ir_nec_decode() - Decode one NEC pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -136,7 +88,7 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!data) return -EINVAL; - if (!data->enabled) + if (!(ir_dev->raw->enabled_protocols & IR_TYPE_NEC)) return 0; if (IS_RESET(ev)) { @@ -260,21 +212,12 @@ static int ir_nec_register(struct input_dev *input_dev) { struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); struct decoder_data *data; - int rc; - - rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group); - if (rc < 0) - return rc; data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) { - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); + if (!data) return -ENOMEM; - } data->ir_dev = ir_dev; - data->enabled = 1; - spin_lock(&decoder_lock); list_add_tail(&data->list, &decoder_list); spin_unlock(&decoder_lock); @@ -291,8 +234,6 @@ static int ir_nec_unregister(struct input_dev *input_dev) if (!data) return 0; - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); - spin_lock(&decoder_lock); list_del(&data->list); spin_unlock(&decoder_lock); @@ -301,6 +242,7 @@ static int ir_nec_unregister(struct input_dev *input_dev) } static struct ir_raw_handler nec_handler = { + .protocols = IR_TYPE_NEC, .decode = ir_nec_decode, .raw_register = ir_nec_register, .raw_unregister = ir_nec_unregister, diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c index ea68a3f..eeca8a8 100644 --- a/drivers/media/IR/ir-raw-event.c +++ b/drivers/media/IR/ir-raw-event.c @@ -21,8 +21,9 @@ #define MAX_IR_EVENT_SIZE 512 /* Used to handle IR raw handler extensions */ -static LIST_HEAD(ir_raw_handler_list); static DEFINE_SPINLOCK(ir_raw_handler_lock); +static LIST_HEAD(ir_raw_handler_list); +static u64 available_protocols; /** * RUN_DECODER() - runs an operation on all IR decoders @@ -65,52 +66,6 @@ static void ir_raw_event_work(struct work_struct *work) RUN_DECODER(decode, raw->input_dev, ev); } -int ir_raw_event_register(struct input_dev *input_dev) -{ - struct ir_input_dev *ir = input_get_drvdata(input_dev); - int rc; - - ir->raw = kzalloc(sizeof(*ir->raw), GFP_KERNEL); - if (!ir->raw) - return -ENOMEM; - - ir->raw->input_dev = input_dev; - INIT_WORK(&ir->raw->rx_work, ir_raw_event_work); - - rc = kfifo_alloc(&ir->raw->kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE, - GFP_KERNEL); - if (rc < 0) { - kfree(ir->raw); - ir->raw = NULL; - return rc; - } - - rc = RUN_DECODER(raw_register, input_dev); - if (rc < 0) { - kfifo_free(&ir->raw->kfifo); - kfree(ir->raw); - ir->raw = NULL; - return rc; - } - - return rc; -} - -void ir_raw_event_unregister(struct input_dev *input_dev) -{ - struct ir_input_dev *ir = input_get_drvdata(input_dev); - - if (!ir->raw) - return; - - cancel_work_sync(&ir->raw->rx_work); - RUN_DECODER(raw_unregister, input_dev); - - kfifo_free(&ir->raw->kfifo); - kfree(ir->raw); - ir->raw = NULL; -} - /** * ir_raw_event_store() - pass a pulse/space duration to the raw ir decoders * @input_dev: the struct input_dev device descriptor @@ -204,6 +159,66 @@ void ir_raw_event_handle(struct input_dev *input_dev) } EXPORT_SYMBOL_GPL(ir_raw_event_handle); +/* used internally by the sysfs interface */ +u64 +ir_raw_get_allowed_protocols() +{ + u64 protocols; + spin_lock(&ir_raw_handler_lock); + protocols = available_protocols; + spin_unlock(&ir_raw_handler_lock); + return protocols; +} + +/* + * Used to (un)register raw event clients + */ +int ir_raw_event_register(struct input_dev *input_dev) +{ + struct ir_input_dev *ir = input_get_drvdata(input_dev); + int rc; + + ir->raw = kzalloc(sizeof(*ir->raw), GFP_KERNEL); + if (!ir->raw) + return -ENOMEM; + + ir->raw->input_dev = input_dev; + INIT_WORK(&ir->raw->rx_work, ir_raw_event_work); + ir->raw->enabled_protocols = ~0; + rc = kfifo_alloc(&ir->raw->kfifo, sizeof(s64) * MAX_IR_EVENT_SIZE, + GFP_KERNEL); + if (rc < 0) { + kfree(ir->raw); + ir->raw = NULL; + return rc; + } + + rc = RUN_DECODER(raw_register, input_dev); + if (rc < 0) { + kfifo_free(&ir->raw->kfifo); + kfree(ir->raw); + ir->raw = NULL; + return rc; + } + + return rc; +} + +void ir_raw_event_unregister(struct input_dev *input_dev) +{ + struct ir_input_dev *ir = input_get_drvdata(input_dev); + + if (!ir->raw) + return; + + cancel_work_sync(&ir->raw->rx_work); + RUN_DECODER(raw_unregister, input_dev); + + kfifo_free(&ir->raw->kfifo); + kfree(ir->raw); + ir->raw = NULL; +} + /* * Extension interface - used to register the IR decoders */ @@ -212,7 +227,9 @@ int ir_raw_handler_register(struct ir_raw_handler *ir_raw_handler) { spin_lock(&ir_raw_handler_lock); list_add_tail(&ir_raw_handler->list, &ir_raw_handler_list); + available_protocols |= ir_raw_handler->protocols; spin_unlock(&ir_raw_handler_lock); + return 0; } EXPORT_SYMBOL(ir_raw_handler_register); @@ -221,6 +238,7 @@ void ir_raw_handler_unregister(struct ir_raw_handler *ir_raw_handler) { spin_lock(&ir_raw_handler_lock); list_del(&ir_raw_handler->list); + available_protocols &= ~ir_raw_handler->protocols; spin_unlock(&ir_raw_handler_lock); } EXPORT_SYMBOL(ir_raw_handler_unregister); diff --git a/drivers/media/IR/ir-rc5-decoder.c b/drivers/media/IR/ir-rc5-decoder.c index 23cdb1b..1be8981 100644 --- a/drivers/media/IR/ir-rc5-decoder.c +++ b/drivers/media/IR/ir-rc5-decoder.c @@ -45,7 +45,6 @@ enum rc5_state { struct decoder_data { struct list_head list; struct ir_input_dev *ir_dev; - int enabled:1; /* State machine control */ enum rc5_state state; @@ -76,53 +75,6 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) return data; } -static ssize_t store_enabled(struct device *d, - struct device_attribute *mattr, - const char *buf, - size_t len) -{ - unsigned long value; - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (strict_strtoul(buf, 10, &value) || value > 1) - return -EINVAL; - - data->enabled = value; - - return len; -} - -static ssize_t show_enabled(struct device *d, - struct device_attribute *mattr, char *buf) -{ - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (data->enabled) - return sprintf(buf, "1\n"); - else - return sprintf(buf, "0\n"); -} - -static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled); - -static struct attribute *decoder_attributes[] = { - &dev_attr_enabled.attr, - NULL -}; - -static struct attribute_group decoder_attribute_group = { - .name = "rc5_decoder", - .attrs = decoder_attributes, -}; - /** * ir_rc5_decode() - Decode one RC-5 pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -141,7 +93,7 @@ static int ir_rc5_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!data) return -EINVAL; - if (!data->enabled) + if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC5)) return 0; if (IS_RESET(ev)) { @@ -256,21 +208,12 @@ static int ir_rc5_register(struct input_dev *input_dev) { struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); struct decoder_data *data; - int rc; - - rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group); - if (rc < 0) - return rc; data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) { - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); + if (!data) return -ENOMEM; - } data->ir_dev = ir_dev; - data->enabled = 1; - spin_lock(&decoder_lock); list_add_tail(&data->list, &decoder_list); spin_unlock(&decoder_lock); @@ -287,8 +230,6 @@ static int ir_rc5_unregister(struct input_dev *input_dev) if (!data) return 0; - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); - spin_lock(&decoder_lock); list_del(&data->list); spin_unlock(&decoder_lock); @@ -297,6 +238,7 @@ static int ir_rc5_unregister(struct input_dev *input_dev) } static struct ir_raw_handler rc5_handler = { + .protocols = IR_TYPE_RC5, .decode = ir_rc5_decode, .raw_register = ir_rc5_register, .raw_unregister = ir_rc5_unregister, diff --git a/drivers/media/IR/ir-rc6-decoder.c b/drivers/media/IR/ir-rc6-decoder.c index 2bf479f..5e940a8 100644 --- a/drivers/media/IR/ir-rc6-decoder.c +++ b/drivers/media/IR/ir-rc6-decoder.c @@ -61,7 +61,6 @@ enum rc6_state { struct decoder_data { struct list_head list; struct ir_input_dev *ir_dev; - int enabled:1; /* State machine control */ enum rc6_state state; @@ -93,53 +92,6 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) return data; } -static ssize_t store_enabled(struct device *d, - struct device_attribute *mattr, - const char *buf, - size_t len) -{ - unsigned long value; - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (strict_strtoul(buf, 10, &value) || value > 1) - return -EINVAL; - - data->enabled = value; - - return len; -} - -static ssize_t show_enabled(struct device *d, - struct device_attribute *mattr, char *buf) -{ - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (data->enabled) - return sprintf(buf, "1\n"); - else - return sprintf(buf, "0\n"); -} - -static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled); - -static struct attribute *decoder_attributes[] = { - &dev_attr_enabled.attr, - NULL -}; - -static struct attribute_group decoder_attribute_group = { - .name = "rc6_decoder", - .attrs = decoder_attributes, -}; - static enum rc6_mode rc6_mode(struct decoder_data *data) { switch (data->header & RC6_MODE_MASK) { case 0: @@ -171,7 +123,7 @@ static int ir_rc6_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!data) return -EINVAL; - if (!data->enabled) + if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC6)) return 0; if (IS_RESET(ev)) { @@ -352,21 +304,12 @@ static int ir_rc6_register(struct input_dev *input_dev) { struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); struct decoder_data *data; - int rc; - - rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group); - if (rc < 0) - return rc; data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) { - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); + if (!data) return -ENOMEM; - } data->ir_dev = ir_dev; - data->enabled = 1; - spin_lock(&decoder_lock); list_add_tail(&data->list, &decoder_list); spin_unlock(&decoder_lock); @@ -383,8 +326,6 @@ static int ir_rc6_unregister(struct input_dev *input_dev) if (!data) return 0; - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); - spin_lock(&decoder_lock); list_del(&data->list); spin_unlock(&decoder_lock); @@ -393,6 +334,7 @@ static int ir_rc6_unregister(struct input_dev *input_dev) } static struct ir_raw_handler rc6_handler = { + .protocols = IR_TYPE_RC6, .decode = ir_rc6_decode, .raw_register = ir_rc6_register, .raw_unregister = ir_rc6_unregister, diff --git a/drivers/media/IR/ir-sony-decoder.c b/drivers/media/IR/ir-sony-decoder.c index 9f440c5..8afd16a 100644 --- a/drivers/media/IR/ir-sony-decoder.c +++ b/drivers/media/IR/ir-sony-decoder.c @@ -38,7 +38,6 @@ enum sony_state { struct decoder_data { struct list_head list; struct ir_input_dev *ir_dev; - int enabled:1; /* State machine control */ enum sony_state state; @@ -66,53 +65,6 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) return data; } -static ssize_t store_enabled(struct device *d, - struct device_attribute *mattr, - const char *buf, - size_t len) -{ - unsigned long value; - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (strict_strtoul(buf, 10, &value) || value > 1) - return -EINVAL; - - data->enabled = value; - - return len; -} - -static ssize_t show_enabled(struct device *d, - struct device_attribute *mattr, char *buf) -{ - struct ir_input_dev *ir_dev = dev_get_drvdata(d); - struct decoder_data *data = get_decoder_data(ir_dev); - - if (!data) - return -EINVAL; - - if (data->enabled) - return sprintf(buf, "1\n"); - else - return sprintf(buf, "0\n"); -} - -static DEVICE_ATTR(enabled, S_IRUGO | S_IWUSR, show_enabled, store_enabled); - -static struct attribute *decoder_attributes[] = { - &dev_attr_enabled.attr, - NULL -}; - -static struct attribute_group decoder_attribute_group = { - .name = "sony_decoder", - .attrs = decoder_attributes, -}; - /** * ir_sony_decode() - Decode one Sony pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -131,7 +83,7 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!data) return -EINVAL; - if (!data->enabled) + if (!(ir_dev->raw->enabled_protocols & IR_TYPE_SONY)) return 0; if (IS_RESET(ev)) { @@ -245,21 +197,12 @@ static int ir_sony_register(struct input_dev *input_dev) { struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); struct decoder_data *data; - int rc; - - rc = sysfs_create_group(&ir_dev->dev.kobj, &decoder_attribute_group); - if (rc < 0) - return rc; data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) { - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); + if (!data) return -ENOMEM; - } data->ir_dev = ir_dev; - data->enabled = 1; - spin_lock(&decoder_lock); list_add_tail(&data->list, &decoder_list); spin_unlock(&decoder_lock); @@ -276,8 +219,6 @@ static int ir_sony_unregister(struct input_dev *input_dev) if (!data) return 0; - sysfs_remove_group(&ir_dev->dev.kobj, &decoder_attribute_group); - spin_lock(&decoder_lock); list_del(&data->list); spin_unlock(&decoder_lock); @@ -286,6 +227,7 @@ static int ir_sony_unregister(struct input_dev *input_dev) } static struct ir_raw_handler sony_handler = { + .protocols = IR_TYPE_SONY, .decode = ir_sony_decode, .raw_register = ir_sony_register, .raw_unregister = ir_sony_unregister, diff --git a/drivers/media/IR/ir-sysfs.c b/drivers/media/IR/ir-sysfs.c index facca11..2efb475 100644 --- a/drivers/media/IR/ir-sysfs.c +++ b/drivers/media/IR/ir-sysfs.c @@ -33,122 +33,178 @@ static struct class ir_input_class = { }; /** - * show_protocol() - shows the current IR protocol + * show_protocols() - shows the current IR protocol(s) * @d: the device descriptor * @mattr: the device attribute struct (unused) * @buf: a pointer to the output buffer * - * This routine is a callback routine for input read the IR protocol type. - * it is trigged by reading /sys/class/rc/rc?/current_protocol. - * It returns the protocol name, as understood by the driver. + * This routine is a callback routine for input read the IR protocol type(s). + * it is trigged by reading /sys/class/rc/rc?/protocols. + * It returns the protocol names of supported protocols. + * Enabled protocols are printed in brackets. */ -static ssize_t show_protocol(struct device *d, - struct device_attribute *mattr, char *buf) +static ssize_t show_protocols(struct device *d, + struct device_attribute *mattr, char *buf) { - char *s; struct ir_input_dev *ir_dev = dev_get_drvdata(d); - u64 ir_type = ir_dev->rc_tab.ir_type; - - IR_dprintk(1, "Current protocol is %lld\n", (long long)ir_type); - - /* FIXME: doesn't support multiple protocols at the same time */ - if (ir_type == IR_TYPE_UNKNOWN) - s = "Unknown"; - else if (ir_type == IR_TYPE_RC5) - s = "rc-5"; - else if (ir_type == IR_TYPE_NEC) - s = "nec"; - else if (ir_type == IR_TYPE_RC6) - s = "rc6"; - else if (ir_type == IR_TYPE_JVC) - s = "jvc"; - else if (ir_type == IR_TYPE_SONY) - s = "sony"; - else - s = "other"; + u64 allowed, enabled; + char *tmp = buf; + + if (ir_dev->props->driver_type == RC_DRIVER_SCANCODE) { + enabled = ir_dev->rc_tab.ir_type; + allowed = ir_dev->props->allowed_protos; + } else { + enabled = ir_dev->raw->enabled_protocols; + allowed = ir_raw_get_allowed_protocols(); + } - return sprintf(buf, "%s\n", s); + IR_dprintk(1, "allowed - 0x%llx, enabled - 0x%llx\n", + (long long)allowed, + (long long)enabled); + + if (allowed & enabled & IR_TYPE_UNKNOWN) + tmp += sprintf(tmp, "[unknown] "); + else if (allowed & IR_TYPE_UNKNOWN) + tmp += sprintf(tmp, "unknown "); + + if (allowed & enabled & IR_TYPE_RC5) + tmp += sprintf(tmp, "[rc5] "); + else if (allowed & IR_TYPE_RC5) + tmp += sprintf(tmp, "rc5 "); + + if (allowed & enabled & IR_TYPE_NEC) + tmp += sprintf(tmp, "[nec] "); + else if (allowed & IR_TYPE_NEC) + tmp += sprintf(tmp, "nec "); + + if (allowed & enabled & IR_TYPE_RC6) + tmp += sprintf(tmp, "[rc6] "); + else if (allowed & IR_TYPE_RC6) + tmp += sprintf(tmp, "rc6 "); + + if (allowed & enabled & IR_TYPE_JVC) + tmp += sprintf(tmp, "[jvc] "); + else if (allowed & IR_TYPE_JVC) + tmp += sprintf(tmp, "jvc "); + + if (allowed & enabled & IR_TYPE_SONY) + tmp += sprintf(tmp, "[sony] "); + else if (allowed & IR_TYPE_SONY) + tmp += sprintf(tmp, "sony "); + + if (tmp != buf) + tmp--; + *tmp = '\n'; + return tmp + 1 - buf; } /** - * store_protocol() - shows the current IR protocol + * store_protocols() - changes the current IR protocol(s) * @d: the device descriptor * @mattr: the device attribute struct (unused) * @buf: a pointer to the input buffer * @len: length of the input buffer * * This routine is a callback routine for changing the IR protocol type. - * it is trigged by reading /sys/class/rc/rc?/current_protocol. - * It changes the IR the protocol name, if the IR type is recognized - * by the driver. - * If an unknown protocol name is used, returns -EINVAL. + * It is trigged by writing to /sys/class/rc/rc?/protocols. + * Writing "+proto" will add a protocol to the list of enabled protocols. + * Writing "-proto" will remove a protocol from the list of enabled protocols. + * Writing "proto" will enable only "proto". + * Returns -EINVAL if an invalid protocol combination or unknown protocol name + * is used, otherwise @len. */ -static ssize_t store_protocol(struct device *d, - struct device_attribute *mattr, - const char *data, - size_t len) +static ssize_t store_protocols(struct device *d, + struct device_attribute *mattr, + const char *data, + size_t len) { struct ir_input_dev *ir_dev = dev_get_drvdata(d); - u64 ir_type = 0; - int rc = -EINVAL; + bool enable, disable; + const char *tmp; + u64 type; + u64 mask; + int rc; unsigned long flags; - char *buf; - - while ((buf = strsep((char **) &data, " \n")) != NULL) { - if (!strcasecmp(buf, "rc-5") || !strcasecmp(buf, "rc5")) - ir_type |= IR_TYPE_RC5; - if (!strcasecmp(buf, "nec")) - ir_type |= IR_TYPE_NEC; - if (!strcasecmp(buf, "jvc")) - ir_type |= IR_TYPE_JVC; - if (!strcasecmp(buf, "sony")) - ir_type |= IR_TYPE_SONY; + + tmp = skip_spaces(data); + + if (*tmp == '+') { + enable = true; + disable = false; + tmp++; + } else if (*tmp == '-') { + enable = false; + disable = true; + tmp++; + } else { + enable = false; + disable = false; } - if (!ir_type) { + if (!strncasecmp(tmp, "unknown", 7)) { + tmp += 7; + mask = IR_TYPE_UNKNOWN; + } else if (!strncasecmp(tmp, "rc5", 3)) { + tmp += 3; + mask = IR_TYPE_RC5; + } else if (!strncasecmp(tmp, "nec", 3)) { + tmp += 3; + mask = IR_TYPE_NEC; + } else if (!strncasecmp(tmp, "rc6", 3)) { + tmp += 3; + mask = IR_TYPE_RC6; + } else if (!strncasecmp(tmp, "jvc", 3)) { + tmp += 3; + mask = IR_TYPE_JVC; + } else if (!strncasecmp(tmp, "sony", 4)) { + tmp += 4; + mask = IR_TYPE_SONY; + } else { IR_dprintk(1, "Unknown protocol\n"); return -EINVAL; } - if (ir_dev->props && ir_dev->props->change_protocol) - rc = ir_dev->props->change_protocol(ir_dev->props->priv, - ir_type); - - if (rc < 0) { - IR_dprintk(1, "Error setting protocol to %lld\n", - (long long)ir_type); + tmp = skip_spaces(tmp); + if (*tmp != '\0') { + IR_dprintk(1, "Invalid trailing characters\n"); return -EINVAL; } - spin_lock_irqsave(&ir_dev->rc_tab.lock, flags); - ir_dev->rc_tab.ir_type = ir_type; - spin_unlock_irqrestore(&ir_dev->rc_tab.lock, flags); + if (ir_dev->props->driver_type == RC_DRIVER_SCANCODE) + type = ir_dev->rc_tab.ir_type; + else + type = ir_dev->raw->enabled_protocols; - IR_dprintk(1, "Current protocol(s) is(are) %lld\n", - (long long)ir_type); + if (enable) + type |= mask; + else if (disable) + type &= ~mask; + else + type = mask; - return len; -} + if (ir_dev->props && ir_dev->props->change_protocol) { + rc = ir_dev->props->change_protocol(ir_dev->props->priv, + type); + if (rc < 0) { + IR_dprintk(1, "Error setting protocols to 0x%llx\n", + (long long)type); + return -EINVAL; + } + } -static ssize_t show_supported_protocols(struct device *d, - struct device_attribute *mattr, char *buf) -{ - char *orgbuf = buf; - struct ir_input_dev *ir_dev = dev_get_drvdata(d); + if (ir_dev->props->driver_type == RC_DRIVER_SCANCODE) { + spin_lock_irqsave(&ir_dev->rc_tab.lock, flags); + ir_dev->rc_tab.ir_type = type; + spin_unlock_irqrestore(&ir_dev->rc_tab.lock, flags); + } else { + ir_dev->raw->enabled_protocols = type; + } - /* FIXME: doesn't support multiple protocols at the same time */ - if (ir_dev->props->allowed_protos == IR_TYPE_UNKNOWN) - buf += sprintf(buf, "unknown "); - if (ir_dev->props->allowed_protos & IR_TYPE_RC5) - buf += sprintf(buf, "rc-5 "); - if (ir_dev->props->allowed_protos & IR_TYPE_NEC) - buf += sprintf(buf, "nec "); - if (buf == orgbuf) - buf += sprintf(buf, "other "); - buf += sprintf(buf - 1, "\n"); + IR_dprintk(1, "Current protocol(s): 0x%llx\n", + (long long)type); - return buf - orgbuf; + return len; } #define ADD_HOTPLUG_VAR(fmt, val...) \ @@ -158,7 +214,7 @@ static ssize_t show_supported_protocols(struct device *d, return err; \ } while (0) -static int ir_dev_uevent(struct device *device, struct kobj_uevent_env *env) +static int rc_dev_uevent(struct device *device, struct kobj_uevent_env *env) { struct ir_input_dev *ir_dev = dev_get_drvdata(device); @@ -173,34 +229,26 @@ static int ir_dev_uevent(struct device *device, struct kobj_uevent_env *env) /* * Static device attribute struct with the sysfs attributes for IR's */ -static DEVICE_ATTR(protocol, S_IRUGO | S_IWUSR, - show_protocol, store_protocol); +static DEVICE_ATTR(protocols, S_IRUGO | S_IWUSR, + show_protocols, store_protocols); -static DEVICE_ATTR(supported_protocols, S_IRUGO | S_IWUSR, - show_supported_protocols, NULL); - -static struct attribute *ir_hw_dev_attrs[] = { - &dev_attr_protocol.attr, - &dev_attr_supported_protocols.attr, +static struct attribute *rc_dev_attrs[] = { + &dev_attr_protocols.attr, NULL, }; -static struct attribute_group ir_hw_dev_attr_grp = { - .attrs = ir_hw_dev_attrs, +static struct attribute_group rc_dev_attr_grp = { + .attrs = rc_dev_attrs, }; -static const struct attribute_group *ir_hw_dev_attr_groups[] = { - &ir_hw_dev_attr_grp, +static const struct attribute_group *rc_dev_attr_groups[] = { + &rc_dev_attr_grp, NULL }; static struct device_type rc_dev_type = { - .groups = ir_hw_dev_attr_groups, - .uevent = ir_dev_uevent, -}; - -static struct device_type ir_raw_dev_type = { - .uevent = ir_dev_uevent, + .groups = rc_dev_attr_groups, + .uevent = rc_dev_uevent, }; /** @@ -220,11 +268,7 @@ int ir_register_class(struct input_dev *input_dev) if (unlikely(devno < 0)) return devno; - if (ir_dev->props->driver_type == RC_DRIVER_SCANCODE) - ir_dev->dev.type = &rc_dev_type; - else - ir_dev->dev.type = &ir_raw_dev_type; - + ir_dev->dev.type = &rc_dev_type; ir_dev->dev.class = &ir_input_class; ir_dev->dev.parent = input_dev->dev.parent; dev_set_name(&ir_dev->dev, "rc%d", devno); ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling 2010-04-24 21:14 ` [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling David Härdeman @ 2010-05-03 19:49 ` Mauro Carvalho Chehab 2010-06-07 18:48 ` David Härdeman 0 siblings, 1 reply; 25+ messages in thread From: Mauro Carvalho Chehab @ 2010-05-03 19:49 UTC (permalink / raw) To: David Härdeman; +Cc: linux-media, linux-input Hi David, David Härdeman wrote: > With the current logic, each raw decoder needs to add a copy of the exact > same sysfs code. This is both unnecessary and also means that (re)loading > an IR driver after raw decoder modules have been loaded won't work as > expected. > > This patch moves that logic into ir-raw-event and adds a single sysfs > file per device. > > Reading that file returns something like: > > "rc5 [rc6] nec jvc [sony]" > > (with enabled protocols in [] brackets) > > Writing either "+protocol" or "-protocol" to that file will > enable or disable the according protocol decoder. > > An additional benefit is that the disabling of a decoder will be > remembered across module removal/insertion so a previously > disabled decoder won't suddenly be activated again. The default > setting is to enable all decoders. > > This is also necessary for the next patch which moves even more decoder > state into the central raw decoding structs. I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver). So, while we can implement some core support at the raw event core, we should keep the decoder attributes internally inside the driver. So, each decoder may have his own code like: static struct attribute *decoder_attributes[] = { &dev_attr_enabled.attr, &dev_attr_bar1.attr, &dev_attr_bar2.attr, &dev_attr_bar3.attr, NULL }; static struct attribute_group decoder_attribute_group = { .name = "foo_decoder", .attrs = decoder_attributes, }; As the attr_enabled is common to all, maybe we can have a default enable code at the .h or inside the core. Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling 2010-05-03 19:49 ` Mauro Carvalho Chehab @ 2010-06-07 18:48 ` David Härdeman 0 siblings, 0 replies; 25+ messages in thread From: David Härdeman @ 2010-06-07 18:48 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, linux-input On Mon, May 03, 2010 at 04:49:20PM -0300, Mauro Carvalho Chehab wrote: > Hi David, Hi, sorry for dropping off the radar...life has been hetic...daughter in hospital (she's fine now). > David Härdeman wrote: > > With the current logic, each raw decoder needs to add a copy of the exact > > same sysfs code. This is both unnecessary and also means that (re)loading > > an IR driver after raw decoder modules have been loaded won't work as > > expected. > > > > This patch moves that logic into ir-raw-event and adds a single sysfs > > file per device. > > > > Reading that file returns something like: > > > > "rc5 [rc6] nec jvc [sony]" > > > > (with enabled protocols in [] brackets) > > > > Writing either "+protocol" or "-protocol" to that file will > > enable or disable the according protocol decoder. > > > > An additional benefit is that the disabling of a decoder will be > > remembered across module removal/insertion so a previously > > disabled decoder won't suddenly be activated again. The default > > setting is to enable all decoders. > > > > This is also necessary for the next patch which moves even more decoder > > state into the central raw decoding structs. > > I liked the idea of your redesign, but I didn't like the removal of > a per-decoder sysfs entry. As already discussed, there are cases where > we'll need a per-decoder sysfs entry (lirc_dev is probably one of those > cases - also Jarod's imon driver is currently implementing a modprobe > parameter that needs to be moved to the driver). per-decoder or per-decoder-per-receiver sysfs entries? If you want the latter, you'll need much more code than what is currently there to not break random module load order (which doesn't work with the current code). Additionally, *if* imon and lirc_dev really need something like that, those modules should carry the burden. > So, while we can implement some core support at the raw event core, we should > keep the decoder attributes internally inside the driver. Why? > So, each decoder > may have his own code like: > > static struct attribute *decoder_attributes[] = { > &dev_attr_enabled.attr, > &dev_attr_bar1.attr, > &dev_attr_bar2.attr, > &dev_attr_bar3.attr, > NULL > }; > > static struct attribute_group decoder_attribute_group = { > .name = "foo_decoder", > .attrs = decoder_attributes, > }; > > As the attr_enabled is common to all, maybe we can have a default enable > code at the .h or inside the core. Smells like lots of duplicated code with no gain for the current set of decoders. -- David Härdeman ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-04-24 21:13 [PATCH 0/4] ir-core sysfs protocol selection simplification David Härdeman 2010-04-24 21:14 ` [PATCH 1/4] ir-core: remove IR_TYPE_PD David Härdeman 2010-04-24 21:14 ` [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling David Härdeman @ 2010-04-24 21:14 ` David Härdeman 2010-05-03 20:00 ` Mauro Carvalho Chehab 2010-04-24 21:14 ` [PATCH 4/4] ir-core: remove ir-functions usage from cx231xx David Härdeman 3 siblings, 1 reply; 25+ messages in thread From: David Härdeman @ 2010-04-24 21:14 UTC (permalink / raw) To: mchehab; +Cc: linux-media, linux-input This patch moves the state from each raw decoder into the ir_raw_event_ctrl struct. This allows the removal of code like this: spin_lock(&decoder_lock); list_for_each_entry(data, &decoder_list, list) { if (data->ir_dev == ir_dev) break; } spin_unlock(&decoder_lock); return data; which is currently run for each decoder on each event in order to get the client-specific decoding state data. In addition, ir decoding modules and ir driver module load order is now independent. Centralizing the data also allows for a nice code reduction of about 30% per raw decoder as client lists and client registration callbacks are no longer necessary. Out-of-tree modules can still use a similar trick to what the raw decoders did before this patch until they are merged. Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/IR/ir-core-priv.h | 37 ++++++++++++- drivers/media/IR/ir-jvc-decoder.c | 90 ++----------------------------- drivers/media/IR/ir-nec-decoder.c | 89 +++---------------------------- drivers/media/IR/ir-raw-event.c | 48 +++-------------- drivers/media/IR/ir-rc5-decoder.c | 103 +++++------------------------------- drivers/media/IR/ir-rc6-decoder.c | 92 ++------------------------------ drivers/media/IR/ir-sony-decoder.c | 93 +++------------------------------ 7 files changed, 87 insertions(+), 465 deletions(-) diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h index 821d012..1e9464a 100644 --- a/drivers/media/IR/ir-core-priv.h +++ b/drivers/media/IR/ir-core-priv.h @@ -23,8 +23,6 @@ struct ir_raw_handler { u64 protocols; /* which are handled by this handler */ int (*decode)(struct input_dev *input_dev, struct ir_raw_event event); - int (*raw_register)(struct input_dev *input_dev); - int (*raw_unregister)(struct input_dev *input_dev); }; struct ir_raw_event_ctrl { @@ -34,6 +32,41 @@ struct ir_raw_event_ctrl { enum raw_event_type last_type; /* last event type */ struct input_dev *input_dev; /* pointer to the parent input_dev */ u64 enabled_protocols; /* enabled raw protocol decoders */ + + /* raw decoder state follows */ + struct ir_raw_event prev_ev; + struct nec_dec { + int state; + unsigned count; + u32 bits; + } nec; + struct rc5_dec { + int state; + u32 bits; + unsigned count; + unsigned wanted_bits; + } rc5; + struct rc6_dec { + int state; + u8 header; + u32 body; + bool toggle; + unsigned count; + unsigned wanted_bits; + } rc6; + struct sony_dec { + int state; + u32 bits; + unsigned count; + } sony; + struct jvc_dec { + int state; + u16 bits; + u16 old_bits; + unsigned count; + bool first; + bool toggle; + } jvc; }; /* macros for IR decoders */ diff --git a/drivers/media/IR/ir-jvc-decoder.c b/drivers/media/IR/ir-jvc-decoder.c index 1055de4..8894d8b 100644 --- a/drivers/media/IR/ir-jvc-decoder.c +++ b/drivers/media/IR/ir-jvc-decoder.c @@ -25,10 +25,6 @@ #define JVC_TRAILER_PULSE (1 * JVC_UNIT) #define JVC_TRAILER_SPACE (35 * JVC_UNIT) -/* Used to register jvc_decoder clients */ -static LIST_HEAD(decoder_list); -DEFINE_SPINLOCK(decoder_lock); - enum jvc_state { STATE_INACTIVE, STATE_HEADER_SPACE, @@ -38,39 +34,6 @@ enum jvc_state { STATE_TRAILER_SPACE, }; -struct decoder_data { - struct list_head list; - struct ir_input_dev *ir_dev; - - /* State machine control */ - enum jvc_state state; - u16 jvc_bits; - u16 jvc_old_bits; - unsigned count; - bool first; - bool toggle; -}; - - -/** - * get_decoder_data() - gets decoder data - * @input_dev: input device - * - * Returns the struct decoder_data that corresponds to a device - */ -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) -{ - struct decoder_data *data = NULL; - - spin_lock(&decoder_lock); - list_for_each_entry(data, &decoder_list, list) { - if (data->ir_dev == ir_dev) - break; - } - spin_unlock(&decoder_lock); - return data; -} - /** * ir_jvc_decode() - Decode one JVC pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -80,12 +43,8 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) */ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) { - struct decoder_data *data; struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - - data = get_decoder_data(ir_dev); - if (!data) - return -EINVAL; + struct jvc_dec *data = &ir_dev->raw->jvc; if (!(ir_dev->raw->enabled_protocols & IR_TYPE_JVC)) return 0; @@ -140,9 +99,9 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (ev.pulse) break; - data->jvc_bits <<= 1; + data->bits <<= 1; if (eq_margin(ev.duration, JVC_BIT_1_SPACE, JVC_UNIT / 2)) { - data->jvc_bits |= 1; + data->bits |= 1; decrease_duration(&ev, JVC_BIT_1_SPACE); } else if (eq_margin(ev.duration, JVC_BIT_0_SPACE, JVC_UNIT / 2)) decrease_duration(&ev, JVC_BIT_0_SPACE); @@ -175,13 +134,13 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (data->first) { u32 scancode; - scancode = (bitrev8((data->jvc_bits >> 8) & 0xff) << 8) | - (bitrev8((data->jvc_bits >> 0) & 0xff) << 0); + scancode = (bitrev8((data->bits >> 8) & 0xff) << 8) | + (bitrev8((data->bits >> 0) & 0xff) << 0); IR_dprintk(1, "JVC scancode 0x%04x\n", scancode); ir_keydown(input_dev, scancode, data->toggle); data->first = false; - data->jvc_old_bits = data->jvc_bits; - } else if (data->jvc_bits == data->jvc_old_bits) { + data->old_bits = data->bits; + } else if (data->bits == data->old_bits) { IR_dprintk(1, "JVC repeat\n"); ir_repeat(input_dev); } else { @@ -201,44 +160,9 @@ out: return -EINVAL; } -static int ir_jvc_register(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - struct decoder_data *data; - - data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) - return -ENOMEM; - - data->ir_dev = ir_dev; - spin_lock(&decoder_lock); - list_add_tail(&data->list, &decoder_list); - spin_unlock(&decoder_lock); - - return 0; -} - -static int ir_jvc_unregister(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - static struct decoder_data *data; - - data = get_decoder_data(ir_dev); - if (!data) - return 0; - - spin_lock(&decoder_lock); - list_del(&data->list); - spin_unlock(&decoder_lock); - - return 0; -} - static struct ir_raw_handler jvc_handler = { .protocols = IR_TYPE_JVC, .decode = ir_jvc_decode, - .raw_register = ir_jvc_register, - .raw_unregister = ir_jvc_unregister, }; static int __init ir_jvc_decode_init(void) diff --git a/drivers/media/IR/ir-nec-decoder.c b/drivers/media/IR/ir-nec-decoder.c index 2cc2b92..52e0f37 100644 --- a/drivers/media/IR/ir-nec-decoder.c +++ b/drivers/media/IR/ir-nec-decoder.c @@ -27,10 +27,6 @@ #define NEC_TRAILER_PULSE (1 * NEC_UNIT) #define NEC_TRAILER_SPACE (10 * NEC_UNIT) /* even longer in reality */ -/* Used to register nec_decoder clients */ -static LIST_HEAD(decoder_list); -static DEFINE_SPINLOCK(decoder_lock); - enum nec_state { STATE_INACTIVE, STATE_HEADER_SPACE, @@ -40,36 +36,6 @@ enum nec_state { STATE_TRAILER_SPACE, }; -struct decoder_data { - struct list_head list; - struct ir_input_dev *ir_dev; - - /* State machine control */ - enum nec_state state; - u32 nec_bits; - unsigned count; -}; - - -/** - * get_decoder_data() - gets decoder data - * @input_dev: input device - * - * Returns the struct decoder_data that corresponds to a device - */ -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) -{ - struct decoder_data *data = NULL; - - spin_lock(&decoder_lock); - list_for_each_entry(data, &decoder_list, list) { - if (data->ir_dev == ir_dev) - break; - } - spin_unlock(&decoder_lock); - return data; -} - /** * ir_nec_decode() - Decode one NEC pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -79,15 +45,11 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) */ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) { - struct decoder_data *data; struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); + struct nec_dec *data = &ir_dev->raw->nec; u32 scancode; u8 address, not_address, command, not_command; - data = get_decoder_data(ir_dev); - if (!data) - return -EINVAL; - if (!(ir_dev->raw->enabled_protocols & IR_TYPE_NEC)) return 0; @@ -143,9 +105,9 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (ev.pulse) break; - data->nec_bits <<= 1; + data->bits <<= 1; if (eq_margin(ev.duration, NEC_BIT_1_SPACE, NEC_UNIT / 2)) - data->nec_bits |= 1; + data->bits |= 1; else if (!eq_margin(ev.duration, NEC_BIT_0_SPACE, NEC_UNIT / 2)) break; data->count++; @@ -174,14 +136,14 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2)) break; - address = bitrev8((data->nec_bits >> 24) & 0xff); - not_address = bitrev8((data->nec_bits >> 16) & 0xff); - command = bitrev8((data->nec_bits >> 8) & 0xff); - not_command = bitrev8((data->nec_bits >> 0) & 0xff); + address = bitrev8((data->bits >> 24) & 0xff); + not_address = bitrev8((data->bits >> 16) & 0xff); + 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->nec_bits); + data->bits); break; } @@ -208,44 +170,9 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) return -EINVAL; } -static int ir_nec_register(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - struct decoder_data *data; - - data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) - return -ENOMEM; - - data->ir_dev = ir_dev; - spin_lock(&decoder_lock); - list_add_tail(&data->list, &decoder_list); - spin_unlock(&decoder_lock); - - return 0; -} - -static int ir_nec_unregister(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - static struct decoder_data *data; - - data = get_decoder_data(ir_dev); - if (!data) - return 0; - - spin_lock(&decoder_lock); - list_del(&data->list); - spin_unlock(&decoder_lock); - - return 0; -} - static struct ir_raw_handler nec_handler = { .protocols = IR_TYPE_NEC, .decode = ir_nec_decode, - .raw_register = ir_nec_register, - .raw_unregister = ir_nec_unregister, }; static int __init ir_nec_decode_init(void) diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c index eeca8a8..eb77378 100644 --- a/drivers/media/IR/ir-raw-event.c +++ b/drivers/media/IR/ir-raw-event.c @@ -25,32 +25,6 @@ static DEFINE_SPINLOCK(ir_raw_handler_lock); static LIST_HEAD(ir_raw_handler_list); static u64 available_protocols; -/** - * RUN_DECODER() - runs an operation on all IR decoders - * @ops: IR raw handler operation to be called - * @arg: arguments to be passed to the callback - * - * Calls ir_raw_handler::ops for all registered IR handlers. It prevents - * new decode addition/removal while running, by locking ir_raw_handler_lock - * mutex. If an error occurs, it stops the ops. Otherwise, it returns a sum - * of the return codes. - */ -#define RUN_DECODER(ops, ...) ({ \ - struct ir_raw_handler *_ir_raw_handler; \ - int _sumrc = 0, _rc; \ - spin_lock(&ir_raw_handler_lock); \ - list_for_each_entry(_ir_raw_handler, &ir_raw_handler_list, list) { \ - if (_ir_raw_handler->ops) { \ - _rc = _ir_raw_handler->ops(__VA_ARGS__); \ - if (_rc < 0) \ - break; \ - _sumrc += _rc; \ - } \ - } \ - spin_unlock(&ir_raw_handler_lock); \ - _sumrc; \ -}) - #ifdef MODULE /* Used to load the decoders */ static struct work_struct wq_load; @@ -59,11 +33,17 @@ static struct work_struct wq_load; static void ir_raw_event_work(struct work_struct *work) { struct ir_raw_event ev; + struct ir_raw_handler *handler; struct ir_raw_event_ctrl *raw = container_of(work, struct ir_raw_event_ctrl, rx_work); - while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) - RUN_DECODER(decode, raw->input_dev, ev); + while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) { + spin_lock(&ir_raw_handler_lock); + list_for_each_entry(handler, &ir_raw_handler_list, list) + handler->decode(raw->input_dev, ev); + spin_unlock(&ir_raw_handler_lock); + raw->prev_ev = ev; + } } /** @@ -193,15 +173,7 @@ int ir_raw_event_register(struct input_dev *input_dev) return rc; } - rc = RUN_DECODER(raw_register, input_dev); - if (rc < 0) { - kfifo_free(&ir->raw->kfifo); - kfree(ir->raw); - ir->raw = NULL; - return rc; - } - - return rc; + return 0; } void ir_raw_event_unregister(struct input_dev *input_dev) @@ -212,8 +184,6 @@ void ir_raw_event_unregister(struct input_dev *input_dev) return; cancel_work_sync(&ir->raw->rx_work); - RUN_DECODER(raw_unregister, input_dev); - kfifo_free(&ir->raw->kfifo); kfree(ir->raw); ir->raw = NULL; diff --git a/drivers/media/IR/ir-rc5-decoder.c b/drivers/media/IR/ir-rc5-decoder.c index 1be8981..7af656d 100644 --- a/drivers/media/IR/ir-rc5-decoder.c +++ b/drivers/media/IR/ir-rc5-decoder.c @@ -30,10 +30,6 @@ #define RC5_BIT_END (1 * RC5_UNIT) #define RC5X_SPACE (4 * RC5_UNIT) -/* Used to register rc5_decoder clients */ -static LIST_HEAD(decoder_list); -static DEFINE_SPINLOCK(decoder_lock); - enum rc5_state { STATE_INACTIVE, STATE_BIT_START, @@ -42,39 +38,6 @@ enum rc5_state { STATE_FINISHED, }; -struct decoder_data { - struct list_head list; - struct ir_input_dev *ir_dev; - - /* State machine control */ - enum rc5_state state; - u32 rc5_bits; - struct ir_raw_event prev_ev; - unsigned count; - unsigned wanted_bits; -}; - - -/** - * get_decoder_data() - gets decoder data - * @input_dev: input device - * - * Returns the struct decoder_data that corresponds to a device - */ - -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) -{ - struct decoder_data *data = NULL; - - spin_lock(&decoder_lock); - list_for_each_entry(data, &decoder_list, list) { - if (data->ir_dev == ir_dev) - break; - } - spin_unlock(&decoder_lock); - return data; -} - /** * ir_rc5_decode() - Decode one RC-5 pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -84,15 +47,11 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) */ static int ir_rc5_decode(struct input_dev *input_dev, struct ir_raw_event ev) { - struct decoder_data *data; struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); + struct rc5_dec *data = &ir_dev->raw->rc5; u8 toggle; u32 scancode; - data = get_decoder_data(ir_dev); - if (!data) - return -EINVAL; - if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC5)) return 0; @@ -128,16 +87,15 @@ again: if (!eq_margin(ev.duration, RC5_BIT_START, RC5_UNIT / 2)) break; - data->rc5_bits <<= 1; + data->bits <<= 1; if (!ev.pulse) - data->rc5_bits |= 1; + data->bits |= 1; data->count++; - data->prev_ev = ev; data->state = STATE_BIT_END; return 0; case STATE_BIT_END: - if (!is_transition(&ev, &data->prev_ev)) + if (!is_transition(&ev, &ir_dev->raw->prev_ev)) break; if (data->count == data->wanted_bits) @@ -169,11 +127,11 @@ again: if (data->wanted_bits == RC5X_NBITS) { /* RC5X */ u8 xdata, command, system; - xdata = (data->rc5_bits & 0x0003F) >> 0; - command = (data->rc5_bits & 0x00FC0) >> 6; - system = (data->rc5_bits & 0x1F000) >> 12; - toggle = (data->rc5_bits & 0x20000) ? 1 : 0; - command += (data->rc5_bits & 0x01000) ? 0 : 0x40; + xdata = (data->bits & 0x0003F) >> 0; + command = (data->bits & 0x00FC0) >> 6; + system = (data->bits & 0x1F000) >> 12; + toggle = (data->bits & 0x20000) ? 1 : 0; + command += (data->bits & 0x01000) ? 0 : 0x40; scancode = system << 16 | command << 8 | xdata; IR_dprintk(1, "RC5X scancode 0x%06x (toggle: %u)\n", @@ -182,10 +140,10 @@ again: } else { /* RC5 */ u8 command, system; - command = (data->rc5_bits & 0x0003F) >> 0; - system = (data->rc5_bits & 0x007C0) >> 6; - toggle = (data->rc5_bits & 0x00800) ? 1 : 0; - command += (data->rc5_bits & 0x01000) ? 0 : 0x40; + command = (data->bits & 0x0003F) >> 0; + system = (data->bits & 0x007C0) >> 6; + toggle = (data->bits & 0x00800) ? 1 : 0; + command += (data->bits & 0x01000) ? 0 : 0x40; scancode = system << 8 | command; IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n", @@ -204,44 +162,9 @@ out: return -EINVAL; } -static int ir_rc5_register(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - struct decoder_data *data; - - data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) - return -ENOMEM; - - data->ir_dev = ir_dev; - spin_lock(&decoder_lock); - list_add_tail(&data->list, &decoder_list); - spin_unlock(&decoder_lock); - - return 0; -} - -static int ir_rc5_unregister(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - static struct decoder_data *data; - - data = get_decoder_data(ir_dev); - if (!data) - return 0; - - spin_lock(&decoder_lock); - list_del(&data->list); - spin_unlock(&decoder_lock); - - return 0; -} - static struct ir_raw_handler rc5_handler = { .protocols = IR_TYPE_RC5, .decode = ir_rc5_decode, - .raw_register = ir_rc5_register, - .raw_unregister = ir_rc5_unregister, }; static int __init ir_rc5_decode_init(void) diff --git a/drivers/media/IR/ir-rc6-decoder.c b/drivers/media/IR/ir-rc6-decoder.c index 5e940a8..a562952 100644 --- a/drivers/media/IR/ir-rc6-decoder.c +++ b/drivers/media/IR/ir-rc6-decoder.c @@ -36,10 +36,6 @@ #define RC6_STARTBIT_MASK 0x08 /* for the header bits */ #define RC6_6A_MCE_TOGGLE_MASK 0x8000 /* for the body bits */ -/* Used to register rc6_decoder clients */ -static LIST_HEAD(decoder_list); -static DEFINE_SPINLOCK(decoder_lock); - enum rc6_mode { RC6_MODE_0, RC6_MODE_6A, @@ -58,41 +54,7 @@ enum rc6_state { STATE_FINISHED, }; -struct decoder_data { - struct list_head list; - struct ir_input_dev *ir_dev; - - /* State machine control */ - enum rc6_state state; - u8 header; - u32 body; - struct ir_raw_event prev_ev; - bool toggle; - unsigned count; - unsigned wanted_bits; -}; - - -/** - * get_decoder_data() - gets decoder data - * @input_dev: input device - * - * Returns the struct decoder_data that corresponds to a device - */ -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) -{ - struct decoder_data *data = NULL; - - spin_lock(&decoder_lock); - list_for_each_entry(data, &decoder_list, list) { - if (data->ir_dev == ir_dev) - break; - } - spin_unlock(&decoder_lock); - return data; -} - -static enum rc6_mode rc6_mode(struct decoder_data *data) { +static enum rc6_mode rc6_mode(struct rc6_dec *data) { switch (data->header & RC6_MODE_MASK) { case 0: return RC6_MODE_0; @@ -114,15 +76,11 @@ static enum rc6_mode rc6_mode(struct decoder_data *data) { */ static int ir_rc6_decode(struct input_dev *input_dev, struct ir_raw_event ev) { - struct decoder_data *data; struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); + struct rc6_dec *data = &ir_dev->raw->rc6; u32 scancode; u8 toggle; - data = get_decoder_data(ir_dev); - if (!data) - return -EINVAL; - if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC6)) return 0; @@ -175,12 +133,11 @@ again: if (ev.pulse) data->header |= 1; data->count++; - data->prev_ev = ev; data->state = STATE_HEADER_BIT_END; return 0; case STATE_HEADER_BIT_END: - if (!is_transition(&ev, &data->prev_ev)) + if (!is_transition(&ev, &ir_dev->raw->prev_ev)) break; if (data->count == RC6_HEADER_NBITS) @@ -196,12 +153,11 @@ again: break; data->toggle = ev.pulse; - data->prev_ev = ev; data->state = STATE_TOGGLE_END; return 0; case STATE_TOGGLE_END: - if (!is_transition(&ev, &data->prev_ev) || + if (!is_transition(&ev, &ir_dev->raw->prev_ev) || !geq_margin(ev.duration, RC6_TOGGLE_END, RC6_UNIT / 2)) break; @@ -211,7 +167,6 @@ again: } data->state = STATE_BODY_BIT_START; - data->prev_ev = ev; decrease_duration(&ev, RC6_TOGGLE_END); data->count = 0; @@ -243,13 +198,11 @@ again: if (ev.pulse) data->body |= 1; data->count++; - data->prev_ev = ev; - data->state = STATE_BODY_BIT_END; return 0; case STATE_BODY_BIT_END: - if (!is_transition(&ev, &data->prev_ev)) + if (!is_transition(&ev, &ir_dev->raw->prev_ev)) break; if (data->count == data->wanted_bits) @@ -300,44 +253,9 @@ out: return -EINVAL; } -static int ir_rc6_register(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - struct decoder_data *data; - - data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) - return -ENOMEM; - - data->ir_dev = ir_dev; - spin_lock(&decoder_lock); - list_add_tail(&data->list, &decoder_list); - spin_unlock(&decoder_lock); - - return 0; -} - -static int ir_rc6_unregister(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - static struct decoder_data *data; - - data = get_decoder_data(ir_dev); - if (!data) - return 0; - - spin_lock(&decoder_lock); - list_del(&data->list); - spin_unlock(&decoder_lock); - - return 0; -} - static struct ir_raw_handler rc6_handler = { .protocols = IR_TYPE_RC6, .decode = ir_rc6_decode, - .raw_register = ir_rc6_register, - .raw_unregister = ir_rc6_unregister, }; static int __init ir_rc6_decode_init(void) diff --git a/drivers/media/IR/ir-sony-decoder.c b/drivers/media/IR/ir-sony-decoder.c index 8afd16a..b9074f0 100644 --- a/drivers/media/IR/ir-sony-decoder.c +++ b/drivers/media/IR/ir-sony-decoder.c @@ -23,10 +23,6 @@ #define SONY_BIT_SPACE (1 * SONY_UNIT) #define SONY_TRAILER_SPACE (10 * SONY_UNIT) /* minimum */ -/* Used to register sony_decoder clients */ -static LIST_HEAD(decoder_list); -static DEFINE_SPINLOCK(decoder_lock); - enum sony_state { STATE_INACTIVE, STATE_HEADER_SPACE, @@ -35,36 +31,6 @@ enum sony_state { STATE_FINISHED, }; -struct decoder_data { - struct list_head list; - struct ir_input_dev *ir_dev; - - /* State machine control */ - enum sony_state state; - u32 sony_bits; - unsigned count; -}; - - -/** - * get_decoder_data() - gets decoder data - * @input_dev: input device - * - * Returns the struct decoder_data that corresponds to a device - */ -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) -{ - struct decoder_data *data = NULL; - - spin_lock(&decoder_lock); - list_for_each_entry(data, &decoder_list, list) { - if (data->ir_dev == ir_dev) - break; - } - spin_unlock(&decoder_lock); - return data; -} - /** * ir_sony_decode() - Decode one Sony pulse or space * @input_dev: the struct input_dev descriptor of the device @@ -74,15 +40,11 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) */ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) { - struct decoder_data *data; struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); + struct sony_dec *data = &ir_dev->raw->sony; u32 scancode; u8 device, subdevice, function; - data = get_decoder_data(ir_dev); - if (!data) - return -EINVAL; - if (!(ir_dev->raw->enabled_protocols & IR_TYPE_SONY)) return 0; @@ -124,9 +86,9 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) if (!ev.pulse) break; - data->sony_bits <<= 1; + data->bits <<= 1; if (eq_margin(ev.duration, SONY_BIT_1_PULSE, SONY_UNIT / 2)) - data->sony_bits |= 1; + data->bits |= 1; else if (!eq_margin(ev.duration, SONY_BIT_0_PULSE, SONY_UNIT / 2)) break; @@ -160,19 +122,19 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) switch (data->count) { case 12: - device = bitrev8((data->sony_bits << 3) & 0xF8); + device = bitrev8((data->bits << 3) & 0xF8); subdevice = 0; - function = bitrev8((data->sony_bits >> 4) & 0xFE); + function = bitrev8((data->bits >> 4) & 0xFE); break; case 15: - device = bitrev8((data->sony_bits >> 0) & 0xFF); + device = bitrev8((data->bits >> 0) & 0xFF); subdevice = 0; - function = bitrev8((data->sony_bits >> 7) & 0xFD); + function = bitrev8((data->bits >> 7) & 0xFD); break; case 20: - device = bitrev8((data->sony_bits >> 5) & 0xF8); - subdevice = bitrev8((data->sony_bits >> 0) & 0xFF); - function = bitrev8((data->sony_bits >> 12) & 0xFE); + device = bitrev8((data->bits >> 5) & 0xF8); + subdevice = bitrev8((data->bits >> 0) & 0xFF); + function = bitrev8((data->bits >> 12) & 0xFE); break; default: IR_dprintk(1, "Sony invalid bitcount %u\n", data->count); @@ -193,44 +155,9 @@ out: return -EINVAL; } -static int ir_sony_register(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - struct decoder_data *data; - - data = kzalloc(sizeof(*data), GFP_KERNEL); - if (!data) - return -ENOMEM; - - data->ir_dev = ir_dev; - spin_lock(&decoder_lock); - list_add_tail(&data->list, &decoder_list); - spin_unlock(&decoder_lock); - - return 0; -} - -static int ir_sony_unregister(struct input_dev *input_dev) -{ - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); - static struct decoder_data *data; - - data = get_decoder_data(ir_dev); - if (!data) - return 0; - - spin_lock(&decoder_lock); - list_del(&data->list); - spin_unlock(&decoder_lock); - - return 0; -} - static struct ir_raw_handler sony_handler = { .protocols = IR_TYPE_SONY, .decode = ir_sony_decode, - .raw_register = ir_sony_register, - .raw_unregister = ir_sony_unregister, }; static int __init ir_sony_decode_init(void) ^ permalink raw reply related [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-04-24 21:14 ` [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl David Härdeman @ 2010-05-03 20:00 ` Mauro Carvalho Chehab 2010-06-07 19:00 ` David Härdeman 0 siblings, 1 reply; 25+ messages in thread From: Mauro Carvalho Chehab @ 2010-05-03 20:00 UTC (permalink / raw) To: David Härdeman; +Cc: linux-media, linux-input David Härdeman wrote: > This patch moves the state from each raw decoder into the > ir_raw_event_ctrl struct. > > This allows the removal of code like this: > > spin_lock(&decoder_lock); > list_for_each_entry(data, &decoder_list, list) { > if (data->ir_dev == ir_dev) > break; > } > spin_unlock(&decoder_lock); > return data; > > which is currently run for each decoder on each event in order > to get the client-specific decoding state data. > > In addition, ir decoding modules and ir driver module load > order is now independent. Centralizing the data also allows > for a nice code reduction of about 30% per raw decoder as > client lists and client registration callbacks are no longer > necessary. The registration callbacks will likely still be needed by lirc, as you need to create/delete lirc_dev interfaces, when the module is registered, but I might be wrong. It would be interesting to add lirc_dev first, in order to be sure about the better interfaces for it. Also, from one side, you reduced the code size, but, on the other hand, you've increased the memory usage, as now the protocol data will be stored even for protocols that weren't compiled/loaded. Probably, the code size savings are big enough to justify the runtime memory footprint, at least with the current decoders. Not sure how big this will become when we add lirc_dev and other decoders that might be missing. > > Out-of-tree modules can still use a similar trick to what > the raw decoders did before this patch until they are merged. > > Signed-off-by: David Härdeman <david@hardeman.nu> > --- > drivers/media/IR/ir-core-priv.h | 37 ++++++++++++- > drivers/media/IR/ir-jvc-decoder.c | 90 ++----------------------------- > drivers/media/IR/ir-nec-decoder.c | 89 +++---------------------------- > drivers/media/IR/ir-raw-event.c | 48 +++-------------- > drivers/media/IR/ir-rc5-decoder.c | 103 +++++------------------------------- > drivers/media/IR/ir-rc6-decoder.c | 92 ++------------------------------ > drivers/media/IR/ir-sony-decoder.c | 93 +++------------------------------ > 7 files changed, 87 insertions(+), 465 deletions(-) > > diff --git a/drivers/media/IR/ir-core-priv.h b/drivers/media/IR/ir-core-priv.h > index 821d012..1e9464a 100644 > --- a/drivers/media/IR/ir-core-priv.h > +++ b/drivers/media/IR/ir-core-priv.h > @@ -23,8 +23,6 @@ struct ir_raw_handler { > > u64 protocols; /* which are handled by this handler */ > int (*decode)(struct input_dev *input_dev, struct ir_raw_event event); > - int (*raw_register)(struct input_dev *input_dev); > - int (*raw_unregister)(struct input_dev *input_dev); > }; > > struct ir_raw_event_ctrl { > @@ -34,6 +32,41 @@ struct ir_raw_event_ctrl { > enum raw_event_type last_type; /* last event type */ > struct input_dev *input_dev; /* pointer to the parent input_dev */ > u64 enabled_protocols; /* enabled raw protocol decoders */ > + > + /* raw decoder state follows */ > + struct ir_raw_event prev_ev; > + struct nec_dec { > + int state; > + unsigned count; > + u32 bits; > + } nec; > + struct rc5_dec { > + int state; > + u32 bits; > + unsigned count; > + unsigned wanted_bits; > + } rc5; > + struct rc6_dec { > + int state; > + u8 header; > + u32 body; > + bool toggle; > + unsigned count; > + unsigned wanted_bits; > + } rc6; > + struct sony_dec { > + int state; > + u32 bits; > + unsigned count; > + } sony; > + struct jvc_dec { > + int state; > + u16 bits; > + u16 old_bits; > + unsigned count; > + bool first; > + bool toggle; > + } jvc; > }; > > /* macros for IR decoders */ > diff --git a/drivers/media/IR/ir-jvc-decoder.c b/drivers/media/IR/ir-jvc-decoder.c > index 1055de4..8894d8b 100644 > --- a/drivers/media/IR/ir-jvc-decoder.c > +++ b/drivers/media/IR/ir-jvc-decoder.c > @@ -25,10 +25,6 @@ > #define JVC_TRAILER_PULSE (1 * JVC_UNIT) > #define JVC_TRAILER_SPACE (35 * JVC_UNIT) > > -/* Used to register jvc_decoder clients */ > -static LIST_HEAD(decoder_list); > -DEFINE_SPINLOCK(decoder_lock); > - > enum jvc_state { > STATE_INACTIVE, > STATE_HEADER_SPACE, > @@ -38,39 +34,6 @@ enum jvc_state { > STATE_TRAILER_SPACE, > }; > > -struct decoder_data { > - struct list_head list; > - struct ir_input_dev *ir_dev; > - > - /* State machine control */ > - enum jvc_state state; > - u16 jvc_bits; > - u16 jvc_old_bits; > - unsigned count; > - bool first; > - bool toggle; > -}; > - > - > -/** > - * get_decoder_data() - gets decoder data > - * @input_dev: input device > - * > - * Returns the struct decoder_data that corresponds to a device > - */ > -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > -{ > - struct decoder_data *data = NULL; > - > - spin_lock(&decoder_lock); > - list_for_each_entry(data, &decoder_list, list) { > - if (data->ir_dev == ir_dev) > - break; > - } > - spin_unlock(&decoder_lock); > - return data; > -} > - > /** > * ir_jvc_decode() - Decode one JVC pulse or space > * @input_dev: the struct input_dev descriptor of the device > @@ -80,12 +43,8 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > */ > static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) > { > - struct decoder_data *data; > struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - > - data = get_decoder_data(ir_dev); > - if (!data) > - return -EINVAL; > + struct jvc_dec *data = &ir_dev->raw->jvc; > > if (!(ir_dev->raw->enabled_protocols & IR_TYPE_JVC)) > return 0; > @@ -140,9 +99,9 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) > if (ev.pulse) > break; > > - data->jvc_bits <<= 1; > + data->bits <<= 1; > if (eq_margin(ev.duration, JVC_BIT_1_SPACE, JVC_UNIT / 2)) { > - data->jvc_bits |= 1; > + data->bits |= 1; > decrease_duration(&ev, JVC_BIT_1_SPACE); > } else if (eq_margin(ev.duration, JVC_BIT_0_SPACE, JVC_UNIT / 2)) > decrease_duration(&ev, JVC_BIT_0_SPACE); > @@ -175,13 +134,13 @@ static int ir_jvc_decode(struct input_dev *input_dev, struct ir_raw_event ev) > > if (data->first) { > u32 scancode; > - scancode = (bitrev8((data->jvc_bits >> 8) & 0xff) << 8) | > - (bitrev8((data->jvc_bits >> 0) & 0xff) << 0); > + scancode = (bitrev8((data->bits >> 8) & 0xff) << 8) | > + (bitrev8((data->bits >> 0) & 0xff) << 0); > IR_dprintk(1, "JVC scancode 0x%04x\n", scancode); > ir_keydown(input_dev, scancode, data->toggle); > data->first = false; > - data->jvc_old_bits = data->jvc_bits; > - } else if (data->jvc_bits == data->jvc_old_bits) { > + data->old_bits = data->bits; > + } else if (data->bits == data->old_bits) { > IR_dprintk(1, "JVC repeat\n"); > ir_repeat(input_dev); > } else { > @@ -201,44 +160,9 @@ out: > return -EINVAL; > } > > -static int ir_jvc_register(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - struct decoder_data *data; > - > - data = kzalloc(sizeof(*data), GFP_KERNEL); > - if (!data) > - return -ENOMEM; > - > - data->ir_dev = ir_dev; > - spin_lock(&decoder_lock); > - list_add_tail(&data->list, &decoder_list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > -static int ir_jvc_unregister(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - static struct decoder_data *data; > - > - data = get_decoder_data(ir_dev); > - if (!data) > - return 0; > - > - spin_lock(&decoder_lock); > - list_del(&data->list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > static struct ir_raw_handler jvc_handler = { > .protocols = IR_TYPE_JVC, > .decode = ir_jvc_decode, > - .raw_register = ir_jvc_register, > - .raw_unregister = ir_jvc_unregister, > }; > > static int __init ir_jvc_decode_init(void) > diff --git a/drivers/media/IR/ir-nec-decoder.c b/drivers/media/IR/ir-nec-decoder.c > index 2cc2b92..52e0f37 100644 > --- a/drivers/media/IR/ir-nec-decoder.c > +++ b/drivers/media/IR/ir-nec-decoder.c > @@ -27,10 +27,6 @@ > #define NEC_TRAILER_PULSE (1 * NEC_UNIT) > #define NEC_TRAILER_SPACE (10 * NEC_UNIT) /* even longer in reality */ > > -/* Used to register nec_decoder clients */ > -static LIST_HEAD(decoder_list); > -static DEFINE_SPINLOCK(decoder_lock); > - > enum nec_state { > STATE_INACTIVE, > STATE_HEADER_SPACE, > @@ -40,36 +36,6 @@ enum nec_state { > STATE_TRAILER_SPACE, > }; > > -struct decoder_data { > - struct list_head list; > - struct ir_input_dev *ir_dev; > - > - /* State machine control */ > - enum nec_state state; > - u32 nec_bits; > - unsigned count; > -}; > - > - > -/** > - * get_decoder_data() - gets decoder data > - * @input_dev: input device > - * > - * Returns the struct decoder_data that corresponds to a device > - */ > -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > -{ > - struct decoder_data *data = NULL; > - > - spin_lock(&decoder_lock); > - list_for_each_entry(data, &decoder_list, list) { > - if (data->ir_dev == ir_dev) > - break; > - } > - spin_unlock(&decoder_lock); > - return data; > -} > - > /** > * ir_nec_decode() - Decode one NEC pulse or space > * @input_dev: the struct input_dev descriptor of the device > @@ -79,15 +45,11 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > */ > static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) > { > - struct decoder_data *data; > struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > + struct nec_dec *data = &ir_dev->raw->nec; > u32 scancode; > u8 address, not_address, command, not_command; > > - data = get_decoder_data(ir_dev); > - if (!data) > - return -EINVAL; > - > if (!(ir_dev->raw->enabled_protocols & IR_TYPE_NEC)) > return 0; > > @@ -143,9 +105,9 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) > if (ev.pulse) > break; > > - data->nec_bits <<= 1; > + data->bits <<= 1; > if (eq_margin(ev.duration, NEC_BIT_1_SPACE, NEC_UNIT / 2)) > - data->nec_bits |= 1; > + data->bits |= 1; > else if (!eq_margin(ev.duration, NEC_BIT_0_SPACE, NEC_UNIT / 2)) > break; > data->count++; > @@ -174,14 +136,14 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) > if (!geq_margin(ev.duration, NEC_TRAILER_SPACE, NEC_UNIT / 2)) > break; > > - address = bitrev8((data->nec_bits >> 24) & 0xff); > - not_address = bitrev8((data->nec_bits >> 16) & 0xff); > - command = bitrev8((data->nec_bits >> 8) & 0xff); > - not_command = bitrev8((data->nec_bits >> 0) & 0xff); > + address = bitrev8((data->bits >> 24) & 0xff); > + not_address = bitrev8((data->bits >> 16) & 0xff); > + 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->nec_bits); > + data->bits); > break; > } > > @@ -208,44 +170,9 @@ static int ir_nec_decode(struct input_dev *input_dev, struct ir_raw_event ev) > return -EINVAL; > } > > -static int ir_nec_register(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - struct decoder_data *data; > - > - data = kzalloc(sizeof(*data), GFP_KERNEL); > - if (!data) > - return -ENOMEM; > - > - data->ir_dev = ir_dev; > - spin_lock(&decoder_lock); > - list_add_tail(&data->list, &decoder_list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > -static int ir_nec_unregister(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - static struct decoder_data *data; > - > - data = get_decoder_data(ir_dev); > - if (!data) > - return 0; > - > - spin_lock(&decoder_lock); > - list_del(&data->list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > static struct ir_raw_handler nec_handler = { > .protocols = IR_TYPE_NEC, > .decode = ir_nec_decode, > - .raw_register = ir_nec_register, > - .raw_unregister = ir_nec_unregister, > }; > > static int __init ir_nec_decode_init(void) > diff --git a/drivers/media/IR/ir-raw-event.c b/drivers/media/IR/ir-raw-event.c > index eeca8a8..eb77378 100644 > --- a/drivers/media/IR/ir-raw-event.c > +++ b/drivers/media/IR/ir-raw-event.c > @@ -25,32 +25,6 @@ static DEFINE_SPINLOCK(ir_raw_handler_lock); > static LIST_HEAD(ir_raw_handler_list); > static u64 available_protocols; > > -/** > - * RUN_DECODER() - runs an operation on all IR decoders > - * @ops: IR raw handler operation to be called > - * @arg: arguments to be passed to the callback > - * > - * Calls ir_raw_handler::ops for all registered IR handlers. It prevents > - * new decode addition/removal while running, by locking ir_raw_handler_lock > - * mutex. If an error occurs, it stops the ops. Otherwise, it returns a sum > - * of the return codes. > - */ > -#define RUN_DECODER(ops, ...) ({ \ > - struct ir_raw_handler *_ir_raw_handler; \ > - int _sumrc = 0, _rc; \ > - spin_lock(&ir_raw_handler_lock); \ > - list_for_each_entry(_ir_raw_handler, &ir_raw_handler_list, list) { \ > - if (_ir_raw_handler->ops) { \ > - _rc = _ir_raw_handler->ops(__VA_ARGS__); \ > - if (_rc < 0) \ > - break; \ > - _sumrc += _rc; \ > - } \ > - } \ > - spin_unlock(&ir_raw_handler_lock); \ > - _sumrc; \ > -}) > - > #ifdef MODULE > /* Used to load the decoders */ > static struct work_struct wq_load; > @@ -59,11 +33,17 @@ static struct work_struct wq_load; > static void ir_raw_event_work(struct work_struct *work) > { > struct ir_raw_event ev; > + struct ir_raw_handler *handler; > struct ir_raw_event_ctrl *raw = > container_of(work, struct ir_raw_event_ctrl, rx_work); > > - while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) > - RUN_DECODER(decode, raw->input_dev, ev); > + while (kfifo_out(&raw->kfifo, &ev, sizeof(ev)) == sizeof(ev)) { > + spin_lock(&ir_raw_handler_lock); > + list_for_each_entry(handler, &ir_raw_handler_list, list) > + handler->decode(raw->input_dev, ev); > + spin_unlock(&ir_raw_handler_lock); > + raw->prev_ev = ev; > + } > } > > /** > @@ -193,15 +173,7 @@ int ir_raw_event_register(struct input_dev *input_dev) > return rc; > } > > - rc = RUN_DECODER(raw_register, input_dev); > - if (rc < 0) { > - kfifo_free(&ir->raw->kfifo); > - kfree(ir->raw); > - ir->raw = NULL; > - return rc; > - } > - > - return rc; > + return 0; > } > > void ir_raw_event_unregister(struct input_dev *input_dev) > @@ -212,8 +184,6 @@ void ir_raw_event_unregister(struct input_dev *input_dev) > return; > > cancel_work_sync(&ir->raw->rx_work); > - RUN_DECODER(raw_unregister, input_dev); > - > kfifo_free(&ir->raw->kfifo); > kfree(ir->raw); > ir->raw = NULL; > diff --git a/drivers/media/IR/ir-rc5-decoder.c b/drivers/media/IR/ir-rc5-decoder.c > index 1be8981..7af656d 100644 > --- a/drivers/media/IR/ir-rc5-decoder.c > +++ b/drivers/media/IR/ir-rc5-decoder.c > @@ -30,10 +30,6 @@ > #define RC5_BIT_END (1 * RC5_UNIT) > #define RC5X_SPACE (4 * RC5_UNIT) > > -/* Used to register rc5_decoder clients */ > -static LIST_HEAD(decoder_list); > -static DEFINE_SPINLOCK(decoder_lock); > - > enum rc5_state { > STATE_INACTIVE, > STATE_BIT_START, > @@ -42,39 +38,6 @@ enum rc5_state { > STATE_FINISHED, > }; > > -struct decoder_data { > - struct list_head list; > - struct ir_input_dev *ir_dev; > - > - /* State machine control */ > - enum rc5_state state; > - u32 rc5_bits; > - struct ir_raw_event prev_ev; > - unsigned count; > - unsigned wanted_bits; > -}; > - > - > -/** > - * get_decoder_data() - gets decoder data > - * @input_dev: input device > - * > - * Returns the struct decoder_data that corresponds to a device > - */ > - > -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > -{ > - struct decoder_data *data = NULL; > - > - spin_lock(&decoder_lock); > - list_for_each_entry(data, &decoder_list, list) { > - if (data->ir_dev == ir_dev) > - break; > - } > - spin_unlock(&decoder_lock); > - return data; > -} > - > /** > * ir_rc5_decode() - Decode one RC-5 pulse or space > * @input_dev: the struct input_dev descriptor of the device > @@ -84,15 +47,11 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > */ > static int ir_rc5_decode(struct input_dev *input_dev, struct ir_raw_event ev) > { > - struct decoder_data *data; > struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > + struct rc5_dec *data = &ir_dev->raw->rc5; > u8 toggle; > u32 scancode; > > - data = get_decoder_data(ir_dev); > - if (!data) > - return -EINVAL; > - > if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC5)) > return 0; > > @@ -128,16 +87,15 @@ again: > if (!eq_margin(ev.duration, RC5_BIT_START, RC5_UNIT / 2)) > break; > > - data->rc5_bits <<= 1; > + data->bits <<= 1; > if (!ev.pulse) > - data->rc5_bits |= 1; > + data->bits |= 1; > data->count++; > - data->prev_ev = ev; > data->state = STATE_BIT_END; > return 0; > > case STATE_BIT_END: > - if (!is_transition(&ev, &data->prev_ev)) > + if (!is_transition(&ev, &ir_dev->raw->prev_ev)) > break; > > if (data->count == data->wanted_bits) > @@ -169,11 +127,11 @@ again: > if (data->wanted_bits == RC5X_NBITS) { > /* RC5X */ > u8 xdata, command, system; > - xdata = (data->rc5_bits & 0x0003F) >> 0; > - command = (data->rc5_bits & 0x00FC0) >> 6; > - system = (data->rc5_bits & 0x1F000) >> 12; > - toggle = (data->rc5_bits & 0x20000) ? 1 : 0; > - command += (data->rc5_bits & 0x01000) ? 0 : 0x40; > + xdata = (data->bits & 0x0003F) >> 0; > + command = (data->bits & 0x00FC0) >> 6; > + system = (data->bits & 0x1F000) >> 12; > + toggle = (data->bits & 0x20000) ? 1 : 0; > + command += (data->bits & 0x01000) ? 0 : 0x40; > scancode = system << 16 | command << 8 | xdata; > > IR_dprintk(1, "RC5X scancode 0x%06x (toggle: %u)\n", > @@ -182,10 +140,10 @@ again: > } else { > /* RC5 */ > u8 command, system; > - command = (data->rc5_bits & 0x0003F) >> 0; > - system = (data->rc5_bits & 0x007C0) >> 6; > - toggle = (data->rc5_bits & 0x00800) ? 1 : 0; > - command += (data->rc5_bits & 0x01000) ? 0 : 0x40; > + command = (data->bits & 0x0003F) >> 0; > + system = (data->bits & 0x007C0) >> 6; > + toggle = (data->bits & 0x00800) ? 1 : 0; > + command += (data->bits & 0x01000) ? 0 : 0x40; > scancode = system << 8 | command; > > IR_dprintk(1, "RC5 scancode 0x%04x (toggle: %u)\n", > @@ -204,44 +162,9 @@ out: > return -EINVAL; > } > > -static int ir_rc5_register(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - struct decoder_data *data; > - > - data = kzalloc(sizeof(*data), GFP_KERNEL); > - if (!data) > - return -ENOMEM; > - > - data->ir_dev = ir_dev; > - spin_lock(&decoder_lock); > - list_add_tail(&data->list, &decoder_list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > -static int ir_rc5_unregister(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - static struct decoder_data *data; > - > - data = get_decoder_data(ir_dev); > - if (!data) > - return 0; > - > - spin_lock(&decoder_lock); > - list_del(&data->list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > static struct ir_raw_handler rc5_handler = { > .protocols = IR_TYPE_RC5, > .decode = ir_rc5_decode, > - .raw_register = ir_rc5_register, > - .raw_unregister = ir_rc5_unregister, > }; > > static int __init ir_rc5_decode_init(void) > diff --git a/drivers/media/IR/ir-rc6-decoder.c b/drivers/media/IR/ir-rc6-decoder.c > index 5e940a8..a562952 100644 > --- a/drivers/media/IR/ir-rc6-decoder.c > +++ b/drivers/media/IR/ir-rc6-decoder.c > @@ -36,10 +36,6 @@ > #define RC6_STARTBIT_MASK 0x08 /* for the header bits */ > #define RC6_6A_MCE_TOGGLE_MASK 0x8000 /* for the body bits */ > > -/* Used to register rc6_decoder clients */ > -static LIST_HEAD(decoder_list); > -static DEFINE_SPINLOCK(decoder_lock); > - > enum rc6_mode { > RC6_MODE_0, > RC6_MODE_6A, > @@ -58,41 +54,7 @@ enum rc6_state { > STATE_FINISHED, > }; > > -struct decoder_data { > - struct list_head list; > - struct ir_input_dev *ir_dev; > - > - /* State machine control */ > - enum rc6_state state; > - u8 header; > - u32 body; > - struct ir_raw_event prev_ev; > - bool toggle; > - unsigned count; > - unsigned wanted_bits; > -}; > - > - > -/** > - * get_decoder_data() - gets decoder data > - * @input_dev: input device > - * > - * Returns the struct decoder_data that corresponds to a device > - */ > -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > -{ > - struct decoder_data *data = NULL; > - > - spin_lock(&decoder_lock); > - list_for_each_entry(data, &decoder_list, list) { > - if (data->ir_dev == ir_dev) > - break; > - } > - spin_unlock(&decoder_lock); > - return data; > -} > - > -static enum rc6_mode rc6_mode(struct decoder_data *data) { > +static enum rc6_mode rc6_mode(struct rc6_dec *data) { > switch (data->header & RC6_MODE_MASK) { > case 0: > return RC6_MODE_0; > @@ -114,15 +76,11 @@ static enum rc6_mode rc6_mode(struct decoder_data *data) { > */ > static int ir_rc6_decode(struct input_dev *input_dev, struct ir_raw_event ev) > { > - struct decoder_data *data; > struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > + struct rc6_dec *data = &ir_dev->raw->rc6; > u32 scancode; > u8 toggle; > > - data = get_decoder_data(ir_dev); > - if (!data) > - return -EINVAL; > - > if (!(ir_dev->raw->enabled_protocols & IR_TYPE_RC6)) > return 0; > > @@ -175,12 +133,11 @@ again: > if (ev.pulse) > data->header |= 1; > data->count++; > - data->prev_ev = ev; > data->state = STATE_HEADER_BIT_END; > return 0; > > case STATE_HEADER_BIT_END: > - if (!is_transition(&ev, &data->prev_ev)) > + if (!is_transition(&ev, &ir_dev->raw->prev_ev)) > break; > > if (data->count == RC6_HEADER_NBITS) > @@ -196,12 +153,11 @@ again: > break; > > data->toggle = ev.pulse; > - data->prev_ev = ev; > data->state = STATE_TOGGLE_END; > return 0; > > case STATE_TOGGLE_END: > - if (!is_transition(&ev, &data->prev_ev) || > + if (!is_transition(&ev, &ir_dev->raw->prev_ev) || > !geq_margin(ev.duration, RC6_TOGGLE_END, RC6_UNIT / 2)) > break; > > @@ -211,7 +167,6 @@ again: > } > > data->state = STATE_BODY_BIT_START; > - data->prev_ev = ev; > decrease_duration(&ev, RC6_TOGGLE_END); > data->count = 0; > > @@ -243,13 +198,11 @@ again: > if (ev.pulse) > data->body |= 1; > data->count++; > - data->prev_ev = ev; > - > data->state = STATE_BODY_BIT_END; > return 0; > > case STATE_BODY_BIT_END: > - if (!is_transition(&ev, &data->prev_ev)) > + if (!is_transition(&ev, &ir_dev->raw->prev_ev)) > break; > > if (data->count == data->wanted_bits) > @@ -300,44 +253,9 @@ out: > return -EINVAL; > } > > -static int ir_rc6_register(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - struct decoder_data *data; > - > - data = kzalloc(sizeof(*data), GFP_KERNEL); > - if (!data) > - return -ENOMEM; > - > - data->ir_dev = ir_dev; > - spin_lock(&decoder_lock); > - list_add_tail(&data->list, &decoder_list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > -static int ir_rc6_unregister(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - static struct decoder_data *data; > - > - data = get_decoder_data(ir_dev); > - if (!data) > - return 0; > - > - spin_lock(&decoder_lock); > - list_del(&data->list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > static struct ir_raw_handler rc6_handler = { > .protocols = IR_TYPE_RC6, > .decode = ir_rc6_decode, > - .raw_register = ir_rc6_register, > - .raw_unregister = ir_rc6_unregister, > }; > > static int __init ir_rc6_decode_init(void) > diff --git a/drivers/media/IR/ir-sony-decoder.c b/drivers/media/IR/ir-sony-decoder.c > index 8afd16a..b9074f0 100644 > --- a/drivers/media/IR/ir-sony-decoder.c > +++ b/drivers/media/IR/ir-sony-decoder.c > @@ -23,10 +23,6 @@ > #define SONY_BIT_SPACE (1 * SONY_UNIT) > #define SONY_TRAILER_SPACE (10 * SONY_UNIT) /* minimum */ > > -/* Used to register sony_decoder clients */ > -static LIST_HEAD(decoder_list); > -static DEFINE_SPINLOCK(decoder_lock); > - > enum sony_state { > STATE_INACTIVE, > STATE_HEADER_SPACE, > @@ -35,36 +31,6 @@ enum sony_state { > STATE_FINISHED, > }; > > -struct decoder_data { > - struct list_head list; > - struct ir_input_dev *ir_dev; > - > - /* State machine control */ > - enum sony_state state; > - u32 sony_bits; > - unsigned count; > -}; > - > - > -/** > - * get_decoder_data() - gets decoder data > - * @input_dev: input device > - * > - * Returns the struct decoder_data that corresponds to a device > - */ > -static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > -{ > - struct decoder_data *data = NULL; > - > - spin_lock(&decoder_lock); > - list_for_each_entry(data, &decoder_list, list) { > - if (data->ir_dev == ir_dev) > - break; > - } > - spin_unlock(&decoder_lock); > - return data; > -} > - > /** > * ir_sony_decode() - Decode one Sony pulse or space > * @input_dev: the struct input_dev descriptor of the device > @@ -74,15 +40,11 @@ static struct decoder_data *get_decoder_data(struct ir_input_dev *ir_dev) > */ > static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) > { > - struct decoder_data *data; > struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > + struct sony_dec *data = &ir_dev->raw->sony; > u32 scancode; > u8 device, subdevice, function; > > - data = get_decoder_data(ir_dev); > - if (!data) > - return -EINVAL; > - > if (!(ir_dev->raw->enabled_protocols & IR_TYPE_SONY)) > return 0; > > @@ -124,9 +86,9 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) > if (!ev.pulse) > break; > > - data->sony_bits <<= 1; > + data->bits <<= 1; > if (eq_margin(ev.duration, SONY_BIT_1_PULSE, SONY_UNIT / 2)) > - data->sony_bits |= 1; > + data->bits |= 1; > else if (!eq_margin(ev.duration, SONY_BIT_0_PULSE, SONY_UNIT / 2)) > break; > > @@ -160,19 +122,19 @@ static int ir_sony_decode(struct input_dev *input_dev, struct ir_raw_event ev) > > switch (data->count) { > case 12: > - device = bitrev8((data->sony_bits << 3) & 0xF8); > + device = bitrev8((data->bits << 3) & 0xF8); > subdevice = 0; > - function = bitrev8((data->sony_bits >> 4) & 0xFE); > + function = bitrev8((data->bits >> 4) & 0xFE); > break; > case 15: > - device = bitrev8((data->sony_bits >> 0) & 0xFF); > + device = bitrev8((data->bits >> 0) & 0xFF); > subdevice = 0; > - function = bitrev8((data->sony_bits >> 7) & 0xFD); > + function = bitrev8((data->bits >> 7) & 0xFD); > break; > case 20: > - device = bitrev8((data->sony_bits >> 5) & 0xF8); > - subdevice = bitrev8((data->sony_bits >> 0) & 0xFF); > - function = bitrev8((data->sony_bits >> 12) & 0xFE); > + device = bitrev8((data->bits >> 5) & 0xF8); > + subdevice = bitrev8((data->bits >> 0) & 0xFF); > + function = bitrev8((data->bits >> 12) & 0xFE); > break; > default: > IR_dprintk(1, "Sony invalid bitcount %u\n", data->count); > @@ -193,44 +155,9 @@ out: > return -EINVAL; > } > > -static int ir_sony_register(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - struct decoder_data *data; > - > - data = kzalloc(sizeof(*data), GFP_KERNEL); > - if (!data) > - return -ENOMEM; > - > - data->ir_dev = ir_dev; > - spin_lock(&decoder_lock); > - list_add_tail(&data->list, &decoder_list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > -static int ir_sony_unregister(struct input_dev *input_dev) > -{ > - struct ir_input_dev *ir_dev = input_get_drvdata(input_dev); > - static struct decoder_data *data; > - > - data = get_decoder_data(ir_dev); > - if (!data) > - return 0; > - > - spin_lock(&decoder_lock); > - list_del(&data->list); > - spin_unlock(&decoder_lock); > - > - return 0; > -} > - > static struct ir_raw_handler sony_handler = { > .protocols = IR_TYPE_SONY, > .decode = ir_sony_decode, > - .raw_register = ir_sony_register, > - .raw_unregister = ir_sony_unregister, > }; > > static int __init ir_sony_decode_init(void) > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-05-03 20:00 ` Mauro Carvalho Chehab @ 2010-06-07 19:00 ` David Härdeman 2010-06-07 20:15 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: David Härdeman @ 2010-06-07 19:00 UTC (permalink / raw) To: Mauro Carvalho Chehab; +Cc: linux-media, linux-input On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote: > David Härdeman wrote: > > This patch moves the state from each raw decoder into the > > ir_raw_event_ctrl struct. > > > > This allows the removal of code like this: > > > > spin_lock(&decoder_lock); > > list_for_each_entry(data, &decoder_list, list) { > > if (data->ir_dev == ir_dev) > > break; > > } > > spin_unlock(&decoder_lock); > > return data; > > > > which is currently run for each decoder on each event in order > > to get the client-specific decoding state data. > > > > In addition, ir decoding modules and ir driver module load > > order is now independent. Centralizing the data also allows > > for a nice code reduction of about 30% per raw decoder as > > client lists and client registration callbacks are no longer > > necessary. > > The registration callbacks will likely still be needed by lirc, > as you need to create/delete lirc_dev interfaces, when the module > is registered, but I might be wrong. It would be interesting to > add lirc_dev first, in order to be sure about the better interfaces > for it. Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the current interfaces are not good enough since it'll break if lirc_dev is loaded after the hardware modules. > Also, from one side, you reduced the code size, but, on the other hand, > you've increased the memory usage, as now the protocol data will be > stored even for protocols that weren't compiled/loaded. In <4BBF3309.6020909@infradead.org>, Mauro Carvalho Chehab wrote: >> Andy Walls wrote: >>> Encoding pulse vs space with a negative sign, even if now hidden >>> with macros, is still just using a sign instead of a boolean. >>> Memory in modern computers (and now microcontrollers) is cheap and >>> only getting cheaper. Don't give up readability, flexibility, or >>> mainatainability, for the sake of saving memory. > > That was my point since the beginning: the amount of saved memory > doesn't justify the lack of readability. Are you worried about memory usage now? > Probably, the code size savings are big enough to justify the runtime > memory footprint, at least with the current decoders. Not sure how big > this will become when we add lirc_dev and other decoders that might be > missing. Right now, the "reasonable default" is a user machine with one hardware decoder and with all of the rc-core decoders loaded (cause that's how his/her distro will set it up). For that machine, the patch will save a lot of memory, not waste it (~380 lines less code...I'll assure you it's a net gain). In addition, random module load order is currently broken (try loading decoders first and hardware later and you'll see). With this patch, it works again. Anyway, I'll post a new patch series this evening and then we can go back to our regular arguing :) -- David Härdeman ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-07 19:00 ` David Härdeman @ 2010-06-07 20:15 ` Jarod Wilson 2010-06-08 17:50 ` David Härdeman 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-07 20:15 UTC (permalink / raw) To: David Härdeman; +Cc: Mauro Carvalho Chehab, linux-media, linux-input On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote: > On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote: > > David Härdeman wrote: > > > This patch moves the state from each raw decoder into the > > > ir_raw_event_ctrl struct. > > > > > > This allows the removal of code like this: > > > > > > spin_lock(&decoder_lock); > > > list_for_each_entry(data, &decoder_list, list) { > > > if (data->ir_dev == ir_dev) > > > break; > > > } > > > spin_unlock(&decoder_lock); > > > return data; > > > > > > which is currently run for each decoder on each event in order > > > to get the client-specific decoding state data. > > > > > > In addition, ir decoding modules and ir driver module load > > > order is now independent. Centralizing the data also allows > > > for a nice code reduction of about 30% per raw decoder as > > > client lists and client registration callbacks are no longer > > > necessary. > > > > The registration callbacks will likely still be needed by lirc, > > as you need to create/delete lirc_dev interfaces, when the module > > is registered, but I might be wrong. It would be interesting to > > add lirc_dev first, in order to be sure about the better interfaces > > for it. > > Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the > current interfaces are not good enough since it'll break if lirc_dev is > loaded after the hardware modules. This is something I've been meaning to mention myself. On system boot, if an mceusb device is connected, it pretty regularly only has the NEC decoder available to use. I have to reload mceusb, or make sure ir-core is explicitly loaded, wait a bit, then load mceusb, if I want to have all of the protocol handlers available -- which includes the needed-by-default rc6 one. I've only briefly tinkered with trying to fix it, sounds like you may already have fixage within this patchset. ... > In addition, random module load order is currently broken (try loading > decoders first and hardware later and you'll see). With this patch, it > works again. Want. > Anyway, I'll post a new patch series this evening and then we can go > back to our regular arguing :) Hey, at least we're making progress too! :) -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-07 20:15 ` Jarod Wilson @ 2010-06-08 17:50 ` David Härdeman 2010-06-09 3:46 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: David Härdeman @ 2010-06-08 17:50 UTC (permalink / raw) To: Jarod Wilson; +Cc: Mauro Carvalho Chehab, linux-media, linux-input On Mon, Jun 07, 2010 at 04:15:30PM -0400, Jarod Wilson wrote: > On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote: > > On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote: > > > David Härdeman wrote: > > > > This patch moves the state from each raw decoder into the > > > > ir_raw_event_ctrl struct. > > > > > > > > This allows the removal of code like this: > > > > > > > > spin_lock(&decoder_lock); > > > > list_for_each_entry(data, &decoder_list, list) { > > > > if (data->ir_dev == ir_dev) > > > > break; > > > > } > > > > spin_unlock(&decoder_lock); > > > > return data; > > > > > > > > which is currently run for each decoder on each event in order > > > > to get the client-specific decoding state data. > > > > > > > > In addition, ir decoding modules and ir driver module load > > > > order is now independent. Centralizing the data also allows > > > > for a nice code reduction of about 30% per raw decoder as > > > > client lists and client registration callbacks are no longer > > > > necessary. > > > > > > The registration callbacks will likely still be needed by lirc, > > > as you need to create/delete lirc_dev interfaces, when the module > > > is registered, but I might be wrong. It would be interesting to > > > add lirc_dev first, in order to be sure about the better interfaces > > > for it. > > > > Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the > > current interfaces are not good enough since it'll break if lirc_dev is > > loaded after the hardware modules. > > This is something I've been meaning to mention myself. On system boot, if > an mceusb device is connected, it pretty regularly only has the NEC > decoder available to use. I have to reload mceusb, or make sure ir-core is > explicitly loaded, wait a bit, then load mceusb, if I want to have all of > the protocol handlers available -- which includes the needed-by-default > rc6 one. I've only briefly tinkered with trying to fix it, sounds like you > may already have fixage within this patchset. The problem is that without the patchset, each decoder is expected to carry it's own list of datastructures for each hardware receiver. Hardware receiver addition/removal is signalled through a callback to the decoder, but the callback will (naturally) not be invoked if the hardware driver is already loaded when the decoder is. So loading a decoder "late" or reloading a decoder will mean that it doesn't know about pre-existing hardware. > > In addition, random module load order is currently broken (try loading > > decoders first and hardware later and you'll see). With this patch, it > > works again. > > Want. Then please help me with two things: a) Test the patches I just sent (especially 6/8 and 7/8, they should be independent from the rest) b) Mauro mentioned in <4BDF28C0.4060102@redhat.com> that: I liked the idea of your redesign, but I didn't like the removal of a per-decoder sysfs entry. As already discussed, there are cases where we'll need a per-decoder sysfs entry (lirc_dev is probably one of those cases - also Jarod's imon driver is currently implementing a modprobe parameter that needs to be moved to the driver). could you please confirm if your lirc and/or imon drivers would be negatively affected by the proposed patches? -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-08 17:50 ` David Härdeman @ 2010-06-09 3:46 ` Jarod Wilson 2010-06-09 13:29 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-09 3:46 UTC (permalink / raw) To: David Härdeman Cc: Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman <david@hardeman.nu> wrote: > On Mon, Jun 07, 2010 at 04:15:30PM -0400, Jarod Wilson wrote: >> On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote: >> > On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote: >> > > David Härdeman wrote: >> > > > This patch moves the state from each raw decoder into the >> > > > ir_raw_event_ctrl struct. >> > > > >> > > > This allows the removal of code like this: >> > > > >> > > > spin_lock(&decoder_lock); >> > > > list_for_each_entry(data, &decoder_list, list) { >> > > > if (data->ir_dev == ir_dev) >> > > > break; >> > > > } >> > > > spin_unlock(&decoder_lock); >> > > > return data; >> > > > >> > > > which is currently run for each decoder on each event in order >> > > > to get the client-specific decoding state data. >> > > > >> > > > In addition, ir decoding modules and ir driver module load >> > > > order is now independent. Centralizing the data also allows >> > > > for a nice code reduction of about 30% per raw decoder as >> > > > client lists and client registration callbacks are no longer >> > > > necessary. >> > > >> > > The registration callbacks will likely still be needed by lirc, >> > > as you need to create/delete lirc_dev interfaces, when the module >> > > is registered, but I might be wrong. It would be interesting to >> > > add lirc_dev first, in order to be sure about the better interfaces >> > > for it. >> > >> > Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the >> > current interfaces are not good enough since it'll break if lirc_dev is >> > loaded after the hardware modules. >> >> This is something I've been meaning to mention myself. On system boot, if >> an mceusb device is connected, it pretty regularly only has the NEC >> decoder available to use. I have to reload mceusb, or make sure ir-core is >> explicitly loaded, wait a bit, then load mceusb, if I want to have all of >> the protocol handlers available -- which includes the needed-by-default >> rc6 one. I've only briefly tinkered with trying to fix it, sounds like you >> may already have fixage within this patchset. > > The problem is that without the patchset, each decoder is expected to > carry it's own list of datastructures for each hardware receiver. > Hardware receiver addition/removal is signalled through a callback to > the decoder, but the callback will (naturally) not be invoked if the > hardware driver is already loaded when the decoder is. So loading a > decoder "late" or reloading a decoder will mean that it doesn't know > about pre-existing hardware. > >> > In addition, random module load order is currently broken (try loading >> > decoders first and hardware later and you'll see). With this patch, it >> > works again. >> >> Want. > > Then please help me with two things: > > a) Test the patches I just sent (especially 6/8 and 7/8, they should > be independent from the rest) Working on merging them into a tree here locally. There's been a bit of churn, so the last few didn't apply cleanly, but I'm almost there. > b) Mauro mentioned in <4BDF28C0.4060102@redhat.com> that: > > I liked the idea of your redesign, but I didn't like the removal > of a per-decoder sysfs entry. As already discussed, there are > cases where we'll need a per-decoder sysfs entry (lirc_dev is > probably one of those cases - also Jarod's imon driver is > currently implementing a modprobe parameter that needs to be > moved to the driver). > > could you please confirm if your lirc and/or imon drivers would be > negatively affected by the proposed patches? Will do so once I get them wedged in on top. -- Jarod Wilson jarod@wilsonet.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-09 3:46 ` Jarod Wilson @ 2010-06-09 13:29 ` Jarod Wilson 2010-06-09 17:56 ` David Härdeman 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-09 13:29 UTC (permalink / raw) To: David Härdeman; +Cc: Mauro Carvalho Chehab, linux-media, linux-input On Tue, Jun 08, 2010 at 11:46:36PM -0400, Jarod Wilson wrote: > On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman <david@hardeman.nu> wrote: > > On Mon, Jun 07, 2010 at 04:15:30PM -0400, Jarod Wilson wrote: > >> On Mon, Jun 07, 2010 at 09:00:03PM +0200, David Härdeman wrote: > >> > On Mon, May 03, 2010 at 05:00:05PM -0300, Mauro Carvalho Chehab wrote: > >> > > David Härdeman wrote: > >> > > > This patch moves the state from each raw decoder into the > >> > > > ir_raw_event_ctrl struct. > >> > > > > >> > > > This allows the removal of code like this: > >> > > > > >> > > > spin_lock(&decoder_lock); > >> > > > list_for_each_entry(data, &decoder_list, list) { > >> > > > if (data->ir_dev == ir_dev) > >> > > > break; > >> > > > } > >> > > > spin_unlock(&decoder_lock); > >> > > > return data; > >> > > > > >> > > > which is currently run for each decoder on each event in order > >> > > > to get the client-specific decoding state data. > >> > > > > >> > > > In addition, ir decoding modules and ir driver module load > >> > > > order is now independent. Centralizing the data also allows > >> > > > for a nice code reduction of about 30% per raw decoder as > >> > > > client lists and client registration callbacks are no longer > >> > > > necessary. > >> > > > >> > > The registration callbacks will likely still be needed by lirc, > >> > > as you need to create/delete lirc_dev interfaces, when the module > >> > > is registered, but I might be wrong. It would be interesting to > >> > > add lirc_dev first, in order to be sure about the better interfaces > >> > > for it. > >> > > >> > Or the lirc_dev patch can add whatever interfaces it needs. Anyway, the > >> > current interfaces are not good enough since it'll break if lirc_dev is > >> > loaded after the hardware modules. > >> > >> This is something I've been meaning to mention myself. On system boot, if > >> an mceusb device is connected, it pretty regularly only has the NEC > >> decoder available to use. I have to reload mceusb, or make sure ir-core is > >> explicitly loaded, wait a bit, then load mceusb, if I want to have all of > >> the protocol handlers available -- which includes the needed-by-default > >> rc6 one. I've only briefly tinkered with trying to fix it, sounds like you > >> may already have fixage within this patchset. > > > > The problem is that without the patchset, each decoder is expected to > > carry it's own list of datastructures for each hardware receiver. > > Hardware receiver addition/removal is signalled through a callback to > > the decoder, but the callback will (naturally) not be invoked if the > > hardware driver is already loaded when the decoder is. So loading a > > decoder "late" or reloading a decoder will mean that it doesn't know > > about pre-existing hardware. > > > >> > In addition, random module load order is currently broken (try loading > >> > decoders first and hardware later and you'll see). With this patch, it > >> > works again. > >> > >> Want. > > > > Then please help me with two things: > > > > a) Test the patches I just sent (especially 6/8 and 7/8, they should > > be independent from the rest) > > Working on merging them into a tree here locally. There's been a bit > of churn, so the last few didn't apply cleanly, but I'm almost there. > > > b) Mauro mentioned in <4BDF28C0.4060102@redhat.com> that: > > > > I liked the idea of your redesign, but I didn't like the removal > > of a per-decoder sysfs entry. As already discussed, there are > > cases where we'll need a per-decoder sysfs entry (lirc_dev is > > probably one of those cases - also Jarod's imon driver is > > currently implementing a modprobe parameter that needs to be > > moved to the driver). > > > > could you please confirm if your lirc and/or imon drivers would be > > negatively affected by the proposed patches? > > Will do so once I get them wedged in on top. Got it all merged and compiling, but not yet runtime tested. Compiling alone sheds some light on things though... So this definitely negatively impacts my ir-core-to-lirc_dev (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device registration work in its register function. However, if (after your patchset) we add a new pair of callbacks replacing raw_register and raw_unregister, which are optional, that work could be done there instead, so I don't think this is an insurmountable obstacle for the lirc bits. As for the imon driver, the modprobe parameter in question (iirc) was already removed from the driver, as its functionality is replaced by implementing a change_protocol callback. However, there's a request to add it (or something like it) back to the driver to allow disabling the IR part altogether, and there are a few other modparams that might be better suited as sysfs entries. However, its actually not relevant to the case of registering raw protocol handlers, as the imon devices do their decoding in hardware. I can see the possibility for protocol-specific knobs in sysfs though. But I think the same optional callbacks I'd use to keep the lirc bits working could also be used for this. Can't think of a good name for these yet, probably need more coffee first... ;) -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-09 13:29 ` Jarod Wilson @ 2010-06-09 17:56 ` David Härdeman 2010-06-09 18:15 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: David Härdeman @ 2010-06-09 17:56 UTC (permalink / raw) To: Jarod Wilson; +Cc: Mauro Carvalho Chehab, linux-media, linux-input On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: > On Tue, Jun 08, 2010 at 11:46:36PM -0400, Jarod Wilson wrote: > > On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman <david@hardeman.nu> wrote: > > > b) Mauro mentioned in <4BDF28C0.4060102@redhat.com> that: > > > > > > I liked the idea of your redesign, but I didn't like the removal > > > of a per-decoder sysfs entry. As already discussed, there are > > > cases where we'll need a per-decoder sysfs entry (lirc_dev is > > > probably one of those cases - also Jarod's imon driver is > > > currently implementing a modprobe parameter that needs to be > > > moved to the driver). > > > > > > could you please confirm if your lirc and/or imon drivers would be > > > negatively affected by the proposed patches? > > > > Will do so once I get them wedged in on top. > > Got it all merged and compiling, but not yet runtime tested. Compiling > alone sheds some light on things though... > > So this definitely negatively impacts my ir-core-to-lirc_dev > (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device > registration work in its register function. However, if (after your > patchset) we add a new pair of callbacks replacing raw_register and > raw_unregister, which are optional, that work could be done there instead, > so I don't think this is an insurmountable obstacle for the lirc bits. While I'm not sure exactly what callbacks you're suggesting, it still sounds like the callbacks would have the exact same problems that the current code has (i.e. the decoder will be blissfully unaware of hardware which exists before the decoder is loaded). Right? > As for the imon driver, the modprobe parameter in question (iirc) was > already removed from the driver, as its functionality is replaced by > implementing a change_protocol callback. However, there's a request to > add it (or something like it) back to the driver to allow disabling the > IR part altogether, and there are a few other modparams that might be > better suited as sysfs entries. However, its actually not relevant to the > case of registering raw protocol handlers, as the imon devices do their > decoding in hardware. I can see the possibility for protocol-specific > knobs in sysfs though. But I think the same optional callbacks I'd use to > keep the lirc bits working could also be used for this. Can't think of a > good name for these yet, probably need more coffee first... ;) But those sysfs entries wouldn't be per-decoder-per-hardware-device....they'd just be per-hardware-device...right? -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-09 17:56 ` David Härdeman @ 2010-06-09 18:15 ` Jarod Wilson 2010-06-10 1:25 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-09 18:15 UTC (permalink / raw) To: David Härdeman; +Cc: Mauro Carvalho Chehab, linux-media, linux-input On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: > On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: > > On Tue, Jun 08, 2010 at 11:46:36PM -0400, Jarod Wilson wrote: > > > On Tue, Jun 8, 2010 at 1:50 PM, David Härdeman <david@hardeman.nu> wrote: > > > > b) Mauro mentioned in <4BDF28C0.4060102@redhat.com> that: > > > > > > > > I liked the idea of your redesign, but I didn't like the removal > > > > of a per-decoder sysfs entry. As already discussed, there are > > > > cases where we'll need a per-decoder sysfs entry (lirc_dev is > > > > probably one of those cases - also Jarod's imon driver is > > > > currently implementing a modprobe parameter that needs to be > > > > moved to the driver). > > > > > > > > could you please confirm if your lirc and/or imon drivers would be > > > > negatively affected by the proposed patches? > > > > > > Will do so once I get them wedged in on top. > > > > Got it all merged and compiling, but not yet runtime tested. Compiling > > alone sheds some light on things though... > > > > So this definitely negatively impacts my ir-core-to-lirc_dev > > (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device > > registration work in its register function. However, if (after your > > patchset) we add a new pair of callbacks replacing raw_register and > > raw_unregister, which are optional, that work could be done there instead, > > so I don't think this is an insurmountable obstacle for the lirc bits. > > While I'm not sure exactly what callbacks you're suggesting, Essentially: .setup_other_crap .tear_down_other_crap ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific hardware receiver as an lirc_dev client, and conversely, tear it down. > it still > sounds like the callbacks would have the exact same problems that the > current code has (i.e. the decoder will be blissfully unaware of > hardware which exists before the decoder is loaded). Right? In my head, this was going to work out, but you're correct, I still have the exact same problem -- its not in ir_raw_handler_list yet when ir_raw_event_register runs, and thus the callback never fires, so lirc_dev never actually gets wired up to ir-lirc-codec. It now knows about the lirc decoder, but its completely useless. Narf. > > As for the imon driver, the modprobe parameter in question (iirc) was > > already removed from the driver, as its functionality is replaced by > > implementing a change_protocol callback. However, there's a request to > > add it (or something like it) back to the driver to allow disabling the > > IR part altogether, and there are a few other modparams that might be > > better suited as sysfs entries. However, its actually not relevant to the > > case of registering raw protocol handlers, as the imon devices do their > > decoding in hardware. I can see the possibility for protocol-specific > > knobs in sysfs though. But I think the same optional callbacks I'd use to > > keep the lirc bits working could also be used for this. Can't think of a > > good name for these yet, probably need more coffee first... ;) > > But those sysfs entries wouldn't be > per-decoder-per-hardware-device....they'd just be > per-hardware-device...right? Most likely. But I think its possible someone would want to want to tweak some parameter that is both protocol and hardware device specific. Just sheer speculation at the moment though, I don't have a concrete example. -- Jarod Wilson jarod@redhat.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-09 18:15 ` Jarod Wilson @ 2010-06-10 1:25 ` Jarod Wilson 2010-06-13 20:29 ` David Härdeman 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-10 1:25 UTC (permalink / raw) To: David Härdeman Cc: Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Wed, Jun 9, 2010 at 2:15 PM, Jarod Wilson <jarod@redhat.com> wrote: > On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: >> On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: ... >> > So this definitely negatively impacts my ir-core-to-lirc_dev >> > (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device >> > registration work in its register function. However, if (after your >> > patchset) we add a new pair of callbacks replacing raw_register and >> > raw_unregister, which are optional, that work could be done there instead, >> > so I don't think this is an insurmountable obstacle for the lirc bits. >> >> While I'm not sure exactly what callbacks you're suggesting, > > Essentially: > > .setup_other_crap > .tear_down_other_crap > > ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific > hardware receiver as an lirc_dev client, and conversely, tear it down. > >> it still >> sounds like the callbacks would have the exact same problems that the >> current code has (i.e. the decoder will be blissfully unaware of >> hardware which exists before the decoder is loaded). Right? > > In my head, this was going to work out, but you're correct, I still have > the exact same problem -- its not in ir_raw_handler_list yet when > ir_raw_event_register runs, and thus the callback never fires, so lirc_dev > never actually gets wired up to ir-lirc-codec. It now knows about the lirc > decoder, but its completely useless. Narf. And now I have it working atop your patches. Its a bit of a nasty-ish hack, at least for the lirc case, but its working, even in the case where the decoder drivers aren't actually loaded until after the device driver. I've added one extra param to each protocol-specific struct in ir-core-priv.h (bool initialized) and hooked into the protocol-specific decode functions to both determine whether a protocol should be enabled or disabled by default, and to run any additionally required initialization (such as in the ir-lirc-codec case). So initially, mceusb comes up with all decoders enabled. Then when ir comes in, every protocol-specific decoder fires. Each of them check for whether or not they've been fully initialized, and if not, we check the loaded keymap, and if it doesn't match, we disable that decoder (bringing back the "disable protocol handlers we don't need" functionality that disappeared w/this patchset). In the lirc case, we actually do all the work needed to wire up the connection over to lirc_dev. This works perfectly fine for all the in-kernel decoders, but has one minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually exist until the first incoming ir signal is seen. lircd can handle this case just fine, it'll wait for /dev/lirc0 to show up, but it doesn't come up fast enough to catch and decode the very first incoming ir signal. Subsequent ones work perfectly fine though. This need to initialize the link via incoming ir is a bit problematic if you're using a device for transmit-only (e.g., and mceusb device hooked to a mythtv backend in the closet or something), as there would be a strong possibility of /dev/lirc0 never getting hooked up. I can think of a few workarounds, but none are particularly clean and/or automagic. Not sure how palatable it is, but here it is: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=commitdiff;h=8f2be576ecb82448daa0c0d789bf0c978c66b103 -- Jarod Wilson jarod@wilsonet.com -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-10 1:25 ` Jarod Wilson @ 2010-06-13 20:29 ` David Härdeman 2010-06-16 20:04 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: David Härdeman @ 2010-06-13 20:29 UTC (permalink / raw) To: Jarod Wilson Cc: Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Wed, Jun 09, 2010 at 09:25:44PM -0400, Jarod Wilson wrote: > On Wed, Jun 9, 2010 at 2:15 PM, Jarod Wilson <jarod@redhat.com> wrote: > > On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: > >> On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: > ... > >> > So this definitely negatively impacts my ir-core-to-lirc_dev > >> > (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device > >> > registration work in its register function. However, if (after your > >> > patchset) we add a new pair of callbacks replacing raw_register and > >> > raw_unregister, which are optional, that work could be done there instead, > >> > so I don't think this is an insurmountable obstacle for the lirc bits. > >> > >> While I'm not sure exactly what callbacks you're suggesting, > > > > Essentially: > > > > .setup_other_crap > > .tear_down_other_crap > > > > ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific > > hardware receiver as an lirc_dev client, and conversely, tear it down. > > > >> it still > >> sounds like the callbacks would have the exact same problems that the > >> current code has (i.e. the decoder will be blissfully unaware of > >> hardware which exists before the decoder is loaded). Right? > > > > In my head, this was going to work out, but you're correct, I still have > > the exact same problem -- its not in ir_raw_handler_list yet when > > ir_raw_event_register runs, and thus the callback never fires, so lirc_dev > > never actually gets wired up to ir-lirc-codec. It now knows about the lirc > > decoder, but its completely useless. Narf. > > And now I have it working atop your patches. Its a bit of a nasty-ish > hack, at least for the lirc case, but its working, even in the case > where the decoder drivers aren't actually loaded until after the > device driver. I've added one extra param to each protocol-specific > struct in ir-core-priv.h (bool initialized) and hooked into the > protocol-specific decode functions to both determine whether a > protocol should be enabled or disabled by default, and to run any > additionally required initialization (such as in the ir-lirc-codec > case). > > So initially, mceusb comes up with all decoders enabled. Then when ir > comes in, every protocol-specific decoder fires. Each of them check > for whether or not they've been fully initialized, and if not, we > check the loaded keymap, and if it doesn't match, we disable that > decoder (bringing back the "disable protocol handlers we don't need" > functionality that disappeared w/this patchset). In the lirc case, we > actually do all the work needed to wire up the connection over to > lirc_dev. > > This works perfectly fine for all the in-kernel decoders, but has one > minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually > exist until the first incoming ir signal is seen. lircd can handle > this case just fine, it'll wait for /dev/lirc0 to show up, but it > doesn't come up fast enough to catch and decode the very first > incoming ir signal. Subsequent ones work perfectly fine though. This > need to initialize the link via incoming ir is a bit problematic if > you're using a device for transmit-only (e.g., and mceusb device > hooked to a mythtv backend in the closet or something), as there would > be a strong possibility of /dev/lirc0 never getting hooked up. I can > think of a few workarounds, but none are particularly clean and/or > automagic. > > Not sure how palatable it is, but here it is: I think it sounds pretty awful :) I have another suggestion, let's keep the client register/unregister callbacks for decoders (but add a comment that they're only used for lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the raw clients so that it can pass all pre-existing clients to newly added decoders. I'll post two patches (compile tested only) in a few seconds to show what I mean. -- David Härdeman -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-13 20:29 ` David Härdeman @ 2010-06-16 20:04 ` Jarod Wilson 2010-06-16 20:41 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-16 20:04 UTC (permalink / raw) To: David Härdeman Cc: Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Sun, Jun 13, 2010 at 4:29 PM, David Härdeman <david@hardeman.nu> wrote: > On Wed, Jun 09, 2010 at 09:25:44PM -0400, Jarod Wilson wrote: >> On Wed, Jun 9, 2010 at 2:15 PM, Jarod Wilson <jarod@redhat.com> wrote: >> > On Wed, Jun 09, 2010 at 07:56:21PM +0200, David Härdeman wrote: >> >> On Wed, Jun 09, 2010 at 09:29:08AM -0400, Jarod Wilson wrote: >> ... >> >> > So this definitely negatively impacts my ir-core-to-lirc_dev >> >> > (ir-lirc-codec.c) bridge driver, as it was doing the lirc_dev device >> >> > registration work in its register function. However, if (after your >> >> > patchset) we add a new pair of callbacks replacing raw_register and >> >> > raw_unregister, which are optional, that work could be done there instead, >> >> > so I don't think this is an insurmountable obstacle for the lirc bits. >> >> >> >> While I'm not sure exactly what callbacks you're suggesting, >> > >> > Essentially: >> > >> > .setup_other_crap >> > .tear_down_other_crap >> > >> > ...which in the ir-lirc-codec case, register ir-lirc-codec for a specific >> > hardware receiver as an lirc_dev client, and conversely, tear it down. >> > >> >> it still >> >> sounds like the callbacks would have the exact same problems that the >> >> current code has (i.e. the decoder will be blissfully unaware of >> >> hardware which exists before the decoder is loaded). Right? >> > >> > In my head, this was going to work out, but you're correct, I still have >> > the exact same problem -- its not in ir_raw_handler_list yet when >> > ir_raw_event_register runs, and thus the callback never fires, so lirc_dev >> > never actually gets wired up to ir-lirc-codec. It now knows about the lirc >> > decoder, but its completely useless. Narf. >> >> And now I have it working atop your patches. Its a bit of a nasty-ish >> hack, at least for the lirc case, but its working, even in the case >> where the decoder drivers aren't actually loaded until after the >> device driver. I've added one extra param to each protocol-specific >> struct in ir-core-priv.h (bool initialized) and hooked into the >> protocol-specific decode functions to both determine whether a >> protocol should be enabled or disabled by default, and to run any >> additionally required initialization (such as in the ir-lirc-codec >> case). >> >> So initially, mceusb comes up with all decoders enabled. Then when ir >> comes in, every protocol-specific decoder fires. Each of them check >> for whether or not they've been fully initialized, and if not, we >> check the loaded keymap, and if it doesn't match, we disable that >> decoder (bringing back the "disable protocol handlers we don't need" >> functionality that disappeared w/this patchset). In the lirc case, we >> actually do all the work needed to wire up the connection over to >> lirc_dev. >> >> This works perfectly fine for all the in-kernel decoders, but has one >> minor shortcoming for ir-lirc-codec, in that /dev/lirc0 won't actually >> exist until the first incoming ir signal is seen. lircd can handle >> this case just fine, it'll wait for /dev/lirc0 to show up, but it >> doesn't come up fast enough to catch and decode the very first >> incoming ir signal. Subsequent ones work perfectly fine though. This >> need to initialize the link via incoming ir is a bit problematic if >> you're using a device for transmit-only (e.g., and mceusb device >> hooked to a mythtv backend in the closet or something), as there would >> be a strong possibility of /dev/lirc0 never getting hooked up. I can >> think of a few workarounds, but none are particularly clean and/or >> automagic. >> >> Not sure how palatable it is, but here it is: > > I think it sounds pretty awful :) So my suspicion wrt palatability was correct. :) > I have another suggestion, let's keep the client register/unregister > callbacks for decoders (but add a comment that they're only used for > lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the > raw clients so that it can pass all pre-existing clients to newly added > decoders. > > I'll post two patches (compile tested only) in a few seconds to show > what I mean. Consider them now runtime tested as well. They appear to do the trick, the lirc bridge comes up just fine, even when ir-lirc-codec isn't loaded until after mceusb. *Much* better implementation than my ugly trick. I'll ack your patches and submit a series on top of them for lirc support, hopefully this evening (in addition to a few other fixes that aren't dependent on any of them). -- Jarod Wilson jarod@wilsonet.com -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-16 20:04 ` Jarod Wilson @ 2010-06-16 20:41 ` Jarod Wilson 2010-06-17 12:14 ` Andy Walls 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-16 20:41 UTC (permalink / raw) To: David Härdeman Cc: Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson <jarod@wilsonet.com> wrote: ... >> I have another suggestion, let's keep the client register/unregister >> callbacks for decoders (but add a comment that they're only used for >> lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the >> raw clients so that it can pass all pre-existing clients to newly added >> decoders. >> >> I'll post two patches (compile tested only) in a few seconds to show >> what I mean. > > Consider them now runtime tested as well. They appear to do the trick, > the lirc bridge comes up just fine, even when ir-lirc-codec isn't > loaded until after mceusb. *Much* better implementation than my ugly > trick. I'll ack your patches and submit a series on top of them for > lirc support, hopefully this evening (in addition to a few other fixes > that aren't dependent on any of them). A fully functional tree carrying both of David's patches and the entire stack of other patches I've submitted today, based on top of the linuxtv staging/rc branch, can be found here: http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches Also includes the lirc patches that I believe are ready to be submitted for actual consideration (note that they're dependent on David's two patches). -- Jarod Wilson jarod@wilsonet.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-16 20:41 ` Jarod Wilson @ 2010-06-17 12:14 ` Andy Walls 2010-06-17 15:11 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: Andy Walls @ 2010-06-17 12:14 UTC (permalink / raw) To: Jarod Wilson Cc: David Härdeman, Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Wed, 2010-06-16 at 16:41 -0400, Jarod Wilson wrote: > On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson <jarod@wilsonet.com> wrote: > ... > >> I have another suggestion, let's keep the client register/unregister > >> callbacks for decoders (but add a comment that they're only used for > >> lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the > >> raw clients so that it can pass all pre-existing clients to newly added > >> decoders. > >> > >> I'll post two patches (compile tested only) in a few seconds to show > >> what I mean. > > > > Consider them now runtime tested as well. They appear to do the trick, > > the lirc bridge comes up just fine, even when ir-lirc-codec isn't > > loaded until after mceusb. *Much* better implementation than my ugly > > trick. I'll ack your patches and submit a series on top of them for > > lirc support, hopefully this evening (in addition to a few other fixes > > that aren't dependent on any of them). > > A fully functional tree carrying both of David's patches and the > entire stack of other patches I've submitted today, based on top of > the linuxtv staging/rc branch, can be found here: > > http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches > > Also includes the lirc patches that I believe are ready to be > submitted for actual consideration (note that they're dependent on > David's two patches). I'll try and play with this this weekend along with some cx23885 cleanup. Regards, Andy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-17 12:14 ` Andy Walls @ 2010-06-17 15:11 ` Jarod Wilson 2010-06-21 0:47 ` Andy Walls 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-17 15:11 UTC (permalink / raw) To: Andy Walls Cc: David Härdeman, Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Thu, Jun 17, 2010 at 8:14 AM, Andy Walls <awalls@md.metrocast.net> wrote: > On Wed, 2010-06-16 at 16:41 -0400, Jarod Wilson wrote: >> On Wed, Jun 16, 2010 at 4:04 PM, Jarod Wilson <jarod@wilsonet.com> wrote: >> ... >> >> I have another suggestion, let's keep the client register/unregister >> >> callbacks for decoders (but add a comment that they're only used for >> >> lirc). Then teach drivers/media/IR/ir-raw-event.c to keep track of the >> >> raw clients so that it can pass all pre-existing clients to newly added >> >> decoders. >> >> >> >> I'll post two patches (compile tested only) in a few seconds to show >> >> what I mean. >> > >> > Consider them now runtime tested as well. They appear to do the trick, >> > the lirc bridge comes up just fine, even when ir-lirc-codec isn't >> > loaded until after mceusb. *Much* better implementation than my ugly >> > trick. I'll ack your patches and submit a series on top of them for >> > lirc support, hopefully this evening (in addition to a few other fixes >> > that aren't dependent on any of them). >> >> A fully functional tree carrying both of David's patches and the >> entire stack of other patches I've submitted today, based on top of >> the linuxtv staging/rc branch, can be found here: >> >> http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches >> >> Also includes the lirc patches that I believe are ready to be >> submitted for actual consideration (note that they're dependent on >> David's two patches). > > > I'll try and play with this this weekend along with some cx23885 > cleanup. Excellent. A few things to note... Many of the lirc_dev ioctls are currently commented out, and haven't in any way been wired up to tx callbacks, I've only enabled the minimum necessary for mceusb. The ioctls are all using __u32 params, which, if you're on x86_64, will require a patched lirc userspace build to make the ioctl types match. I'm using this patch atm: http://wilsonet.com/jarod/lirc_misc/lirc-0.8.6-make-ioctls-u32.patch (In the future, lirc userspace should obviously just build against <media/lirc.h>). -- Jarod Wilson jarod@wilsonet.com ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-17 15:11 ` Jarod Wilson @ 2010-06-21 0:47 ` Andy Walls 2010-06-21 3:51 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: Andy Walls @ 2010-06-21 0:47 UTC (permalink / raw) To: Jarod Wilson Cc: David Härdeman, Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Thu, 2010-06-17 at 11:11 -0400, Jarod Wilson wrote: > On Thu, Jun 17, 2010 at 8:14 AM, Andy Walls <awalls@md.metrocast.net> wrote: > >> A fully functional tree carrying both of David's patches and the > >> entire stack of other patches I've submitted today, based on top of > >> the linuxtv staging/rc branch, can be found here: > >> > >> http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches > >> > >> Also includes the lirc patches that I believe are ready to be > >> submitted for actual consideration (note that they're dependent on > >> David's two patches). > > > > > > I'll try and play with this this weekend along with some cx23885 > > cleanup. > > Excellent. A few things to note... Jarrod, I was unable to get this task completed in the time I had available this weekend. A power supply failure, unexpected hard drive replacement, and my inability to build/install a kernel from a git tree that would actually boot my Fedora 12 installation didn't help. (My productivity has tanked since v4l-dvb went to GIT for CM, and the last time I built a real kernel without rpmbuild was for RedHat 9. I'm still working out processes for doing basic things, sorry.) I'll have time on Thursday night to try again. > Many of the lirc_dev ioctls are > currently commented out, and haven't in any way been wired up to tx > callbacks, Yes, I saw, that's OK. It should be easy enough to hack something in for testing and prototyping. > I've only enabled the minimum necessary for mceusb. The > ioctls are all using __u32 params, which, if you're on x86_64, will > require a patched lirc userspace build to make the ioctl types match. > I'm using this patch atm: > > http://wilsonet.com/jarod/lirc_misc/lirc-0.8.6-make-ioctls-u32.patch > > (In the future, lirc userspace should obviously just build against > <media/lirc.h>). I've got all x86_64 bit machines here, so thank you for the tips. Regards, Andy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-21 0:47 ` Andy Walls @ 2010-06-21 3:51 ` Jarod Wilson 2010-06-21 11:04 ` Andy Walls 0 siblings, 1 reply; 25+ messages in thread From: Jarod Wilson @ 2010-06-21 3:51 UTC (permalink / raw) To: Andy Walls Cc: David Härdeman, Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls <awalls@md.metrocast.net> wrote: > On Thu, 2010-06-17 at 11:11 -0400, Jarod Wilson wrote: >> On Thu, Jun 17, 2010 at 8:14 AM, Andy Walls <awalls@md.metrocast.net> wrote: > >> >> A fully functional tree carrying both of David's patches and the >> >> entire stack of other patches I've submitted today, based on top of >> >> the linuxtv staging/rc branch, can be found here: >> >> >> >> http://git.wilsonet.com/linux-2.6-ir-wip.git/?a=shortlog;h=refs/heads/patches >> >> >> >> Also includes the lirc patches that I believe are ready to be >> >> submitted for actual consideration (note that they're dependent on >> >> David's two patches). >> > >> > >> > I'll try and play with this this weekend along with some cx23885 >> > cleanup. >> >> Excellent. A few things to note... > > Jarrod, > > I was unable to get this task completed in the time I had available this > weekend. A power supply failure, unexpected hard drive replacement, and > my inability to build/install a kernel from a git tree that would > actually boot my Fedora 12 installation didn't help. (My productivity > has tanked since v4l-dvb went to GIT for CM, and the last time I built a > real kernel without rpmbuild was for RedHat 9. I'm still working out > processes for doing basic things, sorry.) Heh, yeah, hardware failure is always fun when you're trying really hard to get something done. :) As for the building your own kernel thing... I've been doing my work mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora 13, and the other an HP xw4400 workstation running RHEL6. In both cases, I copied a distro kernel's, config file out of /boot/, and then ran make oldconfig over it and build straight from what's in my tree, which works well enough on both setups. > I'll have time on Thursday night to try again. No rush yet, we've got a while before the merge window still. Christoph (Bartelmus) helped me out with a bunch of ioctl documentation this weekend, so I've got that to add to the tree, then I think I'll be prepared to resubmit the lirc bits. I'll shoot for doing that next weekend, and hopefully, that'll give you a chance to try 'em out before then and provide any necessary feedback/fixes/etc. (Not that we can't also just fix things up as needed post-merge). I'm still up in the air as to what I should work on next... So many lirc drivers left to port still... Maybe zilog, maybe streamzap... Maybe the MCE IR keyboard... -- Jarod Wilson jarod@wilsonet.com -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-21 3:51 ` Jarod Wilson @ 2010-06-21 11:04 ` Andy Walls 2010-07-06 17:12 ` Jarod Wilson 0 siblings, 1 reply; 25+ messages in thread From: Andy Walls @ 2010-06-21 11:04 UTC (permalink / raw) To: Jarod Wilson Cc: David Härdeman, Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Sun, 2010-06-20 at 23:51 -0400, Jarod Wilson wrote: > On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls <awalls@md.metrocast.net> wrote: > As for the building your own kernel thing... I've been doing my work > mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora > 13, and the other an HP xw4400 workstation running RHEL6. In both > cases, I copied a distro kernel's, config file out of /boot/, and then > ran make oldconfig over it and build straight from what's in my tree, > which works well enough on both setups. I pulled the config out of the kernel*.src.rpm after 'rpmbuild -bp' (I only saw the configs in /boot after doing that :P ), and then ran 'make oldconfig'. The first time around I forgot to do a modules_install and the ext[234] modules weren't in the initramfs. That made it hard for the kernel to read the /boot and / filesystems. ;) After fixing that idiocy, it now hangs in early boot - just a blinking cursor. I'm speculating it is a problem with support for my old-ish ATI Radeon Xpress 200 video chipset with a vanilla kernel. I'll work it out eventually. > > I'll have time on Thursday night to try again. > > No rush yet, we've got a while before the merge window still. > Christoph (Bartelmus) helped me out with a bunch of ioctl > documentation this weekend, so I've got that to add to the tree, then > I think I'll be prepared to resubmit the lirc bits. I'll shoot for > doing that next weekend, and hopefully, that'll give you a chance to > try 'em out before then and provide any necessary feedback/fixes/etc. > (Not that we can't also just fix things up as needed post-merge). I'm > still up in the air as to what I should work on next... So many lirc > drivers left to port still... Maybe zilog, maybe streamzap... Maybe > the MCE IR keyboard... I've got a PVR-150 and HVR-1600 both with the Zilog Z8's on them. If I ever get my act together, I'll at least be able to test that and integrate any changes into the ivtv & cx18 drivers. I've recently seen users having trouble on IRC, BTW. Regards, Andy ^ permalink raw reply [flat|nested] 25+ messages in thread
* Re: [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl 2010-06-21 11:04 ` Andy Walls @ 2010-07-06 17:12 ` Jarod Wilson 0 siblings, 0 replies; 25+ messages in thread From: Jarod Wilson @ 2010-07-06 17:12 UTC (permalink / raw) To: Andy Walls Cc: David Härdeman, Jarod Wilson, Mauro Carvalho Chehab, linux-media, linux-input On Mon, Jun 21, 2010 at 7:04 AM, Andy Walls <awalls@md.metrocast.net> wrote: > On Sun, 2010-06-20 at 23:51 -0400, Jarod Wilson wrote: >> On Sun, Jun 20, 2010 at 8:47 PM, Andy Walls <awalls@md.metrocast.net> wrote: > > >> As for the building your own kernel thing... I've been doing my work >> mainly on a pair of x86_64 systems, one a ThinkPad T61 running Fedora >> 13, and the other an HP xw4400 workstation running RHEL6. In both >> cases, I copied a distro kernel's, config file out of /boot/, and then >> ran make oldconfig over it and build straight from what's in my tree, >> which works well enough on both setups. > > I pulled the config out of the kernel*.src.rpm after 'rpmbuild -bp' (I > only saw the configs in /boot after doing that :P ), and then ran 'make > oldconfig'. > > The first time around I forgot to do a modules_install and the ext[234] > modules weren't in the initramfs. That made it hard for the kernel to > read the /boot and / filesystems. ;) > > After fixing that idiocy, it now hangs in early boot - just a blinking > cursor. I'm speculating it is a problem with support for my old-ish ATI > Radeon Xpress 200 video chipset with a vanilla kernel. I'll work it out > eventually. Seems you've already gotten around this, but it did remind me of someone on one of the fedora mailing lists who said w/their older radeon hardware, they could only boot recent kernels if the passed in nomodeset=1 or something like that. >> > I'll have time on Thursday night to try again. >> >> No rush yet, we've got a while before the merge window still. >> Christoph (Bartelmus) helped me out with a bunch of ioctl >> documentation this weekend, so I've got that to add to the tree, then >> I think I'll be prepared to resubmit the lirc bits. I'll shoot for >> doing that next weekend, and hopefully, that'll give you a chance to >> try 'em out before then and provide any necessary feedback/fixes/etc. >> (Not that we can't also just fix things up as needed post-merge). I'm >> still up in the air as to what I should work on next... So many lirc >> drivers left to port still... Maybe zilog, maybe streamzap... Maybe >> the MCE IR keyboard... > > I've got a PVR-150 and HVR-1600 both with the Zilog Z8's on them. If I > ever get my act together, I'll at least be able to test that and > integrate any changes into the ivtv & cx18 drivers. I've recently seen > users having trouble on IRC, BTW. Yeah, me too. Hoping to dig into porting (and fixing) lirc_zilog RSN... -- Jarod Wilson jarod@wilsonet.com -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply [flat|nested] 25+ messages in thread
* [PATCH 4/4] ir-core: remove ir-functions usage from cx231xx 2010-04-24 21:13 [PATCH 0/4] ir-core sysfs protocol selection simplification David Härdeman ` (2 preceding siblings ...) 2010-04-24 21:14 ` [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl David Härdeman @ 2010-04-24 21:14 ` David Härdeman 3 siblings, 0 replies; 25+ messages in thread From: David Härdeman @ 2010-04-24 21:14 UTC (permalink / raw) To: mchehab; +Cc: linux-media, linux-input Convert drivers/media/video/cx231xx/cx231xx-input.c to not rely on ir-functions.c. (I do not have the hardware so I can only compile test this) Signed-off-by: David Härdeman <david@hardeman.nu> --- drivers/media/video/cx231xx/cx231xx-input.c | 47 +++++---------------------- drivers/media/video/cx231xx/cx231xx.h | 2 + 2 files changed, 10 insertions(+), 39 deletions(-) diff --git a/drivers/media/video/cx231xx/cx231xx-input.c b/drivers/media/video/cx231xx/cx231xx-input.c index dbd6218..a3d6593 100644 --- a/drivers/media/video/cx231xx/cx231xx-input.c +++ b/drivers/media/video/cx231xx/cx231xx-input.c @@ -60,7 +60,6 @@ struct cx231xx_ir_poll_result { struct cx231xx_IR { struct cx231xx *dev; struct input_dev *input; - struct ir_input_state ir; char name[32]; char phys[32]; @@ -68,9 +67,7 @@ struct cx231xx_IR { int polling; struct work_struct work; struct timer_list timer; - unsigned int last_toggle:1; unsigned int last_readcount; - unsigned int repeat_interval; int (*get_key) (struct cx231xx_IR *, struct cx231xx_ir_poll_result *); }; @@ -82,7 +79,6 @@ struct cx231xx_IR { static void cx231xx_ir_handle_key(struct cx231xx_IR *ir) { int result; - int do_sendkey = 0; struct cx231xx_ir_poll_result poll_result; /* read the registers containing the IR status */ @@ -96,44 +92,23 @@ static void cx231xx_ir_handle_key(struct cx231xx_IR *ir) poll_result.toggle_bit, poll_result.read_count, ir->last_readcount, poll_result.rc_data[0]); - if (ir->dev->chip_id == CHIP_ID_EM2874) { + if (poll_result.read_count > 0 && + poll_result.read_count != ir->last_readcount) + ir_keydown(ir->input, + poll_result.rc_data[0], + poll_result.toggle_bit); + + if (ir->dev->chip_id == CHIP_ID_EM2874) /* The em2874 clears the readcount field every time the register is read. The em2860/2880 datasheet says that it is supposed to clear the readcount, but it doesn't. So with the em2874, we are looking for a non-zero read count as opposed to a readcount that is incrementing */ ir->last_readcount = 0; - } - - if (poll_result.read_count == 0) { - /* The button has not been pressed since the last read */ - } else if (ir->last_toggle != poll_result.toggle_bit) { - /* A button has been pressed */ - dprintk("button has been pressed\n"); - ir->last_toggle = poll_result.toggle_bit; - ir->repeat_interval = 0; - do_sendkey = 1; - } else if (poll_result.toggle_bit == ir->last_toggle && - poll_result.read_count > 0 && - poll_result.read_count != ir->last_readcount) { - /* The button is still being held down */ - dprintk("button being held down\n"); - - /* Debouncer for first keypress */ - if (ir->repeat_interval++ > 9) { - /* Start repeating after 1 second */ - do_sendkey = 1; - } - } + else + ir->last_readcount = poll_result.read_count; - if (do_sendkey) { - dprintk("sending keypress\n"); - ir_input_keydown(ir->input, &ir->ir, poll_result.rc_data[0]); - ir_input_nokey(ir->input, &ir->ir); } - - ir->last_readcount = poll_result.read_count; - return; } static void ir_timer(unsigned long data) @@ -199,10 +174,6 @@ int cx231xx_ir_init(struct cx231xx *dev) usb_make_path(dev->udev, ir->phys, sizeof(ir->phys)); strlcat(ir->phys, "/input0", sizeof(ir->phys)); - err = ir_input_init(input_dev, &ir->ir, IR_TYPE_OTHER); - if (err < 0) - goto err_out_free; - input_dev->name = ir->name; input_dev->phys = ir->phys; input_dev->id.bustype = BUS_USB; diff --git a/drivers/media/video/cx231xx/cx231xx.h b/drivers/media/video/cx231xx/cx231xx.h index 17d4d1a..38d4171 100644 --- a/drivers/media/video/cx231xx/cx231xx.h +++ b/drivers/media/video/cx231xx/cx231xx.h @@ -32,7 +32,7 @@ #include <media/videobuf-vmalloc.h> #include <media/v4l2-device.h> -#include <media/ir-kbd-i2c.h> +#include <media/ir-core.h> #if defined(CONFIG_VIDEO_CX231XX_DVB) || \ defined(CONFIG_VIDEO_CX231XX_DVB_MODULE) #include <media/videobuf-dvb.h> -- To unsubscribe from this list: send the line "unsubscribe linux-input" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html ^ permalink raw reply related [flat|nested] 25+ messages in thread
end of thread, other threads:[~2010-07-06 17:12 UTC | newest] Thread overview: 25+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2010-04-24 21:13 [PATCH 0/4] ir-core sysfs protocol selection simplification David Härdeman 2010-04-24 21:14 ` [PATCH 1/4] ir-core: remove IR_TYPE_PD David Härdeman 2010-04-24 21:14 ` [PATCH 2/4] ir-core: centralize sysfs raw decoder enabling/disabling David Härdeman 2010-05-03 19:49 ` Mauro Carvalho Chehab 2010-06-07 18:48 ` David Härdeman 2010-04-24 21:14 ` [PATCH 3/4] ir-core: move decoding state to ir_raw_event_ctrl David Härdeman 2010-05-03 20:00 ` Mauro Carvalho Chehab 2010-06-07 19:00 ` David Härdeman 2010-06-07 20:15 ` Jarod Wilson 2010-06-08 17:50 ` David Härdeman 2010-06-09 3:46 ` Jarod Wilson 2010-06-09 13:29 ` Jarod Wilson 2010-06-09 17:56 ` David Härdeman 2010-06-09 18:15 ` Jarod Wilson 2010-06-10 1:25 ` Jarod Wilson 2010-06-13 20:29 ` David Härdeman 2010-06-16 20:04 ` Jarod Wilson 2010-06-16 20:41 ` Jarod Wilson 2010-06-17 12:14 ` Andy Walls 2010-06-17 15:11 ` Jarod Wilson 2010-06-21 0:47 ` Andy Walls 2010-06-21 3:51 ` Jarod Wilson 2010-06-21 11:04 ` Andy Walls 2010-07-06 17:12 ` Jarod Wilson 2010-04-24 21:14 ` [PATCH 4/4] ir-core: remove ir-functions usage from cx231xx David Härdeman
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).