Re: [PATCH] leds: kill CONFIG_LEDS_CLASS option

[Richard Purdie]

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.

Cheers,

Richard


--
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/