| Message ID | 20171212175331.22599-1-privat@egil-hjelmeland.no |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: dsa: lan9303: Introduce lan9303_read_wait | expand |
Hi Egil, Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > 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 <privat@egil-hjelmeland.no> > --- > 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, ®); > + 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. Looking at how lan9303_read_wait is called, the value argument doesn't seem necessary. You can directly return 0 if (!(reg & mask)). > + usleep_range(1000, 2000); > + } > + return -ETIMEDOUT; A newline before the return statment would be appreciated. > +} > + > 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, ®); > - 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, ®); > - 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
Hi Vivien. Den 12. des. 2017 19:08, skrev Vivien Didelot: > Hi Egil, > > Egil Hjelmeland <privat@egil-hjelmeland.no> writes: > >> 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 <privat@egil-hjelmeland.no> >> --- >> 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, ®); >> + 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. >> + usleep_range(1000, 2000); >> + } >> + return -ETIMEDOUT; > > A newline before the return statment would be appreciated. > Ok. >> +} >> + >> 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, ®); >> - 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, ®); >> - 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
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, ®); + 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, ®); - 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, ®); - 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)
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 <privat@egil-hjelmeland.no> --- drivers/net/dsa/lan9303-core.c | 59 +++++++++++++++++++----------------------- 1 file changed, 27 insertions(+), 32 deletions(-)