From: Christian Lamparter <chunk...@gmail.com>
Date: Wed, 20 Dec 2017 17:02:01 +0100

> diff --git a/drivers/net/ethernet/ibm/emac/emac.h 
> b/drivers/net/ethernet/ibm/emac/emac.h
> index 5afcc27ceebb..8c6d2af7281b 100644
> --- a/drivers/net/ethernet/ibm/emac/emac.h
> +++ b/drivers/net/ethernet/ibm/emac/emac.h
> @@ -112,6 +112,9 @@ struct emac_regs {
>  #define PHY_MODE_RMII        PHY_INTERFACE_MODE_RMII
>  #define PHY_MODE_SMII        PHY_INTERFACE_MODE_SMII
>  #define PHY_MODE_RGMII       PHY_INTERFACE_MODE_RGMII
> +#define PHY_MODE_RGMII_ID    PHY_INTERFACE_MODE_RGMII_ID
> +#define PHY_MODE_RGMII_RXID  PHY_INTERFACE_MODE_RGMII_RXID
> +#define PHY_MODE_RGMII_TXID  PHY_INTERFACE_MODE_RGMII_TXID
>  #define PHY_MODE_TBI PHY_INTERFACE_MODE_TBI
>  #define PHY_MODE_GMII        PHY_INTERFACE_MODE_GMII
>  #define PHY_MODE_RTBI        PHY_INTERFACE_MODE_RTBI

Why does this driver do all of this CPP macro aliasing?

It's really cruddy because anyone who now reads this code:

>  static inline int rgmii_valid_mode(int phy_mode)
>  {
> -     return  phy_mode == PHY_MODE_GMII ||
> +     return  phy_interface_mode_is_rgmii(phy_mode) ||
> +             phy_mode == PHY_MODE_GMII ||
>               phy_mode == PHY_MODE_MII ||
> -             phy_mode == PHY_MODE_RGMII ||
>               phy_mode == PHY_MODE_TBI ||
>               phy_mode == PHY_MODE_RTBI;
>  }

will read this and say "Oh the function tests against these weird
PHY_MODE_* aliases, but the helper function phy_interface_mode_is_rgmii()
tests against PHY_INTERFACE_MODE_*, what is going on?"

I hate to do this to you, but please get rid of these confusing and
completely unnecessary PHY_MODE_* CPP aliases first, and just use the
proper PHY_INTERFACE_MODE_* values consistently.

Thank you.

Reply via email to