Hi, On Thu, Feb 21, 2013 at 03:34:56AM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Wednesday, February 20, 2013 12:50 AM > > > > On Tue, Feb 19, 2013 at 06:50:07PM -0800, Paul Zimmerman wrote: > > > This file contains the PCI bus interface "glue" for the DWC2 > > > driver > > > > > > Signed-off-by: Paul Zimmerman <paulz@xxxxxxxxxxxx> > > > > before anything, out of curiosity, do you have DWC2 PCI configured with > > OTG support ? I remember that on HAPS6x configuring DWC3 PCI OTG would > > take too much space on the FPGA and that would cause difficulties on > > timing closure, but perhaps dwc2 is smaller ?!? > > Yes, our standard HSOTG FPGA image is configured with OTG support. I'm > not sure how difficult it is to synthesize, that work is done by a team on > another continent ;) cool, guess USB2 is far smaller IP than USB3 :-) > > > +static void dwc2_driver_remove(struct pci_dev *dev) > > > +{ > > > + struct dwc2_device *otg_dev = pci_get_drvdata(dev); > > > + > > > + dev_dbg(&dev->dev, "%s(%p)\n", __func__, dev); > > > + > > > + if (!otg_dev) { > > > + dev_dbg(&dev->dev, "%s: otg_dev NULL!\n", __func__); > > > + return; > > > + } > > > > this check can be removed, drivers core guarantees that pci_dev *dev > > will always be true and, since you clearly and unconditionally call > > pci_set_drvdata(), otg_dev will always be valid. > > This function is called as part of the failure path from dwc2_driver_probe(), > and it's possible that pci_set_drvdata() has not been called yet. that means you're calling it at the wrong time ;-) > > > + dev_dbg(&dev->dev, "otg_dev->hsotg = %p\n", otg_dev->hsotg); > > > + if (otg_dev->hsotg) > > > + dwc2_hcd_remove(&dev->dev, otg_dev); > > > + else > > > + dev_dbg(&dev->dev, "%s: otg_dev->hsotg NULL!\n", __func__); > > > > this check can be removed I guess. You shouldn't be able to remove what > > wasn't registered. > > Ditto. same as above. If both checks fail, this function does nothing. Seems like you're calling it at the wrong location. > > > + /* Map the DWC_otg Core memory into virtual address space */ > > > + dev->current_state = PCI_D0; > > > + dev->dev.power.power_state = PMSG_ON; > > > > no no no... you shouldn't access these directly. > > > > Plese use pci_set_state(dev, PCI_D0) and > > dev_pm_qos_constraints_init(&dev->dev); > > I will switch to pci_set_power_state(dev, PCI_D0), I guess that's what > you meant? I can't get dev_pm_qos_constraints_init() to compile, so > I think I'll leave that out for now. I don't have any PM support in the > driver yet, anyway. cool, then leave out PMSG_ON too ;-) > For the rest of your comments I will implement the changes that > you suggested. Thanks. thanks -- balbi
Attachment:
signature.asc
Description: Digital signature
