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!

quoted

+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 ...

quoted

+
+	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?

quoted

+	}
+
+	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.

quoted

+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