DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws;
  s=s1024; d=yahoo.com;
  h=X-YMail-OSG:Received:X-Rocket-MIMEInfo:X-Mailer:References:Message-ID:Date:From:Reply-To:Subject:To:Cc:In-Reply-To:MIME-Version:Content-Type:Content-Transfer-Encoding;
  b=neq+UZBAztsaaGENNsHb+VdsFjSo8ymqYao2uPnS7Cz+iWz8RDGUKV/JYxaMz/xao0OWCY0RwgW+IdaHunyzWazpurh37XJDwPBS2T6OPmvfxF9xwQwFt9Wbb/7GwMblS/KYBYm8ViLwbBaMcsZ+L+45AiesJ2MIdwKUEYzyTjk=;
References: <1389669228-15090-1-git-send-email-chase.southwood@yahoo.com> <1389813752-3009-1-git-send-email-chase.southwood@yahoo.com> <52D7C2E8.9060204@mev.co.uk>
Message-ID: <1389894391.35761.YahooMailNeo@web164002.mail.gq1.yahoo.com>
Date: Thu, 16 Jan 2014 09:46:31 -0800 (PST)
From: Chase Southwood <chase.southwood@yahoo.com>
Reply-To: Chase Southwood <chase.southwood@yahoo.com>
Subject: Re: [PATCH v7] Staging: comedi: convert while loop to timeout in ni_mio_common.c
To: Ian Abbott <abbotti@mev.co.uk>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>
Cc: Ian Abbott <ian.abbott@mev.co.uk>,
        "hsweeten@visionengravers.com" <hsweeten@visionengravers.com>,
        "devel@driverdev.osuosl.org" <devel@driverdev.osuosl.org>,
        "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
In-Reply-To: <52D7C2E8.9060204@mev.co.uk>
MIME-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8BIT
Sender: linux-kernel-owner@vger.kernel.org

>On Thursday, January 16, 2014 5:31 AM, Ian Abbott <abbotti@mev.co.uk> wrote:

>On 2014-01-15 19:22, 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>
>> ---
>>
>> Hartley,
>> I sincerely apologize for the obvious mistake, I thought I had built it
>> but clearly I made a mistake somewhere, as your observation is exactly
>> correct.? It is now fixed.
>>
>> 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.
>>
>> 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).
>>
>>?  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..ab7a74c 100644
>> --- a/drivers/staging/comedi/drivers/ni_mio_common.c
>> +++ b/drivers/staging/comedi/drivers/ni_mio_common.c
>> @@ -55,6 +55,7 @@
>>?  #include <linux/interrupt.h>
>>?  #include <linux/sched.h>
>>?  #include <linux/delay.h>
>> +#include <asm/processor.h>
>>?  #include "8255.h"
>>?  #include "mite.h"
>>?  #include "comedi_fc.h"
>> @@ -687,12 +688,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 + msecs_to_jiffies(500);
>> +??? ??? while (ni_readl(AIFIFO_Status_6143) & 0x10) {
>> +??? ??? ??? if (time_after(jiffies, timeout)) {
>> +??? ??? ??? ??? comedi_error(dev, "FIFO flush timeout.");
>> +??? ??? ??? ??? break;
>> +??? ??? ??? }
>> +??? ??? ??? cpu_relax();
>> +??? ??? }
>>?  ??? } else {
>>?  ??? ??? devpriv->stc_writew(dev, 1, ADC_FIFO_Clear);
>>?  ??? ??? if (board->reg_type == ni_reg_625x) {
>>

>Sorry this is your worst nightmare, Chase.? Your patch is great and has 
>been modified in all the ways various people have suggested, but I don't 
>think it is suitable.
>
>After examining the code, it turns out ni_clear_ai_fifo() is sometimes 
>called from an interrupt service routine and I don't think you can wait 
>for jiffies to change in that case.? I think we need to go back to your 
>PATCH v2 and fix up the checkpatch.pl warning that Greg mentioned.
>
>Sorry about that.
>
>-- 
>-=( Ian Abbott @ MEV Ltd.? ? E-mail: <abbotti@mev.co.uk>? ? ? ? )=-
>-=( Tel: +44 (0)161 477 1898?  FAX: +44 (0)161 718 3587? ? ? ?  )=-

It's absolutely no problem. ?I'll get v2 cleaned up and sent out right away!

Thanks,
Chase Southwood
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/