On Wednesday, December 20, 2017 3:10:46 PM CET David Miller wrote:
> 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?
Ah, well the emac driver is almost as old as dirt ;).
I added Benjamin Herrenschmidt since he seems to be the
original author. maybe he can provide some valuable insights
in this archaeological dig.
I don't know when the ibm_emac driver was added, but it does
predate the shared PHY_INTERFACE_MODE_ macros by a few years.
For example 2.6.11 had the driver and the defines.:
<http://elixir.free-electrons.com/linux/v2.6.11/source/drivers/net/ibm_emac/ibm_emac_phy.h#L41>
But there's no PHY_INTERFACE_MODE_* yet.
Fast forward to 2011:
The patch <https://patchwork.kernel.org/patch/945642/> wedded the
PHY_MODE_* macros to the PHY_INTERFACE_MODE_ enums. But this was
not a complete convertion.
So, as far as I can tell, these definitions have been there since
the beginning.
>
> 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.
Yeah, I can do that. no problem.
Question is, should I also replace the rgmii_mode_name() with phy_modes() too?
The only user of rgmii_mode_name() is this notice printk in rgmii_attach():
<http://elixir.free-electrons.com/linux/latest/source/drivers/net/ethernet/ibm/emac/rgmii.c#L117>
Regards,
Christian
