[alsa-devel] [PATCH] ad1838/cs4231 -- fix MCE timeout upon initial load

[Trent Piepho]

On Tue, 18 Sep 2007, Rene Herman wrote:
> > Second, schedule_timeout() returns immediately unless you have set the
> > task state to TASK_UNINTERRUPTIBLE or TASK_INTERRUPTIBLE.  I don't see
> > anywhere where this is done, so the 250ms delay is in fact a busy loop.
> > The call to schedule_timeout() appears to be quite pointless.
>
> That mce_down code was changed over the last week by Krzysztof, myself and
> Takashi so not sure what version you've been looking at, but the (original)

I originally wrote that email in response to the email I replied to, and
with respect to the code that was in it:
http://article.gmane.org/gmane.linux.alsa.devel/48528

There were more patches before I actually sent the email, so I based my
patch on what was in ALSA Hg at the time.

> version that the quoted patch was against didn't use schedule_timeout, but a
> timeout based sleeping loop for cs4231 and schedule_timeout_interruptible()
> for ad1848 which sets the state itself.

You can see the version in Hg after your patch was applied, and it
uses schedule_timeout()
http://hg.alsa-project.org/alsa-kernel/file/f3f37d068eae/isa/ad1848/ad1848_lib.c

It wasn't until changeset 3675ae62becf, 7 days later (and after I wrote that
email), that it was changed.

> > Here is a patch to fix all these problems and simplify the code in the
> > process.
>
> I looked at it, but am not sure what version it is against. It seems to

Current Hg.

> msleep(3) still under lock. As you said, the minimal fix right now is to

I seem to have deleted the necessary unlock call before I sent the patch.  One
of the tests is inverted too.  Here is a new version which fixes all that.  The
other problem you pointed at out in the current code about mistaken timeouts is
fixed as well.
-------------- next part --------------
ad1848: Fix msleep while atomic



Simplest fix.



Signed-off-by: Trent Piepho <xyzzy at speakeasy.org>

diff -r 50502c13d2cf isa/ad1848/ad1848_lib.c
--- a/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:08:32 2007 +0200
+++ b/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:19:52 2007 -0700
@@ -236,7 +236,9 @@ static void snd_ad1848_mce_down(struct s
 	 * calibration process to start. Needs upto 5 sample periods on AD1848
 	 * which at the slowest possible rate of 5.5125 kHz means 907 us.
 	 */
+	spin_unlock_irqrestore(&chip->reg_lock, flags);
 	msleep(1);
+	spin_lock_irqsave(&chip->reg_lock, flags);
 
 	snd_printdd("(2) jiffies = %lu\n", jiffies);
 
-------------- next part --------------
ad1848: simplify MCE down code

The polling loop to check for ACI to go down was more convoluted than it
needed to be.  New loop should be more efficient and it is a lot simpler.  The
old loop checked for a timeout before checking for ACI down, which could
result in an erroneous timeout.  It's only a failure if the timeout expires
_and_ ACI is still high.  There is nothing wrong with the timeout expiring
while the task is sleeping if ACI went low.

A polling loop to check for the device to leaving INIT mode is removed.  The
device must have already left init for the previous ACI loop to have finished.

Signed-off-by: Trent Piepho <xyzzy at speakeasy.org>

diff -r ec96283a273f isa/ad1848/ad1848_lib.c
--- a/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:20:48 2007 -0700
+++ b/isa/ad1848/ad1848_lib.c	Mon Sep 17 19:22:36 2007 -0700
@@ -203,9 +203,8 @@ static void snd_ad1848_mce_up(struct snd
 
 static void snd_ad1848_mce_down(struct snd_ad1848 *chip)
 {
-	unsigned long flags;
-	int timeout;
-	unsigned long end_time;
+	unsigned long flags, timeout;
+	int regsel;
 
 	spin_lock_irqsave(&chip->reg_lock, flags);
 	for (timeout = 5; timeout > 0; timeout--)
@@ -222,50 +221,34 @@ static void snd_ad1848_mce_down(struct s
 #endif
 
 	chip->mce_bit &= ~AD1848_MCE;
-	timeout = inb(AD1848P(chip, REGSEL));
-	outb(chip->mce_bit | (timeout & 0x1f), AD1848P(chip, REGSEL));
-	if (timeout == 0x80)
+	regsel = inb(AD1848P(chip, REGSEL));
+	outb(chip->mce_bit | (regsel & 0x1f), AD1848P(chip, REGSEL));
+	if (regsel == 0x80)
 		snd_printk(KERN_WARNING "mce_down [0x%lx]: serious init problem - codec still busy\n", chip->port);
-	if ((timeout & AD1848_MCE) == 0) {
+	if ((regsel & AD1848_MCE) == 0) {
 		spin_unlock_irqrestore(&chip->reg_lock, flags);
 		return;
 	}
 
 	/*
-	 * Wait for (possible -- during init auto-calibration may not be set)
-	 * calibration process to start. Needs upto 5 sample periods on AD1848
-	 * which at the slowest possible rate of 5.5125 kHz means 907 us.
+	 * Wait for auto-calibration (AC) process to finish, i.e. ACI to go low.
+	 * It may take up to 5 sample periods (at most 907 us @ 5.5125 kHz) for
+	 * the process to _start_, so it is important to wait at least that long
+	 * before checking.  Otherwise we might think AC has finished when it
+	 * has in fact not begun.  It should take 128 (no AC) or 384 (AC) cycles
+	 * for ACI to drop.  This gives a wait of at most 70 ms with a more
+	 * typical value of 3-9 ms.
 	 */
-	spin_unlock_irqrestore(&chip->reg_lock, flags);
-	msleep(1);
-	spin_lock_irqsave(&chip->reg_lock, flags);
-
-	snd_printdd("(2) jiffies = %lu\n", jiffies);
-
-	end_time = jiffies + msecs_to_jiffies(250);
-	while (snd_ad1848_in(chip, AD1848_TEST_INIT) & AD1848_CALIB_IN_PROGRESS) {
+	timeout = jiffies + msecs_to_jiffies(250);
+	do {
 		spin_unlock_irqrestore(&chip->reg_lock, flags);
-		if (time_after(jiffies, end_time)) {
-			snd_printk(KERN_ERR "mce_down - auto calibration time out (2)\n");
-			return;
-		}
 		msleep(1);
 		spin_lock_irqsave(&chip->reg_lock, flags);
-	}
-
-	snd_printdd("(3) jiffies = %lu\n", jiffies);
-
-	end_time = jiffies + msecs_to_jiffies(100);
-	while (inb(AD1848P(chip, REGSEL)) & AD1848_INIT) {
-		spin_unlock_irqrestore(&chip->reg_lock, flags);
-		if (time_after(jiffies, end_time)) {
-			snd_printk(KERN_ERR "mce_down - auto calibration time out (3)\n");
-			return;
-		}
-		msleep(1);
-		spin_lock_irqsave(&chip->reg_lock, flags);
-	}
-	spin_unlock_irqrestore(&chip->reg_lock, flags);
+		regsel = snd_ad1848_in(chip, AD1848_TEST_INIT);
+	} while ((regsel & AD1848_CALIB_IN_PROGRESS) && time_before(jiffies, timeout));
+	spin_unlock_irqrestore(&chip->reg_lock, flags);
+	if (regsel & AD1848_CALIB_IN_PROGRESS)
+		snd_printk(KERN_ERR "mce_down - auto calibration time out (2)\n");
 
 	snd_printdd("(4) jiffies = %lu\n", jiffies);
 	snd_printd("mce_down - exit = 0x%x\n", inb(AD1848P(chip, REGSEL)));