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

[Egil Hjelmeland]

Hi Vivien.

Den 12. des. 2017 19:08, skrev Vivien Didelot:
> Hi Egil,
>
> Egil Hjelmeland <privat@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> >   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.

> > +		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, &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