RE: [PATCH v4 2/5] net: lan743x: read SFP straps from PCI11x1x device

[Thangaraj.S]

Hi Andrew,
Thanks for the comments.

> -----Original Message-----
> From: Andrew Lunn <andrew@xxxxxxx>
> Sent: Thursday, May 14, 2026 6:17 PM
> To: Thangaraj Samynathan - I53494 <Thangaraj.S@xxxxxxxxxxxxx>
> Cc: netdev@xxxxxxxxxxxxxxx; andrew+netdev@xxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; Bryan Whitehead - C21958
> <Bryan.Whitehead@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; linux@xxxxxxxxxxxxxxx; linux-
> kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v4 2/5] net: lan743x: read SFP straps from PCI11x1x
> device
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Thu, May 14, 2026 at 04:20:25PM +0530, Thangaraj Samynathan wrote:
> > Reads the SFP enable bits from the strap registers to determine if the
> > hardware is configured for SFP usage.
> 
> What exactly does this strapping mean? Is there a definition in the
> documentation?
[Thangaraj Samynathan] PCI11x1x has a strap register, which is configured via
OTP/EEPROM. The driver read this strap registers to determine the interface
configurations.
> 
> > Introduce CONFIG_LAN743X_SFP to guard SFP-specific code
> 
> Why? Does this add a lot of code? Most MAC drivers don't have such a Kconfig
> configuration, so i think this needs some justification.
[Thangaraj Samynathan]  CONFIG_LAN743X_SFP was introduced based on
  Jakub's v3 review feedback. There are devices on this LAN743x family
  of devices (LAN7430, LAN7431, PCI11x1x) that do not support SFP at
  all. So while the amount of code in the common LAN743x driver itself
  may not be that much, the dependencies could force an intended
  minimalistic build of the kernel that wanted to use the lan7430 device
  (which is rgmii only capable device) to have to include tons of kernel
  code (sfp, i2c, gpio) it does not need otherwise.
> 
> > +#ifdef CONFIG_LAN743X_SFP
> > +     if (adapter->is_pci11x1x && !adapter->is_pcs_en &&
> > +         adapter->is_sfp_support_en) {
> > +             netif_err(adapter, drv, adapter->netdev,
> > +                       "Invalid EEPROM configuration: SFP_EN strap specified
> without SGMII_EN strap\n");
> > +             adapter->is_sfp_support_en = false;
> 
> Shouldn't that be fatal? You have no way to drive the hardware, so is there any
> point going further?
[Thangaraj Samynathan] Agreed. Will be fixed in next revision. 
> 
> What also seems to be missing here is the case where the strapping is set to
> indicate SFP, but CONFIG_LAN743X_SFP is not enabled. That should also be a
> fatal error.
[Thangaraj Samynathan] Agreed. The SFP strap read will be moved
  outside the #ifdef so is_sfp_support_en is always set from hardware.
  A #ifndef CONFIG_LAN743X_SFP block will then check the flag and
  return -EINVAL if the hardware is SFP-strapped but support is not
  compiled in. 
> 
>         Andrew