Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752403AbdLLSIa (ORCPT ); Tue, 12 Dec 2017 13:08:30 -0500 Received: from mail.savoirfairelinux.com ([208.88.110.44]:34792 "EHLO mail.savoirfairelinux.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751871AbdLLSI0 (ORCPT ); Tue, 12 Dec 2017 13:08:26 -0500 From: Vivien Didelot To: Egil Hjelmeland , andrew@lunn.ch, f.fainelli@gmail.com, netdev@vger.kernel.org, linux-kernel@vger.kernel.org Cc: Egil Hjelmeland Subject: Re: [PATCH net-next] net: dsa: lan9303: Introduce lan9303_read_wait In-Reply-To: <20171212175331.22599-1-privat@egil-hjelmeland.no> References: <20171212175331.22599-1-privat@egil-hjelmeland.no> Date: Tue, 12 Dec 2017 13:08:24 -0500 Message-ID: <87o9n34y5j.fsf@weeman.i-did-not-set--mail-host-address--so-tickle-me> MIME-Version: 1.0 Content-Type: text/plain Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3204 Lines: 112 Hi Egil, Egil Hjelmeland 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 > --- > 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