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.
>
> Regards,
> --
> Alex
>
--
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
