[U-Boot] [PATCH 3/8] TWL4030 Add usb PHY support

[Tom]

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.
>