Hi Jonathan,
A quick read through on the first review...
On Mon, Jun 08, 2020 at 07:39:53PM +0100, Jonathan McDowell wrote:
> +static int
> +qca8k_phylink_mac_link_state(struct dsa_switch *ds, int port,
> + struct phylink_link_state *state)
> +{
> + struct qca8k_priv *priv = ds->priv;
> + u32 reg;
> +
> + reg = qca8k_read(priv, QCA8K_REG_PORT_STATUS(port));
> +
> + state->link = (reg & QCA8K_PORT_STATUS_LINK_UP);
> + state->an_complete = state->link;
> + state->an_enabled = (reg & QCA8K_PORT_STATUS_LINK_AUTO);
I much prefer to use !!(reg & ...) since these are single-bit
bitfields.
> + state->duplex = (reg & QCA8K_PORT_STATUS_DUPLEX) ? DUPLEX_FULL :
> + DUPLEX_HALF;
> +
> + switch (reg & QCA8K_PORT_STATUS_SPEED) {
> + case QCA8K_PORT_STATUS_SPEED_10:
> + state->speed = SPEED_10;
> + break;
> + case QCA8K_PORT_STATUS_SPEED_100:
> + state->speed = SPEED_100;
> + break;
> + case QCA8K_PORT_STATUS_SPEED_1000:
> + state->speed = SPEED_1000;
> + break;
> + default:
> + state->speed = SPEED_UNKNOWN;
> + break;
> + }
>
> - /* Set duplex mode */
> - if (phy->duplex == DUPLEX_FULL)
> - reg |= QCA8K_PORT_STATUS_DUPLEX;
> + state->pause = MLO_PAUSE_NONE;
> + if (reg & QCA8K_PORT_STATUS_RXFLOW)
> + state->pause |= MLO_PAUSE_RX;
> + if (reg & QCA8K_PORT_STATUS_TXFLOW)
> + state->pause |= MLO_PAUSE_TX;
> + if (reg & QCA8K_PORT_STATUS_FLOW_AUTO)
> + state->pause |= MLO_PAUSE_AN;
There is no need to report back MLO_PAUSE_AN.
>From the quick read of this, nothing else obviously stood out.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC for 0.8m (est. 1762m) line in suburbia: sync at 13.1Mbps down 503kbps up
