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

[Andres Salomon]

On Thu, 17 Mar 2011 17:24:55 +0000
Ed W <lists at wildgooses.com> wrote:

> 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.
> 

OLPC stuff lives in arch/x86/platform/olpc; if there was more
alix-specific stuff, I'd suggest moving it into something similar.
However, I didn't find any.  Maybe an arch/x86/platform/geode as a
place to collect platform drivers for the various geode-based machines
out there (alix, soekris, etc)?  Though honestly, I'm not that
interested in doing the work to migrate stuff over to there.


> 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?).

Yes, it should be module_init.  There shouldn't be any issues with
leds-gpio; the driver will only bind once a device is added (so long as
nothing else named leds-gpio comes along before leds-alix2).

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