On Fri, 12 Nov 2021 16:35:21 +0100
Ansuel Smith [off-list ref] wrote:

On Thu, Nov 11, 2021 at 03:16:08AM +0100, Marek Behún wrote:

quoted

On Thu, 11 Nov 2021 02:34:52 +0100
Ansuel Smith [off-list ref] wrote:
  

quoted

This is another attempt in adding support for PHY LEDs. 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.

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.

I also posted the netdev trigger expanded with the hardware support.

More polish is required but this is just to understand if I'm taking
the correct path with this implementation hoping we find a correct
implementation and we start working on the ""small details""  

Hello Ansuel,

besides other things, I am still against the idea of the
`hardware-phy-activity` trigger: I think that if the user wants the LED
to indicate network device's link status or activity, it should always
be done via the existing netdev trigger, and with that trigger only.

Yes, I know that netdev trigger does not currently support indicating
different link modes, only whether the link is up (in any mode). That
should be solved by extending the netdev trigger.

I am going to try to revive my last attempt and send my proposal again.
Hope you don't mind.

Marek  

Honestly... It's a bit sad.
The netdev trigger have its limitation and I see introducing an
additional trigger a practical way to correctly support some
strange/specific PHY.
I implemented both idea: expand netdev and introduce a dedicated
trigger and still this is problematic.
Is having an additional trigger for the specific task that bad?

I don't care as long as the feature is implemented but again
pretty sad how this LEDs proposal went.

Dear Ansuel,

  Is having an additional trigger for the specific task that bad?

No, for a very specific thing it is not bad. By specific I mean
something that an existing trigger does not support and can't support
in a reasonable way. But netdev trigger already supports blinking on rx
and tx, and even setting blinking frequency, and indicating link. And
it can be reasonably extended to support indicating different link
modes and even more complex things. For example what I would like to
see is having support for indicating different links by different
colors with RGB LEDs. I have ideas about how it can be implemented.

But a very specific thing can be implemented by a separate trigger.
For an ethernet PHY chip this can be something like ethernet collision
indication, or Energy Efficient Ethernet indication. But link and
activity should be done via netdev.

Note that we even have an API for such specific triggers that can only
work with some LEDs: the LED private triggers. Look at the member
  struct led_hw_trigger_type *trigger_type;
of struct led_classdev and struct led_trigger.
With this member you can make a trigger to be only visible for some
LEDs, so that when the user does
  cd /sys/class/leds/<LED_WITH_PRIVATE_TRIGGER>
  cat trigger
it will output
  [none] netdev something something ... my-private-trigger
but for other LEDs (all those with different trigger_type) it will omit
the my-private-trigger.

Your hardware-phy trigger should in fact use this private trigger API
so that the user only sees the trigger for the LEDs that actually
support it.

Anyway, Ansuel, if you are willing, we can have a call about this where
I can explain my ideas to you and you to me and we can discuss it more
and maybe come to an understanding? I am not opposed to working on this
together.

Marek