On Thu, Jan 20, 2011 at 04:28:11PM +0530, Premi, Sanjeev wrote:
> > -----Original Message-----
> > From: Varadarajan, Charulatha
> > Sent: Tuesday, January 18, 2011 2:44 PM
> > To: Premi, Sanjeev
> > Cc: linux-omap@xxxxxxxxxxxxxxx
> > Subject: Re: [PATCH] omap3: Add basic support for 720MHz part
> >
> > Couple of minor comments.
>
> [snip]...[snip]
>
> > > --- a/arch/arm/mach-omap2/control.h
> > > +++ b/arch/arm/mach-omap2/control.h
> > > @@ -365,6 +365,13 @@
> > > #define FEAT_NEON 0
> > > #define FEAT_NEON_NONE 1
> > >
> > > +/*
> > > + * Product ID register
> > > + */
> >
> > Do not use multiline comment style for single line comments
>
> [sp] Was only following the multi-line comment stye few lines
> above...
>
> >
> > > +#define OMAP3_PRODID 0x020C
> > > +
> > > +#define OMAP3_SKUID_MASK 0x0f
> > > +#define OMAP3_SKUID_720MHZ 0x08
> > >
> > > #ifndef __ASSEMBLY__
> > > #ifdef CONFIG_ARCH_OMAP2PLUS
> > > diff --git a/arch/arm/mach-omap2/id.c b/arch/arm/mach-omap2/id.c
> > > index 5f9086c..53fbe01 100644
> > > --- a/arch/arm/mach-omap2/id.c
> > > +++ b/arch/arm/mach-omap2/id.c
> > > @@ -195,6 +195,15 @@ static void __init omap3_check_features(void)
> > > * TODO: Get additional info (where applicable)
> > > * e.g. Size of L2 cache.
> > > */
> > > +
> > > + /*
> > > + * Does it support 720MHz?
> > > + */
> >
> > Ditto
>
> [sp] Same here.
> But since the code is quite evident, I will remove the comment
> altogether.
>
> >
> > > + status = (OMAP3_SKUID_MASK & read_tap_reg(OMAP3_PRODID));
> > > +
> > > + if (status & OMAP3_SKUID_720MHZ) {
> > > + omap3_features |= OMAP3_HAS_720MHZ;
> > > + }
> >
> > No need of { } as there is only one line statement inside the
> > if condition.
>
> [sp] missed this.
Looks fine. Feel free to add after the above changes:
Reviewed-by: G, Manjunath Kondaiah <manjugk@xxxxxx>
-Manjunath
--
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
