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

[santosh.shilimkar@ti.com (Santosh Shilimkar)]

> -----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
>
[...]
> > +
> > +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.

[...]

> > +static void _wakeupgen_set_all(unsigned int cpu, unsigned int
> reg)
> > +{
> > +	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.

[...]

> > +
> > +#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.
>
> > +/*
> > + * Architecture specific set_wake extension
> > + */
> > +static int wakeupgen_set_wake(struct irq_data *d, unsigned int
> on)


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