[PATCH] regmap: add iopoll-like polling macro

[PATCH] regmap: add iopoll-like polling macro

[Philipp Zabel]

This patch adds a macro regmap_read_poll_timeout that works similar
to the readx_poll_timeout defined in linux/iopoll.h, except that this
can also return the error value returned by a failed regmap_read.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
---
 include/linux/regmap.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3dc08ce..26914df 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -95,6 +95,45 @@ struct reg_sequence {
 #define	regmap_fields_force_update_bits(field, id, mask, val) \
 	regmap_fields_update_bits_base(field, id, mask, val, NULL, false, true)
 
+/**
+ * regmap_read_poll_timeout - Poll until a condition is met or a timeout occurs
+ * @map: Regmap to read from
+ * @addr: Address to poll
+ * @val: Unsigned integer variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @sleep_us: Maximum time to sleep between reads in us (0
+ *            tight-loops).  Should be less than ~20ms since usleep_range
+ *            is used (see Documentation/timers/timers-howto.txt).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout or the regmap_read
+ * error return value in case of a error read. In the two former cases,
+ * the last read value at @addr is stored in @val. Must not be called
+ * from atomic context if sleep_us or timeout_us are used.
+ *
+ * This is modelled after the readx_poll_timeout macros in linux/iopoll.h.
+ */
+#define regmap_read_poll_timeout(map, addr, val, cond, sleep_us, timeout_us) \
+({ \
+	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+	int ret; \
+	might_sleep_if(sleep_us); \
+	for (;;) { \
+		ret = regmap_read((map), (addr), &(val)); \
+		if (ret) \
+			break; \
+		if (cond) \
+			break; \
+		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+			ret = regmap_read((map), (addr), &(val)); \
+			break; \
+		} \
+		if (sleep_us) \
+			usleep_range((sleep_us >> 2) + 1, sleep_us); \
+	} \
+	ret ?: ((cond) ? 0 : -ETIMEDOUT); \
+})
+
 #ifdef CONFIG_REGMAP
 
 enum regmap_endian {
-- 
2.8.1

Re: [PATCH] regmap: add iopoll-like polling macro

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 491 bytes --]

On Wed, Jul 06, 2016 at 04:19:41PM +0200, Philipp Zabel wrote:

> This patch adds a macro regmap_read_poll_timeout that works similar
> to the readx_poll_timeout defined in linux/iopoll.h, except that this
> can also return the error value returned by a failed regmap_read.

Please make this a proper function.

> +		if (sleep_us) \
> +			usleep_range((sleep_us >> 2) + 1, sleep_us); \

Just write / 4 rather than a shift, the compiler should do the right
thing and it's easier for readers.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regmap: add iopoll-like polling macro

[Philipp Zabel]

Hi Mark,

thank you for the comments.

Am Donnerstag, den 07.07.2016, 11:42 +0200 schrieb Mark Brown:
> On Wed, Jul 06, 2016 at 04:19:41PM +0200, Philipp Zabel wrote:
> 
> > This patch adds a macro regmap_read_poll_timeout that works similar
> > to the readx_poll_timeout defined in linux/iopoll.h, except that this
> > can also return the error value returned by a failed regmap_read.
> 
> Please make this a proper function.

I can't, the condition has to be evaluated inside the loop. This is
basically a poor man's function template.

> > +		if (sleep_us) \
> > +			usleep_range((sleep_us >> 2) + 1, sleep_us); \
> 
> Just write / 4 rather than a shift, the compiler should do the right
> thing and it's easier for readers.

Ok. Since this is copied from the readx_poll_timeout macro in
include/linux/iopoll.h, I should probably suggest the same change there.

regards
Philipp

Re: [PATCH] regmap: add iopoll-like polling macro

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 727 bytes --]

On Thu, Jul 07, 2016 at 12:01:43PM +0200, Philipp Zabel wrote:
> Am Donnerstag, den 07.07.2016, 11:42 +0200 schrieb Mark Brown:
> > On Wed, Jul 06, 2016 at 04:19:41PM +0200, Philipp Zabel wrote:

> > > This patch adds a macro regmap_read_poll_timeout that works similar
> > > to the readx_poll_timeout defined in linux/iopoll.h, except that this
> > > can also return the error value returned by a failed regmap_read.

> > Please make this a proper function.

> I can't, the condition has to be evaluated inside the loop. This is
> basically a poor man's function template.

Given that the condition is always going to be essentially the same one
checking that (read & mask) == value we could just parameterize it
couldn't we?

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Re: [PATCH] regmap: add iopoll-like polling macro

[Philipp Zabel]

Am Freitag, den 08.07.2016, 16:39 +0200 schrieb Mark Brown:
> On Thu, Jul 07, 2016 at 12:01:43PM +0200, Philipp Zabel wrote:
> > Am Donnerstag, den 07.07.2016, 11:42 +0200 schrieb Mark Brown:
> > > On Wed, Jul 06, 2016 at 04:19:41PM +0200, Philipp Zabel wrote:
> 
> > > > This patch adds a macro regmap_read_poll_timeout that works similar
> > > > to the readx_poll_timeout defined in linux/iopoll.h, except that this
> > > > can also return the error value returned by a failed regmap_read.
> 
> > > Please make this a proper function.
> 
> > I can't, the condition has to be evaluated inside the loop. This is
> > basically a poor man's function template.
> 
> Given that the condition is always going to be essentially the same one
> checking that (read & mask) == value we could just parameterize it
> couldn't we?

The iopoll macros also allow comparisons like < > !=, more complicated
expressions, or even function calls.

Granted, the only existing example I am aware of is
drivers/dma/qcom/hidma_ll.c, which calls a static function in
readl_poll_wait_timeout, that evaluates to ((read == 1) || (read == 2)).

There's a similar condition in rt5659_headset_detect, although that may
be dismissed as it's using snd_soc_read and variable delays:
        while (i < 5) {
                msleep(sleep_time[i]);
                val = snd_soc_read(codec, RT5659_EJD_CTRL_2) & 0x0003;
                i++;
                if (val == 0x1 || val == 0x2 || val == 0x3)
                        break;
        }

A potential user for an inequality condition is
drivers/media/tuners/it913x.c, which has a loop where the condition is
(read != 0):
                #define TIMEOUT 50
                timeout = jiffies + msecs_to_jiffies(TIMEOUT);
                while (!time_after(jiffies, timeout)) {
                        ret = regmap_read(dev->regmap, 0x80ec82, &utmp);
                        if (ret)
                                goto err;

                        if (utmp)
                                break;
                }

Even if by far the most common cases can be covered as you suggest,
I would find it useful to keep this the same as the other
readx_poll_wait_timeout variants.

regards
Philipp

Re: [PATCH] regmap: add iopoll-like polling macro

[Philipp Zabel]

Hi Mark,

Am Freitag, den 08.07.2016, 18:26 +0200 schrieb Philipp Zabel:
> Am Freitag, den 08.07.2016, 16:39 +0200 schrieb Mark Brown:
> > On Thu, Jul 07, 2016 at 12:01:43PM +0200, Philipp Zabel wrote:
> > > Am Donnerstag, den 07.07.2016, 11:42 +0200 schrieb Mark Brown:
> > > > On Wed, Jul 06, 2016 at 04:19:41PM +0200, Philipp Zabel wrote:
> > 
> > > > > This patch adds a macro regmap_read_poll_timeout that works similar
> > > > > to the readx_poll_timeout defined in linux/iopoll.h, except that this
> > > > > can also return the error value returned by a failed regmap_read.
> > 
> > > > Please make this a proper function.
> > 
> > > I can't, the condition has to be evaluated inside the loop. This is
> > > basically a poor man's function template.
> > 
> > Given that the condition is always going to be essentially the same one
> > checking that (read & mask) == value we could just parameterize it
> > couldn't we?
> 
> The iopoll macros also allow comparisons like < > !=, more complicated
> expressions, or even function calls.
> 
> Granted, the only existing example I am aware of is
> drivers/dma/qcom/hidma_ll.c, which calls a static function in
> readl_poll_wait_timeout, that evaluates to ((read == 1) || (read == 2)).
> 
> There's a similar condition in rt5659_headset_detect, although that may
> be dismissed as it's using snd_soc_read and variable delays:
>         while (i < 5) {
>                 msleep(sleep_time[i]);
>                 val = snd_soc_read(codec, RT5659_EJD_CTRL_2) & 0x0003;
>                 i++;
>                 if (val == 0x1 || val == 0x2 || val == 0x3)
>                         break;
>         }
> 
> A potential user for an inequality condition is
> drivers/media/tuners/it913x.c, which has a loop where the condition is
> (read != 0):
>                 #define TIMEOUT 50
>                 timeout = jiffies + msecs_to_jiffies(TIMEOUT);
>                 while (!time_after(jiffies, timeout)) {
>                         ret = regmap_read(dev->regmap, 0x80ec82, &utmp);
>                         if (ret)
>                                 goto err;
> 
>                         if (utmp)
>                                 break;
>                 }
> 
> Even if by far the most common cases can be covered as you suggest,
> I would find it useful to keep this the same as the other
> readx_poll_wait_timeout variants.

Any comments on this? If I can't convince you to keep the macro, I could
change it into a function:

int regmap_poll_bits_wait_timeout(struct regmap *map, unsigned int reg,
				  unsigned int mask, unsigned int val,
				  unsigned long sleep, u64 timeout);

With the changed name there would be no expectation for it to work the
same as readx_poll_wait_timeout, and the mask and val condition is in
the same place as mask and val for regmap_update_bits.

regards
Philipp

Re: [PATCH] regmap: add iopoll-like polling macro

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 246 bytes --]

On Wed, Jul 13, 2016 at 09:37:09AM +0200, Philipp Zabel wrote:

> Any comments on this? If I can't convince you to keep the macro, I could
> change it into a function:

No, I've not looked at your mail yet.  Please don't send content free
pings.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

Applied "regmap: add iopoll-like polling macro" to the regmap tree

[Mark Brown]

The patch

   regmap: add iopoll-like polling macro

has been applied to the regmap tree at

   git://git.kernel.org/pub/scm/linux/kernel/git/broonie/regmap.git 

All being well this means that it will be integrated into the linux-next
tree (usually sometime in the next 24 hours) and sent to Linus during
the next merge window (or sooner if it is a bug fix), however if
problems are discovered then the patch may be dropped or reverted.  

You may get further e-mails resulting from automated or manual testing
and review of the tree, please engage with people reporting problems and
send followup patches addressing any issues that are reported if needed.

If any updates are required or you are submitting further changes they
should be sent as incremental updates against current git, existing
patches will not be replaced.

Please add any relevant lists and maintainers to the CCs when replying
to this mail.

Thanks,
Mark

>From 08188ba8822163922e6b9c600095d21ce02f6a84 Mon Sep 17 00:00:00 2001
From: Philipp Zabel <p.zabel@pengutronix.de>
Date: Wed, 6 Jul 2016 16:19:41 +0200
Subject: [PATCH] regmap: add iopoll-like polling macro

This patch adds a macro regmap_read_poll_timeout that works similar
to the readx_poll_timeout defined in linux/iopoll.h, except that this
can also return the error value returned by a failed regmap_read.

Signed-off-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Mark Brown <broonie@kernel.org>
---
 include/linux/regmap.h | 39 +++++++++++++++++++++++++++++++++++++++
 1 file changed, 39 insertions(+)

diff --git a/include/linux/regmap.h b/include/linux/regmap.h
index 3dc08ce15426..26914dfcc9fc 100644
--- a/include/linux/regmap.h
+++ b/include/linux/regmap.h
@@ -95,6 +95,45 @@ struct reg_sequence {
 #define	regmap_fields_force_update_bits(field, id, mask, val) \
 	regmap_fields_update_bits_base(field, id, mask, val, NULL, false, true)
 
+/**
+ * regmap_read_poll_timeout - Poll until a condition is met or a timeout occurs
+ * @map: Regmap to read from
+ * @addr: Address to poll
+ * @val: Unsigned integer variable to read the value into
+ * @cond: Break condition (usually involving @val)
+ * @sleep_us: Maximum time to sleep between reads in us (0
+ *            tight-loops).  Should be less than ~20ms since usleep_range
+ *            is used (see Documentation/timers/timers-howto.txt).
+ * @timeout_us: Timeout in us, 0 means never timeout
+ *
+ * Returns 0 on success and -ETIMEDOUT upon a timeout or the regmap_read
+ * error return value in case of a error read. In the two former cases,
+ * the last read value at @addr is stored in @val. Must not be called
+ * from atomic context if sleep_us or timeout_us are used.
+ *
+ * This is modelled after the readx_poll_timeout macros in linux/iopoll.h.
+ */
+#define regmap_read_poll_timeout(map, addr, val, cond, sleep_us, timeout_us) \
+({ \
+	ktime_t timeout = ktime_add_us(ktime_get(), timeout_us); \
+	int ret; \
+	might_sleep_if(sleep_us); \
+	for (;;) { \
+		ret = regmap_read((map), (addr), &(val)); \
+		if (ret) \
+			break; \
+		if (cond) \
+			break; \
+		if (timeout_us && ktime_compare(ktime_get(), timeout) > 0) { \
+			ret = regmap_read((map), (addr), &(val)); \
+			break; \
+		} \
+		if (sleep_us) \
+			usleep_range((sleep_us >> 2) + 1, sleep_us); \
+	} \
+	ret ?: ((cond) ? 0 : -ETIMEDOUT); \
+})
+
 #ifdef CONFIG_REGMAP
 
 enum regmap_endian {
-- 
2.8.1