Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode

[Christian Lamparter <chunkeey@gmail.com>]

On Wednesday, December 20, 2017 3:10:46 PM CET David Miller wrote:
> From: Christian Lamparter <chunkeey@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