Messages in this thread | | | From | Grant Likely <> | Date | Fri, 18 Mar 2011 16:48:01 -0600 | Subject | Re: [PATCH] leds: New PCEngines Alix LED driver using gpio interface |
| |
On Fri, Mar 18, 2011 at 12:32 PM, Ed W <lists@wildgooses.com> 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 unregistered.
> 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.
g. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |