From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stephen Warren <swarren@wwwdotorg.org>
Date: Thu, 17 Oct 2013 13:23:38 -0600
Subject: [U-Boot]
 =?utf-8?q?=5BPATCH_v2_2/2=5D_ARM=3A_tegra=3A_Add_the_Tam?=
 =?utf-8?q?onten=E2=84=A2_NG_Evaluation_Carrier_board?=
In-Reply-To: <1382020192-1747-3-git-send-email-alban.bedel@avionic-design.de>
References: <1382020192-1747-1-git-send-email-alban.bedel@avionic-design.de>
	<1382020192-1747-3-git-send-email-alban.bedel@avionic-design.de>
Message-ID: <5260393A.4000609@wwwdotorg.org>
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 10/17/2013 08:29 AM, Alban Bedel wrote:
> Add support for the new Tamonten? NG platform from Avionic Design.
> Currently only I2C, MMC, USB and ethernet have been tested.

> diff --git a/board/avionic-design/common/tamonten-ng.c b/board/avionic-design/common/tamonten-ng.c

> +void pmu_write(uchar reg, uchar data)
> +{
> +	int i;
> +	i2c_set_bus_num(0);	/* PMU is on bus 0 */
> +	for (i = 0; i < MAX_I2C_RETRY; ++i) {
> +		if (i2c_write(PMU_I2C_ADDRESS, reg, 1, &data, 1))
> +			udelay(100);

Why the need for retries here? This really shouldn't fail.

> +void power_init_board(void)
> +{
> +	/* Set the periphs in reset */
> +	gpio_request(GPIO_PI4, "nRST_PERIPH");
> +	gpio_direction_output(GPIO_PI4, 0);
> +
> +	/* Disable the PCIe power */
> +	gpio_request(GPIO_PT3, "EN_3V3_PEX_HVDD");
> +	gpio_direction_output(GPIO_PT3, 0);
> +
> +	/* Disable the SATA power */
> +	gpio_request(GPIO_PK3, "EN_3V3_SATA_HVDD");
> +	gpio_direction_output(GPIO_PK3, 0);
> +
> +	/* Disable LDO1 for PCIe and SATA */
> +	pmu_write(PMU_REG_LDO1, PMU_LDO1(OFF, 1050));
> +
> +	/* Turn on the alive signal */
> +	gpio_request(GPIO_PV2, "ALIVE");
> +	gpio_direction_output(GPIO_PV2, 1);
> +
> +	/* Wait for the periph to finish their reset */
> +	udelay(1000);
> +	gpio_direction_output(GPIO_PI4, 1);
> +}

Aren't all these the HW defaults? Even if not, I'm not sure there's a
need to explicitly turn everything off at boot; why not do this in the
individual drivers?

Aside from that, this patch looks fine.