On 23:22-20230413, Marek Behún wrote: > On Thu, Apr 13, 2023 at 02:02:34PM -0500, Nishanth Menon wrote: > > On 20:56-20230413, Marek Behún wrote: > > > On Thu, Apr 13, 2023 at 01:07:12PM -0500, Nishanth Menon wrote: > > > > Recent commit 75d28899e3e9 ("net: phy: Synchronize PHY interface modes > > > > with Linux") reordered the enum definitions. This caused the range of > > > > enums that this api was checking to go bad. > > > > > > > > While it is possible for the phy drivers to practically use the enum's > > > > directly, drivers such as dp83867 use this helper to manage the > > > > configuration of the phy correctly. > > > > > > > > Reported-by: Tom Rini <tr...@konsulko.com> > > > > Signed-off-by: Nishanth Menon <n...@ti.com> > > > > --- > > > > include/phy.h | 12 ++++++++++-- > > > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/include/phy.h b/include/phy.h > > > > index a837fed72352..1c4dc23bc5ba 100644 > > > > --- a/include/phy.h > > > > +++ b/include/phy.h > > > > @@ -373,8 +373,16 @@ static inline bool phy_interface_is_rgmii(struct > > > > phy_device *phydev) > > > > */ > > > > static inline bool phy_interface_is_sgmii(struct phy_device *phydev) > > > > { > > > > - return phydev->interface >= PHY_INTERFACE_MODE_SGMII && > > > > - phydev->interface <= PHY_INTERFACE_MODE_QSGMII; > > > > + switch (phydev->interface) { > > > > + case PHY_INTERFACE_MODE_SGMII: > > > > + case PHY_INTERFACE_MODE_QUSGMII: > > > > + case PHY_INTERFACE_MODE_USXGMII: > > > > + case PHY_INTERFACE_MODE_QSGMII: > > > > + return 1; > > > > + default: > > > > + fallthrough; > > > > > > Why not just put the return 0; statement here instead of fallthrough and > > > drop it from after the switch statement? > > > > Just dropping the default also will work, though it does leave something > > un-handled. handling the default on the other hand allows for additional > > code (meh?) to be added in default.. But really what? I'd rather drop > > the default and fall through to save on a couple of lines.. if that > > is'nt creating confusion.. > > You can't drop the default if you use a switch statement and not > enumerating all enumerator entries. > > So you either have to use > > switch (x) { > case A: > return 1; > case B: > return 2; > default: > return 3; > } > > or > > switch (x) { > case A: > return 1; > case B: > return 2; > default: > break; /* or fallthourgh */ > } > return 3; > > The first one is shorter one line. It is also used in this way already. > > IMO it makes more sense. > > But keep in mind, this is just a nit-pick. I don't actually care that > much. I just think it makes more sense this way. If all possibilities > are handled by return statements inside the switch, you don't need to > have another return in top level of the function.
Sigh, remnants of a few years of misra misery :( - yep, dropping.. new rev incoming.. > > Look for example at Linux' include/linux/phy.h function phy_modes(): > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/linux/phy.h?h=v6.3-rc6#n216 > -- Regards, Nishanth Menon Key (0xDDB5849D1736249D) / Fingerprint: F8A2 8693 54EB 8232 17A3 1A34 DDB5 849D 1736 249D