[PATCH 01/17] omap4: pm: Add omap WakeupGen module support

-----Original Message-----
From: Kevin Hilman [mailto:khilman at ti.com]
Sent: Thursday, March 03, 2011 3:17 AM
To: Santosh Shilimkar
Cc: linux-omap at vger.kernel.org; linux-arm-kernel at lists.infradead.org
Subject: Re: [PATCH 01/17] omap4: pm: Add omap WakeupGen module
support

[...]

quoted

+
+static inline u32 cpu_readl(u8 idx, u32 cpu)

Minor nit: the cpu_ prefix is too generic, how about wakeupgen_ or
wugen_?

Done. I used wakeupgen_ prefeix.

[...]

quoted

+static void _wakeupgen_set_all(unsigned int cpu, unsigned int

reg)

quoted

+{
+	u8 i;
+
+	for (i = 0; i < NR_BANKS; i++)
+		cpu_writel(reg, i, cpu);
+}
+
+static void _wakeupgen_set(unsigned int irq, unsigned int set)

I (still) don't like a single "set" function, especially when it
takes a
"set" argument.

Make separate set and clear functions, with the common stuff in a
helper function like _wakeupgen_get_irq() or something.

Done.

[...]

quoted

+
+#ifdef CONFIG_PM

I think this should be CONFIG_SUSPEND

I don't see any body using "CONFIG_SUSPEND".
set_wake() seems to be under CONFIG_PM, so I
am retaining as it is.

quoted

+/*
+ * Architecture specific set_wake extension
+ */
+static int wakeupgen_set_wake(struct irq_data *d, unsigned int

on)

quoted

+	/* Static mapping, never released */
+	wakeupgen_base = ioremap(OMAP44XX_WKUPGEN_BASE, SZ_4K);
+	BUG_ON(!wakeupgen_base);

A WARN_ON() with a graceful exit is more appropriate here:

Done.

Regards
Santosh