On Tuesday 26 August 2014 02:16 PM, Daniel Mack wrote:
> 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.
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
--
To unsubscribe from this list: send the line "unsubscribe linux-omap" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
