Messages in this thread | | | From | Janek Kotas <> | Subject | Re: [PATCH 2/2] rtc: Add Cadence RTC driver | Date | Fri, 11 Jan 2019 10:12:13 +0000 |
| |
Hi Alexandre,
Thank you for your reply.
> On 10 Jan 2019, at 23:27, Alexandre Belloni <alexandre.belloni@bootlin.com> wrote: > > Hello, > > On 08/01/2019 12:22:42+0000, Jan Kotas wrote: >> drivers/rtc/Kconfig | 10 ++ >> drivers/rtc/Makefile | 1 + >> drivers/rtc/rtc-cadence.c | 404 ++++++++++++++++++++++++++++++++++++++++++++++ > > I would prefer a name that is a bit less generic, unless you can > guarantee this driver will be able to handle every RTCs from Cadence.
Yes, that’s the only IP that is being sold. It was designed a long time ago, it’s not being actively developed in terms of new features.
>> +static int cdns_rtc_alarm_irq_enable(struct device *dev, unsigned int enabled) >> +{ >> + struct cdns_rtc *crtc = dev_get_drvdata(dev); >> + >> + if (enabled) { >> + writel((CDNS_RTC_AEI_SEC | CDNS_RTC_AEI_MIN | CDNS_RTC_AEI_HOUR >> + | CDNS_RTC_AEI_DATE | CDNS_RTC_AEI_SEC), > > CDNS_RTC_AEI_SEC is used twice here.
Thanks for spotting that, I’ll fix it.
>> >> + ret = devm_request_irq(&pdev->dev, crtc->irq, >> + cdns_rtc_irq_handler, 0, >> + dev_name(&pdev->dev), &pdev->dev); >> + if (ret) { >> + dev_err(&pdev->dev, >> + "Unable to request interrupt for the device, %d\n", >> + ret); >> + goto err_disable_ref_clk; >> + } >> + > > You should use devm_rtc_allocate_device to allocate crtc->rtc_dev before > requesting the IRQ. Else, this leaves a race condition open.
Ok, I’ll fix it.
> Also, please set crtc->rtc_dev->range_min and crtc->rtc_dev->range_max > according to the fully contiguous range of time that is supported by the > RTC.
I’ll add it, thanks for pointing it out.
>> + /* Always use 24-hour mode */ >> + writel(0, crtc->regs + CDNS_RTC_HMR); >> + >> + device_init_wakeup(&pdev->dev, 1); >> + platform_set_drvdata(pdev, crtc); >> + cdns_rtc_set_enabled(crtc, true); > > Is that really necessary? I guess you could check whether it has already > been enabled to know whether the currently set time is valid so > cdns_rtc_read_time could return -EINVAL when it knows it isn't valid. > cdns_rtc_set_time will enable the RTC once the time has been set anyway.
That’s a good idea, I’ll add it.
>> +static const struct of_device_id cdns_rtc_of_match[] = { >> + { .compatible = "cdns,rtc-r109v3" }, > > Is r109v3 an IP name or a revision?
It’s a revision.
> -- > Alexandre Belloni, Bootlin > Embedded Linux and Kernel engineering
Regards, Jan
| |