Re: [PATCH v3 2/2] net: macb: Clean up macb_validate

[Antoine Tenart <atenart@kernel.org>]

Quoting Sean Anderson (2021-10-12 21:46:44)
> +       /* There are three major types of interfaces we support:
> +        * - (R)MII supporting 10/100 Mbit/s
> +        * - GMII, RGMII, and SGMII supporting 10/100/1000 Mbit/s
> +        * - 10GBASER supporting 10 Gbit/s only
> +        * Because GMII and MII both support 10/100, GMII falls through to MII.
> +        *
> +        * If we can't support an interface mode, we just clear the supported
> +        * mask and return. The major complication is that if we get
> +        * PHY_INTERFACE_MODE_NA, we must return all modes we support.  Because
> +        * of this, NA starts at the top of the switch and falls all the way to
> +        * the bottom, and doesn't return early if we don't support a
> +        * particular mode.
> +        */
> +       switch (state->interface) {
> +       case PHY_INTERFACE_MODE_NA:
> +       case PHY_INTERFACE_MODE_10GBASER:
> +               if (bp->caps & MACB_CAPS_HIGH_SPEED &&
> +                   bp->caps & MACB_CAPS_PCS &&
> +                   bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {
> +                       phylink_set_10g_modes(mask);
> +                       phylink_set(mask, 10000baseKR_Full);
> +               } else if (one) {
> +                       bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +                       return;
> +               }
> +               if (one)
> +                       break;

This can go in the first if block.

> +               fallthrough;
> +       case PHY_INTERFACE_MODE_GMII:
> +       case PHY_INTERFACE_MODE_RGMII:
> +       case PHY_INTERFACE_MODE_RGMII_ID:
> +       case PHY_INTERFACE_MODE_RGMII_RXID:
> +       case PHY_INTERFACE_MODE_RGMII_TXID:
> +       case PHY_INTERFACE_MODE_SGMII:
> +               if (macb_is_gem(bp)) {
> +                       if (bp->caps & MACB_CAPS_GIGABIT_MODE_AVAILABLE) {

Is not having MACB_CAPS_GIGABIT_MODE_AVAILABLE acceptable here, or
should the two above checks be merged?

> +                               phylink_set(mask, 1000baseT_Full);
> +                               phylink_set(mask, 1000baseX_Full);
> +                               if (!(bp->caps & MACB_CAPS_NO_GIGABIT_HALF))
> +                                       phylink_set(mask, 1000baseT_Half);
> +                       }
> +               } else if (one) {
> +                       bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
> +                       return;
> +               }
> +               fallthrough;
> +       case PHY_INTERFACE_MODE_MII:
> +       case PHY_INTERFACE_MODE_RMII:
> +               phylink_set(mask, 10baseT_Half);
> +               phylink_set(mask, 10baseT_Full);
> +               phylink_set(mask, 100baseT_Half);
> +               phylink_set(mask, 100baseT_Full);
> +               break;
> +       default:
>                 bitmap_zero(supported, __ETHTOOL_LINK_MODE_MASK_NBITS);
>                 return;
>         }

(For readability, it's not for me to decide in the end).

Thanks,
Antoine