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

["Marek Behún" <kabel@kernel.org>]

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.sharma@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.