Pavana, some last nitpicks, sorry I didn't check this before.
I will send another patch to you which shall fix all this things.
On Fri, 8 Jan 2021 19:50:56 +1000
Pavana Sharma <pavana.sha...@digi.com> wrote:
> +/* Support 10, 100, 200, 1000, 2500, 5000, 10000 Mbps (e.g. 88E6393X)
> + * This function adds new speed 5000 supported by Amethyst family.
> + * Function mv88e6xxx_port_set_speed_duplex() can't be used as the register
> + * values for speeds 2500 & 5000 conflict.
> + */
> +
> +int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
> + int speed, int duplex)
There are basically 2 empty lines between the comment and the function
signature, one containting only comment end token " */" and the other
truly empty. I think you can remove the empty line, since the comment
is already visibly separated from the function body.
I think you can also remove the second line of the comment:
"This function adds new speed 5000 supported by Amethyst family."
The alignment of the speed argument is wrong. It should start at the
same column as the first argument.
So:
/* Support 10, 100, 200, 1000, 2500, 5000, 10000 Mbps (e.g. 88E6393X)
* Function mv88e6xxx_port_set_speed_duplex() can't be used as the register
* values for speeds 2500 & 5000 conflict.
*/
int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
int speed, int duplex)
> + reg &= ~(MV88E6XXX_PORT_MAC_CTL_SPEED_MASK |
> + MV88E6390_PORT_MAC_CTL_ALTSPEED |
> + MV88E6390_PORT_MAC_CTL_FORCE_SPEED);
alignment:
reg &= ~(MV88E6XXX_PORT_MAC_CTL_SPEED_MASK |
MV88E6390_PORT_MAC_CTL_ALTSPEED |
MV88E6390_PORT_MAC_CTL_FORCE_SPEED);
> +static int mv88e6393x_port_policy_write(struct mv88e6xxx_chip *chip, u16
> pointer,
> + u8 data)
> +{
> +
> + int err = 0;
Unneeded empty line before int err = 0;
Also alignment of the data argument:
static int mv88e6393x_port_policy_write(struct mv88e6xxx_chip *chip, u16
pointer,
u8 data)
(It seems to me that you only align with tabs. If you need to align for
example to 20th column, use 2 tabs and 4 spaces (2*8 + 4 = 20).)
> +int mv88e6393x_port_set_ether_type(struct mv88e6xxx_chip *chip, int port,
> + u16 etype)
Alignment:
int mv88e6393x_port_set_ether_type(struct mv88e6xxx_chip *chip, int port,
u16 etype)
> +#define MV88E6XXX_PORT_STS_CMODE_5GBASER 0x000c
> +#define MV88E6XXX_PORT_STS_CMODE_10GBASER 0x000d
> +#define MV88E6XXX_PORT_STS_CMODE_USXGMII 0x000e
These macros should have prefix
MV88E6393X_
instead of
MV88E6XXX_
since these values apply for 6393X family and they conflict with the
values for other switches.
> +int mv88e6xxx_port_wait_bit(struct mv88e6xxx_chip *chip, int port, int reg,
> + int bit, int val);
Alignment.
> +int mv88e6393x_port_set_speed_duplex(struct mv88e6xxx_chip *chip, int port,
> + int speed, int duplex);
Alignment.
> +int mv88e6393x_port_set_ether_type(struct mv88e6xxx_chip *chip, int port,
> + u16 etype);
Alignment.
> +/* Only Ports 0, 9 and 10 have SERDES lanes. Return the SERDES lane address
> + * a port is using else Returns -ENODEV.
> + */
> +int mv88e6393x_serdes_get_lane(struct mv88e6xxx_chip *chip, int port)
> +{
> + u8 cmode = chip->ports[port].cmode;
> + int lane = -ENODEV;
> +
> + if (port != 0 && port != 9 && port != 10)
> + return -EOPNOTSUPP;
> +
> + if (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASEX ||
> + cmode == MV88E6XXX_PORT_STS_CMODE_SGMII ||
> + cmode == MV88E6XXX_PORT_STS_CMODE_2500BASEX ||
> + cmode == MV88E6XXX_PORT_STS_CMODE_5GBASER ||
> + cmode == MV88E6XXX_PORT_STS_CMODE_10GBASER ||
> + cmode == MV88E6XXX_PORT_STS_CMODE_USXGMII)
Alignment.
> + lane = port;
> + return lane;
> +}
> +int mv88e6393x_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port,
> + int lane, bool enable)
Alignment.
> +irqreturn_t mv88e6393x_serdes_irq_status(struct mv88e6xxx_chip *chip, int
> port,
> + int lane)
Alignment.
> +static int mv88e6393x_serdes_port_config(struct mv88e6xxx_chip *chip, int
> lane,
> + bool on)
Alignment.
> + mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> + MV88E6393X_SERDES_POC, &config);
> + config &= ~(MV88E6393X_SERDES_POC_PCS_MODE_MASK |
> + MV88E6393X_SERDES_POC_PDOWN);
> + config |= pcs;
> + mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
> + MV88E6393X_SERDES_POC, config);
> + config |= MV88E6393X_SERDES_POC_RESET;
> + mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
> + MV88E6393X_SERDES_POC, config);
> +
> + /* mv88e6393x family errata 3.7 :
> + * When changing cmode on SERDES port from any other mode to
> + * 1000BASE-X mode the link may not come up due to invalid
> + * 1000BASE-X advertisement.
> + * Workaround: Correct advertisement and reset PHY core.
> + */
> + config = MV88E6390_SGMII_ANAR_1000BASEX_FD;
> + mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
> + MV88E6390_SGMII_ANAR, config);
> +
> + /* soft reset the PCS/PMA */
> + mv88e6390_serdes_read(chip, lane, MDIO_MMD_PHYXS,
> + MV88E6390_SGMII_CONTROL, &config);
> + config |= MV88E6390_SGMII_CONTROL_RESET;
> + mv88e6390_serdes_write(chip, lane, MDIO_MMD_PHYXS,
> + MV88E6390_SGMII_CONTROL, config);
Alignment in all calls to mv88e6390_serdes_write.
> +int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
> + bool on)
Alignment.
> +int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
> + bool on);
Alignment.
> +int mv88e6393x_serdes_irq_enable(struct mv88e6xxx_chip *chip, int port,
> + int lane, bool enable);
Alignment.
> +irqreturn_t mv88e6393x_serdes_irq_status(struct mv88e6xxx_chip *chip, int
> port,
> + int lane);
Alignment.
