From mboxrd@z Thu Jan 1 00:00:00 1970 From: Christian Lamparter Subject: Re: [PATCH v2] net: ibm: emac: support RGMII-[RX|TX]ID phymode Date: Wed, 20 Dec 2017 22:07:43 +0100 Message-ID: <5154461.jTBeEfsTQW@debian64> References: <20171220160201.17143-1-chunkeey@gmail.com> <20171220.151046.2079992548386220503.davem@davemloft.net> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7Bit Cc: netdev@vger.kernel.org, andrew@lunn.ch, christophe.jaillet@wanadoo.fr, Benjamin Herrenschmidt To: David Miller Return-path: Received: from mail-wr0-f178.google.com ([209.85.128.178]:41309 "EHLO mail-wr0-f178.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755754AbdLTVHq (ORCPT ); Wed, 20 Dec 2017 16:07:46 -0500 Received: by mail-wr0-f178.google.com with SMTP id p69so14603140wrb.8 for ; Wed, 20 Dec 2017 13:07:45 -0800 (PST) In-Reply-To: <20171220.151046.2079992548386220503.davem@davemloft.net> Sender: netdev-owner@vger.kernel.org List-ID: On Wednesday, December 20, 2017 3:10:46 PM CET David Miller wrote: > From: Christian Lamparter > 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.: But there's no PHY_INTERFACE_MODE_* yet. Fast forward to 2011: The patch 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(): Regards, Christian