On Thu, Jan 06, 2011 at 12:05:51PM +0000, aaro.koskinen@xxxxxxxxx wrote:
> Hi,
>
> Russell King - ARM Linux [linux@xxxxxxxxxxxxxxxx]:
> > > > -static struct platform_suspend_ops omap_pm_ops = {
> > > > - .begin = omap2_pm_begin,
> > > > - .enter = omap2_pm_enter,
> > > > - .end = omap2_pm_end,
> > > > - .valid = suspend_valid_only_mem,
> > > > +static const struct platform_suspend_ops omap_pm_ops[] = {
> > > > + {
> > > > + .begin = omap2_pm_begin,
> > > > + .enter = omap2_pm_enter,
> > > > + .end = omap2_pm_end,
> > > > + .valid = suspend_valid_only_mem,
> > > > + }
> > > > };
> > > > -#else
> > > > -static const struct platform_suspend_ops __initdata omap_pm_ops;
> > > > #endif /* CONFIG_SUSPEND */
> > > >
> > > > /* XXX This function should be shareable between OMAP2xxx and OMAP3 */
> > > > @@ -582,7 +582,7 @@ static int __init omap2_pm_init(void)
> > > > omap24xx_cpu_suspend_sz);
> > > > }
> > > >
> > > > - suspend_set_ops(&omap_pm_ops);
> > > > + suspend_set_ops(omap_pm_ops);
> >
> > Utterly yuck. Declaring it as a single element array just to avoid an
> > ifdef. That's worse than having an ifdef here.
>
> Why it is worse? Reducing the amount of different preprocessor branches will
> result in better compile test / static analysis coverage.
You're not doing that. You're just spreading the yuck around making it
far worse:
- in some header file
#ifndef CONFIG_FOO
#define omap_pm_ops NULL
#endif
- in lots of C files
#ifdef CONFIG_FOO
static const struct platform_suspend_ops omap_pm_ops[] = {
{
...
}
};
#endif
suspend_set_ops(omap_pm_ops);
So, rather than just having the full story in each file, it's spread
across two files. Not only that, but over time CONFIG_FOO will probably
change, and that will lead to compile errors.
You're creating an array just to be able to use the symbol as a pointer.
That's a hack rather than an elegant solution.
So no, your implementation is _NOT_ a sane approach.
I'm going to explicitly say NAK to your patch here and now. I really
don't like it one bit. It's a complete hack through and through, and
that hack is spread across many files.
--
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
