Re: [PATCH v8 4/4] net: dsa: mv88e6xxx: Add support for mv88e6393x family of Marvell

On Tue,  3 Nov 2020 18:50:38 +1000 Pavana Sharma wrote:

The Marvell 88E6393X device is a single-chip integration of a 11-port
Ethernet switch with eight integrated Gigabit Ethernet (GbE) transceivers
and three 10-Gigabit interfaces.

This patch adds functionalities specific to mv88e6393x family (88E6393X,
88E6193X and 88E6191X)

Please fix all checkpatch --strict --min-conf-desc-length=80 warnings
and what I point out below

Co-developed-by: Ashkan Boldaji <redacted>
Signed-off-by: Ashkan Boldaji <redacted>
Signed-off-by: Pavana Sharma <redacted>

+	reg &= ~(MV88E6XXX_PORT_MAC_CTL_SPEED_MASK |
+			MV88E6390_PORT_MAC_CTL_ALTSPEED |
+			MV88E6390_PORT_MAC_CTL_FORCE_SPEED);

Align the continuation lines under the opening bracket, like the kernel
coding style require, please.

+
+	if (speed != SPEED_UNFORCED)
+		reg |= MV88E6390_PORT_MAC_CTL_FORCE_SPEED;
+
+	reg |= ctrl;
+
+	err = mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_MAC_CTL, reg);
+	if (err)
+		return err;
+
+	return 0;

no need to set err, just directly do:

	return mv88e6xxx_port_write(chip, port, MV88E6XXX_PORT_MAC_CTL,
	reg);

+}
+
 static int mv88e6xxx_port_set_cmode(struct mv88e6xxx_chip *chip, int port,
 				    phy_interface_t mode, bool force)
 {

+/* Offset 0x0E: Policy & MGMT Control Register for FAMILY 6191X 6193X 6393X*/

Missing space at the end of that comment.

+static int mv88e6393x_port_policy_write(struct mv88e6xxx_chip *chip, u16 pointer,
+				u8 data)
+{
+
+	int err = 0;
+	int port;
+	u16 reg;
+
+	/* Setup per Port policy register */
+	for (port = 0; port < mv88e6xxx_num_ports(chip); port++) {
+		if (dsa_is_unused_port(chip->ds, port))
+			continue;
+
+		/* Prevent the use of an invalid port. */
+		if (mv88e6xxx_is_invalid_port(chip, port)) {
+			dev_err(chip->dev, "port %d is invalid\n", port);
+			err = -EINVAL;

did you mean to exit here? this assignment looks pointless

+		}
+		reg = MV88E6393X_PORT_POLICY_MGMT_CTL_UPDATE | pointer | data;
+		err = mv88e6xxx_port_write(chip, port, MV88E6393X_PORT_POLICY_MGMT_CTL, reg);
+	}
+	return err;
+}

+int mv88e6393x_port_mgmt_rsvd2cpu(struct mv88e6xxx_chip *chip)
+{
+	u16 ptr;
+	int err;
+
+	/* Consider the frames with reserved multicast destination
+	 * addresses matching 01:80:c2:00:00:00 and
+	 * 01:80:c2:00:00:02 as MGMT.
+	 */
+	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XLO;
+	err = mv88e6393x_port_policy_write(chip, ptr, 0xff);
+	if (err)
+		return err;
+
+	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000000XHI;
+	err = mv88e6393x_port_policy_write(chip, ptr, 0xff);
+	if (err)
+		return err;
+
+	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XLO;
+	err = mv88e6393x_port_policy_write(chip, ptr, 0xff);
+	if (err)
+		return err;
+
+	ptr = MV88E6393X_PORT_POLICY_MGMT_CTL_PTR_01C280000002XHI;
+	err = mv88e6393x_port_policy_write(chip, ptr, 0xff);
+	if (err)
+		return err;
+
+	return 0;

return mv...

+}
+

+	err = mv88e6xxx_port_write(chip, port, MV88E6393X_PORT_EPC_CMD, val);
+	if (err)
+		return err;
+
+	return 0;

ditto

+}

+
+int mv88e6393x_serdes_power(struct mv88e6xxx_chip *chip, int port, int lane,
+		    bool on)
+{
+	if (port == 0 || port == 9 || port == 10) {

Flip the condition, return early. Entire body of a function should not
have to be indented.

+		u8 cmode = chip->ports[port].cmode;
+
+		mv88e6393x_serdes_port_config(chip, lane, on);
+
+		switch (cmode) {
+		case MV88E6XXX_PORT_STS_CMODE_1000BASEX:
+		case MV88E6XXX_PORT_STS_CMODE_2500BASEX:
+			return mv88e6390_serdes_power_sgmii(chip, lane, on);
+		case MV88E6XXX_PORT_STS_CMODE_10GBASER:
+			return mv88e6390_serdes_power_10g(chip, lane, on);
+		}
+	}
+
+	return 0;
+}

Looks like this should be in patch 3?

 {
 	if (!chip->info->ops->serdes_get_lane)