[PATCH 0/7] rc: don't poke around in rc_dev internals

[PATCH 0/7] rc: don't poke around in rc_dev internals

[David Härdeman]

The following patch series fixes up various drivers which go
poking around in struct rc_dev for no good reason.

---

David Härdeman (7):
      rc-core: ati_remote - leave the internals of rc_dev alone
      rc-core: img-ir - leave the internals of rc_dev alone
      rc-core: img-nec-decoder - leave the internals of rc_dev alone
      rc-core: sanyo - leave the internals of rc_dev alone
      rc-core: ir-raw - leave the internals of rc_dev alone
      rc-core: cx231xx - leave the internals of rc_dev alone
      rc-core: tm6000 - leave the internals of rc_dev alone


 drivers/media/rc/ati_remote.c             |    3 ---
 drivers/media/rc/img-ir/img-ir-hw.c       |    4 ----
 drivers/media/rc/ir-nec-decoder.c         |   10 +++-------
 drivers/media/rc/ir-sanyo-decoder.c       |   10 +++-------
 drivers/media/rc/rc-ir-raw.c              |    4 +---
 drivers/media/usb/cx231xx/cx231xx-input.c |    5 ++---
 drivers/media/usb/tm6000/tm6000-input.c   |    4 ----
 7 files changed, 9 insertions(+), 31 deletions(-)

--
David Härdeman

[PATCH 1/7] rc-core: ati_remote - leave the internals of rc_dev alone

[David Härdeman]

The REP_DELAY setting on the input device is independent of hardware. This
change should not change how to driver works (as it does a keydown/keyup and
has no real repeat handling).

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ati_remote.c |    3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/media/rc/ati_remote.c b/drivers/media/rc/ati_remote.c
index 9cf3e69de16a..a4c6ad4f67c1 100644
--- a/drivers/media/rc/ati_remote.c
+++ b/drivers/media/rc/ati_remote.c
@@ -904,9 +904,6 @@ static int ati_remote_probe(struct usb_interface *interface,
 	if (err)
 		goto exit_kill_urbs;
 
-	/* use our delay for rc_dev */
-	ati_remote->rdev->input_dev->rep[REP_DELAY] = repeat_delay;
-
 	/* Set up and register mouse input device */
 	if (mouse) {
 		input_dev = input_allocate_device();

[PATCH 2/7] rc-core: img-ir - leave the internals of rc_dev alone

[David Härdeman]

Changing the protocol does not imply that the keymap changes.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/img-ir/img-ir-hw.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/rc/img-ir/img-ir-hw.c b/drivers/media/rc/img-ir/img-ir-hw.c
index 431d33b36fb0..8d1439622533 100644
--- a/drivers/media/rc/img-ir/img-ir-hw.c
+++ b/drivers/media/rc/img-ir/img-ir-hw.c
@@ -702,10 +702,6 @@ static void img_ir_set_protocol(struct img_ir_priv *priv, u64 proto)
 {
 	struct rc_dev *rdev = priv->hw.rdev;
 
-	spin_lock_irq(&rdev->rc_map.lock);
-	rdev->rc_map.rc_type = __ffs64(proto);
-	spin_unlock_irq(&rdev->rc_map.lock);
-
 	mutex_lock(&rdev->lock);
 	rdev->enabled_protocols = proto;
 	rdev->allowed_wakeup_protocols = proto;

[PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[David Härdeman]

Obvious fix, leave repeat handling to rc-core

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ir-nec-decoder.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
index 3ce850314dca..75b9137f6faf 100644
--- a/drivers/media/rc/ir-nec-decoder.c
+++ b/drivers/media/rc/ir-nec-decoder.c
@@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			data->state = STATE_BIT_PULSE;
 			return 0;
 		} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 2)) {
-			if (!dev->keypressed) {
-				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
-			} else {
-				rc_repeat(dev);
-				IR_dprintk(1, "Repeat last key\n");
-				data->state = STATE_TRAILER_PULSE;
-			}
+			rc_repeat(dev);
+			IR_dprintk(1, "Repeat last key\n");
+			data->state = STATE_TRAILER_PULSE;
 			return 0;
 		}
 

[PATCH 4/7] rc-core: sanyo - leave the internals of rc_dev alone

[David Härdeman]

Leave repeat handling to rc-core.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/ir-sanyo-decoder.c |   10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/drivers/media/rc/ir-sanyo-decoder.c b/drivers/media/rc/ir-sanyo-decoder.c
index 520bb77dcb62..e6a906a34f90 100644
--- a/drivers/media/rc/ir-sanyo-decoder.c
+++ b/drivers/media/rc/ir-sanyo-decoder.c
@@ -110,13 +110,9 @@ static int ir_sanyo_decode(struct rc_dev *dev, struct ir_raw_event ev)
 			break;
 
 		if (!data->count && geq_margin(ev.duration, SANYO_REPEAT_SPACE, SANYO_UNIT / 2)) {
-			if (!dev->keypressed) {
-				IR_dprintk(1, "SANYO discarding last key repeat: event after key up\n");
-			} else {
-				rc_repeat(dev);
-				IR_dprintk(1, "SANYO repeat last key\n");
-				data->state = STATE_INACTIVE;
-			}
+			rc_repeat(dev);
+			IR_dprintk(1, "SANYO repeat last key\n");
+			data->state = STATE_INACTIVE;
 			return 0;
 		}
 

[PATCH 5/7] rc-core: ir-raw - leave the internals of rc_dev alone

[David Härdeman]

Replace the REP_DELAY value with a static value, which makes more sense.
Automatic repeat handling in the input layer has no relevance for the drivers
idea of "a long time".

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/rc/rc-ir-raw.c |    4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index ae7785c4fbe7..967ab9531e0a 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -102,20 +102,18 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type)
 	s64			delta; /* ns */
 	DEFINE_IR_RAW_EVENT(ev);
 	int			rc = 0;
-	int			delay;
 
 	if (!dev->raw)
 		return -EINVAL;
 
 	now = ktime_get();
 	delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
-	delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
 
 	/* Check for a long duration since last event or if we're
 	 * being called for the first time, note that delta can't
 	 * possibly be negative.
 	 */
-	if (delta > delay || !dev->raw->last_type)
+	if (delta > MS_TO_NS(500) || !dev->raw->last_type)
 		type |= IR_START_EVENT;
 	else
 		ev.duration = delta;

[PATCH 6/7] rc-core: cx231xx - leave the internals of rc_dev alone

[David Härdeman]

Just some debug statements to change.

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/usb/cx231xx/cx231xx-input.c |    5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/media/usb/cx231xx/cx231xx-input.c b/drivers/media/usb/cx231xx/cx231xx-input.c
index 6e80f3c573f3..eecf074b0a48 100644
--- a/drivers/media/usb/cx231xx/cx231xx-input.c
+++ b/drivers/media/usb/cx231xx/cx231xx-input.c
@@ -30,7 +30,7 @@ static int get_key_isdbt(struct IR_i2c *ir, enum rc_type *protocol,
 	int	rc;
 	u8	cmd, scancode;
 
-	dev_dbg(&ir->rc->input_dev->dev, "%s\n", __func__);
+	dev_dbg(&ir->rc->dev, "%s\n", __func__);
 
 		/* poll IR chip */
 	rc = i2c_master_recv(ir->c, &cmd, 1);
@@ -48,8 +48,7 @@ static int get_key_isdbt(struct IR_i2c *ir, enum rc_type *protocol,
 
 	scancode = bitrev8(cmd);
 
-	dev_dbg(&ir->rc->input_dev->dev, "cmd %02x, scan = %02x\n",
-		cmd, scancode);
+	dev_dbg(&ir->rc->dev, "cmd %02x, scan = %02x\n", cmd, scancode);
 
 	*protocol = RC_TYPE_OTHER;
 	*pscancode = scancode;

[PATCH 7/7] rc-core: tm6000 - leave the internals of rc_dev alone

[David Härdeman]

Not sure what the driver is trying to do, however, IR handling seems incomplete
ATM so deleting the offending parts shouldn't affect functionality

Signed-off-by: David Härdeman <david@hardeman.nu>
---
 drivers/media/usb/tm6000/tm6000-input.c |    4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/usb/tm6000/tm6000-input.c b/drivers/media/usb/tm6000/tm6000-input.c
index 39c15bb2b20c..1a033f57fcc1 100644
--- a/drivers/media/usb/tm6000/tm6000-input.c
+++ b/drivers/media/usb/tm6000/tm6000-input.c
@@ -63,7 +63,6 @@ struct tm6000_IR {
 	u8			wait:1;
 	u8			pwled:2;
 	u8			submit_urb:1;
-	u16			key_addr;
 	struct urb		*int_urb;
 
 	/* IR device properties */
@@ -321,9 +320,6 @@ static int tm6000_ir_change_protocol(struct rc_dev *rc, u64 *rc_type)
 
 	dprintk(2, "%s\n",__func__);
 
-	if ((rc->rc_map.scan) && (*rc_type == RC_BIT_NEC))
-		ir->key_addr = ((rc->rc_map.scan[0].scancode >> 8) & 0xffff);
-
 	ir->rc_type = *rc_type;
 
 	tm6000_ir_config(ir);

Re: [PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[Sean Young]

On Mon, May 01, 2017 at 06:10:06PM +0200, David Härdeman wrote:
> Obvious fix, leave repeat handling to rc-core
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/ir-nec-decoder.c |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
> index 3ce850314dca..75b9137f6faf 100644
> --- a/drivers/media/rc/ir-nec-decoder.c
> +++ b/drivers/media/rc/ir-nec-decoder.c
> @@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  			data->state = STATE_BIT_PULSE;
>  			return 0;
>  		} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 2)) {
> -			if (!dev->keypressed) {
> -				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
> -			} else {
> -				rc_repeat(dev);
> -				IR_dprintk(1, "Repeat last key\n");
> -				data->state = STATE_TRAILER_PULSE;
> -			}
> +			rc_repeat(dev);
> +			IR_dprintk(1, "Repeat last key\n");
> +			data->state = STATE_TRAILER_PULSE;

This is not correct. This means that whenever a nec repeat is received,
the last scancode is sent to the input device, irrespective of whether
there has been no IR for hours. The original code is stricter.

>  			return 0;
>  		}
>  

Re: [PATCH 4/7] rc-core: sanyo - leave the internals of rc_dev alone

[Sean Young]

On Mon, May 01, 2017 at 06:10:12PM +0200, David Härdeman wrote:
> Leave repeat handling to rc-core.
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/ir-sanyo-decoder.c |   10 +++-------
>  1 file changed, 3 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/rc/ir-sanyo-decoder.c b/drivers/media/rc/ir-sanyo-decoder.c
> index 520bb77dcb62..e6a906a34f90 100644
> --- a/drivers/media/rc/ir-sanyo-decoder.c
> +++ b/drivers/media/rc/ir-sanyo-decoder.c
> @@ -110,13 +110,9 @@ static int ir_sanyo_decode(struct rc_dev *dev, struct ir_raw_event ev)
>  			break;
>  
>  		if (!data->count && geq_margin(ev.duration, SANYO_REPEAT_SPACE, SANYO_UNIT / 2)) {
> -			if (!dev->keypressed) {
> -				IR_dprintk(1, "SANYO discarding last key repeat: event after key up\n");
> -			} else {
> -				rc_repeat(dev);
> -				IR_dprintk(1, "SANYO repeat last key\n");
> -				data->state = STATE_INACTIVE;
> -			}
> +			rc_repeat(dev);
> +			IR_dprintk(1, "SANYO repeat last key\n");
> +			data->state = STATE_INACTIVE;

Same as the nec decoder: the original code checks whether there has been
a key up event already, so you don't get old scancodes repeated for no
reason.

>  			return 0;
>  		}
>  

Re: [PATCH 5/7] rc-core: ir-raw - leave the internals of rc_dev alone

[Sean Young]

On Mon, May 01, 2017 at 06:10:17PM +0200, David Härdeman wrote:
> Replace the REP_DELAY value with a static value, which makes more sense.
> Automatic repeat handling in the input layer has no relevance for the drivers
> idea of "a long time".
> 
> Signed-off-by: David Härdeman <david@hardeman.nu>
> ---
>  drivers/media/rc/rc-ir-raw.c |    4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index ae7785c4fbe7..967ab9531e0a 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -102,20 +102,18 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type)
>  	s64			delta; /* ns */
>  	DEFINE_IR_RAW_EVENT(ev);
>  	int			rc = 0;
> -	int			delay;
>  
>  	if (!dev->raw)
>  		return -EINVAL;
>  
>  	now = ktime_get();
>  	delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
> -	delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
>  
>  	/* Check for a long duration since last event or if we're
>  	 * being called for the first time, note that delta can't
>  	 * possibly be negative.
>  	 */
> -	if (delta > delay || !dev->raw->last_type)
> +	if (delta > MS_TO_NS(500) || !dev->raw->last_type)
>  		type |= IR_START_EVENT;

So this is just a fail-safe to ensure that the IR decoders are reset after
a period of IR silence. The decoders should reset themselves anyway if they
receive a long space, so it's just belt and braces.

Why is a static value better? At least REP_DELAY can be changed from
user space.

Maybe we should do away with it.


Sean

>  	else
>  		ev.duration = delta;

Re: [PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[David Härdeman]

On Mon, May 22, 2017 at 09:40:30PM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:10:06PM +0200, David Härdeman wrote:
>> Obvious fix, leave repeat handling to rc-core
>> 
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>>  drivers/media/rc/ir-nec-decoder.c |   10 +++-------
>>  1 file changed, 3 insertions(+), 7 deletions(-)
>> 
>> diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
>> index 3ce850314dca..75b9137f6faf 100644
>> --- a/drivers/media/rc/ir-nec-decoder.c
>> +++ b/drivers/media/rc/ir-nec-decoder.c
>> @@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
>>  			data->state = STATE_BIT_PULSE;
>>  			return 0;
>>  		} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 2)) {
>> -			if (!dev->keypressed) {
>> -				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
>> -			} else {
>> -				rc_repeat(dev);
>> -				IR_dprintk(1, "Repeat last key\n");
>> -				data->state = STATE_TRAILER_PULSE;
>> -			}
>> +			rc_repeat(dev);
>> +			IR_dprintk(1, "Repeat last key\n");
>> +			data->state = STATE_TRAILER_PULSE;
>
>This is not correct. This means that whenever a nec repeat is received,
>the last scancode is sent to the input device, irrespective of whether
>there has been no IR for hours. The original code is stricter.

I think that'd be an argument for moving the check to rc_repeat().

But, on the other hand, sending an input scancode for each repeat event
seems kind of pointless, doesn't it? If so, it might make more sense to
just remove the input event generation from rc_repeat() altogether...

-- 
David Härdeman

Re: [PATCH 5/7] rc-core: ir-raw - leave the internals of rc_dev alone

[David Härdeman]

On Tue, May 23, 2017 at 10:20:27AM +0100, Sean Young wrote:
>On Mon, May 01, 2017 at 06:10:17PM +0200, David Härdeman wrote:
>> Replace the REP_DELAY value with a static value, which makes more sense.
>> Automatic repeat handling in the input layer has no relevance for the drivers
>> idea of "a long time".
>> 
>> Signed-off-by: David Härdeman <david@hardeman.nu>
>> ---
>>  drivers/media/rc/rc-ir-raw.c |    4 +---
>>  1 file changed, 1 insertion(+), 3 deletions(-)
>> 
>> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
>> index ae7785c4fbe7..967ab9531e0a 100644
>> --- a/drivers/media/rc/rc-ir-raw.c
>> +++ b/drivers/media/rc/rc-ir-raw.c
>> @@ -102,20 +102,18 @@ int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type)
>>  	s64			delta; /* ns */
>>  	DEFINE_IR_RAW_EVENT(ev);
>>  	int			rc = 0;
>> -	int			delay;
>>  
>>  	if (!dev->raw)
>>  		return -EINVAL;
>>  
>>  	now = ktime_get();
>>  	delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
>> -	delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
>>  
>>  	/* Check for a long duration since last event or if we're
>>  	 * being called for the first time, note that delta can't
>>  	 * possibly be negative.
>>  	 */
>> -	if (delta > delay || !dev->raw->last_type)
>> +	if (delta > MS_TO_NS(500) || !dev->raw->last_type)
>>  		type |= IR_START_EVENT;
>
>So this is just a fail-safe to ensure that the IR decoders are reset after
>a period of IR silence. The decoders should reset themselves anyway if they
>receive a long space, so it's just belt and braces.

Not 100% sure but it also checks for the first call to
ir_raw_event_store_edge()...

>Why is a static value better? At least REP_DELAY can be changed from
>user space.

REP_DELAY serves a completely different purpose. It controls how long a
key should be pressed before the input layer should start generating
soft-repeat events.

The timeout here is related to the IR protocol handling...which is a
quite different matter.

>Maybe we should do away with it.

Maybe...but that'd be a different patch...I think this fix should go in
nevertheless.

-- 
David Härdeman

[PATCH] [media] rc-core: simplify ir_raw_event_store_edge()

[Sean Young]

There is no need to called ir_raw_event_reset() either after a long
space or on startup. Many rc drivers never do this.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/pci/saa7134/saa7134-input.c |  2 +-
 drivers/media/rc/gpio-ir-recv.c           |  6 +++---
 drivers/media/rc/img-ir/img-ir-raw.c      |  4 ++--
 drivers/media/rc/rc-core-priv.h           |  1 -
 drivers/media/rc/rc-ir-raw.c              | 31 +++++--------------------------
 include/media/rc-core.h                   |  9 +--------
 6 files changed, 12 insertions(+), 41 deletions(-)

diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c
index 78849c1..8784bc8 100644
--- a/drivers/media/pci/saa7134/saa7134-input.c
+++ b/drivers/media/pci/saa7134/saa7134-input.c
@@ -1064,7 +1064,7 @@ static int saa7134_raw_decode_irq(struct saa7134_dev *dev)
 	saa_clearb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
 	saa_setb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
 	space = saa_readl(SAA7134_GPIO_GPSTATUS0 >> 2) & ir->mask_keydown;
-	ir_raw_event_store_edge(dev->remote->dev, space ? IR_SPACE : IR_PULSE);
+	ir_raw_event_store_edge(dev->remote->dev, !space);
 
 	/*
 	 * Wait 15 ms from the start of the first IR event before processing
diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
index b4f773b..09889d0 100644
--- a/drivers/media/rc/gpio-ir-recv.c
+++ b/drivers/media/rc/gpio-ir-recv.c
@@ -77,7 +77,7 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 	struct gpio_rc_dev *gpio_dev = dev_id;
 	int gval;
 	int rc = 0;
-	enum raw_event_type type = IR_SPACE;
+	bool pulse = false;
 
 	gval = gpio_get_value(gpio_dev->gpio_nr);
 
@@ -88,9 +88,9 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
 		gval = !gval;
 
 	if (gval == 1)
-		type = IR_PULSE;
+		pulse = true;
 
-	rc = ir_raw_event_store_edge(gpio_dev->rcdev, type);
+	rc = ir_raw_event_store_edge(gpio_dev->rcdev, pulse);
 	if (rc < 0)
 		goto err_get_value;
 
diff --git a/drivers/media/rc/img-ir/img-ir-raw.c b/drivers/media/rc/img-ir/img-ir-raw.c
index 8d2f8e2..ddb7fb4 100644
--- a/drivers/media/rc/img-ir/img-ir-raw.c
+++ b/drivers/media/rc/img-ir/img-ir-raw.c
@@ -40,9 +40,9 @@ static void img_ir_refresh_raw(struct img_ir_priv *priv, u32 irq_status)
 
 	/* report the edge to the IR raw decoders */
 	if (ir_status) /* low */
-		ir_raw_event_store_edge(rc_dev, IR_SPACE);
+		ir_raw_event_store_edge(rc_dev, false);
 	else /* high */
-		ir_raw_event_store_edge(rc_dev, IR_PULSE);
+		ir_raw_event_store_edge(rc_dev, true);
 	ir_raw_event_handle(rc_dev);
 }
 
diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
index 0455b27..d31ad6a 100644
--- a/drivers/media/rc/rc-core-priv.h
+++ b/drivers/media/rc/rc-core-priv.h
@@ -41,7 +41,6 @@ struct ir_raw_event_ctrl {
 	/* fifo for the pulse/space durations */
 	DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
 	ktime_t				last_event;	/* when last event occurred */
-	enum raw_event_type		last_type;	/* last event type */
 	struct rc_dev			*dev;		/* pointer to the parent rc_dev */
 
 	/* raw decoder state follows */
diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 90f66dc..16ef236 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -88,7 +88,7 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store);
 /**
  * ir_raw_event_store_edge() - notify raw ir decoders of the start of a pulse/space
  * @dev:	the struct rc_dev device descriptor
- * @type:	the type of the event that has occurred
+ * @pulse:	true for pulse, false for space
  *
  * This routine (which may be called from an interrupt context) is used to
  * store the beginning of an ir pulse or space (or the start/end of ir
@@ -96,43 +96,22 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store);
  * hardware which does not provide durations directly but only interrupts
  * (or similar events) on state change.
  */
-int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type)
+int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
 {
 	ktime_t			now;
-	s64			delta; /* ns */
 	DEFINE_IR_RAW_EVENT(ev);
 	int			rc = 0;
-	int			delay;
 
 	if (!dev->raw)
 		return -EINVAL;
 
 	now = ktime_get();
-	delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
-	delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
+	ev.duration = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
+	ev.pulse = pulse;
 
-	/* Check for a long duration since last event or if we're
-	 * being called for the first time, note that delta can't
-	 * possibly be negative.
-	 */
-	if (delta > delay || !dev->raw->last_type)
-		type |= IR_START_EVENT;
-	else
-		ev.duration = delta;
-
-	if (type & IR_START_EVENT)
-		ir_raw_event_reset(dev);
-	else if (dev->raw->last_type & IR_SPACE) {
-		ev.pulse = false;
-		rc = ir_raw_event_store(dev, &ev);
-	} else if (dev->raw->last_type & IR_PULSE) {
-		ev.pulse = true;
-		rc = ir_raw_event_store(dev, &ev);
-	} else
-		return 0;
+	rc = ir_raw_event_store(dev, &ev);
 
 	dev->raw->last_event = now;
-	dev->raw->last_type = type;
 	return rc;
 }
 EXPORT_SYMBOL_GPL(ir_raw_event_store_edge);
diff --git a/include/media/rc-core.h b/include/media/rc-core.h
index 73ddd721..b1466e1 100644
--- a/include/media/rc-core.h
+++ b/include/media/rc-core.h
@@ -275,13 +275,6 @@ u32 rc_g_keycode_from_table(struct rc_dev *dev, u32 scancode);
  * split it later into a separate header.
  */
 
-enum raw_event_type {
-	IR_SPACE        = (1 << 0),
-	IR_PULSE        = (1 << 1),
-	IR_START_EVENT  = (1 << 2),
-	IR_STOP_EVENT   = (1 << 3),
-};
-
 struct ir_raw_event {
 	union {
 		u32             duration;
@@ -310,7 +303,7 @@ static inline void init_ir_raw_event(struct ir_raw_event *ev)
 
 void ir_raw_event_handle(struct rc_dev *dev);
 int ir_raw_event_store(struct rc_dev *dev, struct ir_raw_event *ev);
-int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type);
+int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse);
 int ir_raw_event_store_with_filter(struct rc_dev *dev,
 				struct ir_raw_event *ev);
 void ir_raw_event_set_idle(struct rc_dev *dev, bool idle);
-- 
2.9.4

Re: [PATCH] [media] rc-core: simplify ir_raw_event_store_edge()

[Sean Young]

On Sun, May 28, 2017 at 05:49:27PM +0100, Sean Young wrote:
> There is no need to called ir_raw_event_reset() either after a long
> space or on startup. Many rc drivers never do this.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/pci/saa7134/saa7134-input.c |  2 +-
>  drivers/media/rc/gpio-ir-recv.c           |  6 +++---
>  drivers/media/rc/img-ir/img-ir-raw.c      |  4 ++--
>  drivers/media/rc/rc-core-priv.h           |  1 -
>  drivers/media/rc/rc-ir-raw.c              | 31 +++++--------------------------
>  include/media/rc-core.h                   |  9 +--------
>  6 files changed, 12 insertions(+), 41 deletions(-)
> 
> diff --git a/drivers/media/pci/saa7134/saa7134-input.c b/drivers/media/pci/saa7134/saa7134-input.c
> index 78849c1..8784bc8 100644
> --- a/drivers/media/pci/saa7134/saa7134-input.c
> +++ b/drivers/media/pci/saa7134/saa7134-input.c
> @@ -1064,7 +1064,7 @@ static int saa7134_raw_decode_irq(struct saa7134_dev *dev)
>  	saa_clearb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
>  	saa_setb(SAA7134_GPIO_GPMODE3, SAA7134_GPIO_GPRESCAN);
>  	space = saa_readl(SAA7134_GPIO_GPSTATUS0 >> 2) & ir->mask_keydown;
> -	ir_raw_event_store_edge(dev->remote->dev, space ? IR_SPACE : IR_PULSE);
> +	ir_raw_event_store_edge(dev->remote->dev, !space);
>  
>  	/*
>  	 * Wait 15 ms from the start of the first IR event before processing
> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c
> index b4f773b..09889d0 100644
> --- a/drivers/media/rc/gpio-ir-recv.c
> +++ b/drivers/media/rc/gpio-ir-recv.c
> @@ -77,7 +77,7 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>  	struct gpio_rc_dev *gpio_dev = dev_id;
>  	int gval;
>  	int rc = 0;
> -	enum raw_event_type type = IR_SPACE;
> +	bool pulse = false;
>  
>  	gval = gpio_get_value(gpio_dev->gpio_nr);
>  
> @@ -88,9 +88,9 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id)
>  		gval = !gval;
>  
>  	if (gval == 1)
> -		type = IR_PULSE;
> +		pulse = true;
>  
> -	rc = ir_raw_event_store_edge(gpio_dev->rcdev, type);
> +	rc = ir_raw_event_store_edge(gpio_dev->rcdev, pulse);
>  	if (rc < 0)
>  		goto err_get_value;
>  
> diff --git a/drivers/media/rc/img-ir/img-ir-raw.c b/drivers/media/rc/img-ir/img-ir-raw.c
> index 8d2f8e2..ddb7fb4 100644
> --- a/drivers/media/rc/img-ir/img-ir-raw.c
> +++ b/drivers/media/rc/img-ir/img-ir-raw.c
> @@ -40,9 +40,9 @@ static void img_ir_refresh_raw(struct img_ir_priv *priv, u32 irq_status)
>  
>  	/* report the edge to the IR raw decoders */
>  	if (ir_status) /* low */
> -		ir_raw_event_store_edge(rc_dev, IR_SPACE);
> +		ir_raw_event_store_edge(rc_dev, false);
>  	else /* high */
> -		ir_raw_event_store_edge(rc_dev, IR_PULSE);
> +		ir_raw_event_store_edge(rc_dev, true);
>  	ir_raw_event_handle(rc_dev);
>  }
>  
> diff --git a/drivers/media/rc/rc-core-priv.h b/drivers/media/rc/rc-core-priv.h
> index 0455b27..d31ad6a 100644
> --- a/drivers/media/rc/rc-core-priv.h
> +++ b/drivers/media/rc/rc-core-priv.h
> @@ -41,7 +41,6 @@ struct ir_raw_event_ctrl {
>  	/* fifo for the pulse/space durations */
>  	DECLARE_KFIFO(kfifo, struct ir_raw_event, MAX_IR_EVENT_SIZE);
>  	ktime_t				last_event;	/* when last event occurred */
> -	enum raw_event_type		last_type;	/* last event type */
>  	struct rc_dev			*dev;		/* pointer to the parent rc_dev */
>  
>  	/* raw decoder state follows */
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 90f66dc..16ef236 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -88,7 +88,7 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store);
>  /**
>   * ir_raw_event_store_edge() - notify raw ir decoders of the start of a pulse/space
>   * @dev:	the struct rc_dev device descriptor
> - * @type:	the type of the event that has occurred
> + * @pulse:	true for pulse, false for space
>   *
>   * This routine (which may be called from an interrupt context) is used to
>   * store the beginning of an ir pulse or space (or the start/end of ir
> @@ -96,43 +96,22 @@ EXPORT_SYMBOL_GPL(ir_raw_event_store);
>   * hardware which does not provide durations directly but only interrupts
>   * (or similar events) on state change.
>   */
> -int ir_raw_event_store_edge(struct rc_dev *dev, enum raw_event_type type)
> +int ir_raw_event_store_edge(struct rc_dev *dev, bool pulse)
>  {
>  	ktime_t			now;
> -	s64			delta; /* ns */
>  	DEFINE_IR_RAW_EVENT(ev);
>  	int			rc = 0;
> -	int			delay;
>  
>  	if (!dev->raw)
>  		return -EINVAL;
>  
>  	now = ktime_get();
> -	delta = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
> -	delay = MS_TO_NS(dev->input_dev->rep[REP_DELAY]);
> +	ev.duration = ktime_to_ns(ktime_sub(now, dev->raw->last_event));
> +	ev.pulse = pulse;
>  
> -	/* Check for a long duration since last event or if we're
> -	 * being called for the first time, note that delta can't
> -	 * possibly be negative.
> -	 */
> -	if (delta > delay || !dev->raw->last_type)
> -		type |= IR_START_EVENT;
> -	else
> -		ev.duration = delta;

This change was added in commit 3f5c4c7 ("[media] rc: fix ghost keypresses
with certain hw"), so we can't just remove it. I've found the hardware
mentioned in the commit on ebay and try to figure out what is going on
once I get it.

Thanks
Sean

Re: [PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[Sean Young]

On Sun, May 28, 2017 at 10:28:44AM +0200, David Härdeman wrote:
> On Mon, May 22, 2017 at 09:40:30PM +0100, Sean Young wrote:
> >On Mon, May 01, 2017 at 06:10:06PM +0200, David Härdeman wrote:
> >> Obvious fix, leave repeat handling to rc-core
> >> 
> >> Signed-off-by: David Härdeman <david@hardeman.nu>
> >> ---
> >>  drivers/media/rc/ir-nec-decoder.c |   10 +++-------
> >>  1 file changed, 3 insertions(+), 7 deletions(-)
> >> 
> >> diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
> >> index 3ce850314dca..75b9137f6faf 100644
> >> --- a/drivers/media/rc/ir-nec-decoder.c
> >> +++ b/drivers/media/rc/ir-nec-decoder.c
> >> @@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
> >>  			data->state = STATE_BIT_PULSE;
> >>  			return 0;
> >>  		} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 2)) {
> >> -			if (!dev->keypressed) {
> >> -				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
> >> -			} else {
> >> -				rc_repeat(dev);
> >> -				IR_dprintk(1, "Repeat last key\n");
> >> -				data->state = STATE_TRAILER_PULSE;
> >> -			}
> >> +			rc_repeat(dev);
> >> +			IR_dprintk(1, "Repeat last key\n");
> >> +			data->state = STATE_TRAILER_PULSE;
> >
> >This is not correct. This means that whenever a nec repeat is received,
> >the last scancode is sent to the input device, irrespective of whether
> >there has been no IR for hours. The original code is stricter.
> 
> I think that'd be an argument for moving the check to rc_repeat().

It is.

> But, on the other hand, sending an input scancode for each repeat event
> seems kind of pointless, doesn't it? If so, it might make more sense to
> just remove the input event generation from rc_repeat() altogether...

At least there is something visible in user-space saying that a repeat
has been received.


Sean

Re: [PATCH 3/7] rc-core: img-nec-decoder - leave the internals of rc_dev alone

[David Härdeman]

On Sun, Jun 11, 2017 at 05:02:24PM +0100, Sean Young wrote:
>On Sun, May 28, 2017 at 10:28:44AM +0200, David Härdeman wrote:
>> On Mon, May 22, 2017 at 09:40:30PM +0100, Sean Young wrote:
>> >On Mon, May 01, 2017 at 06:10:06PM +0200, David Härdeman wrote:
>> >> Obvious fix, leave repeat handling to rc-core
>> >> 
>> >> Signed-off-by: David Härdeman <david@hardeman.nu>
>> >> ---
>> >>  drivers/media/rc/ir-nec-decoder.c |   10 +++-------
>> >>  1 file changed, 3 insertions(+), 7 deletions(-)
>> >> 
>> >> diff --git a/drivers/media/rc/ir-nec-decoder.c b/drivers/media/rc/ir-nec-decoder.c
>> >> index 3ce850314dca..75b9137f6faf 100644
>> >> --- a/drivers/media/rc/ir-nec-decoder.c
>> >> +++ b/drivers/media/rc/ir-nec-decoder.c
>> >> @@ -88,13 +88,9 @@ static int ir_nec_decode(struct rc_dev *dev, struct ir_raw_event ev)
>> >>  			data->state = STATE_BIT_PULSE;
>> >>  			return 0;
>> >>  		} else if (eq_margin(ev.duration, NEC_REPEAT_SPACE, NEC_UNIT / 2)) {
>> >> -			if (!dev->keypressed) {
>> >> -				IR_dprintk(1, "Discarding last key repeat: event after key up\n");
>> >> -			} else {
>> >> -				rc_repeat(dev);
>> >> -				IR_dprintk(1, "Repeat last key\n");
>> >> -				data->state = STATE_TRAILER_PULSE;
>> >> -			}
>> >> +			rc_repeat(dev);
>> >> +			IR_dprintk(1, "Repeat last key\n");
>> >> +			data->state = STATE_TRAILER_PULSE;
>> >
>> >This is not correct. This means that whenever a nec repeat is received,
>> >the last scancode is sent to the input device, irrespective of whether
>> >there has been no IR for hours. The original code is stricter.
>> 
>> I think that'd be an argument for moving the check to rc_repeat().
>
>It is.

And I just realised that the check is anyway incorrect since
dev->keylock isn't held when dev->keypressed is checked. rc_repeat() on
the other hand does the right thing, which is another argument for
moving the check.

>> But, on the other hand, sending an input scancode for each repeat event
>> seems kind of pointless, doesn't it? If so, it might make more sense to
>> just remove the input event generation from rc_repeat() altogether...
>
>At least there is something visible in user-space saying that a repeat
>has been received.

Yes, but I'm not sure what the value of that information is? The
MSC_SCAN events are mostly useful to create new keymaps for unknown
controls, additional repeat messages won't really be of any use there.

And generating repeat events has the disadvantage that userspace will be
woken up (how often is protocol dependent, but once every 110ms for NEC,
as an example) repeatedly while a button is pressed.

Anyway, I'll spin a new patch in two parts, one which makes the
behaviour consistent between decoders and one which removes the input
event altogether, then you can decide whether you want to merge the
second patch or not.

-- 
David Härdeman