On Thu, Aug 16, 2012 at 11:15:14PM +0100, Sean Young wrote:
>
>> The lirc TX functionality expects the process which writes (TX) data to
>> the lirc dev to sleep until the actual data has been transmitted by the
>> hardware.
>>
>> Since the same timeout calculation is duplicated in more than one driver
>> (and would have to be duplicated in even more drivers as they gain TX
>> support), it makes sense to move this timeout calculation to the lirc
>> layer instead.
>>
>> At the same time, centralize some of the sanity checks.
>>
>> Signed-off-by: David Härdeman <david@xxxxxxxxxxx>
>> Cc: Jarod Wilson <jwilson@xxxxxxxxxx>
>> Signed-off-by: Mauro Carvalho Chehab <mchehab@xxxxxxxxxx>
>> ---
>> drivers/media/rc/ir-lirc-codec.c | 33 +++++++++++++++++++++++++++++----
>> drivers/media/rc/mceusb.c | 18 ------------------
>> drivers/media/rc/rc-loopback.c | 12 ------------
>> 3 files changed, 29 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/media/rc/ir-lirc-codec.c b/drivers/media/rc/ir-lirc-codec.c
>> index d2fd064..6ad4a07 100644
>> --- a/drivers/media/rc/ir-lirc-codec.c
>> +++ b/drivers/media/rc/ir-lirc-codec.c
>> @@ -107,6 +107,12 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>> unsigned int *txbuf; /* buffer with values to transmit */
>> ssize_t ret = -EINVAL;
>> size_t count;
>> + ktime_t start;
>> + s64 towait;
>> + unsigned int duration = 0; /* signal duration in us */
>> + int i;
>> +
>> + start = ktime_get();
>>
>> lirc = lirc_get_pdata(file);
>> if (!lirc)
>> @@ -129,11 +135,30 @@ static ssize_t ir_lirc_transmit_ir(struct file *file, const char __user *buf,
>> goto out;
>> }
>>
>> - if (dev->tx_ir)
>> - ret = dev->tx_ir(dev, txbuf, count);
>> + if (!dev->tx_ir) {
>> + ret = -ENOSYS;
>> + goto out;
>> + }
>> +
>> + ret = dev->tx_ir(dev, txbuf, (u32)n);
>> + if (ret < 0)
>> + goto out;
>> +
>> + for (i = 0; i < ret; i++)
>> + duration += txbuf[i];
>>
>> - if (ret > 0)
>> - ret *= sizeof(unsigned);
>> + 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));
>> + }
>>
>> out:
>
>You've moved the sleeping out of the drivers to ir-lirc-codec, which makes
>sense for some devices. However you haven't updated winbond-cir.c which
>does two things:
>
>1) Modifies the txbuf (which is now used after transmit)
>2) Does the sleeping already since it blocks on the device to complete.
I'm not sure what issue 1) is?
Note that txstate is checked in wbcir_tx() at the beginning and the end.
The buf shouldn't be used after transmit...
>Surely if the driver can block on the device to complete then that is
>better than sleeping; there might some difference due to rounding and
>clock skew.
As noted in other mails, I actually think an asynchronous method is
better since it permits different approaches while a blocking TX method
forces that behavior.
Regards,
David
--
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
