Re: [PATCH net-next v2 2/2] net: phy: broadcom: Do not modify LED configuration for SFP module PHYs

[Robert Hancock <robert.hancock@calian.com>]

On Sat, 2021-02-13 at 10:45 +0000, Russell King - ARM Linux admin wrote:
> On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock wrote:
> > +	if (!phydev->sfp_bus &&
> > +	    (!phydev->attached_dev || !phydev->attached_dev->sfp_bus)) {
> 
> First, do we want this to be repeated in every driver?
> 
> Second, are you sure this is the correct condition to be using for
> this?  phydev->sfp_bus is non-NULL when _this_ PHY has a SFP bus
> connected to its fibre side, it will never be set for a PHY on a
> SFP. The fact that it is non-NULL or NULL shouldn't have a bearing
> on whether we configure the LED register.

I think you're correct, the phydev->sfp_bus portion is probably not useful and
could be dropped. What we're really concerned about is whether this PHY is on
an SFP module or not. I'm not sure that a module-specific quirk makes sense
here since there are probably other models which have a similar design where
the LED outputs from the PHY are used for other purposes, and there's really no
benefit to playing with the LED outputs on SFP modules in any case, so it would
be safer to skip the LED reconfiguration for anything on an SFP. So we could
either have a condition for "!phydev->attached_dev || !phydev->attached_dev-
>sfp_bus" here and anywhere else that needs a similar check, or we do something
different, like have a specific flag to indicate that this PHY is on an SFP
module? What do people think is best here?

> 
-- 
Robert Hancock
Senior Hardware Designer, Calian Advanced Technologies
www.calian.com