Re: [PATCH v2 2/2] leds: max597x: Add support for max597x

From: Naresh Solanki
Date: Mon Mar 27 2023 - 11:47:26 EST


Hi,

On 24-03-2023 09:06 pm, Christophe JAILLET wrote:
Le 24/03/2023 à 11:54, Naresh Solanki a écrit :
Hi,

On 24-03-2023 01:48 am, Christophe JAILLET wrote:
Le 23/03/2023 à 20:45, Naresh Solanki a écrit :
From: Patrick Rudolph <patrick.rudolph@xxxxxxxxxxxxx>

max597x is hot swap controller with indicator LED support.
This driver uses DT property to configure led during boot time &
also provide the LED control in sysfs.


[...]


+static int max597x_led_probe(struct platform_device *pdev)
+{
+    struct device_node *np = dev_of_node(pdev->dev.parent);
+    struct regmap *regmap = dev_get_regmap(pdev->dev.parent, NULL);
+    struct device_node *led_node;
+    struct device_node *child;
+    int ret = 0;
+
+    if (!regmap)
+        return -EPROBE_DEFER;
+
+    led_node = of_get_child_by_name(np, "leds");
+    if (!led_node)
+        return -ENODEV;
+
+    for_each_available_child_of_node(led_node, child) {
+        u32 reg;
+
+        if (of_property_read_u32(child, "reg", &reg))
+            continue;
+
+        if (reg >= MAX597X_NUM_LEDS) {
+            dev_err(&pdev->dev, "invalid LED (%u >= %d)\n", reg,
+                MAX597X_NUM_LEDS);
+            continue;
+        }
+
+        ret = max597x_setup_led(&pdev->dev, regmap, child, reg);
+        if (ret < 0)
+            of_node_put(child);

This of_node_put() looks odd to me.
Not sure if I get this right but if led setup fails of_node_put should be called.

My understanding is that this of_node_put() is there in case of error, to release what would otherwise never be released by for_each_available_child_of_node() if we exit early from the loop.

If the purpose is to release a reference taken in max597x_setup_led() in case of error:
   - this should be done IMHO within max597x_setup_led() directly
   - there should be a of_node_get() somewhere in max597x_setup_led()
     (after:
    if (of_property_read_string(nc, "label", &led->led.name))
        led->led.name = nc->name;
      + error handling path,  *or*
     just before the final return ret; when we know that everything is fine,
     if I understand correctly the code)

Is the reference taken elsewhere?
Did I miss something obvious?


One of the reference is "drivers/leds/leds-sc27xx-bltc.c" line 311
Please do let me know if I have to do anything about it.

"return ret;" or "break;" missing ?

Didn't add a break so that it can continue initializing remaining led if any.

Ok. Don't know the code enough to see if correct or not, but based on my comment above, I think that something is missing in max597x_setup_led() and that errors should be silently ignored here.
In my implementation, I have chosen to continue with the next LED if an error occurs, rather than aborting the 'for loop' with an error. I have seen other implementations also done in a similar way.
Do you have any further inputs or suggestions on this approach.


CJ

+    }
+
+    return ret;
+}
+

[...]

Regards,
Naresh