Re: [PATCH net-next 4/4] net: dsa: realtek: add LED drivers for rtl8366rb

[Luiz Angelo Daros de Luca]

> On Sun, Mar 10, 2024 at 01:52:01AM -0300, Luiz Angelo Daros de Luca wrote:
> > This commit introduces LED drivers for rtl8366rb, enabling LEDs to be
> > described in the device tree using the same format as qca8k. Each port
> > can configure up to 4 LEDs.
> >
> > If all LEDs in a group use the default state "keep", they will use the
> > default behavior after a reset. Changing the brightness of one LED,
> > either manually or by a trigger, will disable the default hardware
> > trigger and switch the entire LED group to manually controlled LEDs.
> > Once in this mode, there is no way to revert to hardware-controlled LEDs
> > (except by resetting the switch).
> >
> > Software triggers function as expected with manually controlled LEDs.
> >
> > Signed-off-by: Luiz Angelo Daros de Luca <luizluca@xxxxxxxxx>
> > Reviewed-by: Linus Walleij <linus.walleij@xxxxxxxxxx>
> > ---
> >  drivers/net/dsa/realtek/rtl8366rb.c | 270 ++++++++++++++++++++++++++++++++----
>
> ...
>
> > +static int rtl8366rb_setup_led(struct realtek_priv *priv, struct dsa_port *dp,
> > +                            struct fwnode_handle *led_fwnode)
> > +{
> > +     struct rtl8366rb *rb = priv->chip_data;
> > +     struct led_init_data init_data = { };
> > +     struct rtl8366rb_led *led;
> > +     enum led_default_state state;
> > +     u32 led_group;
> > +     int ret;
>
> nit: Please consider using reverse xmas tree - longest line to shortest -
>      for local variables in networking code.

Sorry, I might have renamed a variable to a shorter form without
rechecking the order. Thanks for the tip.

> ...
>
> > +static int rtl8366rb_setup_leds(struct realtek_priv *priv)
> > +{
> > +     struct device_node *leds_np, *led_np;
> > +     struct dsa_switch *ds = &priv->ds;
> > +     struct dsa_port *dp;
> > +     int ret;
> > +
> > +     dsa_switch_for_each_port(dp, ds) {
> > +             if (!dp->dn)
> > +                     continue;
> > +
> > +             leds_np = of_get_child_by_name(dp->dn, "leds");
> > +             if (!leds_np) {
> > +                     dev_dbg(priv->dev, "No leds defined for port %d",
> > +                             dp->index);
> > +                     continue;
> > +             }
> > +
> > +             for_each_child_of_node(leds_np, led_np) {
> > +                     ret = rtl8366rb_setup_led(priv, dp,
> > +                                               of_fwnode_handle(led_np));
> > +                     if (ret) {
> > +                             of_node_put(led_np);
>
> FWIIW, Coccinelle complans about "probable double put" here.
> But it looks correct to me as it's only called when breaking out of
> the loop, when it is required AFAIK.

I agree. I do think the put is required when you break the loop as the
put happens in the increment/decrement code, including the last one
when it naturally ends the loop with led_np defined as null.

>
> > +                             break;
> > +                     }
> > +             }
> > +
> > +             of_node_put(leds_np);
> > +             if (ret)
>
> I'm unsure if this can happen. But if for_each_child_of_node()
> iterates zero times then ret may be uninitialised here.
>
> Flagged by Smatch.

Yes, I'll initialize it as 0. It could also use if (led_np), as it
will only be defined if the loop was broken (but checking ret seems to
be simpler).

>
> > +                     return ret;
> > +     }
> > +     return 0;
> > +}
>
> ...