On Wed, Feb 09, 2011 at 07:42:33PM +0530, Anoop P A wrote:

quoted

quoted

+#ifdef CONFIG_MSP_HAS_DUAL_USB
+	gpio_direction_output(MSP_PIN_USB1_HOST_DEV, 1);
+#endif

Please don't put #defines in .c files.

You mean #ifdef ???

Yes, sorry.

quoted

quoted

+static int ehci_msp_suspend(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+	unsigned long flags;
+	int rc;
+
+	return 0;
+	rc = 0;
+
+	if (time_before(jiffies, ehci->next_statechange))
+		msleep(10);

Short sleep, why?

I am not very sure. Person who originally wrote this driver is
unreachable.Any potential issues??

Yes, suspend/resume time delays are not nice for some systems.  I would
verify that this really is necessary, and, as you are going to be the one
maintaining and responsible for the code, it would be good for you to
figure out exactly what it is doing, and why.

quoted

quoted

+static int ehci_msp_resume(struct device *dev)
+{
+	struct usb_hcd *hcd = dev_get_drvdata(dev);
+	struct ehci_hcd *ehci = hcd_to_ehci(hcd);
+
+
+	/* maybe restore FLADJ */

Don't you know?

Not really :)

Heh, you should.

quoted

quoted

+/* may be called without controller electrically present */
+/* may be called with controller, bus, and devices active */
+

What may be called?

may be usb_hcd_msp_remove :)

Then put it in the comment block for that function, not above it, with
an extra space between it, that just causes confusion.

thanks,

greg k-h