[RESEND PATCH v5 4/7] usb: chipidea: consolidate ci_role_driver's API for both roles

[Peter Chen]

On Thu, Jan 24, 2013 at 04:35:30PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen at freescale.com> writes:
> 
> > - Create init/destroy API for probe and remove
> > - start/stop API are only used otg id switch process
> > - Create the gadget at ci_hdrc_probe if the gadget is supported
> > at that port, the main purpose for this is to avoid gadget module
> > load fail at init.rc
> 
> I don't think it's necessary to mention android-specific init scripts
> here in our patches.

Ok, will just mention init script.
> 
> >  
> > +static inline void ci_role_destroy(struct ci13xxx *ci)
> > +{
> > +	enum ci_role role = ci->role;
> > +
> > +	if (role == CI_ROLE_END)
> > +		return;
> > +
> > +	ci->role = CI_ROLE_END;
> > +
> > +	ci->roles[role]->destroy(ci);
> > +}
> 
> What does this do? It should take role an a parameter, at least. Or it
> can be called ci_roles_destroy() and, well, destroy all the roles. Now
> we're in a situation where we destroy one.
Yes, this function has some problems, I suggest just call two lines at
ci_role_destory, do you think so?

	ci->roles[role]->destroy(ci);
	ci->role = CI_ROLE_END;
> 
> The idea is that, with this api, we can (and should) have both roles
> allocated all the time, but only one role start()'ed.
> 
> What we can do here is move the udc's .init() code to
> ci_hdrc_gadget_init() and add a complementary ci_hdrc_gadget_destroy(),
> which we call in ci_hdrc_remove() and probe's error path. And we can
> probably do something similar for the host, or just leave it as it is
> now. Seems simpler to me.
Yes, current code has bug that it should call ci_role_destroy at probe's
error path.
For your comments, it still needs to add destory function for udc which will
be called at core.c. I still prefer current way due to below reasons:

- It can keep ci_hdrc_gadget_init() clean
- .init and .remove are different logical function with .start and .stop.
The .init and .remove are used for create and destroy, .start and .stop
are used at id role switch.

> >  }
> > diff --git a/drivers/usb/chipidea/host.c b/drivers/usb/chipidea/host.c
> > index caecad9..6024a4f 100644
> > --- a/drivers/usb/chipidea/host.c
> > +++ b/drivers/usb/chipidea/host.c
> > @@ -92,8 +92,10 @@ int ci_hdrc_host_init(struct ci13xxx *ci)
> >  	if (!rdrv)
> >  		return -ENOMEM;
> >  
> > +	rdrv->init	= host_start;
> >  	rdrv->start	= host_start;
> >  	rdrv->stop	= host_stop;
> > +	rdrv->destroy	= host_stop;
> 
> Will this allocate the hcd twice if we start in host role? Looks like that.
No, if we start host role, the host will be allocated once at probe.
host_start is only called when the id value from 1 to 0.
> 

-- 

Best Regards,
Peter Chen