| Message ID | 20161101065111.hofyxjps2iwmxpzj@gangnam.samsung (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Andi, On Tue, Nov 01, 2016 at 03:51:11PM +0900, Andi Shyti wrote: > > Andi, it would be good to know what the use-case for the original change is. > > the use case is the ir-spi itself which doesn't need the lirc to > perform any waiting on its behalf. Here is the crux of the problem: in the ir-spi case no wait will actually happen here, and certainly no "over-wait". The patch below will not change behaviour at all. In the ir-spi case, "towait" will be 0 and no wait happens. I think the code is already in good shape but somehow there is a misunderstanding. Did I miss something? Sean > To me it just doesn't look right to simulate a fake transmission > period and wait unnecessary time. Of course, the "over-wait" is not > a big deal and at the end we can decide to drop it. > > Otherwise, an alternative could be to add the boolean > 'tx_no_wait' in the rc_dev structure. It could be set by the > device driver during the initialization and the we can follow > your approach. > > Something like this: > > diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c > index c327730..4553d04 100644 > --- a/drivers/media/rc/ir-lirc-codec.c > +++ b/drivers/media/rc/ir-lirc-codec.c > @@ -161,15 +161,19 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf, > > ret *= sizeof(unsigned int); > > - /* > - * The lircd gap calculation expects the write function to > - * wait for the actual IR signal to be transmitted before > - * returning. > - */ > - towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get()); > - if (towait > 0) { > - set_current_state(TASK_INTERRUPTIBLE); > - schedule_timeout(usecs_to_jiffies(towait)); > + if (!dev->tx_no_wait) { > + /* > + * The lircd gap calculation expects the write function to > + * wait for the actual IR signal to be transmitted before > + * returning. > + */ > + towait = ktime_us_delta(ktime_add_us(start, duration), > + ktime_get()); > + if (towait > 0) { > + set_current_state(TASK_INTERRUPTIBLE); > + schedule_timeout(usecs_to_jiffies(towait)); > + } > + > } > > out: > diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c > index fcda1e4..e44abfa 100644 > --- a/drivers/media/rc/ir-spi.c > +++ b/drivers/media/rc/ir-spi.c > @@ -149,6 +149,7 @@ static int ir_spi_probe(struct spi_device *spi) > if (!idata->rc) > return -ENOMEM; > > + idata->rc->tx_no_wait = true; > idata->rc->tx_ir = ir_spi_tx; > idata->rc->s_tx_carrier = ir_spi_set_tx_carrier; > idata->rc->s_tx_duty_cycle = ir_spi_set_duty_cycle; > diff --git a/include/media/rc-core.h b/include/media/rc-core.h > index fe0c9c4..c3ced9b 100644 > --- a/include/media/rc-core.h > +++ b/include/media/rc-core.h > @@ -85,6 +85,9 @@ enum rc_filter_type { > * @input_dev: the input child device used to communicate events to userspace > * @driver_type: specifies if protocol decoding is done in hardware or software > * @idle: used to keep track of RX state > + * @tx_no_wait: decides whether to perform or not a sync write or not. The > + * device driver setting it to true must make sure to not break the ABI > + * which requires a sync transfer. > * @allowed_protocols: bitmask with the supported RC_BIT_* protocols > * @enabled_protocols: bitmask with the enabled RC_BIT_* protocols > * @allowed_wakeup_protocols: bitmask with the supported RC_BIT_* wakeup protocols > @@ -147,6 +150,7 @@ struct rc_dev { > struct input_dev *input_dev; > enum rc_driver_type driver_type; > bool idle; > + bool tx_no_wait; > u64 allowed_protocols; > u64 enabled_protocols; > u64 allowed_wakeup_protocols; > > Thanks, > Andi > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Sean, > > > Andi, it would be good to know what the use-case for the original change is. > > > > the use case is the ir-spi itself which doesn't need the lirc to > > perform any waiting on its behalf. > > Here is the crux of the problem: in the ir-spi case no wait will actually > happen here, and certainly no "over-wait". The patch below will not change > behaviour at all. > > In the ir-spi case, "towait" will be 0 and no wait happens. > > I think the code is already in good shape but somehow there is a > misunderstanding. Did I miss something? We can just drop this patch, it's just something small that is bothering me. I will send a new patchset without this one. Thanks, Andi -- To unsubscribe from this list: send the line "unsubscribe linux-media" 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/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c index c327730..4553d04 100644 --- a/drivers/media/rc/ir-lirc-codec.c +++ b/drivers/media/rc/ir-lirc-codec.c @@ -161,15 +161,19 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf, ret *= sizeof(unsigned int); - /* - * The lircd gap calculation expects the write function to - * wait for the actual IR signal to be transmitted before - * returning. - */ - towait = ktime_us_delta(ktime_add_us(start, duration), ktime_get()); - if (towait > 0) { - set_current_state(TASK_INTERRUPTIBLE); - schedule_timeout(usecs_to_jiffies(towait)); + if (!dev->tx_no_wait) { + /* + * The lircd gap calculation expects the write function to + * wait for the actual IR signal to be transmitted before + * returning. + */ + towait = ktime_us_delta(ktime_add_us(start, duration), + ktime_get()); + if (towait > 0) { + set_current_state(TASK_INTERRUPTIBLE); + schedule_timeout(usecs_to_jiffies(towait)); + } + } out: diff --git a/drivers/media/rc/ir-spi.c b/drivers/media/rc/ir-spi.c index fcda1e4..e44abfa 100644 --- a/drivers/media/rc/ir-spi.c +++ b/drivers/media/rc/ir-spi.c @@ -149,6 +149,7 @@ static int ir_spi_probe(struct spi_device *spi) if (!idata->rc) return -ENOMEM; + idata->rc->tx_no_wait = true; idata->rc->tx_ir = ir_spi_tx; idata->rc->s_tx_carrier = ir_spi_set_tx_carrier; idata->rc->s_tx_duty_cycle = ir_spi_set_duty_cycle; diff --git a/include/media/rc-core.h b/include/media/rc-core.h index fe0c9c4..c3ced9b 100644 --- a/include/media/rc-core.h +++ b/include/media/rc-core.h @@ -85,6 +85,9 @@ enum rc_filter_type { * @input_dev: the input child device used to communicate events to userspace * @driver_type: specifies if protocol decoding is done in hardware or software * @idle: used to keep track of RX state + * @tx_no_wait: decides whether to perform or not a sync write or not. The + * device driver setting it to true must make sure to not break the ABI + * which requires a sync transfer. * @allowed_protocols: bitmask with the supported RC_BIT_* protocols * @enabled_protocols: bitmask with the enabled RC_BIT_* protocols * @allowed_wakeup_protocols: bitmask with the supported RC_BIT_* wakeup protocols @@ -147,6 +150,7 @@ struct rc_dev { struct input_dev *input_dev; enum rc_driver_type driver_type; bool idle; + bool tx_no_wait; u64 allowed_protocols; u64 enabled_protocols; u64 allowed_wakeup_protocols;
Hi Sean, > Andi, it would be good to know what the use-case for the original change is. the use case is the ir-spi itself which doesn't need the lirc to perform any waiting on its behalf. To me it just doesn't look right to simulate a fake transmission period and wait unnecessary time. Of course, the "over-wait" is not a big deal and at the end we can decide to drop it. Otherwise, an alternative could be to add the boolean 'tx_no_wait' in the rc_dev structure. It could be set by the device driver during the initialization and the we can follow your approach. Something like this: Thanks, Andi -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html