Ashwin Chaugule wrote:
..Only to be re substituted by the macro all over again. So, there really is no value in replacing this with a macro. Also, readx_relaxed_poll_timeout() has a usleep(), which will kill all the optimization here. Its also horribly wrong in this context.
The alternative readx_poll_timeout_atomic() has a udelay() but it also adds way more conditionals than this code. So, there is no need to change things for the sake of cosmetic reasons.
This is not a cosmetic change. Calling a built-in macro instead of re-inventing the wheel is not cosmetic. That's like saying that people shouldn't bother using min() or max() because they can just calculate the minimum and maximum themselves. The macros exist for a reason, and people should be using them when applicable.
Also, the number of conditionals is not really much of an issue, I think.
The while loop has two conditionals:
1) The while-expression !ktime_after(ktime_get(), next_deadline) 2) The if-statement readw_relaxed(&generic_comm_base->status) & PCC_CMD_COMPLETE
readl_relaxed_poll_timeout_atomic() has four:
1) if (cond) break; 2) if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) 3) if (delay_us) udelay(delay_us); 4) (cond) ? 0 : -ETIMEDOUT; \
The third one is compiled-out because delay_us is a constant. The fourth test is to handle the race condition between timeout and success. If we time out, but the register value changes at the last microsecond, then we report success anyway.
And since this is a macro instead of a function, the code is generally optimized better. The compiler typically can optimize macros better than functions, so there's still a chance that the macro version will be more optimized than the function anyway.
Finally, there's the convention of using a built-in macro/function over hand-coding something, even if it's not quite as optimized. I don't think check_pcc_chan() is that time-critical that a single additional conditional is really a problem. This while-loop really does re-invent the wheel.
Anyway, I don't want to belabor this point. The decision is Rafael's now.