From mboxrd@z Thu Jan  1 00:00:00 1970
From: arnd@arndb.de (Arnd Bergmann)
Date: Wed, 01 Oct 2014 10:25:33 +0200
Subject: [PATCH v4 1/2] iopoll: Introduce memory-mapped IO polling macros
In-Reply-To: <1412126893-15796-2-git-send-email-mitchelh@codeaurora.org>
References: <1412126893-15796-1-git-send-email-mitchelh@codeaurora.org>
 <1412126893-15796-2-git-send-email-mitchelh@codeaurora.org>
Message-ID: <23910474.oJQUXZsx4W@wuerfel>
To: linux-arm-kernel@lists.infradead.org
List-Id: linux-arm-kernel.lists.infradead.org

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