Re: [PATCH net-next 3/4] net: phy: marvell10g: Add LED support for 88X3310

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

On Fri, Dec 15, 2023 at 03:44:07PM +0100, Andrew Lunn wrote:
> > +static int mv3310_led_funcs_from_flags(struct mv3310_led *led,
> > +				       unsigned long flags,
> > +				       enum mv3310_led_func *solid,
> > +				       enum mv3310_led_func *blink)
> > +{
> > +	unsigned long activity, duplex, link;
> > +
> > +	if (flags & ~(BIT(TRIGGER_NETDEV_LINK) |
> > +		      BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> > +		      BIT(TRIGGER_NETDEV_FULL_DUPLEX) |
> > +		      BIT(TRIGGER_NETDEV_TX) |
> > +		      BIT(TRIGGER_NETDEV_RX)))
> > +		return -EINVAL;
> 
> This probably should be -EOPNOTSUPP. The trigger will then do the
> blinking in software.
> 
> > +
> > +	link = flags & BIT(TRIGGER_NETDEV_LINK);
> > +
> > +	duplex = flags & (BIT(TRIGGER_NETDEV_HALF_DUPLEX) |
> > +			  BIT(TRIGGER_NETDEV_FULL_DUPLEX));
> > +
> > +	activity = flags & (BIT(TRIGGER_NETDEV_TX) |
> > +			    BIT(TRIGGER_NETDEV_RX));
> > +
> > +	if (link && duplex)
> > +		return -EINVAL;
> 
> It is an odd combination, but again, if the hardware cannot do it,
> return -EOPNOTSUPP and leave it to the software.

I don't recall how the LED triggers work (whether they logically OR
or AND). The hardware supports indicating whether it has a half or
full duplex link, and if the LED is programmed for that, then it will
illuminate when it has link _and_ the duplex is of the specified type.
If the link is down, or the duplex is of the other type, then it won't.

I'm guessing that if link is set and duplex is set, then what userspace
is asking for is the LED to be illuminated whenever the link is up _or_
the duplex is of the specified type, which basically logically resolves
to _only_ "link is up" (because a link that is down has no duplex.)

So, I'm not sure "leaving it to software" is good, that combination is
effectively just "has link".

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