Re: [PATCH v5 3/3] usb: udc: add usb_udc_activation_handler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux Media]     [Linux Input]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Old Linux USB Devel Archive]

  Powered by Linux