* Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx> [111106 05:18]:
> Here's a list of my peaves about current platform code - which are
> causing me great issue when trying to clean up the arch_reset() stuff:
>
> 1. Lack of trailing ',' on structure initializers
> This makes it much harder to add additional initializers at the end
> of existing initializers, and increases the risks of conflicts being
> caused due to more lines having to be modified.
>
> (This won't work directly because the tabs have been converted to space.
> The empty-looking [] contain a space plus a tab.)
> $ grep '[ ][ ]*\.[[:alnum:]_][[:alnum:]_]*[ ]*=[ ]*[[:alnum:]_{][[:alnum:]_|()}]*[^,]$' arch/arm -r|wc -l
> 768
> $ grep '[ ][ ]*\.[[:alnum:]_][[:alnum:]_]*[ ]*=[ ]*[[:alnum:]_{][[:alnum:]_|()}]*[^,]$' arch/arm/*omap* -r|wc -l
> 325
>
> Note that this is _far_ too big a problem - and trivial - to fix in
> a set of silly churn generating patches - it's a problem to be fixed
> as a part of _other_ changes to the files.
>
> But most importantly _stop_ introducing versions of this problem.
Sounds like we need a spatch for this issue?
> 2. Lack of consistent formatting for structure initializers within a
> mach- subdirectory. Some use tabs to align the '=' sign. Others
> use one space. This makes a repeated paste of a new initializer
> mismatch the rest of the formatting of the structure.
>
> I _really_ don't care to fix the new initializer I'm introducing to
> match the random formatting within a subdirectory.
This too could be fixes up using spatch?
> 3. Files containing one data structure or function are quite an annoyance
> when trying to do something like move arch_reset() out of the header
> file into the platform .c code; this requires creating yet another
> file containing one function, or consolidating the platform code
> together first. I've fixed clps711x for that (so I can convert it),
> but left others.
>
> 4. People who think that include files must be stored under a directory
> with 'include' somewhere mentioned in its path. This is a big one
> and a *REAL* hate. Include files _private_ to a group of source files
> in a directory should live in the same directory as those files.
> For instance, this should be zero because the 'map_io' function should
> not be exported outside of the arch/arm/mach-* subdirectory:
>
> $ grep -l map_io arch/arm/mach-*/include/mach/*.h | wc -l
> 21
>
> Let's look at some specific cases:
>
> $ grep omap15xx_map_io arch/arm/mach-omap1 arch/arm/plat-omap/ -r
> arch/arm/mach-omap1/board-innovator.c: omap15xx_map_io();
> arch/arm/mach-omap1/board-palmte.c: .map_io = omap15xx_map_io,
> arch/arm/mach-omap1/board-palmz71.c: .map_io = omap15xx_map_io,
> arch/arm/mach-omap1/board-voiceblue.c: .map_io = omap15xx_map_io,
> arch/arm/mach-omap1/io.c:void __init omap15xx_map_io(void)
> arch/arm/mach-omap1/board-palmtt.c: .map_io = omap15xx_map_io,
> arch/arm/mach-omap1/board-fsample.c: omap15xx_map_io();
> arch/arm/mach-omap1/board-sx1.c: .map_io = omap15xx_map_io,
> arch/arm/mach-omap1/board-ams-delta.c: .map_io = omap15xx_map_io,
> arch/arm/plat-omap/include/plat/io.h:void omap15xx_map_io(void);
> arch/arm/plat-omap/include/plat/io.h:static inline void omap15xx_map_io(void)
>
> What is the point of the omap15xx_map_io prototype being is a
> _completely_ different place to where it is defined?
>
> The problem is where do I put a function prototype for omap1_restart()
> amongst these header files for OMAP1 board files to pick up? Better
> still, don't tell me, but fix this stuff so that OMAP1 private stuff
> is in a 'common.h' or 'board.h' header file in arch/arm/mach-omap1.
Yeah we should add arch/arm/mach-omap1/common.h for this.
> $ grep s5pv210_init_irq arch/arm -r
> arch/arm/mach-s5pv210/mach-aquila.c: .init_irq = s5pv210_init_irq,
> arch/arm/mach-s5pv210/mach-torbreck.c: .init_irq = s5pv210_init_irq,
> arch/arm/mach-s5pv210/mach-goni.c: .init_irq = s5pv210_init_irq,
> arch/arm/mach-s5pv210/cpu.c:void __init s5pv210_init_irq(void)
> arch/arm/mach-s5pv210/mach-smdkc110.c: .init_irq = s5pv210_init_irq,
> arch/arm/mach-s5pv210/mach-smdkv210.c: .init_irq = s5pv210_init_irq,
> arch/arm/plat-samsung/include/plat/s5pv210.h:extern void s5pv210_init_irq(void);
>
> Again, what is the point of this lack of locality? And more-over,
> where the hell do I put a prototype for my new s5pv210_restart()
> which is in arch/arm/mach-s5pv210/cpu.c ? Again, don't tell me but
> fix stuff so that the function prototypes etc are closer to their
> definitions and uses.
>
> There is no excuse for this kind of crap, other than people being
> sheep and following idiotic and rediculous ideas like "include files
> must be under a directory called include".
>
> The arch_reset() branch, when published, will end with a commit removing
> the converted (and therefore empty) arch_reset() functions in mach/system.h,
> and its call site. Those platforms which haven't been converted will
> still have an arch_reset() function but this will no longer be called.
>
> I currently have a couple of commits which move things like OMAP1
> (dangling arch_reset), Exynos4 and S5PV210 (converted but missing a
> function prototype, and therefore fail to build) in the right direction
> _but_ will break because of the issues raised above, particularly (4).
> Whether I care to fix it depends on the reaction to this mail and whether
> people change their behaviour to how they lay code out.
>
> One point to note (for those who question whether it's a good idea to
> touch the old code): the problem platform code is not the old stuff.
> The old stuff, being older, is much less complex and therefore easier
> to convert. What's proving to be more of a headache is the more modern
> and current stuff, particularly where people have decided to follow
> non-kernel convention about things like placement of include files.
>
> For reference, he's the list - after five days work - of those platforms
> which are proving hard to convert:
>
> arch/arm/mach-bcmring/include/mach/system.h
> arch/arm/mach-davinci/include/mach/system.h
> arch/arm/mach-gemini/include/mach/system.h
> arch/arm/mach-ks8695/include/mach/system.h
> arch/arm/mach-netx/include/mach/system.h
> arch/arm/mach-nomadik/include/mach/system.h
> arch/arm/mach-omap1/include/mach/system.h
> arch/arm/mach-omap2/include/mach/system.h
> arch/arm/mach-s3c2410/include/mach/system-reset.h
> arch/arm/mach-s3c64xx/include/mach/system.h
> arch/arm/mach-shmobile/include/mach/system.h
> arch/arm/mach-vt8500/include/mach/system.h
> arch/arm/plat-mxc/include/mach/system.h
> arch/arm/plat-samsung/include/plat/system-reset.h
> arch/arm/plat-tcc/include/mach/system.h
> --
> To unsubscribe from this list: send the line "unsubscribe linux-omap" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
