Re: [PATCH 6/7 v2] usb: dwc3: refactor some existing routines for hibernation support

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]


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


B and H Foto and Electronics Corp.

[Linux Media]     [Video for Linux]     [Linux Input]     [Linux Audio Users]     [Photo]     [Yosemite News]    [Yosemite Photos]    [Free Online Dating]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]     [More Archives]

Add to Google Powered by Linux