Re: [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink
From: "Russell King (Oracle)" <linux@armlinux.org.uk>
Date: 2024-09-17 15:59:14
Also in:
lkml
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);
?
quoted hunk ↗ jump to hunk
@@ -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!