Re: [PATCH net-next V2 1/5] net: lan743x: Add SFP support check flag

["Russell King (Oracle)" <linux@armlinux.org.uk>]

On Thu, Sep 12, 2024 at 05:58:14PM +0200, Andrew Lunn wrote:
> > > > > > +     if (adapter->is_pci11x1x && !adapter->is_sgmii_en &&
> > > > > > +         adapter->is_sfp_support_en) {
> > > > > > +             netif_err(adapter, drv, adapter->netdev,
> > > > > > +                       "Invalid eeprom cfg: sfp enabled with sgmii disabled");
> > > > > > +             return -EINVAL;
> > > > >
> > > > > is_sgmii_en actually means PCS? An SFP might need 1000BaseX or
> > > > > SGMII,
> > > >
> > > > No, not really.
> > > > The PCI11010/PCI1414 chip can support either an RGMII interface or an
> > > > SGMII/1000Base-X/2500Base-X interface.
> > > 
> > > A generic name for SGMII/1000Base-X/2500Base-X would be PCS, or maybe SERDES. To me, is_sgmii_en
> > > means SGMII is enabled, but in fact it actually means SGMII/1000Base-X/2500Base-X is enabled. I just
> > > think this is badly named. It would be more understandable if it was is_pcs_en.
> > > 
> > > > According to the datasheet,
> > > > the "Strap Register (STRAP)" bit 6 is described as "SGMII_EN_STRAP"
> > > > Therefore, the flag is named "is_sgmii_en".
> > > 
> > > Just because the datasheet uses a bad name does not mean the driver has to also use it.
> > > 
> > >         Andrew
> > 
> > The hardware architect, who is a very bright guy (it's not me :-), just called the strap SGMII_EN in order not to make the name too long and to contrast it with the opposite polarity of the bit which means the interface is set to RGMII; but in the description of the strap he clearly stated what it is:
> > 	SGMII_EN_STRAP
> > 	0 = RGMII
> > 	1 = SGMII / 1000/2500BASE-X
> > 
> > I don't think PCS or Serdes (both of which get used in other technologies - some of which are also included in this chip and are therefore bound to create even more confusion if used) are good choices either.

While I can understand the desire to name stuff as documentation names
it, just because documentation calls an apple a banana does not mean
that everyone who doesn't have the documentation will understand that
is_a_banana will be true for an apple.

This is the problem here. SGMII has two meanings (and thanks to the
network industry for creating this in the first place).

First, there is Cisco SGMII, an adaptation of IEEE 802.3 1000base-X.
Secondly, there is its use as "Serial gigabit media independent
interface" which various manufacturers seem to use to refer to their
serial network interface supporting both Cisco SGMII, 1000base-X and
2500base-X.

_That_ is exactly where the problem is. "SGMII" is ambiguous. One can
not even use much in the way of context to separate out which it's
referring to, and naming a variable "is_sgmii_en" just doesn't have
the context. This ambiguous nature adds to the kernel maintenance
burden for those of us who look after subsystems.

It is unfortunate that people continue not to recognise this as the
problem that it is, but everyone loves acronyms... and acronyms are
a way of talking in code that excludes people from discussions or
understanding.

So, consider this a formal request _not_ to name your variable
"is_sgmii_en" but something else.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!