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.