Re: [PATCH 3/6] leds: bd2606mvv: use device_for_each_child_node() to access device child nodes

[Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx>]

On 12/07/2024 23:06, Andreas Kemnade wrote:
> On Mon, 8 Jul 2024 17:45:43 +0200
> Javier Carrasco <javier.carrasco.cruz@xxxxxxxxx> wrote:
> 
>> On 08/07/2024 10:14, Javier Carrasco wrote:
>> What was the reason for this modification? Apparently, similar drivers
>> do everything in one loop to avoid such issues.
>>
> The reason for two loops is that we check in the first loop whether
> broghtness can be individually controlled so we can set max_brightness
> in the second loop. I had the assumption that max_brightness should be
> set before registering leds.
> 
> Some LEDs share brightness register, in the case where leds are defined
> with a shared register, we revert to on-off.
> 
>> Maybe refactoring to have a single loop again (if possible) would be
>> the cleanest solution. Otherwise a get/put mechanism might be
>> necessary.
>>
> I had no idea how to do it the time I wrote the patch.
> 
> Regards,
> Andreas

Then we could leave the two loops, and fix them. I am thinking of something
like this:

 static int bd2606mvv_probe(struct i2c_client *client)
 {
-	struct fwnode_handle *child;
 	struct device *dev = &client->dev;
 	struct bd2606mvv_priv *priv;
 	struct fwnode_handle *led_fwnodes[BD2606_MAX_LEDS] = { 0 };
 	int active_pairs[BD2606_MAX_LEDS / 2] = { 0 };
 	int err, reg;
-	int i;
+	int i, j;

 	if (!dev_fwnode(dev))
 		return -ENODEV;
@@ -93,20 +92,18 @@ static int bd2606mvv_probe(struct i2c_client *client)

 	i2c_set_clientdata(client, priv);

-	device_for_each_child_node(dev, child) {
+	device_for_each_child_node_scoped(dev, child) {
 		struct bd2606mvv_led *led;

 		err = fwnode_property_read_u32(child, "reg", &reg);
-		if (err) {
-			fwnode_handle_put(child);
+		if (err)
 			return err;
-		}
-		if (reg < 0 || reg >= BD2606_MAX_LEDS || led_fwnodes[reg]) {
-			fwnode_handle_put(child);
+
+		if (reg < 0 || reg >= BD2606_MAX_LEDS || led_fwnodes[reg])
 			return -EINVAL;
-		}
+
 		led = &priv->leds[reg];
-		led_fwnodes[reg] = child;
+		led_fwnodes[reg] = fwnode_handle_get(child);
 		active_pairs[reg / 2]++;
 		led->priv = priv;
 		led->led_no = reg;
@@ -129,7 +126,8 @@ static int bd2606mvv_probe(struct i2c_client *client)
 						     &priv->leds[i].ldev,
 						     &init_data);
 		if (err < 0) {
-			fwnode_handle_put(child);
+			for (j = i; j < BD2606_MAX_LEDS; j++)
+				fwnode_handle_put(led_fwnodes[j]);
 			return dev_err_probe(dev, err,
 					     "couldn't register LED %s\n",
 					     priv->leds[i].ldev.name);



Thanks to the call to fwnode_get_handle(child), the child nodes get their
refcount incremented to be used in the second loop, where all child nodes that
have not been registered are released in case of error.

The first loop becomes a scoped one, keeping the `child` variable from being
accessed anywhere else.

Any feedback before I send a v2 with this is very welcome.

Best regards,
Javier Carrasco