On Fri, Apr 13, 2012 at 12:39 PM, ABRAHAM, KISHON VIJAY <kishon@xxxxxx> wrote:
> Hi,
>
> On Tue, Apr 10, 2012 at 9:48 PM, Felipe Balbi <balbi@xxxxxx> wrote:
>> From: Grazvydas Ignotas <notasas@xxxxxxxxx>
>>
>> ...
>> diff --git a/drivers/usb/musb/musb_core.c b/drivers/usb/musb/musb_core.c
>> index 0f8b829..2392146 100644
>> --- a/drivers/usb/musb/musb_core.c
>> +++ b/drivers/usb/musb/musb_core.c
>> @@ -1904,7 +1904,7 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>>
>> if (!musb->isr) {
>> status = -ENODEV;
>> - goto fail3;
>> + goto fail2;
>> }
>>
>> if (!musb->xceiv->io_ops) {
>> @@ -1912,6 +1912,8 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>> musb->xceiv->io_ops = &musb_ulpi_access;
>> }
>>
>> + pm_runtime_get_sync(musb->controller);
>> +
> Move this get_sync above musb_platform_init() and remove the get_sync
> in omap2430_musb_init().
The main reason I did it this way was because I originally aimed this
patch at 3.3-rc, thus wanted to minimize the diff. Doing it before
musb_platform_init() would require to rework all error handling ("goto
failX" parts) and would result in a large diff, which is not good at
-rc time.
>
>> #ifndef CONFIG_MUSB_PIO_ONLY
>> if (use_dma && dev->dma_mask) {
>> struct dma_controller *c;
>> @@ -2023,6 +2025,8 @@ musb_init_controller(struct device *dev, int nIrq, void __iomem *ctrl)
>> goto fail5;
>> #endif
>>
>> + pm_runtime_put(musb->controller);
>> +
>> dev_info(dev, "USB %s mode controller at %p using %s, IRQ %d\n",
>> ({char *s;
>> switch (musb->board_mode) {
>> @@ -2047,6 +2051,9 @@ fail4:
>> musb_gadget_cleanup(musb);
>>
>> fail3:
>> + pm_runtime_put_sync(musb->controller);
>> +
>> +fail2:
>> if (musb->irq_wake)
>> device_init_wakeup(dev, 0);
>> musb_platform_exit(musb);
>> diff --git a/drivers/usb/musb/omap2430.c b/drivers/usb/musb/omap2430.c
>> index 11b571e..3dfd122 100644
>> --- a/drivers/usb/musb/omap2430.c
>> +++ b/drivers/usb/musb/omap2430.c
>> @@ -333,6 +333,7 @@ static int omap2430_musb_init(struct musb *musb)
>>
>> setup_timer(&musb_idle_timer, musb_do_idle, (unsigned long) musb);
>>
>> + pm_runtime_put_noidle(musb->controller);
> This is not needed if you move the get_sync above musb_platform_init()
> (musb_core.c) and remove the get_sync in omap2430_musb_init().
> Though your patch doesn't do any harm, we can avoid redundant calls to
> runtime_get and runtime_put.
Right, but cross-file dependencies like that make code much harder to
follow and less future-proof. I think it's safer to have this function
do it's own get/put as it's accessing the registers, it will just
increase/decrease the reference counters in case caller already did
it. Then in case musb core is changed in future (core is used by a lot
of different devices, so it can happen), we can be sure this function
will continue to work.
--
Gražvydas
--
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
