Re: [PATCH 2/2] rtc: isl12022: Add alarm support

[Alexandre Belloni]

On 12/09/2024 09:09:40+0200, Esben Haabendal wrote:
> Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> writes:
> 
> > On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
> >> Alexandre Belloni <alexandre.belloni@xxxxxxxxxxx> writes:
> >> > On 10/09/2024 12:27:11+0200, Esben Haabendal wrote:
> >> 
> >> >> +static int isl12022_rtc_read_alarm(struct device *dev,
> >> >> +				   struct rtc_wkalrm *alarm)
> >> >> +{
> >> >> +	struct rtc_time *const tm = &alarm->time;
> >> >> +	struct isl12022 *isl12022 = dev_get_drvdata(dev);
> >> >> +	struct regmap *regmap = isl12022->regmap;
> >> >> +	uint8_t buf[ISL12022_ALARM_SECTION_LEN];
> >> >> +	int ret, yr, i;
> >> >> +
> >> >> +	ret = regmap_bulk_read(regmap, ISL12022_ALARM_SECTION,
> >> >> +			       buf, sizeof(buf));
> >> >> +	if (ret) {
> >> >> +		dev_err(dev, "%s: reading ALARM registers failed\n",
> >> >> +			__func__);
> >> >
> >> > I don't really like those error messages because there is nothing the
> >> > user can actually do apart from trying again and this bloats the
> >> > kernel.
> >> 
> >> Ok. Maybe keep it as dev_dbg() then?
> >
> > This is fine, there are other I didn't point out.
> 
> Ok. I will change all of these type of error messages to dev_dbg. No problem.
> 
> >> >> +	isl12022->rtc = rtc;
> >> >>  
> >> >>  	rtc->ops = &isl12022_rtc_ops;
> >> >>  	rtc->range_min = RTC_TIMESTAMP_BEGIN_2000;
> >> >>  	rtc->range_max = RTC_TIMESTAMP_END_2099;
> >> >>  
> >> >> +	if (client->irq > 0) {
> >> >> +		ret = isl12022_setup_irq(isl12022, client->irq);
> >> >
> >> > You can't do this in probe, the RTC lifecycle is longer than the linux
> >> > system. Or said differently: "oh no, my linux has rebooted and now I
> >> > lost my future alarm" ;)
> >> 
> >> Oh.
> >> 
> >> We do need to setup the irq here, so I assume you mean I need to drop
> >> the part of _setup_irq() that clears alarm registers.
> >
> > Yes, this is the main problematic part. The other one being disabling
> > the IRQ output when in battery backup mode as this will surely prevent
> > wakeup of some devices.
> 
> I know. I did this on purpose, as I don't have a setup where I can test
> wakeup, so I thought it was better to start out without this instead of
> shipping something that is most likely broken.
> 
> If I leave IRQ output from RTC chip enabled during battery backup mode,
> I assume I have to add working suspend/resume also. Or do you just want
> me to flip the bit?

The issue is still about the lifecycle. The RTC will remember the
setting so if you change it from the default value without providing a
control, there is no way to change back the driver behavior in the
future because this is going to break a use case and there is no way to
win. So my preference is that you leave the bit to its default value.
You don't necessarily need the suspend/resume callbacks.

> 
> >> And I guess we need to enable irq in probe as well. At least if/when an
> >> alarm is set. I think it should be safe to enable irq unconditionally in
> >> _probe()...
> >
> > I guess you mean requesting the interrupt on the SoC side.
> 
> Yes.
> 
> > Enabling the RTC interrupt should be left untouched in the probe.
> 
> Ok, so if/when an alarm is already set before probe, do application need
> to enable it using RTC_AIE_ON?

If the alarm is on on boot, it must be kept on without any user
intervention.


-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com