From mboxrd@z Thu Jan 1 00:00:00 1970 From: Tony Lindgren Subject: Re: [PATCH 3/17] ARM: OMAP2: Add common register access for 24xx and 34xx Date: Wed, 9 Apr 2008 20:50:06 +0000 Message-ID: <20080409205005.GL31071@atomide.com> References: <1205848935-12078-1-git-send-email-tony@atomide.com> <1205848935-12078-2-git-send-email-tony@atomide.com> <1205848935-12078-3-git-send-email-tony@atomide.com> <1205848935-12078-4-git-send-email-tony@atomide.com> <20080407162049.GF5306@flint.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Return-path: Received: from mho-02-bos.mailhop.org ([63.208.196.179]:52735 "EHLO mho-02-bos.mailhop.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752457AbYDIUuP (ORCPT ); Wed, 9 Apr 2008 16:50:15 -0400 Content-Disposition: inline In-Reply-To: <20080407162049.GF5306@flint.arm.linux.org.uk> Sender: linux-omap-owner@vger.kernel.org List-Id: linux-omap@vger.kernel.org To: Russell King - ARM Linux Cc: linux-arm-kernel@lists.arm.linux.org.uk, linux-omap@vger.kernel.org Hi, Thanks for looking through this big set, I'll repost the series with your comments fixed. Few comments below on what was changed. * Russell King - ARM Linux [080407 16:21]: > On Tue, Mar 18, 2008 at 04:02:01PM +0200, Tony Lindgren wrote: > > diff --git a/arch/arm/mach-omap2/cm.h b/arch/arm/mach-omap2/cm.h > > new file mode 100644 > > index 0000000..f575a87 > > --- /dev/null > > +++ b/arch/arm/mach-omap2/cm.h > > @@ -0,0 +1,142 @@ > > + > > +#ifndef __ASSEMBLER__ > > +/* Clock management global register get/set */ > > + > > +static void __attribute__((unused)) cm_write_reg(u32 val, void __iomem *addr) > > +{ > > + pr_debug("cm_write_reg: writing 0x%0x to 0x%0x\n", val, (u32)addr); > > + > > + __raw_writel(val, addr); > > +} > > + > > +static u32 __attribute__((unused)) cm_read_reg(void __iomem *addr) > > +{ > > + return __raw_readl(addr); > > +} > > +#endif > > Erm, code in a header file? Make them __inline__ or if you really want > to have them as separate functions, put them in a .c file. > > Next point - what's the purpose of them. They're doing nothing (apart > from that excess debug statement). I've made them __inline__ and removed extra debug stuff from these functions. > > +#ifndef __ASSEMBLER__ > > +static void __attribute__((unused)) cm_write_mod_reg(u32 val, s16 module, > > + s16 idx) > > +{ > > + cm_write_reg(val, OMAP_CM_REGADDR(module, idx)); > > +} > > + > > +static u32 __attribute__((unused)) cm_read_mod_reg(s16 module, s16 idx) > > +{ > > + return cm_read_reg(OMAP_CM_REGADDR(module, idx)); > > +} > > +#endif > > Ditto. > > > diff --git a/arch/arm/mach-omap2/prcm.c b/arch/arm/mach-omap2/prcm.c > > index 90f5305..01ab3bc 100644 > > --- a/arch/arm/mach-omap2/prcm.c > > +++ b/arch/arm/mach-omap2/prcm.c > > @@ -16,6 +16,7 @@ > > #include > > #include > > #include > > +#include > > Always suspicious of changes which just _add_ includes. Is this actually > needed? Did it build before? I've moved this to a later patch that needs it. > > diff --git a/arch/arm/mach-omap2/prm.h b/arch/arm/mach-omap2/prm.h > > new file mode 100644 > > index 0000000..455b060 > > --- /dev/null > > +++ b/arch/arm/mach-omap2/prm.h > > @@ -0,0 +1,334 @@ > >.. > > +#ifndef __ASSEMBLER__ > > + > > +/* Power/reset management global register get/set */ > > + > > +static void __attribute__((unused)) prm_write_reg(u32 val, void __iomem *addr) > > +{ > > + pr_debug("prm_write_reg: writing 0x%0x to 0x%0x\n", val, (u32)addr); > > + > > + __raw_writel(val, addr); > > +} > > + > > +static u32 __attribute__((unused)) prm_read_reg(void __iomem *addr) > > +{ > > + return __raw_readl(addr); > > +} > > + > > +#endif > > Yet more code in includes. And this looks like a pointless abstraction. > > > +#ifndef __ASSEMBLER__ > > + > > +/* Power/reset management domain register get/set */ > > + > > +static void __attribute__((unused)) prm_write_mod_reg(u32 val, s16 module, s16 idx) > > +{ > > + prm_write_reg(val, OMAP_PRM_REGADDR(module, idx)); > > +} > > + > > +static u32 __attribute__((unused)) prm_read_mod_reg(s16 module, s16 idx) > > +{ > > + return prm_read_reg(OMAP_PRM_REGADDR(module, idx)); > > +} > > + > > +#endif > > And more. > > > diff --git a/arch/arm/mach-omap2/sdrc.h b/arch/arm/mach-omap2/sdrc.h > > new file mode 100644 > > index 0000000..928e1c4 > > --- /dev/null > > +++ b/arch/arm/mach-omap2/sdrc.h > >... > > +#ifndef __ASSEMBLER__ > > +extern unsigned long omap2_sdrc_base; > > +extern unsigned long omap2_sms_base; > > + > > +#define OMAP_SDRC_REGADDR(reg) \ > > + (void __iomem *)IO_ADDRESS(omap2_sdrc_base + (reg)) > > +#define OMAP_SMS_REGADDR(reg) \ > > + (void __iomem *)IO_ADDRESS(omap2_sms_base + (reg)) > > + > > +/* SDRC global register get/set */ > > + > > +static void __attribute__((unused)) sdrc_write_reg(u32 val, u16 reg) > > +{ > > + pr_debug("sdrc_write_reg: writing 0x%0x to 0x%0x\n", val, > > + (u32)OMAP_SDRC_REGADDR(reg)); > > + > > + __raw_writel(val, OMAP_SDRC_REGADDR(reg)); > > +} > > + > > +static u32 __attribute__((unused)) sdrc_read_reg(u16 reg) > > +{ > > + return __raw_readl(OMAP_SDRC_REGADDR(reg)); > > +} > > + > > +/* SMS global register get/set */ > > + > > +static void __attribute__((unused)) sms_write_reg(u32 val, u16 reg) > > +{ > > + pr_debug("sms_write_reg: writing 0x%0x to 0x%0x\n", val, > > + (u32)OMAP_SMS_REGADDR(reg)); > > + > > + __raw_writel(val, OMAP_SMS_REGADDR(reg)); > > +} > > + > > +static u32 __attribute__((unused)) sms_read_reg(u16 reg) > > +{ > > + return __raw_readl(OMAP_SMS_REGADDR(reg)); > > +} > > And more. > > So that's a NAK.