Re: [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink

["Russell King (Oracle)" <linux@armlinux.org.uk>]

Hi,

A couple of niggles. I know that the patches have already been merged
while I wasn't able to review, but maybe you can address these points.

On Fri, Sep 06, 2024 at 04:05:10PM +0530, Raju Lakkaraju wrote:
> +static void lan743x_phylink_mac_link_up(struct phylink_config *config,
> +					struct phy_device *phydev,
> +					unsigned int link_an_mode,
> +					phy_interface_t interface,
> +					int speed, int duplex,
> +					bool tx_pause, bool rx_pause)
> +{
> +	struct net_device *netdev = to_net_dev(config->dev);
> +	struct lan743x_adapter *adapter = netdev_priv(netdev);
> +	int mac_cr;
> +	u8 cap;
> +
> +	mac_cr = lan743x_csr_read(adapter, MAC_CR);
> +	/* Pre-initialize register bits.
> +	 * Resulting value corresponds to SPEED_10
> +	 */
> +	mac_cr &= ~(MAC_CR_CFG_H_ | MAC_CR_CFG_L_);
> +	if (speed == SPEED_2500)
> +		mac_cr |= MAC_CR_CFG_H_ | MAC_CR_CFG_L_;
> +	else if (speed == SPEED_1000)
> +		mac_cr |= MAC_CR_CFG_H_;
> +	else if (speed == SPEED_100)
> +		mac_cr |= MAC_CR_CFG_L_;
> +
> +	lan743x_csr_write(adapter, MAC_CR, mac_cr);
> +
> +	lan743x_ptp_update_latency(adapter, speed);
> +
> +	/* Flow Control operation */
> +	cap = 0;
> +	if (tx_pause)
> +		cap |= FLOW_CTRL_TX;
> +	if (rx_pause)
> +		cap |= FLOW_CTRL_RX;
> +
> +	lan743x_mac_flow_ctrl_set_enables(adapter,
> +					  cap & FLOW_CTRL_TX,
> +					  cap & FLOW_CTRL_RX);

I'm wondeing about this, which looks to me to be over-complex code.

void lan743x_mac_flow_ctrl_set_enables(struct lan743x_adapter *adapter,
                                       bool tx_enable, bool rx_enable)

This function takes two booleans. You're passing cap & FLOW_CTRL_TX
and cap & FLOW_CTRL_RX to this function for these. However, you are
setting these bits in "cap" immediately above from a pair of booleans
and nowhere else. So why not:

	lan743x_mac_flow_ctrl_set_enables(adapter, tx_pause, rx_pause);

?

> @@ -3115,13 +3217,13 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  	if (ret)
>  		goto close_intr;
>  
> -	ret = lan743x_phy_open(adapter);
> +	ret = lan743x_phylink_connect(adapter);
>  	if (ret)
>  		goto close_mac;
>  
>  	ret = lan743x_ptp_open(adapter);
>  	if (ret)
> -		goto close_phy;
> +		goto close_mac;
>  
>  	lan743x_rfe_open(adapter);
>  
> @@ -3161,9 +3263,8 @@ static int lan743x_netdev_open(struct net_device *netdev)
>  			lan743x_rx_close(&adapter->rx[index]);
>  	}
>  	lan743x_ptp_close(adapter);
> -
> -close_phy:
> -	lan743x_phy_close(adapter);
> +	if (adapter->phylink)
> +		lan743x_phylink_disconnect(adapter);

I'm not sure why this is conditional on adapter->phylink. Surely this
test will always be true?

Thanks.

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