[PATCH] media: av7110: switch to useing timer_setup()

[PATCH] media: av7110: switch to useing timer_setup()

[Dmitry Torokhov]

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

Re: [PATCH] media: av7110: switch to useing timer_setup()

[Jaejoong Kim]

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

Re: [PATCH] media: av7110: switch to useing timer_setup()

[kbuild test robot]

[-- 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 --]

Re: [PATCH] media: av7110: switch to useing timer_setup()

[Kees Cook]

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

Re: [PATCH] media: av7110: switch to useing timer_setup()

[Sean Young]

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

[PATCH] media: ttpci: remove autorepeat handling and use timer_setup

[Sean Young]

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

Re: [PATCH] media: ttpci: remove autorepeat handling and use timer_setup

[Dmitry Torokhov]

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

Re: [PATCH] media: ttpci: remove autorepeat handling and use timer_setup

[Sean Young]

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

[PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Sean Young]

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

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Kees Cook]

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

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Dmitry Torokhov]

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

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Mauro Carvalho Chehab]

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

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Dmitry Torokhov]

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

Re: [PATCH v2] media: ttpci: remove autorepeat handling and use timer_setup

[Kees Cook]

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

Re: [PATCH] media: av7110: switch to useing timer_setup()

[Mauro Carvalho Chehab]

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