Re: [PATCH 8/21] [POWERPC] EP405 boards support for arch/powerpc

[Benjamin Herrenschmidt <benh@kernel.crashing.org>]

On Sat, 2007-12-22 at 10:24 +1100, Stephen Rothwell wrote:

> > +++ linux-merge/arch/powerpc/boot/4xx.c	2007-12-21 14:19:46.000000000 +1100
> > @@ -179,13 +179,16 @@ void ibm40x_dbcr_reset(void)
> >  #define EMAC_RESET 0x20000000
> >  void ibm4xx_quiesce_eth(u32 *emac0, u32 *emac1)
> >  {
> > -	/* Quiesce the MAL and EMAC(s) since PIBS/OpenBIOS don't do this for us */
> > +	/* Quiesce the MAL and EMAC(s) since PIBS/OpenBIOS don't
> > +	 * do this for us
> > +	 */
> >  	if (emac0)
> >  		*emac0 = EMAC_RESET;
> >  	if (emac1)
> >  		*emac1 = EMAC_RESET;
> >  
> >  	mtdcr(DCRN_MAL0_CFG, MAL_RESET);
> > +	while (mfdcr(DCRN_MAL0_CFG) & MAL_RESET) {};
> 
> You need the {} or the ; but not both and please put them indented on the
> next line so that it is very clear that this is an empty loop.

Code was copy/pasted from somewhere else... we can always do a fixup
later.

> > +	if (cpc0_cr0 & 0x80) {
> > +		/* uart0 uses the external clock */
> > +		uart0 = ser_clk;
> > +	} else {
> > +		uart0 = cpu / udiv;
> > +	}
> 
> The braces aren't really needed. (Yes, I noticed that you just moved the
> code here)

Yup :-)

> > +	if (cpc0_cr0 & 0x40) {
> > +		/* uart1 uses the external clock */
> > +		uart1 = ser_clk;
> > +	} else {
> > +		uart1 = cpu / udiv;
> > +	}
> 
> And again.

I'd rather do cleanups separately. I won't send new patches until I'm
back from vacation so Josh, if you want to fix those little nits up,
feel free.

> > +++ linux-merge/arch/powerpc/platforms/40x/ep405.c	2007-12-21 14:19:46.000000000 +1100
> > +
> > +static struct of_device_id ep405_of_bus[] = {
> 
> __initdata (preferably) or const

Ben.