On Tuesday 26 August 2014 02:16 PM, Daniel Mack wrote:

Hi,

On 08/26/2014 08:36 AM, Sekhar Nori wrote:

quoted

On Friday 22 August 2014 01:16 PM, Dave Gerlach wrote:

Thanks for pushing that forward!

quoted

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

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.

May be I am missing something but because of the pm_runtime_get_sync()
in probe() usage count is already 1 when suspend() is called. The
pm_runtime_get_sync() in this function makes it 2 and therefore
pm_runtime_put_sync() returns immediately because the usage count is
greater that 0 after decrementing by 1. That means the module's clocks
is not disabled after suspend() is finished.

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.

Thanks!

Regards,
Sekhar