Hi Alexandre,

Thank you for your reply.

On 10 Jan 2019, at 23:27, Alexandre Belloni [off-list ref] wrote:

Hello,

On 08/01/2019 12:22:42+0000, Jan Kotas wrote:

quoted

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.

quoted

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

quoted

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

quoted

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

quoted

+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