On Thu, Feb 23, 2012 at 01:34:26PM -0800, Paul Zimmerman wrote: > > From: Felipe Balbi [mailto:balbi@xxxxxx] > > Sent: Thursday, February 23, 2012 11:43 AM > > > > > Would you rather I just put all the suspend/resume code into gadget.c > > > and core.c? I really wanted to avoid that, but if you're dead set on > > > keeping all the routines in gadget.c static, then I don't see any > > > other way. > > > > the thing is that you expose too much detail about the implementation, > > where you could make the suspend/resume code a lot simpler by hiding a > > few details. > > > > I generally don't to expose internal APIs to any other user (I'm > > considering core.c as a user) because then core.c would need to know too > > much about the Gadget API. But the fact is that core.c only needs those > > because of some PM features, so instead of exposing implementation > > details to core.c, I would rater see you implement: > > > > int dwc3_gadget_runtime_suspend(struct dwc3 *dwc) > > { > > /* > > * save context > > * go into hibernation > > * blablabla > > */ > > > > return 0; > > } > > > > void dwc3_gadget_runtime_resume(struct dwc3 *dwc) > > { > > /* > > * get out of hibernation > > * restore context > > */ > > > > ret = __dwc3_gadget_start(); > > WARN(ret, "failed to restart gadget\n"); > > > > dwc3_gadget_run_stop(dwc, true); > > } > > > > and use those on core.c's runtime_suspend/runtime_resume respectively. > > That way we hide details about the gadget API implementation, core makes > > no assumptions about gadget.c and if we decide to change the > > implementation of any of those functions, core doesn't even need to know > > as long as the outcome is kept the same. > > > > Of course, the event buffers setup will be on core.c: > > > > static int dwc3_runtime_suspend(struct device *dev) > > { > > struct dwc3 *dwc = dev_get_drvdata(dev); > > > > /* blablabla */ > > > > return 0; > > { > > > > static void dwc3_runtime_resume(struct device *dev) > > { > > struct dwc3 *dev = dev_get_drvdata(dev); > > unsigned mode; > > > > dwc3_event_buffers_setup(dwc); > > > > mode = DWC3_MODE(dwc->hwparams.hwparams0); > > > > switch (mode) { > > case DWC3_MODE_DEVICE: > > dwc3_gadget_runtime_resume(dwc3); > > break; > > case DWC3_MODE_HOST: > > dwc3_host_runtime_resume(dwc3); /* ?? */ > > break; > > case DWC3_MODE_DRD: > > dwc3_gadget_runtime_resume(dwc3); > > dwc3_host_runtime_resume(dwc3); /* ?? */ > > break; > > } > > } > > > > or something similar, but core.c need not know implementation details > > about device or host and that's why I don't want to expose those > > functions. > > OK, I think that means all the hibernation code needs to go into > gadget.c and core.c. Do you mind if I put #ifdef's around the code > if CONFIG_USB_DWC3_HIBERNATION is not defined? That way people who > don't have a core with hibernation support don't need to take the > hit of all that extra unneeded code. sure, go ahead :-) -- balbi
Attachment:
signature.asc
Description: Digital signature
