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

Andrew Lunn
2024-09-05T13:11:29Z

> +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?

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