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:
signature.asc
Description: OpenPGP digital signature
