Message ID | 20200716171730.13227-1-marek.behun@nic.cz |
---|---|
Headers | show |
Series | Add support for LEDs on Marvell PHYs | expand |
Hello, On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote: > Hello, > > this RFC series should apply on both net-next/master and Pavel's > linux-leds/for-master tree. > > This adds support for LED's connected to some Marvell PHYs. > > LEDs are specified via device-tree. Example: > > ethernet-phy@1 { > reg = <1>; > > leds { > led@0 { > reg = <0>; > color = <LED_COLOR_ID_GREEN>; > function = LED_FUNCTION_LINK; > linux,default-trigger = "hw:1000/100/10/nolink"; > }; > > led@1 { > reg = <1>; > color = <LED_COLOR_ID_YELLOW>; > function = LED_FUNCTION_ACTIVITY; > linux,default-trigger = "hw:act/noact"; > }; > }; > }; > > Since Marvell PHYs can control the LEDs themselves in various modes, > we need to be able to switch between them or disable them. > > This is achieved by extending the LED trigger API with LED-private triggers. > The proposal for this is based on work by Ondrej and Pavel, but after writing > this support for Marvell PHYs I am not very happy how this turned out: > - this LED-private triggers API works by registering triggers with specific > private trigger type. If this trigger type is defined for a trigger, only > those LEDs will be able to set this trigger which also have this trigger type. > (Both structs led_classdev and led_trigger have member trigger_type) > - on Marvell PHYs each LED can have up to 8 different triggers > - currently the driver supports up to 6 LEDs, since at least 88E1340 support > 6 LEDs > - almost every LED supports some mode which is not supported by at least one > other LED > - this leads to the following dillema: > 1. either we support one trigger type across the driver, but then the > /sys/class/leds/<LED>/trigger file will also list HW triggers not > supported by this specific LED, > 2. or we add 6 trigger types, each different one LED, and register up to > 8 HW triggers for each trigger type, which results in up to 48 triggers > per this driver. > In this proposal alternative 1 is taken and when unsupported HW trigger > is requested by writing to /sys/class/leds/<LED>/trigger file, the write > system call returns -EOPNOTSUPP. > - therefore I think that this is not the correct way how to implement > LED-private triggers, and instead an approach as desribed in [1]. > What do you people think? This seems easily solvable by using sysfs attributes for specific hw mode configuration. This would result in having one private trigger registered with all the special casing being done in your sysfs *_store/show functions that could be made per LED, because sysfs functions get a reference to led_classdev. You can then apply any kind of constraints fairly easily in sysfs interface for your hw trigger. See my driver: https://megous.com/git/linux/commit/?h=tbs-a711-5.8&id=a4da688b5d48011777d1e870a7d2b42edc9d144f I guess unless you really want to have all possible configureations exposed as differently named triggers via the led/trigger sysfs interface. But as you see it makes things complicated. regards, o. > Marek > > Marek Behún (3): > leds: trigger: add support for LED-private device triggers > leds: trigger: return error value if .activate() failed > net: phy: marvell: add support for PHY LEDs via LED class > > drivers/leds/led-triggers.c | 32 ++-- > drivers/net/phy/Kconfig | 7 + > drivers/net/phy/marvell.c | 307 +++++++++++++++++++++++++++++++++++- > include/linux/leds.h | 10 ++ > 4 files changed, 346 insertions(+), 10 deletions(-) > > -- > 2.26.2 >
On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote: > Hello, > > this RFC series should apply on both net-next/master and Pavel's > linux-leds/for-master tree. > > This adds support for LED's connected to some Marvell PHYs. > > LEDs are specified via device-tree. Example: Hi Marek I've been playing with something similar, off and on, mostly off. Take a look at https://github.com/lunn/linux v5.4-rc6-hw-led-triggers The binding i have is pretty much the same, since we are both following the common LED binding. I see no problems with this. > This is achieved by extending the LED trigger API with LED-private triggers. > The proposal for this is based on work by Ondrej and Pavel. So what i did here was allow triggers to be registered against a specific LED. The /sys/class/leds/<LED>/trigger lists both the generic triggers and the triggers for this specific LED. Phylib can then register a trigger for each blink reason that specific LED can perform. Which does result in a lot of triggers. Especially when you start talking about a 10 port switch each with 2 LEDs. I still have some open issues... 1) Polarity. It would be nice to be able to configure the polarity of the LED in the bindings. 2) PHY LEDs which are not actually part of the PHY. Most of the Marvell Ethernet switches have inbuilt PHYs, which are driven by the Marvell PHY driver. The Marvell PHY driver has no idea the PHY is inside a switch, it is just a PHY. However, the LEDs are not controlled via PHY registers, but Switch registers. So the switch driver is going to end up controlling these LEDs. It would be good to be able to share as much code as possible, keep the naming consistent, and keep the user API the same. 3) Some PHYs cannot control the LEDs independently. Or they have modes which configure two or more LEDs. The Marvell PHYs are like this. There are something like ~10 blink modes which are independent. And then there are 4 modes which control multiple LEDs. There is no simple way to support this with Linux LEDs which assume the LEDs are fully independent. I suspect we simply cannot support these combined modes. As a PHY maintainer, i would like to see a solution which makes use of Linux LEDs. I don't really care who's code it is, and feel free to borrow my code, or ideas, or ignore it. Andrew
On Thu, 16 Jul 2020 20:56:47 +0200 Andrew Lunn <andrew@lunn.ch> wrote: > On Thu, Jul 16, 2020 at 07:17:27PM +0200, Marek Behún wrote: > > Hello, > > > > this RFC series should apply on both net-next/master and Pavel's > > linux-leds/for-master tree. > > > > This adds support for LED's connected to some Marvell PHYs. > > > > LEDs are specified via device-tree. Example: > > Hi Marek > > I've been playing with something similar, off and on, mostly off. > > Take a look at > > https://github.com/lunn/linux v5.4-rc6-hw-led-triggers > > The binding i have is pretty much the same, since we are both > following the common LED binding. I see no problems with this. > > > This is achieved by extending the LED trigger API with LED-private > > triggers. The proposal for this is based on work by Ondrej and > > Pavel. > > So what i did here was allow triggers to be registered against a > specific LED. The /sys/class/leds/<LED>/trigger lists both the generic > triggers and the triggers for this specific LED. Phylib can then > register a trigger for each blink reason that specific LED can > perform. Which does result in a lot of triggers. Especially when you > start talking about a 10 port switch each with 2 LEDs. > > I still have some open issues... > Hi Andrew, Pavel Machek has applied support for LED private triggers yesterday, see https://git.kernel.org/pub/scm/linux/kernel/git/pavel/linux-leds.git/commit/?h=for-next&id=93690cdf3060c61dfce813121d0bfc055e7fa30d The way this is handled has this issue - it results in a lot of triggers, if we want each possible control to have its own trigger in the sysfs trigger file. But as Ondrej pointed out, we can just register one "hw-control" device trigger, and have its activation create another file/files via which the user can select which type of HW control he wants to activate. Something similar is done in netdev trigger. I don't like it much, but this is what can be done if we want to avoid having lots of triggers registered. > 1) Polarity. It would be nice to be able to configure the polarity of > the LED in the bindings. Yes, and also the DUAL mode and everything else. > 2) PHY LEDs which are not actually part of the PHY. Most of the > Marvell Ethernet switches have inbuilt PHYs, which are driven by the > Marvell PHY driver. The Marvell PHY driver has no idea the PHY is > inside a switch, it is just a PHY. However, the LEDs are not > controlled via PHY registers, but Switch registers. So the switch > driver is going to end up controlling these LEDs. It would be good to > be able to share as much code as possible, keep the naming consistent, > and keep the user API the same. I know about this - in fact I want this solved for Turris MOX, which has one 1518 PHY and can have up to three switches connected - I want every LED to be configurable by userspace. The internal PHY of the switch can be identified in the marvell phy driver not to register any LEDs. Then the switch LEDs can be controlled by the switch driver. I don't think we are able to share much of the code, since the access to these registers is different from the LED registers in the PHY, and register values are also different. The only think which could be shared are names of the trigger, I think. But I will look into this and prepare a patch series that will share as much code as is reasonable. > 3) Some PHYs cannot control the LEDs independently. Or they have modes > which configure two or more LEDs. The Marvell PHYs are like > this. There are something like ~10 blink modes which are > independent. And then there are 4 modes which control multiple LEDs. > There is no simple way to support this with Linux LEDs which assume > the LEDs are fully independent. I suspect we simply cannot support > these combined modes. I know about these modes, I have func specs for several differnet Marvell PHYs and switches opened and have read about the LED systems. I intend to do this so that all corner cases are considere. > As a PHY maintainer, i would like to see a solution which makes use of > Linux LEDs. I don't really care who's code it is, and feel free to > borrow my code, or ideas, or ignore it. > > Andrew Wait a few days and I shall send another proposal. Marek