lkml.org 
[lkml]   [2011]   [Mar]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
Date
SubjectRe: [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/

\
 
 \ /
  Last update: 2011-03-18 23:51    [W:1.065 / U:0.128 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site