| Message ID | 20130315100408.GG28361@nchen-desktop (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi, On Fri, Mar 15, 2013 at 06:04:08PM +0800, Peter Chen wrote: > On Fri, Mar 15, 2013 at 09:51:29AM +0200, Felipe Balbi wrote: > > > > that's all wrong and needs to be fixed. UDC-core has to be the central > > point for all these details. We start with the easiest way (call connect > > unconditionally after gadget driver is loaded) and optimize it as we go > > (making sure that there is VBUS before connecting pullups); but we can't > > bypass UDC-core and call usb_gadget_connect() directly from UDC driver. > > I have an idea that let the gadget driver call usb_gadget_connect/disconnect, > and make a refcount for calling .pullup at usb_gadget_connect/disconnect. > Below is an initial idea for it, I think it can also cover both udc > and uvc case. > > drivers/usb/chipidea/udc.c | 6 ++---- > drivers/usb/gadget/composite.c | 6 ++++++ > include/linux/usb/gadget.h | 21 +++++++++++++++++++-- > 3 files changed, 27 insertions(+), 6 deletions(-) > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > index 81184b9..4b6b2d8 100644 > --- a/drivers/usb/chipidea/udc.c > +++ b/drivers/usb/chipidea/udc.c > @@ -1386,17 +1386,15 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) > pm_runtime_get_sync(&_gadget->dev); > hw_device_reset(ci, USBMODE_CM_DC); > hw_device_state(ci, ci->ep0out->qh.dma); > - ci13xxx_pullup(_gadget, true); > + ci->driver->connect(&ci->gadget); > dev_dbg(ci->dev, "Connected to host\n"); > } else { > - if (ci->driver) > - ci->driver->disconnect(&ci->gadget); > + ci->driver->disconnect(&ci->gadget); > hw_device_state(ci, 0); > if (ci->platdata->notify_event) > ci->platdata->notify_event(ci, > CI13XXX_CONTROLLER_STOPPED_EVENT); > _gadget_stop_activity(&ci->gadget); > - ci13xxx_pullup(_gadget, false); > pm_runtime_put_sync(&_gadget->dev); > dev_dbg(ci->dev, "Disconnected from host\n"); > } > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > index 7c821de..b7bf5ae 100644 > --- a/drivers/usb/gadget/composite.c > +++ b/drivers/usb/gadget/composite.c > @@ -1479,6 +1479,11 @@ done: > return value; > } > > +void composite_connect(struct usb_gadget *gadget) > +{ > + usb_gadget_connect(gadget); > +} > + > void composite_disconnect(struct usb_gadget *gadget) > { > struct usb_composite_dev *cdev = get_gadget_data(gadget); > @@ -1492,6 +1497,7 @@ void composite_disconnect(struct usb_gadget *gadget) > reset_config(cdev); > if (cdev->driver->disconnect) > cdev->driver->disconnect(cdev); > + usb_gadget_disconnect(gadget); > spin_unlock_irqrestore(&cdev->lock, flags); > } > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > index 7e373a2..7f7e6bf 100644 > --- a/include/linux/usb/gadget.h > +++ b/include/linux/usb/gadget.h > @@ -536,6 +536,7 @@ struct usb_gadget { > struct device dev; > unsigned out_epnum; > unsigned in_epnum; > + int connection; > }; > > static inline void set_gadget_data(struct usb_gadget *gadget, void *data) > @@ -722,9 +723,16 @@ static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) > */ > static inline int usb_gadget_connect(struct usb_gadget *gadget) > { > + int ret = 0; > if (!gadget->ops->pullup) > return -EOPNOTSUPP; > - return gadget->ops->pullup(gadget, 1); > + > + if (gadget->connection == 0) > + ret = gadget->ops->pullup(gadget, 1); > + if (!ret) > + gadget->connection ++; > + > + return ret; > } > > /** > @@ -744,9 +752,18 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget) > */ > static inline int usb_gadget_disconnect(struct usb_gadget *gadget) > { > + int ret = 0; > + > if (!gadget->ops->pullup) > return -EOPNOTSUPP; > - return gadget->ops->pullup(gadget, 0); > + > + if (gadget->connection == 1) > + ret = gadget->ops->pullup(gadget, 0); > + > + if (!ret) > + gadget->connection --; > + > + return ret; > } this might be a good idea. But we already have something similar, although it's managed at composite device level. Maybe we need to move that to the gadget layer. Still, I don't want to let UDC drivers call usb_gadget_connect()/disconnect() directly. It's easy enough for udc-core to handle that. > > > In that case, the pullup will not be called, it will check if gadget > > > module is loaded. > > > > I don't see that in your patch. > > At each udc driver, it makes sure it, maybe we can move it to usb_gaget_connect > /usb_gadget_disconnect or its abstract layer. we shouldn't add more clutter to all UDC drivers. The target has to be into concentrating generic/duplicated code in udc-core.c
On Fri, Mar 15, 2013 at 12:37:01PM +0200, Felipe Balbi wrote: > > */ > > static inline int usb_gadget_disconnect(struct usb_gadget *gadget) > > { > > + int ret = 0; > > + > > if (!gadget->ops->pullup) > > return -EOPNOTSUPP; > > - return gadget->ops->pullup(gadget, 0); > > + > > + if (gadget->connection == 1) > > + ret = gadget->ops->pullup(gadget, 0); > > + > > + if (!ret) > > + gadget->connection --; > > + > > + return ret; > > } > > this might be a good idea. But we already have something similar, > although it's managed at composite device level. Yes, I know, it should only be managed at one place at last. > Maybe we need to move > that to the gadget layer. Still, I don't want to let UDC drivers call > usb_gadget_connect()/disconnect() directly. > > It's easy enough for udc-core to handle that. Below are all cases I can think about pullup_dp 1. No vbus interrupt support udc driver, manage pullup at load/unload gadget module. 2. For vbus interrupt supported udc driver, manage pullup at vbus interrupt 3. For gadget module, like uvc, it will call pullup_dp through its app. Are there any cases? We need to design one solution to cover all cases, then, we can try to delete the duplicate pullup calling at all udc drivers, and make it be generic.
On Fri, Mar 15, 2013 at 12:37:01PM +0200, Felipe Balbi wrote: > Hi, > > On Fri, Mar 15, 2013 at 06:04:08PM +0800, Peter Chen wrote: > > On Fri, Mar 15, 2013 at 09:51:29AM +0200, Felipe Balbi wrote: > > > > > > that's all wrong and needs to be fixed. UDC-core has to be the central > > > point for all these details. We start with the easiest way (call connect > > > unconditionally after gadget driver is loaded) and optimize it as we go > > > (making sure that there is VBUS before connecting pullups); but we can't > > > bypass UDC-core and call usb_gadget_connect() directly from UDC driver. > > > > I have an idea that let the gadget driver call usb_gadget_connect/disconnect, > > and make a refcount for calling .pullup at usb_gadget_connect/disconnect. > > Below is an initial idea for it, I think it can also cover both udc > > and uvc case. > > > > drivers/usb/chipidea/udc.c | 6 ++---- > > drivers/usb/gadget/composite.c | 6 ++++++ > > include/linux/usb/gadget.h | 21 +++++++++++++++++++-- > > 3 files changed, 27 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c > > index 81184b9..4b6b2d8 100644 > > --- a/drivers/usb/chipidea/udc.c > > +++ b/drivers/usb/chipidea/udc.c > > @@ -1386,17 +1386,15 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) > > pm_runtime_get_sync(&_gadget->dev); > > hw_device_reset(ci, USBMODE_CM_DC); > > hw_device_state(ci, ci->ep0out->qh.dma); > > - ci13xxx_pullup(_gadget, true); > > + ci->driver->connect(&ci->gadget); > > dev_dbg(ci->dev, "Connected to host\n"); > > } else { > > - if (ci->driver) > > - ci->driver->disconnect(&ci->gadget); > > + ci->driver->disconnect(&ci->gadget); > > hw_device_state(ci, 0); > > if (ci->platdata->notify_event) > > ci->platdata->notify_event(ci, > > CI13XXX_CONTROLLER_STOPPED_EVENT); > > _gadget_stop_activity(&ci->gadget); > > - ci13xxx_pullup(_gadget, false); > > pm_runtime_put_sync(&_gadget->dev); > > dev_dbg(ci->dev, "Disconnected from host\n"); > > } > > diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c > > index 7c821de..b7bf5ae 100644 > > --- a/drivers/usb/gadget/composite.c > > +++ b/drivers/usb/gadget/composite.c > > @@ -1479,6 +1479,11 @@ done: > > return value; > > } > > > > +void composite_connect(struct usb_gadget *gadget) > > +{ > > + usb_gadget_connect(gadget); > > +} > > + > > void composite_disconnect(struct usb_gadget *gadget) > > { > > struct usb_composite_dev *cdev = get_gadget_data(gadget); > > @@ -1492,6 +1497,7 @@ void composite_disconnect(struct usb_gadget *gadget) > > reset_config(cdev); > > if (cdev->driver->disconnect) > > cdev->driver->disconnect(cdev); > > + usb_gadget_disconnect(gadget); > > spin_unlock_irqrestore(&cdev->lock, flags); > > } > > > > diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h > > index 7e373a2..7f7e6bf 100644 > > --- a/include/linux/usb/gadget.h > > +++ b/include/linux/usb/gadget.h > > @@ -536,6 +536,7 @@ struct usb_gadget { > > struct device dev; > > unsigned out_epnum; > > unsigned in_epnum; > > + int connection; > > }; > > > > static inline void set_gadget_data(struct usb_gadget *gadget, void *data) > > @@ -722,9 +723,16 @@ static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) > > */ > > static inline int usb_gadget_connect(struct usb_gadget *gadget) > > { > > + int ret = 0; > > if (!gadget->ops->pullup) > > return -EOPNOTSUPP; > > - return gadget->ops->pullup(gadget, 1); > > + > > + if (gadget->connection == 0) > > + ret = gadget->ops->pullup(gadget, 1); > > + if (!ret) > > + gadget->connection ++; > > + > > + return ret; > > } > > > > /** > > @@ -744,9 +752,18 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget) > > */ > > static inline int usb_gadget_disconnect(struct usb_gadget *gadget) > > { > > + int ret = 0; > > + > > if (!gadget->ops->pullup) > > return -EOPNOTSUPP; > > - return gadget->ops->pullup(gadget, 0); > > + > > + if (gadget->connection == 1) > > + ret = gadget->ops->pullup(gadget, 0); > > + > > + if (!ret) > > + gadget->connection --; > > + > > + return ret; > > } > > this might be a good idea. But we already have something similar, > although it's managed at composite device level. Maybe we need to move > that to the gadget layer. Still, I don't want to let UDC drivers call > usb_gadget_connect()/disconnect() directly. > > It's easy enough for udc-core to handle that. > Not so easy, how let udc-core know vbus interrupt? And we need to consider the udc drivers which support or not support vbus interrupt together.
diff --git a/drivers/usb/chipidea/udc.c b/drivers/usb/chipidea/udc.c index 81184b9..4b6b2d8 100644 --- a/drivers/usb/chipidea/udc.c +++ b/drivers/usb/chipidea/udc.c @@ -1386,17 +1386,15 @@ static int ci13xxx_vbus_session(struct usb_gadget *_gadget, int is_active) pm_runtime_get_sync(&_gadget->dev); hw_device_reset(ci, USBMODE_CM_DC); hw_device_state(ci, ci->ep0out->qh.dma); - ci13xxx_pullup(_gadget, true); + ci->driver->connect(&ci->gadget); dev_dbg(ci->dev, "Connected to host\n"); } else { - if (ci->driver) - ci->driver->disconnect(&ci->gadget); + ci->driver->disconnect(&ci->gadget); hw_device_state(ci, 0); if (ci->platdata->notify_event) ci->platdata->notify_event(ci, CI13XXX_CONTROLLER_STOPPED_EVENT); _gadget_stop_activity(&ci->gadget); - ci13xxx_pullup(_gadget, false); pm_runtime_put_sync(&_gadget->dev); dev_dbg(ci->dev, "Disconnected from host\n"); } diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c index 7c821de..b7bf5ae 100644 --- a/drivers/usb/gadget/composite.c +++ b/drivers/usb/gadget/composite.c @@ -1479,6 +1479,11 @@ done: return value; } +void composite_connect(struct usb_gadget *gadget) +{ + usb_gadget_connect(gadget); +} + void composite_disconnect(struct usb_gadget *gadget) { struct usb_composite_dev *cdev = get_gadget_data(gadget); @@ -1492,6 +1497,7 @@ void composite_disconnect(struct usb_gadget *gadget) reset_config(cdev); if (cdev->driver->disconnect) cdev->driver->disconnect(cdev); + usb_gadget_disconnect(gadget); spin_unlock_irqrestore(&cdev->lock, flags); } diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h index 7e373a2..7f7e6bf 100644 --- a/include/linux/usb/gadget.h +++ b/include/linux/usb/gadget.h @@ -536,6 +536,7 @@ struct usb_gadget { struct device dev; unsigned out_epnum; unsigned in_epnum; + int connection; }; static inline void set_gadget_data(struct usb_gadget *gadget, void *data) @@ -722,9 +723,16 @@ static inline int usb_gadget_vbus_disconnect(struct usb_gadget *gadget) */ static inline int usb_gadget_connect(struct usb_gadget *gadget) { + int ret = 0; if (!gadget->ops->pullup) return -EOPNOTSUPP; - return gadget->ops->pullup(gadget, 1); + + if (gadget->connection == 0) + ret = gadget->ops->pullup(gadget, 1); + if (!ret) + gadget->connection ++; + + return ret; } /** @@ -744,9 +752,18 @@ static inline int usb_gadget_connect(struct usb_gadget *gadget) */ static inline int usb_gadget_disconnect(struct usb_gadget *gadget) { + int ret = 0; + if (!gadget->ops->pullup) return -EOPNOTSUPP; - return gadget->ops->pullup(gadget, 0); + + if (gadget->connection == 1) + ret = gadget->ops->pullup(gadget, 0); + + if (!ret) + gadget->connection --; + + return ret; } > >