[PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable

[PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable

[Charles Keepax]

arizona_poll_reg already returns ETIMEDOUT if we don't see the expected
register changes before the time out, so remove pointless local setting of
ETIMEDOUT.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index b6d4bc6..e00f577 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -342,10 +342,8 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
 	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_FLL1_CLOCK_OK_STS,
 			       ARIZONA_FLL1_CLOCK_OK_STS);
-	if (ret) {
-		ret = -ETIMEDOUT;
+	if (ret)
 		goto err_fll;
-	}
 
 	ret = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, 0x0144);
 	if (ret) {
@@ -407,11 +405,9 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
 
 	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
 			       ARIZONA_WSEQ_BUSY, 0);
-	if (ret) {
+	if (ret)
 		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
 			     ARIZONA_WSEQ_ABORT);
-		ret = -ETIMEDOUT;
-	}
 
 err:
 	err = arizona_disable_freerun_sysclk(arizona, &state);
-- 
2.1.4

[PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex

[Charles Keepax]

Register addresses are normally displayed in hex throughout the Arizona
driver. Update the arizona_poll_reg function to follow this convention.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index e00f577..4cb34c3 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -245,7 +245,7 @@ static int arizona_poll_reg(struct arizona *arizona,
 	for (i = 0; i < timeout; i++) {
 		ret = regmap_read(arizona->regmap, reg, &val);
 		if (ret != 0) {
-			dev_err(arizona->dev, "Failed to read reg %u: %d\n",
+			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
 				reg, ret);
 			continue;
 		}
@@ -256,7 +256,7 @@ static int arizona_poll_reg(struct arizona *arizona,
 		usleep_range(1000, 5000);
 	}
 
-	dev_err(arizona->dev, "Polling reg %u timed out: %x\n", reg, val);
+	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
 	return -ETIMEDOUT;
 }
 
-- 
2.1.4

[PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds

[Charles Keepax]

Currently, we specify the timeout in terms of the number of polls but it
is more clear from a user of the functions perspective to specify the
timeout directly in milliseconds, as such update the function to these new
semantics.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c | 17 +++++++++++------
 1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 4cb34c3..ae4cdc4 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -235,14 +235,18 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
+#define ARIZONA_REG_POLL_DELAY_MS 5
+
 static int arizona_poll_reg(struct arizona *arizona,
-			    int timeout, unsigned int reg,
+			    int timeout_ms, unsigned int reg,
 			    unsigned int mask, unsigned int target)
 {
+	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
+			      ARIZONA_REG_POLL_DELAY_MS;
 	unsigned int val = 0;
 	int ret, i;
 
-	for (i = 0; i < timeout; i++) {
+	for (i = 0; i < npolls; i++) {
 		ret = regmap_read(arizona->regmap, reg, &val);
 		if (ret != 0) {
 			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
@@ -253,7 +257,8 @@ static int arizona_poll_reg(struct arizona *arizona,
 		if ((val & mask) == target)
 			return 0;
 
-		usleep_range(1000, 5000);
+		usleep_range((ARIZONA_REG_POLL_DELAY_MS * 1000) / 2,
+			     ARIZONA_REG_POLL_DELAY_MS * 1000);
 	}
 
 	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
@@ -269,7 +274,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	 * we won't race with the interrupt handler as it'll be blocked on
 	 * runtime resume.
 	 */
-	ret = arizona_poll_reg(arizona, 5, ARIZONA_INTERRUPT_RAW_STATUS_5,
+	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_BOOT_DONE_STS, ARIZONA_BOOT_DONE_STS);
 
 	if (!ret)
@@ -339,7 +344,7 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
 			ret);
 		return ret;
 	}
-	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
+	ret = arizona_poll_reg(arizona, 125, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_FLL1_CLOCK_OK_STS,
 			       ARIZONA_FLL1_CLOCK_OK_STS);
 	if (ret)
@@ -403,7 +408,7 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
 		goto err;
 	}
 
-	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
+	ret = arizona_poll_reg(arizona, 25, ARIZONA_WRITE_SEQUENCER_CTRL_1,
 			       ARIZONA_WSEQ_BUSY, 0);
 	if (ret)
 		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-- 
2.1.4

[PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Charles Keepax]

arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
patch updates the implementation to use regmap_read_poll_timeout. We
still keep arizona_poll_reg around as regmap_read_poll_timeout is a
macro so rather than expand this for each caller keep it wrapped in
arizona_poll_reg.

Whilst we are doing this make the timeouts a little more generous as the
previous system had a bit more slack as it was done as a delay per
iteration of the loop whereas regmap_read_poll_timeout compares ktime's.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c | 37 ++++++++++++++-----------------------
 1 file changed, 14 insertions(+), 23 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index ae4cdc4..75488e6 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -235,34 +235,25 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-#define ARIZONA_REG_POLL_DELAY_MS 5
+#define ARIZONA_REG_POLL_DELAY_US 7500
 
 static int arizona_poll_reg(struct arizona *arizona,
 			    int timeout_ms, unsigned int reg,
 			    unsigned int mask, unsigned int target)
 {
-	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
-			      ARIZONA_REG_POLL_DELAY_MS;
 	unsigned int val = 0;
-	int ret, i;
-
-	for (i = 0; i < npolls; i++) {
-		ret = regmap_read(arizona->regmap, reg, &val);
-		if (ret != 0) {
-			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
-				reg, ret);
-			continue;
-		}
-
-		if ((val & mask) == target)
-			return 0;
+	int ret;
 
-		usleep_range((ARIZONA_REG_POLL_DELAY_MS * 1000) / 2,
-			     ARIZONA_REG_POLL_DELAY_MS * 1000);
-	}
+	ret = regmap_read_poll_timeout(arizona->regmap,
+				       ARIZONA_INTERRUPT_RAW_STATUS_5, 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);
 
-	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
-	return -ETIMEDOUT;
+	return ret;
 }
 
 static int arizona_wait_for_boot(struct arizona *arizona)
@@ -274,7 +265,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	 * we won't race with the interrupt handler as it'll be blocked on
 	 * runtime resume.
 	 */
-	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
+	ret = arizona_poll_reg(arizona, 30, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_BOOT_DONE_STS, ARIZONA_BOOT_DONE_STS);
 
 	if (!ret)
@@ -344,7 +335,7 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
 			ret);
 		return ret;
 	}
-	ret = arizona_poll_reg(arizona, 125, ARIZONA_INTERRUPT_RAW_STATUS_5,
+	ret = arizona_poll_reg(arizona, 180, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_FLL1_CLOCK_OK_STS,
 			       ARIZONA_FLL1_CLOCK_OK_STS);
 	if (ret)
@@ -408,7 +399,7 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
 		goto err;
 	}
 
-	ret = arizona_poll_reg(arizona, 25, ARIZONA_WRITE_SEQUENCER_CTRL_1,
+	ret = arizona_poll_reg(arizona, 30, ARIZONA_WRITE_SEQUENCER_CTRL_1,
 			       ARIZONA_WSEQ_BUSY, 0);
 	if (ret)
 		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-- 
2.1.4

Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds

[Lee Jones]

On Tue, 31 Jan 2017, Charles Keepax wrote:

> Currently, we specify the timeout in terms of the number of polls but it
> is more clear from a user of the functions perspective to specify the
> timeout directly in milliseconds, as such update the function to these new
> semantics.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c | 17 +++++++++++------
>  1 file changed, 11 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 4cb34c3..ae4cdc4 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -235,14 +235,18 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> +#define ARIZONA_REG_POLL_DELAY_MS 5
> +
>  static int arizona_poll_reg(struct arizona *arizona,
> -			    int timeout, unsigned int reg,
> +			    int timeout_ms, unsigned int reg,
>  			    unsigned int mask, unsigned int target)
>  {
> +	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
> +			      ARIZONA_REG_POLL_DELAY_MS;

Why the over-complication?

Shouldn't this just be "timeout_ms / ARIZONA_REG_POLL_DELAY_MS"?

>  	unsigned int val = 0;
>  	int ret, i;
>  
> -	for (i = 0; i < timeout; i++) {
> +	for (i = 0; i < npolls; i++) {
>  		ret = regmap_read(arizona->regmap, reg, &val);
>  		if (ret != 0) {
>  			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
> @@ -253,7 +257,8 @@ static int arizona_poll_reg(struct arizona *arizona,
>  		if ((val & mask) == target)
>  			return 0;
>  
> -		usleep_range(1000, 5000);
> +		usleep_range((ARIZONA_REG_POLL_DELAY_MS * 1000) / 2,
> +			     ARIZONA_REG_POLL_DELAY_MS * 1000);

I'm sure there is a macro for conversion from ms to us.

By using such a wide range, you are now not honouring the timeout set
by the caller by as much as 50%.

>  	}
>  
>  	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
> @@ -269,7 +274,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
>  	 * we won't race with the interrupt handler as it'll be blocked on
>  	 * runtime resume.
>  	 */
> -	ret = arizona_poll_reg(arizona, 5, ARIZONA_INTERRUPT_RAW_STATUS_5,
> +	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_BOOT_DONE_STS, ARIZONA_BOOT_DONE_STS);
>  
>  	if (!ret)
> @@ -339,7 +344,7 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
>  			ret);
>  		return ret;
>  	}
> -	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
> +	ret = arizona_poll_reg(arizona, 125, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_FLL1_CLOCK_OK_STS,
>  			       ARIZONA_FLL1_CLOCK_OK_STS);
>  	if (ret)
> @@ -403,7 +408,7 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
>  		goto err;
>  	}
>  
> -	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
> +	ret = arizona_poll_reg(arizona, 25, ARIZONA_WRITE_SEQUENCER_CTRL_1,
>  			       ARIZONA_WSEQ_BUSY, 0);
>  	if (ret)
>  		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable

[Lee Jones]

On Tue, 31 Jan 2017, Charles Keepax wrote:

> arizona_poll_reg already returns ETIMEDOUT if we don't see the expected
> register changes before the time out, so remove pointless local setting of
> ETIMEDOUT.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
  
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index b6d4bc6..e00f577 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -342,10 +342,8 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
>  	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_FLL1_CLOCK_OK_STS,
>  			       ARIZONA_FLL1_CLOCK_OK_STS);
> -	if (ret) {
> -		ret = -ETIMEDOUT;
> +	if (ret)
>  		goto err_fll;
> -	}
>  
>  	ret = regmap_write(arizona->regmap, ARIZONA_SYSTEM_CLOCK_1, 0x0144);
>  	if (ret) {
> @@ -407,11 +405,9 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
>  
>  	ret = arizona_poll_reg(arizona, 5, ARIZONA_WRITE_SEQUENCER_CTRL_1,
>  			       ARIZONA_WSEQ_BUSY, 0);
> -	if (ret) {
> +	if (ret)
>  		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
>  			     ARIZONA_WSEQ_ABORT);
> -		ret = -ETIMEDOUT;
> -	}
>  
>  err:
>  	err = arizona_disable_freerun_sysclk(arizona, &state);

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex

[Lee Jones]

On Tue, 31 Jan 2017, Charles Keepax wrote:

> Register addresses are normally displayed in hex throughout the Arizona
> driver. Update the arizona_poll_reg function to follow this convention.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)

For my own reference:
  Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
  
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index e00f577..4cb34c3 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -245,7 +245,7 @@ static int arizona_poll_reg(struct arizona *arizona,
>  	for (i = 0; i < timeout; i++) {
>  		ret = regmap_read(arizona->regmap, reg, &val);
>  		if (ret != 0) {
> -			dev_err(arizona->dev, "Failed to read reg %u: %d\n",
> +			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
>  				reg, ret);
>  			continue;
>  		}
> @@ -256,7 +256,7 @@ static int arizona_poll_reg(struct arizona *arizona,
>  		usleep_range(1000, 5000);
>  	}
>  
> -	dev_err(arizona->dev, "Polling reg %u timed out: %x\n", reg, val);
> +	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
>  	return -ETIMEDOUT;
>  }
>  

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Lee Jones]

On Tue, 31 Jan 2017, Charles Keepax wrote:

> arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
> patch updates the implementation to use regmap_read_poll_timeout. We
> still keep arizona_poll_reg around as regmap_read_poll_timeout is a
> macro so rather than expand this for each caller keep it wrapped in
> arizona_poll_reg.
> 
> Whilst we are doing this make the timeouts a little more generous as the
> previous system had a bit more slack as it was done as a delay per
> iteration of the loop whereas regmap_read_poll_timeout compares ktime's.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c | 37 ++++++++++++++-----------------------
>  1 file changed, 14 insertions(+), 23 deletions(-)

Roll this up into 3/4.

> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index ae4cdc4..75488e6 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -235,34 +235,25 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -#define ARIZONA_REG_POLL_DELAY_MS 5
> +#define ARIZONA_REG_POLL_DELAY_US 7500
>  
>  static int arizona_poll_reg(struct arizona *arizona,
>  			    int timeout_ms, unsigned int reg,
>  			    unsigned int mask, unsigned int target)
>  {
> -	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
> -			      ARIZONA_REG_POLL_DELAY_MS;
>  	unsigned int val = 0;
> -	int ret, i;
> -
> -	for (i = 0; i < npolls; i++) {
> -		ret = regmap_read(arizona->regmap, reg, &val);
> -		if (ret != 0) {
> -			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
> -				reg, ret);
> -			continue;
> -		}
> -
> -		if ((val & mask) == target)
> -			return 0;
> +	int ret;
>  
> -		usleep_range((ARIZONA_REG_POLL_DELAY_MS * 1000) / 2,
> -			     ARIZONA_REG_POLL_DELAY_MS * 1000);
> -	}
> +	ret = regmap_read_poll_timeout(arizona->regmap,
> +				       ARIZONA_INTERRUPT_RAW_STATUS_5, 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);
>  
> -	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
> -	return -ETIMEDOUT;
> +	return ret;
>  }
>  
>  static int arizona_wait_for_boot(struct arizona *arizona)
> @@ -274,7 +265,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
>  	 * we won't race with the interrupt handler as it'll be blocked on
>  	 * runtime resume.
>  	 */
> -	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
> +	ret = arizona_poll_reg(arizona, 30, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_BOOT_DONE_STS, ARIZONA_BOOT_DONE_STS);
>  
>  	if (!ret)
> @@ -344,7 +335,7 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
>  			ret);
>  		return ret;
>  	}
> -	ret = arizona_poll_reg(arizona, 125, ARIZONA_INTERRUPT_RAW_STATUS_5,
> +	ret = arizona_poll_reg(arizona, 180, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_FLL1_CLOCK_OK_STS,
>  			       ARIZONA_FLL1_CLOCK_OK_STS);
>  	if (ret)
> @@ -408,7 +399,7 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
>  		goto err;
>  	}
>  
> -	ret = arizona_poll_reg(arizona, 25, ARIZONA_WRITE_SEQUENCER_CTRL_1,
> +	ret = arizona_poll_reg(arizona, 30, ARIZONA_WRITE_SEQUENCER_CTRL_1,
>  			       ARIZONA_WSEQ_BUSY, 0);
>  	if (ret)
>  		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Charles Keepax]

On Wed, Feb 08, 2017 at 10:22:32AM +0000, Lee Jones wrote:
> On Tue, 31 Jan 2017, Charles Keepax wrote:
> 
> > arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
> > patch updates the implementation to use regmap_read_poll_timeout. We
> > still keep arizona_poll_reg around as regmap_read_poll_timeout is a
> > macro so rather than expand this for each caller keep it wrapped in
> > arizona_poll_reg.
> > 
> > Whilst we are doing this make the timeouts a little more generous as the
> > previous system had a bit more slack as it was done as a delay per
> > iteration of the loop whereas regmap_read_poll_timeout compares ktime's.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c | 37 ++++++++++++++-----------------------
> >  1 file changed, 14 insertions(+), 23 deletions(-)
> 
> Roll this up into 3/4.
> 

I can if you feel strongly about it but it seems much cleaner to
separate the change in interface for the function and the
refactoring the implementation.

Thanks,
Charles

Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds

[Charles Keepax]

On Wed, Feb 08, 2017 at 10:04:58AM +0000, Lee Jones wrote:
> On Tue, 31 Jan 2017, Charles Keepax wrote:
> 
> > Currently, we specify the timeout in terms of the number of polls but it
> > is more clear from a user of the functions perspective to specify the
> > timeout directly in milliseconds, as such update the function to these new
> > semantics.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c | 17 +++++++++++------
> >  1 file changed, 11 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > index 4cb34c3..ae4cdc4 100644
> > --- a/drivers/mfd/arizona-core.c
> > +++ b/drivers/mfd/arizona-core.c
> > @@ -235,14 +235,18 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
> >  	return IRQ_HANDLED;
> >  }
> >  
> > +#define ARIZONA_REG_POLL_DELAY_MS 5
> > +
> >  static int arizona_poll_reg(struct arizona *arizona,
> > -			    int timeout, unsigned int reg,
> > +			    int timeout_ms, unsigned int reg,
> >  			    unsigned int mask, unsigned int target)
> >  {
> > +	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
> > +			      ARIZONA_REG_POLL_DELAY_MS;
> 
> Why the over-complication?
> 
> Shouldn't this just be "timeout_ms / ARIZONA_REG_POLL_DELAY_MS"?

This will often give you less than the requested timeout if the
requested timeout is not an exact multiple of
ARIZONA_REG_POLL_DELAY_MS. We should never give less timeout than
requested although more is always going to be fine.

> 
> >  	unsigned int val = 0;
> >  	int ret, i;
> >  
> > -	for (i = 0; i < timeout; i++) {
> > +	for (i = 0; i < npolls; i++) {
> >  		ret = regmap_read(arizona->regmap, reg, &val);
> >  		if (ret != 0) {
> >  			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
> > @@ -253,7 +257,8 @@ static int arizona_poll_reg(struct arizona *arizona,
> >  		if ((val & mask) == target)
> >  			return 0;
> >  
> > -		usleep_range(1000, 5000);
> > +		usleep_range((ARIZONA_REG_POLL_DELAY_MS * 1000) / 2,
> > +			     ARIZONA_REG_POLL_DELAY_MS * 1000);
> 
> I'm sure there is a macro for conversion from ms to us.
> 

I will have a look see if I can find it.

> By using such a wide range, you are now not honouring the timeout set
> by the caller by as much as 50%.
> 

Yes apologies my fault here, we really should be applying the
adjustment to the maximum not the minimum here. I don't see a
problem with the wide range, getting more timeout than we asked
for is never going to be a problem but less is. I will respin.

Thanks,
Charles

Re: [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex

[Charles Keepax]

On Wed, Feb 08, 2017 at 10:20:56AM +0000, Lee Jones wrote:
> On Tue, 31 Jan 2017, Charles Keepax wrote:
> 
> > Register addresses are normally displayed in hex throughout the Arizona
> > driver. Update the arizona_poll_reg function to follow this convention.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c | 4 ++--
> >  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> For my own reference:
>   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
>   

Apologies but I am not sure do you want me to add these tags to
new versions of the patches? I would with a regular Acked-by, but
didn't with your last version of these.

Thanks,
Charles

Re: [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex

[Lee Jones]

On Wed, 08 Feb 2017, Charles Keepax wrote:

> On Wed, Feb 08, 2017 at 10:20:56AM +0000, Lee Jones wrote:
> > On Tue, 31 Jan 2017, Charles Keepax wrote:
> > 
> > > Register addresses are normally displayed in hex throughout the Arizona
> > > driver. Update the arizona_poll_reg function to follow this convention.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > >  drivers/mfd/arizona-core.c | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > 
> > For my own reference:
> >   Acked-for-MFD-by: Lee Jones <lee.jones@linaro.org>
> >   
> 
> Apologies but I am not sure do you want me to add these tags to
> new versions of the patches? I would with a regular Acked-by, but
> didn't with your last version of these.

That tag is to let people (including myself) know that I have reviewed
the patch, it looks fine and I will be taking it in via the MFD tree
once all other Acks have been collected.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Charles Keepax]

On Wed, Mar 15, 2017 at 12:17:03PM +0000, Lee Jones wrote:
> On Tue, 14 Mar 2017, Charles Keepax wrote:
> 
> > On Tue, Mar 14, 2017 at 05:07:04PM +0000, Lee Jones wrote:
> > > On Thu, 09 Mar 2017, Charles Keepax wrote:
> > > 
> > > > arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
> > > > patch updates the implementation to use regmap_read_poll_timeout. We
> > > > still keep arizona_poll_reg around as regmap_read_poll_timeout is a
> > > > macro so rather than expand this for each caller keep it wrapped in
> > > > arizona_poll_reg.
> > > > 
> > > > Whilst we are doing this make the timeouts a little more generous as the
> > > > previous system had a bit more slack as it was done as a delay per
> > > > iteration of the loop whereas regmap_read_poll_timeout compares ktime's.
> > > > 
> > > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > ---
> > > >  drivers/mfd/arizona-core.c | 38 ++++++++++++++------------------------
> > > >  1 file changed, 14 insertions(+), 24 deletions(-)
> > > 
> > > Apart from patch count, is there any technical reason why this patch
> > > shouldn't just be rolled into patch 3?
> > > 
> > 
> > I prefer it as two patches as its clearer what happened from the
> > history. One patch changes the interface for the function, the
> > other updates the implementation. Can squash if you feel strongly
> > about it though?
> 
> I don't feel that strongly about it, but to me it looks like patch 4
> reworks everything patch 3 did.
> 

I will spin a new version and squash them.

Thanks,
Charles

Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Lee Jones]

On Tue, 14 Mar 2017, Charles Keepax wrote:

> On Tue, Mar 14, 2017 at 05:07:04PM +0000, Lee Jones wrote:
> > On Thu, 09 Mar 2017, Charles Keepax wrote:
> > 
> > > arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
> > > patch updates the implementation to use regmap_read_poll_timeout. We
> > > still keep arizona_poll_reg around as regmap_read_poll_timeout is a
> > > macro so rather than expand this for each caller keep it wrapped in
> > > arizona_poll_reg.
> > > 
> > > Whilst we are doing this make the timeouts a little more generous as the
> > > previous system had a bit more slack as it was done as a delay per
> > > iteration of the loop whereas regmap_read_poll_timeout compares ktime's.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > >  drivers/mfd/arizona-core.c | 38 ++++++++++++++------------------------
> > >  1 file changed, 14 insertions(+), 24 deletions(-)
> > 
> > Apart from patch count, is there any technical reason why this patch
> > shouldn't just be rolled into patch 3?
> > 
> 
> I prefer it as two patches as its clearer what happened from the
> history. One patch changes the interface for the function, the
> other updates the implementation. Can squash if you feel strongly
> about it though?

I don't feel that strongly about it, but to me it looks like patch 4
reworks everything patch 3 did.

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Charles Keepax]

On Tue, Mar 14, 2017 at 05:07:04PM +0000, Lee Jones wrote:
> On Thu, 09 Mar 2017, Charles Keepax wrote:
> 
> > arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
> > patch updates the implementation to use regmap_read_poll_timeout. We
> > still keep arizona_poll_reg around as regmap_read_poll_timeout is a
> > macro so rather than expand this for each caller keep it wrapped in
> > arizona_poll_reg.
> > 
> > Whilst we are doing this make the timeouts a little more generous as the
> > previous system had a bit more slack as it was done as a delay per
> > iteration of the loop whereas regmap_read_poll_timeout compares ktime's.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c | 38 ++++++++++++++------------------------
> >  1 file changed, 14 insertions(+), 24 deletions(-)
> 
> Apart from patch count, is there any technical reason why this patch
> shouldn't just be rolled into patch 3?
> 

I prefer it as two patches as its clearer what happened from the
history. One patch changes the interface for the function, the
other updates the implementation. Can squash if you feel strongly
about it though?

Thanks,
Charles

Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Lee Jones]

On Thu, 09 Mar 2017, Charles Keepax wrote:

> arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
> patch updates the implementation to use regmap_read_poll_timeout. We
> still keep arizona_poll_reg around as regmap_read_poll_timeout is a
> macro so rather than expand this for each caller keep it wrapped in
> arizona_poll_reg.
> 
> Whilst we are doing this make the timeouts a little more generous as the
> previous system had a bit more slack as it was done as a delay per
> iteration of the loop whereas regmap_read_poll_timeout compares ktime's.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c | 38 ++++++++++++++------------------------
>  1 file changed, 14 insertions(+), 24 deletions(-)

Apart from patch count, is there any technical reason why this patch
shouldn't just be rolled into patch 3?

> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 09d48ed..75488e6 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -235,35 +235,25 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
>  	return IRQ_HANDLED;
>  }
>  
> -#define ARIZONA_REG_POLL_DELAY_MS 5
> -#define ARIZONA_REG_POLL_DELAY_US (ARIZONA_REG_POLL_DELAY_MS * 1000)
> +#define ARIZONA_REG_POLL_DELAY_US 7500
>  
>  static int arizona_poll_reg(struct arizona *arizona,
>  			    int timeout_ms, unsigned int reg,
>  			    unsigned int mask, unsigned int target)
>  {
> -	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
> -			      ARIZONA_REG_POLL_DELAY_MS;
>  	unsigned int val = 0;
> -	int ret, i;
> -
> -	for (i = 0; i < npolls; i++) {
> -		ret = regmap_read(arizona->regmap, reg, &val);
> -		if (ret != 0) {
> -			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
> -				reg, ret);
> -			continue;
> -		}
> -
> -		if ((val & mask) == target)
> -			return 0;
> +	int ret;
>  
> -		usleep_range(ARIZONA_REG_POLL_DELAY_US,
> -			     ARIZONA_REG_POLL_DELAY_US * 2);
> -	}
> +	ret = regmap_read_poll_timeout(arizona->regmap,
> +				       ARIZONA_INTERRUPT_RAW_STATUS_5, 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);
>  
> -	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
> -	return -ETIMEDOUT;
> +	return ret;
>  }
>  
>  static int arizona_wait_for_boot(struct arizona *arizona)
> @@ -275,7 +265,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
>  	 * we won't race with the interrupt handler as it'll be blocked on
>  	 * runtime resume.
>  	 */
> -	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
> +	ret = arizona_poll_reg(arizona, 30, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_BOOT_DONE_STS, ARIZONA_BOOT_DONE_STS);
>  
>  	if (!ret)
> @@ -345,7 +335,7 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
>  			ret);
>  		return ret;
>  	}
> -	ret = arizona_poll_reg(arizona, 125, ARIZONA_INTERRUPT_RAW_STATUS_5,
> +	ret = arizona_poll_reg(arizona, 180, ARIZONA_INTERRUPT_RAW_STATUS_5,
>  			       ARIZONA_FLL1_CLOCK_OK_STS,
>  			       ARIZONA_FLL1_CLOCK_OK_STS);
>  	if (ret)
> @@ -409,7 +399,7 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
>  		goto err;
>  	}
>  
> -	ret = arizona_poll_reg(arizona, 25, ARIZONA_WRITE_SEQUENCER_CTRL_1,
> +	ret = arizona_poll_reg(arizona, 30, ARIZONA_WRITE_SEQUENCER_CTRL_1,
>  			       ARIZONA_WSEQ_BUSY, 0);
>  	if (ret)
>  		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,

-- 
Lee Jones
Linaro STMicroelectronics Landing Team Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog

[PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

[Charles Keepax]

arizona_poll_reg essentially hard-codes regmap_read_poll_timeout, this
patch updates the implementation to use regmap_read_poll_timeout. We
still keep arizona_poll_reg around as regmap_read_poll_timeout is a
macro so rather than expand this for each caller keep it wrapped in
arizona_poll_reg.

Whilst we are doing this make the timeouts a little more generous as the
previous system had a bit more slack as it was done as a delay per
iteration of the loop whereas regmap_read_poll_timeout compares ktime's.

Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
---
 drivers/mfd/arizona-core.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 09d48ed..75488e6 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -235,35 +235,25 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
 	return IRQ_HANDLED;
 }
 
-#define ARIZONA_REG_POLL_DELAY_MS 5
-#define ARIZONA_REG_POLL_DELAY_US (ARIZONA_REG_POLL_DELAY_MS * 1000)
+#define ARIZONA_REG_POLL_DELAY_US 7500
 
 static int arizona_poll_reg(struct arizona *arizona,
 			    int timeout_ms, unsigned int reg,
 			    unsigned int mask, unsigned int target)
 {
-	unsigned int npolls = (timeout_ms + ARIZONA_REG_POLL_DELAY_MS - 1) /
-			      ARIZONA_REG_POLL_DELAY_MS;
 	unsigned int val = 0;
-	int ret, i;
-
-	for (i = 0; i < npolls; i++) {
-		ret = regmap_read(arizona->regmap, reg, &val);
-		if (ret != 0) {
-			dev_err(arizona->dev, "Failed to read reg 0x%x: %d\n",
-				reg, ret);
-			continue;
-		}
-
-		if ((val & mask) == target)
-			return 0;
+	int ret;
 
-		usleep_range(ARIZONA_REG_POLL_DELAY_US,
-			     ARIZONA_REG_POLL_DELAY_US * 2);
-	}
+	ret = regmap_read_poll_timeout(arizona->regmap,
+				       ARIZONA_INTERRUPT_RAW_STATUS_5, 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);
 
-	dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
-	return -ETIMEDOUT;
+	return ret;
 }
 
 static int arizona_wait_for_boot(struct arizona *arizona)
@@ -275,7 +265,7 @@ static int arizona_wait_for_boot(struct arizona *arizona)
 	 * we won't race with the interrupt handler as it'll be blocked on
 	 * runtime resume.
 	 */
-	ret = arizona_poll_reg(arizona, 25, ARIZONA_INTERRUPT_RAW_STATUS_5,
+	ret = arizona_poll_reg(arizona, 30, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_BOOT_DONE_STS, ARIZONA_BOOT_DONE_STS);
 
 	if (!ret)
@@ -345,7 +335,7 @@ static int arizona_enable_freerun_sysclk(struct arizona *arizona,
 			ret);
 		return ret;
 	}
-	ret = arizona_poll_reg(arizona, 125, ARIZONA_INTERRUPT_RAW_STATUS_5,
+	ret = arizona_poll_reg(arizona, 180, ARIZONA_INTERRUPT_RAW_STATUS_5,
 			       ARIZONA_FLL1_CLOCK_OK_STS,
 			       ARIZONA_FLL1_CLOCK_OK_STS);
 	if (ret)
@@ -409,7 +399,7 @@ static int wm5102_apply_hardware_patch(struct arizona *arizona)
 		goto err;
 	}
 
-	ret = arizona_poll_reg(arizona, 25, ARIZONA_WRITE_SEQUENCER_CTRL_1,
+	ret = arizona_poll_reg(arizona, 30, ARIZONA_WRITE_SEQUENCER_CTRL_1,
 			       ARIZONA_WSEQ_BUSY, 0);
 	if (ret)
 		regmap_write(arizona->regmap, ARIZONA_WRITE_SEQUENCER_CTRL_0,
-- 
2.1.4