On Tue, Feb 03, 2015 at 12:04:08PM +0200, Alexander Shishkin wrote:
> Peter Chen <peter.chen@xxxxxxxxxxxxx> writes:
>
> > Currently, connect gadget is unconditional after binding,
> > but some function drivers may want to connect gadget on the fly.
> > With this API, the function driver can disconnect gadget during
> > the initialization, and connect gadget when it wants.
> >
> > During this API, the deactivation count will be update, and it
> > will try to connect or disconnect gadget. It can be used to
> > enable functions for gadget when necessary.
> >
> > Signed-off-by: Peter Chen <peter.chen@xxxxxxxxxxxxx>
> > Acked-by: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> > ---
> > drivers/usb/gadget/udc/udc-core.c | 35 ++++++++++++++++++++++++++++++++++-
> > include/linux/usb/gadget.h | 5 +++++
> > 2 files changed, 39 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/usb/gadget/udc/udc-core.c b/drivers/usb/gadget/udc/udc-core.c
> > index 9396a86..a757334 100644
> > --- a/drivers/usb/gadget/udc/udc-core.c
> > +++ b/drivers/usb/gadget/udc/udc-core.c
> > @@ -37,6 +37,7 @@
> > * @list - for use by the udc class driver
> > * @vbus - for udcs who care about vbus status, this value is real vbus status;
> > * for udcs who do not care about vbus status, this value is always true
> > + * @deactivations - the deactivation count to connect or disconnect gadget
> > *
> > * This represents the internal data structure which is used by the UDC-class
> > * to hold information about udc driver and gadget together.
> > @@ -47,6 +48,7 @@ struct usb_udc {
> > struct device dev;
> > struct list_head list;
> > bool vbus;
> > + int deactivations;
> > };
> >
> > static struct class *udc_class;
> > @@ -168,13 +170,44 @@ EXPORT_SYMBOL_GPL(usb_gadget_set_state);
> >
> > static void usb_udc_connect_control(struct usb_udc *udc)
> > {
> > - if (udc->vbus)
> > + if (udc->vbus && !udc->deactivations)
> > usb_gadget_connect(udc->gadget);
> > else
> > usb_gadget_disconnect(udc->gadget);
> > }
> >
> > /**
> > + * usb_udc_activation_handler - updates udc's deactivation count and
> > + * try to connect or disconnect
> > + *
> > + * @gadget: The gadget which the function is at
> > + * @active: the function needs to be active or not
> > + *
> > + * The composite core or function driver calls it when it
> > + * wants to activate or deactivate function.
> > + */
> > +void usb_udc_activation_handler(struct usb_gadget *gadget, bool active)
> > +{
> > + struct usb_udc *udc = usb_gadget_find_udc(gadget);
> > +
>
> if (!udc)
> return;
>
> can make this function slightly easier on the eyes.
Will change
>
> > + mutex_lock(&udc_lock);
> > + if (udc) {
> > + if (active) {
> > + if (udc->deactivations == 0) {
> > + WARN_ON(1);
> > + return;
>
> This returns with the udc_lock locked.
oops, will change
> Also, you probably want WARN_ON_ONCE() like so:
>
> if (WARN_ON_ONCE(udc->deactivations)) ...
>
Will change
> Also, I don't think you want udc_lock here at all, it looks like its (at
> least intended) use is to serialize accesses to udc_list. You could use
> an atomic if you want to protect against concurrent (de-)activations,
> but then you would still need to synchronize
> usb_gadget_connect()/usb_gadget_disconnect() that result from these
> activations. I assume that normally these will serialize on gadget
> driver's spinlock in pullup(), so it should be all right.
>
I would like udc-core can make sure deactivations count change
and usb_gadget_connect/usb_gadget_disconnect calling be serial.
--
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
