arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

PATCH New Aaro Koskinen 3 participants 9 messages Dec 29, 2010 – Jan 6, 2011

Subsystems (4)

Arm (32-bit) Arm sub-architectures OMAP2+ support OMAP power management support

lore

Share & tools

lore

Aaro Koskinen

author

2010-12-29 16:33 UTC #0 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Make !CONFIG_SUSPEND init declarations identical on all OMAPs and eliminate some ifdefs.

Signed-off-by Aaro Koskinen <[email protected]>

---
 arch/arm/mach-omap2/pm.h     |    4 ++++
 arch/arm/mach-omap2/pm24xx.c |   16 ++++++++--------
 arch/arm/mach-omap2/pm34xx.c |   16 ++++++++--------
 arch/arm/mach-omap2/pm44xx.c |   17 +++++++++--------
 4 files changed, 29 insertions(+), 24 deletions(-)

arch/arm/mach-omap2/pm.h +4 -0

index 1c1b0ab..704766b 100644
@@ -138,4 +138,8 @@ static inline int omap4_twl_init(void)
 }
 #endif
 
+#ifndef CONFIG_SUSPEND
+#define omap_pm_ops NULL
+#endif
+
 #endif

arch/arm/mach-omap2/pm24xx.c +8 -8

index dac2d1d..e65b329 100644
@@ -350,14 +350,14 @@ static void omap2_pm_end(void)
 	enable_hlt();
 }
 
-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);
 	pm_idle = omap2_pm_idle;
 
 	return 0;

arch/arm/mach-omap2/pm34xx.c +8 -8

index 5b323f2..a4c9283 100644
@@ -605,11 +605,13 @@ static void omap3_pm_end(void)
 	return;
 }
 
-static struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap3_pm_begin,
-	.end		= omap3_pm_end,
-	.enter		= omap3_pm_enter,
-	.valid		= suspend_valid_only_mem,
+static const struct platform_suspend_ops omap_pm_ops[] = {
+	{
+		.begin		= omap3_pm_begin,
+		.end		= omap3_pm_end,
+		.enter		= omap3_pm_enter,
+		.valid		= suspend_valid_only_mem,
+	}
 };
 #endif /* CONFIG_SUSPEND */

@@ -1067,9 +1069,7 @@ static int __init omap3_pm_init(void)
 	core_clkdm = clkdm_lookup("core_clkdm");
 
 	omap_push_sram_idle();
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	suspend_set_ops(omap_pm_ops);
 
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();

arch/arm/mach-omap2/pm44xx.c +9 -8

index e9f4862..6022c0a 100644
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 
+#include "pm.h"
 #include "powerdomain.h"
 #include <mach/omap4-common.h>

@@ -65,11 +66,13 @@ static void omap4_pm_end(void)
 	return;
 }
 
-static struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap4_pm_begin,
-	.end		= omap4_pm_end,
-	.enter		= omap4_pm_enter,
-	.valid		= suspend_valid_only_mem,
+static const struct platform_suspend_ops omap_pm_ops[] = {
+	{
+		.begin		= omap4_pm_begin,
+		.end		= omap4_pm_end,
+		.enter		= omap4_pm_enter,
+		.valid		= suspend_valid_only_mem,
+	}
 };
 #endif /* CONFIG_SUSPEND */

@@ -113,9 +116,7 @@ static int __init omap4_pm_init(void)
 	}
 #endif
 
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	suspend_set_ops(omap_pm_ops);
 
 err2:
 	return ret;

Signature · 1 line

1.5.6.5

Kevin Hilman

2011-01-04 22:41 UTC #1 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Quoted 4 lines · Aaro Koskinen

Make !CONFIG_SUSPEND init declarations identical on all OMAPs and eliminate some ifdefs.
Signed-off-by Aaro Koskinen <[email protected]>

Very nice.

I was going to tackle something like this with that last cleanup I did, but was rather late for the main 2.6.38 merge window so I just fixed problem at hand. This is much better, thanks.

Will queue for 2.6.38-rc2 in my pm-fixes branch.

Kevin

Quoted 131 lines

---
 arch/arm/mach-omap2/pm.h     |    4 ++++
 arch/arm/mach-omap2/pm24xx.c |   16 ++++++++--------
 arch/arm/mach-omap2/pm34xx.c |   16 ++++++++--------
 arch/arm/mach-omap2/pm44xx.c |   17 +++++++++--------
 4 files changed, 29 insertions(+), 24 deletions(-)

arch/arm/mach-omap2/pm.h +4 -0

index 1c1b0ab..704766b 100644
@@ -138,4 +138,8 @@ static inline int omap4_twl_init(void)
 }
 #endif
 
+#ifndef CONFIG_SUSPEND
+#define omap_pm_ops NULL
+#endif
+
 #endif

arch/arm/mach-omap2/pm24xx.c +8 -8

index dac2d1d..e65b329 100644
@@ -350,14 +350,14 @@ static void omap2_pm_end(void)
 	enable_hlt();
 }
 
-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);
 	pm_idle = omap2_pm_idle;
 
 	return 0;

arch/arm/mach-omap2/pm34xx.c +8 -8

index 5b323f2..a4c9283 100644
@@ -605,11 +605,13 @@ static void omap3_pm_end(void)
 	return;
 }
 
-static struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap3_pm_begin,
-	.end		= omap3_pm_end,
-	.enter		= omap3_pm_enter,
-	.valid		= suspend_valid_only_mem,
+static const struct platform_suspend_ops omap_pm_ops[] = {
+	{
+		.begin		= omap3_pm_begin,
+		.end		= omap3_pm_end,
+		.enter		= omap3_pm_enter,
+		.valid		= suspend_valid_only_mem,
+	}
 };
 #endif /* CONFIG_SUSPEND */

@@ -1067,9 +1069,7 @@ static int __init omap3_pm_init(void)
 	core_clkdm = clkdm_lookup("core_clkdm");
 
 	omap_push_sram_idle();
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	suspend_set_ops(omap_pm_ops);
 
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();

arch/arm/mach-omap2/pm44xx.c +9 -8

index e9f4862..6022c0a 100644
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 
+#include "pm.h"
 #include "powerdomain.h"
 #include <mach/omap4-common.h>

@@ -65,11 +66,13 @@ static void omap4_pm_end(void)
 	return;
 }
 
-static struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap4_pm_begin,
-	.end		= omap4_pm_end,
-	.enter		= omap4_pm_enter,
-	.valid		= suspend_valid_only_mem,
+static const struct platform_suspend_ops omap_pm_ops[] = {
+	{
+		.begin		= omap4_pm_begin,
+		.end		= omap4_pm_end,
+		.enter		= omap4_pm_enter,
+		.valid		= suspend_valid_only_mem,
+	}
 };
 #endif /* CONFIG_SUSPEND */

@@ -113,9 +116,7 @@ static int __init omap4_pm_init(void)
 	}
 #endif
 
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	suspend_set_ops(omap_pm_ops);
 
 err2:
 	return ret;

Kevin Hilman

2011-01-06 00:25 UTC #2 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

In reply to Aaro Koskinen

Hi Aaro,

Quoted 4 lines · Aaro Koskinen

Make !CONFIG_SUSPEND init declarations identical on all OMAPs and eliminate some ifdefs.
Signed-off-by Aaro Koskinen <[email protected]>

I like this solution, but it introduces compiler warnings:

/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm44xx.c: In function 'omap4_pm_init':
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm44xx.c:119: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type /opt/home/khilman/work.local/kernel/omap/pm/include/linux/suspend.h:125: note: expected 'struct platform_suspend_ops *' but argument is of type 'const struct platform_suspend_ops *' /opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm24xx.c: In function 'omap2_pm_init':
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm24xx.c:585: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type /opt/home/khilman/work.local/kernel/omap/pm/include/linux/suspend.h:125: note: expected 'struct platform_suspend_ops *' but argument is of type 'const struct platform_suspend_ops *' /opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm34xx.c: In function 'omap3_pm_init':
/opt/home/khilman/work.local/kernel/omap/pm/arch/arm/mach-omap2/pm34xx.c:1072: warning: passing argument 1 of 'suspend_set_ops' discards qualifiers from pointer target type /opt/home/khilman/work.local/kernel/omap/pm/include/linux/suspend.h:125:
note: expected 'struct platform_suspend_ops *' but argument is of type
'const struct platform_suspend_ops *'

As you likely noticed, removing the const leads to checkpatch warnings:

WARNING: struct platform_suspend_ops should normally be const

so the choice is between a checkpatch warning or a bunch of compiler warnings.

Alternatively, I just posted a patch[1] to linux-pm propsing to fix this at the source. Let's see what happens there. Merging $SUBJECT patch will depend on how this is fixed upstream.

Kevin

[1] https://patchwork.kernel.org/patch/455831/

Quoted 131 lines

---
 arch/arm/mach-omap2/pm.h     |    4 ++++
 arch/arm/mach-omap2/pm24xx.c |   16 ++++++++--------
 arch/arm/mach-omap2/pm34xx.c |   16 ++++++++--------
 arch/arm/mach-omap2/pm44xx.c |   17 +++++++++--------
 4 files changed, 29 insertions(+), 24 deletions(-)

arch/arm/mach-omap2/pm.h +4 -0

index 1c1b0ab..704766b 100644
@@ -138,4 +138,8 @@ static inline int omap4_twl_init(void)
 }
 #endif
 
+#ifndef CONFIG_SUSPEND
+#define omap_pm_ops NULL
+#endif
+
 #endif

arch/arm/mach-omap2/pm24xx.c +8 -8

index dac2d1d..e65b329 100644
@@ -350,14 +350,14 @@ static void omap2_pm_end(void)
 	enable_hlt();
 }
 
-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);
 	pm_idle = omap2_pm_idle;
 
 	return 0;

arch/arm/mach-omap2/pm34xx.c +8 -8

index 5b323f2..a4c9283 100644
@@ -605,11 +605,13 @@ static void omap3_pm_end(void)
 	return;
 }
 
-static struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap3_pm_begin,
-	.end		= omap3_pm_end,
-	.enter		= omap3_pm_enter,
-	.valid		= suspend_valid_only_mem,
+static const struct platform_suspend_ops omap_pm_ops[] = {
+	{
+		.begin		= omap3_pm_begin,
+		.end		= omap3_pm_end,
+		.enter		= omap3_pm_enter,
+		.valid		= suspend_valid_only_mem,
+	}
 };
 #endif /* CONFIG_SUSPEND */

@@ -1067,9 +1069,7 @@ static int __init omap3_pm_init(void)
 	core_clkdm = clkdm_lookup("core_clkdm");
 
 	omap_push_sram_idle();
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	suspend_set_ops(omap_pm_ops);
 
 	pm_idle = omap3_pm_idle;
 	omap3_idle_init();

arch/arm/mach-omap2/pm44xx.c +9 -8

index e9f4862..6022c0a 100644
@@ -16,6 +16,7 @@
 #include <linux/err.h>
 #include <linux/slab.h>
 
+#include "pm.h"
 #include "powerdomain.h"
 #include <mach/omap4-common.h>

@@ -65,11 +66,13 @@ static void omap4_pm_end(void)
 	return;
 }
 
-static struct platform_suspend_ops omap_pm_ops = {
-	.begin		= omap4_pm_begin,
-	.end		= omap4_pm_end,
-	.enter		= omap4_pm_enter,
-	.valid		= suspend_valid_only_mem,
+static const struct platform_suspend_ops omap_pm_ops[] = {
+	{
+		.begin		= omap4_pm_begin,
+		.end		= omap4_pm_end,
+		.enter		= omap4_pm_enter,
+		.valid		= suspend_valid_only_mem,
+	}
 };
 #endif /* CONFIG_SUSPEND */

@@ -113,9 +116,7 @@ static int __init omap4_pm_init(void)
 	}
 #endif
 
-#ifdef CONFIG_SUSPEND
-	suspend_set_ops(&omap_pm_ops);
-#endif /* CONFIG_SUSPEND */
+	suspend_set_ops(omap_pm_ops);
 
 err2:
 	return ret;

aaro.koskinen at nokia.com

author

2011-01-06 10:15 UTC #3 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Hi,

Kevin Hilman [khilman at ti.com]:

Quoted 13 lines

Quoted 4 lines · Aaro Koskinen
Make !CONFIG_SUSPEND init declarations identical on all OMAPs and eliminate some ifdefs.
Signed-off-by Aaro Koskinen <[email protected]>
I like this solution, but it introduces compiler warnings:
[...]
As you likely noticed, removing the const leads to checkpatch warnings:

Quoted 8 lines

WARNING: struct platform_suspend_ops should normally be const
so the choice is between a checkpatch warning or a bunch of compiler warnings.
Alternatively, I just posted a patch[1] to linux-pm propsing to fix this at the source. Let's see what happens there. Merging $SUBJECT patch will depend on how this is fixed upstream.

Sorry, I should have mentioned this when I posted the patch. I was aware of this issue, but I thought this was already fixed in upstream. Check the following commit in linux-next:

http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=2f55ac072f5344519348c0c94b3d2f4cca46847b

(suspend: constify platform_suspend_ops)

Kevin Hilman

2011-01-06 17:03 UTC #4 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Quoted 25 lines · <[email protected]> writes:

Hi,
Kevin Hilman [khilman at ti.com]:
Quoted 13 lines
Quoted 4 lines · Aaro Koskinen
Make !CONFIG_SUSPEND init declarations identical on all OMAPs and eliminate some ifdefs.
Signed-off-by Aaro Koskinen <[email protected]>
I like this solution, but it introduces compiler warnings:
[...]
As you likely noticed, removing the const leads to checkpatch warnings:
Quoted 8 lines
WARNING: struct platform_suspend_ops should normally be const
so the choice is between a checkpatch warning or a bunch of compiler warnings.
Alternatively, I just posted a patch[1] to linux-pm propsing to fix this at the source. Let's see what happens there. Merging $SUBJECT patch will depend on how this is fixed upstream.

Quoted 7 lines

Sorry, I should have mentioned this when I posted the patch. I was aware of this issue, but I thought this was already fixed in upstream. Check the following commit in linux-next:
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=2f55ac072f5344519348c0c94b3d2f4cca46847b
(suspend: constify platform_suspend_ops)

ah, thanks. somehow I missed that one when looking to see if this was already fixed.

Kevin

Kevin Hilman

2011-01-06 22:31 UTC #5 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Quoted 31 lines · Kevin Hilman

Quoted 29 lines · <[email protected]> writes:
Hi,
Kevin Hilman [khilman at ti.com]:
Quoted 13 lines
Quoted 4 lines · Aaro Koskinen
Make !CONFIG_SUSPEND init declarations identical on all OMAPs and eliminate some ifdefs.
Signed-off-by Aaro Koskinen <[email protected]>
I like this solution, but it introduces compiler warnings:
[...]
As you likely noticed, removing the const leads to checkpatch warnings:
Quoted 8 lines
WARNING: struct platform_suspend_ops should normally be const
so the choice is between a checkpatch warning or a bunch of compiler warnings.
Alternatively, I just posted a patch[1] to linux-pm propsing to fix this at the source. Let's see what happens there. Merging $SUBJECT patch will depend on how this is fixed upstream.
Sorry, I should have mentioned this when I posted the patch. I was aware of this issue, but I thought this was already fixed in upstream. Check the following commit in linux-next:

Quoted 6 lines

Quoted 3 lines
http://git.kernel.org/?p=linux/kernel/git/next/linux-next.git;a=commitdiff;h=2f55ac072f5344519348c0c94b3d2f4cca46847b
(suspend: constify platform_suspend_ops)
ah, thanks. somehow I missed that one when looking to see if this was already fixed.

OK, now your patch and the one from linux-next are conflicting as they both touch the platform_suspend_ops in pm*.c.

So for now, I'm gonna drop this one, but it should be rebased/reposted as soon as 2.6.38-rc1 is out.

Thanks,

Kevin

Russell King - ARM Linux

2011-01-06 11:19 UTC #6 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

In reply to Kevin Hilman

Quoted 6 lines · Kevin Hilman

As you likely noticed, removing the const leads to checkpatch warnings:
WARNING: struct platform_suspend_ops should normally be const
so the choice is between a checkpatch warning or a bunch of compiler warnings.

checkpatch should not be checking that data declarations are const. That's a decision for those people declaring them to decide upon, as they may need to modify the structure before registration.

By doing so, we end up encouraging people to write more code - such as declaring it __initconst, and then kmalloc'ing, memcpy'ing and modifying that before registering it.

Quoted 17 lines

arch/arm/mach-omap2/pm24xx.c +7 -7

-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 */

Quoted 7 lines

arch/arm/mach-omap2/pm24xx.c static int __init omap2_pm_init(void) · line 585
Quoted 7 lines
/* XXX This function should be shareable between OMAP2xxx and OMAP3 */
arch/arm/mach-omap2/pm24xx.c +1 -1
@@ -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.

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.

aaro.koskinen at nokia.com

author

2011-01-06 12:05 UTC #7 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Hi,

Russell King - ARM Linux [linux at arm.linux.org.uk]:

Quoted 22 lines

arch/arm/mach-omap2/pm24xx.c +7 -7

-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);
    }

Quoted 5 lines

Quoted 2 lines
Review static int __init omap2_pm_init(void) · line 585
Quoted 2 lines
(quoted diff) +1 -1
-   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.

Quoted 16 lines

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
...

Quoted 4 lines

        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...

Russell King - ARM Linux

2011-01-06 13:58 UTC #8 lore raw

PATCH 1/1 arm: mach-omap2: pm: cleanup !CONFIG_SUSPEND handling

Quoted 25 lines · aaro.koskinen at nokia.com

Hi,

Russell King - ARM Linux [linux at arm.linux.org.uk]:

Quoted 22 lines

arch/arm/mach-omap2/pm24xx.c +7 -7

-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);
    }

Quoted 8 lines

Quoted 5 lines
Quoted 2 lines
Review static int __init omap2_pm_init(void) · line 585
Quoted 2 lines
(quoted diff) +1 -1
-   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.