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

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

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


On 07/09/15 17:11, Jacek Anaszewski wrote:

>>> 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.
> 
> What exactly do you need to export?

The "static struct class *leds_class" from led-class.c, in one way or
another. of_led_get() needs to go through the led devices from the class.

For now I just removed the "static" from it, so that I can use it from of.c.

>>> 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?
> 
> No, for the case when support for LED subsystem is disabled in the
> kernel config. I'd rather backlight driver depended on LED subsystem
> than selected it.

Sorry, I didn't get that one. How does the backlight driver's
depend/select affect this?

>>> 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?
> 
> This function contains only one instruction, why should we waste
> a function call for it?

I like to keep code in .c files and leave .h files for declarations, and
only in special cases have inline code in .h files.

What do you mean with "waste"?

In this case there's no need to get more performance by inlining (which
anyway may not help and needs to be studied separately for every case),
with inlining the resulting kernel image is larger, and inlining forces
the users of led_put to include module.h to compile.

 Tomi


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