[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