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

[Esben Haabendal <esben@geanix.com>]

Alexandre Belloni <alexandre.belloni@bootlin.com> writes:

> On 12/09/2024 09:09:40+0200, Esben Haabendal wrote:
>> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
>> 
>> > On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
>> >> Alexandre Belloni <alexandre.belloni@bootlin.com> 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.

Yes, sounds like the right approach.

But should I actively set FOBATB bit to the default value, or leave it
at its current value (which potentially could be non-default)?

> 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.

Sure. But do we want to check for an active alarm, and then call
enable_irq() if there is one? If not, the alarm would assert the
interrupt line, but the irq might not be raised as the handler is
disabled.

/Esben