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:
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;
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
+ /* Enable clock for USB device */
+ clk_enable(udc->clk);
+#endif
+ udc_enable(udc);
+ }
+ } else {
+ if (udc->active) {
+ if (udc->gadget.speed != USB_SPEED_UNKNOWN) {
+ DMSG("disconnect %s\n", udc->driver
+ ? udc->driver->driver.name
+ : "(no driver)");
+ stop_activity(udc, udc->driver);
+ }
+ udc_disable(udc);
+#ifdef CONFIG_ARCH_PXA
+ /* Disable clock for USB device */
+ clk_disable(udc->clk);
+#endif
+ udc->active = 0;
}
- udc_disable(udc);
+
}
return 0;
}
@@ -958,9 +974,9 @@ static int pxa2xx_udc_vbus_session(struct usb_gadget *_gadget, int is_active)
struct pxa2xx_udc *udc;
udc = container_of(_gadget, struct pxa2xx_udc, gadget);
- udc->vbus = is_active = (is_active != 0);
+ udc->vbus = (is_active != 0);
DMSG("vbus %s\n", is_active ? "supplied" : "inactive");
- pullup(udc, is_active);
+ pullup(udc);
return 0;
}
@@ -975,9 +991,8 @@ static int pxa2xx_udc_pullup(struct usb_gadget *_gadget, int is_active)
if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
return -EOPNOTSUPP;
- is_active = (is_active != 0);
- udc->pullup = is_active;
- pullup(udc, is_active);
+ udc->pullup = (is_active != 0);
+ pullup(udc);
return 0;
}
@@ -1145,11 +1160,6 @@ static void udc_disable(struct pxa2xx_udc *dev)
udc_clear_mask_UDCCR(UDCCR_UDE);
-#ifdef CONFIG_ARCH_PXA
- /* Disable clock for USB device */
- clk_disable(dev->clk);
-#endif
-
ep0_idle (dev);
dev->gadget.speed = USB_SPEED_UNKNOWN;
}
@@ -1190,11 +1200,6 @@ static void udc_enable (struct pxa2xx_udc *dev)
{
udc_clear_mask_UDCCR(UDCCR_UDE);
-#ifdef CONFIG_ARCH_PXA
- /* Enable clock for USB device */
- clk_enable(dev->clk);
-#endif
-
/* try to clear these bits before we enable the udc */
udc_ack_int_UDCCR(UDCCR_SUSIR|/*UDCCR_RSTIR|*/UDCCR_RESIR);
@@ -1285,7 +1290,7 @@ fail:
* for set_configuration as well as eventual disconnect.
*/
DMSG("registered gadget driver '%s'\n", driver->driver.name);
- pullup(dev, 1);
+ pullup(dev);
dump_state(dev);
return 0;
}
@@ -1328,7 +1333,8 @@ int usb_gadget_unregister_driver(struct usb_gadget_driver *driver)
return -EINVAL;
local_irq_disable();
- pullup(dev, 0);
+ dev->pullup = 0;
+ pullup(dev);
stop_activity(dev, driver);
local_irq_enable();
@@ -2267,7 +2273,9 @@ static int __exit pxa2xx_udc_remove(struct platform_device *pdev)
if (dev->driver)
return -EBUSY;
- udc_disable(dev);
+ dev->pullup = 0;
+ pullup(dev);
+
remove_debug_files(dev);
if (dev->got_irq) {
@@ -2318,7 +2326,8 @@ static int pxa2xx_udc_suspend(struct platform_device *dev, pm_message_t state)
if (!udc->mach->gpio_pullup && !udc->mach->udc_command)
WARN("USB host won't detect disconnect!\n");
- pullup(udc, 0);
+ udc->suspended = 1;
+ pullup(udc);
return 0;
}
@@ -2327,7 +2336,8 @@ static int pxa2xx_udc_resume(struct platform_device *dev)
{
struct pxa2xx_udc *udc = platform_get_drvdata(dev);
- pullup(udc, 1);
+ udc->suspended = 0;
+ pullup(udc);
return 0;
}
diff --git a/drivers/usb/gadget/pxa2xx_udc.h b/drivers/usb/gadget/pxa2xx_udc.h
index c08b1a2..26c0347 100644
--- 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
struct pxa2xx_udc_mach_info *mach;
u64 dma_mask;
struct pxa2xx_ep ep [PXA_UDC_NUM_ENDPOINTS];
>
> On Jan 10, 2008 4:12 AM, Dmitry Baryshkov <dbaryshkov@xxxxxxxxx> wrote:
>> Hi,
>>
>> Russell King - ARM Linux wrote:
>>
>> > On Wed, Jan 09, 2008 at 10:02:20PM +0300, Dmitry wrote:
>> >> 2008/1/9, Russell King - ARM Linux <linux@xxxxxxxxxxxxxxxx>:
>> >> > On Wed, Jan 09, 2008 at 07:36:47PM +0300, Dmitry Baryshkov wrote:
>> >> > > Fix clk->enabled counter so that drivers with unpaired
>> >> > > clock_disable calls (pxa2xx_udc :) can work.
>> >> >
>> >> > NAK. It's already been discussed why this kind of approach is a
>> >> > bad idea.
>> >> >
>> >> >
>> >> Then the pxa2xx_udc should be fixed. Because currently it's broken.
>> >
>> > Yes. It would be nice if someone who uses it could look into coming
>> > up with a tested fix.
>>
>> Yes, it was more like a todo item for me. Please consider this patch:
>>
--
With best wishes
Dmitry
-------------------------------------------------------------------------
Check out the new SourceForge.net Marketplace.
It's the best place to buy or sell services for
just about anything Open Source.
http://ad.doubleclick.net/clk;164216239;13503038;w?http://sf.net/marketplace
_______________________________________________
linux-usb-devel@xxxxxxxxxxxxxxxxxxxxx
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel
[Home]
[Video for Linux]
[Photo]
[Yosemite Forum]
[Yosemite Photos]
[Video Projectors]
[PDAs]
[Hacking TiVo]
[Linux Kernel]
[Linux SCSI]
[XFree86]
[Devices]
[Big List of Linux Books]
[Free Dating]
