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

[Marek Behun]

On Sat, 25 Jul 2020 20:48:46 +0200
Andrew Lunn <andrew@xxxxxxx> wrote:

> > > > +#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 will propose rename of the LED name on interface rename event.

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

This is already done. DUAL LEDs will be handled by the multicolor LED
framework which also is already in Pavel's tree. The problem is that if
we make PHY LEDs OF parsing code generic in phy-core, it will also need
to be robust enough to take care of DUAL LEDs OF parsing. That is
something which is Marvell specific. It is possible that some other
vendor also manufactures PHYs with something like that, but the LEDs
on other PHYs may have different maximum value of brightness, or
additional configurations which will need to be in device tree...

But I will try to make it generic and then we will see where it goes...

Marek