[PATCH 1/2] net: phy: Make phy_interface_is_sgmii a switch statement

[Nishanth Menon]

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 <trini at konsulko.com>
> > > > Signed-off-by: Nishanth Menon <nm at 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