Re: [PATCH] Fix clk->enabled counter

[David Brownell <david-b@xxxxxxxxxxx>]

On Thursday 10 January 2008, Dmitry Baryshkov wrote:
> Hi,
> 
> eric miao wrote:
> 
> > isn't it better to extract clk_{enable, disable} from udc_disable()?
> > this can be treated as two different things, one is for the clock, the
> > other controls the udc register bit, and better be divided.
> 
> Good idea. What about this patch:

Minor issues as noted below.  You tested this?  With suspend,
resume, remote wakeup, unplug-while-active, and rmmod g_<what>
scenarios to make sure the main code paths behave OK?


> Fix the pxa2xx_udc to balance calls to clk_enable/clk_disable
> 
> Signed-off-by: Dmitry Baryshkov dbaryshkov@xxxxxxxxx
> 
> diff --git a/drivers/usb/gadget/pxa2xx_udc.c b/drivers/usb/gadget/pxa2xx_udc.c
> index 2900556..f4484e4 100644
> --- a/drivers/usb/gadget/pxa2xx_udc.c
> +++ b/drivers/usb/gadget/pxa2xx_udc.c
> @@ -934,20 +934,36 @@ static void udc_disable(struct pxa2xx_udc *);
>  /* We disable the UDC -- and its 48 MHz clock -- whenever it's not
>   * in active use.
>   */
> -static int pullup(struct pxa2xx_udc *udc, int is_active)
> +static int pullup(struct pxa2xx_udc *udc)
>  {
> -	is_active = is_active && udc->vbus && udc->pullup;
> +	int is_active = udc->vbus && udc->pullup &&
> +			! udc->suspended;

That should fit neatly on one line, with no superfluous whitespace
before the "!"...


>  	DMSG("%s\n", is_active ? "active" : "inactive");
> -	if (is_active)
> -		udc_enable(udc);
> -	else {
> -		if (udc->gadget.speed != USB_SPEED_UNKNOWN) {
> -			DMSG("disconnect %s\n", udc->driver
> -				? udc->driver->driver.name
> -				: "(no driver)");
> -			stop_activity(udc, udc->driver);
> +	if (is_active) {
> +		if (!udc->active) {
> +			udc->active = 1;
> +#ifdef	CONFIG_ARCH_PXA

Adding #ifdefs ... yeech.  I suppose it's unavoidable until
the IXP processors get <linux/clk.h> support.  What's the
story on IXP support there?



> --- a/drivers/usb/gadget/pxa2xx_udc.h
> +++ b/drivers/usb/gadget/pxa2xx_udc.h
> @@ -119,13 +119,17 @@ struct pxa2xx_udc {
>  						has_cfr : 1,
>  						req_pending : 1,
>  						req_std : 1,
> -						req_config : 1;
> +						req_config : 1,
> +						suspended : 1,
> +						active : 1;
>  
>  #define start_watchdog(dev) mod_timer(&dev->timer, jiffies + (HZ/200))
>  	struct timer_list			timer;
>  
>  	struct device				*dev;
> +#ifdef	CONFIG_ARCH_PXA
>  	struct clk				*clk;
> +#endif

On the principle of not adding needless #ifdefs -- remove that one.
The cost of a pointer here is miniscule.

>  	struct pxa2xx_udc_mach_info		*mach;
>  	u64					dma_mask;
>  	struct pxa2xx_ep			ep [PXA_UDC_NUM_ENDPOINTS];
> 


-------------------------------------------------------------------------
This SF.net email is sponsored by: Microsoft
Defy all challenges. Microsoft(R) Visual Studio 2008.
http://clk.atdmt.com/MRT/go/vse0120000070mrt/direct/01/
_______________________________________________
linux-usb-devel@xxxxxxxxxxxxxxxxxxxxx
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel