Hi Matueuz, On 20 January 2016 at 14:03, Mateusz Kulikowski <mateusz.kulikow...@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.kulikow...@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 _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot