| Message ID | 1341357381-10861-1-git-send-email-timur@freescale.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
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(®s->miimind) & MIIMIND_BUSY), > + 1000, 0); This indentation is absolutely terrible. > + status = spin_event_timeout(!(in_be32(®s->miimind) & > + (MIIMIND_NOTVALID | MIIMIND_BUSY)), 1000, 0); Same here. > + status = spin_event_timeout(!(in_be32(®s->miimind) & MIIMIND_BUSY), > + 1000, 0); And here too. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
David Miller wrote: > Define a macro for the timeout value rather than use an arbitrary > constant. Ok. >> + status = spin_event_timeout(!(in_be32(®s->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.
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(®s->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 -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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(®s->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? -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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(®s->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(®s->miimcon, value); /* Wait for the transaction to finish */ - while (in_be32(®s->miimind) & MIIMIND_BUSY) - cpu_relax(); + status = spin_event_timeout(!(in_be32(®s->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(®s->miimadd, (mii_id << 8) | regnum); @@ -99,9 +102,11 @@ int fsl_pq_local_mdio_read(struct fsl_pq_mdio __iomem *regs, out_be32(®s->miimcom, 0); out_be32(®s->miimcom, MII_READ_COMMAND); - /* Wait for the transaction to finish */ - while (in_be32(®s->miimind) & (MIIMIND_NOTVALID | MIIMIND_BUSY)) - cpu_relax(); + /* Wait for the transaction to finish, normally less than 100us */ + status = spin_event_timeout(!(in_be32(®s->miimind) & + (MIIMIND_NOTVALID | MIIMIND_BUSY)), 1000, 0); + if (!status) + return -ETIMEDOUT; /* Grab the value of the register from miimstat */ value = in_be32(®s->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(®s->miimcfg, MIIMCFG_INIT_VALUE); /* Wait until the bus is free */ - while ((in_be32(®s->miimind) & MIIMIND_BUSY) && timeout--) - cpu_relax(); + status = spin_event_timeout(!(in_be32(®s->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;
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(-)