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

[Alexandre Belloni <alexandre.belloni@bootlin.com>]

On 11/09/2024 10:20:07+0200, Esben Haabendal wrote:
> Alexandre Belloni <alexandre.belloni@bootlin.com> writes:
> 
> > Hello Esben,
> >
> > On 10/09/2024 12:27:11+0200, Esben Haabendal wrote:
> >> The ISL12022 RTC has a combined INT/fOUT pin, which can be used for alarm
> >> interrupt when frequency output is not enabled.
> >> 
> >> The device-tree bindings should ensure that interrupt and clock output is
> >> not enabled at the same time.
> >
> > Ideally, we would get a pinmuxing part in the driver to ensure this ;)
> 
> I hope we can leave this as future work :)

Sure.

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

> >
> >> +	/* The alarm doesn't store the year so get it from the rtc section */
> >> +	ret = regmap_read(regmap, ISL12022_REG_YR, &yr);
> >> +	if (ret) {
> >> +		dev_err(dev, "%s: reading YR register failed\n", __func__);
> >
> > Ditto
> 
> Ditto.
> 
> >> +	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.

> 
> 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. Enabling the
RTC interrupt should be left untouched in the probe.


> 
> >> +		if (ret)
> >> +			return ret;
> >> +	} else {
> >> +		clear_bit(RTC_FEATURE_ALARM, rtc->features);
> >> +	}
> >> +
> >>  	return devm_rtc_register_device(rtc);
> >>  }
> 
> /Esben

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