[U-Boot] [PATCH V2 1/1] IMX: rename mx51 to mx5

[Liu Hui-R64343]

Hi, Wolfgang, 

> -----Original Message-----
> From: Wolfgang Denk [mailto:wd at denx.de]
> Sent: Saturday, October 16, 2010 3:08 AM
> To: Liu Hui-R64343
> Cc: sbabic at denx.de; u-boot at lists.denx.de
> Subject: Re: [U-Boot] [PATCH V2 1/1] IMX: rename mx51 to mx5
> 
> Dear Jason Liu,
> 
> In message <1287155158-16801-1-git-send-email-r64343 at freescale.com>
you
> wrote:
> ...
> > -	freq = decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_MX51_HCLK_FREQ);
> > +	freq = decode_pll(mxc_plls[PLL1_CLOCK], CONFIG_HCLK_FREQ);
> ...
> > -	clockinfo,	CONFIG_SYS_MAXARGS,	1,
do_mx51_showclocks,
> > -	"display mx51 clocks\n",
> > +	clockinfo,	CONFIG_SYS_MAXARGS,	1,	do_showclocks,
> > +	"display clocks\n",
> ...
> > -	lastinc = val / (CONFIG_MX51_CLK32 / CONFIG_SYS_HZ);
> > +	lastinc = val / (CONFIG_CLK32 / CONFIG_SYS_HZ);
> 
> I'm not happy about these changes.
> 
> The old names, containing "MX51" may have been too narrow in their
> naming; but the new names are now way too general.
> 
> "CONFIG_CLK32" or "CONFIG_HCLK_FREQ" look like generic config options
> that apply to any processor or board, or at least potentially to a
> wider range.  This is kind of name space pollution.
> 
> I understand that all these names are specific to the mx5 family of
> processors only, right?

Yes, specific to mx5 family,

> 
> Recent submissions to the Linux kernel seem to use a "MX5" string when
> referring to these processors, like in:
> 
> - commit messages:
> 
> 	ARM: mx5: Add Nand clock support
> 	ARM: mx5: dynamically register mxc-nand device
> 	ARM: mx5: dynamically register imx-i2c devices
> 
> - directory names:
> 
> 	arch/arm/mach-mx5/
> 
> - preprocessor variables:
> 
> 	MX5_USBOTHER_REGS_OFFSET
> 	MX5_USB_UTMI_PHYCTRL1_PLLDIV_MASK
> 
> etc.
> 
> I suggest we do the same. Please add a "mx5" resp. "MX5" to the new
> identifier names.

Yes, agree. 

> 
> Please also keep in mind that CONFIG_* options should be documented in
> the README.

Do you mean all the specific CONFIG_* options in
include/configs/boardname.h should be documented?
I found lots of that is not doing what you called for? :(

BTW, which definition is better?

CONFIG_SYS_MX5_HCLK_FREQ 24000000
CONFIG_MX5_HCLK_FREQ 24000000

According to the REAME, I prefer to the former, what do you think.


> 
> Thanks.
> 
> Best regards,
> 
> Wolfgang Denk
> 
> --
> DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
> HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
> Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
> "May your future be limited only by your dreams."
> - Christa McAuliffe