On Tue, 2020-07-28 at 17:05 +0200, Marek Behún wrote: > Hi, > > this is v4 of my RFC adding support for LEDs connected to Marvell > PHYs. > > Please note that if you want to test this, you still need to first > apply > the patch adding the LED private triggers support from Pavel's tree. > https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d > > What I still don't like about this is that the LEDs created by the > code > don't properly support device names. LEDs should have name in format > "device:color:function", for example "eth0:green:activity". > > The code currently looks for attached netdev for a given PHY, but > at the time this happens there is no netdev attached, so the LEDs > gets > names without the device part (ie ":green:activity"). > > This can be addressed in next version by renaming the LED when a > netdev > is attached to the PHY, but first a API for LED device renaming needs > to > be proposed. I am going to try to do that. This would also solve the > same problem when userspace renames an interface. > > And no, I don't want phydev name there. Hello Marek, thanks for your patches - Andrew suggested me to have a look at them as I'm currently trying to add LED trigger support to the TI DP83867 PHY. Is there already a plan to add support for polarity and similiar settings, at least to the generic part of your changes? In the TI DP83867, there are 2 separate settings for each LED: - Trigger event - Polarity or override (active-high/active-low/force-high/force-low - the latter two would be used for led_brightness_set) - (There is also a 3rd register that defines the blink frequency, but as it allows only a single setting for all LEDs, I would ignore it for now) At least the per-LED polarity setting would be essential to have for this feature to be useful for our TQ-Systems mainboards with TI PHYs. Kind regards, Matthias > > Changes since v3: > - addressed some of Andrew's suggestions > - phy_hw_led_mode.c renamed to phy_led.c > - the DT reading code is now also generic, moved to phy_led.c and > called > from phy_probe > - the function registering the phydev-hw-mode trigger is now called > from > phy_device.c function phy_init before registering genphy drivers > - PHY LED functionality now depends on CONFIG_LEDS_TRIGGERS > > Changes since v2: > - to share code with other drivers which may want to also offer PHY > HW > control of LEDs some of the code was refactored and now resides in > phy_hw_led_mode.c. This code is compiled in when config option > LED_TRIGGER_PHY_HW is enabled. Drivers wanting to offer PHY HW > control > of LEDs should depend on this option. > - the "hw-control" trigger is renamed to "phydev-hw-mode" and is > registered by the code in phy_hw_led_mode.c > - the "hw_control" sysfs file is renamed to "hw_mode" > - struct phy_driver is extended by three methods to support PHY HW > LED > control > - I renamed the various HW control modes offeret by Marvell PHYs to > conform to other Linux mode names, for example the > "1000/100/10/else" > mode was renamed to "1Gbps/100Mbps/10Mbps", or "recv/else" was > renamed > to "rx" (this is the name of the mode in netdev trigger). > > Marek > > > Marek Behún (2): > net: phy: add API for LEDs controlled by PHY HW > net: phy: marvell: add support for PHY LEDs via LED class > > drivers/net/phy/Kconfig | 4 + > drivers/net/phy/Makefile | 1 + > drivers/net/phy/marvell.c | 287 > +++++++++++++++++++++++++++++++++++ > drivers/net/phy/phy_device.c | 25 ++- > drivers/net/phy/phy_led.c | 176 +++++++++++++++++++++ > include/linux/phy.h | 51 +++++++ > 6 files changed, 537 insertions(+), 7 deletions(-) > create mode 100644 drivers/net/phy/phy_led.c >
