Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb

From: Linus Walleij <linus.walleij@linaro.org>
To: Luiz Angelo Daros de Luca <luizluca@gmail.com>
Cc: netdev@vger.kernel.org, alsi@bang-olufsen.dk, andrew@lunn.ch,
	 f.fainelli@gmail.com, olteanv@gmail.com, davem@davemloft.net,
	 edumazet@google.com, kuba@kernel.org, pabeni@redhat.com,
	 arinc.unal@arinc9.com, ansuelsmth@gmail.com
Subject: Re: [RFC net-next 0/2] net: dsa: realtek: fix LED support for rtl8366rb
Date: Sun, 7 Jan 2024 21:16:54 +0100	[thread overview]
Message-ID: <CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com> (raw)
In-Reply-To: <20240106184651.3665-1-luizluca@gmail.com>

Hi Luiz,

On Sat, Jan 6, 2024 at 7:47 PM Luiz Angelo Daros de Luca
<luizluca@gmail.com> wrote:

> The rtl8366rb switch family has 4 LED groups, with one LED from each
> group for each of its 6 ports. LEDs in this family can be controlled
> manually using a bitmap or triggered by hardware. It's important to note
> that hardware triggers are configured at the LED group level, meaning
> all LEDs in the same group share the same hardware triggers settings.
>
> The first part of this series involves dropping most of the existing
> code, as, except for disabling the LEDs, it was not working as expected.
> If not disabled, the LEDs will retain their default settings after a
> switch reset, which may be sufficient for many devices.
>
> The second part introduces the LED driver to control the switch LEDs
> from sysfs or device-tree. This driver still allows the LEDs to retain
> their default settings, but it will shift to the software-based OS LED
> triggers if any configuration is changed. Subsequently, the LEDs will
> operate as normal LEDs until the switch undergoes another reset.
>
> Netdev LED trigger supports offloading to hardware triggers.
> Unfortunately, this isn't possible with the current LED API for this
> switch family. When the hardware trigger is enabled, it applies to all
> LEDs in the LED group while the LED API decides to offload based on only
> the state of a single LED. To avoid inconsistency between LEDs,
> offloading would need to check if all LEDs in the group share the same
> compatible settings and atomically enable offload for all LEDs.

I think these patches look great, and the driver certainly look better
after these changes than before them so if you resend without
RFC, please feel free to add my:

Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

HW triggers may be hard to implement but plain software control
is not bad either so this is already way better than what we had
before.

HW control can always be discussed and added later.

Yours,
Linus Walleij