mfd: arizona: Remove duplicate set of ret variable

PATCH In review Charles Keepax 2 participants 9 messages Jan 4, 2017 – Jan 6, 2017

Subsystems (2)

Multifunction devices (MFD) Wolfson microelectronics drivers

lore

Share & tools

lore

Charles Keepax

author

2017-01-04 15:10 UTC #0 lore raw

PATCH 1/3 mfd: arizona: Remove duplicate set of ret variable

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 <[email protected]>

---
 drivers/mfd/arizona-core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

drivers/mfd/arizona-core.c +2 -6

index b6d4bc6..e00f577 100644
@@ -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);

Signature · 1 line

2.1.4

Charles Keepax

author

2017-01-04 15:10 UTC #1 lore raw

PATCH 2/3 mfd: arizona: Display register addresses in hex

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 <[email protected]>

---
 drivers/mfd/arizona-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/mfd/arizona-core.c +2 -2

index e00f577..4cb34c3 100644
@@ -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;
 }

Signature · 1 line

2.1.4

Lee Jones

2017-01-05 08:04 UTC #2 lore raw

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

Quoted 7 lines · 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 <[email protected]>
---
 drivers/mfd/arizona-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

For my own reference:

Acked-for-MFD-by Lee Jones <[email protected]>

Quoted 22 lines

drivers/mfd/arizona-core.c +2 -2

index e00f577..4cb34c3 100644
@@ -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;
 }

Signature · 4 lines

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

Charles Keepax

author

2017-01-04 15:10 UTC #3 lore raw

PATCH 3/3 mfd: arizona: Use regmap_read_poll_timeout instead of hard coding it

In reply to 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.

Signed-off-by Charles Keepax <[email protected]>

---
 drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)

drivers/mfd/arizona-core.c +11 ~~-17~~

index 4cb34c3..e6fae3c 100644
@@ -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)

Signature · 1 line

2.1.4

Lee Jones

2017-01-05 08:03 UTC #4 lore raw

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

Quoted 16 lines · 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.
Signed-off-by Charles Keepax <[email protected]>
---
 drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/mfd/arizona-core.c +0 -0
index 4cb34c3..e6fae3c 100644

drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 242

Quoted 8 lines

drivers/mfd/arizona-core.c +2 -1

@@ -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?

Quoted 11 lines

drivers/mfd/arizona-core.c +0 ~~-10~~

 	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;
-		}
-

drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 249

Quoted 10 lines

drivers/mfd/arizona-core.c +5 -4

-		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?

Quoted 10 lines

drivers/mfd/arizona-core.c +4 -2

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

Signature · 4 lines

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

Charles Keepax

author

2017-01-05 10:04 UTC #5 lore raw

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

Quoted 20 lines · Lee Jones

Quoted 18 lines · 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.
Signed-off-by Charles Keepax <[email protected]>
---
 drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/mfd/arizona-core.c +0 -0
index 4cb34c3..e6fae3c 100644
@@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
 }
 

Quoted 10 lines

drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 242
Quoted 6 lines
drivers/mfd/arizona-core.c +2 -1
 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.

Quoted 4 lines

... 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.

Quoted 15 lines

drivers/mfd/arizona-core.c +1 ~~-12~~

 	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;

Quoted 10 lines

drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 249
Quoted 6 lines
drivers/mfd/arizona-core.c +4 -2
-		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

Quoted 16 lines

Quoted 10 lines

drivers/mfd/arizona-core.c +4 -2

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

Signature · 4 lines

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

Lee Jones

2017-01-05 14:45 UTC #6 lore raw

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

Quoted 32 lines · Charles Keepax

Quoted 20 lines · Lee Jones
Quoted 18 lines · 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.
Signed-off-by Charles Keepax <[email protected]>
---
 drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/mfd/arizona-core.c +0 -0
index 4cb34c3..e6fae3c 100644
@@ -236,28 +236,22 @@ static irqreturn_t arizona_overclocked(int irq, void *data)
 }
 
Quoted 10 lines
drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 242
Quoted 6 lines
drivers/mfd/arizona-core.c +2 -1
 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.

Quoted 3 lines

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.

Quoted 11 lines

Quoted 4 lines
... 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. ;)

Quoted 26 lines

Quoted 15 lines
Quoted 15 lines
drivers/mfd/arizona-core.c +1 ~~-12~~
 	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;
 
Quoted 10 lines
drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 249
Quoted 6 lines
drivers/mfd/arizona-core.c +4 -2
-		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?

Quoted 3 lines

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.

Quoted 4 lines

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?

Quoted 3 lines

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.

Quoted 11 lines

Quoted 10 lines

drivers/mfd/arizona-core.c +4 -2

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

Signature · 4 lines

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

Charles Keepax

author

2017-01-06 09:28 UTC #7 lore raw

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

Quoted 34 lines · Lee Jones

Quoted 32 lines · Charles Keepax
Quoted 30 lines · Lee Jones
Quoted 16 lines · 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.
Signed-off-by Charles Keepax <[email protected]>
---
 drivers/mfd/arizona-core.c | 28 +++++++++++-----------------
 1 file changed, 11 insertions(+), 17 deletions(-)
drivers/mfd/arizona-core.c +0 -0
index 4cb34c3..e6fae3c 100644
drivers/mfd/arizona-core.c static irqreturn_t arizona_overclocked(int irq, void *data) · line 242
Quoted 8 lines
drivers/mfd/arizona-core.c +2 -1
@@ -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.

Quoted 7 lines

Quoted 3 lines
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.

Quoted 9 lines

Quoted 4 lines
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

Lee Jones

2017-01-05 08:04 UTC #8 lore raw

In reply to Charles Keepax

Quoted 8 lines · 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 <[email protected]>
---
 drivers/mfd/arizona-core.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

For my own reference:

Acked-for-MFD-by Lee Jones <[email protected]>

Quoted 29 lines

drivers/mfd/arizona-core.c +2 -6

index b6d4bc6..e00f577 100644
@@ -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);

Signature · 4 lines

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