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

[Grant Likely <grant.likely@secretlab.ca>]

On Thu, Mar 17, 2011 at 05:24:55PM +0000, Ed W 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.
> 
> 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?

No, that's bad practise.  You should have one Alix setup file,
probably in arch/x86/platform, that takes care of all Alix specific
setup registrations.  Right now that only consists of gpio leds, but
it could potentially grow.  Note that this is only for on-time setup
and registrations.  Actual device drivers still belong under drivers/

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

Nope.  It does not matter if the driver gets registered first or the
device.

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

Unless you've got the hardware to test, probably not.

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

Thanks

g.