media: rc: drivers should produce alternate pulse and space timing events
Subsystems (4)
Sean Young
maintainerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Report an error if this is not the case or any problem with the generated raw events.
--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)
drivers/media/rc/rc-ir-raw.c +15 -4
@@ -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);
Signature · 1 line
2.17.0
Sean Young
maintainerPATCH 2/3 media: rc: decoders do not need to check for transitions
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.
--- 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(-)
drivers/media/rc/ir-mce_kbd-decoder.c +0 -6
@@ -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
drivers/media/rc/ir-rc5-decoder.c +0 -3
@@ -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
drivers/media/rc/ir-rc6-decoder.c +0 -10
@@ -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
Signature · 1 line
2.17.0
Sean Young
maintainerPATCH 3/3 media: rc: winbond: do not send reset and timeout raw events on startup
In reply to Sean Youngir_raw_event_set_idle() sends a timeout event which is not needed, and on startup no reset event is needed either.
--- drivers/media/rc/winbond-cir.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
drivers/media/rc/winbond-cir.c +2 -2
@@ -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);
Signature · 1 line
2.17.0
Jerome Brunet
reviewerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
In reply to Sean YoungQuoted 2 lines · Sean Young
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 ...
Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {
drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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() ?
Quoted 11 lines
drivers/media/rc/rc-ir-raw.c +3
-3list_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);
Sean Young
maintainerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Quoted 50 lines · Jerome Brunet
Quoted 2 lines · Sean Young
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 ...Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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 ?
Quoted 1 line
Could we consider something less invasive ? like
dev_dbg()ordev_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
Quoted 1 line
From
6a44fbe4738d230b9cf378777e7e9a93e5fda726Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
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.
--- drivers/media/rc/meson-ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)
drivers/media/rc/meson-ir.c +2 -1
@@ -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);
Signature · 1 line
2.17.1
Jerome Brunet
reviewerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Quoted 51 lines · Sean Young
Quoted 50 lines · Jerome Brunet
Quoted 2 lines · Sean Young
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 ...Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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 ?
Quoted 4 lines
Quoted 1 line
Could we consider something less invasive ? like
dev_dbg()ordev_warn_once()?Maybe it should be
dev_warn(). The fact that it is notdev_warn_once()means that we now know this occurs regularly.
It seems weird to report this over and over again.
Quoted 27 lines
Would you mind testing the following patch please?
Thanks
Sean
From
6a44fbe4738d230b9cf378777e7e9a93e5fda726Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
Date: Tue, 19 Jun 2018 13:50:36 +0100
Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
spaceThe 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 <[email protected]>--- drivers/media/rc/meson-ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/media/rc/meson-ir.c +0
-0
drivers/media/rc/meson-ir.c static irqreturn_t meson_ir_irq(int irqno, void *dev_id) · line 104
Quoted 9 lines
drivers/media/rc/meson-ir.c +2
-1@@ -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
Cheers
Sean Young
maintainerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Quoted 93 lines · Jerome Brunet
Quoted 51 lines · Sean Young
Quoted 50 lines · Jerome Brunet
Quoted 2 lines · Sean Young
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 ...Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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 ?
Quoted 4 lines
Quoted 1 line
Could we consider something less invasive ? like
dev_dbg()ordev_warn_once()?Maybe it should be
dev_warn(). The fact that it is notdev_warn_once()means that we now know this occurs regularly.It seems weird to report this over and over again.
Quoted 34 lines
Would you mind testing the following patch please?
Thanks
Sean
From
6a44fbe4738d230b9cf378777e7e9a93e5fda726Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
Date: Tue, 19 Jun 2018 13:50:36 +0100
Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
spaceThe 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 <[email protected]>--- drivers/media/rc/meson-ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/media/rc/meson-ir.c +2
-1@@ -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);
Quoted 5 lines
drivers/media/rc/meson-ir.c static irqreturn_t meson_ir_irq(int irqno, void *dev_id) · line 104
Quoted 2 lines
drivers/media/rc/meson-ir.c +0
-0spin_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
Jerome Brunet
reviewerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Quoted 100 lines · Sean Young
Quoted 93 lines · Jerome Brunet
Quoted 51 lines · Sean Young
Quoted 50 lines · Jerome Brunet
Quoted 2 lines · Sean Young
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 ...Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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 ?
Quoted 4 lines
Quoted 1 line
Could we consider something less invasive ? like
dev_dbg()ordev_warn_once()?Maybe it should be
dev_warn(). The fact that it is notdev_warn_once()means that we now know this occurs regularly.It seems weird to report this over and over again.
Quoted 34 lines
Would you mind testing the following patch please?
Thanks
Sean
From
6a44fbe4738d230b9cf378777e7e9a93e5fda726Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
Date: Tue, 19 Jun 2018 13:50:36 +0100
Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
spaceThe 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 <[email protected]>--- drivers/media/rc/meson-ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/media/rc/meson-ir.c +2
-1@@ -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);Quoted 5 lines
drivers/media/rc/meson-ir.c static irqreturn_t meson_ir_irq(int irqno, void *dev_id) · line 104
Quoted 2 lines
drivers/media/rc/meson-ir.c +0
-0spin_unlock (&ir->lock);Solve the problem on meson. Thx
Feel free to add this submitting the patch
Quoted 9 lines
Actually this patch is broken.
ir_raw_event_store_with_timeout()generates IR timeouts, butir_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.
Quoted 2 lines
Sean
Sean Young
maintainerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Quoted 107 lines · Jerome Brunet
Quoted 106 lines · Sean Young
Quoted 93 lines · Jerome Brunet
Quoted 51 lines · Sean Young
Quoted 50 lines · Jerome Brunet
Quoted 2 lines · Sean Young
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 ...Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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 ?
Quoted 4 lines
Quoted 1 line
Could we consider something less invasive ? like
dev_dbg()ordev_warn_once()?Maybe it should be
dev_warn(). The fact that it is notdev_warn_once()means that we now know this occurs regularly.It seems weird to report this over and over again.
Quoted 34 lines
Would you mind testing the following patch please?
Thanks
Sean
From
6a44fbe4738d230b9cf378777e7e9a93e5fda726Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
Date: Tue, 19 Jun 2018 13:50:36 +0100
Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
spaceThe 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 <[email protected]>--- drivers/media/rc/meson-ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/media/rc/meson-ir.c +2
-1@@ -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);Quoted 5 lines
drivers/media/rc/meson-ir.c static irqreturn_t meson_ir_irq(int irqno, void *dev_id) · line 104
Quoted 2 lines
drivers/media/rc/meson-ir.c +0
-0spin_unlock (&ir->lock);Solve the problem on meson. Thx
Feel free to add this submitting the patchActually this patch is broken.
ir_raw_event_store_with_timeout()generates IR timeouts, butir_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.
Quoted 6 lines
Quoted 3 lines
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
Quoted 1 line
From
2f61a8b6e850e34def282f1cbd1bfee0f5ab66c5Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
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.
--- drivers/media/rc/rc-ir-raw.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)
drivers/media/rc/rc-ir-raw.c +4 -4
@@ -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 &
Signature · 1 line
2.17.1
Jerome Brunet
reviewerPATCH 1/3 media: rc: drivers should produce alternate pulse and space timing events
Quoted 126 lines · Sean Young
Quoted 107 lines · Jerome Brunet
Quoted 106 lines · Sean Young
Quoted 93 lines · Jerome Brunet
Quoted 51 lines · Sean Young
Quoted 50 lines · Jerome Brunet
Quoted 2 lines · Sean Young
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 ...Quoted 21 lines
Signed-off-by Sean Young <[email protected]>--- drivers/media/rc/rc-ir-raw.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +2
-1@@ -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 )) {drivers/media/rc/rc-ir-raw.c static int ir_raw_event_thread(void *data) · line 40
Quoted 10 lines
drivers/media/rc/rc-ir-raw.c +10
-0+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 ?
Quoted 4 lines
Quoted 1 line
Could we consider something less invasive ? like
dev_dbg()ordev_warn_once()?Maybe it should be
dev_warn(). The fact that it is notdev_warn_once()means that we now know this occurs regularly.It seems weird to report this over and over again.
Quoted 34 lines
Would you mind testing the following patch please?
Thanks
Sean
From
6a44fbe4738d230b9cf378777e7e9a93e5fda726Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
Date: Tue, 19 Jun 2018 13:50:36 +0100
Subject: [PATCH] media: rc: meson: rc rc0: two consecutive events of type
spaceThe 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 <[email protected]>--- drivers/media/rc/meson-ir.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)drivers/media/rc/meson-ir.c +2
-1@@ -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);Quoted 5 lines
drivers/media/rc/meson-ir.c static irqreturn_t meson_ir_irq(int irqno, void *dev_id) · line 104
Quoted 2 lines
drivers/media/rc/meson-ir.c +0
-0spin_unlock (&ir->lock);Solve the problem on meson. Thx
Feel free to add this submitting the patchActually this patch is broken.
ir_raw_event_store_with_timeout()generates IR timeouts, butir_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.Quoted 6 lines
Quoted 3 lines
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
2f61a8b6e850e34def282f1cbd1bfee0f5ab66c5Mon Sep 17 00:00:00 2001
From: Sean Young <[email protected]>
Date: Tue, 26 Jun 2018 16:03:18 +0100
Subject: [PATCH] media: rc: be less noisy when driver misbehaves
Quoted 8 lines
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 <[email protected]>
Quoted 26 lines
--- drivers/media/rc/rc-ir-raw.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)drivers/media/rc/rc-ir-raw.c +4
-4@@ -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 &