Re: [PATCH v7 00/11] Adds support for PHY LEDs with offload triggers

Alexander Stein
2022-12-15T15:34:40Z

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