MIME-Version: 1.0
References: <20230216013230.22978-1-ansuelsmth@gmail.com>
In-Reply-To: <20230216013230.22978-1-ansuelsmth@gmail.com>
From:   Linus Walleij <linus.walleij@linaro.org>
Date:   Thu, 9 Mar 2023 10:09:18 +0100
Message-ID: <CACRpkda30Ky5oYPn_nGWGOzT5ntZYdE3gafrs7D27ZHxgGuO8A@mail.gmail.com>
Subject: Re: [PATCH v8 00/13] Adds support for PHY LEDs with offload triggers
To:     Christian Marangi <ansuelsmth@gmail.com>,
        Hans de Goede <hdegoede@redhat.com>
Cc:     Pavel Machek <pavel@ucw.cz>, Lee Jones <lee@kernel.org>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Vladimir Oltean <olteanv@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        Eric Dumazet <edumazet@google.com>,
        Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Jonathan Corbet <corbet@lwn.net>,
        "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>,
        Jacek Anaszewski <jacek.anaszewski@gmail.com>,
        John Crispin <john@phrozen.org>, linux-leds@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        netdev@vger.kernel.org, linux-doc@vger.kernel.org,
        Tim Harvey <tharvey@gateworks.com>,
        Alexander Stein <alexander.stein@ew.tq-group.com>,
        Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Arun.Ramadoss@microchip.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Hi Christian,

thanks for your patch!

On Thu, Feb 16, 2023 at 2:36=E2=80=AFAM Christian Marangi <ansuelsmth@gmail=
.com> 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@redhat.=
com/

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.

I think the ambition should be something like this (conjured example)
for a DSA switch:

    platform {
            switch {
                    compatible =3D "foo";

                    leds {
                            #address-cells =3D <1>;
                            #size-cells =3D <0>;
                            led0: led@0 {
                                    reg =3D <0>;
                                    color =3D...
                                    function =3D ...
                                    function-enumerator =3D ...
                                    default-state =3D ...
                            };
                            led1: led@1 {
                                    reg =3D <1>;
                                    color =3D...
                                    function =3D ...
                                    function-enumerator =3D ...
                                    default-state =3D ...
                            };
                    };

                    ports {
                            #address-cells =3D <1>;
                            #size-cells =3D <0>;
                            port@0 {
                                    reg =3D <0>;
                                    label =3D "lan0";
                                    phy-handle =3D <&phy0>;
                                    leds =3D <&led0>;
                            };
                            port@1 {
                                    reg =3D <1>;
                                    label =3D "lan1";
                                    phy-handle =3D <&phy1>;
                                    leds =3D <&led0>;
                            };
                    };

                    mdio {
                            compatible =3D "foo-mdio";
                            #address-cells =3D <1>;
                            #size-cells =3D <0>;

                            phy0: ethernet-phy@0 {
                                    reg =3D <0>;
                            };
                            phy1: ethernet-phy@1 {
                                    reg =3D <1>;
                            };
                    };
            };
    };

I am not the man to tell whether the leds =3D <&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