Hi,

On 25/08/15 16:25, Jacek Anaszewski wrote:

Hi Tomi,

Thanks for the patch. Generally, I'd prefer to add files
drivers/leds/of.c and include/linux/of_leds.h and put related functions
there. Those functions' names should begin with "of_". Please provide

Ok, I'll do that. I do need to export something from led-class in that
case, so that the of.c gets hold of 'leds_class' pointer, either
directly or indirectly.

also no-op versions of the functions to address configurations when
CONFIG_OF isn't enabled. I have also few comments below.

Yep. No-ops for the purpose of making the kernel image smaller? I do
think the current code compiles and works fine with CONFIG_OF disabled
(although I have to say I don't remember if I actually tested it).

quoted

+struct led_classdev *of_led_get(struct device_node *np)
+{
+    struct device *led_dev;
+    struct led_classdev *led_cdev;
+    struct device_node *led_node;
+
+    led_node = of_parse_phandle(np, "leds", 0);
+    if (!led_node)
+        return ERR_PTR(-ENODEV);
+
+    led_dev = class_find_device(leds_class, NULL, led_node,
+        led_match_led_node);

Single of_node_put(led_node) here will do.

Right.

quoted

+    if (!led_dev) {
+        of_node_put(led_node);
+        return ERR_PTR(-EPROBE_DEFER);
+    }
+
+    of_node_put(led_node);

quoted

+    led_cdev = dev_get_drvdata(led_dev);
+
+    if (!try_module_get(led_cdev->dev->parent->driver->owner))
+        return ERR_PTR(-ENODEV);
+
+    return led_cdev;
+}
+EXPORT_SYMBOL_GPL(of_led_get);
+/**
+ * led_put() - release a LED device
+ * @led_cdev: LED device
+ */
+void led_put(struct led_classdev *led_cdev)
+{
+    module_put(led_cdev->dev->parent->driver->owner);
+}
+EXPORT_SYMBOL_GPL(led_put);

Please move it to include/linux/leds.h, make static inline and provide
also no-op version for the case when CONFIG_LEDS_CLASS isn't enabled.

Ok. Why do you want it as static inline in the leds.h? I usually like to
keep the matching functions (get and put here) in the same place.

 Tomi