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.
> There's another solution. Don't mess about with sticking such stuff in
> the header either.
>
> #ifdef WHATEVER
> 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,
> };
> #define PM_OPS omap_pm_ops
> #else
> #define PM_OPS NULL
> #endif
> ...
>
> suspend_set_ops(PM_OPS);
>
> That keeps it all nice and local, and you can see exactly what's going on
> without having it spread across many different random files.
I don't think it's obviously better or simpler. There's already made a mistake in
your example...
A.
--
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
