From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefano Babic <sbabic@denx.de>
Date: Wed, 23 Nov 2011 15:22:09 +0100
Subject: [U-Boot] [PATCH 2/2] ARM: omap3: add support to Technexion
 twister board
In-Reply-To: <4ECCF970.6040605@compulab.co.il>
References: <1322040416-11751-1-git-send-email-sbabic@denx.de>
	<1322040416-11751-3-git-send-email-sbabic@denx.de>
	<4ECCF970.6040605@compulab.co.il>
Message-ID: <4ECD0191.4050208@denx.de>
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 23/11/2011 14:47, Igor Grinberg wrote:
>> +# You should have received a copy of the GNU General Public License
>> +# along with this program; if not, write to the Free Software
>> +# Foundation, Inc., 675 Mass Ave, Cambridge, MA 02139, USA.
> 
> Postal address is subject to change, I'd prefer to not have it
> inside the source code files.

Agree, I will remove it. I think the issue was already raised, I admit I
have not yet completely the thread.

>> +clean:
>> +	rm -f $(OBJS)
>> +
>> +distclean:	clean
>> +	rm -f $(LIB) core *.bak $(obj).depend
> 
> clean and distclean should be removed.

Right, I have copied from a worng file.

>> +
>> +	gpio_request(CONFIG_OMAP_EHCI_PHY1_RESET, "USB_PHY1_RESET");
>> +	gpio_direction_output(CONFIG_OMAP_EHCI_PHY1_RESET, 1);
> 
> CONFIG_OMAP_EHCI_PHY1_RESET_GPIO

Ok

>> +/*
>> + * Routine: misc_init_r
>> + * Description: Init i2c, ethernet, etc... (done here so udelay works)
>> + */
> 
> Comment is wrong.

Completely, it makes no sense - I will drop it

>> +#endif
>> +#if defined(CONFIG_SMC911X)
>> +	/* init cs for extern lan */
>> +	writel(NET_GPMC_CONFIG1, &gpmc_cfg->cs[5].config1);
>> +	writel(NET_GPMC_CONFIG2, &gpmc_cfg->cs[5].config2);
>> +	writel(NET_GPMC_CONFIG3, &gpmc_cfg->cs[5].config3);
>> +	writel(NET_GPMC_CONFIG4, &gpmc_cfg->cs[5].config4);
>> +	writel(NET_GPMC_CONFIG5, &gpmc_cfg->cs[5].config5);
>> +	writel(NET_GPMC_CONFIG6, &gpmc_cfg->cs[5].config6);
>> +	writel(NET_GPMC_CONFIG7, &gpmc_cfg->cs[5].config7);
> 
> There is an enable_gpmc_cs_config() function in
> /arch/arm/cpu/armv7/omap3/mem.c
> can't it be used?

Yes, now that I know there is such as function - thanks !

>> +	MUX_VAL(CP(D2D_MBUSFLAG),	(IEN  | PTD | DIS | M0)) \
>> +	MUX_VAL(CP(D2D_SBUSFLAG),	(IEN  | PTD | DIS | M0)) \
> 
> You have spaces for indentation in multiple places above.
> Please, check.

Ok, I will check it.

>> +#define CONFIG_AUTO_COMPLETE
> 
> This one is present in your common file.

I drop from here, I will let it in the common file.

> 
>> +
>> +/*
>> + * Miscellaneous configurable options
>> + */
>> +#define V_PROMPT			"twister => "
>> +#define CONFIG_SYS_PROMPT		V_PROMPT
> 
> Can this also be inlined?

Yes, of course.

Best regards,
Stefano Babic

-- 
=====================================================================
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: +49-8142-66989-0 Fax: +49-8142-66989-80  Email: office at denx.de
=====================================================================