> 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. -- Paul -- To unsubscribe from this list: send the line "unsubscribe linux-usb" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
