From mboxrd@z Thu Jan 1 00:00:00 1970 From: Timur Tabi Date: Fri, 21 May 2010 14:58:23 -0500 Subject: [U-Boot] [PATCH] Move ICS CLK chip frequenty calculation code into a common board library In-Reply-To: <20100521195233.A5156CCF026@gemini.denx.de> References: <1274433495-31885-1-git-send-email-galak@kernel.crashing.org> <20100521195233.A5156CCF026@gemini.denx.de> Message-ID: <4BF6E5DF.5020306@freescale.com> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de 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