On 08:46 Sun 06 Sep , Tom wrote:
> 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.
ok
>
> >>+ /* 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
ok fine
Best Regards,
J.
_______________________________________________
U-Boot mailing list
U-Boot@lists.denx.de
http://lists.denx.de/mailman/listinfo/u-boot
