* [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex
2017-01-31 14:44 [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Charles Keepax
@ 2017-01-31 14:44 ` Charles Keepax
2017-02-08 10:20 ` Lee Jones
2017-01-31 14:44 ` [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Charles Keepax
` (2 subsequent siblings)
3 siblings, 1 reply; 14+ messages in thread
From: Charles Keepax @ 2017-01-31 14:44 UTC (permalink / raw)
To: lee.jones; +Cc: linux-kernel, patches
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex
2017-01-31 14:44 ` [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex Charles Keepax
@ 2017-02-08 10:20 ` Lee Jones
2017-02-08 11:20 ` Charles Keepax
0 siblings, 1 reply; 14+ messages in thread
From: Lee Jones @ 2017-02-08 10:20 UTC (permalink / raw)
To: Charles Keepax; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex
2017-02-08 10:20 ` Lee Jones
@ 2017-02-08 11:20 ` Charles Keepax
2017-02-08 12:50 ` Lee Jones
0 siblings, 1 reply; 14+ messages in thread
From: Charles Keepax @ 2017-02-08 11:20 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex
2017-02-08 11:20 ` Charles Keepax
@ 2017-02-08 12:50 ` Lee Jones
0 siblings, 0 replies; 14+ messages in thread
From: Lee Jones @ 2017-02-08 12:50 UTC (permalink / raw)
To: Charles Keepax; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds
2017-01-31 14:44 [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Charles Keepax
2017-01-31 14:44 ` [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex Charles Keepax
@ 2017-01-31 14:44 ` Charles Keepax
2017-02-08 10:04 ` Lee Jones
2017-01-31 14:44 ` [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it Charles Keepax
2017-02-08 10:20 ` [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Lee Jones
3 siblings, 1 reply; 14+ messages in thread
From: Charles Keepax @ 2017-01-31 14:44 UTC (permalink / raw)
To: lee.jones; +Cc: linux-kernel, patches
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds
2017-01-31 14:44 ` [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Charles Keepax
@ 2017-02-08 10:04 ` Lee Jones
2017-02-08 11:18 ` Charles Keepax
0 siblings, 1 reply; 14+ messages in thread
From: Lee Jones @ 2017-02-08 10:04 UTC (permalink / raw)
To: Charles Keepax; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds
2017-02-08 10:04 ` Lee Jones
@ 2017-02-08 11:18 ` Charles Keepax
0 siblings, 0 replies; 14+ messages in thread
From: Charles Keepax @ 2017-02-08 11:18 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it
2017-01-31 14:44 [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Charles Keepax
2017-01-31 14:44 ` [PATCH RESEND 2/4] mfd: arizona: Display register addresses in hex Charles Keepax
2017-01-31 14:44 ` [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Charles Keepax
@ 2017-01-31 14:44 ` Charles Keepax
2017-02-08 10:22 ` Lee Jones
2017-02-08 10:20 ` [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Lee Jones
3 siblings, 1 reply; 14+ messages in thread
From: Charles Keepax @ 2017-01-31 14:44 UTC (permalink / raw)
To: lee.jones; +Cc: linux-kernel, patches
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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it
2017-01-31 14:44 ` [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it Charles Keepax
@ 2017-02-08 10:22 ` Lee Jones
2017-02-08 11:14 ` Charles Keepax
0 siblings, 1 reply; 14+ messages in thread
From: Lee Jones @ 2017-02-08 10:22 UTC (permalink / raw)
To: Charles Keepax; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it
2017-02-08 10:22 ` Lee Jones
@ 2017-02-08 11:14 ` Charles Keepax
0 siblings, 0 replies; 14+ messages in thread
From: Charles Keepax @ 2017-02-08 11:14 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable
2017-01-31 14:44 [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Charles Keepax
` (2 preceding siblings ...)
2017-01-31 14:44 ` [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it Charles Keepax
@ 2017-02-08 10:20 ` Lee Jones
3 siblings, 0 replies; 14+ messages in thread
From: Lee Jones @ 2017-02-08 10:20 UTC (permalink / raw)
To: Charles Keepax; +Cc: linux-kernel, patches
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds
2017-03-09 9:28 Charles Keepax
@ 2017-03-09 9:28 ` Charles Keepax
2017-03-14 17:06 ` Lee Jones
0 siblings, 1 reply; 14+ messages in thread
From: Charles Keepax @ 2017-03-09 9:28 UTC (permalink / raw)
To: lee.jones; +Cc: linux-kernel, patches
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 | 18 ++++++++++++------
1 file changed, 12 insertions(+), 6 deletions(-)
diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 4cb34c3..09d48ed 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -235,14 +235,19 @@ 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)
+
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 +258,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_US,
+ ARIZONA_REG_POLL_DELAY_US * 2);
}
dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
@@ -269,7 +275,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 +345,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 +409,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
^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds
2017-03-09 9:28 ` [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Charles Keepax
@ 2017-03-14 17:06 ` Lee Jones
0 siblings, 0 replies; 14+ messages in thread
From: Lee Jones @ 2017-03-14 17:06 UTC (permalink / raw)
To: Charles Keepax; +Cc: linux-kernel, patches
On Thu, 09 Mar 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 | 18 ++++++++++++------
> 1 file changed, 12 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 4cb34c3..09d48ed 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -235,14 +235,19 @@ 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)
> +
> 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 +258,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_US,
> + ARIZONA_REG_POLL_DELAY_US * 2);
> }
>
> dev_err(arizona->dev, "Polling reg 0x%x timed out: %x\n", reg, val);
> @@ -269,7 +275,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 +345,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 +409,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
^ permalink raw reply [flat|nested] 14+ messages in thread