[PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait

Simplify lan9303_indirect_phy_wait_for_completion()
and lan9303_switch_wait_for_completion() by using a new function
lan9303_read_wait()

Signed-off-by: Egil Hjelmeland <redacted>
---
 drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++-----------------------
 1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c1b004fa64d9..96ccce0939d3 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c

@@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
 	return -EIO;
 }
 
+/* Wait a while until mask & reg == value. Otherwise return timeout. */
+static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
+			     char value)
+{
+	int i;
+
+	for (i = 0; i < 25; i++) {
+		u32 reg;
+		int ret;
+
+		ret = lan9303_read(chip->regmap, offset, &reg);
+		if (ret) {
+			dev_err(chip->dev, "%s failed to read offset %d: %d\n",
+				__func__, offset, ret);
+			return ret;
+		}
+		if ((reg & mask) == value)
+			return 0;
+		usleep_range(1000, 2000);
+	}
+	return -ETIMEDOUT;
+}
+
 static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
 {
 	int ret;

@@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
 
 static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
 {
-	int ret, i;
-	u32 reg;
-
-	for (i = 0; i < 25; i++) {
-		ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
-		if (ret) {
-			dev_err(chip->dev,
-				"Failed to read pmi access status: %d\n", ret);
-			return ret;
-		}
-		if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
-			return 0;
-		usleep_range(1000, 2000);
-	}
-
-	return -EIO;
+	return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
+				 LAN9303_PMI_ACCESS_MII_BUSY, 0);
 }
 
 static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)

@@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
 
 static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
 {
-	int ret, i;
-	u32 reg;
-
-	for (i = 0; i < 25; i++) {
-		ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
-		if (ret) {
-			dev_err(chip->dev,
-				"Failed to read csr command status: %d\n", ret);
-			return ret;
-		}
-		if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
-			return 0;
-		usleep_range(1000, 2000);
-	}
-
-	return -EIO;
+	return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
+				 LAN9303_SWITCH_CSR_CMD_BUSY, 0);
 }
 
 static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)

-- 
2.14.1

Hi Egil,

Egil Hjelmeland [off-list ref] writes:

That is weird to mix int, u32 and char for mask checking. I suggest you
to use the u32 type as well for both mask and value.

Looking at how lan9303_read_wait is called, the value argument doesn't
seem necessary. You can directly return 0 if (!(reg & mask)).

A newline before the return statment would be appreciated.

Thanks,

        Vivien

Hi Vivien.

Den 12. des. 2017 19:08, skrev Vivien Didelot:

Hi Egil,

Egil Hjelmeland [off-list ref] writes:

quoted

Simplify lan9303_indirect_phy_wait_for_completion()
and lan9303_switch_wait_for_completion() by using a new function
lan9303_read_wait()

Signed-off-by: Egil Hjelmeland <redacted>
---
  drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++-----------------------
  1 file changed, 27 insertions(+), 32 deletions(-)

diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index c1b004fa64d9..96ccce0939d3 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c

@@ -249,6 +249,29 @@ static int lan9303_read(struct regmap *regmap, unsigned int offset, u32 *reg)
  	return -EIO;
  }
  
+/* Wait a while until mask & reg == value. Otherwise return timeout. */
+static int lan9303_read_wait(struct lan9303 *chip, int offset, int mask,
+			     char value)
+{
+	int i;
+
+	for (i = 0; i < 25; i++) {
+		u32 reg;
+		int ret;
+
+		ret = lan9303_read(chip->regmap, offset, &reg);
+		if (ret) {
+			dev_err(chip->dev, "%s failed to read offset %d: %d\n",
+				__func__, offset, ret);
+			return ret;
+		}
+		if ((reg & mask) == value)
+			return 0;

That is weird to mix int, u32 and char for mask checking. I suggest you
to use the u32 type as well for both mask and value.

Good catch. Will fix that. Same with lan9303_csr_reg_wait() then.

Looking at how lan9303_read_wait is called, the value argument doesn't
seem necessary. You can directly return 0 if (!(reg & mask)).

The idea was to make in more general usable, in case one need to wait for a bit to be set. But I don't have any example from the datasheet that needs it, so I could take "value" away.

quoted

+		usleep_range(1000, 2000);
+	}
+	return -ETIMEDOUT;

A newline before the return statment would be appreciated.

Ok.

quoted

+}
+
  static int lan9303_virt_phy_reg_read(struct lan9303 *chip, int regnum)
  {
  	int ret;

@@ -274,22 +297,8 @@ static int lan9303_virt_phy_reg_write(struct lan9303 *chip, int regnum, u16 val)
  
  static int lan9303_indirect_phy_wait_for_completion(struct lan9303 *chip)
  {
-	int ret, i;
-	u32 reg;
-
-	for (i = 0; i < 25; i++) {
-		ret = lan9303_read(chip->regmap, LAN9303_PMI_ACCESS, &reg);
-		if (ret) {
-			dev_err(chip->dev,
-				"Failed to read pmi access status: %d\n", ret);
-			return ret;
-		}
-		if (!(reg & LAN9303_PMI_ACCESS_MII_BUSY))
-			return 0;
-		usleep_range(1000, 2000);
-	}
-
-	return -EIO;
+	return lan9303_read_wait(chip, LAN9303_PMI_ACCESS,
+				 LAN9303_PMI_ACCESS_MII_BUSY, 0);
  }
  
  static int lan9303_indirect_phy_read(struct lan9303 *chip, int addr, int regnum)

@@ -366,22 +375,8 @@ EXPORT_SYMBOL_GPL(lan9303_indirect_phy_ops);
  
  static int lan9303_switch_wait_for_completion(struct lan9303 *chip)
  {
-	int ret, i;
-	u32 reg;
-
-	for (i = 0; i < 25; i++) {
-		ret = lan9303_read(chip->regmap, LAN9303_SWITCH_CSR_CMD, &reg);
-		if (ret) {
-			dev_err(chip->dev,
-				"Failed to read csr command status: %d\n", ret);
-			return ret;
-		}
-		if (!(reg & LAN9303_SWITCH_CSR_CMD_BUSY))
-			return 0;
-		usleep_range(1000, 2000);
-	}
-
-	return -EIO;
+	return lan9303_read_wait(chip, LAN9303_SWITCH_CSR_CMD,
+				 LAN9303_SWITCH_CSR_CMD_BUSY, 0);
  }
  
  static int lan9303_write_switch_reg(struct lan9303 *chip, u16 regnum, u32 val)

Thanks,

         Vivien

Thanks, Egil