-----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).
>
> 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)?
[1] http://lists.denx.de/pipermail/u-boot/2015-December/239468.html
Regards,
Mateusz
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
iQEcBAEBCAAGBQJWn/YnAAoJELvtohmVtQzBMFMIAITNu+ORG3trzOpc3xaM2QXC
4WZG89SDkM/KW26LpZEj5I/aARr5rPwO637zCfc7Vf6k1VX1CohdRPv7E3wiiOQ3
Lt6NL6yyLQfIzQkFQb5373ao7GbuyKUqvsbsQkd2TGDUTtEgo9tRWLtpt9wTstMT
H0YK2uNb9Zg6pJ6Z/0xCLua723DXcSXPgx8PV2Wbo3nR3BIlz70HYLHKvAMw2O2w
phSX2/TIx7LjCUw4lvIfGJXapnZV3z9hmCOLsHCPEZAbcE5MYKqX/t7GJu3reuao
j9MzZzpxr6CTzdavPhWxcpsNUwVsg7Q9KOIq7DQMA5qoW6EKLeOSKdr6FxKReFg=
=fVR7
-----END PGP SIGNATURE-----
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
