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
