[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ZumnRtZYBLIyI/Gm@shell.armlinux.org.uk>
Date: Tue, 17 Sep 2024 16:59:02 +0100
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Raju Lakkaraju <Raju.Lakkaraju@...rochip.com>
Cc: netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
andrew@...n.ch, hkallweit1@...il.com, richardcochran@...il.com,
rdunlap@...radead.org, bryan.whitehead@...rochip.com,
edumazet@...gle.com, pabeni@...hat.com,
maxime.chevallier@...tlin.com, linux-kernel@...r.kernel.org,
horms@...nel.org, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink
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!
Powered by blists - more mailing lists