Hi Matueuz,
On 20 January 2016 at 14:03, Mateusz Kulikowski mateusz.kulikowski@gmail.com wrote:
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA256
Hi,
On 20.01.2016 05:34, Simon Glass wrote: [...]
On 27 December 2015 at 10:28, Mateusz Kulikowski mateusz.kulikowski@gmail.com wrote:
Add function to poll register waiting for specific bit(s). Similar functions are implemented in few drivers - they are almost identical and can be generalized.
[...]
Sorry I only just saw this, but thought I'd make a few comments.
Nooo, I was expecting at least this to be merged during this merge window :)
[...]
- @param prefix Prefix added to timeout messagge (message visible only
with debug enabled)
- @param reg Register that will be read (using readl())
- @param mask Bit(s) of register that must be active
- @param set Selects wait condition (bit set or clear)
- @param timeout Timeout (in miliseconds)
- @param breakable Enables CTRL-C interruption
- @return 0 on success, -ETIMEDOUT or -EINTR on failure
- */
+static inline int wait_for_bit(const char *prefix, const u32 *reg,
const u32 mask, const bool set,const unsigned int timeout,timeout_ms would be more obvious
This may be a good idea to make it more foolproof -
@trini: Will v4 with small change like that delay merging this series into mainline?
const bool breakable)Wow this is a pretty big inline function.
I personally probably could just drop inline and leave "static" but still keep it in header (so it may not be inlined), but it would probably violate some unwritten holy rules :)
First version was compiled into object file, but then either it would require extra config option, or would pollute rodata of all boards (which is bad).
If you drop the string the rodata add-on (presumably due to the gcc bug) would be tiny, so I don't think it would need a Kconfig.
Do you need the 'prefix' parameter? It seems that the callers print messages anyway. How about adding a flags word for @set and @breakable? Those params could then be combined, and you end up with 4 parameters instead of 6.
I prefer to keep it as is (for now).
This function is supposed to be drop-in replacement for four almost the same functions in drivers (dwc2, ohci-lpc..., ehci-mx6 and zynq_gem).
My intent was to keep all changes as small as possible so I would not cause regressions, but will make some people happy.
As for argument count - there was already request to add new feature [1], which is nice (I appended it to my task queue), so I can rework it a bit later (and perhaps use it in even more places where it would be useful).
As long as this function is inlined - argument count doesn't matter that much imo - as long as one remembers argument order or has smart IDE that does it for him.
+{
u32 val;unsigned long start = get_timer(0);while (1) {val = readl(reg);if (!set)val = ~val;if ((val & mask) == mask)return 0;if (get_timer(start) > timeout)break;if (breakable && ctrlc()) {puts("Abort\n");This is bad if used from drivers. We try not to output things. It it necessary?
Same arguments as above apply.
Although I agree that in future it may be useful not to have puts here.
Is it ok with you (timeout -> timeout_ms if possible I'll do now, rest + [1] in future)?
Please go ahead, you already have a review by Tom. My comment are just ideas.
[1] http://lists.denx.de/pipermail/u-boot/2015-December/239468.html
Regards, Mateusz
Regards, Simon