[alsa-devel] [PATCH] ad1848 and cs4231 busy loop replacement

[Krzysztof Helt]

On Mon, 10 Sep 2007 15:13:24 +0200
Rene Herman <rene.herman at gmail.com> wrote:

> On 09/10/2007 02:42 PM, Krzysztof Helt wrote:
> 
> > On 9/10/07, Rene Herman <rene.herman at gmail.com> wrote:
> 
> >> Well, no, sorry, but I consider that to be completely breaking the logic of
> >> the code.
> > 
> > No I changed the logic of this code not to wait for specifically for
> > callibration "start" but into calibration "under way".
> 
> No, waiting for calibration the be under way is what my 0/1 ms does. You are 
> waiting for it to be nearly done, which is complete nonsense. One line below 
> we are waiting for 250 ms (generally with _one_ pass through the loop -- we 
> only wake up through signals) anyway!
> 

I don't buy your argument that it is complete nonsense.
I want some merit argument like:
your code uses more memory/cpu, 
your code is longer,
your code drives hardware into undefined states,
your code push hardware outside hardware limits,
your code is slower/adds more delay.

Also, I don't think mimicking the hardware behaviour in driver code is 
correct paradigm. I think that correct way is to _drive_ the hardware. If the
hardware must be taken from state A to state C and goes through state B
, the driver may wait only for the C completely ignoring the B if the B
is irrelevant (no need to set or get something from the hardware until the C).

Another thing I believe is that driver should be "CPU friendly" means it should
gives back control to the CPU every time it knows the hardware is busy
and nothing can be done for some known period of time. That's why I don't
want to use all these m/udelay functions. They make CPU busy. Sometimes
they are must have, sometimes not. If one knows that it must wait at least
7ms why it should wait 1ms than makes another 6ms inside the loop?

Actually, if you look into the lower delay argument, the waiting loop after 
the change is inefficient. The whole calibration takes 2-3ms at the highest 
rate, but one has to wait in 10ms blocks. In order to lower this delay one 
must change it to some finer way of returning time to the CPU (cpu_relax()
or schedule_timeout() - like ad1848). If it is done, the loop will make 
thousands of iterations every few miliseconds. Taking your argument 
that loop should be passed in the least possible iterations (you wrote 
"preferably one pass"), the only way is to wait longer before entering the loop.

> 
> No, we are arguing maintaining code. Do not obscure the code flow for no 
> reason. Fix your logic or (for what it's worth) I am going to NAK the change.
> 

The code does not be obscured with one simple comment before msleep: 
/* wait till calibration is nearly done */

Also comments like "my way or highway" (NAK) won't make you many friends.

Krzysztof