Re: [PATCH 5/5] rtc: rtc-rc5t583: add ricoh rc5t619 RTC driver

[Andreas Kemnade]

Hi,

On Thu, 24 Oct 2019 01:17:17 +0200
Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> wrote:

> On 21/10/2019 23:13:59+0200, Andreas Kemnade wrote:
> > > > + * Based on code
> > > > + *  Copyright (C) 2012-2014 RICOH COMPANY,LTD
> > > > + *
> > > > + * Based on code
> > > > + *  Copyright (C) 2011 NVIDIA Corporation    
> > > 
> > > Based on is not useful.
> > >   
> > Well, ok, I guess 90 % of the lines are rewritten by me.
> > So I could remove all that Based on copyright notices?
> >   
> 
> Yes
> 
> > > Also what is that adjusting? If this is adding/removing clock cycles,
> > > you need to use .set_offset and .read_offset.
> > >   
> > It the moment I am just clearing it at init. Since I do not know the
> > exact meaning of the value, I am not offering it through set_offset/read_offset.
> >   
> 
> It is actually kind of an issue to clear it because it may have been set
> to a useful value. Anyway, I understand you don't know much about the
> register...
> 
Or to some nonsense. The reason I am clearing it is that the original driver
is doing that and is also exporting it via sysfs. But I could reduce it to 
clearing it at PON condition, then chances are low that there is something
important in there.

> > > > +}
> > > > +
> > > > +static int rc5t619_rtc_pon_get_clr(struct device *dev, uint8_t *pon_f)
> > > > +{
> > > > +	struct rc5t619_rtc *rtc = dev_get_drvdata(dev);
> > > > +	int err;
> > > > +	unsigned int reg_data;
> > > > +
> > > > +	err = regmap_read(rtc->rn5t618->regmap, RN5T618_RTC_CTRL2, &reg_data);
> > > > +	if (err < 0)
> > > > +		return err;
> > > > +
> > > > +	if (reg_data & CTRL2_PON) {
> > > > +		*pon_f = 1;
> > > > +		/* clear VDET PON */
> > > > +		reg_data &= ~(CTRL2_PON | CTRL2_CTC | 0x4a);	/* 0101-1011 */
> > > > +		reg_data |= 0x20;	/* 0010-0000 */    
> > > 
> > > Is it possible to have more defines for those magic values?
> > >   
> > Well, I only have some GPLed source code as documentation, no good documentation.
> > So I only know that one bit must be VDET.
> > rtc-rc5t583 seems not to be helpful to find these magic numbers.
> > 0x40 might be VDET. At least that is conform with rtc-rs5c348.c.
> > rc5t583 seems not to know a VDET.
> > But there seems to be no clear duplicate anywhere of those two
> > ctrl registers. It seems that the rc5t619 is a cross of everything, having
> > 6 byte bcd time in common with everything. 
> > So we have to keep a bit of magic here.
> >   
> 
> It would be very useful to be able to detect voltage drop. Also PON is a
> useful information that is lost when probing the driver. You
> definitively know the time is incorrect after power on and this should
> be reset only after setting the time at least once (the same for voltage
> drop).
> 
So returing -EIO in read_time and clearing things only after set_time?

Do you see tha VDET issue as a blocker for accepting this driver?
I agree it might be useful. But IMHO it is too much guesswork and hard to test.
But contrary to the other bits used with a clear name it would be pure guesswork
and hard to test.  

> > > > +		cent_flag = 1;
> > > > +	else
> > > > +		cent_flag = 0;
> > > > +
> > > > +	buff[5] = buff[5] & 0x1f;		/* bit5 19_20 */    
> > > 
> > > This assignment is unnecessary, you can mask the value when using it.
> > >   
> > ok.
> >   
> > > Is the RTC 1900-2099 or 2000-2199? Please include the ouput of rtc-range
> > > in the commit log:
> > > 
> > > https://git.kernel.org/pub/scm/linux/kernel/git/abelloni/rtc-tools.git/tree/rtc-range.c
> > >   
> > # ./rtc-range 
> > 
> > Testing 2000-02-28 23:59:59.
> > OK
> > 
> > Testing 2038-01-19 03:14:07.
> > OK
> > 
> > Testing 2069-12-31 23:59:59.
> > OK
> > 
> > Testing 2099-12-31 23:59:59.
> > KO RTC_RD_TIME returned 22 (line 138)
> > 
> > Testing 2100-02-28 23:59:59.
> > KO RTC_RD_TIME returned 22 (line 138)
> > 
> > Testing 2106-02-07 06:28:15.
> > OK
> > 
> > Testing 2262-04-11 23:47:16.
> > KO  Read back 2102-04-11 23:47:16.
> > 
> > I think it is 1900 to 2099.
> >   
> 
> It is, I'm very curious to know why it doesn't roll over properly from
> 2099-12-31 23:59:59 to 1900-01-01 00:00:00.
> 
> You can set range_min and range_max accordingly.
> 
yes, will do.

> > > > +	if (rtc->irq == -1)
> > > > +		return -EIO;    
> > > 
> > > This has to be -EINVAL to get UIE emulation working.
> > >  
> > so then ordinary hwclock still works without irq?
> >    
> 
> Yes
> 
> > > > +	/* using 24h-mode */
> > > > +	err = rc5t619_rtc_24hour_mode_set(&pdev->dev, 1);
> > > > +    
> > > 
> > > Doesn't that corrupt the time if the RTC was previously set in 12h-mode?
> > > 
> > >   
> > Probably it can. but probably that is only important in a multi-boot scenario but
> > who knows... If that is important enough I can try to implement 12h mode.
> > 
> > Is there any testing tool for 12h vs. 24h? Or should I expand
> > rtc-range.c? BTW: apparently rtc-rc5t583.c seems not to properly care
> > about 24h mode too, so I took a bad example. Well, not tested that...
> >   
> 
> There is no specific test because reading any time in the afternoon
> would fail if there is a mismatch. The correct way to handle this would
> be to support both 12h and 24h mode in read_time and always set 24h mode
> in set_time instead of setting it at probe time.
> 
I would expect undefined behavior, not necessarily a fail. Interesting
things will happen the next time the hour register is touched.
If you assume that the scenario with other users in 12h mode is important
enough to consider, then you are right, supporting both 12h and 24h in read_time
is better. About always set 24h: If there is some other user only prepared for
12h mode, there will be trouble. Also we might need to reconfigure an alarm
to the selected mode.

I guess setting 24h mode at PON and checking at every time and alarm read/write
what mode we are in. Then we do not need to reprogram alarm.

What driver is doing 24h/12h in a good way, just as a good example?

> People usually forget that the RTC is still running while Linux is not.
> So for an RTC driver, it is a bad idea to do initialization at every
> probe, unlike many other peripheral.
> 
> > > > +static int rc5t619_rtc_remove(struct platform_device *pdev)
> > > > +{
> > > > +	rc5t619_rtc_alarm_enable(&pdev->dev, 0);    
> > > 
> > > If the PMIC can be used to start the platform, you probably don't want
> > > to disable the alarm here. Even if it doesn't, is it actually useful to
> > > disable the alarm?
> > >   
> > 
> > well, I think this is not executed if you do
> > rtcwake -m off -s something
> > At least my device powers on after that.
> >   
> 
> This is waking from suspend, I was thinking powering up the platform.
> 
well, -m mem is waking from suspend
-m off is halt

man rtcwake says:

              off    ACPI state S5 (Poweroff).  This is done by calling
              '/sbin/shutdown'.  Not officially supported by ACPI, but
              it usually works.


testing
rtcwake -s 60 -m no
halt

it powers on successfully.

but
rtcwake -s 60 -m no
cd /sys/bus/platform/drivers/rc5t619-rtc
echo rc5t619-rtc >unbind
halt

does *not* power on. So only here .remove is called.

But that is just for understanding what we are doing here.
It is more important to have a common behavior across drivers.

> > Somehow I expect the driver to clean up there. e.g. rc5t583 does that, too.
> > But no strong opinion here.
> >   
> 
So it seems that I have found a driver doing something uncommon here an
an example.

> Again, the RTC is still running after a shutdown (which will run the
> .remove callback). Disabling the alarm means that it will not fire while
> linux is not running I'm not sure what the upside is. But this prevents
> the RTC from starting the system if someone uses the alarm pin to do
> that.
> 
As said disabling alarm at this place means not disabling alarm at shutdown
but think it is simply not necessary.

> Also, having an alarm set means that userspace did it intentionally. The
> driver removing the alarm is not something that is expected.
> 
Well, at least it is userspace job to disable it at shutdown if the device 
hould not wake up. And it is UI job to make clear to user what kind of alarm
is enabled.

Regards,
Andreas

Attachment: pgptVJ7zZ7H_B.pgp
Description: OpenPGP digital signature