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

[Esben Haabendal]

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?

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

/Esben