Feedback please: [PATCH] leds: New PCEngines Alix LED driver using gpio interface

[Ed W]

On 17/03/2011 16:08, Grant Likely wrote:
> Actually, it looks like with your changes this isn't even a driver
> anymore.  It is merely code to register a device on a specific
> platform.  Is there any other alix-specific initialization code in the
> kernel?  If so, you should consider relocating the device registration
> with the rest of the alix setup code.

Agreed.  I confess that I don't understand the linux driver structure
enough to shift the code further though

What I observe is that there is a lot of arch specific setup for ARM,
etc, however, this is not currently done at all for x86 (which is Alix),
so at the moment this would seem to sit slightly awkwardly with current
x86 arch code?

Instead I found leds-net5501.c, which is for a very similar platform to
the Alix (not quite similar enough that I could combine the files) and I
used that as my prototype for this driver.

I think given that we already have a similar driver in the leds area
which does platform alike setup, this gives some justification for doing
the same with the Alix leds?  Additionally if we ever find we need Alix
specific setup code then the code is ready to be used as is by the
platform code?


>>> -module_init(alix_led_init);
>>> -module_exit(alix_led_exit);
>>> +arch_initcall(alix_init);
>>
>> Why is this arch_initcall rather than module_init?   If possible, it
>> would be good to have an unload hook as well.
> 
> Yes, unless you've got specific ordering constraints this should
> definitely be module_init().

I'm out of my depth here.  I would be very happy to resubmit either way?

However, is there not a potential ordering issue if leds-alix2 is loaded
*before* leds-gpio? Is this not the reason for making it an arch_initcall?

Also the same code is used in leds-5501.c - would you like me to submit
a patch to change that also (if you confirm it should become a
module_init call?).

Thanks for final confirmation on this and I will quickly resubmit the patch?

Ed W