Hi, On Tue, Feb 12, 2013 at 12:01:28AM +0000, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Monday, February 11, 2013 5:43 AM > > Hi Felipe, > > I'm only responding to the things I want to clarify here, the stuff that > I don't respond to you can consider as "OK, I'll do that". > > > On Sat, Feb 09, 2013 at 07:37:48PM -0800, Paul Zimmerman wrote: > > > +static void dwc2_core_reset(struct dwc2_hcd *hcd) > > > > not sure if I would call this an HCD. Surely it can _be_ an HCD, but > > it's actually a DRD device (not all configurations, but still). > > > > If you call this HCD, it will be a little misleading when peripheral > > mode is merged here IMO. How about: > > > > sed -i 's/dwc2_hcd\s\*\(\w\+\)/dwc2 *dwc/g' ? > > That sed expression doesn't seem to do anything here. Mind > explaining what you meant in English? rename struct dwc2_hcd to struct dwc and *hcd to *dwc. > I see s3c-hsotg uses 'struct s3c_hsotg'. How about if I use > 'struct dwc2_hsotg'? Then when we move that driver here we sure that would be alright, makes sense. > > > +static void dwc2_phy_init(struct dwc2_hcd *hcd) > > > +{ > > > + u32 usbcfg, i2cctl, hs_phy_type, fs_phy_type; > > > + > > > + if (hcd->core_params->speed == DWC2_SPEED_PARAM_FULL && > > > + hcd->core_params->phy_type == DWC2_PHY_TYPE_PARAM_FS) { > > > + /* If FS mode with FS PHY */ > > > + /* > > > + * core_init() is now called on every switch so only call the > > > + * following for the first time through > > > + */ > > > + if (!hcd->phy_init_done) { > > > + hcd->phy_init_done = 1; > > > > I wonder if this flag is really necessary. Seems like you can easily > > make sure that this doesn't get called twice. > > Well, according to the comment, it gets called on every role switch, > so I don't see how. right, but does it have to ? Wouldn't it be enough to initialize PHYs during probe() ? > > > + /* Reset after setting the PHY parameters */ > > > + dwc2_core_reset(hcd); > > > > since this is called on both branches, I guess you could factor it out > > of the branches, no ? > > Not really. In one case things are done after the reset, in the other > not. I see, my bad. > > not really needed right now, but you should start considering dma engine > > support. > > I thought that was only for generic DMA engines? This one is only > used in the HSOTG controller. right, but you can also have external DMAs. But, as I said, we can tackle that headache later ;-) > > > +static void dwc2_gusbcfg_init(struct dwc2_hcd *hcd) > > > +{ > > > + u32 usbcfg; > > > + > > > + usbcfg = readl(hcd->regs + GUSBCFG); > > > + usbcfg &= ~(GUSBCFG_HNPCAP | GUSBCFG_SRPCAP); > > > + > > > + switch (hcd->hwcfg2 & GHWCFG2_OP_MODE_MASK) { > > > + case GHWCFG2_OP_MODE_HNP_SRP_CAPABLE: > > > + if (hcd->core_params->otg_cap == DWC2_CAP_PARAM_HNP_SRP_CAPABLE) > > > > I'm assuming core_params is passed via module parameter, if that's the > > case, then I'm not sure you need a module parameter. You could be using > > DT/platform_data since this is board/platform-specific and won't change > > in runtime. > > > > If you expose to a module parameter, then a not-so-careful user might > > start reporting bugs which don't exist just because he didn't know how > > to pass the proper module parameter. > > See my response to 0/5 about the FPGA development board. For a > production platform this would be set via DT or platform_data. you can't set those parameters via DT, but I see what you mean. > > > +/** > > > + * dwc2_core_init() - Initializes the DWC_otg controller registers and > > > + * prepares the core for device mode or host mode operation > > > + * > > > + * @hcd: Programming view of the DWC_otg controller > > > + */ > > > +int dwc2_core_init(struct dwc2_hcd *hcd) > > > > is this still being called by the PCI layer ? On your cover letter you > > said they're now two separate drivers, if that's the case, this should > > be static and called on this driver's probe(). > > The core is a separate module, not a separate driver. It doesn't have a > probe() function. > > > > + /* Clear the SRP success bit for FS-I2c */ > > > + hcd->srp_success = 0; > > > > wasn't 'hcd' allocated with kzalloc ? (btw, would be nice if you were > > using devm_* API all over this driver, where possible). > > This function is called on every role switch, not just at init time. So > kzalloc is not enough. fair enough. > I am using devm_* in the bus glue code. I don't think I can use it > here, because this is library code and not a driver? right. > > Also, this function seems to be dealing with PIO only, so how about > > appending the name with _pio to make that clear ? > > No, if you read the header comments you will see this is used in both > slave mode and buffer DMA mode. Descriptor DMA mode is handled > by a different function (dwc2_hc_start_transfer_ddma). > > I thnk dwc2_hc_start_transfer_slave_or_bufdma() is a little too > wordy. fair enough :-) > > > + /* Wait for at least 3 PHY Clocks */ > > > + udelay(1); > > > > could you use usleep_range() here ? > > This is a udelay, not a usleep, because of interrupt context. I don't > think there is a udelay_range(), is there? no there isn't. Should've followed the function calls better. > > > + /* > > > + * Need to schedule a work, as there are possible DELAY function calls. > > > + * Release lock before scheduling workq as it holds spinlock during > > > + * scheduling. > > > + */ > > > + spin_unlock(&hcd->lock); > > > + queue_work(hcd->wq_otg, &hcd->wf_otg); > > > > please don't, convert to devm_request_threaded_irq() and run your IRQ > > handler on a thread. > > > > Ideally, you should use hardirq solely to check if this device generated > > the IRQ (yes, I know dwc3 isn't doing that, I have plans to fix it but > > have other more important stuff to do first ;-) > > I would rather not do that at this point if you don't mind. It seems like > a pretty large change, and I would like to get a working driver into the > kernel fairly soon. your call, I guess... -- balbi
Attachment:
signature.asc
Description: Digital signature
