linux-kernel - Re: [PATCH 2/2] rtc: Add Cadence RTC driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 11 Jan 2019 10:12:13 +0000
From:   Janek Kotas <jank@...ence.com>
To:     Alexandre Belloni <alexandre.belloni@...tlin.com>
CC:     Janek Kotas <jank@...ence.com>,
        "a.zummo@...ertech.it" <a.zummo@...ertech.it>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        "linux-rtc@...r.kernel.org" <linux-rtc@...r.kernel.org>,
        "devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH 2/2] rtc: Add Cadence RTC driver

Hi Alexandre,

Thank you for your reply.

> On 10 Jan 2019, at 23:27, Alexandre Belloni <alexandre.belloni@...tlin.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