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 _______________________________________________ Linux-geode mailing list [email protected] http://lists.infradead.org/mailman/listinfo/linux-geode
