Re: [net-next PATCH v13 4/4] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell
From: Marek Behún <kabel@kernel.org>
Date: 2021-01-08 13:52:11
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 [off-list ref] 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.