| Message ID | 20171025004005.hyb43h3yvovp4is2@dtor-ws (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi,
[PATCH] media: av7110: switch to useing timer_setup()
^^^^^^^
typo error.
2017-10-25 9:40 GMT+09:00 Dmitry Torokhov <dmitry.torokhov@gmail.com>:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
>
> Also stop poking into input core internals and override its autorepeat
> timer function. I am not sure why we have such convoluted autorepeat
> handling in this driver instead of letting input core handle autorepeat,
> but this preserves current behavior of allowing controlling autorepeat
> delay and forcing autorepeat period to be whatever the hardware has.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
>
> Note that this has not been tested on the hardware. But it should
> compile, so I have that going for me.
>
> drivers/media/pci/ttpci/av7110.h | 4 ++--
> drivers/media/pci/ttpci/av7110_ir.c | 40 +++++++++++++++++--------------------
> 2 files changed, 20 insertions(+), 24 deletions(-)
>
> diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..0aa3c6f01853 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -80,10 +80,11 @@ struct av7110;
>
> /* infrared remote control */
> struct infrared {
> - u16 key_map[256];
> + u16 key_map[256];
> struct input_dev *input_dev;
> char input_phys[32];
> struct timer_list keyup_timer;
> + unsigned long keydown_time;
> struct tasklet_struct ir_tasklet;
> void (*ir_handler)(struct av7110 *av7110, u32 ircom);
> u32 ir_command;
> @@ -93,7 +94,6 @@ struct infrared {
> u8 inversion;
> u16 last_key;
> u16 last_toggle;
> - u8 delay_timer_finished;
> };
>
>
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..b602e64b3412 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -84,9 +84,9 @@ static u16 default_key_map [256] = {
>
>
> /* key-up timer */
> -static void av7110_emit_keyup(unsigned long parm)
> +static void av7110_emit_keyup(struct timer_list *t)
> {
> - struct infrared *ir = (struct infrared *) parm;
> + struct infrared *ir = from_timer(ir, keyup_timer, t);
>
> if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> return;
> @@ -152,19 +152,20 @@ static void av7110_emit_key(unsigned long parm)
> return;
> }
>
> - if (timer_pending(&ir->keyup_timer)) {
> - del_timer(&ir->keyup_timer);
> + if (del_timer(&ir->keyup_timer)) {
> if (ir->last_key != keycode || toggle != ir->last_toggle) {
> - ir->delay_timer_finished = 0;
> + ir->keydown_time = jiffies;
> input_event(ir->input_dev, EV_KEY, ir->last_key, 0);
> input_event(ir->input_dev, EV_KEY, keycode, 1);
> input_sync(ir->input_dev);
> - } else if (ir->delay_timer_finished) {
> + } else if (time_after(jiffies, ir->keydown_time +
> + msecs_to_jiffies(
> + ir->input_dev->rep[REP_PERIOD]))) {
> input_event(ir->input_dev, EV_KEY, keycode, 2);
> input_sync(ir->input_dev);
> }
> } else {
> - ir->delay_timer_finished = 0;
> + ir->keydown_time = jiffies;
> input_event(ir->input_dev, EV_KEY, keycode, 1);
> input_sync(ir->input_dev);
> }
> @@ -172,9 +173,7 @@ static void av7110_emit_key(unsigned long parm)
> ir->last_key = keycode;
> ir->last_toggle = toggle;
>
> - ir->keyup_timer.expires = jiffies + UP_TIMEOUT;
> - add_timer(&ir->keyup_timer);
> -
> + mod_timer(&ir->keyup_timer, jiffies + UP_TIMEOUT);
> }
>
>
> @@ -184,12 +183,19 @@ static void input_register_keys(struct infrared *ir)
> int i;
>
> set_bit(EV_KEY, ir->input_dev->evbit);
> - set_bit(EV_REP, ir->input_dev->evbit);
> set_bit(EV_MSC, ir->input_dev->evbit);
>
> set_bit(MSC_RAW, ir->input_dev->mscbit);
> set_bit(MSC_SCAN, ir->input_dev->mscbit);
>
> + set_bit(EV_REP, ir->input_dev->evbit);
> + /*
> + * By setting the delay before registering input device we
> + * indicate that we will be implementing the autorepeat
> + * ourselves.
> + */
> + ir->input_dev->rep[REP_DELAY] = 250;
> +
> memset(ir->input_dev->keybit, 0, sizeof(ir->input_dev->keybit));
>
> for (i = 0; i < ARRAY_SIZE(ir->key_map); i++) {
> @@ -205,15 +211,6 @@ static void input_register_keys(struct infrared *ir)
> }
>
>
> -/* called by the input driver after rep[REP_DELAY] ms */
> -static void input_repeat_key(unsigned long parm)
> -{
> - struct infrared *ir = (struct infrared *) parm;
> -
> - ir->delay_timer_finished = 1;
> -}
> -
> -
> /* check for configuration changes */
> int av7110_check_ir_config(struct av7110 *av7110, int force)
> {
> @@ -333,8 +330,7 @@ int av7110_ir_init(struct av7110 *av7110)
> av_list[av_cnt++] = av7110;
> av7110_check_ir_config(av7110, true);
>
> - setup_timer(&av7110->ir.keyup_timer, av7110_emit_keyup,
> - (unsigned long)&av7110->ir);
> + timer_setup(&av7110->ir.keyup_timer, av7110_emit_keyup, 0);
>
> input_dev = input_allocate_device();
> if (!input_dev)
> --
> 2.15.0.rc0.271.g36b669edcc-goog
>
>
> --
> Dmitry
Hi Dmitry,
Thank you for the patch! Yet we hit a small issue.
[auto build test ERROR on linuxtv-media/master]
[also build test ERROR on v4.14-rc6 next-20171018]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Dmitry-Torokhov/media-av7110-switch-to-useing-timer_setup/20171027-014646
base: git://linuxtv.org/media_tree.git master
config: x86_64-randconfig-x001-201743 (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
# save the attached .config to linux build tree
make ARCH=x86_64
All error/warnings (new ones prefixed by >>):
In file included from include/linux/list.h:8:0,
from include/linux/module.h:9,
from drivers/media//pci/ttpci/av7110_ir.c:24:
drivers/media//pci/ttpci/av7110_ir.c: In function 'av7110_emit_keyup':
>> drivers/media//pci/ttpci/av7110_ir.c:89:39: error: 'keyup_timer' undeclared (first use in this function)
struct infrared *ir = from_timer(ir, keyup_timer, t);
^
include/linux/kernel.h:927:26: note: in definition of macro 'container_of'
void *__mptr = (void *)(ptr); \
^~~
>> drivers/media//pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
drivers/media//pci/ttpci/av7110_ir.c:89:39: note: each undeclared identifier is reported only once for each function it appears in
struct infrared *ir = from_timer(ir, keyup_timer, t);
^
include/linux/kernel.h:927:26: note: in definition of macro 'container_of'
void *__mptr = (void *)(ptr); \
^~~
>> drivers/media//pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from drivers/media//pci/ttpci/av7110_ir.c:22:
>> include/linux/kernel.h:928:51: error: 'struct infrared' has no member named 't'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/compiler.h:553:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^~~~~~~~~
include/linux/compiler.h:576:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:46:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/kernel.h:928:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~~~~~~
include/linux/kernel.h:928:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~
include/linux/timer.h:183:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^~~~~~~~~~~~
>> drivers/media//pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
In file included from include/linux/compiler.h:58:0,
from include/uapi/linux/stddef.h:1,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from drivers/media//pci/ttpci/av7110_ir.c:22:
>> include/linux/compiler-gcc.h:165:2: error: 'struct infrared' has no member named 't'
__builtin_offsetof(a, b)
^
include/linux/stddef.h:16:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:931:21: note: in expansion of macro 'offsetof'
((type *)(__mptr - offsetof(type, member))); })
^~~~~~~~
include/linux/timer.h:183:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^~~~~~~~~~~~
>> drivers/media//pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
drivers/media//pci/ttpci/av7110_ir.c: In function 'av7110_ir_init':
>> drivers/media//pci/ttpci/av7110_ir.c:364:30: error: 'input_repeat_key' undeclared (first use in this function)
input_dev->timer.function = input_repeat_key;
^~~~~~~~~~~~~~~~
--
In file included from include/linux/list.h:8:0,
from include/linux/module.h:9,
from drivers/media/pci/ttpci/av7110_ir.c:24:
drivers/media/pci/ttpci/av7110_ir.c: In function 'av7110_emit_keyup':
drivers/media/pci/ttpci/av7110_ir.c:89:39: error: 'keyup_timer' undeclared (first use in this function)
struct infrared *ir = from_timer(ir, keyup_timer, t);
^
include/linux/kernel.h:927:26: note: in definition of macro 'container_of'
void *__mptr = (void *)(ptr); \
^~~
drivers/media/pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
drivers/media/pci/ttpci/av7110_ir.c:89:39: note: each undeclared identifier is reported only once for each function it appears in
struct infrared *ir = from_timer(ir, keyup_timer, t);
^
include/linux/kernel.h:927:26: note: in definition of macro 'container_of'
void *__mptr = (void *)(ptr); \
^~~
drivers/media/pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
In file included from include/uapi/linux/stddef.h:1:0,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from drivers/media/pci/ttpci/av7110_ir.c:22:
>> include/linux/kernel.h:928:51: error: 'struct infrared' has no member named 't'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^
include/linux/compiler.h:553:19: note: in definition of macro '__compiletime_assert'
bool __cond = !(condition); \
^~~~~~~~~
include/linux/compiler.h:576:2: note: in expansion of macro '_compiletime_assert'
_compiletime_assert(condition, msg, __compiletime_assert_, __LINE__)
^~~~~~~~~~~~~~~~~~~
include/linux/build_bug.h:46:37: note: in expansion of macro 'compiletime_assert'
#define BUILD_BUG_ON_MSG(cond, msg) compiletime_assert(!(cond), msg)
^~~~~~~~~~~~~~~~~~
include/linux/kernel.h:928:2: note: in expansion of macro 'BUILD_BUG_ON_MSG'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~~~~~~
include/linux/kernel.h:928:20: note: in expansion of macro '__same_type'
BUILD_BUG_ON_MSG(!__same_type(*(ptr), ((type *)0)->member) && \
^~~~~~~~~~~
include/linux/timer.h:183:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^~~~~~~~~~~~
drivers/media/pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
In file included from include/linux/compiler.h:58:0,
from include/uapi/linux/stddef.h:1,
from include/linux/stddef.h:4,
from include/uapi/linux/posix_types.h:4,
from include/uapi/linux/types.h:13,
from include/linux/types.h:5,
from drivers/media/pci/ttpci/av7110_ir.c:22:
>> include/linux/compiler-gcc.h:165:2: error: 'struct infrared' has no member named 't'
__builtin_offsetof(a, b)
^
include/linux/stddef.h:16:32: note: in expansion of macro '__compiler_offsetof'
#define offsetof(TYPE, MEMBER) __compiler_offsetof(TYPE, MEMBER)
^~~~~~~~~~~~~~~~~~~
include/linux/kernel.h:931:21: note: in expansion of macro 'offsetof'
((type *)(__mptr - offsetof(type, member))); })
^~~~~~~~
include/linux/timer.h:183:2: note: in expansion of macro 'container_of'
container_of(callback_timer, typeof(*var), timer_fieldname)
^~~~~~~~~~~~
drivers/media/pci/ttpci/av7110_ir.c:89:24: note: in expansion of macro 'from_timer'
struct infrared *ir = from_timer(ir, keyup_timer, t);
^~~~~~~~~~
drivers/media/pci/ttpci/av7110_ir.c: In function 'av7110_ir_init':
drivers/media/pci/ttpci/av7110_ir.c:364:30: error: 'input_repeat_key' undeclared (first use in this function)
input_dev->timer.function = input_repeat_key;
^~~~~~~~~~~~~~~~
vim +/keyup_timer +89 drivers/media//pci/ttpci/av7110_ir.c
> 24 #include <linux/module.h>
25 #include <linux/proc_fs.h>
26 #include <linux/kernel.h>
27 #include <linux/bitops.h>
28
29 #include "av7110.h"
30 #include "av7110_hw.h"
31
32
33 #define AV_CNT 4
34
35 #define IR_RC5 0
36 #define IR_RCMM 1
37 #define IR_RC5_EXT 2 /* internal only */
38
39 #define IR_ALL 0xffffffff
40
41 #define UP_TIMEOUT (HZ*7/25)
42
43
44 /* Note: enable ir debugging by or'ing debug with 16 */
45
46 static int ir_protocol[AV_CNT] = { IR_RCMM, IR_RCMM, IR_RCMM, IR_RCMM};
47 module_param_array(ir_protocol, int, NULL, 0644);
48 MODULE_PARM_DESC(ir_protocol, "Infrared protocol: 0 RC5, 1 RCMM (default)");
49
50 static int ir_inversion[AV_CNT];
51 module_param_array(ir_inversion, int, NULL, 0644);
52 MODULE_PARM_DESC(ir_inversion, "Inversion of infrared signal: 0 not inverted (default), 1 inverted");
53
54 static uint ir_device_mask[AV_CNT] = { IR_ALL, IR_ALL, IR_ALL, IR_ALL };
55 module_param_array(ir_device_mask, uint, NULL, 0644);
56 MODULE_PARM_DESC(ir_device_mask, "Bitmask of infrared devices: bit 0..31 = device 0..31 (default: all)");
57
58
59 static int av_cnt;
60 static struct av7110 *av_list[AV_CNT];
61
62 static u16 default_key_map [256] = {
63 KEY_0, KEY_1, KEY_2, KEY_3, KEY_4, KEY_5, KEY_6, KEY_7,
64 KEY_8, KEY_9, KEY_BACK, 0, KEY_POWER, KEY_MUTE, 0, KEY_INFO,
65 KEY_VOLUMEUP, KEY_VOLUMEDOWN, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
66 KEY_CHANNELUP, KEY_CHANNELDOWN, 0, 0, 0, 0, 0, 0,
67 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
68 0, 0, 0, 0, KEY_TEXT, 0, 0, KEY_TV, 0, 0, 0, 0, 0, KEY_SETUP, 0, 0,
69 0, 0, 0, KEY_SUBTITLE, 0, 0, KEY_LANGUAGE, 0,
70 KEY_RADIO, 0, 0, 0, 0, KEY_EXIT, 0, 0,
71 KEY_UP, KEY_DOWN, KEY_LEFT, KEY_RIGHT, KEY_OK, 0, 0, 0,
72 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_RED, KEY_GREEN, KEY_YELLOW,
73 KEY_BLUE, 0, 0, 0, 0, 0, 0, 0, KEY_MENU, KEY_LIST, 0, 0, 0, 0, 0, 0,
74 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
75 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
76 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
77 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
78 0, 0, 0, 0, KEY_UP, KEY_UP, KEY_DOWN, KEY_DOWN,
79 0, 0, 0, 0, KEY_EPG, 0, 0, 0,
80 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
81 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0,
82 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, KEY_VCR
83 };
84
85
86 /* key-up timer */
87 static void av7110_emit_keyup(struct timer_list *t)
88 {
> 89 struct infrared *ir = from_timer(ir, keyup_timer, t);
90
91 if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
92 return;
93
94 input_report_key(ir->input_dev, ir->last_key, 0);
95 input_sync(ir->input_dev);
96 }
97
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
On Tue, Oct 24, 2017 at 5:40 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > In preparation for unconditionally passing the struct timer_list pointer to > all timer callbacks, switch to using the new timer_setup() and from_timer() > to pass the timer pointer explicitly. > > Also stop poking into input core internals and override its autorepeat > timer function. I am not sure why we have such convoluted autorepeat > handling in this driver instead of letting input core handle autorepeat, > but this preserves current behavior of allowing controlling autorepeat > delay and forcing autorepeat period to be whatever the hardware has. > > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> Reviewed-by: Kees Cook <keescook@chromium.org> (with the Subject typo fixed) Hans, since this depends on the input side not changing first, I think it makes sense for Dmitry to carry this in the Input tree before the Input timer updates. Would that be okay? Thanks! -Kees > --- > > Note that this has not been tested on the hardware. But it should > compile, so I have that going for me. > > drivers/media/pci/ttpci/av7110.h | 4 ++-- > drivers/media/pci/ttpci/av7110_ir.c | 40 +++++++++++++++++-------------------- > 2 files changed, 20 insertions(+), 24 deletions(-) > > diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h > index 347827925c14..0aa3c6f01853 100644 > --- a/drivers/media/pci/ttpci/av7110.h > +++ b/drivers/media/pci/ttpci/av7110.h > @@ -80,10 +80,11 @@ struct av7110; > > /* infrared remote control */ > struct infrared { > - u16 key_map[256]; > + u16 key_map[256]; > struct input_dev *input_dev; > char input_phys[32]; > struct timer_list keyup_timer; > + unsigned long keydown_time; > struct tasklet_struct ir_tasklet; > void (*ir_handler)(struct av7110 *av7110, u32 ircom); > u32 ir_command; > @@ -93,7 +94,6 @@ struct infrared { > u8 inversion; > u16 last_key; > u16 last_toggle; > - u8 delay_timer_finished; > }; > > > diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c > index ca05198de2c2..b602e64b3412 100644 > --- a/drivers/media/pci/ttpci/av7110_ir.c > +++ b/drivers/media/pci/ttpci/av7110_ir.c > @@ -84,9 +84,9 @@ static u16 default_key_map [256] = { > > > /* key-up timer */ > -static void av7110_emit_keyup(unsigned long parm) > +static void av7110_emit_keyup(struct timer_list *t) > { > - struct infrared *ir = (struct infrared *) parm; > + struct infrared *ir = from_timer(ir, keyup_timer, t); > > if (!ir || !test_bit(ir->last_key, ir->input_dev->key)) > return; > @@ -152,19 +152,20 @@ static void av7110_emit_key(unsigned long parm) > return; > } > > - if (timer_pending(&ir->keyup_timer)) { > - del_timer(&ir->keyup_timer); > + if (del_timer(&ir->keyup_timer)) { > if (ir->last_key != keycode || toggle != ir->last_toggle) { > - ir->delay_timer_finished = 0; > + ir->keydown_time = jiffies; > input_event(ir->input_dev, EV_KEY, ir->last_key, 0); > input_event(ir->input_dev, EV_KEY, keycode, 1); > input_sync(ir->input_dev); > - } else if (ir->delay_timer_finished) { > + } else if (time_after(jiffies, ir->keydown_time + > + msecs_to_jiffies( > + ir->input_dev->rep[REP_PERIOD]))) { > input_event(ir->input_dev, EV_KEY, keycode, 2); > input_sync(ir->input_dev); > } > } else { > - ir->delay_timer_finished = 0; > + ir->keydown_time = jiffies; > input_event(ir->input_dev, EV_KEY, keycode, 1); > input_sync(ir->input_dev); > } > @@ -172,9 +173,7 @@ static void av7110_emit_key(unsigned long parm) > ir->last_key = keycode; > ir->last_toggle = toggle; > > - ir->keyup_timer.expires = jiffies + UP_TIMEOUT; > - add_timer(&ir->keyup_timer); > - > + mod_timer(&ir->keyup_timer, jiffies + UP_TIMEOUT); > } > > > @@ -184,12 +183,19 @@ static void input_register_keys(struct infrared *ir) > int i; > > set_bit(EV_KEY, ir->input_dev->evbit); > - set_bit(EV_REP, ir->input_dev->evbit); > set_bit(EV_MSC, ir->input_dev->evbit); > > set_bit(MSC_RAW, ir->input_dev->mscbit); > set_bit(MSC_SCAN, ir->input_dev->mscbit); > > + set_bit(EV_REP, ir->input_dev->evbit); > + /* > + * By setting the delay before registering input device we > + * indicate that we will be implementing the autorepeat > + * ourselves. > + */ > + ir->input_dev->rep[REP_DELAY] = 250; > + > memset(ir->input_dev->keybit, 0, sizeof(ir->input_dev->keybit)); > > for (i = 0; i < ARRAY_SIZE(ir->key_map); i++) { > @@ -205,15 +211,6 @@ static void input_register_keys(struct infrared *ir) > } > > > -/* called by the input driver after rep[REP_DELAY] ms */ > -static void input_repeat_key(unsigned long parm) > -{ > - struct infrared *ir = (struct infrared *) parm; > - > - ir->delay_timer_finished = 1; > -} > - > - > /* check for configuration changes */ > int av7110_check_ir_config(struct av7110 *av7110, int force) > { > @@ -333,8 +330,7 @@ int av7110_ir_init(struct av7110 *av7110) > av_list[av_cnt++] = av7110; > av7110_check_ir_config(av7110, true); > > - setup_timer(&av7110->ir.keyup_timer, av7110_emit_keyup, > - (unsigned long)&av7110->ir); > + timer_setup(&av7110->ir.keyup_timer, av7110_emit_keyup, 0); > > input_dev = input_allocate_device(); > if (!input_dev) > -- > 2.15.0.rc0.271.g36b669edcc-goog > > > -- > Dmitry
On Tue, Oct 24, 2017 at 05:40:05PM -0700, Dmitry Torokhov wrote: > In preparation for unconditionally passing the struct timer_list pointer to > all timer callbacks, switch to using the new timer_setup() and from_timer() > to pass the timer pointer explicitly. > > Also stop poking into input core internals and override its autorepeat > timer function. I am not sure why we have such convoluted autorepeat > handling in this driver instead of letting input core handle autorepeat, > but this preserves current behavior of allowing controlling autorepeat > delay and forcing autorepeat period to be whatever the hardware has. I think a better solution is to remove the autorepeat handling completely, and leave it up to the input layer. This simplies the driver greatly and I don't see how this makes sense for an IR driver. The IR protocols specify the IR should repeat the message at set intervals whilst a button is pressed; this has no relation to autorepeat. Ideally this driver would be converted to rc-core, but without access to the hardware I'm not sure that is a great idea. The keymap handling is very convolated so I have no idea what the scancodes for the remote(s) are, for example. Any suggestions for what hardware to get off ebay for this? New patch will be reply this. Thanks, Sean
Em Tue, 31 Oct 2017 17:27:58 +0000 Sean Young <sean@mess.org> escreveu: > On Tue, Oct 24, 2017 at 05:40:05PM -0700, Dmitry Torokhov wrote: > > In preparation for unconditionally passing the struct timer_list pointer to > > all timer callbacks, switch to using the new timer_setup() and from_timer() > > to pass the timer pointer explicitly. > > > > Also stop poking into input core internals and override its autorepeat > > timer function. I am not sure why we have such convoluted autorepeat > > handling in this driver instead of letting input core handle autorepeat, > > but this preserves current behavior of allowing controlling autorepeat > > delay and forcing autorepeat period to be whatever the hardware has. IR devices basically have only something equivalent to key press events. They don't have key release ones. Depending on the device and on the IR protocol, they may have extra events when a key is kept pressed. I've seen two types of IR devices: the ones that emit an special "repeat" event (usually on every 100ms-200ms if a key is kept pressing) and the ones that just repeat the key's scan code if the key is kept pressed. The way IR drivers current work is that they send a key press event when a key is pressed, and if no repeat event happens on an expected time, it sends a release event. Thanks, Mauro
diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h index 347827925c14..0aa3c6f01853 100644 --- a/drivers/media/pci/ttpci/av7110.h +++ b/drivers/media/pci/ttpci/av7110.h @@ -80,10 +80,11 @@ struct av7110; /* infrared remote control */ struct infrared { - u16 key_map[256]; + u16 key_map[256]; struct input_dev *input_dev; char input_phys[32]; struct timer_list keyup_timer; + unsigned long keydown_time; struct tasklet_struct ir_tasklet; void (*ir_handler)(struct av7110 *av7110, u32 ircom); u32 ir_command; @@ -93,7 +94,6 @@ struct infrared { u8 inversion; u16 last_key; u16 last_toggle; - u8 delay_timer_finished; }; diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c index ca05198de2c2..b602e64b3412 100644 --- a/drivers/media/pci/ttpci/av7110_ir.c +++ b/drivers/media/pci/ttpci/av7110_ir.c @@ -84,9 +84,9 @@ static u16 default_key_map [256] = { /* key-up timer */ -static void av7110_emit_keyup(unsigned long parm) +static void av7110_emit_keyup(struct timer_list *t) { - struct infrared *ir = (struct infrared *) parm; + struct infrared *ir = from_timer(ir, keyup_timer, t); if (!ir || !test_bit(ir->last_key, ir->input_dev->key)) return; @@ -152,19 +152,20 @@ static void av7110_emit_key(unsigned long parm) return; } - if (timer_pending(&ir->keyup_timer)) { - del_timer(&ir->keyup_timer); + if (del_timer(&ir->keyup_timer)) { if (ir->last_key != keycode || toggle != ir->last_toggle) { - ir->delay_timer_finished = 0; + ir->keydown_time = jiffies; input_event(ir->input_dev, EV_KEY, ir->last_key, 0); input_event(ir->input_dev, EV_KEY, keycode, 1); input_sync(ir->input_dev); - } else if (ir->delay_timer_finished) { + } else if (time_after(jiffies, ir->keydown_time + + msecs_to_jiffies( + ir->input_dev->rep[REP_PERIOD]))) { input_event(ir->input_dev, EV_KEY, keycode, 2); input_sync(ir->input_dev); } } else { - ir->delay_timer_finished = 0; + ir->keydown_time = jiffies; input_event(ir->input_dev, EV_KEY, keycode, 1); input_sync(ir->input_dev); } @@ -172,9 +173,7 @@ static void av7110_emit_key(unsigned long parm) ir->last_key = keycode; ir->last_toggle = toggle; - ir->keyup_timer.expires = jiffies + UP_TIMEOUT; - add_timer(&ir->keyup_timer); - + mod_timer(&ir->keyup_timer, jiffies + UP_TIMEOUT); } @@ -184,12 +183,19 @@ static void input_register_keys(struct infrared *ir) int i; set_bit(EV_KEY, ir->input_dev->evbit); - set_bit(EV_REP, ir->input_dev->evbit); set_bit(EV_MSC, ir->input_dev->evbit); set_bit(MSC_RAW, ir->input_dev->mscbit); set_bit(MSC_SCAN, ir->input_dev->mscbit); + set_bit(EV_REP, ir->input_dev->evbit); + /* + * By setting the delay before registering input device we + * indicate that we will be implementing the autorepeat + * ourselves. + */ + ir->input_dev->rep[REP_DELAY] = 250; + memset(ir->input_dev->keybit, 0, sizeof(ir->input_dev->keybit)); for (i = 0; i < ARRAY_SIZE(ir->key_map); i++) { @@ -205,15 +211,6 @@ static void input_register_keys(struct infrared *ir) } -/* called by the input driver after rep[REP_DELAY] ms */ -static void input_repeat_key(unsigned long parm) -{ - struct infrared *ir = (struct infrared *) parm; - - ir->delay_timer_finished = 1; -} - - /* check for configuration changes */ int av7110_check_ir_config(struct av7110 *av7110, int force) { @@ -333,8 +330,7 @@ int av7110_ir_init(struct av7110 *av7110) av_list[av_cnt++] = av7110; av7110_check_ir_config(av7110, true); - setup_timer(&av7110->ir.keyup_timer, av7110_emit_keyup, - (unsigned long)&av7110->ir); + timer_setup(&av7110->ir.keyup_timer, av7110_emit_keyup, 0); input_dev = input_allocate_device(); if (!input_dev)
In preparation for unconditionally passing the struct timer_list pointer to all timer callbacks, switch to using the new timer_setup() and from_timer() to pass the timer pointer explicitly. Also stop poking into input core internals and override its autorepeat timer function. I am not sure why we have such convoluted autorepeat handling in this driver instead of letting input core handle autorepeat, but this preserves current behavior of allowing controlling autorepeat delay and forcing autorepeat period to be whatever the hardware has. Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- Note that this has not been tested on the hardware. But it should compile, so I have that going for me. drivers/media/pci/ttpci/av7110.h | 4 ++-- drivers/media/pci/ttpci/av7110_ir.c | 40 +++++++++++++++++-------------------- 2 files changed, 20 insertions(+), 24 deletions(-)