From: "Russell King (Oracle)" <linux@armlinux•org.uk>
To: Raju Lakkaraju <Raju.Lakkaraju@microchip•com>
Cc: netdev@vger•kernel.org, davem@davemloft•net, kuba@kernel•org,
andrew@lunn•ch, hkallweit1@gmail•com, richardcochran@gmail•com,
rdunlap@infradead•org, bryan.whitehead@microchip•com,
edumazet@google•com, pabeni@redhat•com,
maxime.chevallier@bootlin•com, linux-kernel@vger•kernel.org,
horms@kernel•org, UNGLinuxDriver@microchip•com
Subject: Re: [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink
Date: Tue, 17 Sep 2024 16:59:02 +0100 [thread overview]
Message-ID: <ZumnRtZYBLIyI/Gm@shell.armlinux.org.uk> (raw)
In-Reply-To: <20240906103511.28416-5-Raju.Lakkaraju@microchip.com>
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!
next prev parent reply other threads:[~2024-09-17 15:59 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-09-06 10:35 [PATCH net-next V6 0/5] Add support to PHYLINK for LAN743x/PCI11x1x chips Raju Lakkaraju
2024-09-06 10:35 ` [PATCH net-next V6 1/5] net: phylink: Add phylink_set_fixed_link() to configure fixed link state in phylink Raju Lakkaraju
2024-09-06 10:35 ` [PATCH net-next V6 2/5] net: lan743x: Create separate PCS power reset function Raju Lakkaraju
2024-09-06 10:35 ` [PATCH net-next V6 3/5] net: lan743x: Create separate Link Speed Duplex state function Raju Lakkaraju
2024-09-06 10:35 ` [PATCH net-next V6 4/5] net: lan743x: Migrate phylib to phylink Raju Lakkaraju
2024-09-17 15:59 ` Russell King (Oracle) [this message]
2024-09-06 10:35 ` [PATCH net-next V6 5/5] net: lan743x: Add support to ethtool phylink get and set settings Raju Lakkaraju
2024-09-11 10:10 ` [PATCH net-next V6 0/5] Add support to PHYLINK for LAN743x/PCI11x1x chips patchwork-bot+netdevbpf
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ZumnRtZYBLIyI/Gm@shell.armlinux.org.uk \
--to=linux@armlinux$(echo .)org.uk \
--cc=Raju.Lakkaraju@microchip$(echo .)com \
--cc=UNGLinuxDriver@microchip$(echo .)com \
--cc=andrew@lunn$(echo .)ch \
--cc=bryan.whitehead@microchip$(echo .)com \
--cc=davem@davemloft$(echo .)net \
--cc=edumazet@google$(echo .)com \
--cc=hkallweit1@gmail$(echo .)com \
--cc=horms@kernel$(echo .)org \
--cc=kuba@kernel$(echo .)org \
--cc=linux-kernel@vger$(echo .)kernel.org \
--cc=maxime.chevallier@bootlin$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=pabeni@redhat$(echo .)com \
--cc=rdunlap@infradead$(echo .)org \
--cc=richardcochran@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox