> -----Original Message----- > From: Kevin Hilman [mailto:khilman@xxxxxx] > Sent: Thursday, March 03, 2011 3:17 AM > To: Santosh Shilimkar > Cc: linux-omap@xxxxxxxxxxxxxxx; linux-arm-kernel@xxxxxxxxxxxxxxxxxxx > 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 -- 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
