Re: [PATCH v2 net-next 7/8] net: dsa: microchip: Prepare PHY for proper advertisement

[Andrew Lunn <andrew@lunn.ch>]

Hi Tristram

> +static void ksz9477_phy_setup(struct ksz_device *dev, int port,
> +			      struct phy_device *phy)
> +{
> +	if (port < dev->phy_port_cnt) {
> +		/* SUPPORTED_Asym_Pause and SUPPORTED_Pause can be removed to
> +		 * disable flow control when rate limiting is used.
> +		 */
> +		phy->advertising = phy->supported;
> +	}

This looks a bit odd. phy_probe() does the same:

       /* Start out supporting everything. Eventually,
         * a controller will attach, and may modify one
         * or both of these values
         */
        phydev->supported = phydrv->features;
        of_set_phy_supported(phydev);
        phydev->advertising = phydev->supported;

You don't modify anything here, so i don't see why it is needed.

> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> +		     struct phy_device *phydev)
> +{
> +	struct ksz_device *dev = ds->priv;
> +	struct ksz_port *p = &dev->ports[port];
> +
> +	if (phydev->link) {
> +		p->speed = phydev->speed;
> +		p->duplex = phydev->duplex;
> +		p->flow_ctrl = phydev->pause;
> +		p->link_up = 1;
> +		dev->live_ports |= (1 << port) & dev->on_ports;
> +	} else if (p->link_up) {
> +		p->link_up = 0;
> +		p->link_down = 1;
> +		dev->live_ports &= ~(1 << port);
> +	}

The link_down looks odd. If you are setting link_up to 1, should
link_down also be set to 0? Can it be both up and down at the same
time?

	Andrew