Date:   Fri, 17 Mar 2023 17:02:35 +0100
From:   Andrew Lunn <andrew@lunn.ch>
To:     Marek =?iso-8859-1?Q?Beh=FAn?= <kabel@kernel.org>
Cc:     Christian Marangi <ansuelsmth@gmail.com>,
        Rob Herring <robh+dt@kernel.org>, devicetree@vger.kernel.org,
        Lee Jones <lee@kernel.org>, linux-leds@vger.kernel.org,
        pavel@ucw.cz, 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>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        Russell King <linux@armlinux.org.uk>,
        Gregory Clement <gregory.clement@bootlin.com>,
        Sebastian Hesselbarth <sebastian.hesselbarth@gmail.com>,
        Andy Gross <agross@kernel.org>,
        Bjorn Andersson <andersson@kernel.org>,
        Konrad Dybcio <konrad.dybcio@linaro.org>,
        John Crispin <john@phrozen.org>, netdev@vger.kernel.org,
        linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
        linux-arm-msm@vger.kernel.org
Subject: Re: [net-next PATCH v4 10/14] dt-bindings: net: dsa: qca8k: add LEDs
 definition example
Message-ID: <c087d270-56f6-4ead-a15b-aa3bfb732ba8@lunn.ch>
References: <20230317023125.486-1-ansuelsmth@gmail.com>
 <20230317023125.486-11-ansuelsmth@gmail.com>
 <20230317091410.58787646@dellmb>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230317091410.58787646@dellmb>
Precedence: bulk

> I would like to start a discussion on this and hear about your opinions,
> because I think that the trigger-sources and function properties were
> proposed in good faith, but currently the implementation and usage is a
> mess.
 
Hi Marek

We are pushing the boundaries of the LED code here, doing things which
have not been done before, as far as i know. So i expect some
discussion about this. However, that discussion should not really
affect this patchset, which is just adding plain boring software
controlled LEDs.

A quick recap about ledtrig-netdev.

If you have a plain boring LED, you have:

root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls
brightness  device  max_brightness  power  subsystem  trigger  uevent

You can turn the LED on with

root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 1 > brightness 

and turn it off with:

root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo 0 > brightness 

You select the trigger via the trigger sysfs file:

root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# cat trigger 
[none] kbd-scrolllock kbd-numlock kbd-capslock kbd-kanalock kbd-shiftlock kbd-altgrlock kbd-ctrllock kbd-altlock kbd-shiftllock kbd-shiftrlock kbd-ctrlllock kbd-ctrlrlock timer heartbeat netdev mmc0

root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# echo netdev > trigger
root@370rd:/sys/class/net/eth0/phydev/leds/f1072004.mdio-mii:00:WAN# ls
activity	brightness  device_name  half_duplex  link     link_100   max_brightness  rx	     trigger  uevent
available_mode	device	    full_duplex  interval     link_10  link_1000  power		  subsystem  tx

When you select a trigger, that trigger can add additional sysfs
files. For the netdev trigger we gain link, link_10, link_100, link_1000, rx & tx.

Nothing special here, if you selected the timer trigger you get
delay_off delay_on. The oneshot trigger has invert, delay_on,
delay_off etc.

You then configure the trigger via setting values in the sysfs
files. If you want the LED to indicate if there is link, you would do:

echo 1 > link

The LED would then light up if the netdev has carrier.

If you want link plus RX packets

echo 1 > link
echo 1 > rx

The LED will then be on if there is link, and additionally blink if
the netdev stats indicate received frames.

For the netdev trigger, all the configuration values are boolean. So a
simple way to represent this in DT would be boolean properties:

	netdev-link = <1>;
	netdev->rx = <1>;

We probably want these properties name spaced, because we have oneshot
delay_on and timer delay_on for example. The same sysfs name could
have different types, bool vs milliseconds, etc.

I would make it, that when the trigger is activated, the values are
read from DT and used. There is currently no persistent state for
triggers. If you where to swap to the timer trigger and then return to
the netdev trigger, all state is lost, so i would re-read DT.

Offloading to hardware should not make an difference here. All we are
going to do is pass the current configuration to the LED and ask it,
can you do this? If it says no, we keep blinking in software. If yes,
we leave the blinking to the hardware.

There is the open question of if DT should be used like this.  It is
not describing hardware, it is describing configuration of
hardware. So it could well get rejected. You then need to configure it
in software.

   Andrew