[rtc-linux] [PATCH v3 2/3] rtc: Add APM X-Gene SoC RTC driver

[Mark Brown]

On Wed, Apr 09, 2014 at 10:16:11AM -0700, Loc Ho wrote:
> >> +static int xgene_rtc_suspend(struct device *dev)

> >> +     } else {
> >> +             xgene_rtc_alarm_irq_enable(dev, 0);
> >> +             clk_disable(pdata->clk);
> >> +     }

> > Why does the driver only disable the clock over suspend rather than also
> > unpreparing it?

> This was modeled after rtc-spear.c. Is this or rtc-spear in-correct?

They're both broken, looks like a bad conversion to prepare clocks.

> >> +     if (device_may_wakeup(&pdev->dev)) {
> >> +             if (pdata->irq_wake) {
> >> +                     disable_irq_wake(irq);
> >> +                     pdata->irq_wake = 0;
> >> +             }
> >> +     } else {
> >> +             clk_enable(pdata->clk);

> > It's also not checking for errors here.

> This was also modeled after rtc-spear. I guess I can check and print
> an warning but don't believe it matter.

You should return an error if the operation failed.

> >> +             xgene_rtc_alarm_irq_enable(dev, 1);
> >> +     }

> > Won't this unconditionally enable the interrupt regardless of what the
> > setting was prior to suspend?

> This was also modeled after rtc-spear.c. Is this or rtc-spear in-correct?

I would expct they're both broken here.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20140409/5f65346d/attachment-0001.sig>