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

Groups > linux.kernel > #1550907 > unrolled thread

[PATCH 3/3] 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

  [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-01-04 16:20 +0100
    Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead  of hard coding it Lee Jones <lee.jones@linaro.org> - 2017-01-05 09:10 +0100
      Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead  of hard coding it Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-01-05 11:10 +0100
        Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead  of hard coding it Lee Jones <lee.jones@linaro.org> - 2017-01-05 15:50 +0100
          Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead  of hard coding it Charles Keepax <ckeepax@opensource.wolfsonmicro.com> - 2017-01-06 10:30 +0100

#1550907 — [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

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.

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

diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
index 4cb34c3..e6fae3c 100644
--- a/drivers/mfd/arizona-core.c
+++ b/drivers/mfd/arizona-core.c
@@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
 }
 
 static int arizona_poll_reg(struct arizona *arizona,
-			    int timeout, unsigned int reg,
+			    int npolls, unsigned int reg,
 			    unsigned int mask, unsigned int target)
 {
+	const int poll_us = 7500;
 	unsigned int val = 0;
-	int ret, i;
-
-	for (i = 0; i < timeout; 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(1000, 5000);
-	}
+	ret = regmap_read_poll_timeout(arizona->regmap,
+				       ARIZONA_INTERRUPT_RAW_STATUS_5, val,
+				       ((val & mask) == target), poll_us,
+				       npolls * poll_us);
+	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)
-- 
2.1.4

[toc] | [next] | [standalone]

#1551728 — Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

On Wed, 04 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.
> 
> Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> ---
>  drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
>  1 file changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> index 4cb34c3..e6fae3c 100644
> --- a/drivers/mfd/arizona-core.c
> +++ b/drivers/mfd/arizona-core.c
> @@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
>  }
>  
>  static int arizona_poll_reg(struct arizona *arizona,
> -			    int timeout, unsigned int reg,
> +			    int npolls, unsigned int reg,
>  			    unsigned int mask, unsigned int target)
>  {
> +	const int poll_us = 7500;

Get rid of this and replace its usage with a nice #define describing
exactly what the timeout is for i.e what timed out.

... okay, I just read the doc.  This should really be 'SLEEP'.

Where did 7500 come from anyway?  Is it documented?

>  	unsigned int val = 0;
> -	int ret, i;
> -
> -	for (i = 0; i < timeout; 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(1000, 5000);
> -	}
> +	ret = regmap_read_poll_timeout(arizona->regmap,
> +				       ARIZONA_INTERRUPT_RAW_STATUS_5, val,
> +				       ((val & mask) == target), poll_us,
> +				       npolls * poll_us);

What's the relevance of npolls?  Is this number documented, or was it
pulled from your ... err, the air?

> +	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)

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

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

#1551844 — Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

On Thu, Jan 05, 2017 at 08:07:01AM +0000, Lee Jones wrote:
> On Wed, 04 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.
> > 
> > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > ---
> >  drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
> >  1 file changed, 11 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > index 4cb34c3..e6fae3c 100644
> > --- a/drivers/mfd/arizona-core.c
> > +++ b/drivers/mfd/arizona-core.c
> > @@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
> >  }
> >  
> >  static int arizona_poll_reg(struct arizona *arizona,
> > -			    int timeout, unsigned int reg,
> > +			    int npolls, unsigned int reg,
> >  			    unsigned int mask, unsigned int target)
> >  {
> > +	const int poll_us = 7500;
> 
> Get rid of this and replace its usage with a nice #define describing
> exactly what the timeout is for i.e what timed out.
> 

I can replace this with a define if you prefer although since the
value is only used locally I generally prefer a const variable as
it keeps all the relevant code together.

> ... okay, I just read the doc.  This should really be 'SLEEP'.
> 
> Where did 7500 come from anyway?  Is it documented?
> 

To be fair this number is a little pulled from my ... err the
air. But basically provides a reasonable balance of timing
between polls for the various users of the function. It didn't
really seem worth having each user specify its own timing for
this part as we generally don't care down to a couple of mS here
or there.

> >  	unsigned int val = 0;
> > -	int ret, i;
> > -
> > -	for (i = 0; i < timeout; 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(1000, 5000);
> > -	}
> > +	ret = regmap_read_poll_timeout(arizona->regmap,
> > +				       ARIZONA_INTERRUPT_RAW_STATUS_5, val,
> > +				       ((val & mask) == target), poll_us,
> > +				       npolls * poll_us);
> 
> What's the relevance of npolls?  Is this number documented, or was it
> pulled from your ... err, the air?
> 

Its an argument passed into the function, its called npolls in a
function that polls a register I really don't know how to make
the fact it is the number of polls to do any more clear than
that. As for the number passed into the function that depends on
how long that particular register should be polled for, this is a
function that is used in several places for polling for a
particular register state.

Would all these issues perhaps be best solved by my adding some
kernel doc for the function with some more description of what
everything is?

Thanks,
Charles

> > +	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)
> 
> -- 
> Lee Jones
> Linaro STMicroelectronics Landing Team Lead
> Linaro.org │ Open source software for ARM SoCs
> Follow Linaro: Facebook | Twitter | Blog

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

#1552025 — Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

On Thu, 05 Jan 2017, Charles Keepax wrote:

> On Thu, Jan 05, 2017 at 08:07:01AM +0000, Lee Jones wrote:
> > On Wed, 04 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.
> > > 
> > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > ---
> > >  drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
> > >  1 file changed, 11 insertions(+), 17 deletions(-)
> > > 
> > > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > > index 4cb34c3..e6fae3c 100644
> > > --- a/drivers/mfd/arizona-core.c
> > > +++ b/drivers/mfd/arizona-core.c
> > > @@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
> > >  }
> > >  
> > >  static int arizona_poll_reg(struct arizona *arizona,
> > > -			    int timeout, unsigned int reg,
> > > +			    int npolls, unsigned int reg,
> > >  			    unsigned int mask, unsigned int target)
> > >  {
> > > +	const int poll_us = 7500;
> > 
> > Get rid of this and replace its usage with a nice #define describing
> > exactly what the timeout is for i.e what timed out.
> > 
> 
> I can replace this with a define if you prefer although since the
> value is only used locally I generally prefer a const variable as
> it keeps all the relevant code together.

I'd prefer a DEFINE, then you can rid the requirement for a variable
altogether.

> > ... okay, I just read the doc.  This should really be 'SLEEP'.
> > 
> > Where did 7500 come from anyway?  Is it documented?
> > 
> 
> To be fair this number is a little pulled from my ... err the
> air. But basically provides a reasonable balance of timing
> between polls for the various users of the function. It didn't
> really seem worth having each user specify its own timing for
> this part as we generally don't care down to a couple of mS here
> or there.

Very well.  If you define it I'll ask no more questions. ;)

> > >  	unsigned int val = 0;
> > > -	int ret, i;
> > > -
> > > -	for (i = 0; i < timeout; 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(1000, 5000);
> > > -	}
> > > +	ret = regmap_read_poll_timeout(arizona->regmap,
> > > +				       ARIZONA_INTERRUPT_RAW_STATUS_5, val,
> > > +				       ((val & mask) == target), poll_us,
> > > +				       npolls * poll_us);
> > 
> > What's the relevance of npolls?  Is this number documented, or was it
> > pulled from your ... err, the air?
> > 
> 
> Its an argument passed into the function, its called npolls in a
> function that polls a register I really don't know how to make
> the fact it is the number of polls to do any more clear than

I can see what it's used for, I am more concerned about the number
passed in.

> that. As for the number passed into the function that depends on
> how long that particular register should be polled for, this is a
> function that is used in several places for polling for a
> particular register state.

I would have thought a consumer would be more likely to know how long
it would poll for, rather than how many times to poll.  Knowledge of
time-between-polls in only known locally.  Do you see where I'm going
with this?

> Would all these issues perhaps be best solved by my adding some
> kernel doc for the function with some more description of what
> everything is?

I don't think so, to be honest.

> > > +	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)
> > 

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

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

#1552630 — Re: [PATCH 3/3] mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

On Thu, Jan 05, 2017 at 02:48:22PM +0000, Lee Jones wrote:
> On Thu, 05 Jan 2017, Charles Keepax wrote:
> 
> > On Thu, Jan 05, 2017 at 08:07:01AM +0000, Lee Jones wrote:
> > > On Wed, 04 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.
> > > > 
> > > > Signed-off-by: Charles Keepax <ckeepax@opensource.wolfsonmicro.com>
> > > > ---
> > > >  drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
> > > >  1 file changed, 11 insertions(+), 17 deletions(-)
> > > > 
> > > > diff --git a/drivers/mfd/arizona-core.c b/drivers/mfd/arizona-core.c
> > > > index 4cb34c3..e6fae3c 100644
> > > > --- a/drivers/mfd/arizona-core.c
> > > > +++ b/drivers/mfd/arizona-core.c
> > > > @@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
> > > >  }
> > > >  
> > > >  static int arizona_poll_reg(struct arizona *arizona,
> > > > -			    int timeout, unsigned int reg,
> > > > +			    int npolls, unsigned int reg,
> > > >  			    unsigned int mask, unsigned int target)
> > > >  {
> > > > +	const int poll_us = 7500;
> > > 
> > > Get rid of this and replace its usage with a nice #define describing
> > > exactly what the timeout is for i.e what timed out.
> > > 
> > 
> > I can replace this with a define if you prefer although since the
> > value is only used locally I generally prefer a const variable as
> > it keeps all the relevant code together.
> 
> I'd prefer a DEFINE, then you can rid the requirement for a variable
> altogether.
> 

Fair enough I will respin.

> > that. As for the number passed into the function that depends on
> > how long that particular register should be polled for, this is a
> > function that is used in several places for polling for a
> > particular register state.
> 
> I would have thought a consumer would be more likely to know how long
> it would poll for, rather than how many times to poll.  Knowledge of
> time-between-polls in only known locally.  Do you see where I'm going
> with this?

Yeah that's a fair point I will switch it around so the user
specifies the total time out and npolls is worked out internally.

Thanks,
Charles

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web