Messages in this thread | | | Date | Sun, 10 Mar 2024 11:49:28 +0000 | From | Simon Horman <> | Subject | Re: [PATCH net-next 4/4] net: dsa: realtek: add LED drivers for rtl8366rb |
| |
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@gmail.com> > Reviewed-by: Linus Walleij <linus.walleij@linaro.org> > --- > 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.
..
> +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.
> + 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.
> + return ret; > + } > + return 0; > +}
..
| |