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

[Krzysztof Kozlowski]

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@xxxxxxxxx>
> Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>

This is the first version, so where did review happen?

Best regards,
Krzysztof