| Message ID | cover.1604298276.git.pavana.sharma@digi.com |
|---|---|
| Headers | show |
| Series | Add support for mv88e6393x family of Marvell | expand |
On Mon, Nov 02, 2020 at 04:40:02PM +1000, Pavana Sharma wrote: > Thanks for the review. > Here's updated patchset. Please include a short description of what you have changed since the last version. It helps us as maintainers see if you have attempted to make the changes we have requested, or not. Andrew
> @@ -985,12 +985,12 @@ int mv88e6390_serdes_get_regs_len(struct mv88e6xxx_chip *chip, int port) > void mv88e6390_serdes_get_regs(struct mv88e6xxx_chip *chip, int port, void *_p) > { > u16 *p = _p; > - int lane; > + int lane = -ENODEV; > u16 reg; > int i; The reverse christmas tree is wrong here. Andrew
On Mon, Nov 02, 2020 at 04:43:09PM +1000, Pavana Sharma wrote: > Returning 0 is no more an error case with MV88E6393 family > which has serdes lane numbers 0, 9 or 10. > So with this change .serdes_get_lane will return lane number > or error (-ENODEV). > > Signed-off-by: Pavana Sharma <pavana.sharma@digi.com> > --- > drivers/net/dsa/mv88e6xxx/chip.c | 28 +++++------ > drivers/net/dsa/mv88e6xxx/chip.h | 16 +++---- > drivers/net/dsa/mv88e6xxx/port.c | 6 +-- > drivers/net/dsa/mv88e6xxx/serdes.c | 76 +++++++++++++++--------------- > drivers/net/dsa/mv88e6xxx/serdes.h | 50 ++++++++++---------- > 5 files changed, 88 insertions(+), 88 deletions(-) > > diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c > index f0dbc05e30a4..4994b8eee659 100644 > --- a/drivers/net/dsa/mv88e6xxx/chip.c > +++ b/drivers/net/dsa/mv88e6xxx/chip.c > @@ -484,12 +484,12 @@ static int mv88e6xxx_serdes_pcs_get_state(struct dsa_switch *ds, int port, > struct phylink_link_state *state) > { > struct mv88e6xxx_chip *chip = ds->priv; > - u8 lane; > + int lane = -ENODEV; > int err; You have added a lot of initialises which are not needed. > > mv88e6xxx_reg_lock(chip); > lane = mv88e6xxx_serdes_get_lane(chip, port); lane is always set, so there is no point in setting it to -ENODEV first. > - if (lane && chip->info->ops->serdes_pcs_get_state) > + if ((lane >= 0) && chip->info->ops->serdes_pcs_get_state) > err = chip->info->ops->serdes_pcs_get_state(chip, port, lane, > state); > else > @@ -505,11 +505,11 @@ static int mv88e6xxx_serdes_pcs_config(struct mv88e6xxx_chip *chip, int port, > const unsigned long *advertise) > { > const struct mv88e6xxx_ops *ops = chip->info->ops; > - u8 lane; > + int lane = -ENODEV; > > if (ops->serdes_pcs_config) { > lane = mv88e6xxx_serdes_get_lane(chip, port); > - if (lane) > + if (lane >= 0) > return ops->serdes_pcs_config(chip, port, lane, mode, > interface, advertise); > } > @@ -521,15 +521,15 @@ static void mv88e6xxx_serdes_pcs_an_restart(struct dsa_switch *ds, int port) > { > struct mv88e6xxx_chip *chip = ds->priv; > const struct mv88e6xxx_ops *ops; > + int lane = -ENODEV; > int err = 0; > - u8 lane; > > ops = chip->info->ops; > > if (ops->serdes_pcs_an_restart) { > mv88e6xxx_reg_lock(chip); > lane = mv88e6xxx_serdes_get_lane(chip, port); > - if (lane) lane is always set inside this if statement, and is never used outside of it. > + if (lane >= 0) > err = ops->serdes_pcs_an_restart(chip, port, lane); > mv88e6xxx_reg_unlock(chip); > > @@ -543,11 +543,11 @@ static int mv88e6xxx_serdes_pcs_link_up(struct mv88e6xxx_chip *chip, int port, > int speed, int duplex) > { > const struct mv88e6xxx_ops *ops = chip->info->ops; > - u8 lane; > + int lane = -ENODEV; > > if (!phylink_autoneg_inband(mode) && ops->serdes_pcs_link_up) { > lane = mv88e6xxx_serdes_get_lane(chip, port); > - if (lane) > + if (lane >= 0) > return ops->serdes_pcs_link_up(chip, port, lane, lane is always set, and never used outside of the if .... Andrew