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

On 2/16/2021 9:04 AM, Russell King - ARM Linux admin wrote:

On Tue, Feb 16, 2021 at 04:52:13PM +0000, Robert Hancock wrote:

quoted

On Sat, 2021-02-13 at 10:45 +0000, Russell King - ARM Linux admin wrote:

quoted

On Fri, Feb 12, 2021 at 08:18:40PM -0600, Robert Hancock wrote:

quoted

+	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-

quoted

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?

I don't think relying on phydev->attached_dev in any way is a good
idea, and I suspect a flag is going to be way better. One of the
problems is that phydev->dev_flags are PHY specific at the moment.
Not sure if we can do anything about that.

I have some ideas on how we can improve that and hope to be able to post
something by the end of the week.

In the short term, at the very least, I think we should wrap whatever
test we use in a "phy_on_sfp(phydev)" helper so that we have a standard
helper for this.

Sounds reasonable.
-- 
Florian