Re: [PATCH net-next v5 2/3] net: dsa: add Arrow SpeedChips XRS700x driver

[Vladimir Oltean <olteanv@gmail.com>]

On Thu, Jan 14, 2021 at 01:57:33PM -0600, George McCollister wrote:
> Add a driver with initial support for the Arrow SpeedChips XRS7000
> series of gigabit Ethernet switch chips which are typically used in
> critical networking applications.
> 
> The switches have up to three RGMII ports and one RMII port.
> Management to the switches can be performed over i2c or mdio.
> 
> Support for advanced features such as PTP and
> HSR/PRP (IEC 62439-3 Clause 5 & 4) is not included in this patch and
> may be added at a later date.
> 
> Signed-off-by: George McCollister <george.mccollister@gmail.com>
> ---

Reviewed-by: Vladimir Oltean <olteanv@gmail.com>

This driver is good to go, just one small nitpick below, you can fix it
up afterwards if you want.

> +static void xrs700x_port_stp_state_set(struct dsa_switch *ds, int port,
> +				       u8 state)
> +{
> +	struct xrs700x *priv = ds->priv;
> +	unsigned int bpdus = 1;
> +	unsigned int val;
> +
> +	switch (state) {
> +	case BR_STATE_DISABLED:
> +		bpdus = 0;
> +		fallthrough;
> +	case BR_STATE_BLOCKING:
> +	case BR_STATE_LISTENING:
> +		val = XRS_PORT_DISABLED;
> +		break;
> +	case BR_STATE_LEARNING:
> +		val = XRS_PORT_LEARNING;
> +		break;
> +	case BR_STATE_FORWARDING:
> +		val = XRS_PORT_FORWARDING;
> +		break;
> +	default:
> +		dev_err(ds->dev, "invalid STP state: %d\n", state);
> +		return;
> +	}
> +
> +	regmap_fields_write(priv->ps_forward, port, val);
> +
> +	/* Enable/disable inbound policy added by xrs700x_port_add_bpdu_ipf()
> +	 * which allows BPDU forwarding to the CPU port when the front facing
> +	 * port is in disabled/learning state.
                      ~~~~~~~~
You probably mean blocking. When the port is in BR_STATE_DISABLED, you
set bpdus = 1, which makes sense.

> +	 */
> +	regmap_update_bits(priv->regmap, XRS_ETH_ADDR_CFG(port, 0), 1, bpdus);
> +
> +	dev_dbg_ratelimited(priv->dev, "%s - port: %d, state: %u, val: 0x%x\n",
> +			    __func__, port, state, val);
> +}