Re: [PATCH 3/7] rtc: rzn1: Add new RTC driver

[Miquel Raynal <miquel.raynal@bootlin.com>]

Hi Alex,

> > +	tm->tm_sec = bcd2bin(tm->tm_sec);
> > +	tm->tm_min = bcd2bin(tm->tm_min);
> > +	tm->tm_hour = bcd2bin(tm->tm_hour);
> > +	tm->tm_wday = bcd2bin(tm->tm_wday);
> > +	tm->tm_mday = bcd2bin(tm->tm_mday);
> > +	tm->tm_mon = bcd2bin(tm->tm_mon);
> > +	tm->tm_year = bcd2bin(tm->tm_year);
> > +
> > +	dev_dbg(dev, "%d-%d-%d(%d)T%d:%d:%d\n",
> > +		tm->tm_year, tm->tm_mon, tm->tm_mday, tm->tm_wday,
> > +		tm->tm_hour, tm->tm_min, tm->tm_sec);
> > +  
> 
> This is not really useful because we have tracepoints in the core.
> Anyway, please use %ptR.

Nice printk operator :)

I've dropped this dev_dbg call, it actually does not serve any useful
purpose.


[...]

> > +static int rzn1_rtc_probe(struct platform_device *pdev)
> > +{
> > +	struct rzn1_rtc *rtc;
> > +	int ret;
> > +
> > +	rtc = devm_kzalloc(&pdev->dev, sizeof(*rtc), GFP_KERNEL);
> > +	if (!rtc)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, rtc);
> > +
> > +	rtc->clk = devm_clk_get(&pdev->dev, "hclk");
> > +	if (IS_ERR(rtc->clk))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->clk), "Missing hclk\n");
> > +
> > +	rtc->base = devm_platform_ioremap_resource(pdev, 0);
> > +	if (IS_ERR(rtc->base))
> > +		return dev_err_probe(&pdev->dev, PTR_ERR(rtc->base), "Missing reg\n");
> > +
> > +	rtc->rtcdev = devm_rtc_allocate_device(&pdev->dev);
> > +	if (IS_ERR(rtc->rtcdev))
> > +		return PTR_ERR(rtc);
> > +
> > +	rtc->rtcdev->range_max = 3178591199UL; /* 100 years */  
> 
> I'm not sure how you came to this value, this is 2070-09-22T05:59:59.
> I'm pretty sure the RTC will not fail at that time. Also, the comment
> seems fishy.

The RTC itself as no "starting point", but just a counter that can
count up to 100. So the max range is start-year + 100 years. But at
this point I don't yet have access to the start-year value. What's
your advise?

> > +	rtc->rtcdev->ops = &rzn1_rtc_ops;
> > +
> > +	ret = r9a06g032_sysctrl_enable_rtc(true);
> > +	if (ret)
> > +		return ret;
> > +
> > +	ret = clk_prepare_enable(rtc->clk);
> > +	if (ret)
> > +		goto disable_rtc;
> > +
> > +	/*
> > +	 * Ensure the clock counter is enabled.
> > +	 * Set 24-hour mode and possible oscillator offset compensation in SUBU mode.
> > +	 */
> > +	writel(RZN1_RTC_CTL0_CE | RZN1_RTC_CTL0_AMPM | RZN1_RTC_CTL0_SLSB_SUBU,
> > +	       rtc->base + RZN1_RTC_CTL0);
> > +
> > +	/* Disable all interrupts */
> > +	writel(0, rtc->base + RZN1_RTC_CTL1);
> > +
> > +	/* Enable counter operation */
> > +	writel(0, rtc->base + RZN1_RTC_CTL2);
> > +  
> 
> I don't think you should do that unconditionally. The RTC is either
> not already started (and doesn't carry the proper time/date) or already
> started. It would be better to start it in .set_time. Maybe you can even
> use that to detect whether it has already been set once.

Actually there is only one (interesting) writeable bit in this register
and it defaults to 0 whenever we enable the register interface so we
don't really need to enable anything.

But I've added the necessary logic to handle the situation where the
bootloader would disable the rtc (which is the only situation where
the driver can diagnose an erroneous time/date).

> > +	ret = devm_rtc_register_device(rtc->rtcdev);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Failed to register RTC\n");  
> 
> No error message is needed here.

Ok.

> 
> > +		goto disable_clk;
> > +	}
> > +
> > +	return 0;
> > +
> > +disable_clk:
> > +	clk_disable_unprepare(rtc->clk);
> > +disable_rtc:
> > +	r9a06g032_sysctrl_enable_rtc(false);
> > +
> > +	return ret;
> > +}
> > +
> > +static int rzn1_rtc_remove(struct platform_device *pdev)
> > +{
> > +	struct rzn1_rtc *rtc = platform_get_drvdata(pdev);
> > +
> > +	clk_disable_unprepare(rtc->clk);
> > +	r9a06g032_sysctrl_enable_rtc(false);  
> 
> Does this stop the RTC or just the register interface?

Mmmh actually if I reset the sysctrl bit manually I loose the counter
entirely, so I'll drop this call.

Thanks,
Miquèl