[PATCH v2] mfd: arizona: Don't use regmap_read_poll_timeout

[PATCH v2] mfd: arizona: Don't use regmap_read_poll_timeout

[Charles Keepax]

Some Arizona CODECs have a small timing window where they will
NAK an I2C transaction if it happens before the boot done bit is
set. This can cause the read of the register containing the boot
done bit to fail until it is set. Since regmap_read_poll_timeout
will abort polling if a read fails it can't be reliably used to
poll the boot done bit over I2C.

Do a partial revert of ef84f885e037 ("mfd: arizona: Refactor
arizona_poll_reg"), removing the regmap_read_poll_timeout but
leaving the refactoring to make the arizona_poll_reg take more
sensible arguments.

Fixes: ef84f885e037 ("mfd: arizona: Refactor arizona_poll_reg")
Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
---

Changes since v1:
 - Explicitly include ktime header
 - Use USEC_PER_MSEC instead of a constant
 - Refactor the loop to not use a while (true)

Thanks,
Charles

 drivers/mfd/arizona-core.c | 29 ++++++++++++++++++++---------
 1 file changed, 20 insertions(+), 9 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 83f1c5a516d99..fe0c151d9d9d7 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -24,6 +24,7 @@
 #include <linux/regulator/consumer.h>
 #include <linux/regulator/machine.h>
 #include <linux/slab.h>
+#include <linux/ktime.h>
 #include <linux/platform_device.h>
 
 #include <linux/mfd/arizona/core.h>
@@ -240,18 +241,28 @@ static int arizona_poll_reg(struct arizona *arizona,
 			    int timeout_ms, unsigned int reg,
 			    unsigned int mask, unsigned int target)
 {
+	ktime_t timeout = ktime_add_us(ktime_get(), timeout_ms * USEC_PER_MSEC);
 	unsigned int val = 0;
-	int ret;
+	int ret = regmap_read(arizona->regmap, reg, &val);
 
-	ret = regmap_read_poll_timeout(arizona->regmap,
-				       reg, val, ((val & mask) == target),
-				       ARIZONA_REG_POLL_DELAY_US,
-				       timeout_ms * 1000);
-	if (ret)
-		dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n",
-			reg, val);
+	while (ktime_compare(ktime_get(), timeout) <= 0) {
+		if ((val & mask) == target)
+			return 0;
 
-	return ret;
+		usleep_range(ARIZONA_REG_POLL_DELAY_US / 2,
+			     ARIZONA_REG_POLL_DELAY_US);
+
+		ret = regmap_read(arizona->regmap, reg, &val);
+	}
+
+	if (ret) {
+		dev_err(arizona->dev, "Failed polling reg 0x%x: %d\n",
+			reg, ret);
+		return ret;
+	}
+
+	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
+	return -ETIMEDOUT;
 }
 
 static int arizona_wait_for_boot(struct arizona *arizona)
-- 
2.11.0

Re: [PATCH v2] mfd: arizona: Don't use regmap_read_poll_timeout

[Lee Jones]

On Mon, 04 Jun 2018, Charles Keepax wrote:

> Some Arizona CODECs have a small timing window where they will
> NAK an I2C transaction if it happens before the boot done bit is
> set. This can cause the read of the register containing the boot
> done bit to fail until it is set. Since regmap_read_poll_timeout
> will abort polling if a read fails it can't be reliably used to
> poll the boot done bit over I2C.
> 
> Do a partial revert of ef84f885e037 ("mfd: arizona: Refactor
> arizona_poll_reg"), removing the regmap_read_poll_timeout but
> leaving the refactoring to make the arizona_poll_reg take more
> sensible arguments.
> 
> Fixes: ef84f885e037 ("mfd: arizona: Refactor arizona_poll_reg")
> Signed-off-by: Charles Keepax <ckeepax@opensource.cirrus.com>
> ---
> 
> Changes since v1:
>  - Explicitly include ktime header
>  - Use USEC_PER_MSEC instead of a constant
>  - Refactor the loop to not use a while (true)
> 
> Thanks,
> Charles
> 
>  drivers/mfd/arizona-core.c | 29 ++++++++++++++++++++---------
>  1 file changed, 20 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 83f1c5a516d99..fe0c151d9d9d7 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -24,6 +24,7 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/regulator/machine.h>
>  #include <linux/slab.h>
> +#include <linux/ktime.h>
>  #include <linux/platform_device.h>
>  
>  #include <linux/mfd/arizona/core.h>
> @@ -240,18 +241,28 @@ static int arizona_poll_reg(struct arizona *arizona,
>  			    int timeout_ms, unsigned int reg,
>  			    unsigned int mask, unsigned int target)
>  {
> +	ktime_t timeout = ktime_add_us(ktime_get(), timeout_ms * USEC_PER_MSEC);
>  	unsigned int val = 0;
> -	int ret;
> +	int ret = regmap_read(arizona->regmap, reg, &val);

I'm not a fan of this type of initialisation.

Also, what's the point of assigning 'ret' if you never check it?

> -	ret = regmap_read_poll_timeout(arizona->regmap,
> -				       reg, val, ((val & mask) == target),
> -				       ARIZONA_REG_POLL_DELAY_US,
> -				       timeout_ms * 1000);
> -	if (ret)
> -		dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n",
> -			reg, val);
> +	while (ktime_compare(ktime_get(), timeout) <= 0) {
> +		if ((val & mask) == target)
> +			return 0;
>  
> -	return ret;
> +		usleep_range(ARIZONA_REG_POLL_DELAY_US / 2,
> +			     ARIZONA_REG_POLL_DELAY_US);
> +
> +		ret = regmap_read(arizona->regmap, reg, &val);
> +	}
> +
> +	if (ret) {
> +		dev_err(arizona->dev, "Failed polling reg 0x%x: %d\n",
> +			reg, ret);
> +		return ret;
> +	}
> +
> +	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
> +	return -ETIMEDOUT;
>  }
>  
>  static int arizona_wait_for_boot(struct arizona *arizona)

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH v2] mfd: arizona: Don't use regmap_read_poll_timeout

[Charles Keepax]

On Mon, Jun 18, 2018 at 07:27:54AM +0100, Lee Jones wrote:
> On Mon, 04 Jun 2018, Charles Keepax wrote:
> > @@ -240,18 +241,28 @@ static int arizona_poll_reg(struct arizona *arizona,
> >  			    int timeout_ms, unsigned int reg,
> >  			    unsigned int mask, unsigned int target)
> >  {
> > +	ktime_t timeout = ktime_add_us(ktime_get(), timeout_ms * USEC_PER_MSEC);
> >  	unsigned int val = 0;
> > -	int ret;
> > +	int ret = regmap_read(arizona->regmap, reg, &val);
> 
> I'm not a fan of this type of initialisation.
> 
> Also, what's the point of assigning 'ret' if you never check it?
> 

It could get checked if the timeout value is very small causing
the while loop to be skipped and almost certainly will cause
un-initialised warnings without it.

I can move it to its own line if you prefer? Would you want a
comment as well?

Thanks,
Charles

Re: [PATCH v2] mfd: arizona: Don't use regmap_read_poll_timeout

[Lee Jones]

On Mon, 18 Jun 2018, Charles Keepax wrote:

> On Mon, Jun 18, 2018 at 07:27:54AM +0100, Lee Jones wrote:
> > On Mon, 04 Jun 2018, Charles Keepax wrote:
> > > @@ -240,18 +241,28 @@ static int arizona_poll_reg(struct arizona *arizona,
> > >  			    int timeout_ms, unsigned int reg,
> > >  			    unsigned int mask, unsigned int target)
> > >  {
> > > +	ktime_t timeout = ktime_add_us(ktime_get(), timeout_ms * USEC_PER_MSEC);
> > >  	unsigned int val = 0;
> > > -	int ret;
> > > +	int ret = regmap_read(arizona->regmap, reg, &val);
> > 
> > I'm not a fan of this type of initialisation.
> > 
> > Also, what's the point of assigning 'ret' if you never check it?
> > 
> 
> It could get checked if the timeout value is very small causing
> the while loop to be skipped and almost certainly will cause
> un-initialised warnings without it.
> 
> I can move it to its own line if you prefer? Would you want a
> comment as well?

Yes please.

-- 
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog