[PATCH v2] leds: is31fl319x: modernize registration

[PATCH v2] 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>
---
Changes in v2:
- put node free action after cdef is initialized (Sashiko)
- Link to v1: https://patch.msgid.link/20260702-led-modern-v1-1-a4af2e9aad60@kemnade.info

To: Lee Jones <lee@kernel.org>
To: Pavel Machek <pavel@kernel.org>
Cc: linux-leds@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/leds/leds-is31fl319x.c | 59 +++++++++++++++++++++---------------------
 1 file changed, 29 insertions(+), 30 deletions(-)

diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
index 80f38dba0fba..a912adf2c7d3 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,31 +363,17 @@ 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)
+static void is31_free_fwnode(void *data)
 {
-	struct led_classdev *cdev = &led->cdev;
-	int ret;
-
-	if (fwnode_property_read_string(child, "label", &cdev->name))
-		cdev->name = fwnode_get_name(child);
+	struct is31fl319x_chip *is31 = data;
+	int i;
 
-	ret = fwnode_property_read_string(child, "linux,default-trigger", &cdev->default_trigger);
-	if (ret < 0 && ret != -EINVAL) /* is optional */
-		return ret;
+	for (i = 0; i < is31->cdef->num_leds; i++) {
+		if (is31->leds[i].fwnode)
+			fwnode_handle_put(is31->leds[i].fwnode);
 
-	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);
+		is31->leds[i].fwnode = NULL;
 	}
-
-	return 0;
 }
 
 static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
@@ -403,6 +389,10 @@ static int is31fl319x_parse_fw(struct device *dev, struct is31fl319x_chip *is31)
 
 	is31->cdef = device_get_match_data(dev);
 
+	ret = devm_add_action_or_reset(dev, is31_free_fwnode, is31);
+	if (ret)
+		return ret;
+
 	count = 0;
 	device_for_each_child_node_scoped(dev, child)
 		count++;
@@ -427,14 +417,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;
@@ -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 v2] leds: is31fl319x: modernize registration

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] Typo in error message string: 'corrunt' instead of 'current'.

Pre-existing issues:
- [High] Pre-existing NULL pointer dereference of `is31->cdef` when instantiating the driver via I2C sysfs (legacy matching).
--

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

leds: is31fl319x: modernize registration

This patch updates the driver to use the _ext version of LED registration
so properties can be parsed automatically and LEDs can be referenced via
phandle. It also defers LED registration until after iterating over the
nodes to determine the maximum current configuration.

> diff --git a/drivers/leds/leds-is31fl319x.c b/drivers/leds/leds-is31fl319x.c
> index 80f38dba0fba4..a912adf2c7d31 100644
> --- a/drivers/leds/leds-is31fl319x.c
> +++ b/drivers/leds/leds-is31fl319x.c
> @@ -363,31 +363,17 @@ static const struct of_device_id 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)
> +static void is31_free_fwnode(void *data)
>  {
> -	struct led_classdev *cdev = &led->cdev;
> -	int ret;
> -
> -	if (fwnode_property_read_string(child, "label", &cdev->name))
> -		cdev->name = fwnode_get_name(child);
> +	struct is31fl319x_chip *is31 = data;
> +	int i;
>  
> -	ret = fwnode_property_read_string(child, "linux,default-trigger", &cdev->default_trigger);
> -	if (ret < 0 && ret != -EINVAL) /* is optional */
> -		return ret;
> +	for (i = 0; i < is31->cdef->num_leds; i++) {

[Severity: High]
This isn't a bug introduced by this patch, but does this dereference of
is31->cdef in is31_free_fwnode() cause a kernel panic if the driver is
instantiated via I2C sysfs?

If the driver is bound manually via legacy I2C matching (for example, by
writing to /sys/bus/i2c/devices/i2c-0/new_device), device_get_match_data()
will return NULL because the driver's i2c_device_id array does not provide
.driver_data.

The driver already unconditionally dereferences is31->cdef later in
is31fl319x_parse_fw(), but this patch introduces an additional vector
where the new cleanup function runs and crashes on the same NULL pointer
dereference.

> +		if (is31->leds[i].fwnode)
> +			fwnode_handle_put(is31->leds[i].fwnode);
>  
> -	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);
> +		is31->leds[i].fwnode = NULL;
>  	}
> -
> -	return 0;
>  }
[ ... ]
> @@ -427,14 +417,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]
Is there a typo in this error message? It looks like "corrunt" should be
"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;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260703-led-modern-v2-1-d07dd0d6ec6f@kemnade.info?part=1