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