On Tue, Nov 20, 2012 at 10:12:30PM +0800, Peter Chen wrote:
> On Tue, Nov 20, 2012 at 02:08:18PM +0200, Alexander Shishkin wrote:
> > Peter Chen <peter.chen@xxxxxxxxxxxxx> writes:
> >
> > > Since we can't load/unload gadget module on the fly, we can't
> > > de-init gadget structure during the otg switch.
> > > We have to create gadget at the probe function no matter current
> > > role is the host or device, of cource, the gadget will not
> > > be created for host-only port.
> >
> > No, this is much worse than the previous version.
> >
> > >
> > > Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> > > ---
> > > drivers/usb/chipidea/ci.h | 10 +++++++---
> > > drivers/usb/chipidea/core.c | 7 ++++++-
> > > 2 files changed, 13 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/usb/chipidea/ci.h b/drivers/usb/chipidea/ci.h
> > > index d32f932..c704aa7 100644
> > > --- a/drivers/usb/chipidea/ci.h
> > > +++ b/drivers/usb/chipidea/ci.h
> > > @@ -188,9 +188,13 @@ static inline int ci_role_start(struct ci13xxx *ci, enum ci_role role)
> > > if (!ci->roles[role])
> > > return -ENXIO;
> > >
> > > - ret = ci->roles[role]->start(ci);
> > > - if (!ret)
> > > - ci->role = role;
> > > + /* Only host role is supported at this function*/
> > > + if (role == CI_ROLE_HOST) {
> > > + ret = ci->roles[role]->start(ci);
> > > + if (!ret)
> > > + ci->role = role;
> > > + }
> > > +
> > > return ret;
> > > }
> >
> > This is bad in a number of ways. What we need is add a "destroy" method
> > to the role and not add
> >
> > if (ci->role == CI_ROLE_HOST)
> > hackity_hackity();
> >
> > to ten different places in the driver. This is really bad, even worse
> > than the fact that with this patch you're returning an uninitialized
> > 'ret' in case of non-host, which (correct me if I'm wrong) should
> > explode straight away, which in turn makes me wonder how is this
> > patchset fully tested as it claims to be.
>
> Oh, my fault, maybe this uninitialized value is just 0 at my test.
> I will consider this problem more.
How about this:
/* prepare for switch to device */
static int udc_id_switch_for_device(struct ci13xxx *ci)
{
usb_gadget_vbus_disconnect(&ci->gadget);
/* host doesn't care B_SESSION_VALID event */
hw_write(ci, OP_OTGSC, OTGSC_BSVIE, ~OTGSC_BSVIE);
hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
ci->role = CI_ROLE_END;
}
/* prepare for switch to host */
static int udc_id_switch_for_host(struct ci13xxx *ci)
{
ci->role = role;
hw_write(ci, OP_OTGSC, OTGSC_BSVIS, OTGSC_BSVIS);
hw_write(ci, OP_OTGSC, OTGSC_BSVIE, OTGSC_BSVIE);
}
struct ci_role_driver device {
.init = udc_start;
.start = udc_id_switch_for_device;
.stop = udc_id_switch_for_host;
.destroy = udc_stop;
};
struct ci_role_driver host {
.init = host_start;
.start = host_start;
.stop = host_stop;
.destroy = stop_stop;
};
At probe call role .init. At remove, call role .destroy
During id_switch, just call ci_role_stop/ci_role_start.
For host mode at otg port during init case, I still think we
should call device .init, as the modprobe gadget will
fail if device .init isn't called, like the "modprobe g_ether"
will fail at user's init.rc.
--
Best Regards,
Peter Chen
--
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
