| Message ID | 20230624205629.4158216-1-andrew@lunn.ch (mailing list archive) |
|---|---|
| Headers | show |
| Series | Support offload LED blinking to PHY. | expand |
Hi Andrew, On Sat, Jun 24, 2023 at 10:56:26PM +0200, Andrew Lunn wrote: > > Allow offloading of the LED trigger netdev to PHY drivers and > implement it for the Marvell PHY driver. Additionally, correct the > handling of when the initial state of the LED cannot be represented by > the trigger, and so an error is returned. I've used this series in my tree and implemented support for all LED- related features in the mediatek-ge-soc PHY driver: https://github.com/dangowrt/linux/commit/3b9b30a9a6699d290964fc76c56cfcd1dadf5651 I've noticed there is a problem when setting up the netdev trigger using the 'linux,default-trigger' property in device tree. In this case led_classdev_register_ext is called *before* register_netdevice has completed. Hence supports_hw_control returns false when the 'netdev' trigger is initially setup as default trigger. To resolve this, I've tried wrapping led_classdev_register_ext() and introducing led_classdev_register_ext_nodefault() which doesn't call out to led_trigger_set_default, so that can be done later by the caller. However, there isn't any good existing call from netdev to phy informing the phy that the netdev has been registered, so the phy_leds would have to register a netdevice_notifier and wait for the NETDEV_REGISTER events, matching against the netdev's PHY... And that seems a bit overkill, just to support netdev trigger offloading to work when used as a default trigger... Any better ideas anyone? > > Since v1: > > Add true kerneldoc for the new entries in struct phy_driver > Add received Reviewed-by: tags > > Since v0: > > Make comments in struct phy_driver look more like kerneldoc > Add cover letter > > Andrew Lunn (3): > led: trig: netdev: Fix requesting offload device > net: phy: phy_device: Call into the PHY driver to set LED offload > net: phy: marvell: Add support for offloading LED blinking > > drivers/leds/trigger/ledtrig-netdev.c | 8 +- > drivers/net/phy/marvell.c | 243 ++++++++++++++++++++++++++ > drivers/net/phy/phy_device.c | 68 +++++++ > include/linux/phy.h | 33 ++++ > 4 files changed, 349 insertions(+), 3 deletions(-) > > -- > 2.40.1 > >
On Sat, Jul 29, 2023 at 01:38:13PM +0100, Daniel Golle wrote: > Hi Andrew, > > On Sat, Jun 24, 2023 at 10:56:26PM +0200, Andrew Lunn wrote: > > > > Allow offloading of the LED trigger netdev to PHY drivers and > > implement it for the Marvell PHY driver. Additionally, correct the > > handling of when the initial state of the LED cannot be represented by > > the trigger, and so an error is returned. > > I've used this series in my tree and implemented support for all LED- > related features in the mediatek-ge-soc PHY driver: > > https://github.com/dangowrt/linux/commit/3b9b30a9a6699d290964fc76c56cfcd1dadf5651 > > I've noticed there is a problem when setting up the netdev trigger using > the 'linux,default-trigger' property in device tree. In this case > led_classdev_register_ext is called *before* register_netdevice has > completed. > Hence supports_hw_control returns false when the 'netdev' trigger is > initially setup as default trigger. > > To resolve this, I've tried wrapping led_classdev_register_ext() and > introducing led_classdev_register_ext_nodefault() which doesn't call > out to led_trigger_set_default, so that can be done later by the > caller. However, there isn't any good existing call from netdev to phy > informing the phy that the netdev has been registered, so the phy_leds > would have to register a netdevice_notifier and wait for the > NETDEV_REGISTER events, matching against the netdev's PHY... And that > seems a bit overkill, just to support netdev trigger offloading to work > when used as a default trigger... > > Any better ideas anyone? Hi Daniel I've not had chance to look at this yet. I really need to stop being lazy and get my Marvell patches merged, the DT binding for defaults, and then look at this ordering issue. I do however agree, it is going to be messy, since as you say, the PHY does not know what MAC it is bound to until quite late. Maybe we can use the netdev_trig_notify() to update the value from supports_hw_control()? Andrew