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 [off-list ref] wrote:

quoted

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@redhat.com/ (local)

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@redhat.com/ (local)

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