[PATCH v4 1/2] iopoll: Introduce memory-mapped IO polling macros

On Tuesday 30 September 2014 18:28:12 Mitchel Humpherys wrote:

+ */
+#define readl_poll_timeout(addr, val, cond, sleep_us, timeout_us) \
+({ \
+       ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+       might_sleep_if(timeout_us); \

Does it make sense to call this with timeout_us = 0?

+       for (;;) { \
+               (val) = readl(addr); \
+               if (cond) \
+                       break; \
+               if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+                       (val) = readl(addr); \
+                       break; \
+               } \
+               if (sleep_us) \
+                       usleep_range(DIV_ROUND_UP(sleep_us, 4), sleep_us); \
+       } \
+       (cond) ? 0 : -ETIMEDOUT; \
+})

I think it would be better to tie the 'range' argument to the timeout. Also
doing a division seems expensive here.

+/**
+ * readl_poll_timeout_atomic - Periodically poll an address until a condition is met or a timeout occurs
+ * @addr: Address to poll
+ * @val: Variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @max_reads: Maximum number of reads before giving up
+ * @time_between_us: Time to udelay() between successive reads
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout.
+ */
+#define readl_poll_timeout_atomic(addr, val, cond, max_reads, time_between_us) \
+({ \
+       int count; \
+       for (count = (max_reads); count > 0; count--) { \
+               (val) = readl(addr); \
+               if (cond) \
+                       break; \
+               udelay(time_between_us); \
+       } \
+       (cond) ? 0 : -ETIMEDOUT; \
+})

udelay has a large variability, I think it would be better to also use
ktime_compare here and make the interface the same as the other one.
You might want to add a warning if someone tries to pass more than a few
microseconds as the timeout.

More generally speaking, using 'readl' seems fairly specific. I suspect
that we'd have to add the entire range of accessors over time if this
catches on: readb, readw, readq, readb_relaxed, readw_relaxed, readl_relaxed,
readq_relaxed, ioread8, ioread16, ioread16be, ioread32, ioread32be,
inb, inb_p, inw, inw_p, inw, inl, inl_p, and possibly more of those.

Would it make sense to pass that operation as an argument?

	Arnd