| Message ID | 20240605121648.69779-1-csokas.bence@prolan.hu (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | [RFC,1/2] net: include: mii: Refactor: Define LPA_* in terms of ADVERTISE_* | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Not a local patch |
On Wed, Jun 05, 2024 at 02:16:47PM +0200, Csókás, Bence wrote: > Ethernet specification mandates that these bits will be equal. > To reduce the amount of magix hex'es in the code, just define > them in terms of each other. Are magic hexes in this context actually bad? In .c files i would agree. But what you have in effect done is force me into jump another hoop to find the actual hex value so i can manually decode a register value. And you have made the compile slightly slower. These defines have been like this since the beginning of the git history. Is there a good reason to change them after all that time? Andrew
Hi! On 6/5/24 14:51, Andrew Lunn wrote: > On Wed, Jun 05, 2024 at 02:16:47PM +0200, Csókás, Bence wrote: >> Ethernet specification mandates that these bits will be equal. >> To reduce the amount of magix hex'es in the code, just define >> them in terms of each other. > > Are magic hexes in this context actually bad? Yes, as if it ever needs to be changed (for instance in the 2/2 of the series, when I replaced them with BIT() macros), it needs to be changed twice in the file. > In .c files i would > agree. But what you have in effect done is force me into jump another > hoop to find the actual hex value so i can manually decode a register > value. True. I expected this concern, hence why I tagged this as RFC. However, I believe that from a maintainability perspective, it's best to only have one definition, and since these #define's are right under one another, the "jumping around" is minimal anyways. > And you have made the compile slightly slower. C'mon, that's negligible. The time it takes to load the header file from disk will probably take longer than it does to resolve an extra layer of simple #define's. > These defines have been like this since the beginning of the git > history. Is there a good reason to change them after all that time? Just because something was "always like this" doesn't mean that it cannot be changed. Especially since this patch is 100% backwards-compatible, just maybe slightly more future-proof. > Andrew > Bence
diff --git a/include/uapi/linux/mii.h b/include/uapi/linux/mii.h index 39f7c44baf53..33e1b0c717e4 100644 --- a/include/uapi/linux/mii.h +++ b/include/uapi/linux/mii.h @@ -93,22 +93,22 @@ ADVERTISE_100HALF | ADVERTISE_100FULL) /* Link partner ability register. */ -#define LPA_SLCT 0x001f /* Same as advertise selector */ -#define LPA_10HALF 0x0020 /* Can do 10mbps half-duplex */ -#define LPA_1000XFULL 0x0020 /* Can do 1000BASE-X full-duplex */ -#define LPA_10FULL 0x0040 /* Can do 10mbps full-duplex */ -#define LPA_1000XHALF 0x0040 /* Can do 1000BASE-X half-duplex */ -#define LPA_100HALF 0x0080 /* Can do 100mbps half-duplex */ -#define LPA_1000XPAUSE 0x0080 /* Can do 1000BASE-X pause */ -#define LPA_100FULL 0x0100 /* Can do 100mbps full-duplex */ -#define LPA_1000XPAUSE_ASYM 0x0100 /* Can do 1000BASE-X pause asym*/ -#define LPA_100BASE4 0x0200 /* Can do 100mbps 4k packets */ -#define LPA_PAUSE_CAP 0x0400 /* Can pause */ -#define LPA_PAUSE_ASYM 0x0800 /* Can pause asymetrically */ -#define LPA_RESV 0x1000 /* Unused... */ -#define LPA_RFAULT 0x2000 /* Link partner faulted */ -#define LPA_LPACK 0x4000 /* Link partner acked us */ -#define LPA_NPAGE 0x8000 /* Next page bit */ +#define LPA_SLCT ADVERTISE_SLCT /* Same as advertise selector */ +#define LPA_10HALF ADVERTISE_10HALF +#define LPA_1000XFULL ADVERTISE_1000XFULL +#define LPA_10FULL ADVERTISE_10FULL +#define LPA_1000XHALF ADVERTISE_1000XHALF +#define LPA_100HALF ADVERTISE_100HALF +#define LPA_1000XPAUSE ADVERTISE_1000XPAUSE +#define LPA_100FULL ADVERTISE_100FULL +#define LPA_1000XPAUSE_ASYM ADVERTISE_1000XPSE_ASYM +#define LPA_100BASE4 ADVERTISE_100BASE4 +#define LPA_PAUSE_CAP ADVERTISE_PAUSE_CAP +#define LPA_PAUSE_ASYM ADVERTISE_PAUSE_ASYM +#define LPA_RESV ADVERTISE_RESV +#define LPA_RFAULT ADVERTISE_RFAULT /* Link partner faulted */ +#define LPA_LPACK ADVERTISE_LPACK /* Link partner acked us */ +#define LPA_NPAGE ADVERTISE_NPAGE #define LPA_DUPLEX (LPA_10FULL | LPA_100FULL) #define LPA_100 (LPA_100FULL | LPA_100HALF | LPA_100BASE4)
Ethernet specification mandates that these bits will be equal. To reduce the amount of magix hex'es in the code, just define them in terms of each other. Cc: trivial@kernel.org Signed-off-by: "Csókás, Bence" <csokas.bence@prolan.hu> --- include/uapi/linux/mii.h | 32 ++++++++++++++++---------------- 1 file changed, 16 insertions(+), 16 deletions(-)