[PATCH 01/48] staging: comedi: introduce comedi_timeout()
Ian Abbott
abbotti at mev.co.uk
Fri Feb 7 14:52:04 UTC 2014
On 2014-02-06 23:48, H Hartley Sweeten wrote:
> Introduce a comedi core helper function to handle the boilerplate
> needed by the drivers to wait for a condition to occur. Typically
> this condition is the analog input/output end-of-conversion used
> with the comedi (*insn_read) and (*insn_write) operations.
>
> To use this function, the drivers just need to provide a callback
> that checks for the desired condition. The callback should return
> 0 if the condition is met or -EBUSY if it is still waiting. Any
> other errno will be returned to the caller. If the timeout occurs
> before the condition is met -ETIMEDOUT will be returned.
>
> The parameters to the callback function are the comedi_device,
> comedi_subdevice, and comedi_insn pointers that were passed to the
> (*insn_read) or (*insn_write) as well as an unsigned long, driver
> specific, 'context' that can be used to pass any other information
> that might be needed in the callback. This 'context' could be anything
> such as the register offset to read the status or the bits needed
> to check the status. The comedi_timeout() function itself does not
> use any of these parameters.
>
> This will help remove all the crazy "wait this many loops" used by
> some of the drivers. It also creates a common errno for comedi to
> detect when a timeout occurs.
>
> ADC/DAC conversion times are typically pretty fast. A conservative
> timeout of 1 second (HZ) is used in comedi_timeout().
Perhaps it could be made tunable? Or at least the 1 second
comedi_timeout(blah, blah, blah) could call comedi_timeout_ms(blah,
blah, blah, 1000) or whatever.
More comments below....
>
> Signed-off-by: H Hartley Sweeten <hsweeten at visionengravers.com>
> Cc: Ian Abbott <abbotti at mev.co.uk>
> Cc: Greg Kroah-Hartman <gregkh at linuxfoundation.org>
> ---
> drivers/staging/comedi/comedidev.h | 6 ++++++
> drivers/staging/comedi/drivers.c | 33 +++++++++++++++++++++++++++++++++
> 2 files changed, 39 insertions(+)
>
> diff --git a/drivers/staging/comedi/comedidev.h b/drivers/staging/comedi/comedidev.h
> index f82bd42..d27510c 100644
> --- a/drivers/staging/comedi/comedidev.h
> +++ b/drivers/staging/comedi/comedidev.h
> @@ -353,6 +353,12 @@ void comedi_buf_memcpy_from(struct comedi_async *async, unsigned int offset,
>
> /* drivers.c - general comedi driver functions */
>
> +int comedi_timeout(struct comedi_device *, struct comedi_subdevice *,
> + struct comedi_insn *,
> + int (*cb)(struct comedi_device *, struct comedi_subdevice *,
> + struct comedi_insn *, unsigned long context),
> + unsigned long context);
> +
> int comedi_dio_insn_config(struct comedi_device *, struct comedi_subdevice *,
> struct comedi_insn *, unsigned int *data,
> unsigned int mask);
> diff --git a/drivers/staging/comedi/drivers.c b/drivers/staging/comedi/drivers.c
> index 2460803..ceec408 100644
> --- a/drivers/staging/comedi/drivers.c
> +++ b/drivers/staging/comedi/drivers.c
> @@ -18,6 +18,7 @@
>
> #include <linux/device.h>
> #include <linux/module.h>
> +#include <linux/delay.h>
> #include <linux/errno.h>
> #include <linux/kconfig.h>
> #include <linux/kernel.h>
> @@ -155,6 +156,38 @@ int insn_inval(struct comedi_device *dev, struct comedi_subdevice *s,
> }
>
> /**
> + * comedi_timeout() - wait up to 1 sec for a driver condition to occur.
> + * @dev: comedi_device struct
> + * @s: comedi_subdevice struct
> + * @insn: comedi_insn struct
> + * @cb: callback to check for the condition
> + * @context: private context from the driver
> + */
> +int comedi_timeout(struct comedi_device *dev,
> + struct comedi_subdevice *s,
> + struct comedi_insn *insn,
> + int (*cb)(struct comedi_device *dev,
> + struct comedi_subdevice *s,
> + struct comedi_insn *insn,
> + unsigned long context),
> + unsigned long context)
> +{
> + unsigned long timeout = jiffies + msecs_to_jiffies(HZ);
HZ is the number of jiffies in one second, so that should be:
unsigned long timeout = jiffies + HZ;
or:
unsigned long timeout = jiffies + msecs_to_jiffies(1000);
(If the time-out is to be made tunable in milliseconds, the second form
would be preferred.)
> + int ret;
> +
> + while (time_before(jiffies, timeout)) {
> + ret = cb(dev, s, insn, context);
> + if (ret == 0)
> + return 0;
That test is redundant as 0 != -EBUSY below.
> + if (ret != -EBUSY)
> + return ret;
> + udelay(1);
It could just do a cpu_relax() instead of udelay(1).
> + }
> + return -ETIMEDOUT;
> +}
If the function is pre-empted for a long while after calling cb(), the
time_before() test could fail without giving the cb() a final chance to
return 0.
How about:
int ret;
for (;;) {
bool going = time_before(jiffies, timeout);
ret = cb(dev, s, insn, context);
if (ret != -EBUSY)
return ret;
if (!going)
return -ETIMEDOUT;
cpu_relax();
}
(N.B. #include <asm/processor.h> for cpu_relax().)
> +EXPORT_SYMBOL_GPL(comedi_timeout);
> +
> +/**
> * comedi_dio_insn_config() - boilerplate (*insn_config) for DIO subdevices.
> * @dev: comedi_device struct
> * @s: comedi_subdevice struct
>
--
-=( Ian Abbott @ MEV Ltd. E-mail: <abbotti at mev.co.uk> )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
More information about the devel
mailing list