[PATCH 1/3] media: rc: drivers should produce alternate pulse and space timing events

[PATCH 1/3] media: rc: drivers should produce alternate pulse and space timing events

[Sean Young]

Report an error if this is not the case or any problem with the generated
raw events.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 2e50104ae138..49c56da9bc67 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
 {
 	struct ir_raw_event ev;
 	struct ir_raw_handler *handler;
-	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
+	struct ir_raw_event_ctrl *raw = data;
+	struct rc_dev *dev = raw->dev;
 
 	while (1) {
 		mutex_lock(&ir_raw_handler_lock);
 		while (kfifo_out(&raw->kfifo, &ev, 1)) {
+			if (is_timing_event(ev)) {
+				if (ev.duration == 0)
+					dev_err(&dev->dev, "nonsensical timing event of duration 0");
+				if (is_timing_event(raw->prev_ev) &&
+				    !is_transition(&ev, &raw->prev_ev))
+					dev_err(&dev->dev, "two consecutive events of type %s",
+						TO_STR(ev.pulse));
+				if (raw->prev_ev.reset && ev.pulse == 0)
+					dev_err(&dev->dev, "timing event after reset should be pulse");
+			}
 			list_for_each_entry(handler, &ir_raw_handler_list, list)
-				if (raw->dev->enabled_protocols &
+				if (dev->enabled_protocols &
 				    handler->protocols || !handler->protocols)
-					handler->decode(raw->dev, ev);
-			ir_lirc_raw_event(raw->dev, ev);
+					handler->decode(dev, ev);
+			ir_lirc_raw_event(dev, ev);
 			raw->prev_ev = ev;
 		}
 		mutex_unlock(&ir_raw_handler_lock);
-- 
2.17.0

[PATCH 2/3] media: rc: decoders do not need to check for transitions

[Sean Young]

Drivers should never produce consecutive pulse or space raw events. Should
that occur, we would have bigger problems than this code is trying to
guard against.

Note that we already log an error should a driver misbehave.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/ir-mce_kbd-decoder.c |  6 ------
 drivers/media/rc/ir-rc5-decoder.c     |  3 ---
 drivers/media/rc/ir-rc6-decoder.c     | 10 ----------
 3 files changed, 19 deletions(-)

diff --git a/drivers/media/rc/ir-mce_kbd-decoder.c b/drivers/media/rc/ir-mce_kbd-decoder.c
index 9574c3dd90f2..64ea42927669 100644
--- a/drivers/media/rc/ir-mce_kbd-decoder.c
+++ b/drivers/media/rc/ir-mce_kbd-decoder.c
@@ -274,9 +274,6 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		return 0;
 
 	case STATE_HEADER_BIT_END:
-		if (!is_transition(&ev, &dev->raw->prev_ev))
-			break;
-
 		decrease_duration(&ev, MCIR2_BIT_END);
 
 		if (data->count != MCIR2_HEADER_NBITS) {
@@ -313,9 +310,6 @@ static int ir_mce_kbd_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		return 0;
 
 	case STATE_BODY_BIT_END:
-		if (!is_transition(&ev, &dev->raw->prev_ev))
-			break;
-
 		if (data->count == data->wanted_bits)
 			data->state = STATE_FINISHED;
 		else
diff --git a/drivers/media/rc/ir-rc5-decoder.c b/drivers/media/rc/ir-rc5-decoder.c
index cbfaadbee8fa..63624654a71e 100644
--- a/drivers/media/rc/ir-rc5-decoder.c
+++ b/drivers/media/rc/ir-rc5-decoder.c
@@ -88,9 +88,6 @@ static int ir_rc5_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		return 0;
 
 	case STATE_BIT_END:
-		if (!is_transition(&ev, &dev->raw->prev_ev))
-			break;
-
 		if (data->count == CHECK_RC5X_NBITS)
 			data->state = STATE_CHECK_RC5X;
 		else
diff --git a/drivers/media/rc/ir-rc6-decoder.c b/drivers/media/rc/ir-rc6-decoder.c
index 66e07109f6fc..68487ce9f79b 100644
--- a/drivers/media/rc/ir-rc6-decoder.c
+++ b/drivers/media/rc/ir-rc6-decoder.c
@@ -145,9 +145,6 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		return 0;
 
 	case STATE_HEADER_BIT_END:
-		if (!is_transition(&ev, &dev->raw->prev_ev))
-			break;
-
 		if (data->count == RC6_HEADER_NBITS)
 			data->state = STATE_TOGGLE_START;
 		else
@@ -165,10 +162,6 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		return 0;
 
 	case STATE_TOGGLE_END:
-		if (!is_transition(&ev, &dev->raw->prev_ev) ||
-		    !geq_margin(ev.duration, RC6_TOGGLE_END, RC6_UNIT / 2))
-			break;
-
 		if (!(data->header & RC6_STARTBIT_MASK)) {
 			dev_dbg(&dev->dev, "RC6 invalid start bit\n");
 			break;
@@ -210,9 +203,6 @@ static int ir_rc6_decode(struct rc_dev *dev, struct ir_raw_event ev)
 		break;
 
 	case STATE_BODY_BIT_END:
-		if (!is_transition(&ev, &dev->raw->prev_ev))
-			break;
-
 		if (data->count == data->wanted_bits)
 			data->state = STATE_FINISHED;
 		else
-- 
2.17.0

[PATCH 3/3] media: rc: winbond: do not send reset and timeout raw events on startup

[Sean Young]

ir_raw_event_set_idle() sends a timeout event which is not needed, and
on startup no reset event is needed either.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/winbond-cir.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/rc/winbond-cir.c b/drivers/media/rc/winbond-cir.c
index 0adf0991f5ab..851acba9b436 100644
--- a/drivers/media/rc/winbond-cir.c
+++ b/drivers/media/rc/winbond-cir.c
@@ -989,8 +989,7 @@ wbcir_init_hw(struct wbcir_data *data)
 
 	/* Clear RX state */
 	data->rxstate = WBCIR_RXSTATE_INACTIVE;
-	ir_raw_event_reset(data->dev);
-	ir_raw_event_set_idle(data->dev, true);
+	wbcir_idle_rx(data->dev, true);
 
 	/* Clear TX state */
 	if (data->txstate == WBCIR_TXSTATE_ACTIVE) {
@@ -1009,6 +1008,7 @@ wbcir_resume(struct pnp_dev *device)
 	struct wbcir_data *data = pnp_get_drvdata(device);
 
 	wbcir_init_hw(data);
+	ir_raw_event_reset(data->dev);
 	enable_irq(data->irq);
 	led_classdev_resume(&data->led);
 
-- 
2.17.0

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Jerome Brunet]

On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> Report an error if this is not the case or any problem with the generated
> raw events.

Hi,

Since the inclusion of this patch, every 3 to 15 seconds, I get the following
message: 

 "rc rc0: two consecutive events of type space"

on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
s400.dts). I don't know much about ir protocol and surely there is something
worth investigating in the related driver, but ...

> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
>  1 file changed, 15 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 2e50104ae138..49c56da9bc67 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
>  {
>  	struct ir_raw_event ev;
>  	struct ir_raw_handler *handler;
> -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> +	struct ir_raw_event_ctrl *raw = data;
> +	struct rc_dev *dev = raw->dev;
>  
>  	while (1) {
>  		mutex_lock(&ir_raw_handler_lock);
>  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> +			if (is_timing_event(ev)) {
> +				if (ev.duration == 0)
> +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> +				if (is_timing_event(raw->prev_ev) &&
> +				    !is_transition(&ev, &raw->prev_ev))
> +					dev_err(&dev->dev, "two consecutive events of type %s",
> +						TO_STR(ev.pulse));
> +				if (raw->prev_ev.reset && ev.pulse == 0)
> +					dev_err(&dev->dev, "timing event after reset should be pulse");
> +			}

... considering that we continue the processing as if nothing happened, is it
really an error ? 

Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?

>  			list_for_each_entry(handler, &ir_raw_handler_list, list)
> -				if (raw->dev->enabled_protocols &
> +				if (dev->enabled_protocols &
>  				    handler->protocols || !handler->protocols)
> -					handler->decode(raw->dev, ev);
> -			ir_lirc_raw_event(raw->dev, ev);
> +					handler->decode(dev, ev);
> +			ir_lirc_raw_event(dev, ev);
>  			raw->prev_ev = ev;
>  		}
>  		mutex_unlock(&ir_raw_handler_lock);

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Sean Young]

On Tue, Jun 19, 2018 at 02:08:12PM +0200, Jerome Brunet wrote:
> On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> > Report an error if this is not the case or any problem with the generated
> > raw events.
> 
> Hi,
> 
> Since the inclusion of this patch, every 3 to 15 seconds, I get the following
> message: 
> 
>  "rc rc0: two consecutive events of type space"
> 
> on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
> s400.dts). I don't know much about ir protocol and surely there is something
> worth investigating in the related driver, but ...
> 
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
> >  1 file changed, 15 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> > index 2e50104ae138..49c56da9bc67 100644
> > --- a/drivers/media/rc/rc-ir-raw.c
> > +++ b/drivers/media/rc/rc-ir-raw.c
> > @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
> >  {
> >  	struct ir_raw_event ev;
> >  	struct ir_raw_handler *handler;
> > -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> > +	struct ir_raw_event_ctrl *raw = data;
> > +	struct rc_dev *dev = raw->dev;
> >  
> >  	while (1) {
> >  		mutex_lock(&ir_raw_handler_lock);
> >  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> > +			if (is_timing_event(ev)) {
> > +				if (ev.duration == 0)
> > +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> > +				if (is_timing_event(raw->prev_ev) &&
> > +				    !is_transition(&ev, &raw->prev_ev))
> > +					dev_err(&dev->dev, "two consecutive events of type %s",
> > +						TO_STR(ev.pulse));
> > +				if (raw->prev_ev.reset && ev.pulse == 0)
> > +					dev_err(&dev->dev, "timing event after reset should be pulse");
> > +			}
> 
> ... considering that we continue the processing as if nothing happened, is it
> really an error ? 
> 
> Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?

Maybe it should be dev_warn(). The fact that it is not dev_warn_once() means
that we now know this occurs regularly.

Would you mind testing the following patch please?

Thanks

Sean

>From 6a44fbe4738d230b9cf378777e7e9a93e5fda726 Mon Sep 17 00:00:00 2001
From: Sean Young <sean@mess.org>
Date: Tue, 19 Jun 2018 13:50:36 +0100
Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
 space

The meson generates one edge per interrupt. The duration is encoded in 12
bits of 10 microseconds, so it can only encoding a maximum of 40
milliseconds. As a result, it can produce multiple space events.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/meson-ir.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
index f449b35d25e7..9747426719b2 100644
--- a/drivers/media/rc/meson-ir.c
+++ b/drivers/media/rc/meson-ir.c
@@ -97,7 +97,8 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
 	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
 	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
 
-	ir_raw_event_store_with_timeout(ir->rc, &rawir);
+	if (ir_raw_event_store_with_filter(ir->rc, &rawir))
+		ir_raw_event_handle(ir->rc);
 
 	spin_unlock(&ir->lock);
 
-- 
2.17.1

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Jerome Brunet]

On Tue, 2018-06-19 at 13:57 +0100, Sean Young wrote:
> On Tue, Jun 19, 2018 at 02:08:12PM +0200, Jerome Brunet wrote:
> > On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> > > Report an error if this is not the case or any problem with the generated
> > > raw events.
> > 
> > Hi,
> > 
> > Since the inclusion of this patch, every 3 to 15 seconds, I get the following
> > message: 
> > 
> >  "rc rc0: two consecutive events of type space"
> > 
> > on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
> > s400.dts). I don't know much about ir protocol and surely there is something
> > worth investigating in the related driver, but ...
> > 
> > > 
> > > Signed-off-by: Sean Young <sean@mess.org>
> > > ---
> > >  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
> > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> > > index 2e50104ae138..49c56da9bc67 100644
> > > --- a/drivers/media/rc/rc-ir-raw.c
> > > +++ b/drivers/media/rc/rc-ir-raw.c
> > > @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
> > >  {
> > >  	struct ir_raw_event ev;
> > >  	struct ir_raw_handler *handler;
> > > -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> > > +	struct ir_raw_event_ctrl *raw = data;
> > > +	struct rc_dev *dev = raw->dev;
> > >  
> > >  	while (1) {
> > >  		mutex_lock(&ir_raw_handler_lock);
> > >  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> > > +			if (is_timing_event(ev)) {
> > > +				if (ev.duration == 0)
> > > +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> > > +				if (is_timing_event(raw->prev_ev) &&
> > > +				    !is_transition(&ev, &raw->prev_ev))
> > > +					dev_err(&dev->dev, "two consecutive events of type %s",
> > > +						TO_STR(ev.pulse));
> > > +				if (raw->prev_ev.reset && ev.pulse == 0)
> > > +					dev_err(&dev->dev, "timing event after reset should be pulse");
> > > +			}
> > 
> > ... considering that we continue the processing as if nothing happened, is it
> > really an error ? 
> > 
> > Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?
> 
> Maybe it should be dev_warn(). The fact that it is not dev_warn_once() means
> that we now know this occurs regularly.

It seems weird to report this over and over again.

> 
> Would you mind testing the following patch please?
> 
> Thanks
> 
> Sean
> 
> From 6a44fbe4738d230b9cf378777e7e9a93e5fda726 Mon Sep 17 00:00:00 2001
> From: Sean Young <sean@mess.org>
> Date: Tue, 19 Jun 2018 13:50:36 +0100
> Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
>  space
> 
> The meson generates one edge per interrupt. The duration is encoded in 12
> bits of 10 microseconds, so it can only encoding a maximum of 40
> milliseconds. As a result, it can produce multiple space events.
> 
> Signed-off-by: Sean Young <sean@mess.org>
> ---
>  drivers/media/rc/meson-ir.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> index f449b35d25e7..9747426719b2 100644
> --- a/drivers/media/rc/meson-ir.c
> +++ b/drivers/media/rc/meson-ir.c
> @@ -97,7 +97,8 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
>  	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
>  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
>  
> -	ir_raw_event_store_with_timeout(ir->rc, &rawir);
> +	if (ir_raw_event_store_with_filter(ir->rc, &rawir))
> +		ir_raw_event_handle(ir->rc);
>  
>  	spin_unlock(&ir->lock);
>  

Solve the problem on meson. Thx
Feel free to add this submitting the patch

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

Cheers

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Sean Young]

On Tue, Jun 19, 2018 at 04:09:20PM +0200, Jerome Brunet wrote:
> On Tue, 2018-06-19 at 13:57 +0100, Sean Young wrote:
> > On Tue, Jun 19, 2018 at 02:08:12PM +0200, Jerome Brunet wrote:
> > > On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> > > > Report an error if this is not the case or any problem with the generated
> > > > raw events.
> > > 
> > > Hi,
> > > 
> > > Since the inclusion of this patch, every 3 to 15 seconds, I get the following
> > > message: 
> > > 
> > >  "rc rc0: two consecutive events of type space"
> > > 
> > > on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
> > > s400.dts). I don't know much about ir protocol and surely there is something
> > > worth investigating in the related driver, but ...
> > > 
> > > > 
> > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > ---
> > > >  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
> > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > > 
> > > > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> > > > index 2e50104ae138..49c56da9bc67 100644
> > > > --- a/drivers/media/rc/rc-ir-raw.c
> > > > +++ b/drivers/media/rc/rc-ir-raw.c
> > > > @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
> > > >  {
> > > >  	struct ir_raw_event ev;
> > > >  	struct ir_raw_handler *handler;
> > > > -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> > > > +	struct ir_raw_event_ctrl *raw = data;
> > > > +	struct rc_dev *dev = raw->dev;
> > > >  
> > > >  	while (1) {
> > > >  		mutex_lock(&ir_raw_handler_lock);
> > > >  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> > > > +			if (is_timing_event(ev)) {
> > > > +				if (ev.duration == 0)
> > > > +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> > > > +				if (is_timing_event(raw->prev_ev) &&
> > > > +				    !is_transition(&ev, &raw->prev_ev))
> > > > +					dev_err(&dev->dev, "two consecutive events of type %s",
> > > > +						TO_STR(ev.pulse));
> > > > +				if (raw->prev_ev.reset && ev.pulse == 0)
> > > > +					dev_err(&dev->dev, "timing event after reset should be pulse");
> > > > +			}
> > > 
> > > ... considering that we continue the processing as if nothing happened, is it
> > > really an error ? 
> > > 
> > > Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?
> > 
> > Maybe it should be dev_warn(). The fact that it is not dev_warn_once() means
> > that we now know this occurs regularly.
> 
> It seems weird to report this over and over again.
> 
> > 
> > Would you mind testing the following patch please?
> > 
> > Thanks
> > 
> > Sean
> > 
> > From 6a44fbe4738d230b9cf378777e7e9a93e5fda726 Mon Sep 17 00:00:00 2001
> > From: Sean Young <sean@mess.org>
> > Date: Tue, 19 Jun 2018 13:50:36 +0100
> > Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
> >  space
> > 
> > The meson generates one edge per interrupt. The duration is encoded in 12
> > bits of 10 microseconds, so it can only encoding a maximum of 40
> > milliseconds. As a result, it can produce multiple space events.
> > 
> > Signed-off-by: Sean Young <sean@mess.org>
> > ---
> >  drivers/media/rc/meson-ir.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > index f449b35d25e7..9747426719b2 100644
> > --- a/drivers/media/rc/meson-ir.c
> > +++ b/drivers/media/rc/meson-ir.c
> > @@ -97,7 +97,8 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> >  	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
> >  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> >  
> > -	ir_raw_event_store_with_timeout(ir->rc, &rawir);
> > +	if (ir_raw_event_store_with_filter(ir->rc, &rawir))
> > +		ir_raw_event_handle(ir->rc);
> >  
> >  	spin_unlock(&ir->lock);
> >  
> 
> Solve the problem on meson. Thx
> Feel free to add this submitting the patch

Actually this patch is broken. ir_raw_event_store_with_timeout() generates
IR timeouts, but ir_raw_event_store_with_filter() does not.

So this will need some rethinking. I think we need a 
ir_raw_event_store_with_timeout_with_filter() but that is getting silly now.

Maybe filter should be a boolean property of an rc device and happen
transparently. I'll write something like that when I get some time.


Sean

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Jerome Brunet]

On Tue, 2018-06-26 at 15:37 +0100, Sean Young wrote:
> On Tue, Jun 19, 2018 at 04:09:20PM +0200, Jerome Brunet wrote:
> > On Tue, 2018-06-19 at 13:57 +0100, Sean Young wrote:
> > > On Tue, Jun 19, 2018 at 02:08:12PM +0200, Jerome Brunet wrote:
> > > > On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> > > > > Report an error if this is not the case or any problem with the generated
> > > > > raw events.
> > > > 
> > > > Hi,
> > > > 
> > > > Since the inclusion of this patch, every 3 to 15 seconds, I get the following
> > > > message: 
> > > > 
> > > >  "rc rc0: two consecutive events of type space"
> > > > 
> > > > on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
> > > > s400.dts). I don't know much about ir protocol and surely there is something
> > > > worth investigating in the related driver, but ...
> > > > 
> > > > > 
> > > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > > ---
> > > > >  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
> > > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> > > > > index 2e50104ae138..49c56da9bc67 100644
> > > > > --- a/drivers/media/rc/rc-ir-raw.c
> > > > > +++ b/drivers/media/rc/rc-ir-raw.c
> > > > > @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
> > > > >  {
> > > > >  	struct ir_raw_event ev;
> > > > >  	struct ir_raw_handler *handler;
> > > > > -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> > > > > +	struct ir_raw_event_ctrl *raw = data;
> > > > > +	struct rc_dev *dev = raw->dev;
> > > > >  
> > > > >  	while (1) {
> > > > >  		mutex_lock(&ir_raw_handler_lock);
> > > > >  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> > > > > +			if (is_timing_event(ev)) {
> > > > > +				if (ev.duration == 0)
> > > > > +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> > > > > +				if (is_timing_event(raw->prev_ev) &&
> > > > > +				    !is_transition(&ev, &raw->prev_ev))
> > > > > +					dev_err(&dev->dev, "two consecutive events of type %s",
> > > > > +						TO_STR(ev.pulse));
> > > > > +				if (raw->prev_ev.reset && ev.pulse == 0)
> > > > > +					dev_err(&dev->dev, "timing event after reset should be pulse");
> > > > > +			}
> > > > 
> > > > ... considering that we continue the processing as if nothing happened, is it
> > > > really an error ? 
> > > > 
> > > > Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?
> > > 
> > > Maybe it should be dev_warn(). The fact that it is not dev_warn_once() means
> > > that we now know this occurs regularly.
> > 
> > It seems weird to report this over and over again.
> > 
> > > 
> > > Would you mind testing the following patch please?
> > > 
> > > Thanks
> > > 
> > > Sean
> > > 
> > > From 6a44fbe4738d230b9cf378777e7e9a93e5fda726 Mon Sep 17 00:00:00 2001
> > > From: Sean Young <sean@mess.org>
> > > Date: Tue, 19 Jun 2018 13:50:36 +0100
> > > Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
> > >  space
> > > 
> > > The meson generates one edge per interrupt. The duration is encoded in 12
> > > bits of 10 microseconds, so it can only encoding a maximum of 40
> > > milliseconds. As a result, it can produce multiple space events.
> > > 
> > > Signed-off-by: Sean Young <sean@mess.org>
> > > ---
> > >  drivers/media/rc/meson-ir.c | 3 ++-
> > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > > index f449b35d25e7..9747426719b2 100644
> > > --- a/drivers/media/rc/meson-ir.c
> > > +++ b/drivers/media/rc/meson-ir.c
> > > @@ -97,7 +97,8 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> > >  	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
> > >  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> > >  
> > > -	ir_raw_event_store_with_timeout(ir->rc, &rawir);
> > > +	if (ir_raw_event_store_with_filter(ir->rc, &rawir))
> > > +		ir_raw_event_handle(ir->rc);
> > >  
> > >  	spin_unlock(&ir->lock);
> > >  
> > 
> > Solve the problem on meson. Thx
> > Feel free to add this submitting the patch
> 
> Actually this patch is broken. ir_raw_event_store_with_timeout() generates
> IR timeouts, but ir_raw_event_store_with_filter() does not.
> 
> So this will need some rethinking. I think we need a 
> ir_raw_event_store_with_timeout_with_filter() but that is getting silly now.
> 
> Maybe filter should be a boolean property of an rc device and happen
> transparently. I'll write something like that when I get some time.
> 

would you still object to use dev_warn_once() in the meantime ?
Console flooding is quite annoying.

> 
> Sean

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Sean Young]

On Tue, Jun 26, 2018 at 04:39:51PM +0200, Jerome Brunet wrote:
> On Tue, 2018-06-26 at 15:37 +0100, Sean Young wrote:
> > On Tue, Jun 19, 2018 at 04:09:20PM +0200, Jerome Brunet wrote:
> > > On Tue, 2018-06-19 at 13:57 +0100, Sean Young wrote:
> > > > On Tue, Jun 19, 2018 at 02:08:12PM +0200, Jerome Brunet wrote:
> > > > > On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> > > > > > Report an error if this is not the case or any problem with the generated
> > > > > > raw events.
> > > > > 
> > > > > Hi,
> > > > > 
> > > > > Since the inclusion of this patch, every 3 to 15 seconds, I get the following
> > > > > message: 
> > > > > 
> > > > >  "rc rc0: two consecutive events of type space"
> > > > > 
> > > > > on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
> > > > > s400.dts). I don't know much about ir protocol and surely there is something
> > > > > worth investigating in the related driver, but ...
> > > > > 
> > > > > > 
> > > > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > > > ---
> > > > > >  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
> > > > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > > > > 
> > > > > > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> > > > > > index 2e50104ae138..49c56da9bc67 100644
> > > > > > --- a/drivers/media/rc/rc-ir-raw.c
> > > > > > +++ b/drivers/media/rc/rc-ir-raw.c
> > > > > > @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
> > > > > >  {
> > > > > >  	struct ir_raw_event ev;
> > > > > >  	struct ir_raw_handler *handler;
> > > > > > -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> > > > > > +	struct ir_raw_event_ctrl *raw = data;
> > > > > > +	struct rc_dev *dev = raw->dev;
> > > > > >  
> > > > > >  	while (1) {
> > > > > >  		mutex_lock(&ir_raw_handler_lock);
> > > > > >  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> > > > > > +			if (is_timing_event(ev)) {
> > > > > > +				if (ev.duration == 0)
> > > > > > +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> > > > > > +				if (is_timing_event(raw->prev_ev) &&
> > > > > > +				    !is_transition(&ev, &raw->prev_ev))
> > > > > > +					dev_err(&dev->dev, "two consecutive events of type %s",
> > > > > > +						TO_STR(ev.pulse));
> > > > > > +				if (raw->prev_ev.reset && ev.pulse == 0)
> > > > > > +					dev_err(&dev->dev, "timing event after reset should be pulse");
> > > > > > +			}
> > > > > 
> > > > > ... considering that we continue the processing as if nothing happened, is it
> > > > > really an error ? 
> > > > > 
> > > > > Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?
> > > > 
> > > > Maybe it should be dev_warn(). The fact that it is not dev_warn_once() means
> > > > that we now know this occurs regularly.
> > > 
> > > It seems weird to report this over and over again.
> > > 
> > > > 
> > > > Would you mind testing the following patch please?
> > > > 
> > > > Thanks
> > > > 
> > > > Sean
> > > > 
> > > > From 6a44fbe4738d230b9cf378777e7e9a93e5fda726 Mon Sep 17 00:00:00 2001
> > > > From: Sean Young <sean@mess.org>
> > > > Date: Tue, 19 Jun 2018 13:50:36 +0100
> > > > Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
> > > >  space
> > > > 
> > > > The meson generates one edge per interrupt. The duration is encoded in 12
> > > > bits of 10 microseconds, so it can only encoding a maximum of 40
> > > > milliseconds. As a result, it can produce multiple space events.
> > > > 
> > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > ---
> > > >  drivers/media/rc/meson-ir.c | 3 ++-
> > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > 
> > > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > > > index f449b35d25e7..9747426719b2 100644
> > > > --- a/drivers/media/rc/meson-ir.c
> > > > +++ b/drivers/media/rc/meson-ir.c
> > > > @@ -97,7 +97,8 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> > > >  	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
> > > >  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> > > >  
> > > > -	ir_raw_event_store_with_timeout(ir->rc, &rawir);
> > > > +	if (ir_raw_event_store_with_filter(ir->rc, &rawir))
> > > > +		ir_raw_event_handle(ir->rc);
> > > >  
> > > >  	spin_unlock(&ir->lock);
> > > >  
> > > 
> > > Solve the problem on meson. Thx
> > > Feel free to add this submitting the patch
> > 
> > Actually this patch is broken. ir_raw_event_store_with_timeout() generates
> > IR timeouts, but ir_raw_event_store_with_filter() does not.
> > 
> > So this will need some rethinking. I think we need a 
> > ir_raw_event_store_with_timeout_with_filter() but that is getting silly now.
> > 
> > Maybe filter should be a boolean property of an rc device and happen
> > transparently. I'll write something like that when I get some time.
> > 
> 
> would you still object to use dev_warn_once() in the meantime ?
> Console flooding is quite annoying.

Yes, as much as I dislike it, I think that is the right solution for now.

Thanks,

Sean

>From 2f61a8b6e850e34def282f1cbd1bfee0f5ab66c5 Mon Sep 17 00:00:00 2001
From: Sean Young <sean@mess.org>
Date: Tue, 26 Jun 2018 16:03:18 +0100
Subject: [PATCH] media: rc: be less noisy when driver misbehaves

Since commit 48231f289e52 ("media: rc: drivers should produce alternate
pulse and space timing events"), on meson-ir we are regularly producing
errors. Reduce to warning level and only warn once to avoid flooding
the log.

A proper fix for meson-ir is going to be too large for v4.18.

Signed-off-by: Sean Young <sean@mess.org>
---
 drivers/media/rc/rc-ir-raw.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
index 2e0066b1a31c..e7948908e78c 100644
--- a/drivers/media/rc/rc-ir-raw.c
+++ b/drivers/media/rc/rc-ir-raw.c
@@ -30,13 +30,13 @@ static int ir_raw_event_thread(void *data)
 		while (kfifo_out(&raw->kfifo, &ev, 1)) {
 			if (is_timing_event(ev)) {
 				if (ev.duration == 0)
-					dev_err(&dev->dev, "nonsensical timing event of duration 0");
+					dev_warn_once(&dev->dev, "nonsensical timing event of duration 0");
 				if (is_timing_event(raw->prev_ev) &&
 				    !is_transition(&ev, &raw->prev_ev))
-					dev_err(&dev->dev, "two consecutive events of type %s",
-						TO_STR(ev.pulse));
+					dev_warn_once(&dev->dev, "two consecutive events of type %s",
+						      TO_STR(ev.pulse));
 				if (raw->prev_ev.reset && ev.pulse == 0)
-					dev_err(&dev->dev, "timing event after reset should be pulse");
+					dev_warn_once(&dev->dev, "timing event after reset should be pulse");
 			}
 			list_for_each_entry(handler, &ir_raw_handler_list, list)
 				if (dev->enabled_protocols &
-- 
2.17.1

Re: [1/3] media: rc: drivers should produce alternate pulse and space timing events

[Jerome Brunet]

On Tue, 2018-06-26 at 17:33 +0100, Sean Young wrote:
> On Tue, Jun 26, 2018 at 04:39:51PM +0200, Jerome Brunet wrote:
> > On Tue, 2018-06-26 at 15:37 +0100, Sean Young wrote:
> > > On Tue, Jun 19, 2018 at 04:09:20PM +0200, Jerome Brunet wrote:
> > > > On Tue, 2018-06-19 at 13:57 +0100, Sean Young wrote:
> > > > > On Tue, Jun 19, 2018 at 02:08:12PM +0200, Jerome Brunet wrote:
> > > > > > On Sat, 2018-05-12 at 11:55 +0100, Sean Young wrote:
> > > > > > > Report an error if this is not the case or any problem with the generated
> > > > > > > raw events.
> > > > > > 
> > > > > > Hi,
> > > > > > 
> > > > > > Since the inclusion of this patch, every 3 to 15 seconds, I get the following
> > > > > > message: 
> > > > > > 
> > > > > >  "rc rc0: two consecutive events of type space"
> > > > > > 
> > > > > > on the console of amlogic s400 platform (arch/arm64/boot/dts/amlogic/meson-axg-
> > > > > > s400.dts). I don't know much about ir protocol and surely there is something
> > > > > > worth investigating in the related driver, but ...
> > > > > > 
> > > > > > > 
> > > > > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > > > > ---
> > > > > > >  drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++----
> > > > > > >  1 file changed, 15 insertions(+), 4 deletions(-)
> > > > > > > 
> > > > > > > diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> > > > > > > index 2e50104ae138..49c56da9bc67 100644
> > > > > > > --- a/drivers/media/rc/rc-ir-raw.c
> > > > > > > +++ b/drivers/media/rc/rc-ir-raw.c
> > > > > > > @@ -22,16 +22,27 @@ static int ir_raw_event_thread(void *data)
> > > > > > >  {
> > > > > > >  	struct ir_raw_event ev;
> > > > > > >  	struct ir_raw_handler *handler;
> > > > > > > -	struct ir_raw_event_ctrl *raw = (struct ir_raw_event_ctrl *)data;
> > > > > > > +	struct ir_raw_event_ctrl *raw = data;
> > > > > > > +	struct rc_dev *dev = raw->dev;
> > > > > > >  
> > > > > > >  	while (1) {
> > > > > > >  		mutex_lock(&ir_raw_handler_lock);
> > > > > > >  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
> > > > > > > +			if (is_timing_event(ev)) {
> > > > > > > +				if (ev.duration == 0)
> > > > > > > +					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> > > > > > > +				if (is_timing_event(raw->prev_ev) &&
> > > > > > > +				    !is_transition(&ev, &raw->prev_ev))
> > > > > > > +					dev_err(&dev->dev, "two consecutive events of type %s",
> > > > > > > +						TO_STR(ev.pulse));
> > > > > > > +				if (raw->prev_ev.reset && ev.pulse == 0)
> > > > > > > +					dev_err(&dev->dev, "timing event after reset should be pulse");
> > > > > > > +			}
> > > > > > 
> > > > > > ... considering that we continue the processing as if nothing happened, is it
> > > > > > really an error ? 
> > > > > > 
> > > > > > Could we consider something less invasive ? like dev_dbg() or dev_warn_once() ?
> > > > > 
> > > > > Maybe it should be dev_warn(). The fact that it is not dev_warn_once() means
> > > > > that we now know this occurs regularly.
> > > > 
> > > > It seems weird to report this over and over again.
> > > > 
> > > > > 
> > > > > Would you mind testing the following patch please?
> > > > > 
> > > > > Thanks
> > > > > 
> > > > > Sean
> > > > > 
> > > > > From 6a44fbe4738d230b9cf378777e7e9a93e5fda726 Mon Sep 17 00:00:00 2001
> > > > > From: Sean Young <sean@mess.org>
> > > > > Date: Tue, 19 Jun 2018 13:50:36 +0100
> > > > > Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
> > > > >  space
> > > > > 
> > > > > The meson generates one edge per interrupt. The duration is encoded in 12
> > > > > bits of 10 microseconds, so it can only encoding a maximum of 40
> > > > > milliseconds. As a result, it can produce multiple space events.
> > > > > 
> > > > > Signed-off-by: Sean Young <sean@mess.org>
> > > > > ---
> > > > >  drivers/media/rc/meson-ir.c | 3 ++-
> > > > >  1 file changed, 2 insertions(+), 1 deletion(-)
> > > > > 
> > > > > diff --git a/drivers/media/rc/meson-ir.c b/drivers/media/rc/meson-ir.c
> > > > > index f449b35d25e7..9747426719b2 100644
> > > > > --- a/drivers/media/rc/meson-ir.c
> > > > > +++ b/drivers/media/rc/meson-ir.c
> > > > > @@ -97,7 +97,8 @@ static irqreturn_t meson_ir_irq(int irqno, void *dev_id)
> > > > >  	status = readl_relaxed(ir->reg + IR_DEC_STATUS);
> > > > >  	rawir.pulse = !!(status & STATUS_IR_DEC_IN);
> > > > >  
> > > > > -	ir_raw_event_store_with_timeout(ir->rc, &rawir);
> > > > > +	if (ir_raw_event_store_with_filter(ir->rc, &rawir))
> > > > > +		ir_raw_event_handle(ir->rc);
> > > > >  
> > > > >  	spin_unlock(&ir->lock);
> > > > >  
> > > > 
> > > > Solve the problem on meson. Thx
> > > > Feel free to add this submitting the patch
> > > 
> > > Actually this patch is broken. ir_raw_event_store_with_timeout() generates
> > > IR timeouts, but ir_raw_event_store_with_filter() does not.
> > > 
> > > So this will need some rethinking. I think we need a 
> > > ir_raw_event_store_with_timeout_with_filter() but that is getting silly now.
> > > 
> > > Maybe filter should be a boolean property of an rc device and happen
> > > transparently. I'll write something like that when I get some time.
> > > 
> > 
> > would you still object to use dev_warn_once() in the meantime ?
> > Console flooding is quite annoying.
> 
> Yes, as much as I dislike it, I think that is the right solution for now.
> 
> Thanks,
> 
> Sean
> 
> From 2f61a8b6e850e34def282f1cbd1bfee0f5ab66c5 Mon Sep 17 00:00:00 2001
> From: Sean Young <sean@mess.org>
> Date: Tue, 26 Jun 2018 16:03:18 +0100
> Subject: [PATCH] media: rc: be less noisy when driver misbehaves
> 
> Since commit 48231f289e52 ("media: rc: drivers should produce alternate
> pulse and space timing events"), on meson-ir we are regularly producing
> errors. Reduce to warning level and only warn once to avoid flooding
> the log.
> 
> A proper fix for meson-ir is going to be too large for v4.18.
> 
> Signed-off-by: Sean Young <sean@mess.org>

Tested-by: Jerome Brunet <jbrunet@baylibre.com>

> ---
>  drivers/media/rc/rc-ir-raw.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/media/rc/rc-ir-raw.c b/drivers/media/rc/rc-ir-raw.c
> index 2e0066b1a31c..e7948908e78c 100644
> --- a/drivers/media/rc/rc-ir-raw.c
> +++ b/drivers/media/rc/rc-ir-raw.c
> @@ -30,13 +30,13 @@ static int ir_raw_event_thread(void *data)
>  		while (kfifo_out(&raw->kfifo, &ev, 1)) {
>  			if (is_timing_event(ev)) {
>  				if (ev.duration == 0)
> -					dev_err(&dev->dev, "nonsensical timing event of duration 0");
> +					dev_warn_once(&dev->dev, "nonsensical timing event of duration 0");
>  				if (is_timing_event(raw->prev_ev) &&
>  				    !is_transition(&ev, &raw->prev_ev))
> -					dev_err(&dev->dev, "two consecutive events of type %s",
> -						TO_STR(ev.pulse));
> +					dev_warn_once(&dev->dev, "two consecutive events of type %s",
> +						      TO_STR(ev.pulse));
>  				if (raw->prev_ev.reset && ev.pulse == 0)
> -					dev_err(&dev->dev, "timing event after reset should be pulse");
> +					dev_warn_once(&dev->dev, "timing event after reset should be pulse");
>  			}
>  			list_for_each_entry(handler, &ir_raw_handler_list, list)
>  				if (dev->enabled_protocols &