Re: [PATCH v8] Staging: comedi: convert while loop to timeout in ni_mio_common.c

[Ian Abbott <abbotti@mev.co.uk>]

On 2014-01-16 18:27, Chase Southwood wrote:
> This patch for ni_mio_common.c changes out a while loop for a timeout,
> which is preferred.
>
> Signed-off-by: Chase Southwood <chase.southwood@yahoo.com>
> ---
>
> Okay, back to v2, basically.  I fixed the checkpatch warning from v2,
> and added the error checking that was from v3, but otherwise it is the
> same.  Of note, I have used a udelay of 1 here (Greg had suggested to
> use 10, but Ian prefers 1 so I have gone with that, and I assume that
> cpu_relax is no longer an option.).
>
> 2: Changed from simple clean-up to swapping a timeout in for a while loop.
>
> 3: Removed extra counter variable, and added error checking.
>
> 4: No longer using counter variable, using jiffies instead.
>
> 5: udelay for 10u, instead of 1u.
>
> 6: Scrap udelay entirely, in favor of cpu_relax. Include asm/processor.h
> in order to use cpu_relax.
>
> 7: Fix typo (msec vs msecs).
>
> 8: Revert back to v2, with some small changes (see above).
>
>   drivers/staging/comedi/drivers/ni_mio_common.c | 12 +++++++++++-
>   1 file changed, 11 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/staging/comedi/drivers/ni_mio_common.c b/drivers/staging/comedi/drivers/ni_mio_common.c
> index 457b884..10c27cb 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,22 @@ static void ni_clear_ai_fifo(struct comedi_device *dev)
>   {
>   	const struct ni_board_struct *board = comedi_board(dev);
>   	struct ni_private *devpriv = dev->private;
> +	static const int timeout = 10000;
> +	int i;
>
>   	if (board->reg_type == ni_reg_6143) {
>   		/*  Flush the 6143 data FIFO */
>   		ni_writel(0x10, AIFIFO_Control_6143);	/*  Flush fifo */
>   		ni_writel(0x00, AIFIFO_Control_6143);	/*  Flush fifo */
> -		while (ni_readl(AIFIFO_Status_6143) & 0x10) ;	/*  Wait for complete */
> +		/*  Wait for complete */
> +		for (i = 0; i < timeout; i++) {
> +			if (!(ni_readl(AIFIFO_Status_6143) & 0x10))
> +				break;
> +			udelay(1);
> +		}
> +		if (i == timeout) {
> +			comedi_error(dev, "FIFO flush timeout.");
> +		}
>   	} else {
>   		devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
>   		if (board->reg_type == ni_reg_625x) {
>

Personally, I'm happy with it.  The upper bound on the iterations is 
quite high for something that could be called on an interrupt, but it's 
better than an infinite loop, and it only reach the upper bound if the 
hardware is broken or there's some other bug.

Reviewed-by: Ian Abbott <abbotti@mev.co.uk>

-- 
-=( Ian Abbott @ MEV Ltd.    E-mail: <abbotti@mev.co.uk>        )=-
-=( Tel: +44 (0)161 477 1898   FAX: +44 (0)161 718 3587         )=-