Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On Tue, Jan 14, 2014 at 06:23:05PM -0600, 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
> ---
>
> OK, here's another go at it. Hopefully everything looks more correct
> this time. Greg, I've followed the pattern you gave me, and I really
> appreciate all of the tips! As always, just let me know if there are
> still things that need adjusting (especially length of timeout, udelay,
> etc.).
>
> Thanks,
> Chase Southwood
>
> 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.
>
> drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++-
> 1 file changed, 10 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..882b249 100644
> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
> @@ -687,12 +687,21 @@ 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;
> + unsigned long timeout;
>
> 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 */
> + timeout = jiffies + msec_to_jiffies(100);
> + while (ni_readl(AIFIFO_Status_6143) & 0x10) {
> + if (time_after(jiffies, timeout)) {
> + comedi_error(dev, "FIFO flush timeout.");
> + break;
> + }
> + udelay(1);
Sleep for at least 10, as I think that's the smallest time delay you can
sleep for anyway (meaning it will be that long no matter what number you
put there less than 10, depending on the hardware used of course.)
Other than that, looks much better.
thanks,
greg k-h
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On 2014-01-15 03:58, Greg KH wrote:
On Tue, Jan 14, 2014 at 06:23:05PM -0600, 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
---
OK, here's another go at it. Hopefully everything looks more correct
this time. Greg, I've followed the pattern you gave me, and I really
appreciate all of the tips! As always, just let me know if there are
still things that need adjusting (especially length of timeout, udelay,
etc.).
Thanks,
Chase Southwood
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.
drivers/staging/comedi/drivers/ni_mio_common.c | 11 ++-
1 file changed, 10 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..882b249 100644
--- a/drivers/staging/comedi/drivers/ni_mio_common.c
+++ b/drivers/staging/comedi/drivers/ni_mio_common.c
@@ -687,12 +687,21 @@ 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;
+ unsigned long timeout;
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 */
+ timeout = jiffies + msec_to_jiffies(100);
+ while (ni_readl(AIFIFO_Status_6143) & 0x10) {
+ if (time_after(jiffies, timeout)) {
+ comedi_error(dev, "FIFO flush timeout.");
+ break;
+ }
+ udelay(1);
Sleep for at least 10, as I think that's the smallest time delay you can
sleep for anyway (meaning it will be that long no matter what number you
put there less than 10, depending on the hardware used of course.)
udelay() doesn't sleep (though it may get pre-empted) and I think you
can use any value you like within reason (up to a few 1000 microseconds).
Other than that, looks much better.
thanks,
greg k-h
--
-=( Ian Abbott @ MEV Ltd.E-mail: )=-
-=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=-
___
devel mailing list
de...@linuxdriverproject.org
http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
RE: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote: > Sleep for at least 10, as I think that's the smallest time delay you can > sleep for anyway (meaning it will be that long no matter what number you > put there less than 10, depending on the hardware used of course.) A bit off topic here but I have a somewhat related question about timeouts. There are a number of comedi drivers that do a "wait for end-of-conversion" as part of the (*insn_read) for an analog input subdevice or (*insn_write) for an analog output subdevice. These functions return an errno if a timeout occurs. Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting returned to the user as the result of the unlocked_ioctl file operation. What is the more appropriate errno? Or is there is better one that should be used? Regards, Hartley ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On Wed, Jan 15, 2014 at 06:29:21PM +, Hartley Sweeten wrote: > On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote: > > Sleep for at least 10, as I think that's the smallest time delay you can > > sleep for anyway (meaning it will be that long no matter what number you > > put there less than 10, depending on the hardware used of course.) > > A bit off topic here but I have a somewhat related question about timeouts. > > There are a number of comedi drivers that do a "wait for end-of-conversion" > as part of the (*insn_read) for an analog input subdevice or (*insn_write) for > an analog output subdevice. These functions return an errno if a timeout > occurs. > > Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting > returned to the user as the result of the unlocked_ioctl file operation. What > is > the more appropriate errno? Or is there is better one that should be used? I think they should all be -ETIMEDOUT, -ETIME is used for something else, and shouldn't be sent to userspace as I don't think it knows what to do with it. thanks, greg k-h ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
Re: [PATCH v4] Staging: comedi: convert while loop to timeout in ni_mio_common.c
On 2014-01-16 02:30, Greg KH wrote: On Wed, Jan 15, 2014 at 06:29:21PM +, Hartley Sweeten wrote: On Tuesday, January 14, 2014 8:59 PM, Greg KH wrote: Sleep for at least 10, as I think that's the smallest time delay you can sleep for anyway (meaning it will be that long no matter what number you put there less than 10, depending on the hardware used of course.) A bit off topic here but I have a somewhat related question about timeouts. There are a number of comedi drivers that do a "wait for end-of-conversion" as part of the (*insn_read) for an analog input subdevice or (*insn_write) for an analog output subdevice. These functions return an errno if a timeout occurs. Currently either -ETIME or -ETIMEDOUT is returned. This errno ends up getting returned to the user as the result of the unlocked_ioctl file operation. What is the more appropriate errno? Or is there is better one that should be used? I think they should all be -ETIMEDOUT, -ETIME is used for something else, and shouldn't be sent to userspace as I don't think it knows what to do with it. Linux libc ought to deal with both. glibc 2.17 has the following error strings in the "C" locale: ETIMEDOUT --> "Connection timed out" ETIME --> "Timer expired" I think BSD has: ETIMEDOUT --> "Operation timed out" ETIME doesn't exist so -ETIMEDOUT is better, even though the error string is a bit dodgy in some cases under Linux. This betrays the original purpose of the ETIMEDOUT error code, in the same way that the identifier and original purpose of the ENOTTY error code is betrayed by the original error string "Not a typewriter" (which is now "Inappropriate ioctl for device" in current glibc). -- -=( Ian Abbott @ MEV Ltd.E-mail: )=- -=( Tel: +44 (0)161 477 1898 FAX: +44 (0)161 718 3587 )=- ___ devel mailing list de...@linuxdriverproject.org http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
