On Mon, 10 Sep 2007, Rene Herman wrote:
When the ad1848/cs2431 is first being initialized, auto-calibration may not be set causing a timeout waiting for it in snd_ad1848/cs4231_mce_down().
This has no dire consequences other than an alarming printk, but since what we need to wait for is for the calibration to _finish_, let's just check for that instead.
The early chips need a slight delay (as commented -- 5 sample periods) to be sure that _if_ calibration is going to happen, it has started when we check While the CS4231A datasheet implies it'll happen immediately on downing MCE, some testing is showing that there's a window there as well, so just do the delay everywhere.
I don't think this code is doing what you think it does.
First, you call msleep(1) while holding reg_lock with interrupts disabled. That's sleeping while atomic. You should drop the lock first, or use mdelay().
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.
This would explain what is happening when Krzysztof said, "The waiting loop was passed 340 times after mce was down with msleep(1)." 340 loops at 250 ms per loop is 85 seconds. That would mean a call to hw_params() takes 85 seconds, and open(), with three mce_downs, would take over four minutes!
I looked at the ad1848 datasheet, and it says the auto-calibration should take about 384 samples (or ~128, which I think is the time it takes ACI to go low when auto-calibration is not on). That would be 70 ms at the high end and typically more like 3 ms. A 250 ms polling interval seems to be quite long. That would be at least 3/4 second for open(), and 1/4 second for hw_params(). With OSS emulation calling hw_params() many times, all the extra delay would be easily noticeable. Of course you do not notice it now because you are not waiting 250 ms, but busy looping.
The wait for the init bit to be off after the check for ACI seems unneeded too. snd_ad1848_in(), called while waiting for ACI to go low, does the exact same thing when it calls snd_ad1848_wait(). So the INIT bit is already off at this point.
Here is a patch to fix all these problems and simplify the code in the process. I picked a value of 3 ms for the polling interval. Obviously, there is a trade-off in selecting the value. The smaller the delay, the less wasted time from when ACI goes low to when it is detected. But it also means more iterations of the loop, and thus more scheduling and interrupt disabling.
I think a good way to pick a value, is to try select the smallest value such that 90% of the time the loop will finish on the first check. It seems like 3 or maybe 4 would do that.
If you're not concerned much about efficiency, and just want to detect ACI low as soon as possible, you'd use 1 ms. Or if you want to get more complicated, delay 3 ms on the first iteration, and 1 ms thereafter. Or to be even smarter than that, have a lookup table from the sample rate setting to the delay value, so that the delay can be larger for slower sampling rates. But that hardly seems worth the effort.