Re: [PATCH] leds: kill CONFIG_LEDS_CLASS option

[Richard Purdie]

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.

> > Rather than accept the current direction and force everything builtin,
> > I'd much rather this code became modular capable again. There is no good
> > reason we should be forced to build everything into a kernel.
> >
> 
> OK, cool. I'd like to help and could you please also give some
> comments about my ledtrig-cpu driver in this patchset?

I looked at that code and in summary, I like it.

My main concerns are just around being able to build things as modules
and that cpu trigger effectively forces all of led triggers as being
built in. It would be possible to avoid that by having a function
pointer for the cpu activity function and only calling it when its not
NULL. The module removal would need to be a little careful but it
shouldn't be that difficult.

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/