* [PATCH] media: av7110: switch to useing timer_setup()
@ 2017-10-25 0:40 Dmitry Torokhov
2017-10-25 1:03 ` Jaejoong Kim
` (3 more replies)
0 siblings, 4 replies; 15+ messages in thread
From: Dmitry Torokhov @ 2017-10-25 0:40 UTC (permalink / raw)
To: Mauro Carvalho Chehab; +Cc: Hans Verkuil, Kees Cook, linux-media, linux-kernel
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
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] media: av7110: switch to useing timer_setup()
2017-10-25 0:40 [PATCH] media: av7110: switch to useing timer_setup() Dmitry Torokhov
@ 2017-10-25 1:03 ` Jaejoong Kim
2017-10-26 18:27 ` kbuild test robot
` (2 subsequent siblings)
3 siblings, 0 replies; 15+ messages in thread
From: Jaejoong Kim @ 2017-10-25 1:03 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mauro Carvalho Chehab, Hans Verkuil, Kees Cook, linux-media, LKML
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] media: av7110: switch to useing timer_setup()
2017-10-25 0:40 [PATCH] media: av7110: switch to useing timer_setup() Dmitry Torokhov
2017-10-25 1:03 ` Jaejoong Kim
@ 2017-10-26 18:27 ` kbuild test robot
2017-10-30 22:14 ` Kees Cook
2017-10-31 17:27 ` Sean Young
3 siblings, 0 replies; 15+ messages in thread
From: kbuild test robot @ 2017-10-26 18:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: kbuild-all, Mauro Carvalho Chehab, Hans Verkuil, Kees Cook,
linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 13223 bytes --]
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
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 28163 bytes --]
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] media: av7110: switch to useing timer_setup()
2017-10-25 0:40 [PATCH] media: av7110: switch to useing timer_setup() Dmitry Torokhov
2017-10-25 1:03 ` Jaejoong Kim
2017-10-26 18:27 ` kbuild test robot
@ 2017-10-30 22:14 ` Kees Cook
2017-10-31 17:27 ` Sean Young
3 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2017-10-30 22:14 UTC (permalink / raw)
To: Dmitry Torokhov, Hans Verkuil; +Cc: Mauro Carvalho Chehab, linux-media, LKML
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
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] media: av7110: switch to useing timer_setup()
2017-10-25 0:40 [PATCH] media: av7110: switch to useing timer_setup() Dmitry Torokhov
` (2 preceding siblings ...)
2017-10-30 22:14 ` Kees Cook
@ 2017-10-31 17:27 ` Sean Young
2017-10-31 17:45 ` [PATCH] media: ttpci: remove autorepeat handling and use timer_setup Sean Young
2017-10-31 18:14 ` [PATCH] media: av7110: switch to useing timer_setup() Mauro Carvalho Chehab
3 siblings, 2 replies; 15+ messages in thread
From: Sean Young @ 2017-10-31 17:27 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mauro Carvalho Chehab, Hans Verkuil, Kees Cook, linux-media,
linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH] media: ttpci: remove autorepeat handling and use timer_setup
2017-10-31 17:27 ` Sean Young
@ 2017-10-31 17:45 ` Sean Young
2017-10-31 18:22 ` Dmitry Torokhov
2017-10-31 18:14 ` [PATCH] media: av7110: switch to useing timer_setup() Mauro Carvalho Chehab
1 sibling, 1 reply; 15+ messages in thread
From: Sean Young @ 2017-10-31 17:45 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mauro Carvalho Chehab, Hans Verkuil, Kees Cook, linux-media,
linux-kernel
Leave the autorepeat handling up to the input layer, and move
to the new timer API.
Compile tested only.
Signed-off-by: Sean Young <sean@mess.org>
---
drivers/media/pci/ttpci/av7110.h | 2 +-
drivers/media/pci/ttpci/av7110_ir.c | 54 ++++++++++++++-----------------------
2 files changed, 21 insertions(+), 35 deletions(-)
diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
index 347827925c14..bcb72ecbedc0 100644
--- a/drivers/media/pci/ttpci/av7110.h
+++ b/drivers/media/pci/ttpci/av7110.h
@@ -93,7 +93,7 @@ struct infrared {
u8 inversion;
u16 last_key;
u16 last_toggle;
- u8 delay_timer_finished;
+ bool keypressed;
};
diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
index ca05198de2c2..8207bead2224 100644
--- a/drivers/media/pci/ttpci/av7110_ir.c
+++ b/drivers/media/pci/ttpci/av7110_ir.c
@@ -84,15 +84,16 @@ 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, t, keyup_timer);
- if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
+ if (!ir || !ir->keypressed)
return;
input_report_key(ir->input_dev, ir->last_key, 0);
input_sync(ir->input_dev);
+ ir->keypressed = false;
}
@@ -105,6 +106,7 @@ static void av7110_emit_key(unsigned long parm)
u8 addr;
u16 toggle;
u16 keycode;
+ bool new_event;
/* extract device address and data */
switch (ir->protocol) {
@@ -152,29 +154,22 @@ static void av7110_emit_key(unsigned long parm)
return;
}
- if (timer_pending(&ir->keyup_timer)) {
- del_timer(&ir->keyup_timer);
- if (ir->last_key != keycode || toggle != ir->last_toggle) {
- ir->delay_timer_finished = 0;
- 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) {
- input_event(ir->input_dev, EV_KEY, keycode, 2);
- input_sync(ir->input_dev);
- }
- } else {
- ir->delay_timer_finished = 0;
- input_event(ir->input_dev, EV_KEY, keycode, 1);
+ new_event = !ir->keypressed || ir->last_key != keycode ||
+ toggle != ir->last_toggle;
+
+ if (new_event && ir->keypressed)
+ input_event(ir->input_dev, EV_KEY, ir->last_key, 1);
+
+ if (new_event) {
+ input_event(ir->input_dev, EV_KEY, keycode, 0);
input_sync(ir->input_dev);
}
+ ir->keypressed = true;
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);
}
@@ -204,16 +199,6 @@ static void input_register_keys(struct infrared *ir)
ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
}
-
-/* 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 +318,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)
@@ -365,8 +349,10 @@ int av7110_ir_init(struct av7110 *av7110)
input_free_device(input_dev);
return err;
}
- input_dev->timer.function = input_repeat_key;
- input_dev->timer.data = (unsigned long) &av7110->ir;
+
+ /* Let the input layer handle autorepeat for us */
+ input_dev->rep[REP_DELAY] = 250;
+ input_dev->rep[REP_PERIOD] = 125;
if (av_cnt == 1) {
e = proc_create("av7110_ir", S_IWUSR, NULL, &av7110_ir_proc_fops);
--
2.13.6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH] media: av7110: switch to useing timer_setup()
2017-10-31 17:27 ` Sean Young
2017-10-31 17:45 ` [PATCH] media: ttpci: remove autorepeat handling and use timer_setup Sean Young
@ 2017-10-31 18:14 ` Mauro Carvalho Chehab
1 sibling, 0 replies; 15+ messages in thread
From: Mauro Carvalho Chehab @ 2017-10-31 18:14 UTC (permalink / raw)
To: Sean Young
Cc: Dmitry Torokhov, Mauro Carvalho Chehab, Hans Verkuil, Kees Cook,
linux-media, linux-kernel
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
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] media: ttpci: remove autorepeat handling and use timer_setup
2017-10-31 17:45 ` [PATCH] media: ttpci: remove autorepeat handling and use timer_setup Sean Young
@ 2017-10-31 18:22 ` Dmitry Torokhov
2017-10-31 20:07 ` Sean Young
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2017-10-31 18:22 UTC (permalink / raw)
To: Sean Young
Cc: Mauro Carvalho Chehab, Hans Verkuil, Kees Cook, linux-media,
linux-kernel
Hi Sean,
On Tue, Oct 31, 2017 at 05:45:58PM +0000, Sean Young wrote:
> Leave the autorepeat handling up to the input layer, and move
> to the new timer API.
>
> Compile tested only.
>
> Signed-off-by: Sean Young <sean@mess.org>
> ---
> drivers/media/pci/ttpci/av7110.h | 2 +-
> drivers/media/pci/ttpci/av7110_ir.c | 54 ++++++++++++++-----------------------
> 2 files changed, 21 insertions(+), 35 deletions(-)
>
> diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..bcb72ecbedc0 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -93,7 +93,7 @@ struct infrared {
> u8 inversion;
> u16 last_key;
> u16 last_toggle;
> - u8 delay_timer_finished;
> + bool keypressed;
> };
>
>
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..8207bead2224 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -84,15 +84,16 @@ 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, t, keyup_timer);
>
> - if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> + if (!ir || !ir->keypressed)
> return;
>
> input_report_key(ir->input_dev, ir->last_key, 0);
> input_sync(ir->input_dev);
> + ir->keypressed = false;
> }
>
>
> @@ -105,6 +106,7 @@ static void av7110_emit_key(unsigned long parm)
> u8 addr;
> u16 toggle;
> u16 keycode;
> + bool new_event;
>
> /* extract device address and data */
> switch (ir->protocol) {
> @@ -152,29 +154,22 @@ static void av7110_emit_key(unsigned long parm)
> return;
> }
>
> - if (timer_pending(&ir->keyup_timer)) {
> - del_timer(&ir->keyup_timer);
> - if (ir->last_key != keycode || toggle != ir->last_toggle) {
> - ir->delay_timer_finished = 0;
> - 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) {
> - input_event(ir->input_dev, EV_KEY, keycode, 2);
> - input_sync(ir->input_dev);
> - }
> - } else {
> - ir->delay_timer_finished = 0;
> - input_event(ir->input_dev, EV_KEY, keycode, 1);
> + new_event = !ir->keypressed || ir->last_key != keycode ||
> + toggle != ir->last_toggle;
> +
> + if (new_event && ir->keypressed)
> + input_event(ir->input_dev, EV_KEY, ir->last_key, 1);
> +
> + if (new_event) {
> + input_event(ir->input_dev, EV_KEY, keycode, 0);
I do not think this is correct. You want to release the old button, and
press the new one, not the other way around.
Given that we are reworking the code, and input core actually filters
out duplicate events for you, you can probably simplify it all further:
/* Release old key/button */
if (ir->keypressed && ir->last_key != keycode)
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);
and get rid of last_toggle member.
> input_sync(ir->input_dev);
> }
>
> + ir->keypressed = true;
> 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);
> }
>
>
> @@ -204,16 +199,6 @@ static void input_register_keys(struct infrared *ir)
> ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
> }
>
> -
> -/* 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 +318,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)
> @@ -365,8 +349,10 @@ int av7110_ir_init(struct av7110 *av7110)
> input_free_device(input_dev);
> return err;
> }
> - input_dev->timer.function = input_repeat_key;
> - input_dev->timer.data = (unsigned long) &av7110->ir;
> +
> + /* Let the input layer handle autorepeat for us */
> + input_dev->rep[REP_DELAY] = 250;
> + input_dev->rep[REP_PERIOD] = 125;
I'd change this to:
/*
* Input core's default autorepeat is 33 cps with 250 msec
* delay, let's adjust to numbers more suitable for remote
* control.
*/
input_enable_softrepeat(input_dev, 250, 125);
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] media: ttpci: remove autorepeat handling and use timer_setup
2017-10-31 18:22 ` Dmitry Torokhov
@ 2017-10-31 20:07 ` Sean Young
2017-10-31 20:11 ` [PATCH v2] " Sean Young
0 siblings, 1 reply; 15+ messages in thread
From: Sean Young @ 2017-10-31 20:07 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mauro Carvalho Chehab, Hans Verkuil, Kees Cook, linux-media,
linux-kernel
Hi Dmitry,
On Tue, Oct 31, 2017 at 11:22:36AM -0700, Dmitry Torokhov wrote:
> Hi Sean,
>
> On Tue, Oct 31, 2017 at 05:45:58PM +0000, Sean Young wrote:
> > Leave the autorepeat handling up to the input layer, and move
> > to the new timer API.
> >
> > Compile tested only.
> >
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> > drivers/media/pci/ttpci/av7110.h | 2 +-
> > drivers/media/pci/ttpci/av7110_ir.c | 54 ++++++++++++++-----------------------
> > 2 files changed, 21 insertions(+), 35 deletions(-)
> >
> > diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> > index 347827925c14..bcb72ecbedc0 100644
> > --- a/drivers/media/pci/ttpci/av7110.h
> > +++ b/drivers/media/pci/ttpci/av7110.h
> > @@ -93,7 +93,7 @@ struct infrared {
> > u8 inversion;
> > u16 last_key;
> > u16 last_toggle;
> > - u8 delay_timer_finished;
> > + bool keypressed;
> > };
> >
> >
> > diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> > index ca05198de2c2..8207bead2224 100644
> > --- a/drivers/media/pci/ttpci/av7110_ir.c
> > +++ b/drivers/media/pci/ttpci/av7110_ir.c
> > @@ -84,15 +84,16 @@ 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, t, keyup_timer);
> >
> > - if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> > + if (!ir || !ir->keypressed)
> > return;
> >
> > input_report_key(ir->input_dev, ir->last_key, 0);
> > input_sync(ir->input_dev);
> > + ir->keypressed = false;
> > }
> >
> >
> > @@ -105,6 +106,7 @@ static void av7110_emit_key(unsigned long parm)
> > u8 addr;
> > u16 toggle;
> > u16 keycode;
> > + bool new_event;
> >
> > /* extract device address and data */
> > switch (ir->protocol) {
> > @@ -152,29 +154,22 @@ static void av7110_emit_key(unsigned long parm)
> > return;
> > }
> >
> > - if (timer_pending(&ir->keyup_timer)) {
> > - del_timer(&ir->keyup_timer);
> > - if (ir->last_key != keycode || toggle != ir->last_toggle) {
> > - ir->delay_timer_finished = 0;
> > - 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) {
> > - input_event(ir->input_dev, EV_KEY, keycode, 2);
> > - input_sync(ir->input_dev);
> > - }
> > - } else {
> > - ir->delay_timer_finished = 0;
> > - input_event(ir->input_dev, EV_KEY, keycode, 1);
> > + new_event = !ir->keypressed || ir->last_key != keycode ||
> > + toggle != ir->last_toggle;
> > +
> > + if (new_event && ir->keypressed)
> > + input_event(ir->input_dev, EV_KEY, ir->last_key, 1);
> > +
> > + if (new_event) {
> > + input_event(ir->input_dev, EV_KEY, keycode, 0);
>
> I do not think this is correct. You want to release the old button, and
> press the new one, not the other way around.
Yes, you are right. 0 is for key up, 1 is for key down.
> Given that we are reworking the code, and input core actually filters
> out duplicate events for you, you can probably simplify it all further:
Ah, that's useful to know.
> /* Release old key/button */
> if (ir->keypressed && ir->last_key != keycode)
> 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);
That is better.
> and get rid of last_toggle member.
The last_toggle is needed, it's a feature of the RC5 and RC6 IR protocol.
Since there is no key up IR message (just key down which gets repeated if
the key held down), there is no way to distinguish between key hold
and press-release-press. The toggle is a bit in the RC5 and RC6 IR protocol,
which flips if the user pressed, released and then presses a button again.
So if the toggle bit changes, we want to send a keyup/keydown event and
reset autorepeat.
>
> > input_sync(ir->input_dev);
> > }
> >
> > + ir->keypressed = true;
> > 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);
> > }
> >
> >
> > @@ -204,16 +199,6 @@ static void input_register_keys(struct infrared *ir)
> > ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
> > }
> >
> > -
> > -/* 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 +318,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)
> > @@ -365,8 +349,10 @@ int av7110_ir_init(struct av7110 *av7110)
> > input_free_device(input_dev);
> > return err;
> > }
> > - input_dev->timer.function = input_repeat_key;
> > - input_dev->timer.data = (unsigned long) &av7110->ir;
> > +
> > + /* Let the input layer handle autorepeat for us */
> > + input_dev->rep[REP_DELAY] = 250;
> > + input_dev->rep[REP_PERIOD] = 125;
>
>
> I'd change this to:
>
> /*
> * Input core's default autorepeat is 33 cps with 250 msec
> * delay, let's adjust to numbers more suitable for remote
> * control.
> */
> input_enable_softrepeat(input_dev, 250, 125);
>
> Thanks.
Good point, thanks again.
Thanks for the suggestions, I'll send a v2 shortly.
Sean
^ permalink raw reply [flat|nested] 15+ messages in thread
* [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup
2017-10-31 20:07 ` Sean Young
@ 2017-10-31 20:11 ` Sean Young
2017-11-02 23:24 ` Kees Cook
0 siblings, 1 reply; 15+ messages in thread
From: Sean Young @ 2017-10-31 20:11 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mauro Carvalho Chehab, Hans Verkuil, Kees Cook, linux-media,
linux-kernel
Leave the autorepeat handling up to the input layer, and move
to the new timer API.
Compile tested only.
Signed-off-by: Sean Young <sean@mess.org>
---
v2:
- fixes and improvements from Dmitry Torokhov
drivers/media/pci/ttpci/av7110.h | 2 +-
drivers/media/pci/ttpci/av7110_ir.c | 56 +++++++++++++------------------------
2 files changed, 21 insertions(+), 37 deletions(-)
diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
index 347827925c14..bcb72ecbedc0 100644
--- a/drivers/media/pci/ttpci/av7110.h
+++ b/drivers/media/pci/ttpci/av7110.h
@@ -93,7 +93,7 @@ struct infrared {
u8 inversion;
u16 last_key;
u16 last_toggle;
- u8 delay_timer_finished;
+ bool keypressed;
};
diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
index ca05198de2c2..ee414803e6b5 100644
--- a/drivers/media/pci/ttpci/av7110_ir.c
+++ b/drivers/media/pci/ttpci/av7110_ir.c
@@ -84,15 +84,16 @@ 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, t, keyup_timer);
- if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
+ if (!ir || !ir->keypressed)
return;
input_report_key(ir->input_dev, ir->last_key, 0);
input_sync(ir->input_dev);
+ ir->keypressed = false;
}
@@ -152,29 +153,18 @@ static void av7110_emit_key(unsigned long parm)
return;
}
- if (timer_pending(&ir->keyup_timer)) {
- del_timer(&ir->keyup_timer);
- if (ir->last_key != keycode || toggle != ir->last_toggle) {
- ir->delay_timer_finished = 0;
- 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) {
- input_event(ir->input_dev, EV_KEY, keycode, 2);
- input_sync(ir->input_dev);
- }
- } else {
- ir->delay_timer_finished = 0;
- input_event(ir->input_dev, EV_KEY, keycode, 1);
- input_sync(ir->input_dev);
- }
+ if (ir->keypressed &&
+ (ir->last_key != keycode || toggle != ir->last_toggle))
+ 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);
+
+ ir->keypressed = true;
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);
}
@@ -204,16 +194,6 @@ static void input_register_keys(struct infrared *ir)
ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
}
-
-/* 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 +313,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)
@@ -365,8 +344,13 @@ int av7110_ir_init(struct av7110 *av7110)
input_free_device(input_dev);
return err;
}
- input_dev->timer.function = input_repeat_key;
- input_dev->timer.data = (unsigned long) &av7110->ir;
+
+ /*
+ * Input core's default autorepeat is 33 cps with 250 msec
+ * delay, let's adjust to numbers more suitable for remote
+ * control.
+ */
+ input_enable_softrepeat(input_dev, 250, 125);
if (av_cnt == 1) {
e = proc_create("av7110_ir", S_IWUSR, NULL, &av7110_ir_proc_fops);
--
2.13.6
^ permalink raw reply related [flat|nested] 15+ messages in thread
* Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup
2017-10-31 20:11 ` [PATCH v2] " Sean Young
@ 2017-11-02 23:24 ` Kees Cook
2017-11-02 23:50 ` Dmitry Torokhov
0 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2017-11-02 23:24 UTC (permalink / raw)
To: Sean Young
Cc: Dmitry Torokhov, Mauro Carvalho Chehab, Hans Verkuil,
linux-media, LKML, Thomas Gleixner
On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <sean@mess.org> wrote:
> Leave the autorepeat handling up to the input layer, and move
> to the new timer API.
>
> Compile tested only.
>
> Signed-off-by: Sean Young <sean@mess.org>
Hi! Just checking up on this... the input timer conversion is blocked
by getting this sorted out, so I'd love to have something either
media, input, or timer tree can carry. :)
Thanks!
-Kees
> ---
> v2:
> - fixes and improvements from Dmitry Torokhov
>
> drivers/media/pci/ttpci/av7110.h | 2 +-
> drivers/media/pci/ttpci/av7110_ir.c | 56 +++++++++++++------------------------
> 2 files changed, 21 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> index 347827925c14..bcb72ecbedc0 100644
> --- a/drivers/media/pci/ttpci/av7110.h
> +++ b/drivers/media/pci/ttpci/av7110.h
> @@ -93,7 +93,7 @@ struct infrared {
> u8 inversion;
> u16 last_key;
> u16 last_toggle;
> - u8 delay_timer_finished;
> + bool keypressed;
> };
>
>
> diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> index ca05198de2c2..ee414803e6b5 100644
> --- a/drivers/media/pci/ttpci/av7110_ir.c
> +++ b/drivers/media/pci/ttpci/av7110_ir.c
> @@ -84,15 +84,16 @@ 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, t, keyup_timer);
>
> - if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> + if (!ir || !ir->keypressed)
> return;
>
> input_report_key(ir->input_dev, ir->last_key, 0);
> input_sync(ir->input_dev);
> + ir->keypressed = false;
> }
>
>
> @@ -152,29 +153,18 @@ static void av7110_emit_key(unsigned long parm)
> return;
> }
>
> - if (timer_pending(&ir->keyup_timer)) {
> - del_timer(&ir->keyup_timer);
> - if (ir->last_key != keycode || toggle != ir->last_toggle) {
> - ir->delay_timer_finished = 0;
> - 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) {
> - input_event(ir->input_dev, EV_KEY, keycode, 2);
> - input_sync(ir->input_dev);
> - }
> - } else {
> - ir->delay_timer_finished = 0;
> - input_event(ir->input_dev, EV_KEY, keycode, 1);
> - input_sync(ir->input_dev);
> - }
> + if (ir->keypressed &&
> + (ir->last_key != keycode || toggle != ir->last_toggle))
> + 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);
> +
> + ir->keypressed = true;
> 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);
> }
>
>
> @@ -204,16 +194,6 @@ static void input_register_keys(struct infrared *ir)
> ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
> }
>
> -
> -/* 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 +313,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)
> @@ -365,8 +344,13 @@ int av7110_ir_init(struct av7110 *av7110)
> input_free_device(input_dev);
> return err;
> }
> - input_dev->timer.function = input_repeat_key;
> - input_dev->timer.data = (unsigned long) &av7110->ir;
> +
> + /*
> + * Input core's default autorepeat is 33 cps with 250 msec
> + * delay, let's adjust to numbers more suitable for remote
> + * control.
> + */
> + input_enable_softrepeat(input_dev, 250, 125);
>
> if (av_cnt == 1) {
> e = proc_create("av7110_ir", S_IWUSR, NULL, &av7110_ir_proc_fops);
> --
> 2.13.6
>
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup
2017-11-02 23:24 ` Kees Cook
@ 2017-11-02 23:50 ` Dmitry Torokhov
2017-11-03 0:16 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2017-11-02 23:50 UTC (permalink / raw)
To: Kees Cook, Mauro Carvalho Chehab
Cc: Sean Young, Hans Verkuil, linux-media, LKML, Thomas Gleixner
On Thu, Nov 02, 2017 at 04:24:27PM -0700, Kees Cook wrote:
> On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <sean@mess.org> wrote:
> > Leave the autorepeat handling up to the input layer, and move
> > to the new timer API.
> >
> > Compile tested only.
> >
> > Signed-off-by: Sean Young <sean@mess.org>
>
> Hi! Just checking up on this... the input timer conversion is blocked
> by getting this sorted out, so I'd love to have something either
> media, input, or timer tree can carry. :)
Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>From my POV the patch is good. Mauro, do you want me to take it through
my tree, or maybe you could create an immutable branch off 4.14-rc5 (or
6) with this commit and I will pull it in and then can apply Kees input
core conversion patch?
Thanks!
>
> Thanks!
>
> -Kees
>
> > ---
> > v2:
> > - fixes and improvements from Dmitry Torokhov
> >
> > drivers/media/pci/ttpci/av7110.h | 2 +-
> > drivers/media/pci/ttpci/av7110_ir.c | 56 +++++++++++++------------------------
> > 2 files changed, 21 insertions(+), 37 deletions(-)
> >
> > diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> > index 347827925c14..bcb72ecbedc0 100644
> > --- a/drivers/media/pci/ttpci/av7110.h
> > +++ b/drivers/media/pci/ttpci/av7110.h
> > @@ -93,7 +93,7 @@ struct infrared {
> > u8 inversion;
> > u16 last_key;
> > u16 last_toggle;
> > - u8 delay_timer_finished;
> > + bool keypressed;
> > };
> >
> >
> > diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> > index ca05198de2c2..ee414803e6b5 100644
> > --- a/drivers/media/pci/ttpci/av7110_ir.c
> > +++ b/drivers/media/pci/ttpci/av7110_ir.c
> > @@ -84,15 +84,16 @@ 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, t, keyup_timer);
> >
> > - if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> > + if (!ir || !ir->keypressed)
> > return;
> >
> > input_report_key(ir->input_dev, ir->last_key, 0);
> > input_sync(ir->input_dev);
> > + ir->keypressed = false;
> > }
> >
> >
> > @@ -152,29 +153,18 @@ static void av7110_emit_key(unsigned long parm)
> > return;
> > }
> >
> > - if (timer_pending(&ir->keyup_timer)) {
> > - del_timer(&ir->keyup_timer);
> > - if (ir->last_key != keycode || toggle != ir->last_toggle) {
> > - ir->delay_timer_finished = 0;
> > - 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) {
> > - input_event(ir->input_dev, EV_KEY, keycode, 2);
> > - input_sync(ir->input_dev);
> > - }
> > - } else {
> > - ir->delay_timer_finished = 0;
> > - input_event(ir->input_dev, EV_KEY, keycode, 1);
> > - input_sync(ir->input_dev);
> > - }
> > + if (ir->keypressed &&
> > + (ir->last_key != keycode || toggle != ir->last_toggle))
> > + 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);
> > +
> > + ir->keypressed = true;
> > 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);
> > }
> >
> >
> > @@ -204,16 +194,6 @@ static void input_register_keys(struct infrared *ir)
> > ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
> > }
> >
> > -
> > -/* 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 +313,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)
> > @@ -365,8 +344,13 @@ int av7110_ir_init(struct av7110 *av7110)
> > input_free_device(input_dev);
> > return err;
> > }
> > - input_dev->timer.function = input_repeat_key;
> > - input_dev->timer.data = (unsigned long) &av7110->ir;
> > +
> > + /*
> > + * Input core's default autorepeat is 33 cps with 250 msec
> > + * delay, let's adjust to numbers more suitable for remote
> > + * control.
> > + */
> > + input_enable_softrepeat(input_dev, 250, 125);
> >
> > if (av_cnt == 1) {
> > e = proc_create("av7110_ir", S_IWUSR, NULL, &av7110_ir_proc_fops);
> > --
> > 2.13.6
> >
>
>
>
> --
> Kees Cook
> Pixel Security
--
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup
2017-11-02 23:50 ` Dmitry Torokhov
@ 2017-11-03 0:16 ` Mauro Carvalho Chehab
2017-11-03 22:17 ` Dmitry Torokhov
0 siblings, 1 reply; 15+ messages in thread
From: Mauro Carvalho Chehab @ 2017-11-03 0:16 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Kees Cook, Mauro Carvalho Chehab, Sean Young, Hans Verkuil,
linux-media, LKML, Thomas Gleixner
Em Thu, 2 Nov 2017 16:50:37 -0700
Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
> On Thu, Nov 02, 2017 at 04:24:27PM -0700, Kees Cook wrote:
> > On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <sean@mess.org> wrote:
> > > Leave the autorepeat handling up to the input layer, and move
> > > to the new timer API.
> > >
> > > Compile tested only.
> > >
> > > Signed-off-by: Sean Young <sean@mess.org>
> >
> > Hi! Just checking up on this... the input timer conversion is blocked
> > by getting this sorted out, so I'd love to have something either
> > media, input, or timer tree can carry. :)
>
> Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>
> From my POV the patch is good. Mauro, do you want me to take it through
> my tree, or maybe you could create an immutable branch off 4.14-rc5 (or
> 6) with this commit and I will pull it in and then can apply Kees input
> core conversion patch?
Feel free to apply it into your tree with my ack:
Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Regards,
Mauro
>
> Thanks!
>
> >
> > Thanks!
> >
> > -Kees
> >
> > > ---
> > > v2:
> > > - fixes and improvements from Dmitry Torokhov
> > >
> > > drivers/media/pci/ttpci/av7110.h | 2 +-
> > > drivers/media/pci/ttpci/av7110_ir.c | 56 +++++++++++++------------------------
> > > 2 files changed, 21 insertions(+), 37 deletions(-)
> > >
> > > diff --git a/drivers/media/pci/ttpci/av7110.h b/drivers/media/pci/ttpci/av7110.h
> > > index 347827925c14..bcb72ecbedc0 100644
> > > --- a/drivers/media/pci/ttpci/av7110.h
> > > +++ b/drivers/media/pci/ttpci/av7110.h
> > > @@ -93,7 +93,7 @@ struct infrared {
> > > u8 inversion;
> > > u16 last_key;
> > > u16 last_toggle;
> > > - u8 delay_timer_finished;
> > > + bool keypressed;
> > > };
> > >
> > >
> > > diff --git a/drivers/media/pci/ttpci/av7110_ir.c b/drivers/media/pci/ttpci/av7110_ir.c
> > > index ca05198de2c2..ee414803e6b5 100644
> > > --- a/drivers/media/pci/ttpci/av7110_ir.c
> > > +++ b/drivers/media/pci/ttpci/av7110_ir.c
> > > @@ -84,15 +84,16 @@ 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, t, keyup_timer);
> > >
> > > - if (!ir || !test_bit(ir->last_key, ir->input_dev->key))
> > > + if (!ir || !ir->keypressed)
> > > return;
> > >
> > > input_report_key(ir->input_dev, ir->last_key, 0);
> > > input_sync(ir->input_dev);
> > > + ir->keypressed = false;
> > > }
> > >
> > >
> > > @@ -152,29 +153,18 @@ static void av7110_emit_key(unsigned long parm)
> > > return;
> > > }
> > >
> > > - if (timer_pending(&ir->keyup_timer)) {
> > > - del_timer(&ir->keyup_timer);
> > > - if (ir->last_key != keycode || toggle != ir->last_toggle) {
> > > - ir->delay_timer_finished = 0;
> > > - 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) {
> > > - input_event(ir->input_dev, EV_KEY, keycode, 2);
> > > - input_sync(ir->input_dev);
> > > - }
> > > - } else {
> > > - ir->delay_timer_finished = 0;
> > > - input_event(ir->input_dev, EV_KEY, keycode, 1);
> > > - input_sync(ir->input_dev);
> > > - }
> > > + if (ir->keypressed &&
> > > + (ir->last_key != keycode || toggle != ir->last_toggle))
> > > + 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);
> > > +
> > > + ir->keypressed = true;
> > > 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);
> > > }
> > >
> > >
> > > @@ -204,16 +194,6 @@ static void input_register_keys(struct infrared *ir)
> > > ir->input_dev->keycodemax = ARRAY_SIZE(ir->key_map);
> > > }
> > >
> > > -
> > > -/* 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 +313,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)
> > > @@ -365,8 +344,13 @@ int av7110_ir_init(struct av7110 *av7110)
> > > input_free_device(input_dev);
> > > return err;
> > > }
> > > - input_dev->timer.function = input_repeat_key;
> > > - input_dev->timer.data = (unsigned long) &av7110->ir;
> > > +
> > > + /*
> > > + * Input core's default autorepeat is 33 cps with 250 msec
> > > + * delay, let's adjust to numbers more suitable for remote
> > > + * control.
> > > + */
> > > + input_enable_softrepeat(input_dev, 250, 125);
> > >
> > > if (av_cnt == 1) {
> > > e = proc_create("av7110_ir", S_IWUSR, NULL, &av7110_ir_proc_fops);
> > > --
> > > 2.13.6
> > >
> >
> >
> >
> > --
> > Kees Cook
> > Pixel Security
>
Thanks,
Mauro
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup
2017-11-03 0:16 ` Mauro Carvalho Chehab
@ 2017-11-03 22:17 ` Dmitry Torokhov
2017-11-03 23:42 ` Kees Cook
0 siblings, 1 reply; 15+ messages in thread
From: Dmitry Torokhov @ 2017-11-03 22:17 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Kees Cook, Mauro Carvalho Chehab, Sean Young, Hans Verkuil,
linux-media, LKML, Thomas Gleixner
On Thu, Nov 02, 2017 at 10:16:58PM -0200, Mauro Carvalho Chehab wrote:
> Em Thu, 2 Nov 2017 16:50:37 -0700
> Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
>
> > On Thu, Nov 02, 2017 at 04:24:27PM -0700, Kees Cook wrote:
> > > On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <sean@mess.org> wrote:
> > > > Leave the autorepeat handling up to the input layer, and move
> > > > to the new timer API.
> > > >
> > > > Compile tested only.
> > > >
> > > > Signed-off-by: Sean Young <sean@mess.org>
> > >
> > > Hi! Just checking up on this... the input timer conversion is blocked
> > > by getting this sorted out, so I'd love to have something either
> > > media, input, or timer tree can carry. :)
> >
> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> >
> > From my POV the patch is good. Mauro, do you want me to take it through
> > my tree, or maybe you could create an immutable branch off 4.14-rc5 (or
> > 6) with this commit and I will pull it in and then can apply Kees input
> > core conversion patch?
>
> Feel free to apply it into your tree with my ack:
>
> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
Applied and pulled Kees' patch to the input core (dropping the timer_data
business) on top.
Thanks.
--
Dmitry
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup
2017-11-03 22:17 ` Dmitry Torokhov
@ 2017-11-03 23:42 ` Kees Cook
0 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2017-11-03 23:42 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: Mauro Carvalho Chehab, Mauro Carvalho Chehab, Sean Young,
Hans Verkuil, linux-media, LKML, Thomas Gleixner
On Fri, Nov 3, 2017 at 3:17 PM, Dmitry Torokhov
<dmitry.torokhov@gmail.com> wrote:
> On Thu, Nov 02, 2017 at 10:16:58PM -0200, Mauro Carvalho Chehab wrote:
>> Em Thu, 2 Nov 2017 16:50:37 -0700
>> Dmitry Torokhov <dmitry.torokhov@gmail.com> escreveu:
>>
>> > On Thu, Nov 02, 2017 at 04:24:27PM -0700, Kees Cook wrote:
>> > > On Tue, Oct 31, 2017 at 1:11 PM, Sean Young <sean@mess.org> wrote:
>> > > > Leave the autorepeat handling up to the input layer, and move
>> > > > to the new timer API.
>> > > >
>> > > > Compile tested only.
>> > > >
>> > > > Signed-off-by: Sean Young <sean@mess.org>
>> > >
>> > > Hi! Just checking up on this... the input timer conversion is blocked
>> > > by getting this sorted out, so I'd love to have something either
>> > > media, input, or timer tree can carry. :)
>> >
>> > Acked-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>> >
>> > From my POV the patch is good. Mauro, do you want me to take it through
>> > my tree, or maybe you could create an immutable branch off 4.14-rc5 (or
>> > 6) with this commit and I will pull it in and then can apply Kees input
>> > core conversion patch?
>>
>> Feel free to apply it into your tree with my ack:
>>
>> Acked-by: Mauro Carvalho Chehab <mchehab@s-opensource.com>
>
> Applied and pulled Kees' patch to the input core (dropping the timer_data
> business) on top.
Awesome, thanks! :)
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 15+ messages in thread
end of thread, other threads:[~2017-11-03 23:42 UTC | newest]
Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-10-25 0:40 [PATCH] media: av7110: switch to useing timer_setup() Dmitry Torokhov
2017-10-25 1:03 ` Jaejoong Kim
2017-10-26 18:27 ` kbuild test robot
2017-10-30 22:14 ` Kees Cook
2017-10-31 17:27 ` Sean Young
2017-10-31 17:45 ` [PATCH] media: ttpci: remove autorepeat handling and use timer_setup Sean Young
2017-10-31 18:22 ` Dmitry Torokhov
2017-10-31 20:07 ` Sean Young
2017-10-31 20:11 ` [PATCH v2] " Sean Young
2017-11-02 23:24 ` Kees Cook
2017-11-02 23:50 ` Dmitry Torokhov
2017-11-03 0:16 ` Mauro Carvalho Chehab
2017-11-03 22:17 ` Dmitry Torokhov
2017-11-03 23:42 ` Kees Cook
2017-10-31 18:14 ` [PATCH] media: av7110: switch to useing timer_setup() Mauro Carvalho Chehab
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).