Re: [PATCH] leds: kill CONFIG_LEDS_CLASS option

[Bryan Wu]

On Thu, Dec 8, 2011 at 11:02 PM, Richard Purdie <rpurdie@xxxxxxxxx> wrote:
> On Thu, 2011-12-08 at 19:53 +0800, Bryan Wu wrote:
>> On Wed, Aug 31, 2011 at 7:49 PM, Richard Purdie <rpurdie@xxxxxxxxx> wrote:
>> > On Wed, 2011-08-31 at 10:45 +0800, Bryan Wu wrote:
>> >> On Tue, Aug 30, 2011 at 5:34 AM, Richard Purdie <rpurdie@xxxxxxxxx> wrote:
>> >> > On Mon, 2011-08-29 at 16:34 -0400, Nicolas Pitre wrote:
>> >> >> On Tue, 30 Aug 2011, Bryan Wu wrote:
>> >> >>
>> >> >> > Almost all the new leds driver and trigger driver are depends on
>> >> >> > CONFIG_LED_CLASS, so there is no such user with CONFIG_NEW_LEDS=y
>> >> >> > and CONFIG_LED_CLASS=n. Moreover, lots of API functions in led-class.c
>> >> >> > are very common and should be built-in when CONFIG_NEW_LEDS=y.
>> >> >> >
>> >> >> > Obviously, CONFIG_LEDS_CLASS is pointless. This patch kills it and
>> >> >> > also updates defconfigs which contains CONFIG_LEDS_CLASS.
>> >> >> >
>> >> >> > Signed-off-by: Bryan Wu <bryan.wu@xxxxxxxxxxxxx>
>> >> >> > ---
>> >> >
>> >> > Looking at the code I'm a little concerned to see the direction this is
>> >> > going. There was originally a reason that there were two options, it was
>> >> > related to being able to compile as much of the LED code as a module as
>> >> > possible.
>> >> >
>> >>
>> >> I quite understand the original reason for 2 options, but I failed to
>> >> see any user of CONFIG_NEW_LEDS=y and CONFIG_LEDS_CLASS=n.
>> >
>> > Right, the intent was to support CONFIG_NEW_LEDS=y and
>> > CONFIG_LEDS_CLASS=m and I believe that still should be possible.
>> >
>> >> > It looks like commit 5ada28bf76752e33dce3d807bf0dfbe6d1b943ad changed
>> >> > the tristate to a bool at which point the separate option obviously
>> >> > becomes pointless.
>> >>
>> >> Exactly, this patch added some function API which are used very widely
>> >> as default LEDS driver behavior in some drivers.
>> >
>> > Looking at that commit, its adds the code to leds-class.c and shouldn't
>> > have needed to change the tristate to a bool. I know we discussed that
>> > at the time and I think that part might have been unnecessary and just
>> > accidentally committed. It could well be we can just revert that piece
>> > of the patch.
>> >
>>
>> Richard,
>>
>> Sorry for bringing up this topic so late. I was occupied by other
>> stuff for a while.
>>
>> I'm trying to make leds-class.c as a module, but found APIs exported
>> from led-class.c are widely used in kernel.
>>
>> For example: led_classdev_register()
>> --
>> linux-2.6$ grep -r led_classdev_register .
>> ./include/linux/leds.h:extern int led_classdev_register(struct device *parent,
>> ./drivers/mmc/host/au1xmmc.c:         ret = led_classdev_register(mmc_dev(mmc), led);
>
> If there are users of the LED API compiled in, then the config code
> should be handling this so the led class code is compiled in too.
>
> That shouldn't make it impossible to have the class code as a module, if
> the users are also modules.
>
>> When CONFIG_NEW_LEDS=y, CONFIG_LEDS_CLASS=m and CONFIG_LEDS_TRIGGER=y,
>> I got following error for building omap2plus_defconfig
>> --
>> arch/arm/plat-omap/built-in.o: In function `newled_init':
>> /opt/git/linux-2.6/arch/arm/plat-omap/debug-leds.c:248: undefined
>> reference to `led_classdev_register'
>
> So this is a dependency issues as far as I can see,
>
>> drivers/built-in.o: In function `led_trigger_blink':
>> /opt/git/linux-2.6/drivers/leds/led-triggers.c:248: undefined
>> reference to `led_blink_set'
>> drivers/built-in.o: In function `led_trigger_set':
>> /opt/git/linux-2.6/drivers/leds/led-triggers.c:116: undefined
>> reference to `led_brightness_set'
>
> but I agree these are more problematic.
>
>> The commit 5ada28bf76752e33dce3d807bf0dfbe6d1b943ad "led-class: always
>> implement blinking" added 2 API (led_blink_set and
>> `led_brightness_set'), which are used by led-trigger.c
>>
>> For this kind of errors, I think it's quite hard to set led-class.c as
>> a module now and suggest we merge NEW_LEDS and LEDS_CLASS together.
>
> I still don't believe this is necessary.
>
> How about moving led_timer_function(), led_set_software_blink()
> led_blink_set() and to led-triggers.c, putting #ifdef
> CONFIG_LEDS_TRIGGERS around the appropriate pieces of leds-class.c. We'd
> then move led_brightness_set() and led_stop_software_blink() to
> led-core.c since those are the real problem.
>
> It means there is a slightly higher always loaded footprint but its
> better than giving up and making it all always loaded.
>

Richard,

So sorry for the delay, I was occupied by other stuff for a long time.

As you pointed out, I moved some code to led-core.c to solve the
issue, please find my patch about that.

Thanks,
-- 
Bryan Wu <bryan.wu@xxxxxxxxxxxxx>
Kernel Developer    +86.138-1617-6545 Mobile
Canonical Ltd.      www.canonical.com
Ubuntu - Linux for human beings | www.ubuntu.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/