Re: [PATCH 1/3] leds: Add of_led_get() and led_put()

[Tomi Valkeinen <tomi.valkeinen@ti.com>]

[-- Attachment #1: Type: text/plain, Size: 2195 bytes --]

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).

>> +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.

>> +    if (!led_dev) {
>> +        of_node_put(led_node);
>> +        return ERR_PTR(-EPROBE_DEFER);
>> +    }
>> +
>> +    of_node_put(led_node);
> 
>> +    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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]