Re: [PATCH 1/2] dt-bindings: leds: Document netdev trigger netdev-trigger-mode property

["Heiko Stübner" <heiko@sntech.de>]

Hi,

Am Montag, 13. Januar 2025, 01:23:37 Mitteleuropäische Sommerzeit schrieb Marek Vasut:
> Document netdev trigger specific netdev-trigger-mode property which
> is used to configure the netdev trigger mode flags. Those mode flags
> define events on which the LED acts upon when the hardware offload is
> enabled. This is traditionally configured via sysfs, but that depends
> on udev rules which are available either too late or never in case of
> non-Linux systems.
> 
> For each LED with linux,default-trigger = "netdev" described in DT, this
> optional netdev-trigger-mode property supplies the default configuration
> of the PHY LED mode via DT. This property should be set to a subset of
> TRIGGER_NETDEV_* flags.
> 
> Signed-off-by: Marek Vasut <marex@denx.de>

while this is already half a year old, neither me nor b4 have found a
newer thread, so I hope this is still the most recent one to reply to.


> ---
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Christian Marangi <ansuelsmth@gmail.com>
> Cc: Conor Dooley <conor+dt@kernel.org>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Jacek Anaszewski <jacek.anaszewski@gmail.com>
> Cc: Krzysztof Kozlowski <krzk+dt@kernel.org>
> Cc: Lee Jones <lee@kernel.org>
> Cc: Lukasz Majewski <lukma@denx.de>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Rob Herring <robh@kernel.org>
> Cc: devicetree@vger.kernel.org
> Cc: linux-leds@vger.kernel.org
> ---
>  Documentation/devicetree/bindings/leds/common.yaml | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/leds/common.yaml b/Documentation/devicetree/bindings/leds/common.yaml
> index 3e8319e443392..1f1148fdf20c0 100644
> --- a/Documentation/devicetree/bindings/leds/common.yaml
> +++ b/Documentation/devicetree/bindings/leds/common.yaml
> @@ -233,6 +233,12 @@ properties:
>        Maximum timeout in microseconds after which the flash LED is turned off.
>        Required for flash LED nodes with configurable timeout.
>  
> +  # Requires netdev trigger
> +  netdev-trigger-mode:
> +    description:
> +      The netdev LED trigger default mode flags, use TRIGGER_NETDEV_ * flags.
> +    $ref: /schemas/types.yaml#/definitions/uint32-array
> +

as DT is supposed to be a hardware description, I think throwing arbitary
binary values around is not very readable - especially as the above would
be a combination of setting-bits for the TRIGGER_NETDEV_* things.

Instead I'd think using boolean dt props would reflect the binary "or"
way better and also keep all the bitwise nastiness out of the dt.

Also "netdev" is a Linux thing, and therefore also set in the
"linux,default-trigger" property, so I'd think any added netdev-props
should probably also have a linux,* prefix.


So in sum, I think the following might look better?

  linux,netdev-trigger-link:
    description:
      LED is lit on established link
    type: boolean

  linux,netdev-trigger-link-10:
    description:
      LED is lit on established link with 10MBit
    type: boolean

  linux,netdev-trigger-link-100:
    description:
      LED is lit on established link with 100MBit
    type: boolean

[...]

  linux,netdev-trigger-link-tx:
    description:
      LED is triggered when sending data
    type: boolean

  linux,netdev-trigger-link-rx:
    description:
      LED is triggered when receiving data
    type: boolean

[...]

for each element of the led_trigger_netdev_modes enum [0], with the node
then looking something like:

               leds {
                       #address-cells = <1>;
                       #size-cells = <0>;

                       /* Network LED on the front panel */
                       led@0 {
                               reg = <0>;
                               color = <LED_COLOR_ID_AMBER>;
                               function = LED_FUNCTION_LAN;
                               linux,default-trigger = "netdev";
                               linux,netdev-trigger-rx;
                               linux,netdev-trigger-tx;
                       };


Heiko


[0] https://elixir.bootlin.com/linux/v6.16/source/include/linux/leds.h#L603