[PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[Timur Tabi]

Macro spin_event_timeout() was designed for simple polling of hardware
registers with a timeout, so use it when we poll the MIIMIND register.
This allows us to return an error code instead of polling indefinitely.

Note that PHY_INIT_TIMEOUT is a count of loop iterations, so we can't use
it for spin_event_timeout(), which asks for microseconds.

Signed-off-by: Timur Tabi <timur@freescale.com>
---
 drivers/net/ethernet/freescale/fsl_pq_mdio.c |   25 +++++++++++++++----------
 1 files changed, 15 insertions(+), 10 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fsl_pq_mdio.c b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
index 9eb8159..ab0fabd 100644
--- a/drivers/net/ethernet/freescale/fsl_pq_mdio.c
+++ b/drivers/net/ethernet/freescale/fsl_pq_mdio.c
@@ -64,6 +64,8 @@ struct fsl_pq_mdio_priv {
 int fsl_pq_local_mdio_write(struct fsl_pq_mdio __iomem *regs, int mii_id,
 		int regnum, u16 value)
 {
+	u32 status;
+
 	/* Set the PHY address and the register address we want to write */
 	out_be32(&regs->miimadd, (mii_id << 8) | regnum);
 
@@ -71,10 +73,10 @@ int fsl_pq_local_mdio_write(struct fsl_pq_mdio __iomem *regs, int mii_id,
 	out_be32(&regs->miimcon, value);
 
 	/* Wait for the transaction to finish */
-	while (in_be32(&regs->miimind) & MIIMIND_BUSY)
-		cpu_relax();
+	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
+		1000, 0);
 
-	return 0;
+	return status ? 0 : -ETIMEDOUT;
 }
 
 /*
@@ -91,6 +93,7 @@ int fsl_pq_local_mdio_read(struct fsl_pq_mdio __iomem *regs,
 		int mii_id, int regnum)
 {
 	u16 value;
+	u32 status;
 
 	/* Set the PHY address and the register address we want to read */
 	out_be32(&regs->miimadd, (mii_id << 8) | regnum);
@@ -99,9 +102,11 @@ int fsl_pq_local_mdio_read(struct fsl_pq_mdio __iomem *regs,
 	out_be32(&regs->miimcom, 0);
 	out_be32(&regs->miimcom, MII_READ_COMMAND);
 
-	/* Wait for the transaction to finish */
-	while (in_be32(&regs->miimind) & (MIIMIND_NOTVALID | MIIMIND_BUSY))
-		cpu_relax();
+	/* Wait for the transaction to finish, normally less than 100us */
+	status = spin_event_timeout(!(in_be32(&regs->miimind) &
+		(MIIMIND_NOTVALID | MIIMIND_BUSY)), 1000, 0);
+	if (!status)
+		return -ETIMEDOUT;
 
 	/* Grab the value of the register from miimstat */
 	value = in_be32(&regs->miimstat);
@@ -144,7 +149,7 @@ int fsl_pq_mdio_read(struct mii_bus *bus, int mii_id, int regnum)
 static int fsl_pq_mdio_reset(struct mii_bus *bus)
 {
 	struct fsl_pq_mdio __iomem *regs = fsl_pq_mdio_get_regs(bus);
-	int timeout = PHY_INIT_TIMEOUT;
+	u32 status;
 
 	mutex_lock(&bus->mdio_lock);
 
@@ -155,12 +160,12 @@ static int fsl_pq_mdio_reset(struct mii_bus *bus)
 	out_be32(&regs->miimcfg, MIIMCFG_INIT_VALUE);
 
 	/* Wait until the bus is free */
-	while ((in_be32(&regs->miimind) & MIIMIND_BUSY) && timeout--)
-		cpu_relax();
+	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
+		1000, 0);
 
 	mutex_unlock(&bus->mdio_lock);
 
-	if (timeout < 0) {
+	if (!status) {
 		printk(KERN_ERR "%s: The MII Bus is stuck!\n",
 				bus->name);
 		return -EBUSY;
-- 
1.7.3.4

Re: [PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[David Miller]

From: Timur Tabi <timur@freescale.com>
Date: Tue, 3 Jul 2012 18:16:21 -0500

> Macro spin_event_timeout() was designed for simple polling of hardware
> registers with a timeout, so use it when we poll the MIIMIND register.
> This allows us to return an error code instead of polling indefinitely.
> 
> Note that PHY_INIT_TIMEOUT is a count of loop iterations, so we can't use
> it for spin_event_timeout(), which asks for microseconds.
> 
> Signed-off-by: Timur Tabi <timur@freescale.com>

Define a macro for the timeout value rather than use an arbitrary
constant.

> +	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
> +		1000, 0);

This indentation is absolutely terrible.

> +	status = spin_event_timeout(!(in_be32(&regs->miimind) &
> +		(MIIMIND_NOTVALID | MIIMIND_BUSY)), 1000, 0);

Same here.

> +	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
> +		1000, 0);

And here too.

Re: [PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[Tabi Timur-B04825]

David Miller wrote:

> Define a macro for the timeout value rather than use an arbitrary
> constant.

Ok.

>> +	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
>> +		1000, 0);
>
> This indentation is absolutely terrible.

Can you give me a clue as to how you think it should look?  I could not 
come up with a good way to break up these lines and keep them under 80 
characters.

-- 
Timur Tabi
Linux kernel developer at Freescale

Re: [PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[Jan Ceuleers]

On 07/09/2012 04:31 PM, Tabi Timur-B04825 wrote:
>> This indentation is absolutely terrible.
> 
> Can you give me a clue as to how you think it should look?  I could not 
> come up with a good way to break up these lines and keep them under 80 
> characters.
> 

How about this:

+	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
+				    1000, 0);

On the second line use as many tabs as possible, then continue with spaces until the '1' in 1000 lines up with the '!'.

HTH, Jan

Re: [PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[David Miller]

From: Tabi Timur-B04825 <B04825@freescale.com>
Date: Mon, 9 Jul 2012 14:31:33 +0000

> David Miller wrote:
> 
>> Define a macro for the timeout value rather than use an arbitrary
>> constant.
> 
> Ok.
> 
>>> +	status = spin_event_timeout(!(in_be32(&regs->miimind) &	MIIMIND_BUSY),
>>> +		1000, 0);
>>
>> This indentation is absolutely terrible.
> 
> Can you give me a clue as to how you think it should look?  I could not 
> come up with a good way to break up these lines and keep them under 80 
> characters.

It's not the length of the first line, it's how the second line was
indented.

Always line up the arguments on the second and subsequent lines right
after the column containing the openning "(" on the first line.

	foo = function(arg1,
		       arg2);

Do you really mean that:

	foo = function(arg1,
		arg2);

doesn't look like complete and utter shit to you?

Re: [PATCH] net/fsl_pq_mdio: use spin_event_timeout() to poll the indicator register

[Timur Tabi]

David Miller wrote:
> Do you really mean that:
> 
> 	foo = function(arg1,
> 		arg2);
> 
> doesn't look like complete and utter shit to you?

It doesn't bother me, but I'll change it.

-- 
Timur Tabi
Linux kernel developer at Freescale