[U-Boot] [PATCH] Move ICS CLK chip frequenty calculation code into a common board library

Timur Tabi timur at freescale.com
Fri May 21 21:58:23 CEST 2010


Wolfgang Denk wrote:

>> So here's a better version of that function that rounds to the nearest
>> MHz and is of a proper coding style:
> 
> Why do we need that?

Um, because you complained about it?

>> > +static unsigned long ics307_clk_freq(unsigned char cw0, unsigned char cw1,
>> > +				     unsigned char cw2)
>> > +{
>> > +	const unsigned long InputFrequency = CONFIG_ICS307_REFCLK_HZ;
>> > +	unsigned long VDW = ((cw1 << 1) & 0x1FE) + ((cw2 >> 7) & 1);
>> > +	unsigned long RDW = cw2 & 0x7F;
>> > +	unsigned long OD = ics307_S_to_OD[cw0 & 0x7];
>> > +	unsigned long freq;

> Please do not use any CamelCase or UPPER CASE identifiers.
  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Also, because this is silly:

Clock Configuration:
       CPU0:799.992 MHz, CPU1:799.992 MHz,
       CCB:399.996 MHz,
       DDR:299.997 MHz (599.994 MT/s data rate) (Asynchronous), LBC:25   MHz

Why display 799.992 MHZ when 800 MHz makes more sense?

>> Clock Configuration:
>>        CPU0:800  MHz, CPU1:800  MHz,
>>        CCB:400  MHz,
>>        DDR:300  MHz (600 MT/s data rate) (Asynchronous), LBC:25   MHz
> 
> The result looks ugly (why do we have double spaces after the
> numbers?, why do the numbers not align vertically?).

> This makes me wonder why you use a "%-4s" format in
> arch/powerpc/cpu/mpc8?xx/cpu.c - may I recommend changing this into
> "%s" (if you don't care about vertical alignment), or something like
> "%4s" else?

I'm okay with that.

-- 
Timur Tabi
Linux kernel developer at Freescale


More information about the U-Boot mailing list