Hi Alexandre,
Thank you for your reply.
> On 10 Jan 2019, at 23:27, Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> 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
