Hi, On 08/26/2014 08:36 AM, Sekhar Nori wrote: > On Friday 22 August 2014 01:16 PM, Dave Gerlach wrote:
Thanks for pushing that forward! >> +static int edma_pm_suspend(struct device *dev) >> +{ >> + int j, r; >> + >> + r = pm_runtime_get_sync(dev); >> + if (r < 0) { >> + dev_err(dev, "%s: get_sync returned %d\n", __func__, r); >> + return r; >> + } > > The driver currently does a pm_runtime_get_sync() once during probe. And > does not do a put(). So this should actually be not required. In fact > looks like this additional get() call will prevent the clock from > getting disabled which is probably not what you intend. Well, the pm runtime is put again ... >> + >> + for (j = 0; j < arch_num_cc; j++) { >> + struct edma *ecc = edma_cc[j]; >> + >> + disable_irq(ecc->irq_res_start); >> + disable_irq(ecc->irq_res_end); > > Do we really need to disable these irqs? > >> + } >> + >> + pm_runtime_put_sync(dev); ... here, so it's in sync and should be fine. I was also sure than when I wrote the code, disabling the interrupts during suspend was necessary, and even the only thing that has to be done at suspend time. Now that I address this again, my tests show that in can in fact be omitted. So I'll send a v9 now that has no edma_pm_suspend() at all anymore. >> +static const struct dev_pm_ops edma_pm_ops = { >> + .suspend_late = edma_pm_suspend, >> + .resume_early = edma_pm_resume, >> +}; > > You can use SET_LATE_SYSTEM_SLEEP_PM_OPS() as some other DMA drivers are > doing too. Sure, why not. Thanks, Daniel -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html