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

Tom
2009-09-06T13:46:35Z

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