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
