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@xxxxxxxxxx>
>> ---
>> 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 linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
