* [1/3] media: rc: drivers should produce alternate pulse and space timing events [not found] <20180512105531.30482-1-sean@mess.org> @ 2018-06-19 12:08 ` Jerome Brunet 2018-06-19 12:57 ` Sean Young 0 siblings, 1 reply; 7+ messages in thread From: Jerome Brunet @ 2018-06-19 12:08 UTC (permalink / raw) To: linus-amlogic 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); ^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/3] media: rc: drivers should produce alternate pulse and space timing events 2018-06-19 12:08 ` [1/3] media: rc: drivers should produce alternate pulse and space timing events Jerome Brunet @ 2018-06-19 12:57 ` Sean Young 2018-06-19 14:09 ` Jerome Brunet 0 siblings, 1 reply; 7+ messages in thread From: Sean Young @ 2018-06-19 12:57 UTC (permalink / raw) To: linus-amlogic 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [1/3] media: rc: drivers should produce alternate pulse and space timing events 2018-06-19 12:57 ` Sean Young @ 2018-06-19 14:09 ` Jerome Brunet 2018-06-26 14:37 ` Sean Young 0 siblings, 1 reply; 7+ messages in thread From: Jerome Brunet @ 2018-06-19 14:09 UTC (permalink / raw) To: linus-amlogic 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/3] media: rc: drivers should produce alternate pulse and space timing events 2018-06-19 14:09 ` Jerome Brunet @ 2018-06-26 14:37 ` Sean Young 2018-06-26 14:39 ` Jerome Brunet 0 siblings, 1 reply; 7+ messages in thread From: Sean Young @ 2018-06-26 14:37 UTC (permalink / raw) To: linus-amlogic 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/3] media: rc: drivers should produce alternate pulse and space timing events 2018-06-26 14:37 ` Sean Young @ 2018-06-26 14:39 ` Jerome Brunet 2018-06-26 16:33 ` Sean Young 0 siblings, 1 reply; 7+ messages in thread From: Jerome Brunet @ 2018-06-26 14:39 UTC (permalink / raw) To: linus-amlogic 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* [1/3] media: rc: drivers should produce alternate pulse and space timing events 2018-06-26 14:39 ` Jerome Brunet @ 2018-06-26 16:33 ` Sean Young 2018-06-26 16:46 ` Jerome Brunet 0 siblings, 1 reply; 7+ messages in thread From: Sean Young @ 2018-06-26 16:33 UTC (permalink / raw) To: linus-amlogic 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* [1/3] media: rc: drivers should produce alternate pulse and space timing events 2018-06-26 16:33 ` Sean Young @ 2018-06-26 16:46 ` Jerome Brunet 0 siblings, 0 replies; 7+ messages in thread From: Jerome Brunet @ 2018-06-26 16:46 UTC (permalink / raw) To: linus-amlogic 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 & ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-06-26 16:46 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <20180512105531.30482-1-sean@mess.org>
2018-06-19 12:08 ` [1/3] media: rc: drivers should produce alternate pulse and space timing events Jerome Brunet
2018-06-19 12:57 ` Sean Young
2018-06-19 14:09 ` Jerome Brunet
2018-06-26 14:37 ` Sean Young
2018-06-26 14:39 ` Jerome Brunet
2018-06-26 16:33 ` Sean Young
2018-06-26 16:46 ` Jerome Brunet
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox