Re: [PATCH] leds: rgb: qcom-lpg: Use fwnode APIs for LED parsing

[Rosen Penev]

On Thu, Sep 17, 2026 at 7:33 AM Lee Jones <lee@xxxxxxxxxx> wrote:
>
> On Mon, 07 Sep 2026, Rosen Penev wrote:
>
> > Convert LED and channel parsing in lpg_add_led() and
> > lpg_parse_channel() from DT-specific helpers to the fwnode API.
> > The probe loop now uses device_for_each_child_node_scoped(),
> > dropping explicit node lifetime handling.
> >
> > Behaviour is unchanged for device tree: fwnode_get_child_node_count()
> > and both child iteration helpers skip unavailable children on OF,
> > and the fwnode handle is passed straight to LED registration.
> >
> > cdev->default_trigger is only assigned when
> > fwnode_property_read_string() succeeds, so an absent
> > linux,default-trigger property no longer yields an uninitialized
> > pointer.
> >
> > Assisted-by: opencode:deepseek-v4-flash-free
> > Signed-off-by: Rosen Penev <rosenp@xxxxxxxxx>
> > ---
> >  drivers/leds/rgb/leds-qcom-lpg.c | 36 ++++++++++++++++++--------------
> >  1 file changed, 20 insertions(+), 16 deletions(-)
> >
> > diff --git a/drivers/leds/rgb/leds-qcom-lpg.c b/drivers/leds/rgb/leds-qcom-lpg.c
> > index d7d6518de30f..dcc8e6f14c8b 100644
> > --- a/drivers/leds/rgb/leds-qcom-lpg.c
> > +++ b/drivers/leds/rgb/leds-qcom-lpg.c
> > @@ -11,6 +11,7 @@
> >  #include <linux/nvmem-consumer.h>
> >  #include <linux/of.h>
> >  #include <linux/platform_device.h>
> > +#include <linux/property.h>
> >  #include <linux/pwm.h>
> >  #include <linux/regmap.h>
> >  #include <linux/slab.h>
> > @@ -1342,7 +1343,7 @@ static int lpg_add_pwm(struct lpg *lpg)
> >       return ret;
> >  }
> >
> > -static int lpg_parse_channel(struct lpg *lpg, struct device_node *np,
> > +static int lpg_parse_channel(struct lpg *lpg, struct fwnode_handle *fw,
> >                            struct lpg_channel **channel)
> >  {
> >       struct lpg_channel *chan;
> > @@ -1350,17 +1351,18 @@ static int lpg_parse_channel(struct lpg *lpg, struct device_node *np,
> >       u32 reg;
> >       int ret;
> >
> > -     ret = of_property_read_u32(np, "reg", &reg);
> > +     ret = fwnode_property_read_u32(fw, "reg", &reg);
> >       if (ret || !reg || reg > lpg->num_channels)
> > -             return dev_err_probe(lpg->dev, -EINVAL, "invalid \"reg\" of %pOFn\n", np);
> > +             return dev_err_probe(lpg->dev, -EINVAL,
> > +                                  "invalid \"reg\" of %s\n", fwnode_get_name(fw));
> >
> >       chan = &lpg->channels[reg - 1];
> >       chan->in_use = true;
> >
> > -     ret = of_property_read_u32(np, "color", &color);
> > +     ret = fwnode_property_read_u32(fw, "color", &color);
> >       if (ret < 0 && ret != -EINVAL)
> >               return dev_err_probe(lpg->dev, ret,
> > -                                  "failed to parse \"color\" of %pOF\n", np);
> > +                                  "failed to parse \"color\" of %s\n", fwnode_get_name(fw));
> >
> >       chan->color = color;
> >
> > @@ -1369,25 +1371,26 @@ static int lpg_parse_channel(struct lpg *lpg, struct device_node *np,
> >       return 0;
> >  }
> >
> > -static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> > +static int lpg_add_led(struct lpg *lpg, struct fwnode_handle *fw)
> >  {
> >       struct led_init_data init_data = {};
> >       struct led_classdev *cdev;
> >       struct mc_subled *info;
> >       struct lpg_led *led;
> > +     const char *trigger;
> >       const char *state;
> >       int num_channels;
> >       u32 color = 0;
> >       int ret;
> >       int i;
> >
> > -     ret = of_property_read_u32(np, "color", &color);
> > +     ret = fwnode_property_read_u32(fw, "color", &color);
> >       if (ret < 0 && ret != -EINVAL)
> >               return dev_err_probe(lpg->dev, ret,
> > -                           "failed to parse \"color\" of %pOF\n", np);
> > +                           "failed to parse \"color\" of %s\n", fwnode_get_name(fw));
> >
> >       if (color == LED_COLOR_ID_RGB || color == LED_COLOR_ID_MULTI)
> > -             num_channels = of_get_available_child_count(np);
> > +             num_channels = fwnode_get_child_node_count(fw);
>
> Are you sure 'fwnode_get_child_node_count()' skips unavailable children?  It
> counts all children, whereas 'of_get_available_child_count()' only counted
> available ones.  If there are disabled child nodes, 'led->mcdev.num_colors' will
> be larger than the number of populated subleds, leading to uninitialised memory
> access in the LED core.  Should we be using 'i' for 'num_colors' instead, or
> counting available nodes properly?

fwnode_get_child_node_count() uses of_get_next_available_child()
through of_fwnode_get_next_child_node().

I could do

 -               led->mcdev.num_colors = num_channels;
 +               led->mcdev.num_colors = i;

which is just churn anyway.
>
> >       else
> >               num_channels = 1;
> >
> > @@ -1403,7 +1406,7 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> >               if (!info)
> >                       return -ENOMEM;
> >               i = 0;
> > -             for_each_available_child_of_node_scoped(np, child) {
> > +             fwnode_for_each_available_child_node_scoped(fw, child) {
> >                       ret = lpg_parse_channel(lpg, child, &led->channels[i]);
> >                       if (ret < 0)
> >                               return ret;
> > @@ -1426,7 +1429,7 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> >                       cdev->pattern_clear = lpg_pattern_mc_clear;
> >               }
> >       } else {
> > -             ret = lpg_parse_channel(lpg, np, &led->channels[0]);
> > +             ret = lpg_parse_channel(lpg, fw, &led->channels[0]);
> >               if (ret < 0)
> >                       return ret;
> >
> > @@ -1441,14 +1444,15 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> >               }
> >       }
> >
> > -     cdev->default_trigger = of_get_property(np, "linux,default-trigger", NULL);
> > +     if (!fwnode_property_read_string(fw, "linux,default-trigger", &trigger))
> > +             cdev->default_trigger = trigger;
> >
> >       if (lpg->lpg_chan_sdam)
> >               cdev->max_brightness = PPG_MAX_LED_BRIGHTNESS;
> >       else
> >               cdev->max_brightness = LPG_RESOLUTION_9BIT - 1;
> >
> > -     if (!of_property_read_string(np, "default-state", &state) &&
> > +     if (!fwnode_property_read_string(fw, "default-state", &state) &&
> >           !strcmp(state, "on"))
> >               cdev->brightness = cdev->max_brightness;
> >       else
> > @@ -1456,7 +1460,7 @@ static int lpg_add_led(struct lpg *lpg, struct device_node *np)
> >
> >       cdev->brightness_set_blocking(cdev, cdev->brightness);
> >
> > -     init_data.fwnode = of_fwnode_handle(np);
> > +     init_data.fwnode = fw;
> >
> >       if (color == LED_COLOR_ID_RGB || color == LED_COLOR_ID_MULTI)
> >               ret = devm_led_classdev_multicolor_register_ext(lpg->dev, &led->mcdev, &init_data);
> > @@ -1638,8 +1642,8 @@ static int lpg_probe(struct platform_device *pdev)
> >       if (ret < 0)
> >               return ret;
> >
> > -     for_each_available_child_of_node_scoped(pdev->dev.of_node, np) {
> > -             ret = lpg_add_led(lpg, np);
> > +     device_for_each_child_node_scoped(&pdev->dev, child) {
> > +             ret = lpg_add_led(lpg, child);
> >               if (ret)
> >                       return ret;
> >       }
> > --
> > 2.55.0
> >
>
> --
> Lee Jones