[PATCH] cs4231-lib: improved waiting after mce_down

[PATCH] cs4231-lib: improved waiting after mce_down

[Krzysztof Helt]

From: Krzysztof Helt <krzysztof.h1@wp.pl>

This patch replaces long msleeps in waiting loops
with schedule_timeout() calls.

Signed-off-by: Krzysztof Helt <krzysztof.h1@wp.pl>
---

The cs4231 is quite fast and calibration takes about 2ms 
at 48kHz (the quickest). Loops wait in 10ms steps which
is usually too long.

This is done after the ad1848-lib driver, which waits in
this improved way. This makes the cs4231-lib and ad1848-lib
more similar and should help merging of the two.

Regards,
Krzysztof

diff -urp linux-2.6.22.old/sound/isa/cs423x/cs4231_lib.c linux-2.6.22/sound/isa/cs423x/cs4231_lib.c
--- linux-2.6.22.old/sound/isa/cs423x/cs4231_lib.c	2007-09-09 20:33:52.000000000 +0200
+++ linux-2.6.22/sound/isa/cs423x/cs4231_lib.c	2007-09-09 20:31:27.000000000 +0200
@@ -314,6 +314,7 @@ void snd_cs4231_mce_down(struct snd_cs42
 {
 	unsigned long flags;
 	int timeout;
+	long time;
 
 	snd_cs4231_busy_wait(chip);
 #if 0
@@ -340,31 +341,34 @@ void snd_cs4231_mce_down(struct snd_cs42
 
 	snd_printd("(2) jiffies = %li\n", jiffies);
 
-	/* in 10 ms increments, check condition, up to 250 ms */
-	timeout = 25;
+	time = HZ / 4;
 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
-		if (--timeout < 0) {
-			snd_printk("mce_down - auto calibration time out (2)\n");
+		spin_unlock_irqrestore(&chip->reg_lock, flags);
+		if (time <= 0) {
+			snd_printk(KERN_ERR "mce_down - "
+				   "auto calibration time out (2)\n");
 			return;
 		}
-		msleep(10);
+		time = schedule_timeout(time);
+		spin_lock_irqsave(&chip->reg_lock, flags);
 	}
-#if 0
-	printk("(3) jiffies = %li\n", jiffies);
-#endif
-	/* in 10 ms increments, check condition, up to 100 ms */
-	timeout = 10;
+
+	snd_printd("(3) jiffies = %li\n", jiffies);
+
+	time = HZ / 10;
 	while (cs4231_inb(chip, CS4231P(REGSEL)) & CS4231_INIT) {
-		if (--timeout < 0) {
-			snd_printk(KERN_ERR "mce_down - auto calibration time out (3)\n");
+		spin_unlock_irqrestore(&chip->reg_lock, flags);
+		if (time <= 0) {
+			snd_printk(KERN_ERR "mce_down - "
+				   "auto calibration time out (3)\n");
 			return;
 		}
-		msleep(10);
+		time = schedule_timeout(time);
+		spin_lock_irqsave(&chip->reg_lock, flags);
 	}
-#if 0
-	printk("(4) jiffies = %li\n", jiffies);
-	snd_printk("mce_down - exit = 0x%x\n", cs4231_inb(chip, CS4231P(REGSEL)));
-#endif
+	snd_printd("(4) jiffies = %li\n", jiffies);
+	snd_printk("mce_down - exit = 0x%x\n",
+		   cs4231_inb(chip, CS4231P(REGSEL)));
 }
 
 static unsigned int snd_cs4231_get_count(unsigned char format, unsigned int size)

Re: [PATCH] cs4231-lib: improved waiting after mce_down

[Takashi Iwai]

At Sun, 9 Sep 2007 22:11:31 +0200,
Krzysztof Helt wrote:
> 
> -	/* in 10 ms increments, check condition, up to 250 ms */
> -	timeout = 25;
> +	time = HZ / 4;

Use msecs_to_jiffies(250) instead.

>  	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
> -		if (--timeout < 0) {
> -			snd_printk("mce_down - auto calibration time out (2)\n");
> +		spin_unlock_irqrestore(&chip->reg_lock, flags);
> +		if (time <= 0) {
> +			snd_printk(KERN_ERR "mce_down - "
> +				   "auto calibration time out (2)\n");
>  			return;
>  		}
> -		msleep(10);
> +		time = schedule_timeout(time);
> +		spin_lock_irqsave(&chip->reg_lock, flags);
>  	}
> -#if 0
> -	printk("(3) jiffies = %li\n", jiffies);
> -#endif
> -	/* in 10 ms increments, check condition, up to 100 ms */
> -	timeout = 10;
> +
> +	snd_printd("(3) jiffies = %li\n", jiffies);
> +
> +	time = HZ / 10;

Here, too.


thanks,

Takashi

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Krzysztof Helt]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

From: Krzysztof Helt <krzysztof.h1 at wp.pl>

This patch replaces long msleeps in waiting loops
with schedule_timeout() calls.

Signed-off-by: Krzysztof Helt <krzysztof.h1 at wp.pl>
---

This is done against tree with Rene's patches.

Regards,
Krzysztof

[-- Attachment #2: cs4231-wait.diff --]
[-- Type: application/octet-stream, Size: 1448 bytes --]

diff -urp linux-2.6.22/sound/isa/cs423x/cs4231_lib.c linux-2.6.23/sound/isa/cs423x/cs4231_lib.c
--- linux-2.6.22/sound/isa/cs423x/cs4231_lib.c	2007-09-11 00:02:15.000000000 +0200
+++ linux-2.6.23/sound/isa/cs423x/cs4231_lib.c	2007-09-11 00:37:50.968166028 +0200
@@ -343,26 +343,29 @@ void snd_cs4231_mce_down(struct snd_cs42
 
 	snd_printdd("(1) jiffies = %lu\n", jiffies);
 
-	/* in 10 ms increments, check condition, up to 250 ms */
-	timeout = 25;
-	while (snd_cs4231_in(chip, CS4231_TEST_INIT) & CS4231_CALIB_IN_PROGRESS) {
-		if (--timeout < 0) {
-			snd_printk("mce_down - auto calibration time out (2)\n");
+	/* check condition up to 250 ms */
+	timeout = msecs_to_jiffies(250);
+	while (snd_cs4231_in(chip, CS4231_TEST_INIT) &
+		CS4231_CALIB_IN_PROGRESS) {
+
+		if (timeout <= 0) {
+			snd_printk(KERN_ERR "mce_down - "
+					"auto calibration time out (2)\n");
 			return;
 		}
-		msleep(10);
+		timeout = schedule_timeout(timeout);
 	}
 
 	snd_printdd("(2) jiffies = %lu\n", jiffies);
 
-	/* in 10 ms increments, check condition, up to 100 ms */
-	timeout = 10;
+	/* check condition up to 100 ms */
+	timeout = msecs_to_jiffies(100);
 	while (cs4231_inb(chip, CS4231P(REGSEL)) & CS4231_INIT) {
-		if (--timeout < 0) {
+		if (timeout <= 0) {
 			snd_printk(KERN_ERR "mce_down - auto calibration time out (3)\n");
 			return;
 		}
-		msleep(10);
+		timeout = schedule_timeout(timeout);
 	}
 
 	snd_printdd("(3) jiffies = %lu\n", jiffies);

[-- Attachment #3: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Alsa-devel mailing list
Alsa-devel@alsa-project.org
http://mailman.alsa-project.org/mailman/listinfo/alsa-devel

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Takashi Iwai]

At Tue, 11 Sep 2007 00:40:10 +0200,
Krzysztof Helt wrote:
> 
> From: Krzysztof Helt <krzysztof.h1 at wp.pl>
> 
> This patch replaces long msleeps in waiting loops
> with schedule_timeout() calls.
> 
> Signed-off-by: Krzysztof Helt <krzysztof.h1 at wp.pl>
> ---
> 
> This is done against tree with Rene's patches.
> 
> Regards,
> Krzysztof

Applied now.  Thanks.

Takashi

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Rene Herman]

On 09/11/2007 12:40 AM, Krzysztof Helt wrote:

> This patch replaces long msleeps in waiting loops
> with schedule_timeout() calls.

Hope you'll not detest me for getting on your case so much, but...

First, yes, attachments are good as far as I'm concerned, but could you make 
them plain text attachments? They are now Base64 which means that at least 
with my "casual mailer" (Thunderbird) I need to save the patch and can only 
then look at it.

Second -- not schedule_timeout_interruptible? A plain schedule_timeout won't 
work, as you first need to __set_current_state or it will not schedule. That 
is, schedule_timeout_interruptible or _uniterruptible and with the latter, 
the loop wouldn't make much sense anymore as it's just going to sleep for 
the full 250 ms directly.

Rene.

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Takashi Iwai]

At Tue, 11 Sep 2007 00:43:42 +0200,
Rene Herman wrote:
> 
> Second -- not schedule_timeout_interruptible? A plain schedule_timeout won't 
> work, as you first need to __set_current_state or it will not schedule. That 
> is, schedule_timeout_interruptible or _uniterruptible and with the latter, 
> the loop wouldn't make much sense anymore as it's just going to sleep for 
> the full 250 ms directly.

Ah, right.  I'm obviously too sleepy to review now...

Actually, the code with msleep() was basically OK.  If more finer
check is needed, it can simply use msleep(1).  The point is to use
timer_after_eq() instead of loop count for the precise timeouts.


thanks,

Takashi

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Takashi Iwai]

At Tue, 11 Sep 2007 01:08:07 +0200,
I wrote:
> 
> At Tue, 11 Sep 2007 00:43:42 +0200,
> Rene Herman wrote:
> > 
> > Second -- not schedule_timeout_interruptible? A plain schedule_timeout won't 
> > work, as you first need to __set_current_state or it will not schedule. That 
> > is, schedule_timeout_interruptible or _uniterruptible and with the latter, 
> > the loop wouldn't make much sense anymore as it's just going to sleep for 
> > the full 250 ms directly.
> 
> Ah, right.  I'm obviously too sleepy to review now...
> 
> Actually, the code with msleep() was basically OK.  If more finer
> check is needed, it can simply use msleep(1).  The point is to use
> timer_after_eq() instead of loop count for the precise timeouts.

Could you check whether the below patch works?


Takashi

diff -r 475132691bb1 isa/cs423x/cs4231_lib.c
--- a/isa/cs423x/cs4231_lib.c	Tue Sep 11 00:55:46 2007 +0200
+++ b/isa/cs423x/cs4231_lib.c	Tue Sep 11 01:15:17 2007 +0200
@@ -313,6 +313,7 @@ void snd_cs4231_mce_down(struct snd_cs42
 void snd_cs4231_mce_down(struct snd_cs4231 *chip)
 {
 	unsigned long flags;
+	unsigned long end_time;
 	int timeout;
 
 	snd_cs4231_busy_wait(chip);
@@ -344,28 +345,28 @@ void snd_cs4231_mce_down(struct snd_cs42
 	snd_printdd("(1) jiffies = %lu\n", jiffies);
 
 	/* check condition up to 250 ms */
-	timeout = msecs_to_jiffies(250);
+	end_time = jiffies + msecs_to_jiffies(250);
 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) &
 		CS4231_CALIB_IN_PROGRESS) {
 
-		if (timeout <= 0) {
+		if (time_after(jiffies, end_time)) {
 			snd_printk(KERN_ERR "mce_down - "
 					"auto calibration time out (2)\n");
 			return;
 		}
-		timeout = schedule_timeout(timeout);
+		msleep(1);
 	}
 
 	snd_printdd("(2) jiffies = %lu\n", jiffies);
 
 	/* check condition up to 100 ms */
-	timeout = msecs_to_jiffies(100);
+	end_time = jiffies + msecs_to_jiffies(100);
 	while (cs4231_inb(chip, CS4231P(REGSEL)) & CS4231_INIT) {
-		if (timeout <= 0) {
+		if (time_after(jiffies, end_time)) {
 			snd_printk(KERN_ERR "mce_down - auto calibration time out (3)\n");
 			return;
 		}
-		timeout = schedule_timeout(timeout);
+		msleep(1);
 	}
 
 	snd_printdd("(3) jiffies = %lu\n", jiffies);
diff -r 475132691bb1 sparc/cs4231.c
--- a/sparc/cs4231.c	Tue Sep 11 00:55:46 2007 +0200
+++ b/sparc/cs4231.c	Tue Sep 11 01:15:17 2007 +0200
@@ -401,6 +401,7 @@ static void snd_cs4231_mce_down(struct s
 static void snd_cs4231_mce_down(struct snd_cs4231 *chip)
 {
 	unsigned long flags;
+	unsigned long end_time;
 	int timeout;
 
 	spin_lock_irqsave(&chip->lock, flags);
@@ -431,30 +432,30 @@ static void snd_cs4231_mce_down(struct s
 	msleep(1);
 
 	/* check condition up to 250ms */
-	timeout = msecs_to_jiffies(250);
+	end_time = jiffies + msecs_to_jiffies(250);
 	while (snd_cs4231_in(chip, CS4231_TEST_INIT) &
 		CS4231_CALIB_IN_PROGRESS) {
 
 		spin_unlock_irqrestore(&chip->lock, flags);
-		if (timeout <= 0) {
+		if (time_after(jiffies, end_time)) {
 			snd_printk("mce_down - "
 				   "auto calibration time out (2)\n");
 			return;
 		}
-		timeout = schedule_timeout(timeout);
+		msleep(1);
 		spin_lock_irqsave(&chip->lock, flags);
 	}
 
 	/* check condition up to 100ms */
-	timeout = msecs_to_jiffies(100);
+	end_time = jiffies + msecs_to_jiffies(100);
 	while (__cs4231_readb(chip, CS4231U(chip, REGSEL)) & CS4231_INIT) {
 		spin_unlock_irqrestore(&chip->lock, flags);
-		if (timeout <= 0) {
+		if (time_after(jiffies, end_time)) {
 			snd_printk("mce_down - "
 				   "auto calibration time out (3)\n");
 			return;
 		}
-		timeout = schedule_timeout(timeout);
+		msleep(1);
 		spin_lock_irqsave(&chip->lock, flags);
 	}
 	spin_unlock_irqrestore(&chip->lock, flags);

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Rene Herman]

On 09/11/2007 01:17 AM, Takashi Iwai wrote:

>> Actually, the code with msleep() was basically OK.  If more finer
>> check is needed, it can simply use msleep(1).  The point is to use
>> timer_after_eq() instead of loop count for the precise timeouts.
> 
> Could you check whether the below patch works?

Works for me on ISA CS4231 (behind an AZT1605). Earlier Krzysztof suggested 
that 2 ms may be optimum, but I'll leave that upto him.

Acked-by: Rene Herman <rene.herman@gmail.com>

Zzz,
Rene

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Krzysztof Helt]

On Tue, 11 Sep 2007 02:40:34 +0200
Rene Herman <rene.herman@gmail.com> wrote:

> On 09/11/2007 01:17 AM, Takashi Iwai wrote:
> 
> Works for me on ISA CS4231 (behind an AZT1605). Earlier Krzysztof suggested 
> that 2 ms may be optimum, but I'll leave that upto him.
> 

The 2ms is optimal before the loop. Inside the loop the less the better.

Regards,
Krzysztof

Re: [PATCH] cs4231-lib: improved waiting after mce_down (2nd rev)

[Krzysztof Helt]

On 9/11/07, Takashi Iwai <tiwai@suse.de> wrote:
> At Tue, 11 Sep 2007 01:08:07 +0200,
> I wrote:
> >
> > Actually, the code with msleep() was basically OK.  If more finer
> > check is needed, it can simply use msleep(1).  The point is to use
> > timer_after_eq() instead of loop count for the precise timeouts.
>

The patch is ok. You may change this timer_after_eq and jiffies into
just number for the timeout (one pass will be 1ms) for simplicity (not
required).

The ad1848_lib have been using this broken method (schedule_timeout
with set_current_..). You can fix it as well or if the patch gets
applied, I can sync the ad1848_lib to use exactly the same waiting
loops. The ad1848 chip is slower then cs4231, so the granularity of
1ms hurts it less.

I want to have your patch applied as it is improvement (along with
Rene's patch) over the old code: it is simpler, less code and lower
(on average) delay on playback/record start.

Regards,
Krzysztof