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

[Janek Kotas]

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