Re: [PATCH RFC leds + net-next v3 2/2] net: phy: marvell: add support for PHY LEDs via LED class

[Andrew Lunn <andrew@lunn.ch>]

> > > +#if 0
> > > +	/* LED_COLOR_ID_MULTI is not yet merged in Linus' tree */
> > > +	/* TODO: Support DUAL MODE */
> > > +	if (color == LED_COLOR_ID_MULTI) {
> > > +		phydev_warn(phydev, "node %pOF: This driver does not yet support multicolor LEDs\n",
> > > +			    np);
> > > +		return -ENOTSUPP;
> > > +	}
> > > +#endif  
> > 
> > Code getting committed should not be using #if 0. Is the needed code
> > in the LED tree? Do we want to consider a stable branch of the LED
> > tree which DaveM can pull into net-next? Or do you want to wait until
> > the next merge cycle?
> 
> That's why this is RFC. But yes, I would like to have this merged for
> 5.9, so maybe we should ask Dave. Is this common? Do we also need to
> tell Pavel or how does this work?

The Pavel needs to create a stable branch. DaveM then merges that
branch into net-next. Your patches can then be merged. When Linus
pulls the two branches, led and net-next, git sees the exact same
patches twice, and simply drops them from the second pull request.

So you need to ask Pavel and DaveM if they are willing to do this.

> > > +	init_data.fwnode = &np->fwnode;
> > > +	init_data.devname_mandatory = true;
> > > +	init_data.devicename = phydev->attached_dev ? netdev_name(phydev->attached_dev) : "";  
> > 
> > This we need to think about. Are you running this on a system with
> > systemd? Does the interface have a name like enp2s0? Does the LED get
> > registered before or after systemd renames it from eth0 to enp2s0?
> 
> Yes, well, this should be discussed also with LED guys. I don't suppose
> that renaming the sysfs symlink on interface rename event is
> appropriate, but who knows?
> The interfaces are platform specific, on mvebu. They aren't connected
> via PCI, so their names remain eth0, eth1 ...

But the Marvell driver is used with more than just mvebu. And we need
this generic. There are USB Ethernet dongles which used phylib. They
will get their interfaces renamed to include the MAC address, etc.

It is possible to hook the notifier so we know when an interface is
renamed. We can then either destroy and re-create the LED, or if the
LED framework allows it, rename it.

Or we avoid interface names all together and stick with the phy name,
which is stable. To make it more user friendly, you could create
additional symlinks. We already have /sys/class/net/ethX/phydev
linking into sys/bus/mdio_bus/devices/.. .  We could add
/sys/class/net/ethX/ledY linking into /sys/class/led/...

It would also be possible to teach ethtool about LEDs, so that it
follows the symbolic links, and manipulates the LED class files.

> I also want this code to be generalized somehow so that it can be
> reused. The problem is that I want to have support for DUAL mode, which
> is Marvell specific, and a DUAL LED needs to be defined in device tree.

It sounds like you first need to teach the LED core about dual LEDs
and triggers which affect two LEDs..

   Andrew