* [PATCH] leds: rgb: qcom-lpg: Use fwnode APIs for LED parsing
@ 2026-09-08 5:32 Rosen Penev
2026-09-17 14:33 ` Lee Jones
0 siblings, 1 reply; 3+ messages in thread
From: Rosen Penev @ 2026-09-08 5:32 UTC (permalink / raw)
To: linux-leds; +Cc: Lee Jones, Pavel Machek, open list
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@gmail.com>
---
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", ®);
+ ret = fwnode_property_read_u32(fw, "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);
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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] leds: rgb: qcom-lpg: Use fwnode APIs for LED parsing
2026-09-08 5:32 [PATCH] leds: rgb: qcom-lpg: Use fwnode APIs for LED parsing Rosen Penev
@ 2026-09-17 14:33 ` Lee Jones
2026-09-17 18:42 ` Rosen Penev
0 siblings, 1 reply; 3+ messages in thread
From: Lee Jones @ 2026-09-17 14:33 UTC (permalink / raw)
To: Rosen Penev; +Cc: linux-leds, Pavel Machek, open list
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@gmail.com>
> ---
> 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", ®);
> + ret = fwnode_property_read_u32(fw, "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?
> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] leds: rgb: qcom-lpg: Use fwnode APIs for LED parsing
2026-09-17 14:33 ` Lee Jones
@ 2026-09-17 18:42 ` Rosen Penev
0 siblings, 0 replies; 3+ messages in thread
From: Rosen Penev @ 2026-09-17 18:42 UTC (permalink / raw)
To: Lee Jones; +Cc: linux-leds, Pavel Machek, open list
On Thu, Sep 17, 2026 at 7:33 AM Lee Jones <lee@kernel.org> 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@gmail.com>
> > ---
> > 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", ®);
> > + ret = fwnode_property_read_u32(fw, "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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-09-17 18:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-09-08 5:32 [PATCH] leds: rgb: qcom-lpg: Use fwnode APIs for LED parsing Rosen Penev
2026-09-17 14:33 ` Lee Jones
2026-09-17 18:42 ` Rosen Penev
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®