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

[Raju Lakkaraju <Raju.Lakkaraju@microchip.com>]

Hi Andrew,

Thank you for review the patches.

The 09/05/2024 15:11, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
> 
> > +static struct {
> > +     unsigned long mask;
> > +     int speed;
> > +     int duplex;
> > +} lan743x_mac_caps_params[] = {
> > +     { MAC_2500FD,   SPEED_2500,   DUPLEX_FULL },
> > +     { MAC_1000FD,   SPEED_1000,   DUPLEX_FULL },
> > +     { MAC_100FD,    SPEED_100,    DUPLEX_FULL },
> > +     { MAC_10FD,     SPEED_10,     DUPLEX_FULL },
> > +     { MAC_100HD,    SPEED_100,    DUPLEX_HALF },
> > +     { MAC_10HD,     SPEED_10,     DUPLEX_HALF },
> > +};
> > +
> > +static int lan743x_find_max_speed(unsigned long caps, int *speed, int *duplex)
> > +{
> > +     int i;
> > +
> > +     *speed = SPEED_UNKNOWN;
> > +     *duplex = DUPLEX_UNKNOWN;
> > +     for (i = 0; i < ARRAY_SIZE(lan743x_mac_caps_params); i++) {
> > +             if (caps & lan743x_mac_caps_params[i].mask) {
> > +                     *speed = lan743x_mac_caps_params[i].speed;
> > +                     *duplex = lan743x_mac_caps_params[i].duplex;
> > +                     break;
> > +             }
> > +     }
> > +
> > +     return *speed == SPEED_UNKNOWN ? -EINVAL : 0;
> > +}
> > +
> > +static int lan743x_phylink_create(struct lan743x_adapter *adapter)
> > +{
> > +     struct net_device *netdev = adapter->netdev;
> > +     struct phylink *pl;
> > +
> > +     adapter->phylink_config.dev = &netdev->dev;
> > +     adapter->phylink_config.type = PHYLINK_NETDEV;
> > +     adapter->phylink_config.mac_managed_pm = false;
> > +
> > +     adapter->phylink_config.mac_capabilities = MAC_ASYM_PAUSE |
> > +             MAC_SYM_PAUSE | MAC_10 | MAC_100 | MAC_1000FD;
> > +
> 
> 
> > +     case PHY_INTERFACE_MODE_SGMII:
> > +             __set_bit(PHY_INTERFACE_MODE_SGMII,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_1000BASEX,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             __set_bit(PHY_INTERFACE_MODE_2500BASEX,
> > +                       adapter->phylink_config.supported_interfaces);
> > +             adapter->phylink_config.mac_capabilities |= MAC_2500FD;
> 
> 
> > +static int lan743x_phylink_connect(struct lan743x_adapter *adapter)
> > +{
> > +     struct device_node *dn = adapter->pdev->dev.of_node;
> > +     struct net_device *dev = adapter->netdev;
> > +     struct phy_device *phydev;
> > +     int ret;
> > +
> > +     if (dn)
> > +             ret = phylink_of_phy_connect(adapter->phylink, dn, 0);
> > +
> > +     if (!dn || (ret && !lan743x_phy_handle_exists(dn))) {
> > +             phydev = phy_find_first(adapter->mdiobus);
> > +             if (phydev) {
> > +                     /* attach the mac to the phy */
> > +                     ret = phylink_connect_phy(adapter->phylink, phydev);
> > +             } else if (((adapter->csr.id_rev & ID_REV_ID_MASK_) ==
> > +                           ID_REV_ID_LAN7431_) || adapter->is_pci11x1x) {
> > +                     struct phylink_link_state state;
> > +                     unsigned long caps;
> > +
> > +                     caps = adapter->phylink_config.mac_capabilities;
> > +                     ret = lan743x_find_max_speed(caps, &state.speed,
> > +                                                  &state.duplex);
> 
> This seems like an overly complicated way to determine the speed. If
> MAC_2500FD is set, you know it is 2500. Otherwise it must by 1000. Why
> not use a simple if?

Ok. I will fix

> 
> I'm not the best at reviewing phylink patches, but since Russell is
> off-lime, i'm the best you have. So
> 
> Reviewed-by: Andrew Lunn <andrew@lunn.ch>
> 
> but please do consider simplifying the code above.
> 
>     Andrew

-- 
Thanks,                                                                         
Raju