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

[Jean-Christophe PLAGNIOL-VILLARD]

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.