| Message ID | 1441980024-1944-1-git-send-email-eric@nelint.com |
|---|---|
| State | Under Review, archived |
| Headers | show |
On Fri, Sep 11, 2015 at 07:00:24AM -0700, Eric Nelson wrote: > Many decoders require a trailing space (period without IR illumination) > to be delivered before completing a decode. > > Since the gpio-ir-recv driver only delivers events on gpio transitions, > a single IR symbol (caused by a quick touch on an IR remote) will not > be properly decoded without the use of a timer to flush the tail end > state of the IR receiver. This is a problem other IR drivers suffer from too. It might be better to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, with the duration set to what the timeout was. That is what irraw timeouts are for; much better than fake transitions. > This patch adds an optional device tree node "flush-ms" which, if > present, will use a jiffie-based timer to complete the last pulse > stream and allow decode. A common value for this is 100ms, I'm not sure what use it has to have it configurable. It's nice to have it exposed in rc_dev->timeout. Sean -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Shawn, On 09/14/2015 03:00 AM, Sean Young wrote: > On Fri, Sep 11, 2015 at 07:00:24AM -0700, Eric Nelson wrote: >> Many decoders require a trailing space (period without IR illumination) >> to be delivered before completing a decode. >> >> Since the gpio-ir-recv driver only delivers events on gpio transitions, >> a single IR symbol (caused by a quick touch on an IR remote) will not >> be properly decoded without the use of a timer to flush the tail end >> state of the IR receiver. > > This is a problem other IR drivers suffer from too. It might be better > to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, > with the duration set to what the timeout was. That is what irraw > timeouts are for; much better than fake transitions. > If I'm understanding this correctly, this would require modification of each decoder to handle what seems to be a special case regarding the GPIO IR driver (which needs an edge to trigger an interrupt). Isn't it better to have the device interface handle this in one place? >> This patch adds an optional device tree node "flush-ms" which, if >> present, will use a jiffie-based timer to complete the last pulse >> stream and allow decode. > > A common value for this is 100ms, I'm not sure what use it has to have > it configurable. It's nice to have it exposed in rc_dev->timeout. > I'm enough of a n00b regarding the details of the various decoders not to know that... I looked through the couple of decoders my customer was using (NEC and RC6) and came up with a value of 100ms though... Implementing this through DT and having the default as 0 (disabled) provides an interim solution if the choice is made to change each of the decoders, since I would expect that to take a while and a bunch of remote control devices for testing. Regards, Eric -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 14, 2015 at 07:34:10AM -0700, Eric Nelson wrote: > Thanks Shawn, > > On 09/14/2015 03:00 AM, Sean Young wrote: > > On Fri, Sep 11, 2015 at 07:00:24AM -0700, Eric Nelson wrote: > >> Many decoders require a trailing space (period without IR illumination) > >> to be delivered before completing a decode. > >> > >> Since the gpio-ir-recv driver only delivers events on gpio transitions, > >> a single IR symbol (caused by a quick touch on an IR remote) will not > >> be properly decoded without the use of a timer to flush the tail end > >> state of the IR receiver. > > > > This is a problem other IR drivers suffer from too. It might be better > > to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, > > with the duration set to what the timeout was. That is what irraw > > timeouts are for; much better than fake transitions. > > > > If I'm understanding this correctly, this would require modification > of each decoder to handle what seems to be a special case regarding > the GPIO IR driver (which needs an edge to trigger an interrupt). No, this is not a special case. Many drivers do have extra code to generate some sort of end-of-signal message: redrat3; igorplugusb; st_rc. They don't handle it consistently but this should be fixed. Secondly, the decoders already handle it. A timeout event matches is_timing_event(), so it's processed by the decoders. The duration should be set correctly. > Isn't it better to have the device interface handle this in one place? > >> This patch adds an optional device tree node "flush-ms" which, if > >> present, will use a jiffie-based timer to complete the last pulse > >> stream and allow decode. > > > > A common value for this is 100ms, I'm not sure what use it has to have > > it configurable. It's nice to have it exposed in rc_dev->timeout. > > > > I'm enough of a n00b regarding the details of the various decoders > not to know that... > > I looked through the couple of decoders my customer was using (NEC and > RC6) and came up with a value of 100ms though... > > Implementing this through DT and having the default as 0 (disabled) > provides an interim solution if the choice is made to change each of > the decoders, since I would expect that to take a while and a bunch of > remote control devices for testing. Many other drivers use 100ms just fine and I don't remember ever seeing any bug reports on that. Sean -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks again Sean, On 09/14/2015 07:54 AM, Sean Young wrote: > On Mon, Sep 14, 2015 at 07:34:10AM -0700, Eric Nelson wrote: >> Thanks Shawn, >> >> On 09/14/2015 03:00 AM, Sean Young wrote: >>> On Fri, Sep 11, 2015 at 07:00:24AM -0700, Eric Nelson wrote: >>>> Many decoders require a trailing space (period without IR illumination) >>>> to be delivered before completing a decode. >>>> >>>> Since the gpio-ir-recv driver only delivers events on gpio transitions, >>>> a single IR symbol (caused by a quick touch on an IR remote) will not >>>> be properly decoded without the use of a timer to flush the tail end >>>> state of the IR receiver. >>> >>> This is a problem other IR drivers suffer from too. It might be better >>> to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, >>> with the duration set to what the timeout was. That is what irraw >>> timeouts are for; much better than fake transitions. >>> >> >> If I'm understanding this correctly, this would require modification >> of each decoder to handle what seems to be a special case regarding >> the GPIO IR driver (which needs an edge to trigger an interrupt). > > No, this is not a special case. Many drivers do have extra code to generate > some sort of end-of-signal message: redrat3; igorplugusb; st_rc. They don't > handle it consistently but this should be fixed. > > Secondly, the decoders already handle it. A timeout event matches > is_timing_event(), so it's processed by the decoders. The duration should > be set correctly. > I think I did misunderstand you. You're suggesting that I re-work the patch to gpio-ir-recv.c to produce a timeout instead of an edge. Is that right? >> Isn't it better to have the device interface handle this in one place? > >>>> This patch adds an optional device tree node "flush-ms" which, if >>>> present, will use a jiffie-based timer to complete the last pulse >>>> stream and allow decode. >>> >>> A common value for this is 100ms, I'm not sure what use it has to have >>> it configurable. It's nice to have it exposed in rc_dev->timeout. >>> >> >> I'm enough of a n00b regarding the details of the various decoders >> not to know that... >> >> I looked through the couple of decoders my customer was using (NEC and >> RC6) and came up with a value of 100ms though... >> >> Implementing this through DT and having the default as 0 (disabled) >> provides an interim solution if the choice is made to change each of >> the decoders, since I would expect that to take a while and a bunch of >> remote control devices for testing. > > Many other drivers use 100ms just fine and I don't remember ever seeing > any bug reports on that. > So you'd like to see this as a constant? Please advise, Eric -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Sep 14, 2015 at 08:05:24AM -0700, Eric Nelson wrote: > Thanks again Sean, > > On 09/14/2015 07:54 AM, Sean Young wrote: > > On Mon, Sep 14, 2015 at 07:34:10AM -0700, Eric Nelson wrote: > >> Thanks Shawn, > >> > >> On 09/14/2015 03:00 AM, Sean Young wrote: > >>> On Fri, Sep 11, 2015 at 07:00:24AM -0700, Eric Nelson wrote: > >>>> Many decoders require a trailing space (period without IR illumination) > >>>> to be delivered before completing a decode. > >>>> > >>>> Since the gpio-ir-recv driver only delivers events on gpio transitions, > >>>> a single IR symbol (caused by a quick touch on an IR remote) will not > >>>> be properly decoded without the use of a timer to flush the tail end > >>>> state of the IR receiver. > >>> > >>> This is a problem other IR drivers suffer from too. It might be better > >>> to send a IR timeout event like st_rc_send_lirc_timeout() in st_rc.c, > >>> with the duration set to what the timeout was. That is what irraw > >>> timeouts are for; much better than fake transitions. > >>> > >> > >> If I'm understanding this correctly, this would require modification > >> of each decoder to handle what seems to be a special case regarding > >> the GPIO IR driver (which needs an edge to trigger an interrupt). > > > > No, this is not a special case. Many drivers do have extra code to generate > > some sort of end-of-signal message: redrat3; igorplugusb; st_rc. They don't > > handle it consistently but this should be fixed. > > > > Secondly, the decoders already handle it. A timeout event matches > > is_timing_event(), so it's processed by the decoders. The duration should > > be set correctly. > > > > I think I did misunderstand you. > > You're suggesting that I re-work the patch to gpio-ir-recv.c to > produce a timeout instead of an edge. Is that right? Yes, that's right. I'm thinking about patches the other drivers that don't do this and to test all drivers that they always output "[pulse space].. pulse timeout". At least for the hardware I've got. > >> Isn't it better to have the device interface handle this in one place? > > > >>>> This patch adds an optional device tree node "flush-ms" which, if > >>>> present, will use a jiffie-based timer to complete the last pulse > >>>> stream and allow decode. > >>> > >>> A common value for this is 100ms, I'm not sure what use it has to have > >>> it configurable. It's nice to have it exposed in rc_dev->timeout. > >>> > >> > >> I'm enough of a n00b regarding the details of the various decoders > >> not to know that... > >> > >> I looked through the couple of decoders my customer was using (NEC and > >> RC6) and came up with a value of 100ms though... > >> > >> Implementing this through DT and having the default as 0 (disabled) > >> provides an interim solution if the choice is made to change each of > >> the decoders, since I would expect that to take a while and a bunch of > >> remote control devices for testing. > > > > Many other drivers use 100ms just fine and I don't remember ever seeing > > any bug reports on that. > > > > So you'd like to see this as a constant? The way this can be configured with other drivers is using the ioctl LIRC_SET_REC_TIMEOUT, I'm not sure we need a new method for this in device tree. So you could either just have a default of 100ms; or in addition you can simply set the min_timeout and max_timeout on rcdev; give rcdev a default and then read the value of rcdev->timeout whenever a timeout is necessary. See how ite-cir.c does it for example. Actually it might be good to have #define IR_DEFAULT_TIMEOUT MS_TO_NS(100) in include/media/rc-core.h Sean -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Add timeout support to the gpio-ir-recv driver as discussed in this original patch: https://patchwork.ozlabs.org/patch/516827/ V2 uses the timeout field of the rcdev instead of a device tree field to set the timeout value as suggested by Sean Young. Eric Nelson (2): rc-core: define a default timeout for drivers rc: gpio-ir-recv: add timeout on idle drivers/media/rc/gpio-ir-recv.c | 22 ++++++++++++++++++++++ include/media/rc-core.h | 1 + 2 files changed, 23 insertions(+)
Add timeout support to the gpio-ir-recv driver as discussed in this original patch: https://patchwork.ozlabs.org/patch/516827/ V2 uses the timeout field of the rcdev instead of a device tree field to set the timeout value as suggested by Sean Young. Eric Nelson (2): rc-core: define a default timeout for drivers rc: gpio-ir-recv: add timeout on idle drivers/media/rc/gpio-ir-recv.c | 22 ++++++++++++++++++++++ include/media/rc-core.h | 1 + 2 files changed, 23 insertions(+)
On Mon, Sep 21, 2015 at 12:08:44PM -0700, Eric Nelson wrote: > Many decoders require a trailing space (period without IR illumination) > to be delivered before completing a decode. > > Since the gpio-ir-recv driver only delivers events on gpio transitions, > a single IR symbol (caused by a quick touch on an IR remote) will not > be properly decoded without the use of a timer to flush the tail end > state of the IR receiver. > > This patch initializes and uses a timer and the timeout field of rcdev > to complete the stream and allow decode. > > The timeout can be overridden through the use of the LIRC_SET_REC_TIMEOUT > ioctl. Thanks, this is much nicer. > Signed-off-by: Eric Nelson <eric@nelint.com> > --- > drivers/media/rc/gpio-ir-recv.c | 22 ++++++++++++++++++++++ > 1 file changed, 22 insertions(+) > > diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c > index 7dbc9ca..d3b216a 100644 > --- a/drivers/media/rc/gpio-ir-recv.c > +++ b/drivers/media/rc/gpio-ir-recv.c > @@ -30,6 +30,7 @@ struct gpio_rc_dev { > struct rc_dev *rcdev; > int gpio_nr; > bool active_low; > + struct timer_list flush_timer; > }; > > #ifdef CONFIG_OF > @@ -93,12 +94,26 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) > if (rc < 0) > goto err_get_value; > > + mod_timer(&gpio_dev->flush_timer, > + jiffies + nsecs_to_jiffies(gpio_dev->rcdev->timeout)); > + > ir_raw_event_handle(gpio_dev->rcdev); > > err_get_value: > return IRQ_HANDLED; > } > > +static void flush_timer(unsigned long arg) > +{ > + struct gpio_rc_dev *gpio_dev = (struct gpio_rc_dev *)arg; > + DEFINE_IR_RAW_EVENT(ev); > + > + ev.timeout = true; > + ev.duration = gpio_dev->rcdev->timeout; Nitpick: two spaces, checkpatch would have found this. > + ir_raw_event_store(gpio_dev->rcdev, &ev); > + ir_raw_event_handle(gpio_dev->rcdev); > +} > + > static int gpio_ir_recv_probe(struct platform_device *pdev) > { > struct gpio_rc_dev *gpio_dev; > @@ -144,6 +159,9 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) > rcdev->input_id.version = 0x0100; > rcdev->dev.parent = &pdev->dev; > rcdev->driver_name = GPIO_IR_DRIVER_NAME; > + rcdev->min_timeout = 1; > + rcdev->timeout = IR_DEFAULT_TIMEOUT; > + rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT; > if (pdata->allowed_protos) > rcdev->allowed_protocols = pdata->allowed_protos; > else > @@ -154,6 +172,10 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) > gpio_dev->gpio_nr = pdata->gpio_nr; > gpio_dev->active_low = pdata->active_low; > > + init_timer(&gpio_dev->flush_timer); > + gpio_dev->flush_timer.function = flush_timer; > + gpio_dev->flush_timer.data = (unsigned long)gpio_dev; You could use "setup_timer(&gpio_dev->flush_timer, flush_timer, (unsigned long)gpio_dev);" here. > + > rc = gpio_request(pdata->gpio_nr, "gpio-ir-recv"); > if (rc < 0) > goto err_gpio_request; You'll need a "del_timer_sync(&gpio_dev->flush_timer);" in gpio_ir_recv_remove() or you'll have a race on remove. Sean -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks for the review, Sean. On 09/23/2015 06:26 AM, Sean Young wrote: > On Mon, Sep 21, 2015 at 12:08:44PM -0700, Eric Nelson wrote: >> Many decoders require a trailing space (period without IR illumination) >> to be delivered before completing a decode. >> >> Since the gpio-ir-recv driver only delivers events on gpio transitions, >> a single IR symbol (caused by a quick touch on an IR remote) will not >> be properly decoded without the use of a timer to flush the tail end >> state of the IR receiver. >> >> This patch initializes and uses a timer and the timeout field of rcdev >> to complete the stream and allow decode. >> >> The timeout can be overridden through the use of the LIRC_SET_REC_TIMEOUT >> ioctl. > > Thanks, this is much nicer. > >> Signed-off-by: Eric Nelson <eric@nelint.com> >> --- >> drivers/media/rc/gpio-ir-recv.c | 22 ++++++++++++++++++++++ >> 1 file changed, 22 insertions(+) >> >> diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c >> index 7dbc9ca..d3b216a 100644 >> --- a/drivers/media/rc/gpio-ir-recv.c >> +++ b/drivers/media/rc/gpio-ir-recv.c >> @@ -30,6 +30,7 @@ struct gpio_rc_dev { >> struct rc_dev *rcdev; >> int gpio_nr; >> bool active_low; >> + struct timer_list flush_timer; >> }; >> >> #ifdef CONFIG_OF >> @@ -93,12 +94,26 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) >> if (rc < 0) >> goto err_get_value; >> >> + mod_timer(&gpio_dev->flush_timer, >> + jiffies + nsecs_to_jiffies(gpio_dev->rcdev->timeout)); >> + >> ir_raw_event_handle(gpio_dev->rcdev); >> >> err_get_value: >> return IRQ_HANDLED; >> } >> >> +static void flush_timer(unsigned long arg) >> +{ >> + struct gpio_rc_dev *gpio_dev = (struct gpio_rc_dev *)arg; >> + DEFINE_IR_RAW_EVENT(ev); >> + >> + ev.timeout = true; >> + ev.duration = gpio_dev->rcdev->timeout; > > Nitpick: two spaces, checkpatch would have found this. > Oddly, it didn't. ~/linux-ir$ ./scripts/checkpatch.pl --strict 0002-rc-gpio-ir-recv-add-timeout-on-idle.patch total: 0 errors, 0 warnings, 0 checks, 52 lines checked 0002-rc-gpio-ir-recv-add-timeout-on-idle.patch has no obvious style problems and is ready for submission. >> + ir_raw_event_store(gpio_dev->rcdev, &ev); >> + ir_raw_event_handle(gpio_dev->rcdev); >> +} >> + >> static int gpio_ir_recv_probe(struct platform_device *pdev) >> { >> struct gpio_rc_dev *gpio_dev; >> @@ -144,6 +159,9 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) >> rcdev->input_id.version = 0x0100; >> rcdev->dev.parent = &pdev->dev; >> rcdev->driver_name = GPIO_IR_DRIVER_NAME; >> + rcdev->min_timeout = 1; >> + rcdev->timeout = IR_DEFAULT_TIMEOUT; >> + rcdev->max_timeout = 10 * IR_DEFAULT_TIMEOUT; >> if (pdata->allowed_protos) >> rcdev->allowed_protocols = pdata->allowed_protos; >> else >> @@ -154,6 +172,10 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) >> gpio_dev->gpio_nr = pdata->gpio_nr; >> gpio_dev->active_low = pdata->active_low; >> >> + init_timer(&gpio_dev->flush_timer); >> + gpio_dev->flush_timer.function = flush_timer; >> + gpio_dev->flush_timer.data = (unsigned long)gpio_dev; > > > You could use "setup_timer(&gpio_dev->flush_timer, flush_timer, (unsigned long)gpio_dev);" here. > Cool. I'll fix this in V3. >> + >> rc = gpio_request(pdata->gpio_nr, "gpio-ir-recv"); >> if (rc < 0) >> goto err_gpio_request; > > You'll need a "del_timer_sync(&gpio_dev->flush_timer);" in > gpio_ir_recv_remove() or you'll have a race on remove. > Oops. I'll send a V3 shortly. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Sep 23, 2015 at 07:07:08AM -0700, Eric Nelson wrote: > Many decoders require a trailing space (period without IR illumination) > to be delivered before completing a decode. > > Since the gpio-ir-recv driver only delivers events on gpio transitions, > a single IR symbol (caused by a quick touch on an IR remote) will not > be properly decoded without the use of a timer to flush the tail end > state of the IR receiver. > > This patch initializes and uses a timer and the timeout field of rcdev > to complete the stream and allow decode. > > The timeout can be overridden through the use of the LIRC_SET_REC_TIMEOUT > ioctl. > > Signed-off-by: Eric Nelson <eric@nelint.com> Acked-by: Sean Young <sean@mess.org> -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Mon, 21 Sep 2015 12:08:43 -0700 Eric Nelson <eric@nelint.com> escreveu: > A default timeout value of 100ms is workable for most decoders. > Declare a constant to help standardize its' use. I guess the worse case scenario is the NEC protocol: http://www.sbprojects.com/knowledge/ir/nec.php with allows a repeat message to be sent on every 110ms. As the repeat message is 11.25 ms, that would mean a maximum time without data for 98.75 ms. So, in thesis, 100 ms would be ok. However, IR timings are not always precise and may affected by the battery charge. So, I think that a timeout of 100ms is too close to 98.75 and may cause troubles. S, IMHO, it is safer to define the default timeout as 125ms. Regards, Mauro > > Signed-off-by: Eric Nelson <eric@nelint.com> > --- > include/media/rc-core.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/include/media/rc-core.h b/include/media/rc-core.h > index ec921f6..62c64bd 100644 > --- a/include/media/rc-core.h > +++ b/include/media/rc-core.h > @@ -239,6 +239,7 @@ static inline void init_ir_raw_event(struct ir_raw_event *ev) > memset(ev, 0, sizeof(*ev)); > } > > +#define IR_DEFAULT_TIMEOUT MS_TO_NS(100) > #define IR_MAX_DURATION 500000000 /* 500 ms */ > #define US_TO_NS(usec) ((usec) * 1000) > #define MS_TO_US(msec) ((msec) * 1000) -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Mon, 21 Sep 2015 12:08:42 -0700 Eric Nelson <eric@nelint.com> escreveu: > Add timeout support to the gpio-ir-recv driver as discussed > in this original patch: > > https://patchwork.ozlabs.org/patch/516827/ > > V2 uses the timeout field of the rcdev instead of a device tree > field to set the timeout value as suggested by Sean Young. > > Eric Nelson (2): > rc-core: define a default timeout for drivers > rc: gpio-ir-recv: add timeout on idle I'm ok on having a default timeout for drivers, but the better would be to implement it at the RC core, and not inside each driver. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Thanks Mauro, On 10/03/2015 07:25 AM, Mauro Carvalho Chehab wrote: > Em Mon, 21 Sep 2015 12:08:43 -0700 > Eric Nelson <eric@nelint.com> escreveu: > >> A default timeout value of 100ms is workable for most decoders. >> Declare a constant to help standardize its' use. > > I guess the worse case scenario is the NEC protocol: > http://www.sbprojects.com/knowledge/ir/nec.php > > with allows a repeat message to be sent on every 110ms. As the > repeat message is 11.25 ms, that would mean a maximum time without > data for 98.75 ms. So, in thesis, 100 ms would be ok. However, IR > timings are not always precise and may affected by the battery charge. > > So, I think that a timeout of 100ms is too close to 98.75 and may > cause troubles. > > S, IMHO, it is safer to define the default timeout as 125ms. > Sounds good. I'll submit V3 shortly. -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Mauro, On 10/03/2015 07:27 AM, Mauro Carvalho Chehab wrote: > Em Mon, 21 Sep 2015 12:08:42 -0700 > Eric Nelson <eric@nelint.com> escreveu: > >> Add timeout support to the gpio-ir-recv driver as discussed >> in this original patch: >> >> https://patchwork.ozlabs.org/patch/516827/ >> >> V2 uses the timeout field of the rcdev instead of a device tree >> field to set the timeout value as suggested by Sean Young. >> >> Eric Nelson (2): >> rc-core: define a default timeout for drivers >> rc: gpio-ir-recv: add timeout on idle > > I'm ok on having a default timeout for drivers, but the better would > be to implement it at the RC core, and not inside each driver. > Can you elaborate? I'm not sure how that can be done, since the issue with the gpio-ir driver stems from the fact that there's no tail-end interrupt at the end of a burst. I think this isn't needed for other hardware. It seems that the call to schedule() in ir_raw_event_thread() could be changed to schedule_timeout(), but - this should only be done when not idle, and - only some hardware drivers need it I'm also not familiar with the rest of the code, so I'd be concerned about breaking other interfaces. Please advise, Eric -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt index 56e726e..13ff92d 100644 --- a/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt +++ b/Documentation/devicetree/bindings/media/gpio-ir-receiver.txt @@ -6,6 +6,7 @@ Required properties: Optional properties: - linux,rc-map-name: Linux specific remote control map name. + - flush-ms: time for final flush of 'space' pulse (period with no IR) Example node: diff --git a/drivers/media/rc/gpio-ir-recv.c b/drivers/media/rc/gpio-ir-recv.c index 7dbc9ca..e3c353e 100644 --- a/drivers/media/rc/gpio-ir-recv.c +++ b/drivers/media/rc/gpio-ir-recv.c @@ -29,7 +29,9 @@ struct gpio_rc_dev { struct rc_dev *rcdev; int gpio_nr; + int flush_jiffies; bool active_low; + struct timer_list flush_timer; }; #ifdef CONFIG_OF @@ -42,6 +44,7 @@ static int gpio_ir_recv_get_devtree_pdata(struct device *dev, struct device_node *np = dev->of_node; enum of_gpio_flags flags; int gpio; + u32 flush_ms = 0; gpio = of_get_gpio_flags(np, 0, &flags); if (gpio < 0) { @@ -50,6 +53,9 @@ static int gpio_ir_recv_get_devtree_pdata(struct device *dev, return gpio; } + of_property_read_u32(np, "flush-ms", &flush_ms); + pdata->flush_jiffies = msecs_to_jiffies(flush_ms); + pdata->gpio_nr = gpio; pdata->active_low = (flags & OF_GPIO_ACTIVE_LOW); /* probe() takes care of map_name == NULL or allowed_protos == 0 */ @@ -71,9 +77,8 @@ MODULE_DEVICE_TABLE(of, gpio_ir_recv_of_match); #endif -static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) +static void flush_gp(struct gpio_rc_dev *gpio_dev) { - struct gpio_rc_dev *gpio_dev = dev_id; int gval; int rc = 0; enum raw_event_type type = IR_SPACE; @@ -81,7 +86,7 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) gval = gpio_get_value(gpio_dev->gpio_nr); if (gval < 0) - goto err_get_value; + return; if (gpio_dev->active_low) gval = !gval; @@ -90,15 +95,30 @@ static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) type = IR_PULSE; rc = ir_raw_event_store_edge(gpio_dev->rcdev, type); - if (rc < 0) - goto err_get_value; + if (rc >= 0) + ir_raw_event_handle(gpio_dev->rcdev); +} - ir_raw_event_handle(gpio_dev->rcdev); +static irqreturn_t gpio_ir_recv_irq(int irq, void *dev_id) +{ + struct gpio_rc_dev *gpio_dev = dev_id; + + flush_gp(gpio_dev); + + if (gpio_dev->flush_jiffies) + mod_timer(&gpio_dev->flush_timer, + jiffies + gpio_dev->flush_jiffies); -err_get_value: return IRQ_HANDLED; } +static void flush_timer(unsigned long arg) +{ + struct gpio_rc_dev *gpio_dev = (struct gpio_rc_dev *)arg; + + flush_gp(gpio_dev); +} + static int gpio_ir_recv_probe(struct platform_device *pdev) { struct gpio_rc_dev *gpio_dev; @@ -152,8 +172,13 @@ static int gpio_ir_recv_probe(struct platform_device *pdev) gpio_dev->rcdev = rcdev; gpio_dev->gpio_nr = pdata->gpio_nr; + gpio_dev->flush_jiffies = pdata->flush_jiffies; gpio_dev->active_low = pdata->active_low; + init_timer(&gpio_dev->flush_timer); + gpio_dev->flush_timer.function = flush_timer; + gpio_dev->flush_timer.data = (unsigned long)gpio_dev; + rc = gpio_request(pdata->gpio_nr, "gpio-ir-recv"); if (rc < 0) goto err_gpio_request; diff --git a/include/media/gpio-ir-recv.h b/include/media/gpio-ir-recv.h index 0142736..88fae78 100644 --- a/include/media/gpio-ir-recv.h +++ b/include/media/gpio-ir-recv.h @@ -17,6 +17,7 @@ struct gpio_ir_recv_platform_data { int gpio_nr; bool active_low; u64 allowed_protos; + int flush_jiffies; const char *map_name; };
Many decoders require a trailing space (period without IR illumination) to be delivered before completing a decode. Since the gpio-ir-recv driver only delivers events on gpio transitions, a single IR symbol (caused by a quick touch on an IR remote) will not be properly decoded without the use of a timer to flush the tail end state of the IR receiver. This patch adds an optional device tree node "flush-ms" which, if present, will use a jiffie-based timer to complete the last pulse stream and allow decode. The "flush-ms" value should be chosen with a value that will convert well to jiffies (multiples of 10 are good). Signed-off-by: Eric Nelson <eric@nelint.com> --- Re-sending with expanded CC list. .../devicetree/bindings/media/gpio-ir-receiver.txt | 1 + drivers/media/rc/gpio-ir-recv.c | 39 ++++++++++++++++++---- include/media/gpio-ir-recv.h | 1 + 3 files changed, 34 insertions(+), 7 deletions(-)