Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Alexander Stein <alexander.stein@ew.tq-group.com>
To:     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>,
        Rob Herring <robh+dt@kernel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>,
        Jonathan Corbet <corbet@lwn.net>, Pavel Machek <pavel@ucw.cz>,
        Christian Marangi <ansuelsmth@gmail.com>,
        "Russell King (Oracle)" <rmk+kernel@armlinux.org.uk>,
        John Crispin <john@phrozen.org>, netdev@vger.kernel.org,
        devicetree@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-leds@vger.kernel.org,
        Tim Harvey <tharvey@gateworks.com>,
        Rasmus Villemoes <rasmus.villemoes@prevas.dk>,
        Christian Marangi <ansuelsmth@gmail.com>
Subject: Re: [PATCH v7 00/11] Adds support for PHY LEDs with offload triggers
Date:   Thu, 15 Dec 2022 16:34:30 +0100
Message-ID: <2379211.Icojqenx9y@steina-w>
Organization: TQ-Systems GmbH
In-Reply-To: <20221214235438.30271-1-ansuelsmth@gmail.com>
References: <20221214235438.30271-1-ansuelsmth@gmail.com>
MIME-Version: 1.0
Content-Transfer-Encoding: 7Bit
Content-Type: text/plain; charset="us-ascii"
Precedence: bulk

Hi,

thanks for the new series.

Am Donnerstag, 15. Dezember 2022, 00:54:27 CET schrieb Christian Marangi:
> This is another attempt on adding this feature on LEDs, hoping this is
> the right time and someone finally notice this.

Unfortunately I'm out of office from next week on, so there is only limited 
feedback from my side.

> Most of the times Switch/PHY have connected multiple LEDs that are
> controlled by HW based on some rules/event. Currently we lack any
> support for a generic way to control the HW part and normally we
> either never implement the feature or only add control for brightness
> or hw blink.
> 
> This is based on Marek idea of providing some API to cled but use a
> different implementation that in theory should be more generilized.
> 
> 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.

I'm a bit confused about that blink mode is supposed to mean. I don't know 
what to implement for blink_set. Reading qca8k_cled_blink_set it seems to just 
configure the blink interval for the corresponding LED.
Unfortunately that's not possible for all PHYs. In my case, DP83867, I can 
configure the blink interval only globally. I'm not sure how this will fit 
into this LED trigger.

> Each LED driver will have to declare explicit support for the offload
> trigger (or return not supported error code) as we the trigger_data that
> the LED driver will elaborate and understand what is referring to (based
> on the current active trigger).
> 
> I posted a user for this new implementation that will benefit from this
> and will add a big feature to it. Currently qca8k can have up to 3 LEDs
> connected to each PHY port and we have some device that have only one of
> them connected and the default configuration won't work for that.

My DP83867 proof of concept implementation also supports several LEDs, but my 
actual hardware also only has 2 of them attached (LED0 and LED2).

Best regards,
Alexander

> The netdev trigger is expanded and it does now support hardware only
> triggers.
> The idea is to use hardware mode when a device_name is not defined.
> An additional sysfs entry is added to give some info about the available
> trigger modes supported in the current configuration.
> 
> 
> It was reported that at least 3 other switch family would benefits by
> this as they all lack support for a generic way to setup their leds and
> netdev team NACK each try to add special code to support LEDs present
> on switch in favor of a generic solution.
> 
> v7:
> - Rebase on top of net-next (for qca8k changes)
> - Fix some typo in commit description
> - Fix qca8k leds documentation warning
> - Remove RFC tag
> v6:
> - Back to RFC.
> - Drop additional trigger
> - Rework netdev trigger to support common modes used by switch and
>   hardware only triggers
> - Refresh qca8k leds logic and driver
> v5:
> - Move out of RFC. (no comments from Andrew this is the right path?)
> - Fix more spelling mistake (thx Randy)
> - Fix error reported by kernel test bot
> - Drop the additional HW_CONTROL flag. It does simplify CONFIG
>   handling and hw control should be available anyway to support
>   triggers as module.
> v4:
> - Rework implementation and drop hw_configure logic.
>   We now expand blink_set.
> - Address even more spelling mistake. (thx a lot Randy)
> - Drop blink option and use blink_set delay.
> - Rework phy-activity trigger to actually make the groups dynamic.
> v3:
> - Rework start/stop as Andrew asked.
> - Introduce more logic to permit a trigger to run in hardware mode.
> - Add additional patch with netdev hardware support.
> - Use test_bit API to check flag passed to hw_control_configure.
> - Added a new cmd to hw_control_configure to reset any active blink_mode.
> - Refactor all the patches to follow this new implementation.
> v2:
> - Fix spelling mistake (sorry)
> - Drop patch 02 "permit to declare supported offload triggers".
>   Change the logic, now the LED driver declare support for them
>   using the configure_offload with the cmd TRIGGER_SUPPORTED.
> - Rework code to follow this new implementation.
> - Update Documentation to better describe how this offload
>   implementation work.
> 
> Christian Marangi (11):
>   leds: add support for hardware driven LEDs
>   leds: add function to configure hardware controlled LED
>   leds: trigger: netdev: drop NETDEV_LED_MODE_LINKUP from mode
>   leds: trigger: netdev: rename and expose NETDEV trigger enum modes
>   leds: trigger: netdev: convert device attr to macro
>   leds: trigger: netdev: add hardware control support
>   leds: trigger: netdev: use mutex instead of spinlocks
>   leds: trigger: netdev: add available mode sysfs attr
>   leds: trigger: netdev: add additional hardware only triggers
>   net: dsa: qca8k: add LEDs support
>   dt-bindings: net: dsa: qca8k: add LEDs definition example
> 
>  .../devicetree/bindings/net/dsa/qca8k.yaml    |  24 ++
>  Documentation/leds/leds-class.rst             |  53 +++
>  drivers/leds/Kconfig                          |  11 +
>  drivers/leds/led-class.c                      |  27 ++
>  drivers/leds/led-triggers.c                   |  29 ++
>  drivers/leds/trigger/ledtrig-netdev.c         | 385 ++++++++++++-----
>  drivers/net/dsa/qca/Kconfig                   |   9 +
>  drivers/net/dsa/qca/Makefile                  |   1 +
>  drivers/net/dsa/qca/qca8k-8xxx.c              |   4 +
>  drivers/net/dsa/qca/qca8k-leds.c              | 406 ++++++++++++++++++
>  drivers/net/dsa/qca/qca8k.h                   |  62 +++
>  include/linux/leds.h                          | 103 ++++-
>  12 files changed, 1015 insertions(+), 99 deletions(-)
>  create mode 100644 drivers/net/dsa/qca/qca8k-leds.c