Re: [PATCH net-next v3 3/5] net: pcs: qcom-ipq9574: Add PCS instantiation and phylink operations

[Russell King (Oracle)]

Hi,

On Mon, Dec 16, 2024 at 09:40:25PM +0800, Lei Wei wrote:
> +static int ipq_pcs_config_sgmii(struct ipq_pcs *qpcs,
> +				int index,
> +				unsigned int neg_mode,
> +				phy_interface_t interface)
> +{
> +	int ret;
> +
> +	/* Access to PCS registers such as PCS_MODE_CTRL which are
> +	 * common to all MIIs, is lock protected and configured
> +	 * only once.
> +	 */
> +	mutex_lock(&qpcs->config_lock);
> +
> +	if (qpcs->interface != interface) {
> +		ret = ipq_pcs_config_mode(qpcs, interface);
> +		if (ret) {
> +			mutex_unlock(&qpcs->config_lock);
> +			return ret;
> +		}
> +	}
> +
> +	mutex_unlock(&qpcs->config_lock);

Phylink won't make two concurrent calls to this function (it's protected
by phylink's state_lock). Since this looks to me like "qpcs" is per PCS,
the lock does nothing that phylink doesn't already do.

> +static const struct phylink_pcs_ops ipq_pcs_phylink_ops = {
> +	.pcs_validate = ipq_pcs_validate,

I would also like to see the recently added .pcs_inband_caps() method
implemented too, so that phylink gets to know whether inband can be
supported by the PCS.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!