Re: [PATCH net-next 2/4] net: dsa: realtek: keep default LED state in rtl8366rb

[Simon Horman <horms@kernel.org>]

On Sun, Mar 10, 2024 at 09:01:46AM +0100, Krzysztof Kozlowski wrote:
> On 10/03/2024 05:51, Luiz Angelo Daros de Luca wrote:
> > This switch family supports four LEDs for each of its six ports. Each
> > LED group is composed of one of these four LEDs from all six ports. LED
> > groups can be configured to display hardware information, such as link
> > activity, or manually controlled through a bitmap in registers
> > RTL8366RB_LED_0_1_CTRL_REG and RTL8366RB_LED_2_3_CTRL_REG.
> > 
> > After a reset, the default LED group configuration for groups 0 to 3
> > indicates, respectively, link activity, link at 1000M, 100M, and 10M, or
> > RTL8366RB_LED_CTRL_REG as 0x5432. These configurations are commonly used
> > for LED indications. However, the driver was replacing that
> > configuration to use manually controlled LEDs (RTL8366RB_LED_FORCE)
> > without providing a way for the OS to control them. The default
> > configuration is deemed more useful than fixed, uncontrollable turned-on
> > LEDs.
> > 
> > The driver was enabling/disabling LEDs during port_enable/disable.
> > However, these events occur when the port is administratively controlled
> > (up or down) and are not related to link presence. Additionally, when a
> > port N was disabled, the driver was turning off all LEDs for group N,
> > not only the corresponding LED for port N in any of those 4 groups. In
> > such cases, if port 0 was brought down, the LEDs for all ports in LED
> > group 0 would be turned off. As another side effect, the driver was
> > wrongly warning that port 5 didn't have an LED ("no LED for port 5").
> > Since showing the administrative state of ports is not an orthodox way
> > to use LEDs, it was not worth it to fix it and all this code was
> > dropped.
> > 
> > The code to disable LEDs was simplified only changing each LED group to
> > the RTL8366RB_LED_OFF state. Registers RTL8366RB_LED_0_1_CTRL_REG and
> > RTL8366RB_LED_2_3_CTRL_REG are only used when the corresponding LED
> > group is configured with RTL8366RB_LED_FORCE and they don't need to be
> > cleaned. The code still references an LED controlled by
> > RTL8366RB_INTERRUPT_CONTROL_REG, but as of now, no test device has
> > actually used it. Also, some magic numbers were replaced by macros.
> > 
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@gmail.com>
> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
> 
> This is the first version, so where did review happen?

FWIIW, I think this relates to review of an RFC of this patch-set.

https://lore.kernel.org/netdev/CACRpkda8tQ2u4+jCrpOQXbBd84oW98vmiDgU+GgdYCHuZfn49A@mail.gmail.com/