Re: [PATCH net-next v3 3/8] net: qualcomm: ipqess: introduce the Qualcomm IPQESS driver

[Romain Gantois <romain.gantois@bootlin.com>]

On Tue, 14 Nov 2023, Andrew Lunn wrote:
...
> > +	phy_support_asym_pause(phy);
> > +
> > +	ipqess_port_set_state_now(port, BR_STATE_FORWARDING, false);
> > +
> > +	if (port->pl)
> > +		phylink_start(port->pl);
> 
> That looks odd. You unconditionally call phy_support_asym_pause() yet
> conditionally call phylink_start(). I would expect there to always be
> a phylink instance.
> 
> Also, you should be telling phylink about the pause capabilities in
> config->mac_capabilities. It is then phylinks problem to tell the PHY,
> or the PCS driving the SFP etc about pause.
You are correct. I probably fumbled this when splitting the calibration code.

> > +		if (tx_pause || port->index == 0)
> > +			reg |= QCA8K_PORT_STATUS_TXFLOW;
> > +	}
> > +
> > +	reg |= QCA8K_PORT_STATUS_TXMAC | QCA8K_PORT_STATUS_RXMAC;
> > +
> > +	qca8k_write(priv, QCA8K_REG_PORT_STATUS(port->index), reg);
> > +}
> 
> qca8k_phylink_mac_link_up() with some refactoring can be
> reused. Please look through the driver and find other instances like
> this where you can reuse more code.
I tried to be conservative with modifying qca8k-common.c when it required 
modifying qca8k-8xxx.c. But I'll factor this code more aggressively since you 
think it is preferable.

Best,

-- 
Romain Gantois, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com