Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1570801 > unrolled thread
| Started by | Charles Keepax <ckeepax@opensource.wolfsonmicro.com> |
|---|---|
| First post | 2017-01-31 15:50 +0100 |
| Last post | 2017-02-08 12:20 +0100 |
| Articles | 5 — 2 participants |
Back to article view | Back to linux.kernel
[PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-01-31 15:50 +0100
[PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-01-31 15:50 +0100
Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Lee Jones <lee.jones@linaro.org> - 2017-02-08 11:40 +0100
Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-02-08 13:30 +0100
Re: [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable Lee Jones <lee.jones@linaro.org> - 2017-02-08 12:20 +0100
| From | Charles Keepax <ckeepax@opensource.wolfsonmicro.com> |
|---|---|
| Date | 2017-01-31 15:50 +0100 |
| Subject | [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable |
| Message-ID | <t5HUR-1S1-11@gated-at.bofh.it> |
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
[toc] | [next] | [standalone]
| From | Charles Keepax <ckeepax@opensource.wolfsonmicro.com> |
|---|---|
| Date | 2017-01-31 15:50 +0100 |
| Subject | [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds |
| Message-ID | <t5HUT-1S1-33@gated-at.bofh.it> |
| In reply to | #1570801 |
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
[toc] | [prev] | [next] | [standalone]
| From | Lee Jones <lee.jones@linaro.org> |
|---|---|
| Date | 2017-02-08 11:40 +0100 |
| Subject | Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds |
| Message-ID | <t8xPk-7AX-17@gated-at.bofh.it> |
| In reply to | #1570806 |
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
[toc] | [prev] | [next] | [standalone]
| From | Charles Keepax <ckeepax@opensource.wolfsonmicro.com> |
|---|---|
| Date | 2017-02-08 13:30 +0100 |
| Subject | Re: [PATCH RESEND 3/4] mfd: arizona: Update arizona_poll_reg to take a timeout in milliseconds |
| Message-ID | <t8zxM-h3-37@gated-at.bofh.it> |
| In reply to | #1576426 |
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
[toc] | [prev] | [next] | [standalone]
| From | Lee Jones <lee.jones@linaro.org> |
|---|---|
| Date | 2017-02-08 12:20 +0100 |
| Subject | Re: [PATCH RESEND 1/4] mfd: arizona: Remove duplicate set of ret variable |
| Message-ID | <t8ys1-84Y-17@gated-at.bofh.it> |
| In reply to | #1570801 |
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
[toc] | [prev] | [standalone]
Back to top | Article view | linux.kernel
csiph-web