[PATCH] leds: is31fl319x: modernize registration

[PATCH] leds: is31fl319x: modernize registration

[Andreas Kemnade]

Use _ext version to have properties parsed to avoid needing to parse
them in the driver itself. More modern properties are recognized and
the leds can be referenced via phandle.
Due to the maximum current mechanics, leds are not registered right
in the first iteration over the nodes.

Signed-off-by: Andreas Kemnade <andreas@kemnade.info>
---
 drivers/leds/leds-is31fl319x.c | 71 +++++++++++++++++++++---------------------
 1 file changed, 35 insertions(+), 36 deletions(-)

diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
index 80f38dba0fba..e5cca596fe7c 100644
--- a/drivers/leds/leds-is31fl319x.c
+++ b/drivers/leds/leds-is31fl319x.c
@@ -98,7 +98,7 @@ struct is31fl319x_chip {
 		struct is31fl319x_chip  *chip;
 		struct led_classdev     cdev;
 		u32                     max_microamp;
-		bool                    configured;
+		struct fwnode_handle *fwnode;
 	} leds[IS31FL319X_MAX_LEDS];
 };
 
@@ -363,33 +363,6 @@ static const struct of_device_id of_is31fl319x_match[] = {
 };
 MODULE_DEVICE_TABLE(of, of_is31fl319x_match);
 
-static int is31fl319x_parse_child_fw(const struct device *dev,
-				     const struct fwnode_handle *child,
-				     struct is31fl319x_led *led,
-				     struct is31fl319x_chip *is31)
-{
-	struct led_classdev *cdev = &led->cdev;
-	int ret;
-
-	if (fwnode_property_read_string(child, "label", &cdev->name))
-		cdev->name = fwnode_get_name(child);
-
-	ret = fwnode_property_read_string(child, "linux,default-trigger", &cdev->default_trigger);
-	if (ret < 0 && ret != -EINVAL) /* is optional */
-		return ret;
-
-	led->max_microamp = is31->cdef->current_default;
-	ret = fwnode_property_read_u32(child, "led-max-microamp", &led->max_microamp);
-	if (!ret) {
-		if (led->max_microamp < is31->cdef->current_min)
-			return -EINVAL;	/* not supported */
-		led->max_microamp = min(led->max_microamp,
-					is31->cdef->current_max);
-	}
-
-	return 0;
-}
-
 static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 {
 	struct fwnode_handle *fwnode = dev_fwnode(dev);
@@ -427,14 +400,20 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 
 		led = &is31->leds[reg - 1];
 
-		if (led->configured)
+		if (led->fwnode)
 			return dev_err_probe(dev, -EINVAL, "led %u is already configured\n", reg);
 
-		ret = is31fl319x_parse_child_fw(dev, child, led, is31);
-		if (ret)
-			return dev_err_probe(dev, ret, "led %u DT parsing failed\n", reg);
+		led->max_microamp = is31->cdef->current_default;
+		ret = fwnode_property_read_u32(child, "led-max-microamp", &led->max_microamp);
+		if (!ret) {
+			if (led->max_microamp < is31->cdef->current_min)
+				return dev_err_probe(dev, -EINVAL, "invalid maximum corrunt\n");
+
+			led->max_microamp = min(led->max_microamp,
+						is31->cdef->current_max);
+		}
 
-		led->configured = true;
+		led->fwnode = fwnode_handle_get(child);
 	}
 
 	is31->audio_gain_db = 0;
@@ -483,6 +462,19 @@ static inline int is31fl3196_db_to_gain(u32 dezibel)
 	return dezibel / IS31FL3196_AUDIO_GAIN_DB_STEP;
 }
 
+static void is31_free_fwnode(void *data)
+{
+	struct is31fl319x_chip *is31 = data;
+	int i;
+
+	for (i = 0; i < is31->cdef->num_leds; i++) {
+		if (is31->leds[i].fwnode)
+			fwnode_handle_put(is31->leds[i].fwnode);
+
+		is31->leds[i].fwnode = NULL;
+	}
+}
+
 static int is31fl319x_probe(struct i2c_client *client)
 {
 	struct is31fl319x_chip *is31;
@@ -498,6 +490,10 @@ static int is31fl319x_probe(struct i2c_client *client)
 	if (!is31)
 		return -ENOMEM;
 
+	err = devm_add_action_or_reset(dev, is31_free_fwnode, is31);
+	if (err)
+		return err;
+
 	err = devm_mutex_init(dev, &is31->lock);
 	if (err)
 		return err;
@@ -531,7 +527,7 @@ static int is31fl319x_probe(struct i2c_client *client)
 	 */
 	aggregated_led_microamp = is31->cdef->current_max;
 	for (i = 0; i < is31->cdef->num_leds; i++)
-		if (is31->leds[i].configured &&
+		if (is31->leds[i].fwnode &&
 		    is31->leds[i].max_microamp < aggregated_led_microamp)
 			aggregated_led_microamp = is31->leds[i].max_microamp;
 
@@ -545,14 +541,17 @@ static int is31fl319x_probe(struct i2c_client *client)
 
 	for (i = 0; i < is31->cdef->num_leds; i++) {
 		struct is31fl319x_led *led = &is31->leds[i];
+		struct led_init_data init_data = {};
 
-		if (!led->configured)
+		if (!led->fwnode)
 			continue;
 
+		init_data.fwnode = led->fwnode;
+
 		led->chip = is31;
 		led->cdev.brightness_set_blocking = is31->cdef->brightness_set;
 
-		err = devm_led_classdev_register(&client->dev, &led->cdev);
+		err = devm_led_classdev_register_ext(&client->dev, &led->cdev, &init_data);
 		if (err < 0)
 			return err;
 	}

---
base-commit: dc59e4fea9d83f03bad6bddf3fa2e52491777482
change-id: 20260702-led-modern-d427ed011f83

Best regards,
--  
Andreas Kemnade <andreas@kemnade.info>

Re: [PATCH] leds: is31fl319x: modernize registration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] The newly added devres cleanup action `is31_free_fwnode` unconditionally dereferences `is31->cdef` before it is initialized, causing a fatal NULL pointer dereference on early probe failures.
- [Low] Typo in the error message for invalid maximum current validation: 'corrunt' instead of 'current'.
--

commit f41ac0185576615af09273cdd8749b261df7d08e
Author: Andreas Kemnade <andreas@kemnade.info>

leds: is31fl319x: modernize registration

This patch modifies the is31fl319x driver to use the extended LED registration
API to parse properties automatically. It also delays LED registration until
after all firmware nodes are parsed to correctly handle the maximum current
mechanics.

> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
> index 80f38dba0fba4..e5cca596fe7c0 100644
> --- a/drivers/leds/leds-is31fl319x.c
> +++ b/drivers/leds/leds-is31fl319x.c
[ ... ]
> @@ -427,14 +400,20 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
>  
>  		led = &is31->leds[reg - 1];
>  
> -		if (led->configured)
> +		if (led->fwnode)
>  			return dev_err_probe(dev, -EINVAL, "led %u is already configured\n", reg);
>  
> -		ret = is31fl319x_parse_child_fw(dev, child, led, is31);
> -		if (ret)
> -			return dev_err_probe(dev, ret, "led %u DT parsing failed\n", reg);
> +		led->max_microamp = is31->cdef->current_default;
> +		ret = fwnode_property_read_u32(child, "led-max-microamp", &led->max_microamp);
> +		if (!ret) {
> +			if (led->max_microamp < is31->cdef->current_min)
> +				return dev_err_probe(dev, -EINVAL, "invalid maximum corrunt\n");

[Severity: Low]
This isn't a bug, but there appears to be a typo here ("corrunt" instead
of "current").

> +
> +			led->max_microamp = min(led->max_microamp,
> +						is31->cdef->current_max);
> +		}
>  
> -		led->configured = true;
> +		led->fwnode = fwnode_handle_get(child);
>  	}
>  
>  	is31->audio_gain_db = 0;
[ ... ]
> @@ -483,6 +462,19 @@ static inline int is31fl3196_db_to_gain(u32 dezibel)
>  	return dezibel / IS31FL3196_AUDIO_GAIN_DB_STEP;
>  }
>  
> +static void is31_free_fwnode(void *data)
> +{
> +	struct is31fl319x_chip *is31 = data;
> +	int i;
> +
> +	for (i = 0; i < is31->cdef->num_leds; i++) {
> +		if (is31->leds[i].fwnode)
> +			fwnode_handle_put(is31->leds[i].fwnode);
> +
> +		is31->leds[i].fwnode = NULL;
> +	}
> +}
> +
>  static int is31fl319x_probe(struct i2c_client *client)
>  {
>  	struct is31fl319x_chip *is31;
> @@ -498,6 +490,10 @@ static int is31fl319x_probe(struct i2c_client *client)
>  	if (!is31)
>  		return -ENOMEM;
>  
> +	err = devm_add_action_or_reset(dev, is31_free_fwnode, is31);

[Severity: High]
Could this sequence cause a NULL pointer dereference on the error path?

The cleanup action is registered here before is31->cdef is populated.
If an initialization step fails early before is31->cdef is assigned, the
devres core will call is31_free_fwnode() which then dereferences the
uninitialized is31->cdef in its loop condition.

For example, if devm_gpiod_get_optional() defers probe in is31fl319x_parse_fw():

is31fl319x_parse_fw() {
	is31->shutdown_gpio = devm_gpiod_get_optional(...);
	if (IS_ERR(is31->shutdown_gpio))
		return dev_err_probe(...);

	...
	is31->cdef = device_get_match_data(dev);
}

The early return triggers the cleanup, which evaluates
is31->cdef->num_leds, leading to a panic.

> +	if (err)
> +		return err;
> +
>  	err = devm_mutex_init(dev, &is31->lock);
>  	if (err)
>  		return err;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260702-led-modern-v1-1-a4af2e9aad60@kemnade.info?part=1