Re: [PATCH] leds: New PCEngines Alix LED driver using gpio interface
From: Grant Likely
Date: Fri Mar 18 2011 - 18:48:30 EST
On Fri, Mar 18, 2011 at 12:32 PM, Ed W <lists@xxxxxxxxxxxxxx> wrote:
> Hi, Thanks for "mentoring" me over this. I appreciate that this
> consumes your valuable time
> I have reposted what I hope is close to the correct code for this new
> driver. I'm somewhat apprehensive, so to draw attention to the things I
> have most likely "done wrong":
>> :000000 100644 0000000... bc1b3b3... A arch/x86/platform/geode/alix_leds.c
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index d5ed94d..b16ab56 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2094,6 +2094,15 @@ config OLPC_OPENFIRMWARE_DT
>> default y if OLPC_OPENFIRMWARE && PROC_DEVICETREE
>> select OF_PROMTREE
>> +config ALIX_LEDS
>> + bool "PCEngines ALIX.2/.3 LED Support"
>> + select GPIOLIB
>> + depends on LEDS_CLASS
>> + depends on LEDS_GPIO_PLATFORM && GPIO_CS5535
>> + ---help---
>> + This option enables support for the PCEngines ALIX.2 and ALIX.3 LEDs.
>> + You have to set alix-leds.force=1 for boards with Award BIOS.
>> endif # X86_32
> Driver and KConfig show this being specifically a driver for the Alix
> LEDs rather than being a general Alix plaform initialisation module?
> I was unsure if the trend was to have one module which initialised all
> Alix platform stuff (whatever it needs), or to split by function?
> Looking at other platform modules they seem to be somewhat fine grained
> so I went with a specific "ALIX Led Module" approach?
That's very much up to the board maintainer. Because it is just
device registrations, I would lump them all into one file. If they
were actual drivers then I tend to split them up.
> Additionally I am unsure how strictly to set dependencies for my module?
> It clearly requires all the LED_GPIO_PLATFORM, GPIO_CS5535 dependencies
> to do anything, but equally it doesn't seem to *break* anything if those
> dependencies aren't compiled in? Listing all the dependencies seems to
> make it hard for users to find the option to enable it since it's not
> even listed in menuconfig until your deps are met? Please correct me as
> to what level of deps should be listed?
Correct, you don't need to depend on the driver because this is just a
device registration. Who cares if it never gets bound to the driver?
The device registration will happily sit in the device model
> On the topic of dependencies, Andres has changed cs5535-gpio.c to depend
> on a new module cs5535-mfd - however, apart from the commit log message
> this is not obvious to see? OK, I'm an idiot, but it took me most of
> this afternoon to understand why my GPIOs stopped working after moving
> to 2.6.38? Q: Should the new -mfd module be listed as a dep of -gpio?
> Or perhaps -gpio should "select" -mfd? At least it would be helpful to
> list the dependency in the Kconfig message?
select is a little touchy because selecting another symbol bypasses
and dependences the other symbol has. In this case, it could skip
CONFIG_MFD_CORE which may be bad. -gpio should be a dependancy of
-mfd in this case I think.
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/