On Thu, 19 Mar 2015, Thomas Petazzoni wrote: > This commit adds new register values for the USB_CMD and USB_MODE > registers, which allows to avoid the usage of a number of magic values > in orion_usb_phy_v1_setup(). > > Signed-off-by: Thomas Petazzoni <thomas.petazzoni@xxxxxxxxxxxxxxxxxx> The new symbol values are fine. But (side issue) ... > @@ -69,8 +75,8 @@ static void orion_usb_phy_v1_setup(struct usb_hcd *hcd) > /* > * Reset controller > */ > - wrl(USB_CMD, rdl(USB_CMD) | 0x2); > - while (rdl(USB_CMD) & 0x2); > + wrl(USB_CMD, rdl(USB_CMD) | USB_CMD_RESET); > + while (rdl(USB_CMD) & USB_CMD_RESET); For one thing, this use of whitespace does not make the syntax clear. At the very least, it should be written as: while (rdl(USB_CMD) & USB_CMD_RESET) ; so that the reader can see this is a loop with an empty body. Right now it looks like an ordinary, non-looping statement. For another, have you considered what will happen if the hardware is defective and never turns off the USB_CMD_RESET bit? This sort of thing should always be implemented with some sort of timeout. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
