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;
 }
 
> > 
> > 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.

> > 
> > I am not familiar with SRP, but I think vbus is pre-condition.
> 
> hehe, google SRP.
> 
> Nah, I'll save you the trouble. SRP is a mechanism for the USB
> peripheral to ask the host to enable VBUS. This means that we can keep
> the VBUS charge pump disabled, and thus save power, until peripheral
> requests for a session.

Thanks.

> 
> -- 
> balbi



-- 

Best Regards,
Peter Chen

--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to