| Message ID | 20241203075622.2452169-4-o.rempel@pengutronix.de (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Introduce unified and structured PHY | expand |
From: Oleksij Rempel > Sent: 03 December 2024 07:56 > > Convert the PHY flag definitions to use the BIT() macro instead of > hexadecimal values. This improves readability and maintainability. > > No functional changes are introduced by this modification. Are you absolutely sure. You are changing the type of the constants from 'signed int' to 'unsigned long' and that can easily have unexpected consequences. Especially since MDIO_DEVICE_IS_PHY was negative. David > > Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> > --- > include/linux/phy.h | 10 +++++----- > 1 file changed, 5 insertions(+), 5 deletions(-) > > diff --git a/include/linux/phy.h b/include/linux/phy.h > index 20a0d43ab5d4..a6c47b0675af 100644 > --- a/include/linux/phy.h > +++ b/include/linux/phy.h > @@ -86,11 +86,11 @@ extern const int phy_10gbit_features_array[1]; > #define PHY_POLL -1 > #define PHY_MAC_INTERRUPT -2 > > -#define PHY_IS_INTERNAL 0x00000001 > -#define PHY_RST_AFTER_CLK_EN 0x00000002 > -#define PHY_POLL_CABLE_TEST 0x00000004 > -#define PHY_ALWAYS_CALL_SUSPEND 0x00000008 > -#define MDIO_DEVICE_IS_PHY 0x80000000 > +#define PHY_IS_INTERNAL BIT(0) > +#define PHY_RST_AFTER_CLK_EN BIT(1) > +#define PHY_POLL_CABLE_TEST BIT(2) > +#define PHY_ALWAYS_CALL_SUSPEND BIT(3) > +#define MDIO_DEVICE_IS_PHY BIT(31) > > /** > * enum phy_interface_t - Interface Mode definitions > -- > 2.39.5 > - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
On Thu, Dec 05, 2024 at 02:50:32AM +0000, David Laight wrote: > From: Oleksij Rempel > > Sent: 03 December 2024 07:56 > > > > Convert the PHY flag definitions to use the BIT() macro instead of > > hexadecimal values. This improves readability and maintainability. > > > > No functional changes are introduced by this modification. > > Are you absolutely sure. > You are changing the type of the constants from 'signed int' to > 'unsigned long' and that can easily have unexpected consequences. > Especially since MDIO_DEVICE_IS_PHY was negative. In current kernel code following flags are assigned to u32 variable: > > -#define PHY_IS_INTERNAL 0x00000001 > > -#define PHY_RST_AFTER_CLK_EN 0x00000002 > > -#define PHY_POLL_CABLE_TEST 0x00000004 > > -#define PHY_ALWAYS_CALL_SUSPEND 0x00000008 phydrv->flags (u32) This one is assigned to an int: > > -#define MDIO_DEVICE_IS_PHY 0x80000000 mdiodrv->flags (int)
On Tue, Dec 03, 2024 at 08:56:17AM +0100, Oleksij Rempel wrote: > Convert the PHY flag definitions to use the BIT() macro instead of > hexadecimal values. This improves readability and maintainability. Maybe only readability. One can argue that changing them introduces the possibility of conflicts when porting patches which adds maintenance burden. However, this change looks fine to me: Reviewed-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk> Thanks!
On Thu, Dec 05, 2024 at 12:02:19PM +0000, Russell King (Oracle) wrote: > On Tue, Dec 03, 2024 at 08:56:17AM +0100, Oleksij Rempel wrote: > > Convert the PHY flag definitions to use the BIT() macro instead of > > hexadecimal values. This improves readability and maintainability. > > Maybe only readability. One can argue that changing them introduces the > possibility of conflicts when porting patches which adds maintenance > burden. Thinking a bit more about this, we can do much better to improve readability. The question that stands out right now is "but what are these flags used for?" and their definition doesn't make any hint. The PHY_* constants are for struct phy_driver.flags, and I think this needs to be documented. I think that's a much more important detail here that would massively improve readability way beyond simply changing them to be defined in terms of BIT().
diff --git a/include/linux/phy.h b/include/linux/phy.h index 20a0d43ab5d4..a6c47b0675af 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -86,11 +86,11 @@ extern const int phy_10gbit_features_array[1]; #define PHY_POLL -1 #define PHY_MAC_INTERRUPT -2 -#define PHY_IS_INTERNAL 0x00000001 -#define PHY_RST_AFTER_CLK_EN 0x00000002 -#define PHY_POLL_CABLE_TEST 0x00000004 -#define PHY_ALWAYS_CALL_SUSPEND 0x00000008 -#define MDIO_DEVICE_IS_PHY 0x80000000 +#define PHY_IS_INTERNAL BIT(0) +#define PHY_RST_AFTER_CLK_EN BIT(1) +#define PHY_POLL_CABLE_TEST BIT(2) +#define PHY_ALWAYS_CALL_SUSPEND BIT(3) +#define MDIO_DEVICE_IS_PHY BIT(31) /** * enum phy_interface_t - Interface Mode definitions
Convert the PHY flag definitions to use the BIT() macro instead of hexadecimal values. This improves readability and maintainability. No functional changes are introduced by this modification. Signed-off-by: Oleksij Rempel <o.rempel@pengutronix.de> --- include/linux/phy.h | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-)