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 <[email protected]>
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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
