Re: Pet Peaves about Platform code, and arch_reset
On Wed, Nov 09, 2011 at 01:46:52PM -0800, Tony Lindgren wrote:
> * Tony Lindgren [07 16:11]:
> > * Russell King - ARM Linux [06 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?
>
> I guess spatch would need some patching to deal with this.. I tried
> with something like:
>
> @r@
> identifier I, s, fld;
> position p0,p;
> expression E;
> @@
>
> struct I s =@p0 {
> ...
> - .fld@p = E,
> + .fld = E,
> ...
> };
>
> That catches them, but adds an extra comma in the beginning :)
>
> --- a/ams-delta-fiq.c 2011-11-08 18:03:20.110707512 -0800
> +++ b/ams-delta-fiq.c 2011-11-08 18:04:36.094948038 -0800
> @@ -25,7 +25,7 @@
> #include
>
> static struct fiq_handler fh = {
> - .name = "ams-delta-fiq"
> + ,.name = "ams-delta-fiq",
> };
>
> /*
>
> And it also messes up the formatting for other structs..
>
> Anyways, I think I got most of these fixed for all ARM subarchitectures
> in a pile of 72 patches, the stats are:
>
> 486 files changed, 2296 insertions(+), 2296 deletions(-)
>
> This is something people can then use as a base to start chipping away at
> the problem. I'm thinking it may be possible to use this as a base for
> search and replacement type work and then hopefully git mergetool will
> pick the relevant changes when rebasing a working branch to the mainline.
>
> I can also post the patches here if people want, but sounds like we're
> not going to merge them as they are, but instead will slowly fix the
> issue in other related patches?
>
> I've pushed the patches into a git branch at:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap formatting:
>
> Web interface at:
>
> http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap.git;a=shortlog;h=refs/heads/formatting
>
For record, we may not want to do this for cases below, since we should
never add entry after sentinel.
static const struct of_device_id imx53_iomuxc_of_match[] __initconst = {
@@ -75,7 +75,7 @@ static const struct of_device_id imx53_iomuxc_of_match[]
__initconst = {
{ .compatible = "fsl,imx53-iomuxc-evk", .data = imx53_evk_common_init,
},
{ .compatible = "fsl,imx53-iomuxc-qsb", .data = imx53_qsb_common_init,
},
{ .compatible = "fsl,imx53-iomuxc-smd", .data = imx53_smd_common_init,
},
- { /* sentinel */ }
+ { /* sentinel */ },
};
static void __init imx53_dt_init(void)
@@ -112,7 +112,7 @@ static const char *imx53_dt_board_compat[] __initdata = {
"fsl,imx53-evk",
"fsl,imx53-qsb",
"fsl,imx53-smd",
- NULL
+ NULL,
};
--
Regards,
Shawn
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pet Peaves about Platform code, and arch_reset
* Tony Lindgren [07 16:11]:
> * Russell King - ARM Linux [06 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?
I guess spatch would need some patching to deal with this.. I tried
with something like:
@r@
identifier I, s, fld;
position p0,p;
expression E;
@@
struct I s =@p0 {
...
- .fld@p = E,
+ .fld = E,
...
};
That catches them, but adds an extra comma in the beginning :)
--- a/ams-delta-fiq.c 2011-11-08 18:03:20.110707512 -0800
+++ b/ams-delta-fiq.c 2011-11-08 18:04:36.094948038 -0800
@@ -25,7 +25,7 @@
#include
static struct fiq_handler fh = {
- .name = "ams-delta-fiq"
+ ,.name = "ams-delta-fiq",
};
/*
And it also messes up the formatting for other structs..
Anyways, I think I got most of these fixed for all ARM subarchitectures
in a pile of 72 patches, the stats are:
486 files changed, 2296 insertions(+), 2296 deletions(-)
This is something people can then use as a base to start chipping away at
the problem. I'm thinking it may be possible to use this as a base for
search and replacement type work and then hopefully git mergetool will
pick the relevant changes when rebasing a working branch to the mainline.
I can also post the patches here if people want, but sounds like we're
not going to merge them as they are, but instead will slowly fix the
issue in other related patches?
I've pushed the patches into a git branch at:
git://git.kernel.org/pub/scm/linux/kernel/git/tmlind/linux-omap formatting:
Web interface at:
http://git.kernel.org/?p=linux/kernel/git/tmlind/linux-omap.git;a=shortlog;h=refs/heads/formatting
Regards,
Tony
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Pet Peaves about Platform code, and arch_reset
* Russell King - ARM Linux [06 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 call
