From mboxrd@z Thu Jan  1 00:00:00 1970
From: Tom <Tom.Rix@windriver.com>
Date: Sun, 06 Sep 2009 08:46:35 -0500
Subject: [U-Boot] [PATCH 3/8] TWL4030 Add usb PHY support
In-Reply-To: <20090905000256.GZ30118@game.jcrosoft.org>
References: <1252095170-5492-1-git-send-email-Tom.Rix@windriver.com>
	<1252095170-5492-2-git-send-email-Tom.Rix@windriver.com>
	<1252095170-5492-3-git-send-email-Tom.Rix@windriver.com>
	<20090905000256.GZ30118@game.jcrosoft.org>
Message-ID: <4AA3BD3B.2040009@windriver.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

Jean-Christophe PLAGNIOL-VILLARD wrote:
> On 15:12 Fri 04 Sep     , Tom Rix wrote:
>   
<snip>
>>     
> please add an empty line
>   
I will take care of all the empty lines.

>> +	if (ret == 0)
>> +		ret = data;
>> +	else
>> +		printf("TWL4030:USB:Read[0x%x] Error %d\n", address, ret);
>> +
>> +	return ret;
>>     
> why not this and avoid the copy of data
> 	if (ret != 0) {
> 		printf("TWL4030:USB:Read[0x%x] Error %d\n", address, ret);
> 		return ret;
> 	}
>
> 	return data;
> }
>
>   
In general I rather have only one exit point from a function.
This makes tracing execution easier.

>> +	/* Check if the PHY DPLL is locked */
>> +	sts = twl4030_usb_read(TWL4030_USB_PHY_CLK_CTRL_STS);
>> +	while (!(sts & PHY_DPLL_CLK) && 0 < timeout) {
>> +		udelay(10);
>> +		sts = twl4030_usb_read(TWL4030_USB_PHY_CLK_CTRL_STS);
>> +		timeout -= 10;
>> +	}
>>     
> why not set time to 100 * 1000
> 	and just decrease by 1
>   
Yes.

> make some enums by group of function will be better as it simplify the code
>   
>> +#define TWL4030_USB_VENDOR_ID_LO			0x00
>> +#define TWL4030_USB_VENDOR_ID_HI			0x01
>>     
I see your point, but this is how they are defined in a twl4030 spec.
This way makes it easy to look up #define in the spec.
Just remove the prefix.
>> +#define TWL4030_USB_PRODUCT_ID_LO			0x02
>> +#define TWL4030_USB_PRODUCT_ID_HI			0x03
>>     
Tom
>
> Best Regards,
> J.
>