From mboxrd@z Thu Jan  1 00:00:00 1970
From: Marek Vasut <marek.vasut@gmail.com>
Date: Thu, 22 Sep 2011 14:29:32 +0200
Subject: [U-Boot] [PATCH 2/2] EfikaSB: Add preliminary EfikaSB support
In-Reply-To: <4E7B0361.8000309@denx.de>
References: <1316428941-8957-1-git-send-email-marek.vasut@gmail.com>
	<1316428941-8957-3-git-send-email-marek.vasut@gmail.com>
	<4E7B0361.8000309@denx.de>
Message-ID: <201109221429.32951.marek.vasut@gmail.com>
List-Id: <u-boot.lists.denx.de>
MIME-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
To: u-boot@lists.denx.de

On Thursday, September 22, 2011 11:44:01 AM Stefano Babic wrote:
> On 09/19/2011 12:42 PM, Marek Vasut wrote:
> > Signed-off-by: Marek Vasut <marek.vasut@gmail.com>
> > Cc: Stefano Babic <sbabic@denx.de>
> > ---
> 
> Hi Marek,
> 
> sorry for late review. I missed this patch...
> 
> Only a couple of minor issues:
> > +#if !defined(CONFIG_MACH_EFIKAMX) && !defined(CONFIG_MACH_EFIKASB)
> > +#error "Missing CONFIG_MACH_EFIKAMX or CONFIG_MACH_EFIKASB in config.h!"
> > +#endif
> 
> This seems not necessary because CONFIG_MACH_EFIKA* is set at the build
> time with the option in boards.cfg. With a correct boards.cfg, we cannot
> get this error.

Well once someone adds another efika, he can forget about it. And there's mx53 
efika in the works.

> 
> > @@ -284,9 +304,9 @@ int board_mmc_init(bd_t *bis)
> > 
> >  	int ret;
> >  	
> >  	/* SDHC1 is used on all revisions, setup control pins first */
> > 
> > -	mxc_request_iomux(MX51_PIN_GPIO1_0,
> > +	mxc_request_iomux(EFIKA_SD1_CD,
> > 
> >  		IOMUX_CONFIG_ALT0 | IOMUX_CONFIG_SION);
> > 
> > -	mxc_iomux_set_pad(MX51_PIN_GPIO1_0,
> > +	mxc_iomux_set_pad(EFIKA_SD1_CD,
> > 
> >  		PAD_CTL_DRV_HIGH | PAD_CTL_HYS_ENABLE |
> >  		PAD_CTL_PUE_KEEPER | PAD_CTL_100K_PU |
> >  		PAD_CTL_ODE_OPENDRAIN_NONE |
> > 
> > @@ -298,11 +318,13 @@ int board_mmc_init(bd_t *bis)
> > 
> >  		PAD_CTL_100K_PU | PAD_CTL_ODE_OPENDRAIN_NONE |
> >  		PAD_CTL_SRE_FAST);
> > 
> > -	gpio_direction_input(IOMUX_TO_GPIO(MX51_PIN_GPIO1_0));
> > +	gpio_direction_input(IOMUX_TO_GPIO(EFIKA_SD1_CD));
> > 
> >  	gpio_direction_input(IOMUX_TO_GPIO(MX51_PIN_GPIO1_1));
> > 
> > +#ifndef	CONFIG_MACH_EFIKASB
> 
> It is better to have the check consistent in the file. You mix #ifdef
> CONFIG_MACH_EFIKAMX  with #ifndef CONFIG_MACH_EFIKASB, that is the same.

It expresses the intention much better IMO. And see above -- mx53 efika in the 
works.

> 
> >  	/* Internal SDHC1 IOMUX + SDHC2 IOMUX on old boards */
> >  	if (get_efika_rev() < EFIKAMX_BOARD_REV_12) {
> > 
> > +#endif
> 
> At the moment, the #ifdef seems redundant. You hard-code the efikasb
> revision to zero, and then get_efika_rev() is always smaller as
> EFIKAMX_BOARD_REV_12. What about to introduce a macro such as board_is()
> to increase readability ?

Yes it would, but it'd also increase code size.

> 
> This if statement really means:
> 
> if (board_is(EFIKASB) || (board_is(EFIKAMX) &&
>   get_efika_rev() < EFIKAMX_BOARD_REV_12))
> 
> >  /*
> >  
> >   * Board initialization
> > 
> > @@ -616,7 +661,11 @@ int board_early_init_f(void)
> > 
> >  int board_init(void)
> >  {
> > 
> > +#ifdef	CONFIG_MACH_EFIKAMX
> > 
> >  	gd->bd->bi_arch_number = MACH_TYPE_MX51_EFIKAMX;
> > 
> > +#else
> > +	gd->bd->bi_arch_number = MACH_TYPE_MX51_EFIKASB;
> > +#endif
> > 
> >  	gd->bd->bi_boot_params = PHYS_SDRAM_1 + 0x100;
> 
> Can we use the new rule to set up the MACH-ID ? You can move the #ifdef
> inside config.h and let common code to set it.

Can we do that in a subsequent patch ?
> 
> Best regards,
> Stefano Babic