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

Arnd Bergmann
2014-10-01T08:25:33Z

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