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
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
