Hi, On 3/9/23 10:09, Linus Walleij wrote: > Hi Christian, > > thanks for your patch! > > On Thu, Feb 16, 2023 at 2:36 AM Christian Marangi <ansuelsmth@xxxxxxxxx> wrote: > >> The current idea is: >> - LED driver implement 3 API (hw_control_status/start/stop). >> They are used to put the LED in hardware mode and to configure the >> various trigger. >> - We have hardware triggers that are used to expose to userspace the >> supported hardware mode and set the hardware mode on trigger >> activation. >> - We can also have triggers that both support hardware and software mode. >> - The LED driver will declare each supported hardware blink mode and >> communicate with the trigger all the supported blink modes that will >> be available by sysfs. >> - A trigger will use blink_set to configure the blink mode to active >> in hardware mode. >> - On hardware trigger activation, only the hardware mode is enabled but >> the blink modes are not configured. The LED driver should reset any >> link mode active by default. > > The series looks good as a start. > There are some drivers and HW definitions etc for switch-controlled > LEDs, which is great. > > I am a bit reluctant on the ambition to rely on configuration from sysfs > for the triggers, and I am also puzzled to how a certain trigger on a > certain LED is going to associate itself with, say, a certain port. > > I want to draw your attention to this recently merged patch series > from Hans de Goede: > https://lore.kernel.org/linux-leds/20230120114524.408368-1-hdegoede@xxxxxxxxxx/ > > This adds the devm_led_get() API which works similar to getting > regulators, clocks, GPIOs or any other resources. > > It is not yet (I think) hooked into the device tree framework, but it > supports software nodes so adding DT handling should be sort of > trivial. That series contains this (unmerged) patch to hookup DT handling: https://lore.kernel.org/linux-leds/20230120114524.408368-6-hdegoede@xxxxxxxxxx/ this was not merged because there are no current users, but adding support is as easy as picking up that patch :) Note there also already is a devicetree *only*: struct led_classdev *of_led_get(struct device_node *np, int index); Since I was working on a x86/ACPI platform I needed something more generic though and ideally new code would use the generic approach. Regards, Hans > > I think the ambition should be something like this (conjured example) > for a DSA switch: > > platform { > switch { > compatible = "foo"; > > leds { > #address-cells = <1>; > #size-cells = <0>; > led0: led@0 { > reg = <0>; > color =... > function = ... > function-enumerator = ... > default-state = ... > }; > led1: led@1 { > reg = <1>; > color =... > function = ... > function-enumerator = ... > default-state = ... > }; > }; > > ports { > #address-cells = <1>; > #size-cells = <0>; > port@0 { > reg = <0>; > label = "lan0"; > phy-handle = <&phy0>; > leds = <&led0>; > }; > port@1 { > reg = <1>; > label = "lan1"; > phy-handle = <&phy1>; > leds = <&led0>; > }; > }; > > mdio { > compatible = "foo-mdio"; > #address-cells = <1>; > #size-cells = <0>; > > phy0: ethernet-phy@0 { > reg = <0>; > }; > phy1: ethernet-phy@1 { > reg = <1>; > }; > }; > }; > }; > > I am not the man to tell whether the leds = <&led0>; phandle should be on > the port or actually on the phy, it may even vary. You guys know the answer > to this. > > But certainly something like this resource phandle will be necessary to > assign the right LED to the right port or phy, I hope you were not going > to rely on strings and naming conventions? > > Yours, > Linus Walleij >
