Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1600717 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout  instead of hard coding it Lee Jones <lee.jones@linaro.org> - 2017-03-14 18:10 +0100
    Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout  instead of hard coding it Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-03-14 19:50 +0100
      Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout  instead of hard coding it Lee Jones <lee.jones@linaro.org> - 2017-03-15 13:20 +0100
        Re: [PATCH RESEND 4/4] mfd: arizona: Use regmap_read_poll_timeout  instead of hard coding it Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-03-15 15:50 +0100

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

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

[toc] | [next] | [standalone]

#1600801

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

[toc] | [prev] | [next] | [standalone]

#1601352

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

[toc] | [prev] | [next] | [standalone]

#1601456

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web